Skip to content

multi: Reorg-safety extras (round finality gate, boarding sweep, fraud invariants)#558

Closed
ellemouton wants to merge 1 commit into
unroll-reorg-safetyfrom
reorg-safety-extras
Closed

multi: Reorg-safety extras (round finality gate, boarding sweep, fraud invariants)#558
ellemouton wants to merge 1 commit into
unroll-reorg-safetyfrom
reorg-safety-extras

Conversation

@ellemouton

@ellemouton ellemouton commented May 28, 2026

Copy link
Copy Markdown
Member

Phase 0.5 of the reorg-safety epic (darepo#454). Stacks on top of #410.

This PR collects the smaller pieces that round out reorg-aware operation in the
parts of the daemon adjacent to the chainsource substrate (#422), the lwwallet
backend (#461), and the unroll subsystem (#410):

  • round: gate commitment-tx terminal transition on finality
    interim safety mode that the design doc keeps until the
    BatchCanonicalityManager exists. The round FSM only moves to
    Confirmed after the commitment tx reaches the configured
    finality depth, so a one-conf reorg cannot leave the local actor
    in a stale terminal state.
  • fraud: pin reorg-safety invariants for the ancestor spend watch — defensive tests that lock in the one-way alarm
    semantics: repeated spend observations are idempotent (registry
    dedup absorbs them), the watcher never decides canonicality, and
    it does not undo prior alarms on reorg.
  • wallet: extend boarding-sweep spend watch to the reorg-aware lifecycle — the boarding-sweep watcher subscribes to the
    new Reorged / Done channels on the chainsource spend
    registration so a reorged-out sweep is properly tracked through
    the new chainsource lifecycle.
  • systest: boarding sweep reorg-aware lifecycle round-trip /
    systest: round commitment reorg-aware lifecycle round-trip with finality — end-to-end coverage exercising the
    observed → reorged → reconfirmed → done sequence through the
    wallet and round subsystems.

Stack

#422 (chainsource substrate) → #410 (unroll recovery-tx safety) → this PR.
#461 (lwwallet) is a sibling of #410.

Test plan

  • go vet ./...
  • make unit pkg=round
  • make unit pkg=fraud
  • make unit pkg=wallet
  • make unit pkg=unroll
  • CI systests (systest/boarding_sweep_reorg_test.go,
    systest/round_commitment_reorg_test.go)

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces reorg-aware lifecycles for the fraud watcher, round client actor, and boarding sweep actor, adding provisional confirmation caching, reorg/finality event handlers, and extensive unit and system tests. The review identified two critical issues: first, a state leak in the round actor where cached confirmations are deleted before the FSM transition successfully completes, potentially leaving rounds permanently stuck; second, a logic bug in the boarding sweep actor where the spend watch is immediately unregistered upon provisional confirmation, defeating the reorg-aware lifecycle.

Comment thread round/actor.go
Comment on lines +2140 to 2157
}
delete(a.pendingCommitmentConfs, event.Txid)

confirmEvt := &BoardingConfirmed{
TxID: event.Txid,
BlockHeight: event.BlockHeight,
BlockHash: event.BlockHash,
Confirmations: int32(event.Confirmations),
TxID: cached.Txid,
BlockHeight: cached.BlockHeight,
BlockHash: cached.BlockHash,
Confirmations: int32(cached.Confirmations),
}

err := a.askEventAndProcessOutbox(ctx, roundFSM, confirmEvt)
if err != nil {
return fn.Err[actormsg.RoundActorResp](
fmt.Errorf("FSM error processing commitment "+
"confirmation: %w", err),
fmt.Errorf("FSM error promoting round on finality: %w",
err),
)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Critical State Leak / Stuck Round Bug

In handleCommitmentFinalized, the cached confirmation is deleted from a.pendingCommitmentConfs before the FSM transition is attempted via askEventAndProcessOutbox:

delete(a.pendingCommitmentConfs, event.Txid)
...
err := a.askEventAndProcessOutbox(ctx, roundFSM, confirmEvt)

If askEventAndProcessOutbox fails (for example, due to a transient database error in FinalizeRound or a temporary communication failure in processOutbox), the round will not transition to ConfirmedState and onRoundComplete will not run.

Because pendingCommitmentConfs is an in-memory map and is not persisted, the cached confirmation is permanently lost. Any subsequent attempt to finalize the round will fail with the warning "Commitment-tx finalized without a cached prior ConfirmationEvent; skipping FSM transition", leaving the round permanently stuck.

Recommendation

Only delete the cached confirmation from a.pendingCommitmentConfs after askEventAndProcessOutbox successfully completes.

	confirmEvt := &BoardingConfirmed{
		TxID:          cached.Txid,
		BlockHeight:   cached.BlockHeight,
		BlockHash:     cached.BlockHash,
		Confirmations: int32(cached.Confirmations),
	}

	err := a.askEventAndProcessOutbox(ctx, roundFSM, confirmEvt)
	if err != nil {
		return fn.Err[actormsg.RoundActorResp](
			fmt.Errorf("FSM error promoting round on finality: %w",
				err),
		)
	}

	delete(a.pendingCommitmentConfs, event.Txid)

	return fn.Ok[actormsg.RoundActorResp](nil)

Comment on lines +1143 to +1144
case BoardingSweepSpendStatusSpent:
// Fall through to the existing Spent handling below.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Critical Logic Bug: Reorg-Aware Spend Watch Immediately Destroyed

In handleSweepSpendNotification, when notif.Status is BoardingSweepSpendStatusSpent, the code falls through to the legacy spent handling logic below:

case BoardingSweepSpendStatusSpent:
	// Fall through to the existing Spent handling below.

However, the legacy spent handling code immediately unregisters the spend watch (via UnregisterSpendRequest) and deletes the outpoint from p.inputs and a.pendingSweepInputs:

delete(p.inputs, notif.Outpoint)
delete(a.pendingSweepInputs, notif.Outpoint)
...
if err := a.chainSource.Tell(ctx, &chainsource.UnregisterSpendRequest{...}); err != nil { ... }

This completely defeats the reorg-aware spend watch lifecycle described in the comments. Once unregistered, the SpendActor is stopped, meaning the wallet will never receive subsequent Reorged or Done events for this outpoint. Furthermore, even if those events were received, they could not be correlated because the in-memory tracking maps have already been cleared.

Recommendation

To preserve the reorg-aware lifecycle, do not unregister the spend watch or delete the outpoint from the in-memory tracking maps on BoardingSweepSpendStatusSpent. Instead, keep the watch alive and defer the unregistration and map cleanup to the BoardingSweepSpendStatusDone event.

@levmi levmi added the P1 Priority 1 — high label Jun 10, 2026
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch from 01ecfd9 to 778e719 Compare June 24, 2026 22:03
@ellemouton
ellemouton force-pushed the reorg-safety-extras branch from 4a9daf9 to df03320 Compare June 24, 2026 22:09
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch from 778e719 to 38cf5e6 Compare June 29, 2026 15:33
@ellemouton
ellemouton force-pushed the reorg-safety-extras branch from df03320 to 8227500 Compare June 29, 2026 15:34
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch from 3609304 to fc520a1 Compare July 1, 2026 16:17
@ellemouton
ellemouton force-pushed the reorg-safety-extras branch from 8227500 to 7a633d7 Compare July 1, 2026 16:17
@levmi levmi added reorg round safety Fund-safety: stuck, lost, or mis-counted funds labels Jul 6, 2026
@ellemouton
ellemouton force-pushed the unroll-reorg-safety branch from fc520a1 to d52c121 Compare July 8, 2026 20:49
Squashed for the btcd v2 port. Interim reorg-safety extras: round
commitment-tx terminal transition gated on finality, boarding-sweep
spend watch extended to the reorg-aware lifecycle, fraud ancestor
spend-watch reorg invariants, and reorg-aware lifecycle systests.
@ellemouton

Copy link
Copy Markdown
Member Author

Superseded by #897 as part of condensing the reorg-safety client stack (epic lightninglabs/darepo#454) from 12 PRs into 3. The commits are carried over unchanged; see #897. Branch retained as a backup.

@ellemouton ellemouton closed this Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 Priority 1 — high reorg round safety Fund-safety: stuck, lost, or mis-counted funds

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants