Feat/background sync just with viewkey - #208
Conversation
Introduce the ability to sync the chain in the background using only the view key, keeping the spend key encrypted at rest. When background sync stops, the spend key is restored and background-synced txs are processed. Background sync (core): - account: add set_spend_key() to restore the spend key with a derived public-key check - wallet2: setup/start/stop_background_sync, background_sync_data_t cache, store/load/process_background_cache, and key re-encryption via a shared scope-leave handler - serialization: add std::unordered_map / std::map value() serializers and supporting infrastructure for the background cache - cryptonote_config: add BACKGROUND_CACHE / BACKGROUND_KEYS_FILE hashkeys - wallet_rpc_server: expose background sync + IS_BACKGROUND_WALLET / IS_BACKGROUND_SYNCING error codes - wallet2_api: BackgroundSyncType enum and setup/start/stop/isBackgroundSyncing/ isBackgroundWallet, plus checkBackgroundSync() guards on state-changing ops Wallet API enhancements: - proxy, offline toggle, default mixin, deterministic check, device reconnect, byte counters, output import/export, password getter, stop() - TransactionInfo isCoinbase()/description(), TransactionHistory setTxNote(), AddressBook setDescription() - WalletManager networkDifficulty()/setProxy() - simplewallet: `address mnew <N>` and `address one-off <major> <minor>`
…th-viewkey wallet: add background sync with view key + wallet API enhancements
Replace implicit nlohmann::json conversions with explicit typed get<T>() calls across wallet, simplewallet, wallet API, and node RPC proxy code. This keeps JSON handling compatible with stricter conversion rules.
…th-viewkey Fix explicit JSON value extraction in wallet flow
The txs_hashes field in the get_transactions RPC parameters was wrapped in
an extra pair of braces ({{"txs_hashes", hex_hashes}}), making it one level
deeper than the other object members. This caused nlohmann::json to
serialize the parameters incorrectly, so the daemon rejected every pool
transaction request with "Invalid params" ("Failed to retrieve
transactions" in the wallet log), preventing wallet synchronization.
…th-viewkey wallet2: fix malformed get_transactions params in pool tx fetch
|
@CodeRabbit review |
✅ Action performedReview finished.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds background-wallet synchronization with encrypted caches, background key files, lifecycle controls, wallet API and RPC support, CLI restrictions, serialization updates, wallet metadata APIs, and functional tests for synchronization and reorg recovery. ChangesBackground wallet synchronization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contrib/epee/include/epee/scope_leaver.h`:
- Around line 66-91: Update the call_befor_die constructor and
create_scope_leave_handler_shared to move callback parameters into their
destinations using std::move, matching scope_leave_handler_impl. Preserve the
existing shared handler behavior while allowing move-only callbacks without
unnecessary copies.
In `@src/cryptonote_basic/account.cpp`:
- Around line 156-165: Update account_base::set_spend_key to check the boolean
result of crypto::secret_key_to_public_key before comparing spend_public_key;
reject or throw on conversion failure, and only assign m_keys.m_spend_secret_key
after both conversion and derived-key validation succeed.
In `@src/serialization/serialization.h`:
- Around line 559-573: Update the wallet cache deserialization flow using
MAGIC_FIELD and VERSION_FIELD so load_wallet_cache() validates the decoded magic
against "beldex wallet cache" and the version against 2, rejecting the cache
when either value mismatches before using its contents.
In `@src/wallet/api/transaction_info.h`:
- Around line 53-58: Update TransactionHistoryImpl::refresh() to populate
m_coinbase and m_description for every created TransactionInfoImpl across
incoming, outgoing, unconfirmed, and pool transactions. Determine m_coinbase
from the transaction type using the appropriate pd.m_type comparison, and read
m_description through wallet2::get_tx_note with the transaction hash so notes
written by setTxNote are exposed. Ensure all refresh construction paths apply
the same population logic.
In `@src/wallet/api/wallet.cpp`:
- Around line 2574-2581: Guard each optional encrypted_*_value lookup in the
response parsing block before calling get<std::string>(). For
encrypted_bchat_value, encrypted_wallet_value, encrypted_belnet_value, and
encrypted_eth_addr_value, use the existing find/contains pattern from the
surrounding code and preserve "(none)" when the field is missing or empty.
In `@src/wallet/api/wallet2_api.h`:
- Line 484: Remove the public getPassword() contract from
src/wallet/api/wallet2_api.h:484-484 and remove the corresponding WalletImpl
override in src/wallet/api/wallet.h:101-101, ensuring the retained plaintext
wallet password is no longer exposed through the API.
In `@src/wallet/wallet2.cpp`:
- Around line 4900-4901: Update the background-wallet detection expression in
the account-data loading logic to use !HasParseError() && json.IsObject(),
matching the sibling loader’s successful parse check. Preserve the existing
no_spend_key assignment while ensuring valid object-form background key files
are recognized.
- Around line 4378-4382: Update get_custom_background_key to derive the
intermediate crypto::chacha_key from the supplied password using the requested
kdf_rounds before passing it to derive_cache_key; ensure neither password nor
kdf_rounds is ignored, while preserving the existing custom_background_key
assignment flow.
In `@tests/functional_tests/transfer.py`:
- Line 884: Update TransferTest.create() to persist the generated seeds on
self.seeds or a module-level constant, then replace the new test-method
references to the local seeds variable with that persisted symbol, including the
references at the indicated wallet restoration sites.
- Line 965: Update the assertions in the transfer comparison checks, including
the corresponding locations around the other reported lines, to compare the
lengths of the actual transfer collections rather than the top-level response
wrapper objects. Use the incoming-transfers collection fields from both
responses so the counts represent transfers.
- Around line 39-43: Add the missing imports in transfer.py for DeepDiff, pp,
and util_resources, using the project’s established import paths and ensuring
the DeepDiff dependency is available to the functional-test environment. Keep
diff_transfers behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f70d3be6-ffdf-4433-9bbf-42fee2048fdf
📒 Files selected for processing (32)
contrib/epee/include/epee/scope_leaver.hsrc/cryptonote_basic/account.cppsrc/cryptonote_basic/account.hsrc/cryptonote_config.hsrc/serialization/container.hsrc/serialization/serialization.hsrc/simplewallet/simplewallet.cppsrc/simplewallet/simplewallet.hsrc/wallet/api/address_book.cppsrc/wallet/api/address_book.hsrc/wallet/api/transaction_history.cppsrc/wallet/api/transaction_history.hsrc/wallet/api/transaction_info.cppsrc/wallet/api/transaction_info.hsrc/wallet/api/wallet.cppsrc/wallet/api/wallet.hsrc/wallet/api/wallet2_api.hsrc/wallet/api/wallet_manager.cppsrc/wallet/api/wallet_manager.hsrc/wallet/node_rpc_proxy.cppsrc/wallet/wallet2.cppsrc/wallet/wallet2.hsrc/wallet/wallet_errors.hsrc/wallet/wallet_rpc_server.cppsrc/wallet/wallet_rpc_server.hsrc/wallet/wallet_rpc_server_commands_defs.cppsrc/wallet/wallet_rpc_server_commands_defs.hsrc/wallet/wallet_rpc_server_error_codes.htests/functional_tests/transfer.pytests/functional_tests/wallet.pytests/unit_tests/wipeable_string.cpputils/python-rpc/framework/wallet.py
| virtual bool isCoinbase() const override; | ||
| virtual uint64_t amount() const override; | ||
| //! always 0 for incoming txes | ||
| virtual uint64_t fee() const override; | ||
| virtual uint64_t blockHeight() const override; | ||
| virtual std::string description() const override; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
m_coinbase and m_description are never populated in refresh().
The new isCoinbase() and description() methods expose m_coinbase and m_description, but TransactionHistoryImpl::refresh() (the sole place TransactionInfoImpl objects are created and populated) never sets either field. Consequently, isCoinbase() always returns false and description() always returns "".
For m_description specifically, the new setTxNote method writes a note to wallet2 via set_tx_note, but refresh() never reads it back (e.g., via wallet2::get_tx_note). The note is persisted but invisible through the API.
🐛 Proposed fix: populate fields in refresh()
In TransactionHistoryImpl::refresh(), for each transaction (incoming, outgoing, unconfirmed, pool), add:
// For incoming payments (around line 157):
+ ti->m_coinbase = pd.m_type == wallet::pay_type::miner;
+ ti->m_description = w->get_tx_note(pd.m_tx_hash);
m_history.push_back(ti);
// For confirmed outgoing (around line 207):
+ ti->m_coinbase = false;
+ ti->m_description = w->get_tx_note(hash);
m_history.push_back(ti);
// For unconfirmed outgoing (around line 239):
+ ti->m_coinbase = false;
+ ti->m_description = w->get_tx_note(hash);
m_history.push_back(ti);
// For pool payments (around line 267):
+ ti->m_coinbase = pd.m_type == wallet::pay_type::miner;
+ ti->m_description = w->get_tx_note(pd.m_tx_hash);
m_history.push_back(ti);Note: Verify that wallet2::get_tx_note exists and accepts crypto::hash. If the coinbase determination logic differs, adjust the pd.m_type comparison accordingly.
Also applies to: 81-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wallet/api/transaction_info.h` around lines 53 - 58, Update
TransactionHistoryImpl::refresh() to populate m_coinbase and m_description for
every created TransactionInfoImpl across incoming, outgoing, unconfirmed, and
pool transactions. Determine m_coinbase from the transaction type using the
appropriate pd.m_type comparison, and read m_description through
wallet2::get_tx_note with the transaction hash so notes written by setTxNote are
exposed. Ensure all refresh construction paths apply the same population logic.
Fixed several wallet and functional test issues by guarding optional JSON field access, correcting background key derivation and background-wallet detection logic, and tightening functional-test transfer assertions/imports.
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/wallet/wallet2.h (1)
1-1: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMissing fields in
BEGIN_SERIALIZE_OBJECTcause data loss during cache load. These four serialization blocks omit fields that are correctly serialized in theirboost::serializationequivalents. When the wallet cache is saved/loaded via epee binary serialization, these critical transaction properties will be lost or default-initialized.
src/wallet/wallet2.h#L454-470: addFIELD(m_unlock_times)andVARINT_FIELD(m_pay_type)toconfirmed_transfer_details.src/wallet/wallet2.h#L419-435: addVARINT_FIELD(m_pay_type)tounconfirmed_transfer_details.src/wallet/wallet2.h#L366-378: addVARINT_FIELD(m_type)topayment_details.src/wallet/wallet2.h#L385-389: addpayment_details::serialize(a, ver);(or serialize base fields manually) toaddress_txto prevent losing the entire basepayment_detailsstate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet/wallet2.h` at line 1, Update the BEGIN_SERIALIZE_OBJECT blocks for confirmed_transfer_details, unconfirmed_transfer_details, payment_details, and address_tx to match their boost::serialization counterparts: include m_unlock_times and m_pay_type, m_pay_type, and m_type respectively, and invoke payment_details::serialize(a, ver) from address_tx so base fields are preserved during cache serialization.src/wallet/wallet2.cpp (3)
3988-3989: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant background sync state update.
detach_blockchain(called on line 3986) already performs this exact check and assignment. This code is redundant and can be removed.♻️ Proposed refactor
detached_blockchain_data dbd = detach_blockchain(height, output_tracker_cache); - - if (m_background_syncing && height < m_background_sync_data.start_height) - m_background_sync_data.start_height = height; if (m_callback)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet/wallet2.cpp` around lines 3988 - 3989, Remove the redundant m_background_syncing/start_height conditional immediately after the detach_blockchain call in detach_blockchain’s surrounding flow. Rely on detach_blockchain to perform this check and assignment, leaving the surrounding blockchain-detachment behavior unchanged.
4111-4112: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winFix incorrect locking of the background keys file.
When
background_keys_fileis true, this unconditionally locks the main keys file (m_keys_file) instead of the background keys file, leaving the background keys file unlocked and potentially causing deadlocks or redundant locks on the main file.🐛 Proposed fix
fs::rename(tmp_file_name, keys_file_name, e); - lock_keys_file(); + if (!background_keys_file) + lock_keys_file(); + else + lock_background_keys_file(keys_file_name.string()); if (e) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet/wallet2.cpp` around lines 4111 - 4112, Update the locking step following fs::rename in the wallet key-file handling flow to lock the file selected by background_keys_file: use the background keys-file lock when true and the main keys-file lock otherwise. Preserve the existing rename behavior and avoid unconditionally calling the main-file lock.
4058-4060: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winClear
m_additional_tx_keysto ensure all sensitive user data is wiped.
clear_user_dataclearsm_tx_keysbut missesm_additional_tx_keys. If this is not cleared, the background wallet will retain additional sensitive transaction keys in memory and serialize them into the background cache file, violating the intended isolation.🛡️ Proposed fix
for (auto i = m_transfers.begin(); i != m_transfers.end(); ++i) i->m_frozen = false; m_tx_keys.clear(); + m_additional_tx_keys.clear(); m_tx_notes.clear();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet/wallet2.cpp` around lines 4058 - 4060, Update clear_user_data to also clear m_additional_tx_keys alongside m_tx_keys and m_tx_notes, ensuring additional transaction keys are removed from memory before any background cache serialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/functional_tests/transfer.py`:
- Around line 88-90: Update the wallet cleanup block around
self.wallet[i].close_wallet() to use a multiline try/except structure and catch
Exception explicitly instead of a bare except, preserving the current behavior
of ignoring cleanup failures.
---
Outside diff comments:
In `@src/wallet/wallet2.cpp`:
- Around line 3988-3989: Remove the redundant m_background_syncing/start_height
conditional immediately after the detach_blockchain call in detach_blockchain’s
surrounding flow. Rely on detach_blockchain to perform this check and
assignment, leaving the surrounding blockchain-detachment behavior unchanged.
- Around line 4111-4112: Update the locking step following fs::rename in the
wallet key-file handling flow to lock the file selected by background_keys_file:
use the background keys-file lock when true and the main keys-file lock
otherwise. Preserve the existing rename behavior and avoid unconditionally
calling the main-file lock.
- Around line 4058-4060: Update clear_user_data to also clear
m_additional_tx_keys alongside m_tx_keys and m_tx_notes, ensuring additional
transaction keys are removed from memory before any background cache
serialization.
In `@src/wallet/wallet2.h`:
- Line 1: Update the BEGIN_SERIALIZE_OBJECT blocks for
confirmed_transfer_details, unconfirmed_transfer_details, payment_details, and
address_tx to match their boost::serialization counterparts: include
m_unlock_times and m_pay_type, m_pay_type, and m_type respectively, and invoke
payment_details::serialize(a, ver) from address_tx so base fields are preserved
during cache serialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 676842e8-110e-407e-a169-8abbcaac0cd5
📒 Files selected for processing (9)
contrib/epee/include/epee/scope_leaver.hsrc/cryptonote_basic/account.cppsrc/wallet/api/transaction_history.cppsrc/wallet/api/wallet.cppsrc/wallet/wallet2.cppsrc/wallet/wallet2.htests/functional_tests/deepdiff.pytests/functional_tests/transfer.pytests/functional_tests/util_resources.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wallet/wallet2.cpp`:
- Around line 4110-4113: Enclose both branches of the if/else statement in
braces: wrap the lock_background_keys_file call under the background_keys_file
condition and the lock_keys_file call under the else branch, without changing
their behavior.
In `@tests/functional_tests/transfer.py`:
- Around line 89-92: In the wallet cleanup loop, replace the broad try/except
around self.wallet[i].close_wallet() with contextlib.suppress(Exception), and
add the contextlib import if absent. Preserve the current behavior of ignoring
cleanup exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 87ce30e3-272b-4a59-8f6b-cbb636fa5fb0
📒 Files selected for processing (4)
Dockerfilesrc/wallet/wallet2.cppsrc/wallet/wallet2.htests/functional_tests/transfer.py
| if (background_keys_file) | ||
| lock_background_keys_file(keys_file_name.string()); | ||
| else | ||
| lock_keys_file(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Enclose statements in braces.
Add braces around the if/else block to resolve the linter warning and align with standard coding practices.
♻️ Proposed refactor
- if (background_keys_file)
- lock_background_keys_file(keys_file_name.string());
- else
- lock_keys_file();
+ if (background_keys_file) {
+ lock_background_keys_file(keys_file_name.string());
+ } else {
+ lock_keys_file();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (background_keys_file) | |
| lock_background_keys_file(keys_file_name.string()); | |
| else | |
| lock_keys_file(); | |
| if (background_keys_file) { | |
| lock_background_keys_file(keys_file_name.string()); | |
| } else { | |
| lock_keys_file(); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 4110-4110: statement should be inside braces
(readability-braces-around-statements)
[warning] 4112-4112: statement should be inside braces
(readability-braces-around-statements)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wallet/wallet2.cpp` around lines 4110 - 4113, Enclose both branches of
the if/else statement in braces: wrap the lock_background_keys_file call under
the background_keys_file condition and the lock_keys_file call under the else
branch, without changing their behavior.
Source: Linters/SAST tools
| try: | ||
| self.wallet[i].close_wallet() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use contextlib.suppress to handle the ignored exception cleanly.
This resolves linter warnings and provides a more idiomatic approach to suppressing expected exceptions during cleanup.
♻️ Proposed refactor
- try:
- self.wallet[i].close_wallet()
- except Exception:
- pass
+ with contextlib.suppress(Exception):
+ self.wallet[i].close_wallet()Make sure to add import contextlib at the top of the file if it's not already imported.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| self.wallet[i].close_wallet() | |
| except Exception: | |
| pass | |
| with contextlib.suppress(Exception): | |
| self.wallet[i].close_wallet() |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 89-92: Use contextlib.suppress(Exception) instead of try-except-pass
Replace try-except-pass with with contextlib.suppress(Exception): ...
(SIM105)
[error] 91-92: try-except-pass detected, consider logging the exception
(S110)
[warning] 91-91: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/functional_tests/transfer.py` around lines 89 - 92, In the wallet
cleanup loop, replace the broad try/except around self.wallet[i].close_wallet()
with contextlib.suppress(Exception), and add the contextlib import if absent.
Preserve the current behavior of ignoring cleanup exceptions.
Source: Linters/SAST tools
4 participants
wallet: add background sync with view key + wallet API enhancements
Introduce the ability to sync the chain in the background using only the
view key, keeping the spend key encrypted at rest. When background sync
stops, the spend key is restored and background-synced txs are processed.
Background sync (core):
public-key check
store/load/process_background_cache, and key re-encryption via a
shared scope-leave handler
supporting infrastructure for the background cache
IS_BACKGROUND_SYNCING error codes
isBackgroundWallet, plus checkBackgroundSync() guards on state-changing ops
Wallet API enhancements:
reconnect, byte counters, output import/export, password getter, stop()
AddressBook setDescription()
address mnew <N>andaddress one-off <major> <minor>wallet2: fix malformed get_transactions params in pool tx fetch
The txs_hashes field in the get_transactions RPC parameters was wrapped in
an extra pair of braces ({{"txs_hashes", hex_hashes}}), making it one level
deeper than the other object members. This caused nlohmann::json to
serialize the parameters incorrectly, so the daemon rejected every pool
transaction request with "Invalid params" ("Failed to retrieve
transactions" in the wallet log), preventing wallet synchronization.
Fix explicit JSON value extraction in wallet flow
Replace implicit nlohmann::json conversions with explicit typed get()
calls across wallet, simplewallet, wallet API, and node RPC proxy code.
This keeps JSON handling compatible with stricter conversion rules.