fix: improve wallet encryption robustness #6945
Conversation
Add defensive checks to prevent double-encryption of HD chains and improve wallet encryption robustness: - Check IsCrypted() status to prevent re-encryption attempts - Add TOCTOU protection by re-checking IsCrypted() under lock - Validate decryption keys before TopUp operations - Only decrypt HD chains when actually encrypted - Remove irrelevant HasEncryptionKeys() checks (keys passed as parameters) - Log warning when db rewrite fails after encryption
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThe changes introduce defensive pre-checks for HD chain cryptographic state across wallet key management functions. In Sequence DiagramsequenceDiagram
participant Caller
participant EncryptWallet
participant ScriptPubKeyMan
participant HDChain
Caller->>EncryptWallet: Initiate wallet encryption
EncryptWallet->>EncryptWallet: Check if already encrypted (NEW GUARD)
alt Already encrypted
EncryptWallet-->>Caller: Return false
else Not encrypted
EncryptWallet->>ScriptPubKeyMan: CheckDecryptionKey(vMasterKey) (NEW CHECK)
alt Invalid key
ScriptPubKeyMan-->>EncryptWallet: Return false
EncryptWallet-->>Caller: Return false
else Valid key
EncryptWallet->>ScriptPubKeyMan: TopUp
EncryptWallet->>EncryptWallet: Rewrite (NEW SUCCESS CHECK)
alt Rewrite fails
EncryptWallet->>EncryptWallet: Log warning
EncryptWallet-->>Caller: Return false
else Rewrite succeeds
EncryptWallet->>ScriptPubKeyMan: EncryptKeys
EncryptWallet-->>Caller: Return true
end
end
end
sequenceDiagram
participant Caller
participant ScriptPubKeyMan
participant HDChain as HD Chain State
Caller->>ScriptPubKeyMan: Encrypt/Decrypt/Derive operation
alt GetDecryptedHDChain
ScriptPubKeyMan->>HDChain: Get current chain
alt Chain is non-null AND crypted (NEW GUARD)
ScriptPubKeyMan->>HDChain: Decrypt
HDChain-->>ScriptPubKeyMan: Decrypted chain
else Chain is null OR not crypted
ScriptPubKeyMan->>ScriptPubKeyMan: Skip decryption (NEW BRANCH)
ScriptPubKeyMan-->>ScriptPubKeyMan: Return as-is
end
end
alt EncryptHDChain
ScriptPubKeyMan->>HDChain: Check encryption state
alt Already crypted (NEW GUARD)
ScriptPubKeyMan-->>Caller: Return false
else Not crypted
ScriptPubKeyMan->>HDChain: Perform encryption
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 1
🧹 Nitpick comments (1)
src/wallet/wallet.cpp (1)
792-795: Rewrite failure only logs; consider surfacing to caller or setting a persistent warning flagIf Rewrite() fails, we log but proceed. Consider returning false or setting a wallet flag that surfaces in RPC/UI so operators can act, and covering this path in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/wallet/scriptpubkeyman.cpp(6 hunks)src/wallet/wallet.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/wallet.cppsrc/wallet/scriptpubkeyman.cpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/wallet/wallet.cppsrc/wallet/scriptpubkeyman.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/wallet/wallet.cppsrc/wallet/scriptpubkeyman.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/wallet/scriptpubkeyman.cpp
🧬 Code graph analysis (1)
src/wallet/wallet.cpp (3)
src/wallet/hdchain.cpp (2)
IsCrypted(37-41)IsCrypted(37-37)src/wallet/wallet.h (1)
IsCrypted(463-502)src/wallet/interfaces.cpp (1)
spk_man(214-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (7)
src/wallet/wallet.cpp (1)
723-728: Good TOCTOU guard on encryption stateRe-checking IsCrypted() under cs_wallet closes the race window. LGTM.
src/wallet/scriptpubkeyman.cpp (6)
250-254: Prevent double-encrypting HD chainEarly-return when hdChainCurrent is already crypted is correct and matches the PR goal.
348-354: Only decrypt when cryptedThe additional guard avoids unnecessary DecryptHDChain calls.
485-491: GetDecryptedHDChain: skip decryption unless neededConditional decrypt keeps behavior tight and avoids redundant work.
506-508: EncryptHDChain: idempotence guardReturning false if already crypted prevents accidental re-encryption attempts.
1177-1183: GetKey: decrypt HD chain only when cryptedThe guard reduces unnecessary decryption attempts and aligns with robustness goals.
1294-1300: DeriveNewChildKey: conditional decryptionCorrectly avoids decrypting unencrypted chains.
1 participant
Issue being fixed or feature implemented
Add defensive checks to prevent double-encryption of HD chains and improve wallet encryption robustness:
What was done?
How Has This Been Tested?
run tests
Breaking Changes
n/a
Checklist: