multi: Reorg-aware chainsource + chainbackends + txconfirm - #422
multi: Reorg-aware chainsource + chainbackends + txconfirm#422ellemouton wants to merge 1 commit into
Conversation
![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 implements reorg-awareness and finality tracking throughout the chain monitoring subsystem. Key changes include updating the LND backend and lndclient adapters to propagate reorg and completion signals, and enhancing ConfActor and SpendActor to process these events while synthesizing finality based on block height when backends do not provide a native signal. The TxBroadcasterActor was also updated to support reversible confirmation states and a terminal finalized state. Review feedback identifies a high-severity issue where notifying subscribers via blocking Tell calls could stall the main actor loop, as well as medium-severity concerns regarding potential resource leaks if block subscriptions fail during finality synthesis.
|
@claude review this |
|
Claude finished @ellemouton's task in 4m 45s —— View job PR Review: multi: Reorg-aware chainsource + chainbackends + txconfirmTasks
Overall, the design is sound and the implementation is well-structured. The lifecycle shape ( [Critical]
|
1450ae5 to
92de073
Compare
92de073 to
29b9872
Compare
|
@claude re review |
|
Claude finished @ellemouton's task in 4m 17s —— View job Re-review: multi: Reorg-aware chainsource + chainbackends + txconfirm Tasks
Status of previous review findingsEvery critical and high issue from the first pass has been addressed. Here is the full reconciliation: [Critical] FinalityDepth not wired in darepod → FIXED
[High] Blocking
[Medium] Block subscription failure silently breaks finality synthesis → FIXED
[Medium] Backend Done dropping when state is not Confirmed → ADDRESSED WITH DOCUMENTATION The behaviour is retained intentionally (a hypothetical future backend that fires Done during a reorg gap would render the watch unrecoverable, so the warn-and-drop with a comment naming the fix is the right tradeoff). The long multi-paragraph comment in [Low] SpendReorg struct unreferenced → NON-ISSUE (misidentification) The type in the previous review was confused with [Low] txconfirm/CLAUDE.md not updated → FIXED
Remaining finding[Low]
The chainsource package is the foundation of the reorg-safety stack; callers who read its CLAUDE.md to understand the registration contract will see an incomplete picture of what the channels now carry. Minor observations (not blocking)
VerdictThe implementation is correct and complete. All structural issues identified in the first review are resolved. The remaining |
53b0b2d to
4b1f063
Compare
Make confRegistration / spendRegistration multi-shot reorg-aware:
- Add Reorged / Done channels alongside Confirmed / Spend on the
returned chainsource.ConfRegistration and SpendRegistration so
the backend now satisfies the multi-shot contract that PR #422
introduced for the LND-backed backend.
- Track a per-registration state (stateWatching | statePositive)
plus the last-delivered block hash so the reorg handler can
decide whether a given reorg invalidates a previous delivery.
- Stop deleting registrations on the first positive event. A
confirmation that survives a future reorg is now repeatedly
re-deliverable on the same registration; the chainsource conf /
spend actor synthesizes Done at FinalityDepth from block epochs.
Subscribe to the TipPoller's new ReorgSubscribe stream alongside the
existing TipBlock stream via BestBlockAndSubscribeAll. When a
ReorgEvent arrives, walk every active conf / spend registration and:
- For each one in statePositive whose last block hash is in the
disconnected set, fire Reorged (non-blocking; the channel is
buffered to 1 and a coalesced reorg signal is semantically
correct -- the consumer re-queries either way), reset state to
stateWatching, and re-check status against the new chain so a
re-confirmation / re-spend fires Confirmed / Spend immediately
in the same handler turn.
- For registrations whose last block hash was never resolved
(typically a transient Esplora failure at delivery time) we
leave them alone; the broad tip-driven re-check still runs and
will catch up on the next block.
Cancel now uses a regMu-protected once-style check so a
double-Cancel is a safe no-op. Channel sends are performed outside
regMu so a slow consumer never blocks the broad re-check goroutine.
Make confRegistration / spendRegistration multi-shot reorg-aware:
- Add Reorged / Done channels alongside Confirmed / Spend on the
returned chainsource.ConfRegistration and SpendRegistration so
the backend now satisfies the multi-shot contract that PR #422
introduced for the LND-backed backend.
- Track a per-registration state (stateWatching | statePositive)
plus the last-delivered block hash so the reorg handler can
decide whether a given reorg invalidates a previous delivery.
- Stop deleting registrations on the first positive event. A
confirmation that survives a future reorg is now repeatedly
re-deliverable on the same registration; the chainsource conf /
spend actor synthesizes Done at FinalityDepth from block epochs.
Subscribe to the TipPoller's new ReorgSubscribe stream alongside the
existing TipBlock stream via BestBlockAndSubscribeAll. When a
ReorgEvent arrives, walk every active conf / spend registration and:
- For each one in statePositive whose last block hash is in the
disconnected set, fire Reorged (non-blocking; the channel is
buffered to 1 and a coalesced reorg signal is semantically
correct -- the consumer re-queries either way), reset state to
stateWatching, and re-check status against the new chain so a
re-confirmation / re-spend fires Confirmed / Spend immediately
in the same handler turn.
- For registrations whose last block hash was never resolved
(typically a transient Esplora failure at delivery time) we
leave them alone; the broad tip-driven re-check still runs and
will catch up on the next block.
Cancel now uses a regMu-protected once-style check so a
double-Cancel is a safe no-op. Channel sends are performed outside
regMu so a slow consumer never blocks the broad re-check goroutine.
Make confRegistration / spendRegistration multi-shot reorg-aware:
- Add Reorged / Done channels alongside Confirmed / Spend on the
returned chainsource.ConfRegistration and SpendRegistration so
the backend now satisfies the multi-shot contract that PR #422
introduced for the LND-backed backend.
- Track a per-registration state (stateWatching | statePositive)
plus the last-delivered block hash so the reorg handler can
decide whether a given reorg invalidates a previous delivery.
- Stop deleting registrations on the first positive event. A
confirmation that survives a future reorg is now repeatedly
re-deliverable on the same registration; the chainsource conf /
spend actor synthesizes Done at FinalityDepth from block epochs.
Subscribe to the TipPoller's new ReorgSubscribe stream alongside the
existing TipBlock stream via BestBlockAndSubscribeAll. When a
ReorgEvent arrives, walk every active conf / spend registration and:
- For each one in statePositive whose last block hash is in the
disconnected set, fire Reorged (non-blocking; the channel is
buffered to 1 and a coalesced reorg signal is semantically
correct -- the consumer re-queries either way), reset state to
stateWatching, and re-check status against the new chain so a
re-confirmation / re-spend fires Confirmed / Spend immediately
in the same handler turn.
- For registrations whose last block hash was never resolved
(typically a transient Esplora failure at delivery time) we
leave them alone; the broad tip-driven re-check still runs and
will catch up on the next block.
Cancel now uses a regMu-protected once-style check so a
double-Cancel is a safe no-op. Channel sends are performed outside
regMu so a slow consumer never blocks the broad re-check goroutine.
…connector-radix server: Fix fraud-response safety gate connector radix
aebb4c7 to
3877221
Compare
Make confRegistration / spendRegistration multi-shot reorg-aware:
- Add Reorged / Done channels alongside Confirmed / Spend on the
returned chainsource.ConfRegistration and SpendRegistration so
the backend now satisfies the multi-shot contract that PR #422
introduced for the LND-backed backend.
- Track a per-registration state (stateWatching | statePositive)
plus the last-delivered block hash so the reorg handler can
decide whether a given reorg invalidates a previous delivery.
- Stop deleting registrations on the first positive event. A
confirmation that survives a future reorg is now repeatedly
re-deliverable on the same registration; the chainsource conf /
spend actor synthesizes Done at FinalityDepth from block epochs.
Subscribe to the TipPoller's new ReorgSubscribe stream alongside the
existing TipBlock stream via BestBlockAndSubscribeAll. When a
ReorgEvent arrives, walk every active conf / spend registration and:
- For each one in statePositive whose last block hash is in the
disconnected set, fire Reorged (non-blocking; the channel is
buffered to 1 and a coalesced reorg signal is semantically
correct -- the consumer re-queries either way), reset state to
stateWatching, and re-check status against the new chain so a
re-confirmation / re-spend fires Confirmed / Spend immediately
in the same handler turn.
- For registrations whose last block hash was never resolved
(typically a transient Esplora failure at delivery time) we
leave them alone; the broad tip-driven re-check still runs and
will catch up on the next block.
Cancel now uses a regMu-protected once-style check so a
double-Cancel is a safe no-op. Channel sends are performed outside
regMu so a slow consumer never blocks the broad re-check goroutine.
Make confRegistration / spendRegistration multi-shot reorg-aware:
- Add Reorged / Done channels alongside Confirmed / Spend on the
returned chainsource.ConfRegistration and SpendRegistration so
the backend now satisfies the multi-shot contract that PR #422
introduced for the LND-backed backend.
- Track a per-registration state (stateWatching | statePositive)
plus the last-delivered block hash so the reorg handler can
decide whether a given reorg invalidates a previous delivery.
- Stop deleting registrations on the first positive event. A
confirmation that survives a future reorg is now repeatedly
re-deliverable on the same registration; the chainsource conf /
spend actor synthesizes Done at FinalityDepth from block epochs.
Subscribe to the TipPoller's new ReorgSubscribe stream alongside the
existing TipBlock stream via BestBlockAndSubscribeAll. When a
ReorgEvent arrives, walk every active conf / spend registration and:
- For each one in statePositive whose last block hash is in the
disconnected set, fire Reorged (non-blocking; the channel is
buffered to 1 and a coalesced reorg signal is semantically
correct -- the consumer re-queries either way), reset state to
stateWatching, and re-check status against the new chain so a
re-confirmation / re-spend fires Confirmed / Spend immediately
in the same handler turn.
- For registrations whose last block hash was never resolved
(typically a transient Esplora failure at delivery time) we
leave them alone; the broad tip-driven re-check still runs and
will catch up on the next block.
Cancel now uses a regMu-protected once-style check so a
double-Cancel is a safe no-op. Channel sends are performed outside
regMu so a slow consumer never blocks the broad re-check goroutine.
c86210e to
1ab3019
Compare
Make confRegistration / spendRegistration multi-shot reorg-aware:
- Add Reorged / Done channels alongside Confirmed / Spend on the
returned chainsource.ConfRegistration and SpendRegistration so
the backend now satisfies the multi-shot contract that PR #422
introduced for the LND-backed backend.
- Track a per-registration state (stateWatching | statePositive)
plus the last-delivered block hash so the reorg handler can
decide whether a given reorg invalidates a previous delivery.
- Stop deleting registrations on the first positive event. A
confirmation that survives a future reorg is now repeatedly
re-deliverable on the same registration; the chainsource conf /
spend actor synthesizes Done at FinalityDepth from block epochs.
Subscribe to the TipPoller's new ReorgSubscribe stream alongside the
existing TipBlock stream via BestBlockAndSubscribeAll. When a
ReorgEvent arrives, walk every active conf / spend registration and:
- For each one in statePositive whose last block hash is in the
disconnected set, fire Reorged (non-blocking; the channel is
buffered to 1 and a coalesced reorg signal is semantically
correct -- the consumer re-queries either way), reset state to
stateWatching, and re-check status against the new chain so a
re-confirmation / re-spend fires Confirmed / Spend immediately
in the same handler turn.
- For registrations whose last block hash was never resolved
(typically a transient Esplora failure at delivery time) we
leave them alone; the broad tip-driven re-check still runs and
will catch up on the next block.
Cancel now uses a regMu-protected once-style check so a
double-Cancel is a safe no-op. Channel sends are performed outside
regMu so a slow consumer never blocks the broad re-check goroutine.
Review:
|
92f45bd to
bc869df
Compare
|
Addressed the automated review in 5321ad4 + bc869df (pushed to 🔴 Cross-channel ordering hazard at the lndclient bridge — fixed. lndclient drives the whole confirmation lifecycle off a single ordered gRPC receive loop, but the adapter split it across two channels (the confirmation channel and the Fix: forward the whole lifecycle through a single goroutine ( 🟡 Unbounded Verification:
|
Review: reorg-safe chain observationThanks for the thorough PR description and the careful split between the observation substrate and the later canonicality layer — it made reviewing the intent much easier. The actor lifecycle changes, the reversible-vs-terminal notification split in 🔴 High1. In-process lnd forwarder doesn't preserve Reorged-before-reconfirm ordering — the exact bug the lndclient path was built to avoid. The The in-process 🟡 Medium2. Spend actor is missing the
3. Conf-watch leak on The terminal branch stops the FSM and evicts broadcaster state but never calls 4. Reversible Each reversible event fans out on a fresh per-subscriber goroutine with no per-subscriber ordering, so a 🟢 Low / nits
TestsCoverage is strong — reorg lifecycle unit tests across both actors, height-based Nice work overall — the high/medium items are mostly about bringing the in-process path and the spend actor up to the same discipline the lndclient/conf paths already establish. 🤖 Generated with Claude Code |
Review: Reorg-aware chainsource + chainbackends + txconfirmThanks for the thorough write-up — the layered framing (observation substrate now, batch/VTXO interpretation later) is clear and the docs/CLAUDE.md updates are excellent. The state machines in The recurring risk across this PR is event ordering where reorg signals travel a different path than the confirmation they revert. A few of these are real correctness bugs. Findings below, most-severe first. 🔴 High —
|
|
Addressed the second review pass (the 🔴 ordering hazards + the medium/low findings). Pushed 6 commits to The ordering fix is now correct for both interleavings (
|
Review: reorg-safe chain observation (#422)Reviewed the full diff with focus on the concurrency-heavy core (
No high- or medium-severity correctness bugs surfaced. The items below are low-severity robustness/observability nits. Low — worth addressing
Low — observability / nits
TestsCoverage is strong: forwarder reorg lifecycle tests, height-based Nice work on the documentation density in the forwarder and the FSM — the inline rationale (defer ordering, seq authority, buffer sizing) made this much easier to review. 🤖 Generated with Claude Code |
|
@claude please re-review. The 6 new commits (
Please focus on the seq-ordering approach in |
|
Claude finished @ellemouton's task in 5m 14s —— View job Re-review: seq-ordering (
|
3eb6f61 to
449b6cf
Compare
Review:
|
Make confRegistration / spendRegistration multi-shot reorg-aware:
- Add Reorged / Done channels alongside Confirmed / Spend on the
returned chainsource.ConfRegistration and SpendRegistration so
the backend now satisfies the multi-shot contract that PR #422
introduced for the LND-backed backend.
- Track a per-registration state (stateWatching | statePositive)
plus the last-delivered block hash so the reorg handler can
decide whether a given reorg invalidates a previous delivery.
- Stop deleting registrations on the first positive event. A
confirmation that survives a future reorg is now repeatedly
re-deliverable on the same registration; the chainsource conf /
spend actor synthesizes Done at FinalityDepth from block epochs.
Subscribe to the TipPoller's new ReorgSubscribe stream alongside the
existing TipBlock stream via BestBlockAndSubscribeAll. When a
ReorgEvent arrives, walk every active conf / spend registration and:
- For each one in statePositive whose last block hash is in the
disconnected set, fire Reorged (non-blocking; the channel is
buffered to 1 and a coalesced reorg signal is semantically
correct -- the consumer re-queries either way), reset state to
stateWatching, and re-check status against the new chain so a
re-confirmation / re-spend fires Confirmed / Spend immediately
in the same handler turn.
- For registrations whose last block hash was never resolved
(typically a transient Esplora failure at delivery time) we
leave them alone; the broad tip-driven re-check still runs and
will catch up on the next block.
Cancel now uses a regMu-protected once-style check so a
double-Cancel is a safe no-op. Channel sends are performed outside
regMu so a slow consumer never blocks the broad re-check goroutine.
Review:
|
ded7b90 to
32871de
Compare
Add the reorg-aware chain-observation substrate the rest of the
reorg-safety stack consumes:
- chainsource: reorg-aware, seq-ordered conf/spend watch lifecycle
(Confirmed -> Reorged -> re-Confirmed -> Done) with height-based
finality synthesis for backends that cannot deliver Done, armed
off the select loop and tied to the sub-actor's long-lived ctx.
- chainbackends: forward lnd and lndclient reorg/finality signals
(NegativeConf -> reorg) with buffered forwarding.
- txconfirm: reorg-aware confirmation watches delivering the full
TxConfirmed/TxReorged/re-TxConfirmed/TxFinalized/TxFailed cycle,
a terminal seal guarding reversible fire-and-forget deliveries,
and a catch-up TxReorged when a reorg lands while a subscriber's
initial TxConfirmed is still parked on the async notify path.
- wallet: make the boarding sweep reorg-aware.
- darepod: enable height-based finality on the chainsource actor.
- harness: GetRawTransaction and SignedV3Tx helpers; end-to-end
reorg tests for chainsource and txconfirm.
Squashed and ported to the btcd v2 module set for the rebase onto
main.
32871de to
4a1f9b1
Compare
Review: Reorg-aware chainsource + chainbackends + txconfirmThorough, well-documented foundation PR. The design rationale in the comments and the A few things worth a second look before merge. 1. Contradictory ordering comments in
|
The finality-synthesis block-epoch subscription must be armed with the sub-actor's long-lived context, handed to RegisterBlocks unwrapped. A per-attempt context.WithTimeout+cancel tears the subscription down the instant it is armed (the in-process block-epoch forwarder is tied to the ctx it receives), so no Done is ever synthesized and reorg-aware confirmations never finalize -- rounds/exits sit provisional forever even as the chain buries them far past the reorg-safety depth. This restores the reviewed #422 behavior that a sibling branch's stale copy of finality.go had reverted; it was reintroduced here when the reorg stack was condensed. Regression-caught by the round-never-confirmed itest/systest failures.
The finality-synthesis block-epoch subscription must be armed with the sub-actor's long-lived context, handed to RegisterBlocks unwrapped. A per-attempt context.WithTimeout+cancel tears the subscription down the instant it is armed (the in-process block-epoch forwarder is tied to the ctx it receives), so no Done is ever synthesized and reorg-aware confirmations never finalize -- rounds/exits sit provisional forever even as the chain buries them far past the reorg-safety depth. This restores the reviewed #422 behavior that a sibling branch's stale copy of finality.go had reverted; it was reintroduced here when the reorg stack was condensed. Regression-caught by the round-never-confirmed itest/systest failures.
The finality-synthesis block-epoch subscription must be armed with the sub-actor's long-lived context, handed to RegisterBlocks unwrapped. A per-attempt context.WithTimeout+cancel tears the subscription down the instant it is armed (the in-process block-epoch forwarder is tied to the ctx it receives), so no Done is ever synthesized and reorg-aware confirmations never finalize -- rounds and exits sit provisional forever even as the chain buries them past the reorg-safety depth. This restores the reviewed #422 behavior that a sibling branch's stale copy of finality.go had reverted; it was reintroduced when the reorg stack was condensed. Regression-caught by the round-never-confirmed itest and systest failures.
4 participants
Summary
Foundation layer for reorg-safe chain observation. This PR makes the chain-event surface reversible end to end so downstream subsystems can observe a tx or spend moving through:
For
txconfirm, the equivalent lifecycle is:The important semantic change is that the first positive observation is no longer terminal. A confirmation or spend can be reported, later leave the best chain, then be reported again. This is required for later client and server work where Ark may use a chain fact at low confirmation depth, likely one confirmation, while still remaining recoverable if that fact reorgs out.
What
Done/FinalizedMeansDoneandTxFinalizedmean policy finality at the configuredFinalityDepth, not absolute Bitcoin finality. They are not a claim that the tx can never be reorged under consensus.The model is:
usability_depth: when a higher layer is willing to act on a chain fact. For fast Ark UX this may be1confirmation.FinalityDepth: when this observation layer considers the watch complete enough to emitDone/TxFinalized. The production default wired here ischainsource.DefaultFinalityDepth(6).So yes, we still care about reorgs after a one-confirmation observation. That is the main point of this PR: between first observation and policy finality, the watch stays alive and can emit
Reorged, then a later positive observation again. OnceDone/TxFinalizedfires, low-level watchers may release resources. Later PRs will keep long-lived batch/VTXO meaning above this layer and can still reconcile or park on deeper-than-policy reorgs for still-relevant lineage.Why This PR Exists Before Batch/VTXO Reorg Safety
This PR intentionally does not decide whether a VTXO, batch, round, OOR receive, or unroll source is usable. It only observes chain facts.
Later PRs will add a
BatchCanonicalityManagerabovechainsourcethat interprets these raw observations:That later layer will own VTXO lineage states such as
available_provisional,limbo_reorg,limbo_conflict, andinvalidated. This PR is the observation substrate those states need.chainsource
ReorgedandDonechannels alongside the existingConfirmed/Spendchannel.ConfReorgedEvent,ConfDoneEvent,SpendReorgedEvent,SpendDoneEvent.Confirmed -> Reorged -> Confirmed -> Doneinstead of terminate-after-first-event.Doneis synthesized from configurableFinalityDepthheight watermarks so backends that drop upstream Done signals, notably lndclient over gRPC, still complete the lifecycle.RegisterBlocksfor finality synthesis is retried with bounded backoff before falling back to backend Done.chainbackends
LNDBackendforwards lnd reorg and finality notifications into the chainsource event surface.lndclientadapter wiresWithReOrgChanso the underlying gRPC stream is not torn down after the first event.txconfirm
TxConfirmedis no longer terminal.TxReorgedandTxFinalizedevents.Finalizedterminal state.TxConfirmed,TxReorged, cached-confirmation replay) are fire-and-forget on bounded per-subscriber goroutines.TxFinalized,TxFailed) keep the reliable goroutine + timeout + idempotent-retry pattern.Donefor an entry not currently confirmed is logged and dropped rather than incorrectly promoting toFinalized.darepod
chainsource.DefaultFinalityDepth(6) into productionChainSourceConfig.Donesource.Follow-Up Stack
This PR is the bottom of the reorg-safety stack.
Next PR (#410) adds unroll recovery-tx reorg handling:
PhaseCompleted.Later PRs will add the batch/VTXO canonicality layer:
Tests
chainsourceConfActor and SpendActor lifecycles.Donesynthesis.LNDBackendforwarder reorg tests.txconfirmreorg lifecycle tests.TestEnsureConfirmedDoneDuringReorgGapDroppedpins the Done-during-reorg-gap behavior.Test Plan
make lint-nativecleanDone/TxFinalizedare policy-finality events at configured depth, not absolute consensus finality