wallet: background sync with just the view key [master] #8619
Conversation
|
Updated: ignored callbacks (like |
j-berman
force-pushed
the
background-sync-master
branch
from
7bf5053
to
f6eeeec
Compare
j-berman
force-pushed
the
background-sync-master
branch
from
f6eeeec
to
2b7dcd5
Compare
| @@ -1699,10 +1715,11 @@ namespace tools | |||
| */ | |||
| bool load_keys_buf(const std::string& keys_buf, const epee::wipeable_string& password); | |||
| bool load_keys_buf(const std::string& keys_buf, const epee::wipeable_string& password, boost::optional<crypto::chacha_key>& keys_to_encrypt); | |||
| void process_new_transaction(const crypto::hash &txid, const cryptonote::transaction& tx, const std::vector<uint64_t> &o_indices, uint64_t height, uint8_t block_version, uint64_t ts, bool miner_tx, bool pool, bool double_spend_seen, const tx_cache_data &tx_cache_data, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL); | |||
| void process_new_transaction(const crypto::hash &txid, const cryptonote::transaction& tx, const std::vector<uint64_t> &o_indices, uint64_t height, uint8_t block_version, uint64_t ts, bool miner_tx, bool pool, bool double_spend_seen, const tx_cache_data &tx_cache_data, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL, bool ignore_callbacks = false); | |||
There was a problem hiding this comment.
ignore_callbacks parameter provides the option to bypass the execution of callbacks if it is set to true.
src/wallet/wallet2.h
Outdated
| bool should_skip_block(const cryptonote::block &b, uint64_t height) const; | ||
| void process_new_blockchain_entry(const cryptonote::block& b, const cryptonote::block_complete_entry& bche, const parsed_block &parsed_block, const crypto::hash& bl_id, uint64_t height, const std::vector<tx_cache_data> &tx_cache_data, size_t tx_cache_data_offset, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL); | ||
| void detach_blockchain(uint64_t height, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL); | ||
| detached_blockchain_data detach_blockchain(uint64_t height, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL); |
There was a problem hiding this comment.
returns a detached_blockchain_data object, which contains information about the detached part of the blockchain up to the given height.
src/wallet/wallet2.h
Outdated
| bool should_skip_block(const cryptonote::block &b, uint64_t height) const; | ||
| void process_new_blockchain_entry(const cryptonote::block& b, const cryptonote::block_complete_entry& bche, const parsed_block &parsed_block, const crypto::hash& bl_id, uint64_t height, const std::vector<tx_cache_data> &tx_cache_data, size_t tx_cache_data_offset, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL); | ||
| void detach_blockchain(uint64_t height, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL); | ||
| detached_blockchain_data detach_blockchain(uint64_t height, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL); | ||
| void handle_reorg(uint64_t height, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL); |
There was a problem hiding this comment.
The purpose of the handle_reorg function is to handle a blockchain reorganization event. A blockchain reorganization occurs when a different branch of the blockchain becomes longer than the current branch, causing a switch to the new branch. When this happens, some blocks that were previously part of the blockchain may become invalidated, and their transactions must be removed from the wallet's transaction pool and reprocessed against the new branch.
handle_reorg function ensures that the wallet is synchronized with the current state of the blockchain after a reorganization event.
src/wallet/wallet2.h
Outdated
| @@ -1741,6 +1758,7 @@ namespace tools | |||
| std::vector<size_t> get_only_rct(const std::vector<size_t> &unused_dust_indices, const std::vector<size_t> &unused_transfers_indices) const; | |||
| void scan_output(const cryptonote::transaction &tx, bool miner_tx, const crypto::public_key &tx_pub_key, size_t i, tx_scan_info_t &tx_scan_info, int &num_vouts_received, std::unordered_map<cryptonote::subaddress_index, uint64_t> &tx_money_got_in_outs, std::vector<size_t> &outs, bool pool); | |||
| void trim_hashchain(); | |||
| cryptonote::block_header_response get_block_header_by_height(uint64_t height); | |||
There was a problem hiding this comment.
used to retrieve the block header data of a block at a specific height. This information can be useful for various operations such as verifying the blockchain or retrieving information about a particular block.
src/wallet/wallet2.h
Outdated
| @@ -1753,6 +1771,8 @@ namespace tools | |||
| crypto::chacha_key get_ringdb_key(); | |||
| void setup_keys(const epee::wipeable_string &password); | |||
| size_t get_transfer_details(const crypto::key_image &ki) const; | |||
| tx_entry_data get_sorted_tx_entries(const std::unordered_set<crypto::hash> &txids); | |||
There was a problem hiding this comment.
This function is typically used to retrieve transaction data from the wallet's transaction pool, which contains all unconfirmed transactions known to the wallet.
is to retrieve and sort transaction data for a given set of transaction IDs.
|
this pull request currently has a 10.5~xmr reward after being reviewed/merged https://bounties.monero.social/posts/41/10-503m-implement-viewkey-based-background-refresh-to-significantly-reduce-wallet-sync-time |
When spends have real change outputs, those txes will be cached. When using Unfortunately, this doesn't work when there is no change, because the dummy output is to a random address, not one of our own. So, there is nothing to indicate to a view-only wallet that it should pay attention to these transactions at all. The user must run |
|
I don't think this feature makes much sense for view-only wallets. View-only wallets can just keep the refresh thread alive in the background without concern. I disabled the feature for view-only wallets (you can't enable background sync mode on a view-only wallet in this PR). Also to be clear, "background sync mode" in this PR has a totally separate implementation from "view-only" mode. It's implemented like light wallets. The "background sync mode" scanner picks up all "plausible" spends (txs with ring(s) which include a prior received output as a ring member), then when the spend key loads, processes all those "plausible" txs to determine if they are spends or not. This is how it picks up 0 change spends like light wallets. I also included a more detailed comment above Separate comment: will resolve merge conflicts soon. I also think #8568 is going to be merged soon which will make the diff a lot easier to get through too. |
Nice! Thanks for the clarification. |
|
Update: I need to update this PR to ensure that when background sync is disabled, txs in the background sync cache are processed in the correct order they appear in the chain. Related to this comment on the I'm going to wait until #8568 is merged before working on this; waiting until then will simplify conflict resolution and review. |
j-berman
force-pushed
the
background-sync-master
branch
from
2b7dcd5
to
4b6c2ca
Compare
|
This PR is ready for review again. At the time of writing this comment, the feature this PR implements has a bounty of 10.503 XMR. If this PR is merged, I'll allocate 50% of the bounty to reviewers (proportions at my discretion), and 50% to @hyc for the initial idea. Disclaimer 1: the CLI and RPC wallets currently wipe secret spend key material from memory on a best effort basis with possible room for improvement; users shouldn't assume this is perfectly ironclad. CLI + RPCSome avenues worth further investigation and consideration:
Wallet API@tobtoht proposed a relatively simple idea here:
Imo it also may be worth investigating using |
j-berman
force-pushed
the
background-sync-master
branch
from
4b6c2ca
to
e300285
Compare
|
Implemented a cleaner and more sane approach when disabling background sync mode and reloading the spend key (by reading just the spend key from the wallet keys file, rather than reload the entire wallet keys file) and made tests more robust. Not planning any further changes on my end until review. |
|
Updated to make sure the wallet doesn't throw when processing the background sync cache if |
|
I started to look into this today with the goal of doing a full review within a useful time frame. |
|
I started to review and test. I tested what happens if you have a second wallet for the same keys and you spend in the second wallet while the first one is background-syncing. My expectation: The first wallet sees the change coming back normally. However, it cannot recognize the spends for sure, but adds the transaction as "plausible spend" to the background sync cache. What I see happening, if I interpret the log output correctly: Contrary to expectation, the first wallet sees everything. Here part of the log from the first wallet after it processes the transaction in question: I checked in the debugger that the spend secret key is indeed all zero in the account of the wallet object, yet still it sees the spends somehow. What do I misunderstand or overlook? |
|
In the meantime I learned more about the system, and I think I know now the answer to my own question from a few hours ago here: It sees those spends because it knows their key images. I am running now a test where the first wallet does not have them, and as expected, it does not see the spends yet while background-syncing. |
|
I noticed that scanning / watching the transaction pool makes no difference between normal and background sync, i.e. pool processing continues normally even in background sync. Is this really useful? Seems to me that if we suppress that while background syncing we have lower load on the connected daemon, less file writes, less processing, and we don't have to worry whether the peculiarities of the missing spend secret key could influence detecting pool transactions somehow and lead to unfortunate differences in behavior. |
| size_t bgs_cache_idx = m_background_sync_cache.size(); | ||
| LOG_PRINT_L2("Adding plausible spent tx " << txid << " to background sync cache (idx=" << bgs_cache_idx << ")"); | ||
| m_background_sync_cache.insert({txid, background_syncd_tx{bgs_cache_idx, tx, o_indices, height, ts, double_spend_seen}}); | ||
| } |
There was a problem hiding this comment.
Info that may help fellow reviewers and testers:
This case is not easy to hit because in most situations a transaction has either an enote going into the wallet, or the wallet knows already one of the enotes that serve as inputs for a transaction, both cases which are then handled elsewhere.
The easiest way that I found to trigger this particular case while testing "manually":
- Create a new wallet and make copies of the wallet files
- Let the first still empty wallet go into background sync
- Send a tx to the wallet address to fund, wait until unlocked
- Spend the funds using the copied wallet files, using
sweep_singleinstead of transfer to avoid change - The first wallet in background sync will go through this code as soon as the spend tx gets mined
There was a problem hiding this comment.
If you're testing with the RPC wallet, the following should trigger it as well:
- Create a new wallet
- Receive to the wallet
- Sweep all funds out of the wallet (or use
sweep_single) - Enable background sync mode via the new
/set_background_syncRPC - Hard rescan via
/rescan_blockchain- The wallet should consider the sweep tx a "plausible spent tx" when scanning
The functional tests do this.
Agree it is not and with your point that the wallet shouldn't process the pool when background sync is enabled. Will update the PR. Adding some more color to your question prior why the second wallet could still identify a spend from the first wallet: in that example you gave, the scanner picks up the change output in that tx, which the scanner doesn't need the spend key to identify. Plus, the scanner will only add a tx to the background cache one time. Thus, I wouldn't expect it to hit that "plausible spent tx" line when scanning that tx that actually has the spend. Additionally, when the wallet is not in background sync mode and scans a received output, the wallet will use the spend key to generate a key image for that output and save the output's associated key image in the wallet cache (this is why the default CLI asks for a password when it identifies a received output when scanning). Thus, the scanner is able to later identify when that output is spent even while in background sync mode, and does not need to add any "plausible" spends of that output when scanning later. As you've noticed, it's only when the wallet scans a received output while in background sync mode when I'd expect the wallet to then add "plausible" spends for that tx. This is because while in background sync mode, the wallet is unable to generate a received output's associated key image and thus can't identify when the output is spent. And final point that you picked up: if the output is spent in a tx where the user received change, the "background sync" scanner will add the tx to the background sync cache as a "received tx" and won't then need to add it again as a "plausible". |
|
I brainstormed a bit. Maybe the following is out-of-scope for this PR, but maybe not, curious what you people say: If I imagine the perfect background sync mechanism for Monero wallets, I see something that is able to survive computer / notebook / smartphone restarts. Say my machine has a Monero wallet in background sync, and as it can be a long time until I use that wallet again in a regular way, maybe weeks, there is a realistic chance that a reboot / restart will happen in the meantime, with a Windows machine probably about once a month, after updates. Can we build a really robust background sync mechanism that resumes after restart, automatically without user intervention and ist just about impossible to "kill"? After all, especially after a long period of not using a Monero wallet, background sync would be of the greatest value, and if I then find out that background sync stopped a few weeks ago because of a restart, that's not a good UX. What would be needed to build something like that? I see a need for a new low-level wallet open method, or a new boolean for the existing open method in But - of course - we have a much more fundamental problem: For opening the wallet again when resuming work after a restart, any background sync service / server process would need the wallet's password, and where would it get that from without undermining the security of the whole system that we build now with this PR? My brainstorming came up with the following possible solution:
|
|
The most significant difference with your proposed approach is that the view key would remain decrypt-able on disk forever. I had a similar line of thinking here and decided against that approach because it felt wrong to do that. If there is significant interest from others to implement the feature like that instead (an always-on background syncing gadget that does not require user input to start but can leak a wallet's entire balance on disk), then I think it would make sense to move in that direction. Your brainstormed solution I think is a viable path. Tagging @hyc, @tobtoht, @m2049r, @SamsungGalaxyPlayer, and bounty author @CryptoGrampy in case they have opinions as well. I personally think as is, this PR enables a sweep spot of security and usability for the feature:
This is also doable. I think it would make sense to implement it if/when someone indicates they would actually use it if it's there. |
Right, didn't think of that, security-wise it's a step to the worse. Maybe asking for the password when resuming background sync would be indeed doable. Your current solution definitely has a surprising simplicity, which is a big plus. |
|
I think thanks to reviewing and testing I finally get a conceptual grasp of this thing, and I can summarize 3 possible levels of background sync functionality based on this PR:
Level 1 is possible with the PR like it is as of today, with its only small and quite simple additions to the existing Monero code. Smartphone devs could check whether this is possible for an Android or iOS app like Monerujo or Cake Wallet: Open a wallet file in the foreground, go into background with the file open, stay there running for days or even weeks, all the while with, you guessed it, keeping the file open continuously. Level 2 would be possible, as far as I can see, with only 1 small addition to the PR as-is: The capability to open a wallet file into background sync mode. This would make it possible for the wallet app to send a filename and a password to that background sync service, that service to open the wallet file, wipe the password from memory right away, and then keep syncing for days or weeks. Level 3 would need more additions to the code, maybe along the lines of my earlier "brainstorming" comment here, whith the additional security trade-off of having the view key stored on disk permanently. |
|
Yep I figure this PR as is is the optimal first step to enable the feature. |
|
For Cake, we don't really care about requiring a user prompt, if I'm understanding this correctly. The idea for us is having the background sync easily run in the background as much as the OS allows, without needing a "hot" private spend key. |
j-berman
force-pushed
the
background-sync-master
branch
from
7b8cdea
to
3566c93
Compare
|
Squashed and rebased |
src/cryptonote_config.h
Outdated
| @@ -231,6 +231,7 @@ namespace config | |||
| } }; // Bender's nightmare | |||
| std::string const GENESIS_TX = "013c01ff0001ffffffffffff03029b2e4c0281c0b02e7c53291a94d1d0cbff8883f8024f5142ee494ffbbd08807121017767aafcde9be00dcfd098715ebcf7f410daebc582fda69d24a28e9d0bc890d1"; | |||
| uint32_t const GENESIS_NONCE = 10000; | |||
| std::string const BACKGROUND_WALLET_SUFFIX = ".background"; | |||
There was a problem hiding this comment.
I don't know if this string belongs here
There was a problem hiding this comment.
Moved into wallet2.cpp
src/wallet/wallet2.h
Outdated
| if ((m_background_sync_type == BackgroundSyncCustomPassword && m_is_background_wallet) || | ||
| (m_background_sync_type == BackgroundSyncReusePassword && !m_is_background_wallet)) |
There was a problem hiding this comment.
This conditional makes deserialization dependent on runtime variables, which makes it impossible to load as-is without dependencies. What I suggest is incrementing the version, then, if the version is great enough, serializing m_background_sync_data unconditionally. It makes the code much cleaner, allows the cache to be deserialized without any known context, and makes recovering from errors easier, only at the cost of a couple bytes. Since you can check the actual value of m_is_background_wallet and m_background_sync_type against the expected values, you can actually provide meaningful feedback to the user instead of "huh, wallet didn't deserialize, that sucks".
if (ver < 31)
{
m_background_sync_data = background_sync_data_t{};
return;
}
a & m_background_sync_data;
... (AT THE BOTTOM OF THE FILE)
BOOST_CLASS_VERSION(tools::wallet2, 31)
There was a problem hiding this comment.
Done in the latest.
The reason I originally did that was to enable loading a newer normal wallet with no background cache in an older wallet version, but in hindsight I think that was unnecessary.
src/wallet/wallet2.h
Outdated
| if ((m_background_sync_type == BackgroundSyncCustomPassword && m_is_background_wallet) || | ||
| (m_background_sync_type == BackgroundSyncReusePassword && !m_is_background_wallet)) | ||
| { | ||
| FIELD(m_background_sync_data) | ||
| } |
There was a problem hiding this comment.
Same comment here, except you change VERSION_FIELD.
src/wallet/wallet2.h
Outdated
| @@ -1376,6 +1460,7 @@ namespace tools | |||
| void enable_multisig(bool enable) { m_enable_multisig = enable; } | |||
| bool is_mismatched_daemon_version_allowed() const { return m_allow_mismatched_daemon_version; } | |||
| void allow_mismatched_daemon_version(bool allow_mismatch) { m_allow_mismatched_daemon_version = allow_mismatch; } | |||
| uint64_t kdf_rounds() const { return m_kdf_rounds; }; | |||
| VERSION_FIELD(0) | ||
| VARINT_FIELD(index_in_background_sync_data) | ||
|
|
||
| // prune tx; don't need to keep signature data |
There was a problem hiding this comment.
If we're not serializing the signatures, could we use a cryptonote::transaction_prefix?
There was a problem hiding this comment.
process_new_transaction processes background synced txs upon reloading the spend key and takes a cryptonote::transaction as a param; changing it to take a cryptnote::transaction_prefix instead has other undesirable effects e.g. requires changing the wallet2 callbacks as well. Which means we'd want to initialize a cryptonote::transaction from a cryptonote::transaction_prefix instead so we don't have to change process_new_transaction's function signature, which seems more complex than simply serializing the base here. I'm open to an alternative if you think there's a simpler solution here.
src/wallet/wallet2.cpp
Outdated
| m_background_sync_data.first_refresh_done = false; | ||
| m_background_sync_data.start_height = 0; | ||
| m_background_sync_data.txs.clear(); |
There was a problem hiding this comment.
| m_background_sync_data.first_refresh_done = false; | |
| m_background_sync_data.start_height = 0; | |
| m_background_sync_data.txs.clear(); | |
| m_background_sync_data = background_sync_data_t{}; |
src/wallet/wallet2.cpp
Outdated
| m_background_sync_data.first_refresh_done = false; | ||
| m_background_sync_data.start_height = 0; | ||
| m_background_sync_data.txs.clear(); |
There was a problem hiding this comment.
| m_background_sync_data.first_refresh_done = false; | |
| m_background_sync_data.start_height = 0; | |
| m_background_sync_data.txs.clear(); | |
| m_background_sync_data = background_sync_data_t{}; |
| @@ -1685,6 +1777,7 @@ namespace tools | |||
| void get_short_chain_history(std::list<crypto::hash>& ids, uint64_t granularity = 1) const; | |||
| bool clear(); | |||
| void clear_soft(bool keep_key_images=false); | |||
| void clear_user_data(); | |||
There was a problem hiding this comment.
I would put a comment here explaining to future devs what constitutes "user data" in this case, so if they add fields in the future they can know to clear them here when applicable. We know that this method is meant to clear large blobs of data pertaining to wallets that isn't useful for background scanning, but future devs won't have that context.
src/wallet/wallet2.h
Outdated
| @@ -1850,6 +1956,11 @@ namespace tools | |||
| uint64_t m_ignore_outputs_above; | |||
| uint64_t m_ignore_outputs_below; | |||
| bool m_track_uses; | |||
| bool m_is_background_wallet; | |||
| BackgroundSyncType m_background_sync_type; | |||
| bool m_background_syncing; | |||
There was a problem hiding this comment.
If this variable is being accessed across threads, it needs to be an std::atomic<bool>.
There was a problem hiding this comment.
I think the best way to get thread safe access to m_background_syncing is to let the caller handle thread safety, which I made sure to do via LOCK_REFRESH in the wallet API, using LOCK_IDLE_SCOPE in the CLI, and the RPC should be ok because the server runs in a single thread.
I think if we wanted to add more logic into wallet2 that handles thread safety surrounding m_background_syncing, then every function that reads/writes m_background_syncing would need to grab a lock, which adds a fair bit of complexity I think for little to no gain. I don't think an atomic would solve the problem here, but maybe I'm missing something. Can you expand on an example where an atomic would yield correct behavior where the current code doesn't?
| @@ -4700,6 +4896,7 @@ bool wallet2::load_keys_buf(const std::string& keys_buf, const epee::wipeable_st | |||
| m_ignore_outputs_above = MONEY_SUPPLY; | |||
| m_ignore_outputs_below = 0; | |||
| m_track_uses = false; | |||
| m_background_sync_type = BackgroundSyncOff; | |||
There was a problem hiding this comment.
should we also set m_is_background_wallet here?
There was a problem hiding this comment.
It's set above already
| @@ -6310,6 +6705,21 @@ void wallet2::store_to(const std::string &path, const epee::wipeable_string &pas | |||
| } | |||
There was a problem hiding this comment.
We should throw an error if m_is_background_wallet and !same_file since it semantically makes sense to not move just the background sync file.
There was a problem hiding this comment.
Good spot -- this made me think to also add slightly clearer THROW_WALLET_EXCEPTION_IF(m_wallet_file.empty() to both store_background_cache and store_background_keys. Those shouldn't be triggerable.
| try | ||
| { | ||
| THROW_WALLET_EXCEPTION_IF(!m_custom_background_key, error::wallet_internal_error, "Custom background key not set"); | ||
| store_background_cache(m_custom_background_key.get(), false/*do_reset_background_sync_data*/); |
There was a problem hiding this comment.
Is only overwriting the background cache the behavior we want when storing the main wallet file? I would think that we would want to store the main wallet file as-is and not overwrite the background cache so we don't lose progress when calling store_to().
There was a problem hiding this comment.
Sensitive user data from the cache + the m_cache_key have been wiped by this point (grep m_cache_key.scrub()). For all intents and purposes, it's as if the opened wallet is the background wallet at this point. As long as the main wallet processes the background cache on reopen, which it will attempt to do automatically via process_background_cache_on_open, no progress gets lost.
If you look at the test in transfer.py with the comment # Close wallet while background syncing, then reopen, the custom password test is testing this case to make sure no progress is lost.
| return WALLET_DIRECTORY + '/' + name | ||
|
|
||
| def remove_wallet_files(name): | ||
| for suffix in ['', '.keys', '.background', '.background.keys']: |
There was a problem hiding this comment.
Should .address.txt be included here?
tests/functional_tests/transfer.py
Outdated
| REUSE_PASSWORD = 'reuse-wallet-password' | ||
| CUSTOM_PASSWORD = 'custom-background-password' |
There was a problem hiding this comment.
These constants (as well as off) could be really useful to add to the Wallet class
|
After those last comments, I'm ready to approve. Awesome work @j-berman |
|
I think this PR needs to be rebased against master and refactored to remove references to the |
j-berman
force-pushed
the
background-sync-master
branch
from
c06da8c
to
de37267
Compare
j-berman
force-pushed
the
background-sync-master
branch
from
de37267
to
adb733e
Compare
|
Squashed |
j-berman
force-pushed
the
background-sync-master
branch
from
adb733e
to
9e04db1
Compare
|
Another +1 in favor of merging. I recently added this to Monerujo and had no issues, API is simple and everything works as it should. |
src/simplewallet/simplewallet.cpp
Outdated
| tools::wallet2::BackgroundSyncType background_sync_type; | ||
| if (!parse_background_sync_type(args[1], background_sync_type)) | ||
| { | ||
| fail_msg_writer() << tr("invalid option"); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This should be put before get_and_verify_password() since it would be frustrating to type your password in and THEN get a parse error.
src/simplewallet/simplewallet.cpp
Outdated
| } | ||
|
|
||
| const auto pwd_container = get_and_verify_password(); | ||
| if (pwd_container) |
There was a problem hiding this comment.
For code clarity, this if branch could be negated and styled as a guard i.e:
if (!pwd_container)
return true;
<UNINDENT EVERYTHING ELSE INSIDE>
- 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.
j-berman
force-pushed
the
background-sync-master
branch
from
067b011
to
e71c8bf
Compare
|
Squashed and rebased |
There was a problem hiding this comment.
About half a year since my last review I looked over the code again, and played around with the new commands in the CLI wallet again. Everything ok.
It's a bit difficult to keep the overview what happened in the code over that half year, with all the rebasing and changes from other PRs coming in, but at least for the most important source file wallet2.cpp I confirmed that nothing "bad" happened in the code that is relevant to this PR in the meantime that I may have overlooked in my pass over the latest version of the code.
I think the only sane way to continue with this is merging it into master now so that people start to actually use it, and find problems if there should be any still, which we can correct with new PRs.
This PR brings a set of additions to wallet2 to enable background syncing as discussed in #8082 (initially proposed by @hyc). Note: I've made significant changes to my original PR from discussion in this PR.
How it works
This PR exposes 2 options for background syncing.
Option 1: Reuse the wallet password to encrypt the background cache
Option 2: Use a custom background cache password
Option 1
A user would choose Option 1 if they do not want to grant their system perpetual access to their plaintext view key.
NOTE: Ideally this Option 1 would be a full wallet's default behavior and the user would not need to opt-in to this behavior. This PR does not make this Option 1 the wallet's default behavior in order to minimize impact in case there is an issue with its implementation. It can be made the wallet's default behavior in the future.
Option 2
A user would choose Option 2 if they want to grant their system perpetual access to their plaintext view key.
CLI
set background-sync <off|reuse-wallet-password|custom-background-password>off(the default)reuse-wallet-passwordcustom-background-passwordOpening a "background" wallet
This feature is only available for users that selected Option 2 (
custom-background-password).If the wallet's filename is
foo, then to open its corresponding background wallet, open wallet filefoo.backgroundof the same directory.Thus, a user could write a script to auto-start background sync on startup that starts the CLI
monero-wallet-cli --wallet-file <path to background wallet> --password <custom background password>.RPC
/setup_background_syncEnables syncing in the background with just a view key hot in memory.
Inputs
background_sync_type-string("off"|"reuse-wallet-password"|"custom-background-password");"reuse-wallet-password"(reuse the wallet password to encrypt the background cache);"custom-background-password"(use a custom background password to encrypt the background cache)wallet_password-stringbackground_cache_password-string; (Optional) Custom background cache password used to encrypt the background cache. This value is only necessary when thebackground_sync_typeis"custom-background-password".Outputs
None.
/start_background_syncWipes the wallet's spend key from memory and enables an already open wallet to continue syncing with just the view key hot in memory.
Inputs
None.
Outputs
None.
/stop_background_syncA background syncing wallet reloads the spend key back in memory and processes the background cache. After successful execution, the wallet can then continue syncing normally from where the background sync left off.
Inputs
wallet_password-stringseed-string; (Optional) Mnemonic phrase of the wallet.seed_offset-string; (Optional) Offset used to derive a new seed from the given mnemonic to recover a secret wallet from the mnemonic phrase.Outputs
None.
Opening a "background" wallet
If the wallet's filename is
my_foo_wallet, then to open its background wallet, simply call/open_walletwithfilenameset tomy_foo_wallet.backgroundandpasswordset to whatever the background password is set to.GUI
See monero-project/monero-gui#4050
Mobile wallets
This is how mobile wallet devs can utilize the Options from this PR should they choose to (inspired by discussions with @valldrac, @r4v3r23, and @sgp).
Option 1
setupBackgroundSync(BackgroundSync_ReusePassword, <main wallet password>)in order for the wallet to use this option.startBackgroundSync()on whatever trigger the dev wants, and continue syncing the wallet as they would a normal wallet.stopBackgroundSync(wallet_password).openWallet(path="<path>/wallet_filename", password=<main wallet password>)like normal and the wallet will process the background cache automatically.Option 2
setupBackgroundSync(BackgroundSync_CustomPassword, <main wallet password>, <custom background password>)in order for the wallet to use this option.startBackgroundSync()on whatever trigger the dev wants, and continue syncing the wallet as they would a normal wallet.stopBackgroundSync(wallet_password).openWallet(path="<path>/wallet_filename.background", password=<random custom background password saved in key store>)and sync the background wallet as they would a normal wallet.openWallet(path="<path>/wallet_filename", password=<main wallet password>)like normal and the wallet will process the background cache automatically.