[PastaPastaPasta]

Issue being fixed or feature implemented

CWallet::Unlock(SecureString) returned true without checking the passphrase whenever the encrypted wallet was already fully unlocked. walletpassphrase treats that result as authentication and replaces the relock deadline and timer. So while a wallet is fully unlocked, a caller allowed to use walletpassphrase could keep it unlocked past the owner's deadline with any nonempty wrong passphrase. Spending or exporting keys after the original deadline still needs those RPC permissions too. A locked or mixing-only wallet cannot be unlocked this way, and RPC authentication is not bypassed.

The early return is Dash-specific (added in 78c5ca5, blocked in 0.12.1, reintroduced by ff5540b in 0.16.0.0). Bitcoin Core decrypts the master key on every Unlock.

What was done?

• Removed the early return. Every request on an encrypted wallet now derives the key from the passphrase and decrypts the master key, as Bitcoin Core does.
• When a master key is already held (full or mixing-only unlock), the decrypted candidate must match it, compared with TimingResistantEqual. A mismatch moves on to the next master-key entry.
  • This matters because the already-unlocked path skips CheckDecryptionKey, and a blank wallet passes that check trivially. Without the comparison, a wrong passphrase that happens to produce valid padding would still be accepted.
• An authenticated request on a fully unlocked wallet is a no-op success. The metadata and descriptor-cache upgrades are not repeated.
• The mixing-only → full upgrade path is unchanged, and so are the RPC timer logic, full → mixing rejection and the unencrypted-wallet result.

How Has This Been Tested?

macOS arm64, depends build.

• Regression test. wallet_encryption.py now ends with one deterministic check under the mocktime the test already sets: with the wallet fully unlocked for 100 seconds, walletpassphrase with a wrong passphrase and a 1000-second timeout fails with -14, and unlocked_until does not change. Against an unpatched wallet.cpp the check fails ("No exception raised"). With the fix it passes.
• wallet_encryption.py --legacy-wallet and wallet_encryption.py --descriptors both pass.
• test_dash --run_test=bip39_tests,wallet_crypto_tests passes, and lint-python.py is clean.
• All Unlock(SecureString) callers were checked: Qt unlock dialog and wallet controller, createwallet, EncryptWallet, migration and upgradetohd.

Limits:

• The valid-padding mismatch guard was reviewed in the source, not fault-injected. A wrong passphrase gets past decrypt padding only about 1 in 256 times, so a functional test cannot exercise the guard deterministically.
• A locked blank encrypted wallet still accepts about 1 in 256 wrong passphrases. That gap already existed and Bitcoin Core has it too. Closing it needs a stored key check, so it is out of scope here.

Breaking Changes

None.

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 07b5449c-d09c-45fc-8a15-cff97877d99c

📥 Commits

Reviewing files that changed from the base of the PR and between f35a24c and 13e6e8e.

📒 Files selected for processing (2)

• src/wallet/wallet.cpp
• test/functional/wallet_encryption.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

---

Walkthrough

CWallet::Unlock now validates the supplied passphrase for encrypted wallets, including when the wallet is already unlocked. It returns success immediately for unencrypted wallets. Functional tests cover passphrase rejection, unlock deadlines, signing after unlock, mixing-only unlocks, and blank wallets.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 13e6e

No actionable merge-blocking risk is established; the change is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely identifies the wallet authentication fix for unlock deadline extensions, which is the main change in the pull request.
Description check	✅ Passed	The description directly explains the authentication flaw, the implementation changes, the regression tests, and the scope limitations.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.}

[thepastaclaw]

✅ Final review complete — no blockers (commit 86ba039) · triage: low · Phase 2 only (queue backlog)

[thepastaclaw]

Final validation — Phase 1 + Phase 2

