[v0.21.x-branch] Backport #11019: lnwallet/chancloser: fix data race in the legacy coop close state machine - #11031
Merged
Conversation
In this commit, we have DustLimitForSize fall back to the generic witness dust threshold for any script size that doesn't match one of the well-known templates. The size switch covered P2WPKH, P2WSH, P2SH, P2PKH, and the explicit unknown-witness size, and treated every other length as unreachable. That's a narrower assumption than the callers can actually make good on: a witness program for versions 1 through 16 carries a program of anywhere from 2 to 40 bytes, so its serialized length won't always land on one of those exact values. The dust calculation only needs a representative output of roughly the right shape, and the unknown-witness pricing is the conservative choice among the ones we have, so we make it the default. That leaves the helper well defined across the whole range of sizes callers can pass it, including scripts carrying witness versions we don't know about yet. (cherry picked from commit f80f92d)
In this commit, we make the RBF co-op closer validate the remote party's delivery script in all cases, matching what the negotiation closer already does. Previously we only ran the check when we had an upfront shutdown script on record for the peer, so a peer that never committed to an upfront script could hand us a delivery script that we'd stash and carry through the rest of the close flow without ever looking at it. We now always call validateShutdownScript with the (possibly nil) upfront script: a nil upfront script still runs the well-formedness check on the peer's script, and a non-nil one additionally enforces the exact match, same as before. We also require the script to be present. The wire format puts no lower bound on the address length, and validateShutdownScript treats an absent peer script as nothing to check, so an empty one passed validation by default rather than on its merits. Both entry points now go through one helper that insists on a script before running the usual checks over it, which also covers a CloserScript swapped in mid-negotiation via ClosingComplete rather than letting that one go unchecked. The delivery-form coverage is spelled out in the tests: the spec dropped p2pkh and p2sh for co-op closes to keep the dust calculations uniform, and we don't implement the OP_RETURN form that option_simple_close allows, so all of those are rejected along with an empty or malformed script. (cherry picked from commit a8e2a0f)
In this commit, we give the legacy ChanCloser a single owner, rather than letting two goroutines advance it. The peer's channelManager drives the state machine for the Shutdown and ClosingSigned messages that come off the wire, and for local close requests. The link drives it as well: while we wait for the channel to drain we register a flush hook, and the link invokes that hook from its own goroutine, where it called BeginNegotiation directly. Nothing kept the two apart, so the state field, the priorFeeOffers map, and the signing step could all be touched at once. Under `go test -race` this shows up as a data race on the state field. Rather than reach for a lock, we route the flush through the channelManager. The hook now only reports the channel ID over a new chanCloseFlushed channel, and handleChanFlushed picks it up next to the close messages. Every transition, the cached offer processing, the fee map, and the signing then happen on the one goroutine, so the closer needs no synchronization of its own. We spell that out on the type, since it's an invariant a new caller can break from the outside. The report goes out from a fresh goroutine, which matters more than it looks. The link may well be holding its own lock while it invokes the hook, and channelManager reaches for that same lock in DisableAdds, so blocking on the handoff would trade the race for a deadlock. The `go` in front of RemoveLink just above it is there for the same reason. We look the closer up with a plain map load rather than through fetchActiveChanCloser, as that one builds a fresh closer when it doesn't find an existing one, and a flush that lands after the negotiation was torn down has no business starting a new negotiation. One behavior change falls out of the move: the flush path now runs the same finalization tail as the message path. It skipped that before, so a responder that drained a cached offer would reach closeFinished and broadcast, but nothing ran finalizeChanClosure until the next close message showed up, and having already sent its final signature, there may not be one. The link == nil path already ran the tail, so this makes all three paths agree. The new test drives a close with a link that hands us the flush hook instead of running it inline, so we can check that negotiation waits on the report, and that a report for a channel we have no closer for is dropped. (cherry picked from commit e5e134d)
In this commit, we hold off on recording the remote party's close output until we've decided we can act on their Shutdown. ReceiveShutdown wrote the field before it looked at the state, so a Shutdown that arrives at a point where we have nothing to do with it, say once we've already finished the negotiation, would still overwrite the output we settled on before being turned away with ErrInvalidState. The output we report for the close then describes a message we rejected. Nothing acts on this today, as we hand the outputs to the caller only after ClosingTx tells it the negotiation finished, but the field is what we report to the party that asked for the close, so we may as well only fill it in from a message we accepted. (cherry picked from commit fb89732)
(cherry picked from commit 4944bb0)
ziggie1984
approved these changes
Aug 6, 2026
ziggie1984
left a comment
ziggie1984
left a comment
Collaborator
There was a problem hiding this comment.
LGTM (no merge conflicts)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #11019
In this PR, we fix a data race in the legacy cooperative close state machine,
and tighten up two pieces of input handling on the close path while we're in
there.
The
ChanCloserhas no lock, but it's driven from two goroutines. The link'sflush hook calls
BeginNegotiation(which drains any cachedClosingSignedoffer) on the link goroutine, while the peer's close-message handler calls
ReceiveShutdownandReceiveClosingSignedon the peer goroutine. Nothingsynchronized the two, so the
statefield, thepriorFeeOffersmap, and thesigning step could all be touched concurrently.
go test -racereports therace on
statedirectly.We add a mutex and take it in every exported entry point that touches mutable
state.
BeginNegotiationdrains its cached offer through a lock-free helper soit doesn't re-enter the mutex it already holds. With the transitions guarded,
we also funnel them all through one
setStatehelper rather than leaving themscattered across the message handlers, which gives us a single place for the
bookkeeping tied to entering a state.
The other two commits are smaller. The RBF closer only validated the remote
party's delivery script when we had an upfront shutdown script on record for
that peer, so a peer that never committed to one could hand us a script we'd
carry through the rest of the close without ever looking at it. We now validate
it in all cases, matching what the negotiation closer already does, and give a
script swapped in mid-negotiation the same treatment.
See each commit message for the incremental detail.