chainntnfs: extend scans for earlier height hints

[Roasbeef]

In this PR, we make confirmation notifications independent of the order in which subscribers register height hints.

Root Cause

The TxNotifier coalesces confirmation subscriptions for the same transaction and output script into a single historical scan. Before this change, the first registration selected that scan's lower bound. A later subscriber with an earlier height hint reused the first result without scanning the uncovered prefix.

This became reachable for channel closes after #10331 added a second close-transaction confirmation subscriber that waits for the capacity-scaled confirmation depth, including six confirmations for larger channels. After a restart, the wallet rebroadcaster can register its six-confirmation subscription at the current tip before the chain watcher restores its subscription with the transaction's actual height. If the close transaction was mined below the first hint, the shared scan missed it. Backends without an independent transaction index could not recover the confirmation.

Follow-up state-machine property testing found a second height-hint ownership issue. The final subscriber could cancel after its scan advanced the cached hint, then a reorg and restart could reuse that stale hint and skip a confirmation or spend on the replacement chain. Pending callbacks and Neutrino scan progress could also restore a hint after cancellation.

Changes

• Track the earliest subscriber hint and the earliest dispatched scan height for each confirmation request.
• Dispatch only the uncovered historical prefix when a later subscriber supplies an earlier hint.
• Keep the shared height hint at the earliest outstanding range until all concurrent scans complete.
• Persist an accepted earlier range before dispatch so it survives another restart.
• Preserve bounded supplemental scan ranges in the Neutrino backend.
• Purge confirmation and spend hints after the final subscriber cancels.
• Retain pending scans and known in-memory results for later subscribers while keeping their durable hints purged.
• Keep subscriberless known results indexed until a reorg invalidates them.
• Exclude subscriberless requests from tip-driven hint updates.
• Serialize Neutrino spend-scan progress writes with cancellation.
• Preserve the first result for script-only spend requests so its reorg index remains consistent.
• Add registration-order, scan-completion-order, restart, cancellation, polling, pending-callback, and reorg regression coverage.
• Add a Rapid property that generates two to six subscribers and arbitrary scan-completion orders, then checks that their historical ranges form a gapless, non-overlapping partition and every subscriber receives the same result exactly once.
• Add a second Rapid state machine that varies three confirmation requests and three subscriber slots across registration, cancellation, blocks, reorgs, and notifier restarts. Its oracle checks exact updates, confirmations, negative confirmations, and duplicate delivery.

Test Plan

• go test ./chainntnfs -run 'TestConfirmation(RegistrationOrder|EqualHeightHintsShareScan)$' -count=20
• go test -race ./chainntnfs -run 'TestConfirmation(RegistrationOrder|EqualHeightHintsShareScan)$' -count=10
• go test ./chainntnfs -run '^TestConfirmationRegistrationOrderProperty$' -rapid.checks=1000 -count=1
• go test -race ./chainntnfs -run '^TestConfirmationRegistrationOrderProperty$' -rapid.checks=200 -count=1
• go test ./chainntnfs/... -count=1
• go test ./chainntnfs -run '^TestTxNotifierModelProperty$' -rapid.checks=10000 -count=1
• go test -race ./chainntnfs -run 'TestTxNotifier(Model|Stale|Canceled|Relevant|Repeated)' -rapid.checks=1000 -count=1
• go test -race ./chainntnfs/neutrinonotify -run '^TestCommitSpendHintIfActive$' -count=1
• make itest-only icase=open_psbt_channel_with_unstable_utxos backend=neutrino timeout=15m
• make itest-only icase=open_channel_with_unstable_utxos backend=neutrino timeout=15m
• make itest-only icase=channel_backup-restore_leased_zero_conf backend=neutrino timeout=15m
• make itest-only icase=channel_backup-restore_leased backend=neutrino timeout=15m
• make lint
• go vet ./chainntnfs/...
• go test ./contractcourt -run '^$'

Fixes #11182

[github-actions]

🟠 PR Severity: HIGH

chainntnfs package changes | 3 files | 314 lines changed

🟠 High (2 files)

