multi: add unilateral-exit subsystem - #235
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 introduces a comprehensive unilateral exit (unrolling) subsystem for on-chain VTXO recovery, featuring a new state-machine-driven manager, a transaction broadcaster with CPFP support, and RPC/CLI interfaces for manual triggers. Feedback focuses on improving code maintainability and consistency, specifically by refactoring hardcoded transaction size estimates into constants, simplifying redundant error logging in the LND backend, and standardizing log formatting within the actor library.
| var txErrors []string | ||
| for wtxid, txResult := range result.TxResults { | ||
| if txResult.Error == nil { | ||
| continue | ||
| } | ||
|
|
||
| txErrors = append(txErrors, fmt.Sprintf( | ||
| "wtxid=%s txid=%s error=%q", | ||
| wtxid, txResult.TxID, *txResult.Error)) | ||
| } |
There was a problem hiding this comment.
The error collection and sorting logic is redundant. Consider using a simpler approach to log errors directly without creating a slice of strings.
| // covers trees up to depth 10 (33+320 = 353 byte control | ||
| // block). Non-witness overhead is ~55 bytes; witness is | ||
| // discounted 4x, giving roughly 55 + (sig+script+cb)/4. | ||
| // Use 200 vbytes as a safe ceiling. |
There was a problem hiding this comment.
The vbyte estimation logic is hardcoded. Consider moving this to a constant or a helper function to make it easier to update if the transaction structure changes.
e515008 to
4273045
Compare
|
@claude review this |
|
Claude finished @ellemouton's task in 6m 43s —— View job PR Review: Client: add unilateral-exit unroller runtimeTasks:
This is a large, well-structured PR. The three-actor design (monitor / executor / manager+jobs) is clean, FSM state transitions are comprehensive with good test coverage, and the daemon wiring handles all three wallet modes. The fixes (PSBT rejection, sweep-as-local-spend, GetBalance) are sound. A few issues worth addressing below. Medium — should fix before merge1. Structured logging violation in
// executor.go:143
e.log.WarnS(ctx,
fmt.Sprintf("Unroll node package execution failed: target=%s txid=%s mode=%s kind=%s height=%d",
req.TargetOutpoint, plan.Txid, plan.Mode,
plan.NodeKind, req.Height,
),
err,
)Per the codebase structured logging guide, log messages must be static strings with key-value pairs. Should be: e.log.WarnS(ctx,
"Unroll node package execution failed",
err,
"target", req.TargetOutpoint,
"txid", plan.Txid,
"mode", plan.Mode,
"kind", plan.NodeKind,
"height", req.Height,
)2. Magic number should be a constant in // executor.go:316-323
// Estimate sweep virtual size … Use 200 vbytes as a safe ceiling.
sweepVSize := int64(200)The 200-vbyte estimate is well-justified in the comment, but using a named constant improves greppability and signals intent: // In broadcast or executor package constants:
const sweepVSizeEstimate = int64(200)3. // proof_assembler.go:406-415
witness := wire.TxWitness(witnessItems)
var buf bytes.Buffer
if err := psbt.WriteTxWitness(&buf, witness); err != nil {
return // ← silently bails, leaving FinalScriptWitness unset
}
in.FinalScriptWitness = buf.Bytes()If if err := psbt.WriteTxWitness(&buf, witness); err != nil {
// Encoding failed; caller will detect the missing
// FinalScriptWitness and return an appropriate error.
log.Debugf("finalizeTaprootScriptSpend: encode witness: %v", err)
return
}4. Redundant double-loop in // lnd.go:221-253
var txErrors []string
for wtxid, txResult := range result.TxResults { ... }
sort.Strings(txErrors)
// Then a second full iteration for debug logging:
for wtxid, txResult := range result.TxResults { ... DebugS(...) }The first loop builds a sorted error slice for the error message. The second loop re-iterates the same map for debug logging. These can be merged: // Single pass: collect errors and emit debug log together.
for wtxid, txResult := range result.TxResults {
if txResult.Error != nil {
txErrors = append(txErrors, fmt.Sprintf("wtxid=%s …", …))
}
b.logger(ctx).DebugS(ctx, "Package tx result", …)
}
sort.Strings(txErrors)Low — nits5. Both occurrences construct an error from a plain string: // fsm_transitions.go:74
err := job.RecordNodeFailure(
evt.Txid, fmt.Errorf("%s", evt.Reason),
)
6. return strings.Contains(err.Error(), "not supported")"not supported" is a very common phrase that could match unrelated errors (e.g., a fee estimation backend that doesn't support a certain confirmation target). Consider being more specific, e.g. matching 7. // handleNodePackages loop:
records = append(records, *record)
e.notifyNodeSuccess(ctx, plan.Txid, records) // ← records grows on each iteration
8.
Fixes included in this PRThe three bug-fix additions are solid:
|
2443488 to
49d2e1b
Compare
|
Addressed review comments from both Claude and Gemini: Claude review (comment #4196073160):
Gemini inline comments: All three addressed (structured logging, error collection, vbyte constant). Additional cleanup:
|
f79d018 to
858864c
Compare
d76eb1f to
66f0cbf
Compare
66f0cbf to
3dcfd03
Compare
ellemouton
changed the title
e24372a to
7384186
Compare
The round FSM saves VTXOs to the store before the VTXO manager sends VTXOCreatedNotification. Previously, CommitmentTxID, BatchExpiry, and CreatedHeight were left zero in the first save and filled in asynchronously by the manager's second upsert. This created a race: callers reading the VTXO between the two saves would see incomplete metadata. Fix: add CommitmentTxID, BatchExpiry, and CreatedHeight fields to ClientVTXO and populate them from the BoardingConfirmed event before SaveVTXOs. The first write is now complete, making the manager's upsert a harmless no-op.
Add a Log option to DurableActorConfig so actors can surface warning and error logs through the caller's logger instead of the context fallback. Log message type and delivery ID on Tell failures so silent nack loops become visible. Add WarnS calls to TransactionExecutor for begin, body, commit, and retry-exhaustion failures so DB transaction errors are observable without debug-level logging.
a6868ca to
c4a1daf
Compare
Add SubmitPackageRequest/Response to chainsource with backend forwarding. Add SubmitPackage to LNDBackend with optional PackageSubmitter interface (backed by bitcoind RPC). Add Esplora package relay support to lwwallet chain backend. Add PackageSubmitter field to LNDBackendFromLndClientConfig and daemon Config for wiring. Add 15-second timeout to LndClientChainNotifier's RegisterConfirmationsNtfn to prevent hangs under heavy block load. Add 10-second timeout to ConfActor's backend registration. Export GetLNDClientConn, BitcoindRPCUser, BitcoindRPCPass from the harness. Add BitcoindPackageSubmitter for itest package submission via direct bitcoind JSON-RPC. Export WalletKit() on BoardingBackend.
Add ForceUnrollEvent so manual unroll requests route through the VTXO actor's FSM rather than bypassing it with direct DB writes. LiveState handles ForceUnrollEvent by transitioning to UnilateralExitState and emitting ExpiringNotification through the chain resolver seam, converging manual and automatic triggers. Add ForceUnrollRequest/Response to the VTXO manager admission types and TestLiveStateForceUnroll unit test.
Add Unroll RPC for triggering unilateral exit by outpoint and GetUnrollStatus RPC for querying job progress. Add UnrollJobStatus enum and request/response messages for both endpoints.
Add onchain_wallet_confirmed_sat field to GetBalanceResponse and populate it from the backing wallet (LND, lwwallet, or btcwallet) so clients can see confirmed on-chain funds including sweep proceeds.
Add unilateral_exit_jobs migration, sqlc queries, and persistence store for manager-facing unroll job control-plane rows. Provides UpsertJob, GetJob, ListNonTerminalJobs, and MarkJobTerminal.
Add the immutable recovery proof graph (Proof, Node, NodeKind) with topological layering and parent/child tracking. Add the recovery Session that tracks per-node broadcast/confirmation state and computes CSV maturity. Add SessionState with custom JSON marshaling for map[chainhash.Hash] keys. Add proof binary codec for checkpoint persistence. Add tree path extraction helpers. Includes comprehensive unit tests for proof construction, session lifecycle, multi-branch graphs, and state round-trip serialization.
Pure computation layer that answers: given a recovery proof, durable progress state, and current block height, which proof transactions are ready to broadcast, which are blocked, and when is the target CSV-mature for sweeping? No I/O, no actors — callers own the durable State and the planner re-derives everything from first principles on each call. Validates state consistency against the proof graph before planning. Key types: Planner, State, Snapshot, TxFrontier, CSVInfo, SweepState.
Generic shared actor that deduplicates confirmation requests by txid and ensures transactions confirm on-chain. Any subsystem that needs "get this tx confirmed" can use it. Features: - Automatic anchor detection and CPFP child construction - Package relay with individual-broadcast fallback - Periodic fee bumping (non-terminal on failure so the original broadcast can still confirm via the active watch) - Subscriber fan-out for deduped confirmation/failure notifications - protofsm-based lifecycle: New → Broadcasting → AwaitingConfirmation → (FeeBumping loop) → Confirmed/Failed Key types: TxBroadcasterActor, CPFPBroadcaster, EnsureConfirmedReq.
Per-target durable actor managing the full unilateral-exit lifecycle: proof assembly, transaction materialization via txconfirm, CSV maturity wait, sweep construction and confirmation. Features: - UnrollRegistryActor: thin registry, dedup by outpoint, boot restore - VTXOUnrollActor: durable FSM per target, delegates to txconfirm - Sweep retry (up to 3 attempts before terminal failure) - Spend watch on target outpoint for early external-spend detection - Proof assembly from VTXO descriptors and OOR artifacts - Checkpoint persistence after every FSM transition Key types: UnrollRegistryActor, VTXOUnrollActor, LocalProofAssembler.
Add FinalizePsbtDirect for signing and finalizing PSBTs via btcwallet without going through LND's WalletKit RPC layer. Add WaitForSync which blocks until btcwallet's sync height reaches the Esplora chain tip, closing the race between the chain source actor's confirmation events and btcwallet's asynchronous block processing pipeline.
Add NewWalletAddress and ListWalletUnspent methods on Server so the root itest harness can request backing-wallet addresses and enumerate confirmed UTXOs across all wallet backends (LND, lwwallet, btcwallet).
Initialize the unroll subsystem during daemon startup: - Build wallet adapter (LND, lwwallet, btcwallet) satisfying both txconfirm.Wallet and unroll.SweepWallet - Create shared TxBroadcasterActor for CPFP package submission - Create UnrollRegistryActor with proof assembler and registry store - Restore non-terminal jobs on boot via RestoreNonTerminal - Wire critical-expiry path: VTXO ExpiringNotification → lazy chain resolver → unroll registry (solves circular init dependency) RPC integration: - Unroll RPC routes through VTXO manager → chain resolver → registry - GetUnrollStatus queries active registry first, falls back to DB for historical jobs
Add 'darepocli unroll --outpoint <txid:index>' to trigger unilateral exit and 'darepocli unroll status --outpoint <txid:index>' to query job progress.
c4a1daf to
20ccbe5
Compare
|
Replacing this PR with a 5-branch stack so each layer can be reviewed in isolation. All commits preserve @ellemouton as the git author.
The split also pulled in main's drift: I'll close this PR once the stack is reviewed; keeping it open for now as a cross-reference. |
|
@Roasbeef: review reminder |
In this commit, we switch the timeout-path sweep transaction from version 2 to the canonical Ark v3 (TRUC) version defined in `lib/tx/arktx`. CSV-relative timelocks work for any version >= 2, so the sequence-based CSV check is unaffected; this is purely a TRUC- compatibility fix. The shared `txconfirm` CPFP broadcaster gained a TRUC gate in 0c0f211 ("txconfirm: gate Submit on v3 (TRUC) parents") after PR #235 landed, but the unroll sweep builder was not updated at the same time. The net effect is that every unilateral-exit sweep fails on its first broadcast attempt with: broadcast: parent transaction must be v3 (TRUC) for CPFP broadcast: got version 2, want 3 And the failure mode is subtle: the FSM still transitions AwaitingSweepBroadcast -> AwaitingSweepConfirmation because that transition is driven by the `Ask` reply, not the txconfirm notification. The subsequent `TxFailed` that txconfirm fires gets dropped on a "mailbox full" error because the unroll actor's mailbox is saturated with `HeightObservedMsg` deliveries from the chainsource block subscription. So the FSM never learns the sweep failed and `GetUnrollStatus` sits at `UNROLL_JOB_STATUS_UNSPECIFIED` forever. We re-use `arktx.TxVersion` instead of re-hardcoding the constant so the unroll sweep and the rest of the Ark transaction graph stay in lockstep if the canonical Ark version ever changes. Validated against the three unilateral-exit itests on darepo (`TestUnilateralExitManualStartSingleParentTree`, `TestUnilateralExitRoundBornCompletion`, `TestUnilateralExitOORDerivedCompletion`); all three now run sweep broadcast -> confirmation -> completion end-to-end.
|
Merged in via this series: #235 (comment) |
ci: fix nightly doc-gardening workflow failures
4 participants
Summary
This PR adds the client-side unilateral-exit runtime and the daemon
plumbing around it.
The subsystem is split into three packages with distinct
responsibilities:
unrollplan: pure dependency-resolution planner. Given a recoveryproof, durable progress state, and current block height, it determines
which proof transactions are ready to broadcast, which are blocked by
unconfirmed parents, when the target becomes CSV-mature, and whether
the sweep is needed. No I/O, no actors — callers own the durable State
and the planner re-derives everything from first principles on each
call.
txconfirm: generic shared actor for getting transactions confirmedon-chain. Not tied to unrolling — any subsystem that needs "get this
tx confirmed with CPFP" can reuse it. Handles anchor detection, CPFP
child construction, package relay with individual-broadcast fallback,
periodic fee bumping (non-terminal on failure so the original broadcast
can still confirm via the active watch), and subscriber fan-out for
deduped confirmation/failure notifications.
unroll: durable per-target actor and thin registry. EachVTXOUnrollActor owns one target outpoint's lifecycle, delegates
broadcasting to txconfirm, and drives the FSM through proof
materialization → CSV wait → sweep. The UnrollRegistryActor manages
job creation, dedup, boot restore, and terminal bookkeeping.
Supporting infrastructure added by earlier commits:
lib/recovery: immutable proof graph, session state, andserialization for representing the target VTXO's reachable on-chain
ancestry
db: persisted unilateral-exit job store for tracking non-terminaland terminal unroll jobs, including sweep txid
daemonrpc: Unroll, GetUnrollStatus, and GetBalance.onchain_walletRPCs
vtxo: ForceUnrollEvent in the VTXO lifecycle FSM andLazyChainResolver to bridge critical-expiry notifications into the
unroll registry without a circular init dependency
chainsource/chainbackends/lwwallet: package submission supportacross all wallet backends
Daemon wiring:
txconfirm.Wallet and unroll.SweepWallet
resolver → unroll registry
for historical/completed jobs
User-Visible Behavior
The client can now:
Resilience
broadcast is still live and the confirmation watch remains active.
The FSM recovers to AwaitingConfirmation with an updated broadcast
height so the next bump waits the full interval before retrying.
before the actor transitions to terminal failure.
terminating the job with a clear "spent externally" reason instead
of waiting for the sweep to fail with a cryptic missing-input error.
Validation
go test ./unrollplan/... ./txconfirm/... ./unroll/...make lint-nativemake itest icase=UnilateralExit(manual, round-born, OOR-derived)in the server repo against this client branch
Related