Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wallet: background sync with just the view key [release] #8617

Open
wants to merge 1 commit into
base: release-v0.18
Choose a base branch
from
Open

wallet: background sync with just the view key [release] #8617

wants to merge 1 commit into from

Conversation

j-berman
Copy link
Collaborator

@j-berman j-berman commented Oct 19, 2022 •
edited
Loading

See #8619

This was referenced Oct 19, 2022
Open
Merged
@plowsof plowsof mentioned this pull request Oct 20, 2022
Closed
@j-berman j-berman force-pushed the background-sync branch 2 times, most recently from 7d6cb91 to 8bf1b87 Compare October 26, 2022 23:40
@SamsungGalaxyPlayer SamsungGalaxyPlayer mentioned this pull request Oct 28, 2022
Closed
@j-berman j-berman force-pushed the background-sync branch 2 times, most recently from 2fc25b8 to 772b594 Compare July 14, 2023 16:06
@j-berman j-berman changed the title wallet: background sync with just the view key [release] [WIP] wallet: background sync with just the view key [release] Sep 21, 2023
@j-berman j-berman changed the title [WIP] wallet: background sync with just the view key [release] wallet: background sync with just the view key [release] Oct 27, 2023
@plowsof plowsof mentioned this pull request Oct 28, 2023
Closed
@j-berman
wallet: background sync with just the view key
e44e8b1 
- When background syncing, the wallet wipes the spend key
from memory and processes all new transactions. The wallet saves
all receives, spends, and "plausible" spends of receives the
wallet does not know key images for.
- When background sync disabled, the wallet processes all
background synced txs and then clears the background sync cache.
- Adding "plausible" spends to the background sync cache ensures
that the wallet does not need to query the daemon to see if any
received outputs were spent while background sync was enabled.
This would harm privacy especially for users of 3rd party daemons.
- To enable the feature in the CLI wallet, the user can set
background-sync to reuse-wallet-password or
custom-background-password and the wallet automatically syncs in
the background when the wallet locks, then processes all
background synced txs when the wallet is unlocked.
- The custom-background-password option enables the user to
open a distinct background wallet that only has a view key saved
and can be opened/closed/synced separately from the main wallet.
When the main wallet opens, it processes the background wallet's
cache.
- To enable the feature in the RPC wallet, there is a new
`/setup_background_sync` endpoint.
- HW, multsig and view-only wallets cannot background sync.
rbrunner7
rbrunner7 reviewed Jun 5, 2024
View reviewed changes
src/wallet/api/wallet.cpp
setStatusError(tr("View only wallet cannot use background sync")); \
return false; \
} \
if (m_wallet->multisig()) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The master PR uses get_multisig_status().multisig_is_active instead here which is ok

rbrunner7
rbrunner7 reviewed Jun 5, 2024
View reviewed changes
src/wallet/wallet2.cpp

if (!m_persistent_rpc_client_id)
set_rpc_client_secret_key(rct::rct2sk(rct::skGen()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 lines are not in master, that's why the master PR diff does not show them as deleted, which is ok

rbrunner7
rbrunner7 reviewed Jun 5, 2024
View reviewed changes
src/wallet/wallet2.h
{
bool first_refresh_done = false;
uint64_t start_height = 0;
serializable_unordered_map<crypto::hash, background_synced_tx_t> txs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

master PR can use std::unordered_map here thanks to improvements in the serializer

rbrunner7
rbrunner7 approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@rbrunner7 rbrunner7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I systematically compared the changes in this PR with the changes in the corresponding PR to the master branch. I commented on the very few differences that all have "natural" explanations because of differences between master and release unrelated to this PR, and the differences do not lead to any change in functionality.

I can therefore approve this PR.

@MrCyjaneK
Copy link

will this get released as a part of v0.18.3.4?

@selsta
Copy link
Collaborator

selsta commented Jul 25, 2024 •
edited
Loading

@MrCyjaneK It is currently only merged to master so that it can get tested for a bit. So not yet in .4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rbrunner7 rbrunner7 rbrunner7 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@j-berman @MrCyjaneK @selsta @rbrunner7