Wallet: Fix deadlock between walletpassphrase and the lockwallet relock timer - #1889
Wallet: Fix deadlock between walletpassphrase and the lockwallet relock timer#1889Danswar wants to merge 3 commits into
Conversation
RPCRunLater() erases any previously scheduled lockwallet timer, and destroying a libevent timer blocks until that timer's callback has finished executing. walletpassphrase() called it while still holding LOCK2(cs_main, pwallet->cs_wallet), so when the LockWallet() callback happened to be running at that moment it was itself blocked acquiring cs_wallet, and the two threads deadlocked: the RPC worker waiting on libevent, the HTTP event loop waiting on cs_wallet. Because the RPC worker also holds cs_main, every thread that needs cs_main then piles up behind it (net message handling, the scheduler, connection handling) and the node stops making progress entirely. Nothing is logged, and SIGTERM does not complete either, since the shutdown path needs the same locks. Scope the lock so it is released before scheduling the relock, and pass the expected nRelockTime into LockWallet() so an obsolete callback that fires after the wallet was re-unlocked or already locked does nothing.
|
User [email protected] does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
Summary by CodeRabbit
WalkthroughWallet relock callbacks now validate captured timestamps, concurrent ChangesWallet and RPC timer synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant walletpassphrase
participant CWallet
participant RPCRunLater
participant LockWallet
walletpassphrase->>CWallet: serialize with cs_unlock
walletpassphrase->>CWallet: update nRelockTime
walletpassphrase->>RPCRunLater: schedule captured relock time
RPCRunLater->>LockWallet: invoke relock callback
LockWallet->>CWallet: validate timestamp and lock wallet
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0509d602e6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wallet/rpcwallet.cpp`:
- Around line 3074-3082: Update the deadlineTimers scheduling path around
RPCRunLater to use a dedicated mutex protecting all accesses to the global
deadlineTimers map, including erase and emplacement. Ensure the mutex is held
while RPCRunLater performs its map operations, while preserving the existing
ordering that releases cs_wallet before scheduling the relock timer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 231ec610-3fa7-47c2-8130-bde566c004b3
📒 Files selected for processing (1)
src/wallet/rpcwallet.cpp
Moving the relock scheduling out of the cs_wallet scope also removed the serialization that cs_main used to provide for the two things walletpassphrase does after a successful unlock: publishing pwallet->nRelockTime and installing the lockwallet timer. RPCRunLater() can now be entered concurrently by up to -rpcthreads (default 4) HTTP worker threads, and both steps are racy: - deadlineTimers in rpc/server.cpp is a plain std::map that RPCRunLater() erases from and emplaces into, and that StopRPC() clears. Nothing serialized those accesses other than walletpassphrase happening to hold cs_main, so concurrent calls can now corrupt the map. - Two overlapping walletpassphrase calls can publish nRelockTime and schedule the timer in opposite orders. The slower call then installs the surviving timer while the faster call owns nRelockTime; when that timer fires, the generation check in LockWallet() correctly rejects it, no other timer is left, and the wallet stays unlocked past the requested timeout. Add a per-wallet cs_unlock that walletpassphrase holds across both the unlock and RPCRunLater(), so the two steps are atomic with respect to each other. It deliberately is not cs_wallet: the lock is held across RPCRunLater(), which blocks until a running relock callback returns, and that callback takes cs_wallet - which is exactly the deadlock fixed in the previous commit. Give deadlineTimers its own cs_deadlineTimers as well, so the map stays safe regardless of which RPC calls RPCRunLater() and independently of the fact that this build only ever has one wallet. It also closes the existing race between StopRPC() and an in-flight walletpassphrase, as StopRPC() runs before the HTTP worker threads have been joined. Neither new lock is acquired by any timer callback, so the resulting lock order (cs_unlock -> cs_main -> cs_wallet, and cs_unlock -> cs_deadlineTimers) has no cycle and cannot reintroduce the deadlock.
|
Thanks — both findings are correct, and both are addressed in 1. Unsynchronized 2. An older request replacing the newer timer. Confirmed, and it was a regression introduced by splitting the lock scope in the previous commit: publishing On why For context on scope: this release resolves every RPC to the single global Still unbuilt on my side, as noted in the PR description — the reasoning above is from reading the code, so the compile and the functional tests are worth treating as unverified until CI runs. |
|
@CodeAnt-AI review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:M
User descriptionFixes #1888.
Since the RPC worker also holds This is a known hazard in Bitcoin CoreCore hit the same cycle and documented it in the code, directly above the relock scheduling (permalink, // rpcRunLater must be called without cs_wallet held otherwise a deadlock
// can occur. The deadlock would happen when RPCRunLater removes the
// previous timer (and waits for the callback to finish if already running)
// and the callback locks cs_wallet.
AssertLockNotHeld(wallet->cs_wallet);Relevant Core changes:
ChangeTwo parts, mirroring #18814:
I kept TestingThe deadlock was diagnosed from a symbolized I have not built and run the patched binary — I do not currently have a build environment for this repo, so please treat the compile as unverified. The change is confined to one file and does not alter behaviour outside the CodeAnt-AI DescriptionPrevent wallet unlock commands from freezing the node What Changed
Impact
💡 Usage GuideChecking Your Pull RequestEvery time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later. Talking to CodeAnt AIGot a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask: This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code. ExamplePreserve Org Learnings with CodeAntYou can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input: This helps CodeAnt AI learn and adapt to your team's coding style and standards. ExampleRetrigger reviewAsk CodeAnt AI to review the PR again, by typing: Check Your Repository HealthTo analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health. |
Sequence DiagramThis PR prevents walletpassphrase from deadlocking with an active relock callback by releasing the wallet lock before replacing the timer. It also serializes concurrent unlock requests and ignores stale relock callbacks. sequenceDiagram
participant RPC Worker
participant Wallet
participant Timer Manager
RPC Worker->>Wallet: Serialize unlock requests
RPC Worker->>Wallet: Unlock and publish relock deadline
RPC Worker->>Timer Manager: Replace relock timer after releasing wallet lock
Timer Manager-->>Wallet: Run relock callback with expected deadline
Wallet->>Wallet: Compare expected and current deadline
alt Deadline is current
Wallet->>Wallet: Lock wallet and clear deadline
else Callback is stale
Wallet-->>Timer Manager: Ignore callback
end
Generated by CodeAnt AI |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| { | ||
| LOCK(cs_deadlineTimers); | ||
| deadlineTimers.clear(); | ||
| } |
There was a problem hiding this comment.
Suggestion: Clearing the HTTP-backed timers while the HTTP event loop is still running is unsafe. HTTPRPCTimer destruction calls event_free() without synchronizing with httpevent_callback_fn; if a callback is concurrently executing, the callback can access the freed HTTPEvent through self, causing a use-after-free during shutdown. Stop or synchronize the event loop before destroying these timers, or make timer destruction wait for callback completion. [race condition]
Severity Level: Critical 🚨
- ❌ Shutdown can crash during an executing HTTP timer callback.
- ⚠️ Wallet relock callbacks use the affected HTTP event path.
- ⚠️ Node termination becomes nondeterministic under concurrent shutdown.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/rpc/server.cpp
**Line:** 415:418
**Comment:**
*Race Condition: Clearing the HTTP-backed timers while the HTTP event loop is still running is unsafe. `HTTPRPCTimer` destruction calls `event_free()` without synchronizing with `httpevent_callback_fn`; if a callback is concurrently executing, the callback can access the freed `HTTPEvent` through `self`, causing a use-after-free during shutdown. Stop or synchronize the event loop before destroying these timers, or make timer destruction wait for callback completion.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| { | ||
| if (!timerInterface) | ||
| throw JSONRPCError(RPC_INTERNAL_ERROR, "No timer handler registered for RPC"); | ||
| LOCK(cs_deadlineTimers); |
There was a problem hiding this comment.
Suggestion: The new lock does not protect the lifetime of timerInterface. RPCRunLater checks timerInterface before acquiring cs_deadlineTimers, then may block on that lock while shutdown calls StopHTTPRPC(), unregisters and deletes the interface, and only afterward resumes to call Name() or NewTimer() through the dangling pointer. Synchronize interface registration and unregistration with this operation, or capture a lifetime-safe interface before releasing the lifecycle lock. [race condition]
Severity Level: Critical 🚨
- ❌ Concurrent RPC timer creation can crash during shutdown.
- ⚠️ Wallet relock scheduling uses this unprotected interface.
- ⚠️ Shutdown races with active RPC worker threads.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/rpc/server.cpp
**Line:** 628:628
**Comment:**
*Race Condition: The new lock does not protect the lifetime of `timerInterface`. `RPCRunLater` checks `timerInterface` before acquiring `cs_deadlineTimers`, then may block on that lock while shutdown calls `StopHTTPRPC()`, unregisters and deletes the interface, and only afterward resumes to call `Name()` or `NewTimer()` through the dangling pointer. Synchronize interface registration and unregistration with this operation, or capture a lifetime-safe interface before releasing the lifecycle lock.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. |
reubenyap
left a comment
reubenyap
left a comment
There was a problem hiding this comment.
Review summary
This is a correct, well-scoped fix that faithfully ports Bitcoin Core's solution. I verified the compile locally (details below) — recommend approval once CI has been run for this first-time contributor.
Verification performed
- Compile check (the one open risk — the author noted they could not build): I built the bls-dash dependency and compiled both changed translation units at the PR head (
67c1623) with GCC 13 in Release config —rpc/server.cppandwallet/rpcwallet.cpp(which includes the modifiedwallet.h). Both compile cleanly with no new warnings. The constructs I specifically checked:AssertLockNotHeldexists insrc/sync.h:95with a realDEBUG_LOCKORDERimplementation insync.cpp:171(no link failure in lock-debug builds), and the three-argumentboost::bindis fine. - The deadlock mechanism is real:
HTTPRPCTimerowns anHTTPEventwhose destructor callsevent_free(httpserver.cpp:533), andevthread_use_pthreadsis enabled (httpserver.cpp:405), so destroying the timer genuinely blocks until a running callback returns. The thread dumps in #1888 match this cycle. - Faithful to upstream, as claimed. Diffed against Bitcoin Core v28.0: the wallet side is a direct port of bitcoin/bitcoin#18814 (scoped
cs_walletblock, captured relock generation, generation check in the callback,AssertLockNotHeld, andcs_unlock↔ Core'sm_unlock_mutex, correctly renamed to this codebase'scs_convention). Theserver.cpphunk is line-for-line equivalent to Core'sg_deadline_timers_mutex, guarding the same two sites (RPCRunLater,StopRPC). The parts of Core's fix that were not ported are correctly inapplicable here: theweak_ptrunload guard (single staticpwalletMain, no dynamic unload) and the #32862 scheduler rework (deliberately skipped to stay minimal — the right call). - Supporting claims all check out:
walletpassphraseis the onlyRPCRunLatercaller in the tree;walletlockresetsnRelockTimeundercs_wallet(rpcwallet.cpp:3186), so the generation check handles explicit locks;StopRPCruns before HTTP workers are joined (init.cpp:267-268), socs_deadlineTimersdoes close a pre-existing shutdown race.
Correctness
- Lock ordering is acyclic:
cs_unlock → cs_main → cs_walletandcs_unlock → cs_deadlineTimers; neither new lock is acquired by any timer callback, andcs_unlockhas a single acquisition site. - A stale callback firing between the
cs_walletrelease andRPCRunLaterfails the generation check;cs_unlockprevents the overlapping-unlock ordering bug that the first commit alone would have introduced. - No behavior change outside the fixed path: error paths, help text,
TopUpKeyPoolplacement and theRPCRunLater-throws case are identical to master; theSecureStringpassphrase is now destroyed earlier, a marginal improvement. - Functional coverage exists via
qa/rpc-tests/wallet-encryption.py(unlock, timeout relock, timeout extension), which runs in CI. The deadlock itself is a timing race that is impractical to test deterministically — upstream added no test for #18814 either.
Bot findings triage
- The two Codex P2 findings and the CodeRabbit finding were real and are fixed by
67c1623; the author's replies are technically accurate. I am resolving those threads. - The two codeant-ai "Critical" findings can be dismissed:
- "Use-after-free:
event_free()doesn't synchronize with a running callback" — incorrect. Withevthread_use_pthreadsenabled,event_free/event_delfrom another thread blocks until the running callback returns; that blocking is precisely the deadlock mechanism this PR fixes. TheStopRPCtimer-clearing placement is also unchanged from master. - "
timerInterfacedangling pointer during shutdown" — a real but pre-existing theoretical race (StopHTTPRPCatinit.cpp:265deletes the interface while workers may still run untilStopHTTPServerat:268joins them). This PR neither introduces nor meaningfully widens it; it belongs in a separate cleanup, ideally a Core-#32862-style scheduler rework.
- "Use-after-free:
Minor notes (non-blocking)
- Roughly half the added lines are comments — more verbose than Core's four-line equivalent, but the lock-rationale content is genuinely useful; fine to keep.
- Pre-existing gaps Core later hardened (not regressions of this PR): no rejection of negative
timeoutand no clamp on huge values that can overflowGetTime() + nSleepTime. A follow-up adding Core's validation is being prepared separately. - Process nits, no code change needed: the PR title lacks the repo's
Wallet:area prefix, and the description doesn't follow the template headings (PR intention/Code changes brief).
Generated by Claude Code
|
@Danswar — we'd like to fold the timeout-validation hardening mentioned in the review into this PR rather than land it separately. The commit is ready on a branch in this repo, stacked directly on your git fetch https://github.com/firoorg/firo.git claude/review-pr-1889-firo-ygv719
git cherry-pick 8b4d7393a559f426a8df2691009bdf18c5d13728
git pushWhat it does (mirrors upstream Bitcoin Core's
The commit was compile-checked against your branch head ( Generated by Claude Code |
Reject a negative timeout with RPC_INVALID_PARAMETER instead of accepting it and scheduling a relock in the past, and clamp the timeout to 100000000 seconds so GetTime() + nSleepTime cannot overflow nRelockTime. Both checks run before the wallet is unlocked, so a rejected call leaves the wallet state untouched. This mirrors the validation upstream Bitcoin Core performs in walletpassphrase (the negative-timeout error and MAX_SLEEP_TIME clamp). Also cover the negative-timeout rejection in wallet-encryption.py, including that the wallet stays locked afterwards. Follow-up to firoorg#1889. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01UTd2DsfD7pTdvxDbubqASZ
|
User [email protected] does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
Danswar
changed the title
|
@reubenyap thanks, I addressed your comments |
|
Confirmed — the cherry-pick at Next step is on us: approving the CI workflow run for a first-time contributor, then review sign-off from the requested reviewers. Thanks for the quick turnaround and for a well-researched fix. Generated by Claude Code |
|
The failure is in the Guix bootstrap, not in the build: codeberg.org was degraded on 2026-07-31, so It is not specific to this PR:
Upstream looks recovered — |
|
Verified — the The re-run can't be triggered via the API from here, so it needs a manual "Re-run failed jobs" click — flagged for that. No action needed on your side. Generated by Claude Code |
|
sorry closed by mistake, reopened |
|
any chances of moving forward with this fix? anything else I can do to help? 🙏 |
|
Status update: CI was fully re-run on Aug 7 (run 30912163059) and the earlier guix infra failure is cleared — The only remaining red is So there's nothing for you to do — the branch has effectively demonstrated green across both runs combined. It needs one more "Re-run failed jobs" click from a maintainer, then reviewer sign-off. I've flagged it again on our side; sorry for the slow turnaround here. Generated by Claude Code |
3 participants
Fixes #1888.
PR intention
walletpassphraseholdsLOCK2(cs_main, pwallet->cs_wallet)across its call toRPCRunLater.RPCRunLatererases the previously scheduledlockwallet(...)timer, and destroying a libevent timer blocks until that timer's callback has finished running. WhenLockWalletis the callback currently executing on the HTTP event loop, it is itself blocked acquiringcs_wallet— so the RPC worker and the event loop deadlock on each other.Since the RPC worker also holds
cs_main, message handling, the scheduler and connection handling all stall behind it and the node stops entirely. Nothing is logged, andSIGTERMdoes not complete because the shutdown path needs the same locks. Full thread-dump evidence is in #1888.This is a known hazard in Bitcoin Core
Core hit the same cycle and documented it in the code, directly above the relock scheduling (permalink,
21b42f3):Relevant Core changes:
nRelockTime != relock_timegeneration check andm_unlock_mutex. Fixes #18811.RPCRunLater/RPCTimerInterface. Superseded #32796, part of #31194 (remove libevent). Its motivation was libevent removal rather than this deadlock, but it does eliminate the hazard structurally.Code changes brief
Three commits, mirroring Core's #18814.
1. Break the deadlock —
Fix deadlock between walletpassphrase and the wallet relock timerLOCK2(cs_main, pwallet->cs_wallet)moves into a block that ends beforeRPCRunLateris called, so the relock is scheduled with no wallet lock held. This alone breaks the cycle — the pending callback can acquirecs_wallet, finish, and letevent_delreturn.LockWalletnow takes thenRelockTimeit was scheduled with and returns without doing anything ifpwallet->nRelockTimeno longer matches.walletlockalready resetsnRelockTimeto0, so a callback that fires after an explicit lock is also correctly ignored.2. Restore the serialization
cs_mainused to provide —Serialize walletpassphrase and guard the RPC timer mapScoping the lock also removed the mutual exclusion that holding
cs_mainincidentally gave the two steps after a successful unlock — publishingnRelockTimeand installing the timer:cs_unlockis held across both, so two overlappingwalletpassphrasecalls cannot publishnRelockTimeand schedule the timer in opposite orders, which would leave the wallet unlocked past its timeout. It deliberately is notcs_wallet: it is held acrossRPCRunLater, whose callback takescs_wallet.deadlineTimersinrpc/server.cppgets its owncs_deadlineTimers. It is a plainstd::mapthatRPCRunLatererases from and emplaces into and thatStopRPCclears, previously unserialized. This also closes the pre-existingStopRPC/ in-flight-walletpassphraserace, sinceStopRPC(init.cpp:267) runs before the HTTP workers are joined byStopHTTPServer(init.cpp:268).The resulting lock order is acyclic —
cs_unlock → cs_main → cs_walletandcs_unlock → cs_deadlineTimers— and no timer callback acquires either new lock.3. Validate the timeout —
Validate walletpassphrase timeout before unlockWritten by @reubenyap and cherry-picked in at his request; ported from upstream Core's
walletpassphrasevalidation.timeoutis rejected withRPC_INVALID_PARAMETERinstead of scheduling a relock in the past.100000000seconds. Core's stated reason for that bound is that larger values trigger a macos/libevent bug; it also keepsGetTime() + nSleepTimefrom overflowingnRelockTime.Unlock(), so a rejected call leaves wallet state untouched. Previously the timeout was only parsed afterUnlock()andTopUpKeyPool(), so a non-integer value threw with the wallet already unlocked and no relock scheduled at all.qa/rpc-tests/wallet-encryption.pycovers the rejection and that the wallet stays locked afterwards.Not included
I kept
RPCRunLaterrather than porting Core's scheduler rework (#32862), to keep the change small. Happy to do the larger version instead if you prefer — it removes the "destroy an event from inside a lock its callback needs" pattern rather than working around it.Core's
weak_ptrunload guard from #18814 does not apply here: this build has a single staticpwalletMainand no dynamic wallet unload.Testing
The deadlock was diagnosed from a symbolized
thread apply all bttaken on a live hung node, which pins both sides of the cycle to exact call sites (see #1888).qa/rpc-tests/wallet-encryption.pycovers unlock, timeout relock, timeout extension and now negative-timeout rejection. It is listed inqa/pull-tester/rpc-tests.py, which the Jenkins pipeline runs asqa/pull-tester/rpc-tests.py -extended. The deadlock itself is a timing race that is impractical to test deterministically — upstream added no test for #18814 either.I still do not have a build environment for this repo, so I have not compiled or run the patched binary myself. @reubenyap reports having compile-checked both the branch head and the timeout-validation commit locally. The Jenkins run is the real gate, and it has not run on this branch yet.