lnwallet/chancloser: fix data race in the legacy coop close state machine - #11019
Conversation
Roasbeef
added
the
backport-v0.21.x-branch
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (4 files)
🟢 Low (4 files)
AnalysisAll substantive changes land in To override, add a |
2f99346 to
15c00da
Compare
ziggie1984
added
the
backport-v0.20.x-branch
15c00da to
265b1d7
Compare
|
The mutex looks like a reasonable scoped fix for this race, but I think the mutex-based ownership remains somewhat fragile as a long-term design. In particular, the closer holds the lock while calling into channel methods, the MuSig helper, the optional aux closer, channel disabling, and transaction publication. None of the current production implementations appear to re-enter Longer term, could we move toward single-goroutine ownership of the close state machine? For example, the link flush hook could enqueue a I don't think that architectural change needs to be part of this PR, but it would be good to track it as follow-up work so the mutex does not silently become the permanent concurrency model. |
265b1d7 to
06bf839
Compare
|
Thanks for the review. Revised the approach to just stick with the existing single threaded dispatch loop, so no need for the mutex at all. There was only a single message path that could race, which now goes thru a unified dispatch path. |
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.
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.
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.
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.
06bf839 to
4944bb0
Compare
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-11019-to-v0.20.x-branch
git worktree add --checkout .worktree/backport-11019-to-v0.20.x-branch backport-11019-to-v0.20.x-branch
cd .worktree/backport-11019-to-v0.20.x-branch
git reset --hard HEAD^
git cherry-pick -x a8e2a0f7fa5958d3e530ee17ca4667c4e5794486 e5e134ddacbcbfb6998b6b8b18111f3b7a8cd5ee fb89732d24ebe101c475b74a3027af32b099cf5c 4944bb079424ec4e4636276824a332b50b9e2d7b
git push --force-with-lease |
|
Successfully created backport PR for |
…21.x-branch [v0.21.x-branch] Backport #11019: lnwallet/chancloser: fix data race in the legacy coop close state machine
Backport of the release note from PR #11019, retargeted from release-notes-0.21.2.md to release-notes-0.20.2.md.
4 participants
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.