Fix wallet lock state races - #5136
Merged
Merged
Conversation
wallet::lock cleared the password fan without holding the store mutex, so a clear landing in the middle of a rekey was overwritten by it and the wallet remained unlocked even though lock() had completed. The clear now happens in wallet_store under the same mutex as rekey and attempt_password: either it lands after the rekey and the wallet ends up locked, or before it, in which case the rekey fails cleanly.
The wallet store primitives that consume the wallet key re-unlocked the store internally and asserted on failure, while their callers gated on a separate valid_password check. A lock landing between the check and the assert aborted the node; wallet_lock concurrent with account_create or wallet_change_seed made this reachable over RPC. The primitives now take a wallet_cipher, obtainable only through unlock(), so an unauthenticated call cannot be expressed. Entry points unlock once and thread the cipher through, move and import unlock both stores up front, and change_seed reports a locked wallet through its result instead of relying on the racy is_locked pre-checks in the RPC, CLI and Qt callers. The insert_lock_race test flips the lock state while hammering inserts; against the old check-then-act pattern it crashes on the first run.
A failed password attempt overwrites the live password and locks the wallet as a side effect, but enter_password refreshed the rep keys cache only on success. The node kept voting with keys from a wallet that already reported itself locked, until the periodic reps scan rebuilt the cache. Refresh unconditionally so the cache never outlives the lock state it was built from.
Test Results for Commit e0bee69Pull Request 5136: Results Test Case Results
Last updated: 2026-08-14 21:06:28 UTC |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes wallet lock-state races by making key operations use validated ciphers and synchronizing password clearing.
Changes:
- Serializes locking against password and rekey operations.
- Propagates locked-wallet errors through RPC, CLI, and Qt.
- Adds wallet lock and representative-cache concurrency tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
nano/node/wallet.hpp |
Defines cipher-based wallet APIs. |
nano/node/wallet.cpp |
Implements synchronized locking and cipher propagation. |
nano/node/json_handler.cpp |
Handles locked seed changes. |
nano/node/cli.cpp |
Reports locked seed-change failures. |
nano/qt/qt.cpp |
Handles locked seed imports. |
nano/core_test/wallet.cpp |
Updates APIs and adds lock-race coverage. |
nano/core_test/wallets.cpp |
Tests representative-cache invalidation. |
nano/rpc_test/wallet_rpc.cpp |
Adds concurrent wallet RPC coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+4794
to
+4797
| if (auto account_result = wallet->change_seed (seed, static_cast<uint32_t> (count))) | ||
| { | ||
| nano::public_key account (wallet->change_seed (seed, static_cast<uint32_t> (count))); | ||
| rpc_l->response_l.put ("success", ""); | ||
| rpc_l->response_l.put ("last_restored_account", account.to_account ()); | ||
| rpc_l->response_l.put ("last_restored_account", account_result.value ().to_account ()); |
Contributor
Author
There was a problem hiding this comment.
Addressed in e1ec94a: change_seed now returns a change_seed_result with the last restored account and the count, both read within its own write transaction; both RPC handlers populate the response from that single result.
🤖 Generated with Claude Code
Contributor
Author
There was a problem hiding this comment.
Correction: the atomic change_seed result was dropped from this PR and will be revisited separately — the two-transaction read of restored_count remains for now. (It predates this PR.)
🤖 Generated with Claude Code
A cipher proves that some wallet was unlocked, but nothing tied it to the store that issued it: passing wallet B's cipher to wallet A's seed_set compiled and produced ciphertext A cannot decrypt. The cipher now records its issuer and every consuming primitive asserts on it in all builds, so cross-wallet use cannot write ciphertext, including a swapped cipher pair inside move or import.
lock_rekey_race flips the lock while another thread rekeys, asserting the wallet always stays locked once lock() returns. The lock samples the rekey cycle with a varying delay, since the transaction handoff otherwise phase-locks the two threads ahead of the race window. With the password clear made unsynchronized again the test fails within milliseconds once the rekey window is at production KDF scale. The concurrent lock test now accepts only the wallet locked error for failed key operations, so a torn state surfacing as a different error no longer passes.
pwojcikdev
force-pushed
the
wallet-lock-race
branch
from
August 14, 2026 18:25
b8d64b0 to
e0bee69
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Investigating a TSAN report on #5127 surfaced three related races around the wallet lock state:
wallet::lockcleared the password fan without holding the store mutex. A clear landing in the middle of arekeywas overwritten by it, leaving the wallet unlocked even thoughlock()had completed.insert_adhoc,seed_set,seed,deterministic_key,deterministic_insert) re-unlocked the store internally andrelease_asserted on failure, while their callers gated on a separatevalid_passwordcheck. A lock landing between the check and the assert aborted the node — reachable over RPC by runningwallet_lockconcurrently withaccount_createorwallet_change_seed.enter_passwordrefreshed the rep keys cache only on success, so the node kept voting with keys from a wallet that already reported itself locked.None of these can corrupt the wallet database: entries are encrypted under the wallet key, which never changes —
rekeyonly re-wraps it under the new password, and every write goes through a check-validated cipher.Changes
wallet_store::password_clear, serialized under the store mutex againstrekeyandattempt_password: either the clear lands after the rekey and the wallet ends up locked, or before it, in which case the rekey fails cleanlywallet_cipher, obtainable only throughunlock(), so an unauthenticated call cannot be expressed; therelease_asserts are gone. Entry points unlock once and thread the cipher through,moveandimportunlock both stores up front, andchange_seedreports a locked wallet through its result instead of relying on the racyis_lockedpre-checks in the RPC, CLI and Qt callersenter_passwordrefreshes the rep keys cache unconditionally so the cache never outlives the lock state it was built fromwallet::lock, which also clears cached rep keys — previously locking from the GUI left the node voting with the cached key until the next periodic reps scanTesting
wallet.insert_lock_raceflips the lock state on one thread while hammering inserts on another; validated against a temporarily reintroduced check-then-act pattern, where it crashes on the first runwallets.rep_keys_cache_failed_password_attemptfails without the unconditional refreshrpc.wallet_concurrent_lock_createhammersaccount_create,wallet_lock,wallet_change_seedandpassword_enterin concurrent batches as a handler-level consistency check🤖 Generated with Claude Code