fix: HD chain encryption check ordering issue

[UdjinM6]

Issue being fixed or feature implemented

Previously, LoadHDChain() would fail if CRYPTED_HDCHAIN records were
read before MASTER_KEY records during wallet loading, because the
check m_storage.HasEncryptionKeys() != chain.IsCrypted() would
incorrectly fail when mapMasterKeys was still empty.

This PR fixes the issue by:

• Adding an optional fSkipEncryptionCheck parameter to LoadHDChain()
• Skipping the encryption check during wallet loading in ReadKeyValue()
• Adding comprehensive validation in LoadWallet() after all records
  are loaded to ensure HD chain encryption consistency

What was done?

How Has This Been Tested?

run tests

Breaking Changes

n/a

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

Walkthrough

The changes refactor HD chain loading validation timing by introducing a skip_encryption_check parameter to LegacyScriptPubKeyMan::LoadHDChain. During wallet loading in walletdb.cpp, the HD chain is loaded with the check skipped, and a new post-load validation is added after the wallet data is fully initialized. This validation ensures encryption state consistency between the wallet and HD chain, deferring the check from the initial load to after complete wallet setup.

Sequence Diagram

sequenceDiagram
    participant LoadWallet
    participant LoadHDChain
    participant HDChainValidation
    
    LoadWallet->>LoadHDChain: LoadHDChain(chain, skip_encryption_check=true)
    Note over LoadHDChain: Skips encryption consistency check
    LoadHDChain-->>LoadWallet: Return true
    
    Note over LoadWallet: Complete wallet data loading...
    
    rect rgb(230, 245, 250)
        Note over LoadWallet: Post-load validation phase
        LoadWallet->>HDChainValidation: Check encryption state<br/>storage.HasEncryptionKeys() vs<br/>chain.IsCrypted()
        alt Mismatch detected
            HDChainValidation-->>LoadWallet: Return CORRUPT
        else State consistent
            HDChainValidation-->>LoadWallet: Proceed normally
        end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• src/wallet/scriptpubkeyman.h: Signature change with new optional parameter requiring understanding of backward compatibility
• src/wallet/scriptpubkeyman.cpp: Conditional logic added around the encryption consistency check; reviewer should verify the logic preserves existing behavior when skip_encryption_check=false
• src/wallet/walletdb.cpp: New validation logic added post-load; reviewer should confirm the validation correctly mirrors the skipped check and error handling path returns appropriate wallet status (CORRUPT on mismatch)
• Key attention areas: Ensure the deferred validation in walletdb.cpp covers all encryption inconsistency scenarios previously caught during initial load; verify error message updates are complete

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing an ordering issue with HD chain encryption checks during wallet loading.
Description check	✅ Passed	The description is directly related to the changeset, explaining the issue being fixed, the specific changes made to LoadHDChain(), and the validation approach.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 98600eb and 35dc258.

📒 Files selected for processing (3)

• src/wallet/scriptpubkeyman.cpp (1 hunks)
• src/wallet/scriptpubkeyman.h (1 hunks)
• src/wallet/walletdb.cpp (2 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/scriptpubkeyman.h
• src/wallet/scriptpubkeyman.cpp
• src/wallet/walletdb.cpp

🧠 Learnings (3)

📓 Common learnings

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.

📚 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/scriptpubkeyman.h
• src/wallet/scriptpubkeyman.cpp
• src/wallet/walletdb.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/walletdb.cpp

🧬 Code graph analysis (2)

src/wallet/scriptpubkeyman.h (1)

src/wallet/scriptpubkeyman.cpp (2)

• LoadHDChain (430-438)
• LoadHDChain (430-430)

src/wallet/walletdb.cpp (1)

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: arm-linux-build / Build source
• GitHub Check: linux64_ubsan-build / Build source
• GitHub Check: linux64_sqlite-build / Build source
• GitHub Check: linux64-build / Build source
• GitHub Check: linux64_fuzz-build / Build source
• GitHub Check: mac-build / Build source
• GitHub Check: win64-build / Build source
• GitHub Check: linux64_nowallet-build / Build source
• GitHub Check: linux64_tsan-build / Build source
• GitHub Check: Lint / Run linters

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[PastaPastaPasta]

utACK 35dc258