Repository navigation
fix: validate descriptor owners and mnemonic keys when loading wallets - #7823
PastaPastaPasta wants to merge 2 commits into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 23 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (3)
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 |
|
✅ Final review complete — no blockers (commit fdcff7c) · triage: low · Phase 2 only (queue backlog) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The exact reviewed head a950add safely limits descriptor record installation to compatible owners and reports inconsistent plaintext mnemonic/key associations as wallet corruption; no blocking defect or required code prerequisite was found. One non-blocking documentation suggestion remains for these user-visible loading changes. Verification was static: no builds or tests were run locally; the supplied CI snapshot shows successful container builds and formatting checks, two queued manifest jobs, and no completed unit or functional test checks.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The production changes are small and contained wallet-loading checks and a failed-key-installation return path, with straightforward regression coverage rather than intricate changes to key handling. - Phase 1 reviewers: not run (skipped for throughput: 13 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/walletdb.cpp`:
- [SUGGESTION] src/wallet/walletdb.cpp:1008-1013: Adapt the upstream wallet-loading release note to the selected behavior
This PR changes operator-visible wallet loading: inconsistent plaintext mnemonic/key associations now report wallet corruption instead of asserting, failed descriptor key installation also returns corruption, and records without a compatible descriptor owner are skipped. The changed range contains no release note, so these outcomes are documented only in the PR description. Add a narrowly scoped Dash wallet release note describing the actual changes and unchanged recovery-format support. Do not copy upstream #24914's broader warning about corrupted address-book entries or recommend downgrading for mnemonic mismatches: those upstream policies are outside this adaptation, and the previous Dash path asserted on mismatched mnemonic/key associations.
| if (!spk_man->AddKey(desc_key_pair.first.second, desc_key_pair.second, "", "")) return DBErrors::CORRUPT; | ||
| } else { | ||
| ((DescriptorScriptPubKeyMan*)spk_man)->AddKey(desc_key_pair.first.second, desc_key_pair.second, it->second.first, it->second.second); | ||
| if (!spk_man->AddKey(desc_key_pair.first.second, desc_key_pair.second, it->second.first, it->second.second)) { | ||
| pwallet->WalletLogPrintf("Error reading wallet database: descriptor mnemonic does not match private key\n"); | ||
| return DBErrors::CORRUPT; | ||
| } |
There was a problem hiding this comment.
⏭️ Deferred (maintainer decision; not blocking) at
a950add8; see the replies below.
🟡 Suggestion: Adapt the upstream wallet-loading release note to the selected behavior
This PR changes operator-visible wallet loading: inconsistent plaintext mnemonic/key associations now report wallet corruption instead of asserting, failed descriptor key installation also returns corruption, and records without a compatible descriptor owner are skipped. The changed range contains no release note, so these outcomes are documented only in the PR description. Add a narrowly scoped Dash wallet release note describing the actual changes and unchanged recovery-format support. Do not copy upstream bitcoin#24914's broader warning about corrupted address-book entries or recommend downgrading for mnemonic mismatches: those upstream policies are outside this adaptation, and the previous Dash path asserted on mismatched mnemonic/key associations.
source: gpt-6.1-sol (phase2-reviewer: general, backport-reviewer, dash-core-commit-history)
There was a problem hiding this comment.
Not adding a release note here. Every outcome this changes applies to a wallet file that is already inconsistent: a descriptor record with no matching owner, a failed key install, or a stored mnemonic that doesn't derive the stored key. Before, those files aborted dashd. Now they either load with the orphan record ignored or fail with the existing "wallet corrupted" load error. Operators have no new option, RPC, file format, or migration to act on, and recovery-format support is unchanged. A crash-to-error fix like this belongs in the aggregated release notes at release time, not in a per-PR doc/release-notes-*.md. The PR description documents the exact behavior if the release manager wants to include it.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
I accept leaving this for release-time aggregation rather than requiring a per-PR note: the diff changes inconsistent-wallet handling without introducing a new format, option, or recovery requirement.
Noted as intentionally deferred; I will not press it further here.
thepastaclaw
added
the
pastaclaw:commented
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review after discussion — commit a950add
Standing review was COMMENTED; this re-review is APPROVE: no blocking findings remain.
Approved: the discussion withdrew or deferred every finding the full review of a950add8 (run 3816) raised, and the approval rests on that review.
Same commit as the standing review; the inline threads above carry the per-finding outcome. This follow-up exists only to correct the verdict.
Review provenance
Source: conversation lane gpt-6.1-sol (agent: conversation); no reviewer or verifier lanes ran for this follow-up
- Verdict moved because the inline discussion withdrew or deferred findings of the standing review of this commit; the code was not re-reviewed
thepastaclaw
added
pastaclaw:approved
Install descriptor caches and keys only into a descriptor manager, matching upstream owner-scoped loading. Preserve Dash whole-wallet scanning instead of importing the full prefix-loader refactor and Bitcoin bitcoin#27790 prerequisites. Check key installation results before rewriting or upgrading a wallet. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
a950add to
fdcff7c
Compare
thepastaclaw
removed
the
pastaclaw:approved
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
The complete diff at fdcff7c contains no actionable in-scope defects: descriptor records are attached only to compatible managers, and mnemonic/key mismatches return corruption before installation or database upgrades. The release-note suggestion duplicates an explicitly deferred documentation item, not a missing prerequisite for the selected upstream adaptation. Validation was static only; the supplied CI snapshot shows formatting and merge checks passing, while build-related checks remain queued, and author-reported tests were not independently rerun.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff makes small, contained wallet-loading changes to validate descriptor owner types and propagate key-installation failures, with straightforward regression coverage rather than intricate changes to key handling. - Phase 1 reviewers: not run (skipped for throughput: 16 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
2 participants
Issue being fixed or feature implemented
Descriptor database records can reference a missing descriptor manager, or collide with a legacy manager ID. Installing these records through unchecked casts can abort wallet loading or use an incompatible manager. Separately, a stored mnemonic/passphrase that derives a different master key reaches a production assertion when the descriptor key is installed. These are operator-selected wallet-file failure paths, not peer input.
What was done?
Adapt the owner-scoped descriptor loading behavior of Bitcoin PR bitcoin#24914 (
405b4d914712b5de3b230a0e2960e89f6a0a2b2a) to Dash's current loader: attach cache, plaintext-key, and encrypted-key records only to an actual descriptor manager. Ignore records without a matching descriptor owner, as upstream's per-known-descriptor prefix loading does. Reject failed key installation as wallet corruption.This is a proportionate behavioral adaptation, not a full backport of bitcoin#24914's record-loader refactor. The remaining per-record loaders, global scan restructuring, error-classification changes and tests are omitted; the prefix-cursor prerequisite Bitcoin bitcoin#27790 is not introduced because this change retains Dash's existing scan. Adapted loader regressions cover the selected owner behavior. This does not claim that every corruption policy from upstream is present.
A separate Dash correction makes mnemonic/master-key inconsistency a failed
AddKeyresult and a recoverable corruption result during loading. It preserves old records lacking mnemonic fields and checksum-invalid recovery phrases that derive the exact stored key; no new BIP39 checksum policy is added.How Has This Been Tested?
On macOS arm64 using the depends-backed BDB/SQLite/Qt build:
make -j1passed.test_dash --run_test=walletdb_tests,platformkeys_tests,bip39_testspassed all 29 cases. New tests exercise actual public wallet loading for parent/derived/last-hardened caches, plaintext/encrypted orphan records and incompatible legacy owner IDs. Known-owner records still load; keys from old and matching-mnemonic records sign messages that verify. Inconsistent passphrases/phrases return corruption.assert(spk_man)and the mnemonic mismatch reachedassert(key == master_key_tmp.key)(exit 134). Both pass after the fix.test_runner.pywithwallet_descriptor.py,wallet_encryption.py,wallet_mnemonicbits.py,wallet_hd.py,wallet_backup.py,wallet_createwallet.pyandwallet_upgradetohd.py, each with--descriptors, passed. These cover normal descriptor creation/import, reloading persisted and encrypted descriptor wallets, unlocking and signing.git diff --checkpassed.The new isolated loader tests use the normal mock wallet database and public loader, not a full malformed SQLite RPC wallet. Existing Platform-key tests retain checksum-invalid mnemonic recovery coverage. No secret disclosure, theft, or peer trigger is established.
Breaking Changes
Malformed mnemonic/key associations now return a wallet-corruption error. Unowned descriptor records are ignored; database serialization and supported wallet recovery formats are unchanged.
Checklist:
🤖 Generated with Claude Code