contractcourt: insta-dispatch CLOSED_CHANNEL on first conf of coop close#10794
contractcourt: insta-dispatch CLOSED_CHANNEL on first conf of coop close#10794Roasbeef wants to merge 3 commits into
Conversation
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (5 files)
🟡 Medium (1 file)
AnalysisThis PR touches the Expert review is required before merging. To override, add a |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This PR addresses a regression where cooperative channel closes were delayed until the full confirmation depth, causing issues for clients expecting immediate notification. By introducing an early-dispatch callback in the chain watcher, the system now fires a CLOSED_CHANNEL event upon the first confirmation of a cooperative close, while maintaining reorg safety by deferring the full resolution event. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an early notification mechanism for cooperative channel closes, ensuring that RPC subscribers are notified as soon as a close transaction is detected on-chain. This is implemented by adding an early dispatch path in the chain watcher and suppression logic in the channel arbitrator to avoid duplicate events, supported by new unit and integration tests. The review feedback identifies an improvement opportunity in the isCoopCloseSpend helper to use the specific input index from the spend details rather than a hardcoded value, which enhances the robustness of the detection logic for transactions with multiple inputs.
| tx := &wire.MsgTx{ | ||
| TxIn: []*wire.TxIn{{Sequence: tc.sequence}}, | ||
| } | ||
| require.Equal(t, tc.want, isCoopCloseSpend(tx)) |
There was a problem hiding this comment.
Update the test call to isCoopCloseSpend to include the input index.
| require.Equal(t, tc.want, isCoopCloseSpend(tx)) | |
| require.Equal(t, tc.want, isCoopCloseSpend(tx, 0)) |
|
|
||
| // A degenerate tx with no inputs should not be classified as a | ||
| // coop close. | ||
| require.False(t, isCoopCloseSpend(&wire.MsgTx{})) |
There was a problem hiding this comment.
Update the test call to isCoopCloseSpend to include the input index.
| require.False(t, isCoopCloseSpend(&wire.MsgTx{})) | |
| require.False(t, isCoopCloseSpend(&wire.MsgTx{}, 0)) |
Roasbeef
force-pushed
the
coop-close-insta-dispatch
branch
from
bc783e2 to
82c17a5
Compare
|
@Roasbeef, remember to re-request review from reviewers when ready |
Roasbeef
force-pushed
the
coop-close-insta-dispatch
branch
from
82c17a5 to
29b4d35
Compare
Today NotifyClosedChannelEvent rebuilds its event by round-tripping through FetchClosedChannel, which forces the caller to have already persisted the close summary to the closed-channel bucket. The chain watcher needs to surface a CLOSED_CHANNEL event to RPC subscribers as soon as a coop close spend is first detected on chain, well before the close has reached the required confirmation depth at which the state machine would normally call MarkChannelClosed. In this commit, we add NotifyEarlyClosedChannelEvent, which dispatches a ClosedChannelEvent built from a caller-supplied summary directly through the subscribe server. The summary is expected to carry IsPending=true so subscribers can recognize that the close has not yet been finalized in the database. Two unit tests assert that the new path delivers the supplied summary verbatim and produces exactly one event per call.
Roasbeef
force-pushed
the
coop-close-insta-dispatch
branch
from
29b4d35 to
c1253e4
Compare
PR lightningnetwork#10331 introduced a multi-confirmation reorg-aware dispatch in the chain watcher. In production builds CloseConfsForCapacity is at least 3, so the chain watcher waits for three confirmations of a close tx before running dispatchCooperativeClose, MarkChannelClosed, and NotifyClosedChannel. Subscribers of the SubscribeChannelEvents stream that used to receive a CLOSED_CHANNEL event after a single confirmation in v0.20.1 stopped seeing the event entirely on shorter test cycles and were delayed by two extra blocks on longer ones. This is the regression alexbosworth reported on zero-conf channels. The intent behind the original change was to wait three confirmations under the hood for reorg safety while still dispatching a CLOSED_CHANNEL event to RPC subscribers immediately, matching the v0.20.1 surface. That insta-dispatch was wired into peer.WaitForChanToClose for the local CloseChannel response stream but was never extended to the channel-notifier path that drives SubscribeChannelEvents. In this commit, we wire a new optional notifyEarlyCoopClose callback into the chain watcher's processDetectedSpend. The first time a coop close spend is detected on chain, the chain watcher synthesizes a ChannelCloseSummary with IsPending=true and dispatches a CLOSED_CHANNEL event over the channel notifier, no DB round-trip required. The callback is plumbed through ChainArbitratorConfig .NotifyEarlyClosedChannel to the new ChannelNotifier.NotifyEarlyClosedChannelEvent. The summary builder shared with dispatchCooperativeClose is extracted into buildCoopCloseSummary so the early and post-N-conf paths produce equivalent payloads. A coopCloseEarlyDispatched flag on the chain watcher keeps the dispatch idempotent across blockbeat replays of the same spend, and the closeObserver clears it on negativeConfChan so a re-mined or replacement close after a deep reorg re-fires the preliminary event with its own summary. The early-dispatch call sits before the fast-path check so numConfs==1 also fires the early event through the same code path. Suppressing the duplicate notify at MarkChannelClosed time happens inline in the chain_arbitrator MarkChannelClosed callback: after CloseChannel succeeds, NotifyClosedChannel is fired only when the close type is not CooperativeClose. Force, breach, and abandon paths intentionally remain on the existing N-confirmation dispatch contract.
In this commit, we add three focused unit tests in contractcourt plus an itest that exercises the regression end-to-end. The chain watcher harness gains an opt-in early-dispatch capture that records every notifyEarlyCoopClose invocation so tests can assert how many fired and what summaries they carried. On top of that: TestEarlyDispatchCoopClose verifies the headline behavior. An async-path coop close fires exactly one early dispatch with IsPending=true and the post-N-conf flow still produces the regular CooperativeCloseInfo downstream. TestEarlyDispatchForceCloseNotInvoked guards the carve-out: force closes never fire the early dispatch since their CLOSED_CHANNEL event timing is intentionally unchanged. TestEarlyDispatchReorgRefiresOnReReplacement nails down the reorg path. Once a deep reorg removes the close, the early-dispatch flag is cleared and the next coop close re-fires the early event with its own summary, so a subscriber observes each distinct close attempt. testZeroConfCoopCloseSubscribeEvents brings up a zero-conf channel between Alice and Bob with --dev.force-channel-close-confs=3 so the chain watcher takes the async multi-confirmation path. Alice subscribes to channel events, initiates a cooperative close, and the test asserts that CLOSED_CHANNEL fires after only one confirmation of the close tx (not after the full three) and that FULLY_RESOLVED_CHANNEL arrives once the close has reached three confirmations. A quiet-window assertion at the end verifies that exactly one CLOSED_CHANNEL event is delivered. If the suppression in MarkChannelClosed broke and let it re-fire NotifyClosedChannel at N confs, this assertion would catch the duplicate.
Roasbeef
force-pushed
the
coop-close-insta-dispatch
branch
from
c1253e4 to
6dc9fe6
Compare
| [clarifies](https://github.com/lightningnetwork/lnd/issues/10568) the ZMQ | ||
| port-mismatch warnings so they no longer suggest that the connection failed. | ||
|
|
||
| * [Restored insta-dispatch of `CLOSED_CHANNEL` on the first confirmation of a |
|
New design: sequenceDiagram
participant CW as chainWatcher
participant CA as ChannelArbitrator
participant CHA as ChainArbitrator
participant CN as ChannelNotifier
participant RPC as SubscribeChannelEvents client
Note over CW: close tx reaches 1 conf
CW->>CW: build in-memory summary (IsPending=true)
CW->>CN: NotifyEarlyClosedChannelEvent(summary)
CN->>RPC: CLOSED_CHANNEL
CW->>CW: coopCloseEarlyDispatched.Store(true)
Note over CW: close tx reaches N confs
CW->>CA: CooperativeClosure event over channel
CA->>CHA: invoke MarkChannelClosed callback
CHA->>CHA: channel.CloseChannel persists summary
CHA->>CW: EarlyCoopCloseDispatched()?
CW-->>CHA: true
Note over CHA: skip NotifyClosedChannel
CA->>CHA: invoke NotifyChannelResolved callback
CHA->>CN: NotifyFullyResolvedChannel(chanPoint)
CN->>RPC: FULLY_RESOLVED_CHANNEL
|
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
I have one comment regarding a potential race see below, otherwise gtg
I am proposing however a new design in the long run:
This PR fixes the regression, but the notification ownership feels split: the watcher emits CLOSED_CHANNEL early, then the arbitrator later has to ask the watcher whether to suppress its own CLOSED_CHANNEL.
A cleaner model would make the watcher the only owner of CLOSED_CHANNEL emission, at first close-spend detection, and leave the arbitrator to persist the close and emit FULLY_RESOLVED_CHANNEL once resolution is final.
That would remove NotifyEarlyClosedChannel, coopCloseEarlyDispatched, the EarlyCoopCloseDispatched() lookup, and the duplicate-suppression branch. It would also give all close types the same lifecycle shape instead of special-casing coop closes.
sequenceDiagram
participant CW as chainWatcher
participant CA as ChannelArbitrator
participant CHA as ChainArbitrator
participant CN as ChannelNotifier
participant RPC as SubscribeChannelEvents client
Note over CW: close tx reaches 1 conf
CW->>CW: detect spend, build summary (IsPending=true)
CW->>CN: NotifyClosedChannelEvent(summary)
CN->>RPC: CLOSED_CHANNEL
Note over CW: close tx reaches N confs
CW->>CA: CooperativeClosure event over channel
CA->>CHA: invoke MarkChannelClosed callback
CHA->>CHA: channel.CloseChannel persists summary
Note over CHA: no notify step anymore
CA->>CHA: invoke NotifyChannelResolved callback
CHA->>CN: NotifyFullyResolvedChannel(chanPoint)
CN->>RPC: FULLY_RESOLVED_CHANNEL
| // reorg-aware dispatch was introduced. The suppression of the | ||
| // duplicate notify at MarkChannelClosed time is handled in | ||
| // chain_arbitrator.go via a CloseType check. | ||
| c.maybeDispatchEarlyCoopClose(spend) |
There was a problem hiding this comment.
Would be nice to fix: The early-dispatch flag can survive a replacement spend if the replacement is processed before the old confirmation subscription’s NegativeConf.
processDetectedSpend currently calls maybeDispatchEarlyCoopClose before it checks whether there is already a different currentPendingSpend. If the first coop close spend already fired
the early CLOSED_CHANNEL, then coopCloseEarlyDispatched is true. A later replacement coop close can enter this function while currentPendingSpend is still the old tx. In that ordering,
the replacement’s early dispatch is skipped because the flag is still true, then the different-spend branch cancels/re-registers the confirmation notification without clearing the flag.
At final confirmation depth, MarkChannelClosed sees a cooperative close and EarlyCoopCloseDispatched()==true, so it suppresses the final NotifyClosedChannel. That leaves subscribers
with the stale early event for the old txid and no closed-channel event for the replacement txid.
This is a narrow reorg race, not ordinary mempool RBF: it requires the original confirmed spend to be reorged out, a different coop close to confirm, and the watcher to process the
replacement spend before draining the old NegativeConf. But because the watcher selects over both channels independently, that ordering is possible once both notifications are ready.
Suggested fix comment
Consider handling duplicate/replacement spend detection before attempting early coop-close dispatch. For a different pending spender, clear the early-dispatch flag after canceling the
old confirmation notification, then let the replacement spend decide whether to emit its own early close event. That makes the flag represent the currently tracked pending spend rather
than any previous close attempt.
| if c.cfg.notifyEarlyCoopClose == nil { | ||
| return | ||
| } | ||
| if !isCoopCloseSpend(spend.SpendingTx) { |
There was a problem hiding this comment.
Nit: maybe add a comment to clarify why we only to it for the Coop-close case ?
|
|
||
| // The fast path dispatches the regular CooperativeCloseInfo | ||
| // synchronously without going through the multi-conf state machine. | ||
| closeInfo := harness.waitForCoopClose(5 * time.Second) |
There was a problem hiding this comment.
can we not use something like wait until to make the test less flaky ?
| c.Lock() | ||
| w := c.activeWatchers[summary.ChanPoint] | ||
| c.Unlock() | ||
| if w != nil && w.EarlyCoopCloseDispatched() { |
There was a problem hiding this comment.
I think this case is missing a unit test
|
|
||
| // FULLY_RESOLVED_CHANNEL must arrive after the close advances | ||
| // through the channel arbitrator at full N confs. | ||
| resolvedSeen := waitForChannelEventOfType( |
There was a problem hiding this comment.
missing a check here that no second CLOSED_CHANNEL is sent before the FULLY_RESOLVED_CHANNEL
4 participants
In this PR, we fix a regression that landed with the multi-confirmation
reorg-aware close dispatch in #10331: SubscribeChannelEvents stopped
emitting
CLOSED_CHANNELon cooperative closes for production buildsuntil the close had reached the full required confirmation depth
(
CloseConfsForCapacity, min 3). On test cycles that didn't mine farenough past the close tx the event never arrived at all, and on longer
cycles it was delayed by two extra blocks compared to the v0.20.1
surface. This is what alexbosworth was hitting on zero-conf channels in
v0.21.0-beta.rc1 and master (gist:
https://gist.github.com/alexbosworth/b0efefe06264b6f06103437312d9d1b1).
The original intent of #10331 was to wait three confirmations under the
hood for reorg safety while still firing a
CLOSED_CHANNELevent overRPC immediately, so user-visible behavior didn't change. That
insta-dispatch was wired into
peer.WaitForChanToClosefor the localCloseChannelresponse stream, but we never extended it to thechannel-notifier path that drives
SubscribeChannelEvents. This PRplugs that gap.
How it works
The chain watcher's
processDetectedSpendnow calls a new optionalnotifyEarlyCoopClosecallback the first time it sees a coop closespend in the async path (numConfs > 1). The callback is wired through
ChainArbitratorConfig.NotifyEarlyClosedChannelto a newChannelNotifier.NotifyEarlyClosedChannelEvent, which dispatches theClosedChannelEventfrom a caller-suppliedChannelCloseSummarywithIsPending=true, no DB round-trip. The summary is built bybuildCoopCloseSummary, extracted out ofdispatchCooperativeClosesothe early dispatch and the post-N-conf dispatch produce equivalent
payloads.
A
coopCloseEarlyDispatchedatomic.Boolon the chain watcher keepsthe dispatch idempotent across blockbeat replays of the same spend, and
the
closeObserverclears it onnegativeConfChanso a re-mined closestill re-fires the preliminary event with its own summary. The flag is
read across goroutine boundaries (writer is the closeObserver, reader
is the channel arbitrator's
MarkChannelClosedcallback), so theatomic.Boolmakes that cross-goroutine read race-free.Suppression of the duplicate
CLOSED_CHANNELat full conf depth ishandled inside the
MarkChannelClosedclosure constructed inchain_arbitrator.go: when the supplied close summary is a coop close,it looks up the chain watcher via
c.activeWatchers[summary.ChanPoint]and skips
NotifyClosedChannelifEarlyCoopCloseDispatched()returnstrue. Force-close and breach summaries always fall through to
NotifyClosedChannel, so force closes intentionally remain on theN-conf dispatch contract for now.
The fast-path (numConfs == 1) is left untouched, so integration tests
with the
--dev.force-channel-close-confs=1build-tag override stillsee exactly one synchronous
CLOSED_CHANNELfromhandleCommitSpend, no early dispatch.Resulting event timeline
For a coop close on a 1M sat channel with default
CloseConfsForCapacity == 3:CLOSED_CHANNELMarkChannelClosedpersistFULLY_RESOLVED_CHANNELThis matches the v0.20.1 surface for
CLOSED_CHANNELand keeps theFULLY_RESOLVED_CHANNELevent behind the reorg-safe N-conf gate asintended.
Reorg behavior
If the close gets reorged out at depth less than N, the existing
negativeConfChanhandler resets state and we now also clear theearly-dispatch flag. The next time a coop close lands (the same tx
re-mined or a different one mined in the new chain) we fire the early
event again with the new summary. RBF replacement isn't a concern here
because the early dispatch only fires after the spend has confirmed
once, and a confirmed tx can't be RBF'd while it remains in the chain.
We don't emit a synthetic "un-close" event — clients that acted on
CLOSED_CHANNELbeforeFULLY_RESOLVED_CHANNELcan re-derive statefrom
ListChannelsif needed. This matches the chain semantics andavoids inventing a new event type that callers would need to handle.
Scope notes
Force closes are out of scope. Their
CLOSED_CHANNELtiming isunchanged. Once we have confidence in the coop close path we can
revisit.
No proto change.
lnrpc.ChannelCloseSummarydoesn't gain anis_pendingfield; v0.20.1 also firedCLOSED_CHANNELat coop-closedetection without that distinction and clients use
FULLY_RESOLVED_CHANNELas the reorg-safe signal.See each commit message for a detailed description w.r.t the
incremental changes.
Test plan
channelnotifiercover the early-dispatchnotifier API.
contractcourtcover the chain watcherinsta-dispatch (idempotency, force-close carve-out, reorg
re-fire, fast-path unchanged, nil-callback no-op) and the
EarlyCoopCloseDispatchedgate the arbitrator reads.testZeroConfCoopCloseSubscribeEventsopens a zero-confchannel under
--dev.force-channel-close-confs=3, subscribes tochannel events, coop-closes, and asserts that
CLOSED_CHANNELfiresafter one block and
FULLY_RESOLVED_CHANNELafter three, with noduplicate
CLOSED_CHANNELin between.contractcourtandchannelnotifiertest suites passwith
-race.