• chainntnfs/txnotifier.go - core chain-notification confirmation/spend tracking logic (chainntnfs/*)
• chainntnfs/neutrinonotify/neutrino.go - neutrino backend chain notifier implementation (chainntnfs/*)

🟢 Low (1 file)

• chainntnfs/confirmation_registration_order_test.go - new test file only

Analysis

chainntnfs/* is listed as a HIGH severity package (chain notification infrastructure used by wallet, sweeper, and contract resolution logic for confirmation/spend tracking). The two non-test files modified — txnotifier.go and neutrinonotify/neutrino.go — both fall under this package, driving the overall severity to HIGH. The remaining file is a new test and does not affect the classification. Total non-test lines changed (~112) and file count (2) are well below the thresholds for a severity bump.

---

_{To override, add a severity-override-{critical,high,medium,low} label.}

[Roasbeef]

Rebased.

Pushed the property tests a bit harder and found some other lingering edge cases. The bulk of the diff is now extra tests.

[ViktorT-11]

Generally the fixes introduced by this PR looks good from my end 🔥! Adding a few feedback comments below, where especially the first comment is feedback which I think might make sense to address (perhaps in a follow-up PR).

[ViktorT-11]

It looks like the issue we're fixing here in this commit for "Confirmation subscribers", where a second subscriber registering with an earlier hint doesn't result in an actual scan, is also present for "Spend subscribers" below, i.e. via the RegisterSpend API endpoint IIUC.

I.e. a second "Spend subscriber" won't result in a scan if it's hint is earlier than the initial subscriber.
In theory, if future code for some reason introduces a second subscriber, or an external subscriber calls that endpoint with the current chain tip on a restart, this could lead to lnd missing a force-close after the restart if that second subscriber is registered first.

Does it make sense to fix that issue for "Spend subscribers" in this PR as well? Or should we tackle that in a follow-up PR?

[ViktorT-11]

nit: This applies to quite a bit of the test functions in this PR, but IMO I think the docs here is quite lacking, and we'd improve readability if the docs were expanded.

For example this function would benefit from some godocs despite being un-exported as it's quite a big function, as well as from more comments within the function which explains what that part of the function is doing (that applies to quite many test functions within this PR).

[ViktorT-11]

nit: Should this really be a debug log level only? Perhaps a warn level is warranted, given that the message also details that this error doesn't prevent the live notifier from scanning the prefix.

[ziggie1984]

important bug fix

[ziggie1984]

Blocker (IMO): a failed historical rescan permanently pins this counter above zero.

All three backends bail out of the dispatch goroutine on scan error without calling UpdateConfDetails (bitcoindnotify/bitcoind.go:272, btcdnotify/btcd.go:387, neutrinonotify/neutrino.go:456 — the btcd one even carries a TODO(wilmer): add retry logic if rescan fails?). So pendingRescans never unwinds.

Before this PR that just meant "one scan stuck pending". Now it is considerably worse, because this guard and the one at L1015 make the counter authoritative for the whole set:

• the other, successful scan's result is swallowed here and rescanStatus never reaches rescanComplete;
• the persisted hint was already lowered to the earlier subscriber's value at registration (L789);
• unconfirmedRequests() skips non-rescanComplete sets, so nothing ever raises it again.

Net effect: a request that was healthy (rescanComplete, hint tracking tip) can be permanently downgraded to a frozen, very low persisted hint purely because a second subscriber registered with an earlier hint and that prefix scan happened to fail. Every subsequent restart then redoes the full rescan, forever.

The backends need to report dispatch failure back to the notifier (decrement the counter and restore the previous hint), not just log and return.

[ziggie1984]

If this CommitConfirmHint fails we log and carry on, but confSet.rescanStartHeight is lowered unconditionally a few lines down (L840) while the persisted hint keeps the higher value.

The live notifier is fine, but the comment right above says persisting first exists precisely so "a restart before this prefix completes would retain the later cached hint and lose the earlier subscriber's range" — which is exactly what happens on this path. A write failure here silently degrades back to the bug this commit fixes.

Suggest either not lowering rescanStartHeight when the persist fails (so the set stays consistent with what's on disk), or treating it as a hard failure for the supplemental dispatch. Either way this is more than a debug log.

[ziggie1984]

Moving the lock above the hint-cache query closes a real TOCTOU against updateHints, but it also pulls kvdb I/O inside the notifier lock, which is on the block-processing path (ConnectTip/NotifyHeight):

• QueryConfirmHint (read tx) now runs under the lock for every registration;
• the supplemental path at L789 does a CommitConfirmHint under it;
• CancelConf/CancelSpend now do Purge{Confirm,Spend}Hint under it, and those are kvdb.Batch → a bolt read-write transaction that can wait up to MaxBatchDelay before committing.

Two concerns:

1. Latency — every cancel is now a synchronous disk write serialized against block processing.
2. Reentrancy — bolt's write lock is not reentrant, so any caller that registers or cancels from inside an open kvdb.Update on the same DB would now self-deadlock. Previously RegisterConf never wrote the hint cache, so this hazard is new. I spot-checked the call sites in contractcourt/* and peer/brontide.go:4947 and they look clean, but I didn't trace every closure — worth an explicit audit.

The purge in particular looks like it could happen outside the lock.

[ziggie1984]

Purging the hint when the last subscriber goes away is the right call for correctness, but it's worth being explicit about the cost: after this, re-registering the same request rescans from the caller's original (possibly very old) hint rather than resuming.

For subsystems that cancel and re-register the same request during normal operation (contract resolvers, chain watcher), that could turn into repeated multi-thousand-block rescans. Would it be viable to keep the hint but mark it unowned — i.e. usable as a floor but not trusted as proof of scan progress — instead of deleting it outright?

Not blocking, but I think it deserves a sentence in the commit message either way.

[ziggie1984]

Minor asymmetry with the rescanComplete break at L709, which additionally requires confSet.details == nil before taking the supplemental path.

I couldn't construct a reachable rescanPending && details != nil state (the tip handler sets both together, and L1026 sets rescanComplete alongside the details), so I don't think this is a live bug — but the two guards should probably read the same so a future change can't diverge them.

[ziggie1984]

When the confirmation is already deeper than reorgSafetyLimit, this skips the index insert, so the set is cached with details but never indexed — and ConnectTip's maturity pruner only walks confsByInitialHeight. The subscriberless set then stays in confNotifications for the lifetime of the process.

master leaked these too (it never deleted sets on cancel at all), so this isn't a regression — but since this commit is specifically about aligning set lifetime with ownership, it seems worth closing here rather than leaving one path that still never gets collected.

[ziggie1984]

This index insert is missing the reorgSafeHeight > n.currentHeight guard that both of its siblings have — the conf-side equivalent at L1050 and dispatchSpendDetails at L1640.

Without it, a historical scan that lands a spend older than reorgSafetyLimit inserts into a spendsByHeight bucket whose maturity height has already passed. ConnectTip never sweeps it again and DisconnectTip only walks the height being disconnected, so both the spendsByHeight entry and the spendNotifications entry stay for the lifetime of the process.

Looks like a straightforward oversight — same guard as the other two should do it.

[ziggie1984]

ntfn.Event.Cancel is reassigned here after RegisterSpend has already inserted the event into the TxNotifier's internal map, and without holding the notifier lock. That's an unsynchronized write to a field of an object that is already reachable from shared state.

It's benign today — I grepped and nothing inside chainntnfs calls Event.Cancel, which is why -race stays quiet — but it's a race the moment anything on the notifier side ever does, and that's a non-obvious invariant for a future change to preserve.

Cheapest fix is to build the wrapper before calling RegisterSpend, or to carry the cancellation flag on the registration struct instead of patching the published event.

[ziggie1984]

This holds scanMtx across originalCancel(), which goes CancelSpend → n.Lock() → PurgeSpendHint (a bolt RW transaction).

There's a pre-existing hazard where ConnectTip can block holding n.Lock() on a client that isn't draining its Event.Spend channel. If that same client is the one calling Cancel(), it now blocks holding scanMtx, which in turn wedges the neutrino historical-rescan goroutine in commitSpendHintIfActive. The client/ConnectTip interaction isn't new, but dragging the rescan goroutine into it is.

An atomic bool (or a try-lock on the progress write) would give the same ownership boundary without holding a mutex across a lock acquisition and a disk write.

[ziggie1984]

This infers "initial scan" from an equality that can hold accidentally rather than from anything the TxNotifier actually states.

If a set previously took the startHeight > currentHeight path, rescanStartHeight is set to currentHeight + 1, so a later supplemental dispatch gets EndHeight == currentHeight == ntfn.Height and is treated as initial (extended to the neutrino tip). It happens to be harmless in that specific case — nothing had been scanned, so extending is correct — but it's an implicit coupling between two files that is easy to break.

An explicit Supplemental bool (or similar) on HistoricalConfDispatch would make the contract legible from both sides.