Repository navigation
fix: relock encrypted wallets after HD upgrade failures - #7821
PastaPastaPasta merged 1 commit 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 2 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (2)
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 973f59e) · 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)
Verified the Phase-2 review claims against exact head 973f59e; no in-scope defects were found, and the single commit forms one coherent fix with regression coverage. The cleanup handler is installed before temporary unlocking and relocks on success or exception while both wallet locks remain held, before rescanning; the tests check signing denial and recovery after explicit unlock in legacy and descriptor modes. Verification was static only: the supplied CI snapshot shows successful container builds and formatting checks, two queued manifest jobs, and a cancelled conflict-prediction check, but does not establish completion of the build and test suite.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: 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 is a small, contained cleanup-handler ordering fix in upgradetohd with focused regression tests, not a large or intricate change 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— dash-core-commit-history (completed, effort medium); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:approved
…led upgrade instead creating brand-new scenario
…ollow-up fixes for lately added tests. 99cdb0b test: follow-up for #7812: use less fixture to test quorum dkgstatus (Konstantin Akimov) 0288ca9 docs: be more explicit about "scope", "scenario", "case" when writing tests. (Konstantin Akimov) 8a620db test: follow-up for #7838: replace brand new scenario to extra check (Konstantin Akimov) 4a19742 test: follow-up for #7821: check if wallet is locked after failed upgrade instead creating brand-new scenario (Konstantin Akimov) 1435bef test: follow-up for #7829: replace brand-new scenario to just extra check for mixing-unlock (Konstantin Akimov) 8a67e4d test: RPC importelectrumwallet have functional tests, removed an exception (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented #7829, #7821, #7838, #7812, added brand-new scenarious to functional tests while it could tested by extra check in existing scenarious. ## What was done? Updated guidance for agents about writing new tests. Follow-up fixes for #7829, #7821, #7838 by replacing scenarious to single check. Removed an exception for `importelectrumwallet` from list of un-tested RPCs For #7812: - Rotation test. The existing check after the DKG finishes now asserts 0 for each masternode. It no longer sums the stale debug sessions. The PR's two added scenarios are removed. - rpc_quorum.py. It now mines to the next DKG start block while the DKG spork is still off. Then it asserts the masternode's active_dkgs is above 0, before turning the spork on. The PR's separate scenario is removed. - test_framework.py. The helper that only the removed scenarios used is gone. ## How Has This Been Tested? Run functional tests. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: 625e2792ad4a848fa9a70dc65109846dae822e87fe0a8a116c7ce8bac224c7a35179bde6751915391d2d1b1bcab32eb1e28eaeaa4e54a5ff777d5c3f997924ba
2 participants
Issue being fixed or feature implemented
An encrypted wallet temporarily unlocked by
upgradetohdremained fully unlocked when mnemonic validation or later HD setup threw. An already authenticated caller with signing/spending permissions could then use existing keys until another lock event. This does not bypass RPC authentication.What was done?
Install the existing cleanup handler before the temporary unlock and relock on every subsequent exit, including success, while the wallet locks are still held. Keep early validation behavior and relock before any rescan.
How Has This Been Tested?
On macOS arm64 with prebuilt depends, built baseline and candidate with
make -j1. The added regression fails on the baseline in both legacy and descriptor modes because signing succeeds after a rejected upgrade. Fullwallet_upgradetohd.py --legacy-walletand--descriptorspass with the fix, covering signing denial after invalid mnemonic, legacy overlength passphrase and successful upgrade, plus verified signatures after explicit unlock. Existing recovery/rescan tests remain covered by those full runs.Ran
test_dash --run_test=bip39_tests,wallet_crypto_tests, changed-file Python lint using the repository's enabled rules, andgit diff --check. An independent four-pass review approved the final source and inspected the reproduced baseline evidence; its remaining cache/test suggestions were resolved by the complete reruns. No fund movement or RPC authentication bypass was demonstrated.Breaking Changes
None.
Checklist:
This pull request was created by Codex.