vtxo: retract an unroll whose transaction the chain no longer has - #1185
vtxo: retract an unroll whose transaction the chain no longer has#1185bitcoin-coder-bob wants to merge 5 commits into
Conversation
A vtxo is marked unrolled the moment its outpoint appears on chain, before any confirmation. That is deliberate and protective: the mark blocks the vtxo from being spent inside the Ark while its unroll is in flight. Nothing ever cleared it, so an unroll that is evicted or replaced and never mines left the vtxo wrongly unrolled forever, unspendable by its owner through three guards and unsweepable by the operator through two more. Retraction runs on positive evidence in both directions, never on absence alone, which is the discipline the spend reconciler already follows. An outpoint the wallet still lists as unspent exists on chain and no number of passes may retract past it. Only once that is absent does the transaction lookup decide, and only a backend that positively has no record of the transaction counts, over several passes, so a transaction that has been broadcast but has not reached our node yet is not mistaken for one that is gone. The wallet gains the signal that makes this possible. Asking whether a transaction is confirmed answered false both for one waiting in the mempool and for one the backend has never seen, and the difference is the whole point, so the response carries a not_found flag. It is phrased so false is the safe reading: an older wallet never sets it, every transaction reads as known, and no retraction can fire. The repository write is scoped to vtxos still believed unspent, so it can never clear the mark on one spent inside the Ark and then unrolled, which is the fraud path the sweeper resolves through spent_by.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Self-review found the feature could be silently disconnected and its safety properties were unpinned. Removing the retraction's one call site from the reconcile pass left every test green, because each case drove the function directly. That call is now pinned by a case that runs the reconcile pass. The two wallet-side seams had no coverage at all. The handler is what sets the not-found flag, and setting it on the wrong branch would report a live transaction as gone; the client is what reads it, and the reading is what makes an older wallet degrade to never retracting rather than to retracting blindly. Both are covered, and both fail when the behaviour they describe is inverted. Also drops a comment that justified production code by pointing at the tests.
Testing against a live NBXplorer showed the retraction as committed could never fire. The design assumed a transaction that will not confirm eventually becomes unknown to the backend. It does not. A transaction replaced by a higher-fee conflict, dropped by the node, keeps answering HTTP 200 with zero confirmations indefinitely, so asking whether the backend still knows it always answered yes and no unroll was ever retracted. What the backend does say is replacedBy, naming the transaction that superseded this one, populated once the replacement confirms. That is positive evidence of the kind this reconciler already insists on, and stronger than absence: it says what happened rather than that something is missing. The wallet already parsed the field and dropped it on the floor. So the signal is now "the backend has positively dropped this transaction", either replaced or unknown, rather than "the backend has forgotten it". Both halves keep the property that an older wallet, setting neither, reads as still live, so no retraction can fire against one. Verified end to end against the running regtest stack: a transaction replaced by a confirmed conflict reports its replacement through the real client, and a confirmed transaction reports none. Still not covered, and now known rather than assumed: a transaction that simply expires from the mempool without being replaced is never reported as either replaced or unknown, so it produces no signal at all.
The replacement signal cannot carry this on its own. The transaction that marks a vtxo unrolled is the pre-signed tree transaction, broadcast as a package with a client-signed fee-bumping child. Nobody can produce a conflicting version of a musig2-signed transaction, so it is never replaced; it fails by sitting in the mempool until the node lets it go. Keying only on replacement left the retraction unable to fire for the way an unroll actually fails. NBXplorer cannot see that either. It keeps a transaction in its own index after the node has dropped it, reporting zero confirmations indefinitely, so one waiting and one gone look identical through it. The node is the only component that knows, and NBXplorer will forward the question. So the wallet now answers a single judgement, dropped, meaning the transaction will not confirm: superseded, unknown, or unconfirmed and no longer held by the node. It is computed wallet-side because only the wallet can see all three, and it is false whenever the wallet cannot tell, including on an older wallet that never sets it, so nothing can act on a live transaction. Verified against the running regtest stack through the real client: a transaction the node dropped reports false, one waiting in the mempool reports true, and a confirmed one reports false, which is why the answer combines the mempool question with the confirmation state instead of reading either alone.
Driving this end to end on regtest showed the retraction correctly clearing six vtxos left stuck by earlier runs, and then refusing to clear the one the test had just unrolled and evicted. The unspent-set check was the reason. That check read as the stronger signal: an outpoint the wallet still lists as unspent exists on chain, so nothing may retract past it. But the list is built from the chain backend's own index, which keeps an unconfirmed transaction long after the node has dropped it. With the unroll evicted, the node reported no such output while the backend still listed the outpoint as unspent, so the guard vetoed exactly the case it was guarding. The node's answer already accounts for confirmation, so the check added no safety and only blocked the fix. Retraction now rests on that single piece of positive evidence, which also removes a wallet call from every reconcile pass. Verified on the rebuilt stack: the vtxo whose unroll was evicted is retracted within one reconcile pass, and one whose transaction the node still holds is not.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — #1185
vtxo: retract an unroll whose transaction the chain no longer holds
🚨 Protocol-critical — touches the VTXO unrolled-state machine. Please have a human sign off before merging.
What this does
When a vtxo is marked unrolled its output is blocked from being spent inside Ark while the unroll is in flight — a protective mark. The bug: nothing ever cleared it, so an unroll that got evicted/replaced and never mined left the vtxo permanently unspendable and unsweepable.
This PR adds retractStaleUnrolls, driven by the existing reconcile loop, which clears the mark once the chain backend positively has no record of the transaction — across three consecutive passes to distinguish propagation delay from genuine eviction.
Review
Correctness
The three-pass threshold (unrollRetractionObservations = 3) is a sound design. A broadcast-but-not-yet-received tx reads identical to an evicted one, and requiring multiple stable observations prevents a race.
The deliberate choice not to consult the wallet's unspent index is well explained and correct: the backend's own index keeps evicted mempool txs as unspent long after the node has dropped them, so checking it would veto exactly the retraction this exists to enable.
The in-memory observation counter resets on restart — this is explicitly noted as the safe direction (only delays retraction, never erases real state). Good.
The forgetUnrollObservations cleanup (discarding counts for vtxos no longer in candidates) prevents a resolved vtxo's stale count from being attributed to a future unroll at the same outpoint. Correct.
After retraction, recordUnrollObservation(outpoint, true) clears the entry (via delete). Belt-and-suspenders since the vtxo won't appear in candidates again, but harmless.
Proto API
New fields not_found, replaced_by, dropped on IsTransactionConfirmedResponse use safe defaults — all zero/empty on an older wallet, so callers treating them as unknown-safe won't act on stale-positive signals. Well-documented. Field numbers are non-conflicting (4, 5, 6).
Tests
mockedScanner.IsTransactionDropped and the DroppedCalls helper are clean. The existing reconcile test fixtures are updated consistently.
Minor
The nil-map guard in recordUnrollObservation (initialising s.unrollObservations if nil) is belt-and-suspenders given the constructor already sets it — fine to keep.
Verdict
Logic is sound, edge cases are considered, defaults are safe. Looks ready to merge — pending human review on this protocol-critical path.
2 participants
Closes #1182. Part of #1159, Milestone 5. Stacked on #1174.
Important
Stacked. Based on
feat/unrolled-vtxo-onchain-spend(#1174), so the diff shows that PR's commits until it lands. The change itself is the last commit. Sibling of #1184, not dependent on it: that one widens the spend predicates to the on-chain kind, this one is about the unroll mark, and a vtxo born on-chain was never unrolled.Problem
A vtxo is marked unrolled the moment its outpoint appears on chain, before any confirmation, and nothing ever clears the mark. An unroll that is evicted from the mempool, replaced, or reorged out and never mined leaves the vtxo wrongly unrolled forever.
That has two costs, not one:
VTXO_ALREADY_UNROLLED.Nothing in the system corrects either. This predates #1174.
Why the mark is not simply delayed
The obvious fix, only marking unrolled once the unroll confirms, is the wrong lever and was rejected. The mark is protective: during the mempool window it is what stops the vtxo being spent inside the Ark while its unroll is in flight. Delaying it trades a stale-flag problem for a spendability problem, which is the worse of the two.
So the mark stays immediate and is retracted instead.
Evidence, and why absence alone is not enough
Retraction runs on positive evidence in both directions, the discipline #1174 established for spends, because absence is ambiguous and the failure mode is severe: wrongly retracting a live unroll hands the owner back a vtxo whose output exists on chain.
Two signals, checked in order of strength:
Retraction rests on one piece of positive evidence: the node no longer holds the transaction that would materialise the vtxo, and has not mined it. That single answer covers being superseded, being dropped, and never having been seen.
The wallet's unspent set is deliberately not consulted, and finding out why took the live run. It reads as the stronger signal and was used here at first. It is built from the chain backend's own index, which keeps an unconfirmed transaction long after the node has dropped it. With the unroll evicted, the node reported no such output while the backend still listed the outpoint as unspent, so the guard vetoed exactly the case it was guarding. The node's answer already accounts for confirmation, so the check added no safety and only blocked the fix.
Getting this signal right took two corrections, both from testing against a live stack rather than reasoning.
The first revision assumed a transaction that will not confirm eventually becomes unknown to the backend. It does not: NBXplorer keeps it at zero confirmations indefinitely, so asking whether the backend still knows it always answered yes and nothing could ever be retracted.
The second revision keyed on
replacedBy, which NBXplorer does report. That is real, but nearly unreachable here: the transaction that marks a vtxo unrolled is the pre-signed tree transaction, broadcast as a package with a client-signed fee-bumping child. Nobody can produce a conflicting version of a musig2-signed transaction, so it is never replaced. It fails by sitting in the mempool until the node lets it go.The node is the only component that knows that, and NBXplorer forwards the question. So the wallet combines all three facts into one judgement.
The transaction to ask about needs no new state: the unroll mark fires when the vtxo's own outpoint appears on chain, so the vtxo's txid is the transaction that has to confirm.
A single unknown answer is not enough. A transaction that has been broadcast but has not yet reached our node reads exactly like one that is gone, so retraction requires several consecutive passes. The count lives in memory: losing it on a restart only delays a retraction, which is the safe direction.
A lookup error is not evidence and does not advance the count, so a flapping backend cannot accumulate its way to a retraction.
The wallet signal this needed
Asking whether a transaction is confirmed answered
falsefor one waiting in the mempool, for one that was replaced, and for one the backend has never seen.IsTransactionConfirmedResponsetherefore gainsreplaced_by,not_found, anddropped: the wallet's judgement that the transaction will not confirm, being superseded, unknown, or unconfirmed and no longer held by the node. It is computed wallet-side because only the wallet can see all three, and it is false whenever the wallet cannot tell.Both were verified against a live NBXplorer 2.6.7 rather than assumed. A funded, tracked output was spent, that spend was replaced by a higher-fee conflict, and the conflict was mined:
confirmations: 0,replacedBy: nullconfirmations: 0,replacedBy: nullconfirmations: 0,replacedBy: <txid>confirmations: nThe node's answer alone cannot separate confirmed from gone, which is why
droppedcombines it with the confirmation state rather than reading either on its own.Both fields are additive and phrased so the zero value is safe. An older arkd-wallet sets neither, every transaction reads as still live, and no retraction can fire. That is the same degradation shape #1174 used for its new RPCs, without needing an
Unimplementedpath.Write path
UnmarkVtxosUnrolledon all three backends, scoped to vtxos still believed unspent so it can never clear the mark on one spent inside the Ark and then unrolled, which is the fraud path the sweeper resolves throughspent_by. No migration: the column already exists.It runs in the existing reconcile pass rather than a second loop, over the same candidate set #1174 already loads. Same set, the other direction: a spend that never confirmed is retracted there, an unroll that never confirmed is retracted here.
Known divergence, recorded rather than hidden
Badger zeroes
ExpiresAtwhen it marks a vtxo unrolled; the SQL backends leave it alone. That divergence predates this PR. Retraction cannot restore what badger destroyed, so a retracted vtxo keeps a zero expiry on badger and its original expiry on SQL. Noted in the code at the point it matters.Test plan
ark_txid. Verified to fail on badger with the guard removedgo buildand the full suites on both modules,make lint0 issuesConfirmed by the end-to-end run
Two premises this design rests on were checked rather than assumed, and both hold: the transaction that materialises a vtxo is the vtxo's own txid, so no extra state is needed to know what to ask about; and arkd marks a vtxo unrolled straight from the mempool, before any confirmation, which is what makes the mark protective and is why it is retracted rather than delayed.
Not covered
The node question depends on NBXplorer's RPC proxy being enabled. Where it is not, that lookup fails,
droppedstays false, and the retraction degrades to firing only on an explicit replacement, which for an unroll is nearly never. That is a deployment-visible limitation rather than a silent one, since the failure is logged.A deep reorg that unmines a confirmed unroll is not exercised by a test here.