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 intolightningnetwork:masterfrom
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.
| func isCoopCloseSpend(spendingTx *wire.MsgTx) bool { | ||
| if len(spendingTx.TxIn) == 0 { | ||
| return false | ||
| } | ||
|
|
||
| log.Infof("Cooperative closure for ChannelPoint(%v): %v", | ||
| c.cfg.chanState.FundingOutpoint, | ||
| lnutils.SpewLogClosure(broadcastTx)) | ||
| switch spendingTx.TxIn[0].Sequence { | ||
| case wire.MaxTxInSequenceNum: | ||
| return true | ||
| case mempool.MaxRBFSequence: | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
The isCoopCloseSpend function currently hardcodes the input index to 0 when checking the sequence number. While cooperative close transactions typically have a single input, it is safer and more accurate to use the actual input index that spends the funding output, which is provided by the SpendDetail. This ensures correctness if the funding spend happens to be part of a larger transaction where the funding input is not at index 0.
| func isCoopCloseSpend(spendingTx *wire.MsgTx) bool { | |
| if len(spendingTx.TxIn) == 0 { | |
| return false | |
| } | |
| log.Infof("Cooperative closure for ChannelPoint(%v): %v", | |
| c.cfg.chanState.FundingOutpoint, | |
| lnutils.SpewLogClosure(broadcastTx)) | |
| switch spendingTx.TxIn[0].Sequence { | |
| case wire.MaxTxInSequenceNum: | |
| return true | |
| case mempool.MaxRBFSequence: | |
| return true | |
| } | |
| return false | |
| } | |
| func isCoopCloseSpend(spendingTx *wire.MsgTx, inputIdx uint32) bool { | |
| if int(inputIdx) >= len(spendingTx.TxIn) { | |
| return false | |
| } | |
| switch spendingTx.TxIn[inputIdx].Sequence { | |
| case wire.MaxTxInSequenceNum: | |
| return true | |
| case mempool.MaxRBFSequence: | |
| return true | |
| } | |
| return false | |
| } |
| if c.cfg.notifyEarlyCoopClose == nil { | ||
| return | ||
| } | ||
| if !isCoopCloseSpend(spend.SpendingTx) { |
There was a problem hiding this comment.
Pass the actual input index from the SpendDetail to isCoopCloseSpend to ensure the correct input's sequence number is checked.
| if !isCoopCloseSpend(spend.SpendingTx) { | |
| if !isCoopCloseSpend(spend.SpendingTx, spend.InputIndex) { |
| 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)) |
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.
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
bc783e2 to
82c17a5
Compare
1 participant
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
coopCloseEarlyDispatchedflag on the chain watcher keeps thedispatch idempotent across blockbeat replays of the same spend, and the
closeObserverclears it onnegativeConfChanand on RBF replacementdetection so a re-mined or replacement close still re-fires the
preliminary event with its own summary. The flag rides along on
CooperativeCloseInfo.EarlyDispatchedso the channel arbitrator'shandleCoopCloseEventcan suppress the duplicateNotifyClosedChannelthat
MarkChannelClosedwould otherwise fire at N confs. To make thesuppression possible without breaking force closes,
NotifyClosedChannelis moved out of the
MarkChannelClosedclosure inchain_arbitrator.goand called explicitly by each close handler inchannel_arbitrator.go. The coop handler gates onEarlyDispatched;local force, remote force, and breach handlers always fire it (force
closes intentionally remain on the N-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 (either the same
tx re-mined or a replacement) we fire the early event again with the
new summary. We don't emit a synthetic "un-close" event — clients that
acted on
CLOSED_CHANNELbeforeFULLY_RESOLVED_CHANNELcan re-derivestate from
ListChannelsif needed. This matches the chain semanticsand avoids 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, RBF replacement re-fire, fast-path unchanged, nil-callback
no-op) and the channel arbitrator suppression (coop close gates on
EarlyDispatched, force-close handlers always fire).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 pass.