multi: reorg-aware chain observation (chainsource + backends) - #895
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
@claude review this |
|
Claude finished @ellemouton's task in 1m 40s —— View job Review: reorg-aware chain observationI reviewed the ~9.4k-line diff (
🔴 High1. 🟠 Medium2. 3. 4. 5. 6. 7. 🟡 Low / informational
Not verified here (live outside these files / external deps): ledger idempotency on re-confirm is asserted by comment only; the lnd |
d63bd67 to
3412852
Compare
|
@claude please review this PR one more time for merge readiness. |
|
Claude finished @ellemouton's task in 2m 3s —— View job Merge-readiness re-review
Branch state: no commits have landed since my previous review —
Findings #3, #5, #7 and the Low items are likewise unchanged. Recommendation: not yet — a few in-scope correctness items should land firstThis is careful, well-tested work and the observation core is sound. But three of the open findings are correctness regressions inside the exact plumbing this PR ships (the chain-observation layer), not in the deferred PR-2 scope, and one is a silent data-integrity suppression. I'd treat these as merge blockers: Blockers (in-scope, correctness):
Strongly recommend before merge (data integrity, but touches PR-2 territory):
Can be follow-ups (leak / cosmetic / needs-confirmation):
Bottom line: the design and test coverage are in good shape, but #2, #3 and #4 are in-scope regressions/hangs in the observation layer itself and #1 is a silent divergence — I'd want at least those addressed (or #1 explicitly deferred with a tracked issue + code comment) before merge. Everything else is fair game for a follow-up. If you'd like, comment |
|
Claude finished @ellemouton's task in 3m 43s —— View job Merge-readiness re-review — HEAD
|
| # | Fix (verified in b43d2b3) |
Regression test | Status |
|---|---|---|---|
| 2 | clearFinalityRegistration() now nils a.blockReg on epoch-stream close (direct close and the drainToLatestEpoch close path), then re-arms when confirmHeight != 0. The re-arm guard blockReg == nil && !arming now passes. (conf_actor.go:436-522, spend_actor.go:405-489) |
TestConfActorRearmsFinalityAfterEpochStreamClose, TestSpendActorRearms… |
✅ |
| 3 | Both errChan reader goroutines call cancel() on stream error (lndclient_adapters.go:216, :374). That cancels ctx, forwardOrderedReorg returns, its deferred close(orderedConfirmed) fires → failConfirmation fires downstream. |
TestLndClientConfStreamErrorClosesRegistration, …SpendStream… |
✅ |
| 4 | handleBlockObserved now retries Confirmed entries every tick via an explicit if state == TxStateConfirmed { retryLifecycleNotifications; continue } (actor.go:1282-1286); helper renamed and comments corrected. retryConfirmedRedelivery is live again. |
TestLifecycleDeliveryRetriesAfterTellFailure (now drives an emitBlock tick, not emitConfDone) |
✅ |
| 5 | checkSingleSpend returns a third error value; reorgSpendReg and deliverSpendIfNew bail on error instead of collapsing transient HTTP/parse failure into (nil, {}) → no spurious Reorged. (chain_backend.go:1055-1065, :1491-1545) |
TestChainBackendSpendReorgSkipsTransientStatusFailure |
✅ |
| 1 | The provisional first TxConfirmed no longer commits the store: the Confirmed arm is now log-only, and handleSweepSpendNotification defers the sweep's own-tx input spend (SpendingTxid == pending.txid) until finality (:1093-1112). Store/ledger transitions moved to reconcileSweepInputsOnFinalized + emitSweepConfirmedLedger in the Finalized arm. The TxFailed guard can now only suppress once the record is genuinely terminal-success (finalized, or externally resolved by a real competing spend) — a real post-reorg divergence is no longer masked. |
TestSweepTxNotificationConfirmedRemainsProvisional, …FinalizedEmitsLedger, …FinalizedExternalDestSkipsCreated |
✅ |
| 6 | The Finalized arm now performs the sole cleanup — delete(a.pendingSweeps, txid) + cancelSweepSpendWatches(pending) (which also clears pendingSweepInputs and unregisters the SpendActors). Because own-tx spends are deferred (fix #1), they no longer front-run and orphan this state. |
TestSweepTxNotificationFinalizedCommitsAndCleansUp |
✅ |
No dangling references to the removed reconcileSweepInputsOnConfirm, and the Finalized arm ordering is correct (reconcile reads pending.inputs before cancelSweepSpendWatches clears them). The misleading "priority draining" comment (a prior Low item) was also corrected.
Note: local test execution is blocked in this sandbox (no exec permission for
go test/make unit), so I verified the fixes and their tests by source inspection rather than by running them here. The fixes each carry a targeted regression test and CI runs the full suite.
Recommendation: go — no remaining in-scope blockers
All four items I flagged as merge blockers last round (#2, #3, #4, and the #1 data-integrity suppression) are resolved, and the two leak/false-alarm items (#5, #6) are fixed too. The fixes are minimal, match the intended lifecycle (Confirmed provisional → Finalized commits), and don't introduce new stuck states that I could find.
Follow-up, non-blocking (unchanged from before):
- multi: clean up unnecessary stuff and use instance logging in favor of global logger #7 — verify lnd's
chainntnfsDoneis sent-on vs closed at reorg-safety depth against the pinned revision; thebtcwbackendforwarders (chain_backend.go:466,:579) treat it as value-carrying, and the unit stub uses a value-send. Worth a quick confirmation but outside this diff's control. - The remaining Low/informational items from the first review (
RegisterBlocksrequest-ctx capture inlnd.go,TipPollercontext.Background()in one-shots blockingStop(), theBlockStamp()zero-hash window inesplora_chain.go:787, andlastSeq-starts-at-0 inconf_actor.go) are all self-healing or benign-today and fine as follow-ups.
This is good to merge from a correctness standpoint; human review remains the final gate.
· reorg-observe
7538aa3 to
8fb912d
Compare
8fb912d to
9d42cf2
Compare
|
@claude please re-review the latest head. An independent whole-substrate pass found two additional cases now folded into their owning commits: TipPoller verifies PrevBlock continuity at every height of a multi-block catch-up (including a reorg landing mid-walk), and a late subscriber attaching to an already-finalized tx no longer receives a spurious TxConfirmed on delivery retry. Please focus on those fixes and their regression tests, and flag any remaining merge blocker. |
|
Claude finished @ellemouton's task in 2m 54s —— View job Re-review: latest head — two folded fixes + merge-readiness
I re-read the checked-out
1. TipPoller PrevBlock continuity at every catch-up height ✅
The mid-walk case is handled correctly: on mismatch it re-resolves the live tip at Regression test 2. Late-finalized subscriber: no spurious
|
| # | Check | Status |
|---|---|---|
| 1 | reconcileSweepInputsOnConfirm fully removed (no refs); commit is now log-only, store/ledger transitions live in reconcileSweepInputsOnFinalized + emitSweepConfirmedLedger on the Finalized arm (boarding_sweep_actor.go:1237-1238) |
✅ |
| 2 | clearFinalityRegistration() nils a.blockReg (conf_actor.go:523, spend_actor.go:484) and is called on both epoch-close paths so synthesis can re-arm |
✅ |
| 3 | conf + spend errChan readers call cancel() on stream error (lndclient_adapters.go:216, :374) |
✅ |
| 4 | if state == TxStateConfirmed { retryLifecycleNotifications } on the block-tick path (actor.go:1283-1284) — Confirmed redelivery live again |
✅ |
| 5 | checkSingleSpend returns an explicit error (chain_backend.go:1497) so a transient Esplora failure no longer collapses to a spurious Reorged |
✅ |
| 6 | Finalized arm performs the sole cleanup, correctly ordered — reconcile reads pending inputs (:1237) before cancelSweepSpendWatches clears them (:1243) |
✅ |
Recommendation: go — no remaining in-scope merge blocker
Both folded fixes are minimal, match the intended lifecycle, and carry regression tests that exercise the exact failure they close. I found no new stuck states or ordering regressions.
Non-blocking follow-ups (unchanged): #7 (verify lnd chainntnfs Done is sent-on vs closed against the pinned revision — the btcwbackend forwarders treat it as value-carrying) and the Low/informational items (RegisterBlocks request-ctx capture, TipPoller context.Background() one-shots blocking Stop(), the BlockStamp() zero-hash window, lastSeq-starts-at-0). All self-healing or benign-today.
Human review remains the final merge gate.
· reorg-observe
9d42cf2 to
292394e
Compare
|
The prior I made only that test use a one-hour background interval and explicitly call |
Add the reorg-aware chain-observation substrate the rest of the reorg-safety stack consumes: a seq-ordered conf/spend watch lifecycle (Confirmed -> Reorged -> re-Confirmed -> Done) with height-based finality synthesis for backends that cannot deliver a native Done (gRPC lndclient and lwwallet). Finality synthesis is armed off the select loop, tied to the sub-actor's long-lived context (a per-attempt timeout would tear the in-process block-epoch forwarder down the instant it armed), retried with a capped exponential backoff until it succeeds or the watch's context is cancelled (a single-confirmation tx has no later event to piggy-back a retry on), and evaluated against the best height captured at arm time so a tx already buried past FinalityDepth when the watch arms finalizes immediately instead of hanging for the next block.
Deliver the full TxConfirmed/TxReorged/re-TxConfirmed/TxFinalized/ TxFailed cycle while a terminal seal keeps reversible notifications from trailing finality. Keep the initial TxConfirmed delivery reliable across mailbox failures. A subscriber attaching after finality receives only the authoritative TxFinalized result. This prevents a retry from inventing a provisional confirmation after the transaction has already sealed.
Forward the lnd and lndclient chain-notifier reorg/finality signals (NegativeConf -> reorg) into the reorg-aware chainsource lifecycle with buffered forwarding.
Detect same-height, shorter, deeper, and mid-catch-up reorgs using a bounded hash history and PrevBlock continuity checks at every walked height. Checking every height prevents a chain replacement that lands mid-walk from permanently stitching together blocks from two forks. Publish replacements through one ordered ChainEvent stream. This lets btcwallet see every BlockDisconnected before the new canonical TipBlock events.
Forward Neutrino chain-notifier reorg signals into the reorg-aware chainsource lifecycle.
Consume the reorg-aware substrate at the edges: make the wallet boarding sweep reorg-aware, enable height-based finality on the darepod chainsource actor, add the harness GetRawTransaction / SignedV3Tx helpers, and add the end-to-end reorg systests for chainsource and txconfirm.
292394e to
233abdd
Compare
|
@claude please perform one final whole-PR review of head Independent tracing found that The rewritten head now uses one small |
|
Claude finished @ellemouton's task in 3m 31s —— View job Final whole-PR review — head
|
| Boundary | Positive→Done release | Retain-on-early-Done | Reorg clears pending |
|---|---|---|---|
LND conf (chainbackends/lnd.go:412/430/437) |
✅ | ✅ (event.Done = nil) |
✅ |
LND spend (lnd.go:543/561/568) |
✅ | ✅ | ✅ |
Neutrino conf (btcwbackend/chain_backend.go:635/655/663) |
✅ | ✅ | ✅ |
Neutrino spend (chain_backend.go:759/779/787) |
✅ | ✅ | ✅ |
ConfActor (chainsource/conf_actor.go:347/418/426, helpers :513-551) |
✅ | ✅ (registration.Done = nil) |
✅ |
SpendActor (spend_actor.go:319/399/406) |
✅ | ✅ | ✅ |
I checked the two failure shapes that matter and neither hangs: the deep-observation case (Done+positive both buffered) delivers positive→Done in order and terminates; disabling the drained one-shot (Done = nil) after latching also correctly avoids a spin if the upstream closes rather than sends Done (the open question from finding #7). The actor-level doneOrder is belt-and-suspenders over the forwarder's, and the lastSeq highest-seq-wins gate composes correctly — a stale (discarded) positive does not call ObservePositive, so it can't prematurely release a latched Done.
Prior blockers #1–#6 — all still resolved at this head ✅
Re-verified each survived the squash to 6 commits:
| # | Check at 233abdd6 |
Status |
|---|---|---|
| 1 | reconcileSweepInputsOnConfirm gone; Confirmed arm is log-only (boarding_sweep_actor.go:1192); store/ledger commit + TxFailed-suppression gate moved to the Finalized path so a genuine post-reorg failure is no longer masked (:1237, :1255) |
✅ |
| 2 | clearFinalityRegistration() nils blockReg (conf_actor.go:561, spend_actor.go:497) and runs on both epoch-close paths → synthesis re-arms |
✅ |
| 3 | conf+spend errChan readers cancel() on stream error → failConfirmation/failSpend fire |
✅ |
| 4 | Confirmed redelivery live on the block-tick path (non-terminal Confirmed) |
✅ |
| 5 | checkSingleSpend returns an explicit error → transient Esplora failure no longer collapses to a spurious Reorged |
✅ |
| 6 | Sole pendingSweeps/sub-actor cleanup on the Finalized arm, correctly ordered (reconcile reads pending inputs at :1237 before cancelSweepSpendWatches at :1243); own-tx spends deferred until finality (:1103) so they can't front-run it |
✅ |
Recommendation: go — no remaining in-scope merge blocker
The positive-before-Done invariant is implemented correctly and uniformly across the LND, Neutrino, ConfActor, and SpendActor boundaries, with regression tests forcing Done-first at each layer. All six prior blockers remain fixed. I found no new stuck states, ordering regressions, or hangs in the whole-PR sweep.
Non-blocking follow-ups (unchanged + one new low observation):
- New (low / out-of-scope): once a
Doneis latched (registration.Done = nil/event.Done = nil) and then an intervening reorg clears it viaObserveReorg(), the one-shotDonechannel stays disabled — a native-Done backend (Neutrino) that re-emitsDoneafter the re-confirmation would not be re-observed. This is only reachable whenDonewas buffered before the first positive and a reorg then unwinds a confirmation already at reorg-safety depth — i.e. a reorg deeper than the safety horizon, which is exactly the deferred out-of-scope territory. lndclient/lwwallet are unaffected (finality comes from height synthesis, a separate arm). Worth a one-line comment noting the latched channel is intentionally not re-enabled, but not a blocker. - multi: clean up unnecessary stuff and use instance logging in favor of global logger #7 — verify lnd
chainntnfsDoneis sent-on vs closed at reorg-safety depth against the pinned revision (forwarders now tolerate a close via theDone = nilguard, so this is lower-risk than in the first review, but still worth confirming). - The remaining Low/informational items (
RegisterBlocksrequest-ctx capture inlnd.go,TipPollercontext.Background()one-shots blockingStop(), theBlockStamp()zero-hash window inesplora_chain.go,lastSeq-starts-at-0) are all self-healing or benign-today.
This is good to merge from a correctness standpoint; human review remains the final gate.
· reorg-observe
|
Successfully created backport PR for |
Summary
PR 1 of the six-PR basic-v1 reorg-safety stack tracked by
lightninglabs/lumos#454.
This PR is the chain-observation layer. It makes confirmation and spend
observation reversible end to end:
For
txconfirm:The first positive observation is no longer terminal. Higher layers can keep a
fact provisional, react when it leaves the best chain, and observe it again
after reconfirmation.
What is included
chainsource: multi-shot confirmation/spend actors, reorg and doneevents, ordered delivery, and height-based finality synthesis for transports
that do not provide native
Done. Transport boundaries explicitly defer abuffered
Doneuntil the positive confirmation/spend identity has crossedfirst.
txconfirm: reversible confirmation notifications, a distinct terminalTxFinalizedstate, and correct terminal-only replay for late subscribers.terminating after the first positive event. Today this transport supplies a
payload-less reorg ping, so
chainsourcesynthesizes finality from blockheight.
replacements, then forward disconnected/connected blocks in order.
txconfirmsubscriber to the new four-event lifecycle. Without this,
TxReorgedandTxFinalizedfall through the old mapper as failure-shaped notifications.Broader boarding spend-watch recovery belongs to Client PR 2.
Native LND transport completion
This PR remains compatible with today's released lnd/lndclient by synthesizing
terminal
Doneat thechainsourceboundary. The proper native transport pathis being completed in parallel:
carries confirmation re-org depth and explicit confirmation/spend
Doneonthe
ChainNotifiergRPC streams.exposes those native fields/events through lndclient.
After both land and Wavelength bumps the dependencies, the LND backend can use
native depth/Done directly while height synthesis remains a compatibility
fallback for older transports.
Deliberate boundary
This PR does not implement:
FinalityDepthis configurable observation policy. This PR retains theinterim default of 6 because no one-confirmation lineage is exposed by it.
Client PR 2 makes the shared basic-v1 terminal boundary configurable so a
deployment can choose how deep a reorg remains recoverable.
This is necessary observation plumbing; it does not, by itself, claim
end-to-end reorg safety through the configured policy depth.
Basic-v1 stack
compatibility.
disabled capability.
restart reconciliation, and objective conditional restore.
lineage admission/pre-side-effect gate.
complete evidence/admission/pre-side-effect gates.
test depth, sealing at that policy boundary, and the final S1-S5/R2/F9
contract across both sides.
Generic automatic replay, durable lineage waiters, per-operation usability
depths, and detection after the configured safety horizon remain follow-ups.
Validation
At
233abdd6:go test ./lwwallet ./txconfirm -count=1go test -race ./lwwallet -run TestTipPollerReorgDuringMultiBlockAdvance -count=10go test -race ./lwwallet -run TestTipPollerDeeperForwardReorg -count=100go test -race ./lwwallet -count=10go test -race ./txconfirm -run TestLateFinalizedSubscriberRetrySkipsConfirmed -count=10go test -race ./chainsource ./chainbackends ./btcwbackend -count=5Doneregressions across those three packages,-count=20make lint-changed-local— 0 issuesThe prior
unit-racefailure exposed nondeterminism in the deeper-forwardTipPoller test harness: its background poll could observe the deliberately
mutated stub chain between the test's two intended snapshots. That test now
drives the two observations manually and passed 100 focused race runs plus ten
full-package race runs. Fresh full CI is running for this rewritten head. A
human approving review remains the final merge gate.