lnwallet: fix limboMtx/intentMtx lock order inversion in PsbtFundingVerify

[LNBIG-COM]

Change Description

PsbtFundingVerify acquires intentMtx and then limboMtx, while
handleFundingCancelRequest — which runs in the wallet's single requestHandler
goroutine — acquires the two in the opposite order. With PSBT or batch funding
both paths run concurrently, so the two goroutines can deadlock.

This was hit in production on v0.20.2-beta and confirmed with a goroutine dump
taken from the still-wedged process. The inversion is unchanged in master and
in v0.21.1-beta, so it is not fixed by upgrading. The full analysis, the
four-goroutine deadlock cycle and the stacks are in #11011.

The consequences are node wide and completely silent. requestHandler is the
only executor of every ChannelReservation method, and those methods are
unconditional round trips with neither a timeout nor a quit escape. Once it is
stuck, a peer disconnect parks funding.Manager's resMtx forever in
CancelPeerReservations, and the next zombie sweeper tick kills
reservationCoordinator on resMtx.RLock. From that point the node can neither
open nor accept channels, channels whose funding transaction confirmed stay in
the channelReadySent opening state forever and are never added to the graph nor
announced, and nothing is logged about any of it. Only a restart recovers.

The fix looks the channel reservation up, and releases limboMtx, before
acquiring intentMtx, and documents the required order on the mutex
declaration. The only behavioural change is which of two "not found" errors is
returned first.

The issue also lists several contributing factors that are out of scope here and
would each need their own change: the wallet round trip performed while holding
resMtx, the reservation leaked when Cancel() fails, the missing timeouts on
ChannelReservation methods, and BatchOpenChannel not honouring its request
context.

Fixes #11011

Steps to Test

go test ./funding/ passes, including the TestBatchFund/initial_negotiation_failure
subtest. gofmt, go vet ./lnwallet/ and go build ./lnwallet/ are clean on
both master and v0.20.2-beta. Integration tests were not run locally.

[Lrifton92]

Traced this against master before commenting, and the inversion is real — not just plausible from the description.

The two orders, on current master (lnwallet/wallet.go):

• handleFundingCancelRequest (L1459): limboMtx.Lock() with defer Unlock(), so it is still held when it reaches intentMtx.Lock() at L1488 → limbo → intent, nested.
• PsbtFundingVerify (L752): intentMtx.Lock() with defer Unlock(), then limboMtx.Lock() at L777 → intent → limbo, nested.

Two goroutines, both nesting, opposite order. That is a textbook AB-BA deadlock, and since handleFundingCancelRequest runs in the wallet's single requestHandler goroutine, losing it takes down every ChannelReservation operation for the node — which matches the failure you describe.

I also checked whether this is the only inversion, since a partial fix here would be worse than none. The two other sites that touch both mutexes are handleFundingCounterPartySigs (L2302) and the funding-complete path (L2545). Both release limboMtx before acquiring intentMtx — sequential, not nested — so neither can participate in a cycle, and both already happen to follow the order this PR documents. PsbtFundingVerify was the lone outlier. The fix is complete, not a spot patch.

On the fix itself: hoisting the reservationIDs/fundingLimbo lookup above intentMtx and releasing limboMtx before taking it is the minimal correct shape — it removes the nesting entirely rather than reordering one nest into another. I confirmed enforceNewReservedValue does not reach back into limboMtx (it goes to CurrentNumAnchorChans → DB, then WithCoinSelectLock → CheckReservedValue), so the tail of the function does not silently reintroduce the pair. And your claim that the only behavioural change is which of the two "not found" errors surfaces first holds up: the moved block is pure map lookup with no side effects.

Two non-blocking notes:

1. The reservation is now read outside the intentMtx window, so a concurrent handleFundingCancelRequest can delete it from fundingLimbo and then park on intentMtx while PsbtFundingVerify is still running. In that interleaving PsbtFundingVerify proceeds on a reservation that is already gone from limbo and can return success for a reservation that is cancelled a moment later — whereas before it would more likely have returned no channel reservation found. It looks harmless in practice (the only fields read are ChannelFlags and ChanType, both fixed for the lifetime of the reservation, and the intent is cancelled right after anyway), but it is a widening of that window and seemed worth naming explicitly rather than leaving implicit.

2. The ordering is now guarded only by the NOTE: on the limboMtx declaration. That is the right place for it and it is well worded, but nothing mechanical stops the next inversion from being reintroduced — worth a thought if you or the maintainers know of a lock-ordering check that fits lnd's tooling.

Neither of those blocks the fix. The diff is correct and the deadlock is genuine.

[litbot-9000]

@LNBIG-COM, remember to re-request review from reviewers when ready

[ziggie1984]

Could you please rebase this branch onto the current master and force-push the rebased fix, dropping the final merge commit (f532245ea)? That would leave the PR with a single focused fix commit and keep the history clean. The effective PR diff is already correct; this is only a commit-history cleanup.

[ziggie1984]

Non-blocking: could we make the invariant slightly more precise? “When both mutexes are needed” can be read as forbidding a safe sequential intentMtx.Lock(); intentMtx.Unlock(); limboMtx.Lock() pattern. The actual rule is that the reverse order must not be nested. Perhaps: Never acquire limboMtx while holding intentMtx. If both mutexes must be held simultaneously, acquire limboMtx before intentMtx.

[ziggie1984]

Thank you for this fix.

Can you also add release notes for 20.4 and 21.3, the empty release notes should be merged today I opened the PRs