At head d2bd1a2, the wallet change authenticates unlock extensions before returning success and preserves the existing mixing-mode and RPC timer behavior. No blocking defect was identified; one non-blocking concern remains about the new regression test's real-time timing margins. Verification was static only: the supplied CI snapshot shows successful formatting and metadata checks, but container builds remain queued and no completed build or test results are available.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); 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: low by gpt-6.1-sol (effort low) — The diff is a small, contained change to CWallet::Unlock passphrase authentication with focused regression tests, not a large or intricate key-handling change meeting the critical bar.
• Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer
• Phase 1 model: glm-5.3-flash — zai quota: 5h 90% left, weekly 94% left; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-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 `test/functional/wallet_encryption.py`:
- [SUGGESTION] test/functional/wallet_encryption.py:36-51: Give the new real-time relock assertions more scheduling margin
  The initial 3-second unlock must cover two wrong-passphrase key derivations, several RPC round trips, and the first signing operation. The extension scenario then sleeps for 3 seconds against a 5-second timer, leaving less than 2 seconds for the successful signing RPC. These timers use real libevent time, and --timeout-factor does not scale either the wallet deadlines or these sleeps, so a loaded or instrumented worker can relock the wallet before an operation that the test expects to succeed. The expiry assertions also allow only about one second for the timer callback to run. Increase the unlock durations and corresponding sleeps together, preserving the checks on both sides of the original and replacement deadlines. This is a scheduling robustness concern, not an observed failure in this review; repository history in 7513a30d87 independently confirms sanitizer-related wallet timeout flakes.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Verified the complete diff at 13e6e8e, including wallet unlock callers, master-key validation, RPC timer handling, and functional-test timeout scaling; no blocking correctness issues were found. The prior scheduling-margin concern is fixed, and the only remaining suggestion is to consolidate the test correction into its introducing commit. This was a static review without builds or test execution; build and validation checks were still queued or pending in the supplied CI snapshot.

🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Fold the regression timing correction into the original fix
<commit:13e6e8ed49>:1

Commit 13e6e8e only changes timeout scaling, unlock windows, and expiry waits in test_unlock_authentication, introduced immediately beforehand by d2bd1a2. It revises the same regression scenario rather than adding independently scoped coverage. CONTRIBUTING.md's "Squashing Commits" section identifies repeated changes to the same lines as fixup commits suitable for consolidation. Fold this follow-up into d2bd1a2 so the permanent history presents the authentication fix with its final regression test instead of an intermediate test followed by its timing correction. This is a non-blocking history suggestion, not a defect in the final code.

_{source: gpt-6.1-sol (phase2-reviewer: dash-core-commit-history)}

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); 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: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — The diff is a small, contained authentication fix in CWallet::Unlock with focused regression coverage, not a large or intricate change despite touching key handling.
• Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer
• Phase 1 model: glm-5.3-flash — zai quota: 5h 88% left, weekly 89% left; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-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 `<commit:13e6e8ed49>`:
- [SUGGESTION] <commit:13e6e8ed49>:1: Fold the regression timing correction into the original fix
  Commit 13e6e8ed49 only changes timeout scaling, unlock windows, and expiry waits in test_unlock_authentication, introduced immediately beforehand by d2bd1a2673. It revises the same regression scenario rather than adding independently scoped coverage. CONTRIBUTING.md's "Squashing Commits" section identifies repeated changes to the same lines as fixup commits suitable for consolidation. Fold this follow-up into d2bd1a2673 so the permanent history presents the authentication fix with its final regression test instead of an intermediate test followed by its timing correction. This is a non-blocking history suggestion, not a defect in the final code.

[PastaPastaPasta]

The timing concern is resolved at 13e6e8ed49b8, and I have marked the GitHub conversation resolved following this exact-head re-review. I have also recorded the nonblocking history suggestion: fold the timing correction into d2bd1a2673 at the next required restack or before an unsquashed merge. A history-only consolidation must preserve the exact final tree; if the base also changes, I will verify that rebase separately. Current-head source CI is still queued.

---

🤖 Posted autonomously by Codex on behalf of pasta.

[thepastaclaw]

Re-review — Final validation — Phase 2 only (queue backlog)

At exact head 86ba039, the complete diff correctly authenticates encrypted-wallet unlock requests before accepting deadline updates, while preserving authenticated no-op success, mixing-only upgrades, and unencrypted-wallet behavior; no actionable in-scope defects were found. Both prior suggestions are addressed: the range contains one cohesive commit, and the replacement regression checks wrong-passphrase rejection and deadline preservation without the added real-time expiry scenarios. This was static verification only; the supplied exact-head CI snapshot shows successful formatting and merge checks, with container builds and title checks still queued.

🔴 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: dash-core-commit-history); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: 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: low by gpt-6.1-sol (effort low) — The diff is a small, contained authentication fix in CWallet::Unlock with a focused regression test, not a large or intricate change despite touching key handling.
• Phase 1 reviewers: not run (skipped for throughput: 12 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; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-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.

[PastaPastaPasta]

approved