multi: improve resolution lifecycle safety - #11054
Conversation
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (10 files)
🟠 High (1 file)
AnalysisThis PR touches multiple To override, add a |
34933ca to
7c564ec
Compare
7c564ec to
6e0fff8
Compare
6e0fff8 to
ead9dea
Compare
ead9dea to
da4a243
Compare
da4a243 to
13a152e
Compare
e07d220 to
0ced69c
Compare
ae62e91 to
6071a7f
Compare
gijswijs
left a comment
gijswijs
left a comment
There was a problem hiding this comment.
Did a first pass. Have made some inline comments throughout.
But the commit structure is not sound yet. Also six of nineteen commits don't build or pass their own tests: 7a8e9a2, 35c7d37, c9f1363, fc83d2d, d0d8bc9, 66af0c9.
Three broken windows, each repaired one commit later: #2→#3, #12–14→#15, #16–17→#18.
The linter isn't happy. There are a few lines too long.
| "request %v", details.SpendingHeight, spendRequest) | ||
|
|
||
| spendSet.details = details | ||
| n.trackSpendByHeight(spendRequest, details) |
There was a problem hiding this comment.
This change and the one at L1410 is a fix of the previous commit. There are a few of those in this commit. I'll comment on each of them. Please update the commit structure.
There was a problem hiding this comment.
Addressed in the rebuilt stack: candidate indexing and reorg reset are isolated in chainntnfs: reset immature spends, while client scheduling remains in chainntnfs: schedule spend maturity.
|
|
||
| // TODO(wilmer): cancel pending historical rescans if any? | ||
| spendSet := n.spendNotifications[spendRequest] | ||
|
|
There was a problem hiding this comment.
I would separate this spendSet.details != nil guard out as it's own commit.
potential commit message: chainntnfs: keep the first canonical spend candidate immutable
There was a problem hiding this comment.
Addressed as suggested: the immutable first-candidate guard and its ownership regression are isolated in chainntnfs: preserve spend candidate.
| // We'll note the spending height of the request in order to correctly | ||
| // handle dispatching notifications when the spending transactions gets | ||
| // reorged out of the chain. | ||
| spendHeight := uint32(details.SpendingHeight) | ||
| opSet, exists := n.spendsByHeight[spendHeight] | ||
| if !exists { | ||
| opSet = make(map[SpendRequest]struct{}) | ||
| n.spendsByHeight[spendHeight] = opSet | ||
| // Queue tip candidates even at depth one so callers cannot | ||
| // observe them until NotifyHeight completes the block. | ||
| if err := n.scheduleSpendNtfn(ntfn, details, true); err != nil { | ||
| Log.Errorf("Unable to schedule spend for %v: %v", | ||
| spendRequest, err) | ||
| } | ||
| } | ||
| opSet[spendRequest] = struct{}{} | ||
|
|
||
| Log.Debugf("Spend request %v spent at tip=%d", spendRequest, | ||
| spendHeight) | ||
| details.SpendingHeight) |
There was a problem hiding this comment.
This whole change should be folded into the previous commit.
There was a problem hiding this comment.
Addressed in the rebuilt stack: the notification scheduling change is folded into chainntnfs: schedule spend maturity; candidate indexing remains separately scoped to reorg reset.
| spendSet := n.spendNotifications[op] | ||
| // Remove each client maturity before clearing the shared | ||
| // candidate. Its inclusion height selects the bucket. | ||
| for _, ntfn := range spendSet.ntfns { | ||
| n.removeSpendMaturity(ntfn) | ||
| } |
There was a problem hiding this comment.
This is also a fix of the previous commit. Fold it in.
There was a problem hiding this comment.
Addressed: maturity-index cleanup during disconnect is now part of the scheduling commit that introduces that index.
| spendHeight := uint32(details.SpendingHeight) | ||
|
|
||
| // We also add to spendsByHeight to notify on chain reorgs. | ||
| reorgSafeHeight := spendHeight + n.reorgSafetyLimit | ||
| if reorgSafeHeight > n.currentHeight { | ||
| txSet, exists := n.spendsByHeight[spendHeight] | ||
| if !exists { | ||
| txSet = make(map[SpendRequest]struct{}) | ||
| n.spendsByHeight[spendHeight] = txSet | ||
| } | ||
| txSet[ntfn.SpendRequest] = struct{}{} | ||
| } | ||
|
|
There was a problem hiding this comment.
iiuc the block was put there deliberately by 2fbee31 ("chainntnfs: populate spendsByHeight during historical dispatch").
The hole: if the historical rescan completes after the last client cancelled, updateSpendDetails sets spendSet.details but the dispatch loop body never runs, so nothing is tracked — and DisconnectTip can then never invalidate those cached details. A later client registering hits the rescanComplete branch and receives a stale spend. Moving the tracking to candidate-discovery time closes that, independently of the depth feature.
So I agree that this should be removed here. I think this should be the entire 3rd commit tho. Everything else should be folded into the previous commit, or separated out into its own commit.
There was a problem hiding this comment.
Addressed: discovery-time candidate indexing and its disconnect reset now form the focused chainntnfs: reset immature spends commit; scheduling changes were folded into the preceding commit.
| t.Fatalf("historical spend did not mature") | ||
| } | ||
|
|
||
| var cachedClient *chainntnfs.SpendEvent |
There was a problem hiding this comment.
This lands on spendConfirmHeight == currentHeight, so only the == half of scheduleSpendNtfn's condition is covered. The < branch (a client registering well after maturity, i.e. the restart path) is untested. Mining one extra block before this registration would cover it.
There was a problem hiding this comment.
Addressed: the historical backend test mines an extra block after maturity before registering cachedClient, explicitly exercising the < currentHeight cached/restart path.
| the reported network statistics such as total network capacity, channel | ||
| count and max out degree. | ||
|
|
||
| * [Fixed a reorganization safety |
There was a problem hiding this comment.
nit: Again about commit structure, but can this be a separate commit and the last one of the PR?
There was a problem hiding this comment.
Addressed: docs: note spend lifecycle safety is a separate signed commit and is the final commit in the 22-commit stack.
| // A missing-input response can describe a shallow confirmation that | ||
| // the publisher observes before the depth-aware input monitor. Retry | ||
| // the set on the next block and let that monitor decide terminality. | ||
| if resp.deferTerminal && errors.Is(r.Err, ErrInputMissing) { |
There was a problem hiding this comment.
The comment says this covers "a shallow confirmation that the publisher observes before the depth-aware input monitor", but handleMissingInputs only emits TxFatal/ErrInputMissing when no spend was found at all — its own comment calls that "an orphan tx that's no longer valid, e.g. the anchor output from the remote commitment after the local commitment has confirmed". For a genuinely orphaned input the outpoint doesn't exist on chain, so the terminal observer will never fire; the input cycles PublishFailed → re-cluster → ErrInputMissing every block forever, no Result ever reaches the resolver, and each attempt leaks a monitorFeeBumpResult goroutine. There's no attempt counter (publishAttempts is reported, never enforced) and no timeout. Master marks it Fatal and the resolver moves on.
Suggested fix: record a firstMissingHeight on SweeperInput and fall through to markInputsFatal once currentHeight - firstMissing >= RequiredConfs.
There was a problem hiding this comment.
Addressed: missingSince bounds missing-input deferral by RequiredConfs, then falls through to terminal failure; sweep: test bounded orphan recovery covers the terminal boundary.
| // Keep this provisionally spent input quarantined while | ||
| // its terminal observer waits for the requested depth. | ||
| // A shallow reorg requeues this outpoint independently. | ||
| input.state = Published |
There was a problem hiding this comment.
Unconditional. If the shallow observer's reorg already moved the input to PublishFailed, a racing TxUnknownSpend puts it back to Published, where updateSweeperInputs skips it. At that point nothing can wake it: the publisher record is gone, the terminal observer won't fire because the spend has vanished, and the shallow observer won't re-fire because dispatchSpendReorg already reset dispatched (chainntnfs/txnotifier.go:2246). Stranded until restart.
Fix:
Snapshot before the flatten in line 2132:
// Snapshot inputs a reorg observer has already requeued, so the
// quarantine below cannot undo that requeue with stale spend evidence.
requeued := make(map[wire.OutPoint]struct{})
for _, inp := range r.set.Inputs() {
if pi, ok := s.inputs[inp.OutPoint()]; ok &&
pi.state == PublishFailed {
requeued[inp.OutPoint()] = struct{}{}
}
}
s.markInputsPublishFailed(r.set, r.result.FeeRate)
And then here:
if _, wasRequeued := requeued[op]; !wasRequeued &&
input.params.RequiredConfs > DefaultRequiredConfs {
input.state = Published
continue
}
There was a problem hiding this comment.
Addressed: handleBumpEventTxUnknownSpend snapshots PublishFailed inputs before flattening the batch and does not restore a reorg-requeued input to Published.
| // Subscribe at the selected depth so only the block path controls | ||
| // terminality. The mempool path can reveal a preimage early because | ||
| // that secret remains actionable across a reorg. | ||
| blockSpent, err := h.Notifier.RegisterSpendNtfn( |
There was a problem hiding this comment.
The split subscription only happens when h.Mempool == nil. On btcd/bitcoind this registers the block client at spendConfDepth, and relies on the mempool subscription for early preimage discovery — but that fallback is empty exactly when it matters: a preimage spend that is already confirmed at registration time was never in our mempool.
That's the common case, not an edge case. The chain watcher itself gates force-close dispatch on spendConfDepth, so resolvers launch 2-5 blocks after the commitment confirms, and any restart re-registers against an already-mined spend.
So resolveTimeoutTx and resolveRemoteCommitOutput can still delay claimCleanUp -> AddPreimages -> DeliverResolutionMsg by up to SpendConfDepth - 1 blocks on the majority backend, while neutrino now learns it at depth 1. The fix landed on the backend that needed it less.
Worth noting the contradiction is internal to this file: the mempool arm of consumeSpendEvents acts on a preimage at zero confirmations, from a tx that may never confirm, while the block path on the same outpoint is gated at 3-6. A preimage in a block is strictly stronger evidence than one in a mempool.
Could waitForPreimageOrMatureSpend's two-client structure be used on both paths? The mempool subscription then becomes an optimisation rather than the only early channel.
There was a problem hiding this comment.
Addressed: every deep policy now enters waitForPreimageOrMatureSpend before backend selection, registering a one-confirmation block observer plus the terminal observer; mempool delivery is only an optimization.
Keep canonical spend candidates separate from client delivery. Clear queued maturity entries when a spending block disconnects so a replacement can be discovered without notifying immature clients.
Exercise independent one- and multi-confirmation clients, historical spend delivery, reorg replacement, and cancellation at the maturity boundary.
Keep the first same-script spend immutable until it disconnects or ages out. Cover candidate ownership, cancellation turnover, and depth validation so queued clients cannot inherit a later immature spend.
Run future spend maturity and boundary checks through the shared notifier suite so every backend forwards the explicit registration option for live transactions.
Run historical, cached, reorg, and cancellation maturity cases through the backend-neutral notifier suite without exceeding the per-commit review budget.
Derive one validated channel-level confirmation depth and pass it to the funding watcher and resolvers. This keeps existing close behavior while giving terminal lifecycle consumers one policy authority.
Attach an immutable confirmation depth to each admitted input and use it for the input spend registration. Reject invalid or conflicting updates without stopping the collector.
Apply the channel spend depth to resolver terminal waits and sweep requests. Keep preimage observations and synchronous preparatory lookups at one confirmation because those actions remain reorg-safe.
Carry the channel maturity policy into live Nursery registrations and reconstruct it from the closed-channel summary after restart. Apply it to promotion and sweep handoffs without changing persisted formats.
26abe2f to
65257de
Compare
Clamp late Crib promotions to the next unprocessed class and reject replayed incubation once the same output has advanced to Kindergarten. This keeps persisted claims reachable across restart and shallow reorg.
Verify actionable preimages and preparatory lookups remain available at one confirmation while terminal resolver waits use the channel policy. Assert exact notifier options and registration cleanup.
Verify Preschool and Crib promotion depths, restart height clamping, and final sweep policy reconstruction with exact mock expectations and controlled goroutine shutdown.
Treat publisher confirmations and missing-input observations as provisional for deeper inputs. Finalize only the outpoint backed by its mature notifier event and retain rebroadcast through shallow reorgs.
Verify mature spend evidence finalizes and cleans only its notified outpoint, while deeper siblings remain publishable. Assert the input monitor registers the admitted confirmation depth.
Cover inherited and conflicting input policies, provisional publisher confirmation handling, and admission rejection for unsupported depths with exact mock cardinality.
Exercise a legacy Nursery final sweep through shallow confirmation, reorg, node restart, rebroadcast, and terminal maturity using existing lntest helpers and no new harness API.
Bound missing-input retries while shallow discovery is pending. Keep unknown-spend evidence authoritative across retry fee errors. Terminalize only viable siblings.
Observe first-level breached HTLC transitions at one confirmation so justice can follow the new outpoint promptly. Keep terminal cleanup behind the maximum channel-close depth so a shallow reorg cannot discard persisted recovery ownership.
Carry shallow and terminal subscriptions under one recovery owner. Surface lifecycle metadata through the serialized waiter. Terminal mutation semantics remain unchanged.
Observe time-sensitive first-level HTLC spends at one confirmation. Redirect justice through an in-memory candidate. Retain the original recovery output until terminal depth or reorg.
Exercise provisional justice spends independently from terminal spends and verify that first-level HTLC transitions register both early and mature observers. Pin the confirmation policy used by each lifecycle path.
Document that contract resolvers and the sweeper now wait for the channel's configured close depth before irreversible spend handling.
65257de to
01d71df
Compare
|
@yyforyongyu, remember to re-request review from reviewers when ready |
6 participants
Summary
Prevent contract resolutions and owned sweeps from finalizing on shallow spends that can be reorged.
Change Description
Notes
This is the preserved PR 3 of 3 for the SI #95 resolution-lifecycle fix. It depends on PR #11147 and the forthcoming PR 2 sweep lifecycle layer. Until those prerequisites merge, this PR remains draft and retains the full stack to preserve its review history. After PR 2 merges, this branch will be rebased and reduced to the final resolver, Nursery, and breach-recovery consumer layer before being marked ready.