txconfirm: reusable tx confirmation actor with CPFP (3/5) - #262
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 adds the txconfirm package, featuring a generic actor and a CPFP-capable broadcaster for managing transaction confirmations. The implementation uses a state machine to track transaction lifecycles. Key feedback identifies critical issues: deduplication by txid fails to account for differing confirmation targets or scripts, terminal entries are not pruned from memory, and synchronous polling in the broadcaster blocks the actor's execution. Furthermore, a logic error in the polling exit condition could lead to premature broadcast failures.
| if existing, ok := a.tracked[txid]; ok { | ||
| return a.attachExistingSubscriber( | ||
| ctx, existing, req.Subscriber, | ||
| ), nil | ||
| } |
There was a problem hiding this comment.
Deduplication by txid alone is insufficient and can lead to incorrect behavior when subscribers have different requirements. For example, if a transaction is already tracked with TargetConfs=1, a new request for the same txid requiring TargetConfs=6 will be attached to the existing entry and notified prematurely at 1 confirmation. Similarly, if requests specify different ConfirmationPkScript values, they will be incorrectly merged. The actor should either use a more specific key for the tracked map (e.g., including the target and script) or update the existing entry to satisfy the most stringent requirements (e.g., the maximum TargetConfs).
| if state == TxStateConfirmed || state == TxStateFailed { | ||
| return resp, nil | ||
| } |
There was a problem hiding this comment.
The tracked map is never pruned of terminal entries, leading to a memory leak. In handleCancel, the logic explicitly returns early if the transaction is in a terminal state (TxStateConfirmed or TxStateFailed), which prevents the entry from being deleted from a.tracked even when no subscribers remain. Additionally, notifyConfirmed and notifyFailed remove subscribers from the entry but do not remove the entry from the actor's map. Over time, this map will grow indefinitely as more transactions are processed.
if state == TxStateConfirmed || state == TxStateFailed {
delete(a.tracked, entry.data.Txid)
resp.StoppedTracking = true
return resp, nil
}| deadline := time.Now().Add(2 * time.Second) | ||
|
|
||
| for { | ||
| utxos, err := b.cfg.Wallet.ListUnspent(ctx, 1, 9999999) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("list unspent: %w", err) | ||
| } | ||
|
|
||
| var best *wallet.Utxo | ||
| for _, utxo := range utxos { | ||
| _, excluded := b.usedFeeOutpoints[utxo.Outpoint] | ||
| if excluded { | ||
| continue | ||
| } | ||
|
|
||
| if utxo.Amount < minAmount { | ||
| continue | ||
| } | ||
|
|
||
| if best == nil || utxo.Amount < best.Amount { | ||
| best = utxo | ||
| } | ||
| } | ||
|
|
||
| if best != nil { | ||
| return &FeeInput{ | ||
| Outpoint: best.Outpoint, | ||
| Output: &wire.TxOut{ | ||
| Value: int64(best.Amount), | ||
| PkScript: best.PkScript, | ||
| }, | ||
| Confirmed: true, | ||
| }, nil | ||
| } | ||
|
|
||
| // After one CPFP package confirms, wallet backends | ||
| // with asynchronous chain ingestion can lag briefly | ||
| // before the confirmed change output becomes visible. | ||
| // Poll through that handoff instead of immediately | ||
| // falling back to direct broadcast of a zero-fee | ||
| // parent. | ||
| if len(b.usedFeeOutpoints) == 0 || time.Now().After(deadline) { | ||
| break | ||
| } | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
|
|
||
| case <-time.After(100 * time.Millisecond): | ||
| } | ||
| } |
There was a problem hiding this comment.
The selectFeeInput method performs synchronous polling with a 2-second deadline inside the actor's Receive loop. This blocks the actor from processing any other messages, including block updates or new confirmation requests, for the duration of the poll. This can cause significant latency and potential mailbox overflows if multiple transactions require fee bumps simultaneously. A non-blocking approach, such as scheduling a retry message to the actor itself or making the UTXO selection asynchronous, would be more appropriate for a shared service.
| // parent. | ||
| if len(b.usedFeeOutpoints) == 0 || time.Now().After(deadline) { | ||
| break |
There was a problem hiding this comment.
The condition len(b.usedFeeOutpoints) == 0 causes the polling loop to exit immediately if no other transactions have been bumped in the current block. This defeats the stated purpose of the polling (handling wallet lag after a block) for the first transaction processed in a new block. If the wallet backend is lagging, the first bump attempt will fail immediately instead of waiting for the confirmed UTXOs to become visible.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b47f73975
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ctx, &chainsource.UnregisterConfRequest{ | ||
| CallerID: a.confCallerID(entry.data.Txid), | ||
| Txid: &txid, | ||
| TargetConfs: entry.data.TargetConfs, |
There was a problem hiding this comment.
This unregister request omits PkScript, but registerConfWatch always registers with both Txid and PkScript. In chainsource.txidOrScriptKey, that means registration uses a different service key (txid+script) than unregistration (txid), so the confirmation actor is never found and never stopped. The leak happens for every tracked tx and leaves stale confirmation subscriptions/goroutines behind after confirm/cancel/shutdown paths.
Useful? React with 👍 / 👎.
| b.usedFeeOutpoints[feeInput.Outpoint] = struct{}{} | ||
|
|
||
| changePkScript, err := b.deriveChangePkScript(ctx) |
There was a problem hiding this comment.
The fee outpoint is marked as used before CPFP construction/signing/submission succeeds, and several later branches (deriveChangePkScript, child build/sign, package submit errors, or direct-fallback paths) can return without spending that UTXO. Because usedFeeOutpoints is only cleared on a new block, subsequent submissions in the same block can incorrectly treat a still-unspent coin as unavailable and fail with ErrCPFPFeeInputUnavailable.
Useful? React with 👍 / 👎.
58c2e1f to
bf7f490
Compare
c0731c8 to
9e35a2d
Compare
3b47f73 to
4f3aaba
Compare
|
Claude finished @Roasbeef's task in 3m 24s —— View job Code Review —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f3aaba1a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| txid := req.Tx.TxHash() | ||
| if existing, ok := a.tracked[txid]; ok { | ||
| return a.attachExistingSubscriber( | ||
| ctx, existing, req.Subscriber, | ||
| ), nil |
There was a problem hiding this comment.
This dedup path reuses an existing tracked entry solely by txid, so later requests with a different TargetConfs are silently attached to the first request’s watch. In practice, a subscriber asking for 6 confs can be notified at 1 conf (or vice versa), because registerConfWatch is never reconciled to the new target. That breaks the EnsureConfirmedReq contract that each request is confirmed to its requested depth.
Useful? React with 👍 / 👎.
| ) | ||
| } | ||
|
|
||
| b.usedFeeOutpoints[feeInput.Outpoint] = struct{}{} |
There was a problem hiding this comment.
The fee input is marked as used before CPFP construction/signing/submission has succeeded. If any later step falls back to direct broadcast or returns an error, the outpoint remains excluded for the rest of the block height even though no child spent it. That can cause subsequent anchor submissions at the same height to hit ErrCPFPFeeInputUnavailable and skip otherwise valid broadcasts.
Useful? React with 👍 / 👎.
| a.failTrackedTx(ctx, entry, fmt.Sprintf( | ||
| "broadcast: %v", err, | ||
| )) |
There was a problem hiding this comment.
This failure path marks the tracked tx as terminally failed but does not unregister the confirmation subscription that was created earlier in handleEnsure. That leaves a live conf watch for a txid the actor no longer processes, consuming chainsource resources and producing callbacks that are ignored due to the failed state.
Useful? React with 👍 / 👎.
| return false | ||
| } | ||
|
|
||
| return a.bestHeight-lastBroadcastHeight >= |
There was a problem hiding this comment.
We should abstract this further, as it can eventually use the lnd fee function. https://github.com/lightningnetwork/lnd/blob/a8a3e13120ebd52050ea4c6b653e8182f62fbae9/sweep/fee_function.go#L120
sweep.NewLinearFeeFunction. It takes into account the step, target, max, and then uses a linear function to bump progressively as needed.
|
|
||
| // notifyOneFailed delivers one terminal failure notification. | ||
| func (a *TxBroadcasterActor) notifyOneFailed(ctx context.Context, | ||
| subscriber actor.TellOnlyRef[Notification], txid chainhash.Hash, |
There was a problem hiding this comment.
Nice pattern here re an abstract actor for subscriptions!
| // ChildVSizeEstimate is the estimated virtual size of a CPFP child with | ||
| // one anchor input, one confirmed fee input, and a single change | ||
| // output. Revisit this estimate if the child input composition changes. | ||
| ChildVSizeEstimate = 155 |
There was a problem hiding this comment.
We can use the actual fee estimator here instead: input.TxWeightEstimator.
| Output *wire.TxOut | ||
|
|
||
| // Confirmed indicates whether this UTXO is confirmed. | ||
| Confirmed bool |
| } | ||
|
|
||
| // BroadcastRequest describes a signed transaction to broadcast. | ||
| type BroadcastRequest struct { |
There was a problem hiding this comment.
Would think this accepts a package? Or the idea is that we always spend an anchor if it's there.
| if err != nil { | ||
| return nil, fmt.Errorf("list unspent: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
We should be exposing a LeaseOutput method via an interface to call once we finally select the input: https://github.com/lightningnetwork/lnd/blob/a8a3e13120ebd52050ea4c6b653e8182f62fbae9/lnrpc/walletrpc/walletkit.proto#L39-L53.
We'd then need to unlock then as needed as well.
Only the lnd+btcwallet backends need this, it'll be a noop for lwallet for now (can make an issue abouve that).
|
|
||
| // EstimatePackageFee computes the total package fee for one parent+child | ||
| // submission at the given fee rate. | ||
| func EstimatePackageFee(parentTx *wire.MsgTx, |
There was a problem hiding this comment.
We can use input.TxWeight estimator here again.
5398bf5 to
88df42b
Compare
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.
Addresses two lifecycle-correctness bugs found in the PR 262 review of
the new txconfirm actor.
H-1: terminal a.tracked entries were never evicted. Once a tracked txid
reached Confirmed or Failed, its *trackedTx (with a cached *wire.MsgTx
and a live per-tx FSM goroutine) stayed in the map until actor shutdown.
A long-lived daemon accumulated an O(total_txs_ever) leak even when
otherwise idle. handleConfirmationObserved and failTrackedTx now call a
new evictTerminal helper that unregisters any outstanding conf watch,
stops the FSM, and drops the entry. The previous replay-on-late-subscribe
path in attachExistingSubscriber is no longer reachable via dedup: a late
ensure for a previously-confirmed txid starts fresh tracking, which
re-registers with chainsource and — for already-confirmed txs — receives
an immediate confirmation notification through the normal path.
H-8: chainsource derives a conf sub-actor's service key by hashing
CallerID + Txid + PkScript + TargetConfs (via txidOrScriptKey in
chainsource.go). RegisterConfRequest in this package passed all four
fields, producing the key "<txid>+script:<hex>". UnregisterConfRequest
omitted PkScript, producing a different key "<txid>" — so the unregister
silently found nothing and leaked one conf sub-actor per tracked tx. The
existing test fake did not model service-key derivation so the bug went
uncaught. unregisterConfWatch now passes PkScript alongside Txid,
matching the register payload.
A confWatchRegistered flag on trackedTx records whether a watch is
currently live so evictTerminal knows whether an unregister round trip
is even needed (entries that failed during block-subscription setup
never registered).
Regression tests:
- TestTerminalEntriesEvictedAfterConfirmation drives three
unrelated txids end-to-end and uses Cancel-as-probe to assert the
entries are no longer present.
- TestTerminalEntryEvictedAfterFailure does the same for the broadcast
error path.
- TestUnregisterConfMatchesRegisterServiceKey asserts every service-key
input (CallerID, Txid, PkScript, TargetConfs) matches byte-for-byte
between the register and unregister requests.
- TestEnsureConfirmedAlreadyConfirmedUsesSuccessPath and
TestEnsureConfirmedBroadcastFailureNotifiesFailure are updated to
reflect that late subscribers now open fresh tracking.
Addresses H-6 from the PR 262 review. signCPFPChild wrote WitnessUtxo values into packet.Inputs[0] and [1] by positional index, assuming the anchor sat at index 0 and the fee input at index 1 — an invariant upheld only by BuildCPFPChild's current construction. The post-finalize witness copy loop iterated finalTx.TxIn with the same positional assumption and would panic with an index-out-of-range if the wallet returned a transaction whose input composition did not exactly round-trip the PSBT (added or reordered inputs, which some backends do as part of BIP 69 canonicalization or internal bookkeeping). The function now takes the anchor outpoint as an explicit parameter and locates the anchor / fee slots in the PSBT by matching each input's PreviousOutPoint. Witness copy-back builds an outpoint-keyed map over finalTx.TxIn and copies into the corresponding child.TxIn slot, making reordered finalized transactions transparently correct. A length mismatch or an outpoint substitution produces a clean error routed through the existing fallbackDirectBroadcast path rather than a panic or a malformed package submission. A new rewritingWallet test double parses the incoming PSBT, applies dummy witnesses, and lets the test rewrite the finalized tx before returning. TestSignCPFPChildHandlesWalletInputRewrites exercises three cases: reordered inputs (succeeds), extra inputs (length mismatch → fallback), and substituted outpoint (missing key → fallback).
Phase 1 of the CPFP-correctness fixes from the PR 262 review. txconfirm
is intentionally subsystem-neutral, but the CPFP fee-bump strategy and
anchor-detection heuristic both assume BIP-431 (TRUC / v3) semantics:
- The zero-fee ephemeral anchor only relays as part of a v3 package.
- Pattern-based anchor detection (findAnchorOutput → arktx.IsAnchorOutput
which matches P2A script bytes) is structurally unsafe for a non-v3
parent whose outputs were chosen without anchor-awareness; a
legitimate output that happens to look like an anyone-can-spend
anchor would silently receive a CPFP child that never relays and
burns the caller's wallet fee input.
- Package RBF replacement policy (the landing mechanism for our
fee-bump loop) requires TRUC topology.
CPFPBroadcaster.Submit now rejects any parent whose Version is not
arktx.TxVersion (= 3) with a new sentinel ErrNonTRUCParent before doing
any chainsource work. TestCPFPBroadcasterFallbackAndErrors gains a
"non-v3 parent rejected at Submit" sub-test that drives the rejection
and asserts via errors.Is.
This closes the API-boundary gap that was the root cause of review
finding H-5 (pattern-based anchor detection) and a precondition for
Phase 2/3 (bump comparator and per-parent outpoint reservation) to
have well-defined semantics.
Phase 2 of the CPFP-correctness fixes from the PR 262 review. The
fee-bump loop submitted a fresh anchor + child package every
FeeBumpIntervalBlocks blocks, but the new package's fee was computed
from scratch against the current estimator on each cycle with no
comparison to the package it was meant to replace. Two concrete
regressions followed:
- Flat estimator. Every bump regenerated a byte-identical package at
the same feerate and same absolute fee. Core rejected each cycle as
"already in mempool" / "already known"; the ignorable-error list
even classified that as non-fatal so the log silently papered over
the fact that we were making zero forward progress.
- Dipping estimator. When fee estimates dropped between bumps (blocks
with less competition, or a backend that rolls window averages),
the new package paid a strictly lower feerate than the original.
Core's package RBF rejected the replacement and the tx sat with
whatever fee it had at the previous cycle.
CPFPBroadcaster now maintains a per-parent-txid parentBumpState
recording LastFeeRate and LastPackageFee. Every broadcastWithCPFP
invocation runs the fresh estimator output through applyReplacementFloor
before selecting a fee input:
- Rule 4: if the new feerate is <= the prior feerate, ratchet it to
prior + 1 sat/vB.
- Rule 3: if the resulting absolute fee doesn't exceed the prior
package fee by at least IncrementalRelayFeeSatPerVByte *
packageVSize, top it up to do so.
A new BroadcasterConfig.IncrementalRelayFeeSatPerVByte (defaulting to
DefaultIncrementalRelayFeeSatPerVByte = 1 sat/vB, matching Bitcoin
Core's default) lets operators with non-default nodes pin their own
incremental relay feerate. The same knob is exposed on the actor's
Config and forwarded through.
CPFPBroadcaster.Evict(txid) releases the per-parent state; the actor's
evictTerminal now calls it alongside its own tracked-entry cleanup so
parentStates doesn't accumulate after H-1's fix is applied.
Regression tests:
- TestApplyReplacementFloor is a white-box unit test hitting every
branch of the comparator (no prior state, flat estimator, dipping
estimator, Rule 3 domination via custom IRF, caller-provided larger
totalFee preserved).
- TestCPFPBroadcasterFeeBumpReplacementFloor drives the full
broadcaster end-to-end across three submissions under flat and
dipping estimators and asserts strictly-increasing feerate and
absolute fee across bumps, plus Evict clearing the per-parent
history.
Phase 3 of the CPFP-correctness fixes from the PR 262 review. The
broadcaster previously tracked fee-input reservations in a global
usedFeeOutpoints map cleared on every new block. Two concrete bugs
followed:
- A parent still waiting for its CPFP package to confirm would lose
its reservation on the very next block, and an unrelated concurrent
parent could then pick the same wallet UTXO for its own fee input,
double-spending the UTXO across two in-flight children.
- The same parent's next fee-bump cycle (cleared by the block
advance) would re-select the same UTXO — which, without BIP-125
signaling on v1/v2 parents, could not RBF the first child. Phase 1
gates non-v3 parents at Submit so this isn't a consensus foot gun
anymore, but TRUC package RBF *wants* the new child to spend the
same fee UTXO as the previous child (that is how the replacement
lands), so the reservation has to stay scoped to the parent while
excluding every *other* parent.
UsedFeeOutpoints now lives on parentBumpState alongside the fee-history
fields. selectFeeInput receives the parent txid and builds its
exclusion set from outpoints held by parents other than the current
one. The per-block clear is removed entirely; Evict is the single
release point, called when the actor's FSM learns the parent has
terminally confirmed or failed.
Regression test TestUsedFeeOutpointsKeyedByParent exercises:
- Reservation persisting across a new block for the same parent.
- A second parent being blocked from reusing the first parent's UTXO.
- Evict releasing the UTXO for other parents.
- The same parent being allowed to re-pick a UTXO it already
reserved (the TRUC package-RBF replacement path).
Phase 4 of the CPFP-correctness fixes from the PR 262 review. The CPFP child's fee input now carries the BIP-125 replaceable sequence number (wire.MaxTxInSequenceNum - 2 = 0xfffffffd) instead of the sentinel MaxTxInSequenceNum. The anchor input keeps its sentinel sequence because it is anyone-can-spend with no timelock semantics. This is a belt-and-suspenders fix on top of Phase 1 (Submit now rejects non-v3 parents), so in practice the parent tx's v3 version already implies replaceability for TRUC package RBF. The explicit sequence signal means that if a future caller ever slips past the version gate with a v2 parent (for example by bypassing the helper), the child would still be eligible for BIP-125 replacement on non-TRUC relays instead of being permanently pinned. The existing "build cpfp child" test now asserts the sequence layout so any future change to BuildCPFPChild must either preserve this property or explicitly update the test.
Bitcoin Core's testmempoolaccept RPC has accepted an array of hex
transactions since 24.0, so the full-package testmempoolaccept flow
can preflight an entire CPFP parent+child set without broadcasting
either transaction first. The ChainBackend interface only exposed the
single-tx form, which makes it impossible for callers like txconfirm's
CPFPBroadcaster to preflight a package against local node policy
before submission.
Change the interface to accept a variadic *wire.MsgTx and return a
slice of MempoolAcceptResult (one per input tx, in request order). The
new ErrPackageMempoolAcceptUnsupported sentinel lets implementations
that cannot evaluate multi-tx packages distinguish "backend refused to
validate at all" from "backend evaluated and one tx was rejected".
TestMempoolAcceptRequest.Tx becomes TestMempoolAcceptRequest.Txs and
TestMempoolAcceptResponse.{Accepted,Reason} becomes
TestMempoolAcceptResponse.Results []MempoolAcceptResult, matching the
per-tx return shape. handleTestMempoolAccept and the internal
BroadcastTx fallback path (which uses TestMempoolAccept as a
post-broadcast-error sanity check) are updated accordingly.
All three existing ChainBackend implementations (btcwbackend,
chainbackends/lnd, lwwallet) preserve their previous behaviour: none
supports testmempoolaccept today, so each returns an error regardless
of how many txs are supplied. Real single- and package-support can
land in a follow-up that wires lwwallet's ChainBackend through to the
existing EsploraClient.TestMempoolAccept (which already accepts
multiple txs). Test mocks (mockBackend, broadcastErrorBackend,
errorBackend) and the three tests that constructed the old
request/response shape are updated to the new API.
This commit does not yet add a CPFPBroadcaster-side caller; wiring
txconfirm's preflight onto the new interface is the next step.
Phase 5 of the CPFP-correctness fixes from the PR 262 review. With the
new variadic chainsource.ChainBackend.TestMempoolAccept interface in
place, wire CPFPBroadcaster to preflight every broadcast attempt
against the local node's mempool policy before handing the transaction
(or parent+child package) off to the network.
Preflight exposes three observable outcomes:
- Accepted: submission proceeds.
- Rejected with reason: submission aborts with an error that
includes the backend's reject reason and the rejected txid.
Callers surface this to the FSM rather than waste a network round
trip on a package we know the mempool will drop.
- Backend doesn't implement testmempoolaccept: downgraded to a soft
miss (debug log) and submission proceeds. This keeps the flag safe
to leave enabled on btcwbackend, chainbackends/lnd, and
lwwallet deployments today, while still gaining real preflight
checks on backends that do support the RPC.
The check is opt-in via BroadcasterConfig.PreSubmitTestMempoolAccept,
defaulting off. The same knob is plumbed through the actor Config so
callers of TxBroadcasterActor can enable preflight without reaching
into the internal broadcaster.
broadcastDirect preflights the single tx before BroadcastTx;
broadcastWithCPFP preflights parent+child together after the child is
signed but before SubmitPackage. The CPFP path deliberately preflights
after signing so a rejection caught here represents a policy-level
rather than construction-level failure.
TestPreflightTestMempoolAccept covers: CPFP path preflights
parent+child together; direct path preflights only the single tx;
backend rejection surfaces the reason and aborts before broadcast;
"not supported" is treated as a soft-miss and lets submission
through; preflight is disabled by default.
Expand doc.go into a full literate overview of the package. The new
doc walks through the Architecture / Lifecycle / CPFP correctness /
PSBT finalization / Service-key round trip / Eviction story so a
reader landing here for the first time can understand both WHAT each
primitive does and WHY each invariant exists, linking back to the
five CPFP-correctness phases this branch added.
Also expand a handful of core godocs that were previously terse:
- TxBroadcasterActor now spells out the single-threaded-Receive,
one-watch-per-non-terminal-entry, no-terminal-entries, and
lazy-block-subscription invariants.
- trackedTx notes that mutation is actor-goroutine-exclusive and
therefore not mutex-guarded.
- handleBlockObserved explains why fee-bump failures are
non-terminal — the original broadcast and confirmation watch are
still live, and the recovery path lets the next interval
retry rather than escalating to Failed.
No functional change.
Defines wallet.LockID ([32]byte) and wallet.OutputLeaser with LeaseOutput/ReleaseOutput methods that match the canonical shape used by btcwallet and lndclient's WalletKit. Concrete backends (btcwbackend, lndbackend, lwwallet) now satisfy the interface by forwarding to the underlying btcwallet or LND WalletKit. txconfirm.Wallet embeds OutputLeaser so every CPFP fee-input selected by the broadcaster takes a wallet-level lease on reservation and releases it on terminal eviction or fallback. This closes a narrow race where a wallet shared across subsystems could hand the same UTXO to another caller in between CPFP child construction and broadcast. The broadcaster's per-parent parentStates map remains the source of truth; the wallet lease is defence in depth, and lease errors are treated as soft misses so a noop-leaser wallet is still functional.
Introduces per-package documentation for the new txconfirm package (broadcast + CPFP fee-bump + confirmation notification actor) and adds it to ARCHITECTURE.md's Layer 2 infrastructure table. Covers key types, chainsource / wallet message flows, and the CPFP correctness invariants that future wiring must preserve.
88df42b to
269fb2b
Compare
|
Claude finished @Roasbeef's task in 4m 47s —— View job Code Review —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 269fb2bfcc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| entry.fsm.Stop() | ||
| } | ||
|
|
||
| delete(a.tracked, entry.data.Txid) | ||
| resp.StoppedTracking = true |
There was a problem hiding this comment.
When CancelInterestReq removes the last subscriber, this path stops the FSM and deletes the tracker but never calls a.broadcaster.Evict(...). If the tx previously reserved a CPFP fee input, that wallet lease stays active until expiry (default 1 hour), so later broadcasts can be starved of UTXOs even though tracking was explicitly canceled. This is observable for anchor transactions that reached broadcastWithCPFP before cancellation.
Useful? React with 👍 / 👎.
| if entry.fsm != nil { | ||
| entry.fsm.Stop() | ||
| } | ||
| } |
There was a problem hiding this comment.
OnStop unregisters watches and stops FSMs but does not evict broadcaster state for active tracked txs. As a result, CPFP fee-input leases are not proactively released on shutdown; with backends that persist leases across restarts, those outpoints remain locked until lease timeout and can block unrelated wallet coin selection after restart.
Useful? React with 👍 / 👎.
handleCancel (last subscriber) and OnStop (active tracked txs) now invoke a.broadcaster.Evict(ctx, txid) so any wallet-level fee-input lease taken during broadcastWithCPFP is released immediately rather than lingering until the wallet's expiry. Without this, cancelling a subscription or restarting the daemon with in-flight CPFP parents left UTXOs locked in backends that persist leases across restarts (btcwallet, lndclient WalletKit), starving unrelated wallet coin-selection paths. Adds two regression tests that assert every recorded LeaseOutput has a matching ReleaseOutput under the package-scoped txconfirmLockID.
Extend the ledger actor's UTXO diff subsystem so every wallet movement gets a definitive classification and gets booked as either a round / sweep attribution (no external leg) or an operator external_deposit / external_withdrawal -- the piece that was left at 'audit-only' in PR #201. Rather than stand up a separate attribution table, piggyback on the existing wallet_utxo_log: * New source_id BYTEA NULL column carries round_id / batch_id when a round or sweep handler pre-inserts an attributed row. * New classifications: withdrawal (spent-side deposit), sweep_consumption (spent-side sweep_return), round_change (naming parity with round_funding), and pending (the two-phase limbo the diff loop uses before reconciliation). * New sqlc helpers PromotePendingWalletUTXOLog (atomic flip of stale pending rows into deposit/withdrawal) and InsertWalletUTXOLog now carries source_id and returns the rowcount so the diff loop can tell a genuine insert from a silent no-op against an already-attributed row. Classifier loop lives in two passes on each BlockEpochMsg: 1. reconcilePendingAuditRows promotes any pending row left behind by the previous block's diff to deposit or withdrawal and books the matching external_* ledger leg via fees.RecordExternalDeposit / RecordExternalWithdrawal. 2. applyUTXODiff inserts the current block's diff rows as pending; pre-inserts from round / sweep handlers short-circuit via the UNIQUE (hash, index, event) constraint so the classifier never double-books. The one-block grace window covers the narrow race where a BlockEpochMsg lands on the ledger actor's mailbox before the matching RoundConfirmedMsg / SweepCompletedMsg from a simultaneously-confirmed round or sweep. Follow-up commits wire the round / sweep producers to populate the new TLV outpoint slices and the handlers to pre-insert the attributed audit rows.
3 participants
Summary
Part 3 of 5 in the stacked split of #235. Adds
txconfirm: ageneric 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 — not tied to unrolling.
9fc7359—txconfirm: add reusable tx confirmation actor with CPFPFeatures:
broadcast is still live and the confirmation watch remains active,
so the next bump waits the full interval before retrying)
New → Broadcasting → AwaitingConfirmation → (FeeBumping loop) → Confirmed/FailedKey types:
TxBroadcasterActor,CPFPBroadcaster,EnsureConfirmedReq.Depends on the
SubmitPackagechain-backend surface added in theprep PR (#260).
Forward-port from the original branch
Imports of
lib/scriptswere migrated tolib/arkscript(matchingmain's removal of the legacy helper package). The fixup was folded
back into Elle's original commit.
Stack
unroll-01-prepunroll-02-planlib/recovery+unrollplanunroll-03-txconfirmtxconfirmactorunroll-04-coreunroll/unroll-05-wireSupersedes #235.
Authorship
Commit authored by @ellemouton.
Test plan
go test ./txconfirm/...go vet ./...go build ./cmd/...