tapfreighter: resolve stranded transfers against the chain#2163
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses the issue of stranded transfers in the tapfreighter service. Previously, transfers could remain in a 'pending' state indefinitely if their anchor transaction failed to confirm. The changes introduce a mechanism to query the chain for confirmed spends of transfer inputs. If a conflicting transaction is found to have spent the inputs, the transfer is marked as superseded, allowing the system to clean up its state and stop attempting to resume the transfer. This ensures better resource management and prevents unnecessary retries for transactions that can no longer confirm. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
614a59a to
a667f75
Compare
![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 resolves stranded transfers against the chain when their anchor transaction can no longer confirm by monitoring spend notifications for the transfer's inputs. If a conflicting transaction spends any input, the transfer is marked as superseded and its locked inputs are released. The feedback highlights a critical issue where spent asset-level inputs are not marked as spent in the database, potentially causing 'ghost' balances. Additionally, a potential panic in a mock implementation was identified, and an optimization was suggested to avoid redundant database queries for zero-value inputs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
7c73ce2 to
23a0489
Compare
|
@jtobin, remember to re-request review from reviewers when ready |
…idge Add RegisterSpendNtfn to the ChainBridge interface, mirroring the existing RegisterConfirmationsNtfn: registrations for outpoints that were already spent by a confirmed transaction are dispatched immediately, so a spend lookup doubles as a query of historical chain state. The lnd-backed bridge wraps lndclient's RegisterSpendNtfn with a reorg channel internally and exposes it via the returned SpendEvent's Reorg field, so callers can react when a previously reported confirmed spend is reorged out. The supplycommit mock already carried a RegisterSpendNtfn method with a slightly different signature; it is adjusted to match the interface. MockChainBridge gains SetSpend, SendSpendNtfn, SendSpendReorg, SendSpendErr, FailConfFor, and ConfReqForTxid helpers so test code can target spend events, reorgs, and stream errors per outpoint, and target conf-registration failures and lookups per txid, independent of the order the porter happens to issue registrations in.
MarkTransferSuperseded marks the unconfirmed transfer anchored by a given transaction as superseded, complementing the existing confirmation-time supersession of conflicting transfers with a way to mark a transfer whose conflict was discovered directly. A transfer whose anchor transaction has confirmed is never eligible; marking an already superseded transfer again succeeds, so concurrent marking through both paths is safe. The transfer's input assets are also marked as spent in the same DB transaction, but only those whose anchor_point appears in the caller-supplied spentOutpoints list — these are the outpoints the conflicting transaction actually consumed on-chain. A multi-input transfer whose conflict only consumed one input leaves the others available, rather than over-aggressively removing them from coin selection. Without this bookkeeping the consumed inputs would resurface as available when their leases expired, leading to ghost balances and a self-reinforcing broadcast-rejection-and-supersede loop on every retry. asset_transfer_inputs only holds rows for real asset inputs (sweep / zero-value inputs are tracked separately), so an ErrNoRows from SetAssetSpent on a listed outpoint is a genuine data anomaly — a script-key mismatch or stale record — not a benign case. Log loudly so the operator sees the inconsistency, but still continue: the on-chain reality is that the input is gone, and blocking the supersession on a stale asset record helps no one. FetchAnchorOutputPkScripts returns the pkScripts of the given anchor outpoints, indexed by outpoint. Source transactions are fetched and deserialized at most once per unique txid, so a transfer whose inputs all spend from the same prior tx incurs one chain_txn fetch rather than one per input. Used by the chain porter to register spend notifications on transfer inputs.
When broadcasting a transfer's anchor transaction was rejected as a double spend, the porter previously aborted the parcel and left the transfer pending forever: it was resumed on every startup, rejected again, and aborted again, while also inflating the unconfirmed transfer count indefinitely (the steady state observed in lightninglabs#1888). A double spend rejection means the chain knows something we don't, and the chain can be interrogated directly. Add a chain query (locateConfirmedInputSpend) that registers spend notifications on every transfer input, dispatching historical confirmed spends immediately, and returns the first confirmed spender it sees. Underneath it sits registerInputSpendNtfns / watchInputSpend, which fan per-input spend events, reorg signals, and stream errors into a single per-watch result channel; both helpers are reused by the confirmation-waiting state in a later commit. On ErrDoubleSpend in SendStateBroadcast: if a confirmed spender is located (our own anchor or any other transaction), transition to SendStateWaitTxConf and let that state finalise the outcome. If the lookup is inconclusive (no confirmed spend located within the short query window — the conflict may still be unconfirmed), release any fee inputs and retry the broadcast on next startup, preserving today's behaviour for that case.
A parcel in the confirmation-waiting state only watched for the confirmation of its own anchor transaction. If a conflicting transaction confirmed spending one of the parcel's inputs, the transfer could never confirm, yet nothing resolved it: the parcel waited forever, and the transfer stayed pending across restarts. For parcels whose anchor transaction is broadcast by an external system (lnd's sweeper for force-close sweeps, the channel peers for commitment transactions), this was the only way to learn of a lost race, since such parcels never attempt a broadcast of their own and so never reach the double spend resolution at the broadcast state. A remote channel party claiming an HTLC output ahead of our sweep, or the remote commitment confirming ahead of ours, left a permanently pending transfer. Register spend notifications for the parcel's inputs alongside the confirmation notification. Registration is all-or-nothing across the input set: silently dropping coverage for any input would let a foreign confirmed spend of the unwatched input go unnoticed, recreating the very stranding this machinery exists to prevent. A 1-confirmation foreign spend is, however, not by itself sufficient to act on. Supersession marks the transfer superseded=true and its input assets spent=true atomically in the DB, and there is no inverse operation. A routine 1-block reorg of the conflicting spender would then convert a recoverable stall into silent, permanent balance loss — strictly worse than the pre-PR behaviour. Instead, the porter registers a SafeDepth confirmation notification on the spender tx (using its own SpendingTx pkScript and SpendingHeight as lookup hints) and only calls supersedeTransfer once that finality depth is reached. SafeDepth is plumbed from the existing --reorgsafedepth config (defaults 6 mainnet, 120 testnet) and clamped to at least 1. Pending finality state is keyed per outpoint, not per parcel: two inputs with distinct conflicting spenders each get their own finality watch, so a reorg of one cannot cancel the other's verdict. Per-input spend stream errors trigger recovery of just that input's spend notification, leaving the conf ntfn and the other inputs undisturbed; tearing down everything on each per-input flap would let a chronically-erroring stream indefinitely delay an imminent own-anchor confirmation. Spend reorg signals cancel any pending finality watch on the affected input; the input's spend channel will re-fire if and when the input is spent again on the dominant chain. ErrTransferSuperseded is exported as a wrapped sentinel returned on the deliberate-supersede path. mainEventLoop checks for it via errors.Is and logs at Info instead of Error; subscribers see the same AssetSendErrorEvent shape but can branch on errors.Is to treat it as a benign terminal state rather than a real failure. Transient DB write failures keep a sentinel-free error. supersedeTransfer marks the transfer superseded in the asset store before unlocking inputs: the DB write is the durable state change; unlocking is transient (lnd's wallet re-derives availability from the chain), and mark-then-unlock preserves the invariant that the durable change always lands first.
locateConfirmedInputSpend's 10-second budget on every ErrDoubleSpend broadcast rejection used to be a hardcoded constant. On slow or rescanning chain backends 10s can be insufficient; the lookup returns nil (inconclusive), the porter unlocks and waits for the next startup, the broadcast hits ErrDoubleSpend again, repeat — without any visible signal that the timeout is the bottleneck. Lift the constant to ChainPorterConfig.SpendQueryTimeout, expose it via a new --spendquerytimeout config flag (defaults to 10s in tapcfg, the same value as before), and emit a Warnf when the timeout fires with no confirmed spend observed — so operators seeing repeated inconclusive cycles can correlate them with the setting and raise it. A zero value at the porter layer falls back to defaultSpendQueryTimeout.
Adds an integration test for the broadcast-state recovery path the chain porter takes when a transfer's anchor transaction has confirmed on-chain but the porter died before observing the confirmation. On the next startup the parcel is resumed at SendStateBroadcast; the rebroadcast attempt either silently succeeds (if the chain backend ignores re-publishing an already-confirmed tx) or returns ErrDoubleSpend, in which case locateConfirmedInputSpend identifies the parcel's own anchor as the confirmed spender and transitions to SendStateWaitTxConf. Either way the historical confirmation event drives the transfer to completion. Before the stranded-transfer recovery work, the ErrDoubleSpend branch aborted the parcel and re-tried the same broadcast on every startup (issue lightninglabs#1888). The test asserts the operational outcome — that the receiver eventually sees the asset and the sender's transfer is reported as complete with a confirmed anchor height. Two related deficiencies remain uncovered at the itest level and are documented inline as follow-up: - The conflicting-spender resolution: where a foreign transaction reaches SafeDepth and marks the transfer superseded. Constructing a key-path P2TR spend of an asset's anchor UTXO from outside tapd requires harness tooling that doesn't exist today. - The reorg-rescue path: where a 1-conf foreign spend is reorged out before reaching SafeDepth. Covered at the waitForConfEventOnce unit-test level (TestWaitForTransferTxConfReorgRescue) but not end-to-end here.
23a0489 to
024ce9b
Compare
2 participants
Resolves #2160.
Resolves permanently-pending transfers by querying the chain when broadcast is rejected, and by watching the inputs (not just the anchor tx) while we wait for confirmation.
The vast majority of the +LoC here is in tests. I think the "actual change" is about +400 LoC, mostly to the chain porter and asset store.