fix(utxo): preserve replay protection when pruning - #1702
Conversation
Persist Aerospike parent markers before removing children and remove probabilistic update skipping. Add atomic SQL parent markers and reject replay of pruned children in both SQL spend paths. Cover collisions, pagination, failures, retries, TTL pruning and SQL schema upgrades. Fixes bsv-blockchain#1701
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
🤖 Claude Code Review Status: Complete Reviewed this consensus-critical UTXO change (pruned-replay protection across Aerospike, SQL/Lua, block validation and legacy netsync). The change is large but exceptionally well-documented, and each fix is backed by a test proven to fail without it. I traced the core invariants across all three stores and both block paths; they hold. Verified — no issues found:
Stated residuals (skip-lock disables the mark; the Spend/Create prune-gap; non-retroactive markers on already-pruned backlogs) are documented in code and the PR body, not hidden. History:
Advisory only — human reviewers make the final call. Given the consensus surface, a live Aerospike (native-op) integration run against the new Lua contract is worth confirming before merge, since CE-with-Lua tests do not exercise the server-fork native |
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-09-08 16:46 UTC |
… cycle A record whose input references cannot be recovered (external blob gone, or a key that will not build) made getTxInputsFromBins return a ProcessingError. That error unwound through processRecordChunk, the chunk group and partitionWorker into PruneWithPartitions, which classifies a non-TimeoutError as fatal and does not retry it. One anomalous record therefore blocked all pruning node-wide, every cycle, and discarded the partial progress of sibling partitions. Retention is still the right call for such a record: its own presence is the replay protection its parents would otherwise inherit. It is now retained on its own. The record is counted as skipped, a new utxo_pruner_input_resolution_errors_total counter is incremented, one error line is logged per chunk, and the rest of the batch prunes normally. Parent markers are now staged in a per-record map and merged into the chunk-wide map only once every input resolved, so a mid-record failure cannot leave partial markers behind for a record that is then skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jgtsr5ZpUmn8HqDzk23uPM
Build the SQL candidate-materialization statement as a complete literal per branch instead of concatenating "CREATE TEMP TABLE ... AS " onto a fragment at runtime. Nothing in the statement ever came from data, but Sonar flagged the concatenation as a Major vulnerability and the literal form is clearer anyway. Record why PrunedTxSet stays in the tree: no production path calls it after this PR, but TestPrunerReplayProtection/filter_collision uses NewPrunedTxSet to build the controlled collision that proves the replay marker now survives one, so deleting the type would delete that regression fixture. The stale "soft cap on PrunedTxSet entries" comment in settings.go now says deprecated, matching the struct tag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jgtsr5ZpUmn8HqDzk23uPM
|
Round 2, on the two non-blocking items plus the Sonar gate. Pushed 597ca1c. PrunedTxSet dead code. Confirmed by repo-wide grep: Sonar quality gate (was failing). Two issues, both now accounted for:
Benchmarks: the comparison report on 6304b5a shows 0 regressions and 0 improvements across 139 benchmarks, which is consistent with the tradeoffs being on paths that suite does not exercise. The throughput caveat in the PR body still stands: the extra Aerospike round-trip, the serializable pruning transaction and the per-spend EXISTS subquery are not throughput-benchmarked. CI: all 30 check-runs on 6304b5a completed with zero failures. |
Two comments still pointed at executeBatchCleanupCombined, which this PR removes. buildDeletionBatchRecords now has one caller, executeBatchDeletions, because parent markers are always written in their own batch first, and the TestTallyParentUpdateResults header no longer names a function that is gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jgtsr5ZpUmn8HqDzk23uPM
|
Round 3, on the three minor observations from the last review. Pushed abd997e. Stale Two round-trips on the non-defensive path, and records with no recoverable inputs retained indefinitely at Error level: both deliberate, both already documented at the code, no change. On the second one, the log is once per chunk with a sample error rather than once per record, and Sonar gate now passes on 597ca1c, down from failing. The Major "dynamically formatted SQL query" vulnerability is gone. One new issue remains, the cognitive-complexity smell on CI: all check-runs on 597ca1c completed with zero failures. |
The skip comment and its log line both implied eventual progress. For a transaction whose external blob is gone for good, resolution fails on every cycle, so retention is permanent. Both now say so and point at utxo_pruner_input_resolution_errors_total as the thing to alert on. The line drops from Error to Warn: the prune cycle succeeded and the records are safe where they are, so a per-cycle Error on every affected chunk was the wrong level for a stable condition. Also record why the sibling branch deletes an outputs-only record without markers while a fully-missing one is retained. An outputs blob is written only for zero-input transactions, guarded on len(tx.Inputs) == 0 at both write sites in create.go, so such a record has no parents that could need a marker. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jgtsr5ZpUmn8HqDzk23uPM
|
Head is now cbd808d: a merge of Cleanup left recreated descendants spendable (P1). For a pruned chain P -> C -> D, replaying C and D recreates both; C hits P's marker, but D spends the freshly recreated C and succeeds, so the cleanup removed C and left D mined with an unspent output that E had already consumed. Expression spending bypassed the marker after Unspend (P1). Confirmed with the fixture at A transient cleanup failure became permanent (P1). The exclusion of every One residual, stated on Master marker map unbounded in defensive mode (P2). The master copy is removed entirely, as oskarszoon asked; details in the reply to his comment. 1e289d2. Merge with main. #1502 added Sonar. The gate was failing on 74.7% coverage on new code. The five new Aerospike replay subtests cover the defensive scan ( Verification. |
freemans13
left a comment
freemans13
left a comment
There was a problem hiding this comment.
Two P1 correctness issues reproduced against the real SQLite store at cbd808d; details are attached inline.
Validation: existing targeted SQL, Aerospike pruner, blockvalidation, and legacy netsync regression tests passed. Two additional blockvalidation regression tests fail: one verifies that deleting a recreated descendant releases its fresh spend of an unrelated output; the other verifies that a replay is cleaned up when its input has been unspent and spent by a replacement transaction.
| } | ||
| } | ||
|
|
||
| err = store.DeleteComplete(ctx, hash) |
There was a problem hiding this comment.
[P1] Reverse successful descendant spends when deleting replay ghosts
DeleteComplete removes the transaction's own records but does not reverse its input spends. A descendant can successfully spend both a recreated pruned transaction and an unrelated valid output; PrunedReplayGhosts then selects that descendant for deletion, leaving the unrelated output spent by a transaction that no longer exists. This affects both blockvalidation and legacy netsync through this shared helper.
Reproduced using newPrunedChainFixture on the real SQLite store: create a new descendant with inputs (f.child, 0) and (f.parent, 1), then run createAndSpendUTXOsForBatch on [f.child, descendant]. The block fails with the pruned-spend error and the descendant is deleted, but a subsequent SpendAndCreate(f.sibling), which legitimately spends (f.parent, 1), fails with UTXO_SPENT naming the deleted descendant. Compensation needs to track and unwind the descendant's successful spends of surviving outputs, with the appropriate spender checks, as well as delete its record.
There was a problem hiding this comment.
Fixed in 672021f. PrunedReplayGhosts now returns the transactions rather than their hashes, and DeleteCreated first releases, for every ghost, each input whose parent is not itself a ghost, through Store.Unspend with the ghost named as the spender. Both stores match on the spending data (SQL: AND spending_data = $3; Lua: bytes_equal(existingSpendingData, expectedSpendingData)), so an output already rolled back, or since taken by another spender, is left alone. Inputs whose parent is a ghost are skipped, since that record goes.
Ordering matters and is deliberate: the release runs for every ghost before the first delete. Aerospike's unspend verifies the UTXO hash, which for an outpoint-only replay has to be fetched through PreviousOutputsDecorate, and a ghost parent must still exist when its ghost child is decorated; the previous per-ghost interleaving would have raced that.
Your repro is now TestQuickValidateCompensationReleasesDescendantSpends: descendant with inputs (child, 0) and (parent, 1), block [child, descendant], then SpendAndCreate(sibling) on (parent, 1) must succeed. With the release stubbed out it fails exactly as you describe, UTXO_SPENT naming the deleted descendant. TestDeleteCreatedReleasesSurvivingSpends pins which spends are released, with what spender and vin, and that outpoint-only inputs are decorated first. Legacy netsync goes through the same helper, so it gets the same behaviour.
| validationErrors[i] = errors.NewProcessingError(errFailedCreateSpendingData, parseErr) | ||
| continue | ||
| } | ||
| validationErrors[i] = errors.NewUtxoSpentError(*spend.TxID, spend.Vout, *spend.UTXOHash, existingSpendData) |
There was a problem hiding this comment.
[P1] Check the replay marker before returning a spend conflict
This branch returns ErrSpent before evaluating childPruned below; trySendSpendBatchBulk has the same ordering. After Unspend clears a pruned child's input and a replacement transaction spends it, replaying the child produces ErrSpent instead of ErrUtxoSpendingTxPruned. The block create phase has already recreated the child, but spendBatchWithRetry does not add it to prunedReplays, so compensation leaves the recreated record behind.
Reproduced on the real SQLite store using newPrunedChainFixture: Unspend (f.parent, 0) with f.child's spending data, successfully SpendAndCreate an alternative transaction spending that output, then run createAndSpendUTXOsForBatch on [f.child]. Validation fails with UTXO_SPENT, but store.Get(f.child.TxIDChainHash()) succeeds afterward instead of returning ErrTxNotFound. Give the pruned-child marker precedence over the conflicting-spender branch in both SQL implementations so the caller can reliably identify and remove the replay.
There was a problem hiding this comment.
Fixed in 83082df: the childPruned check now runs before the conflicting-spender test in both the bulk and the per-row path.
Aerospike needed no change, and I checked rather than assumed: the Lua spend already evaluates BIN_DELETED_CHILDREN before the already-spent handling, and the expression path's first-seen clause fails on any element that is not the bare hash, so a conflict is always re-issued through Lua. The new replaced_parent and expressions_replaced_parent subtests of TestPrunerReplayProtection pin that with require.NotErrorIs(err, errors.ErrSpent).
Your repro is TestQuickValidateRemovesReplayAfterReplacementSpend on the block path and TestPrunedReplayRejectedAfterReplacementSpend on the store (sqlite, which takes the per-row path, and postgres, which takes the bulk path). With the old order restored, the block test fails with UTXO_SPENT ... already spent by <replacement> and the store test fails on the error code, on both engines.
Both SQL spend paths returned ErrSpent for a conflicting spender before they looked at the replay marker. Once a rollback had cleared a pruned child's input and a replacement transaction had taken it, replaying the child was answered ErrSpent instead of ErrUtxoSpendingTxPruned. The block paths cannot tell that from an ordinary double spend, so the record their create phase had written for the replay was never compensated and survived. Reproduced by a reviewer on the real SQLite store. The marker check now runs first in both the bulk and the per-row path. Lua already checks it before the already-spent handling, and the expression path hands every conflict to Lua, so Aerospike needed no change; the new replaced_parent subtests pin that. TestPrunedReplayRejectedAfterReplacementSpend (sqlite and postgres) rolls the spend back, lets a replacement take the output, and asserts the replay is still answered with the pruned-replay code and not ErrSpent; restoring the old order fails it on both engines.
…ing it DeleteComplete removes a transaction's own records and does not reverse its input spends. A descendant ghost can have spent both the recreated pruned transaction and an unrelated, valid output in the same block; deleting its record alone left that output recorded as spent by a transaction that no longer exists, and its legitimate spender was refused with ErrSpent naming a ghost. Reproduced by a reviewer on the real SQLite store, on both block paths through the shared helper. PrunedReplayGhosts now returns the transactions rather than their hashes, and DeleteCreated first unspends, for every ghost, each input whose parent is not itself a ghost, naming the ghost as the spender so both stores' own match protects any other spender. Inputs whose parent is a ghost are skipped, since that record goes. The release runs for every ghost before the first delete: Aerospike's unspend verifies the UTXO hash, which for an outpoint-only replay has to be fetched through PreviousOutputsDecorate, and a ghost parent must still exist when its ghost child is decorated. Tests, each proven by breaking the fix: - TestDeleteCreatedReleasesSurvivingSpends asserts which spends are released, with what spender, and that outpoint-only inputs are decorated first. - TestQuickValidateCompensationReleasesDescendantSpends runs the reviewer's scenario on a real store and asserts the surviving output is spendable by its legitimate spender afterwards. - TestQuickValidateRemovesReplayAfterReplacementSpend pins the previous commit at the block level: the recreated replay is removed and the replacement is untouched.
freemans13
left a comment
freemans13
left a comment
There was a problem hiding this comment.
Re-review of 672021f: both original regression tests now pass, but the new cleanup introduces two P1 regressions reproduced against the real SQLite store. I recommend addressing the inline findings before merging.
Updated targeted UTXO, SQL, blockvalidation, and legacy netsync tests passed. Two additional regression tests fail: TestReview1702CleanupPreservesHistoricalSpend and TestReview1702CleanupWithAlreadyPrunedInputParent.
| tx := tx | ||
|
|
||
| releaseG.Go(func() error { | ||
| err := retryStoreCall(ctx, func() error { return releaseSurvivingSpends(ctx, store, tx, ghostSet) }) |
There was a problem hiding this comment.
[P1] Preserve historical spends when compensating a rejected replay
The release loop unspends every ghost, including the pruned transaction whose spend was rejected. Its input spending data still identifies the original confirmed spend, so matching the spender does not distinguish that historical spend from a new spend made by this attempt. Cleanup clears the historical spending_data even though the replay never committed that input. The deleted_children marker only rejects the original child txid; a different transaction can now spend the output.
Reproduced on the real SQLite store with newPrunedChainFixture: construct an alternative transaction spending (f.parent, 0). SpendAndCreate initially returns ErrSpent, since the mined child consumed that output. Run createAndSpendUTXOsForBatch on [f.child]; the replay is rejected and its recreated record is deleted. SpendAndCreate of the same alternative then succeeds. This turns replay rejection into a way to release an already-confirmed output.
Compensation must distinguish spends newly committed by this attempt from historical spends. In particular, deleting a rejected root must not unspend the original confirmed input that its replay marker protects.
There was a problem hiding this comment.
You are right, and the conclusion is stronger than "distinguish fresh from historical": the release step was the wrong design, and abe415d removes it entirely rather than refining it.
A replayed transaction carries the same identity as its original, so a parent output that names the ghost as its spender cannot say whether that spend is the confirmed one or one this attempt just made, and neither store's spender-matched Unspend can either. So the question is whether a ghost can ever hold a fresh spend on a surviving output, and in every consistent history it cannot. A rejected root's spend of the markered output never committed, and the store rolls back its fresh sibling spends itself (needsSpendRollback). A dependent that spends a surviving output either hit that output's marker for itself, in which case it was rejected and is a root, or the output carries no marker for it, which means it was pruned by code that wrote none and has recorded that spend all along, so the re-spend was idempotent. Deleting the record and leaving the spends is exactly the state a normal prune leaves.
The one state where a release would have helped is a brand-new transaction freshly spending an unrelated, unspent output alongside a markered sibling, which is what my earlier descendant test modelled. No consistent history produces it, and in that state the outcome without a release is a burned output rather than a double spend, which is the direction to fail in. That test is gone; the reasoning is on DeleteCreated.
Your repro is now TestQuickValidateCompensationPreservesHistoricalSpends/the_rejected_root's_confirmed_input_stays_spent: the alternative gets ErrSpent before the replay and still gets ErrSpent after. The dependent form is the second subtest, which models a pre-upgrade prune by deleting the dependent directly (no marker) before the pruner removes its parent, then replays both and asserts the surviving output is still spent. With the 672021f compensate_created.go restored, both subtests fail; TestDeleteCreatedNeverTouchesSpends pins that no Unspend and no decorate is issued.
| }) | ||
| } | ||
|
|
||
| return store.Unspend(ctx, spends) |
There was a problem hiding this comment.
[P1] Do not let already-pruned input parents block replay cleanup
Being outside ghostSet does not establish that an input parent still exists. This call includes already-pruned parents, and SQL Unspend returns ErrNotFound for a missing output. After three identical failures, releaseG.Wait aborts the entire delete phase, leaving the recreated replay record behind. For undecorated inputs, PreviousOutputsDecorate can fail on the missing parent before Unspend is reached as well.
Reproduced on the real SQLite store: create P with two outputs, Q with one output, and C spending P:0 and Q:0. Mine them, spend and mine C's only output, then prune Q and C while P survives and retains C's marker. Replaying C through createAndSpendUTXOsForBatch correctly hits P's marker, but compensation repeatedly fails trying to unspend the already-pruned Q:0. store.Get(C) succeeds afterward instead of returning ErrTxNotFound. No transient retry can restore Q.
Handle absent parent outputs without preventing deletion of the replay, while still ensuring that other outstanding spends are released. Simply swallowing a batch-level ErrNotFound is insufficient because SQL rolls back the whole Unspend transaction.
There was a problem hiding this comment.
Fixed in abe415d by removing the release step, which was the only thing that touched the missing parent; the cleanup now deletes the replay's record and issues no Unspend or decorate, so an already-pruned input parent has nothing to block.
Your repro is TestQuickValidateCompensationSurvivesPrunedInputParent: P with two outputs, Q with one, C spending P:0 and Q:0, E spending C's only output, mined, then a prune that removes Q and C while P survives with C's marker. Replaying C is rejected on P's marker and store.Get(C) returns ErrTxNotFound afterwards. With the 672021f version restored it fails as you describe, on the record still being present.
The release step added in 672021f was wrong in design, not just incomplete, and a reviewer reproduced both consequences on SQLite. A replayed transaction carries the same identity as its original, so a parent output that names the ghost as its spender cannot say whether that spend is the confirmed, historical one or one this attempt just made, and the stores' spender-matched Unspend cannot tell them apart either. Releasing a rejected root's input therefore cleared the confirmed spend its marker protects, and any new transaction could then take the output: replay rejection had become a way to free a confirmed output. And an input whose parent was itself pruned made Unspend fail with not-found on every retry, which aborted the delete phase and left the recreated replay behind. Every spend a ghost holds on a surviving output is the historical one. A rejected root's spend of the markered output never committed and the store rolls back its fresh sibling spends itself. A dependent that spends a surviving output either hit that output's marker for itself, in which case it is a root, or the output carries no marker for it, which means it was pruned by code that wrote none and has recorded that spend all along, so the re-spend was idempotent. Deleting the record and leaving the spends is exactly the state a normal prune leaves. DeleteCreated now deletes and nothing else; the reasoning is on the function. The one state in which a release would have helped, a brand-new transaction freshly spending an unrelated unspent output while a marker for its sibling exists, has no consistent history behind it, and there the outcome is a burned output rather than a double spend. Tests, each proven against the previous behaviour: - TestQuickValidateCompensationPreservesHistoricalSpends: the rejected root's confirmed input stays spent (the reviewer's repro), and a dependent's historical spend of a surviving output stays spent, modelled by pruning the dependent without a marker before the pruner removes its parent. - TestQuickValidateCompensationSurvivesPrunedInputParent: a replay whose other input parent is pruned and gone is still removed. - TestDeleteCreatedNeverTouchesSpends pins that no Unspend and no decorate is issued.
… the call wrote, and let the marker win Four defects found by an adversarial review panel, each reproduced by a failing test against the real stores before it was fixed. A replay of a chain the pruner removed end to end was blessed. The marker lives on the parent record, so once the parent is pruned too nothing in the store says the child existed. Both stores answer a spend whose parent record is gone with "already blessed" when the spending transaction's own record exists, on the reasoning that it was validated before its parent was pruned. The create-first block paths write that record before they spend, so the replay was blessed by the copy it had just created: the block validated and the child came back mined with unspent outputs that a confirmed grandchild had consumed. The block paths now say which records they wrote in this pass (WithSpenderCreatedByCaller, on the store's IgnoreFlags and as a validator option); for those the fallback is off, the missing parent surfaces, and a created-here transaction whose spend fails on a missing parent is classified as a ghost like a marker hit (utxo.IsPrunedReplayRejection). A pre-existing child with a pruned parent is still blessed, which is what the fallback exists for. The validator's own copy of the fallback is gated too; with these stores it is shadowed by the aggregate ErrUtxoError they return, so that gate is defensive. Rejecting a replay released its confirmed sibling spends. A replay rejected on one parent's marker took the idempotent branch on an un-markered sibling parent, whose output already recorded exactly that spend from history, and the rollback then unspent it: the stores' spender-matched Unspend cannot tell a historical spend from a fresh one. The panel reached that state through this PR's own hold-back path (one parent's marker landed, the sibling's failed, the child was retained). The rollback now reverses only what the call wrote. SQL marks an idempotent match on the batch item; the Lua spend reports idempotent indexes in a new "idempotent" list and the Go side flags the items before completing them; both keep flagged items out of the rollback while still counting them as success. LuaPackage is teranode_v63. A native-op server that reports no such list gets the previous behaviour. The marker now takes precedence over every other answer about an output (frozen, conflicting, locked, height-gated, spent by another, hash mismatch) on both SQL paths. A replay of a frozen output was answered ErrFrozen, which the block paths cannot tell from an ordinary failure, so the record their create phase wrote for it was never compensated. Lua already checked the marker before the frozen and already-spent answers; it now also checks it before the height gate. The SQL pruner's retry on a serialization or lock conflict never fired on Postgres: each statement wrapped its error with errors.NewStorageError, which replaces a foreign error with a bare *errors.Error carrying only its message, so isPruneRetryable never saw the driver code. The attempt now reports through pruneStepError, which keeps the driver error reachable, and the final error is wrapped for the caller after the retry decision. Also stated as an accepted residual, on PrunedReplayGhosts and in the PR body: a dependent written by an interrupted earlier attempt at the same block exists when the next attempt starts and cannot be told from a legitimately pre-existing one; closing that needs a durable per-transaction mark the stores do not have. Tests, each proven by breaking its fix: - TestQuickValidateRejectsReplayWhoseParentWasAlsoPruned through the production quick-validate path; TestSpendDoesNotBlessSpenderCreatedByCaller on SQLite, Postgres and Aerospike (fails with either store's gate removed); TestValidateDoesNotBlessCallerCreatedSpender proves the flag reaches the store through the validator on the legacy path. - TestPrunedReplayRollbackPreservesHistoricalSpend (SQLite, Postgres) and TestPrunedReplayRollbackKeepsSiblingHistoricalSpend (Aerospike, through the hold-back path); TestPrunedReplayRollsBackSiblingSpends still passes, so the fresh-spend rollback icellan asked for is intact. - TestPrunedReplayRejectedBeforeFrozen (both engines) fails with the old order restored; the Aerospike frozen_parent subtest pins existing behaviour. - TestIsPruneRetryableSeesTheDriverErrorThroughAStepError, with the old wrapping as the control that does not match.
|
Before pushing the fixes for the last review I ran an adversarial panel over the branch: eight independent reviewers, each finding then handed to a reproducer that had to produce a failing test on the real store and to a skeptic told to refute it. Seven reproductions survived, in four groups. All four are fixed in this push, each with the reproducing test adopted and proven by breaking the fix. A replay of a chain the pruner removed end to end was blessed (P0). The marker lives on the parent, so once the parent is pruned too nothing in the store says the child existed. Both stores' "already blessed" fallback then cleared the missing-parent error because the spending transaction's record exists, and it exists because phase 1 had just written it. On the quick path the block validated and the child came back mined with an unspent output that a confirmed grandchild had consumed; the reproducer unlocked it the way the post-accept pass does and spent it a second time. On the legacy path the store's bless is reached through the validator; the validator's own copy of the fallback is gated too, but with these stores it is shadowed by the aggregate error they return, so it is defensive. Fix: the block paths tell the store and the validator which records they wrote in this pass ( Rejecting a replay released its confirmed sibling spends (P0). A replay rejected on one parent's marker took the idempotent branch on an un-markered sibling parent (the output already recorded exactly that spend, from history), and the rollback then unspent it, because spender-matched The marker now takes precedence over every other answer about the output (frozen, conflicting, locked, height-gated, spent by another, hash mismatch) on both SQL paths. A replay of a frozen output was answered The SQL pruner's retry never fired on Postgres (P2). The attempt wrapped the driver error with One reproduction is not fixed, on purpose, and is now stated as a residual. A dependent written by an interrupted earlier attempt at the same block exists when the next attempt starts, answers Sixteen of the panel's verifications were cut off by a session limit before they ran; the findings they covered were the same four themes from other lenses, so nothing unverified is unaddressed, but I am running a second, smaller pass on this head to confirm the panel comes back dry. |
…'s own write, carry the bless flag over the wire, and reset the idempotent mark per retry A second adversarial panel pass at 3fa6315 found ten defects, every one reproduced by a failing test on the real stores and none refuted. They fall into four groups; all are fixed here, and two test-quality items are closed with the panel's own tests. A leftover of an interrupted attempt was blessed on the retry. The created-here gate was derived from ErrTxExists in the CURRENT attempt, so a record an earlier attempt at the same block wrote and never finished with (a crash or cancellation between the create phase and the compensating delete, a delete that failed past its retries, a spend phase that saw only retryable errors, or an Aerospike create batch the client re-sent and got KEY_EXISTS back for) was filed as pre-existing. For a replay of a chain the pruner removed end to end that is fatal: no marker exists anywhere, the missing-parent bless was back on, phase 1.5 had already unlocked the record, and the block validated with the child's outputs unspent. The stated residual ("a dependent, locked, the block never completes") was wrong on all three counts: roots too, unlocked by the retry's own SetMinedMulti, and the block completes. The durable mark already exists on the quick path: phase 1 creates records locked and the post-commit pass unlocks them, and a legitimately pre-existing mined record is never locked (mempool locks clear on SetMined, block locks on the post-commit pass). utxo.LeftoversAmong reads the lock for every ErrTxExists transaction BEFORE phase 1.5 clears it; a locked one is this node's own earlier write and is treated as created here, on the spend (flag on, so no bless) and in the compensation (a ghost if rejected, dependents included). Leftovers are left out of phase 1.5, which is safe because AssignBlockID is idempotent per block hash, so they already carry this block's id and keep their lock until the block commits or the cleanup deletes them. The legacy path created unlocked and so had no mark. createUtxos now creates locked under the same setting as the quick path (blockvalidation_quick_validate_skip_utxo_lock, whose description now covers both routes and says what the lock protects), PreValidateTransactions spends with IgnoreLocked like the quick path, and unlockBlockTransactions releases the lock once ProcessBlock has committed the block, chunked like SetMinedMultiChunked and skipping the coinbase. That is one extra write per transaction on the non-unified legacy route, the same cost the quick path pays by default. Two limits, stated on LeftoversAmong: with the lock setting off nothing writes the mark, and conflict resolution locks the parents of a conflicting transaction for a few round trips, so a replay of such a parent's block in exactly that window would misfile it; that fails towards a missing record this node notices itself, not towards a double-spendable output. WithSpenderCreatedByCaller was dropped at the validator's gRPC and HTTP boundary, so a remote validator kept the bless. The request carries it as field 15, the client sets it on both transports and the server reads it. batchSpend.idempotent was never reset between the SQL batch's deadlock retry attempts, so an input classified idempotent in a failed attempt and written fresh in the retry was kept out of the rollback. Reset per attempt. Tests, each proven by breaking its fix: - TestQuickValidateLeftoverOfInterruptedAttemptIsNotBlessedOnRetry, TestQuickValidateLeftoverAfterFailedDeleteIsNotBlessedOnRetry and TestQuickValidateLeftoverDependentIsRemovedOnRetry on the quick path; TestLegacyLeftoverOfFullyPrunedChainIsNotBlessedOnRetry and TestUnlockBlockTransactionsReleasesTheCreatePhaseLock on the legacy path; TestCreateRetryKeyExistsDoesNotResurrectPrunedOutput on Aerospike, with the create batch delivered twice through the batch seam. - TestOptionsFromValidateRequest_SpenderCreatedByCallerRoundTrip and TestHTTPHandlerPath_SpenderCreatedByCaller. - TestSpendRetryRollsBackSpendWrittenOnRetry (SQLite and Postgres), driving a real deadlock retry with a concurrent unspend inside the window. - Adopted from the panel: TestPrunerRetriesSerializationFailureOnDelete_Postgres raises a genuine 40001 from the pruner's own DELETE and asserts the retry fires; TestLegacyBlockRejectsReplayWhenStoreSurfacesBareTxNotFound observes the validator-level gate through a store that returns a bare ErrTxNotFound.
|
Second panel pass, on 3fa6315: five lenses, every finding handed to a reproducer that had to fail a test on the real store and to a skeptic told to refute. Ten reproductions, none refuted, in four groups. All four are fixed in this push and the panel's tests are adopted. A leftover of an interrupted attempt was blessed on the retry (P0). The created-here gate I added last round was derived from The fix uses the mark the quick path already writes: phase 1 creates records locked and the post-commit pass unlocks them, and a legitimately pre-existing mined record is never locked. The bless flag was dropped over gRPC and HTTP (P1). The SQL idempotent mark was never reset between deadlock-retry attempts (P2). An input classified idempotent in a failed attempt and written fresh in the retry stayed out of the rollback. Reset per attempt; the panel's test drives a real retry with a concurrent unspend inside the window, on both engines. Two test-quality items, closed with the panel's tests. The remaining accepted residuals are unchanged: non-retroactive markers, the Spend/Create gap, and a native-op server not reporting idempotent matches. |
| -- leaves BIN_DELETED_CHILDREN alone, so the marker survived but matched | ||
| -- nothing. Matching the spender means the rejection survives any | ||
| -- rollback of the spend it protects. | ||
| if deletedChildren ~= nil and spendingData ~= nil then |
There was a problem hiding this comment.
[Minor] Cross-store divergence: Lua answers three record-level states before the replay marker.
The bulk and per-row SQL paths place the childPruned marker check first, and the invariant comment in sql.go states the marker must precede frozen, conflicting, locked, height-gated, spent-by-another, and hash-mismatch. Lua honors that for per-output frozen (spendableIn, line 403) and already-spent (418), but three record-level answers return before the per-spend loop, and so before this marker check: BIN_CONFLICTING (307), BIN_LOCKED (317), and coinbase BIN_SPENDING_HEIGHT immaturity (327). getUTXOAndSpendingData (364) also runs its hash-mismatch check ahead of the marker.
A pruned-child replay whose parent record is simultaneously in one of these states is answered CONFLICTING/LOCKED/COINBASE_IMMATURE instead of ERR_UTXO_SPENDING_TX_PRUNED, which the block paths ghost classifier (IsPrunedReplayRejection) does not recognize, leaving the created ghost record uncompensated.
Practical reachability looks very low: the block paths spend with WithIgnoreLocked(true) (quick_validate.go:1472), so locked is bypassed; and a buried/pruned child implies a deeply-confirmed, fully-spent parent output, which contradicts both conflicting and coinbase-immaturity at the current height. So this reads as a consistency gap rather than an exploitable defect, and no test covers a conflicting/locked/immature parent replay. Worth either aligning Lua ordering with the SQL invariant, or narrowing the sql.go comment to note Lua record-level exceptions.
There was a problem hiding this comment.
Confirmed addressed in 6931f09: the invariant comment (sql.go:2286-2289) no longer claims cross-store order parity and now spells out that Lua keeps three record-level answers (conflicting, locked, coinbase immaturity) ahead of its marker check, none reachable for a fully-spent buried parent whose child is replayed, and that the block paths spend with IgnoreLocked. Resolving.
|
|
||
| // ctx, not the errgroup context: the spend group's context is cancelled | ||
| // by the time we get here. | ||
| if deleteErr := utxo.DeleteCreated(ctx, u.logger, u.utxoStore, ghosts, |
There was a problem hiding this comment.
[Minor] DeleteCreated here runs under batchCtx, which a sibling write goroutine can cancel.
In the two pipeline callers (line 583 and 716) this function is invoked with batchCtx from errgroup.WithContext(gCtx), and a sibling goroutine (writeSubtreeFilesForBatch / subtree build) runs in the same batchG. The comment at 1396-1397 only reasons about the inner spend group context; but the ctx passed to DeleteCreated is batchCtx, so if the sibling write fails concurrently with the spend hard-fail, batchCtx is cancelled and retryStoreCall aborts (compensate_created.go:286-290), or the first DeleteComplete runs on an already-cancelled ctx, and a ghost can survive.
Mitigation exists, and is why this is Minor: with quick_validate_skip_utxo_lock off (the default), the surviving ghost is still locked, so the next attempt LeftoversAmong reclassifies it as created-here and re-compensates it. The gap is only under skip_utxo_lock, already a documented limitation. Still, decoupling the compensation from the sibling goroutine (e.g. context.WithoutCancel(gCtx) with a bounded timeout) would make ghost cleanup robust regardless of a concurrent write failure, and match the comment stated intent.
There was a problem hiding this comment.
Confirmed addressed in 6931f09: DeleteCreated now runs on a context detached from the batch errgroup (context.WithoutCancel(ctx) + a bounded compensatingDeleteTimeout), so a sibling write goroutine cancelling batchCtx at the moment of the hard-fail can no longer abort the compensating delete and leave the ghost. See quick_validate.go:1402. Resolving.
…and say where Lua's check order differs Two minor review-bot items on 563e622. The compensating delete after a failed spend phase ran under the batch's errgroup context, which the sibling subtree-write goroutine shares. A write failure landing at the same moment as the hard-fail cancelled the delete and left the ghost; with the catch-up lock on the next attempt would reclassify it as its own and clean it, but only then. The delete now runs detached from the batch context on a bounded timeout of its own. The SQL invariant comment claimed the marker precedes every other answer on both stores. Lua keeps three record-level answers ahead of it (conflicting, locked, coinbase immaturity), none of which a fully spent, buried parent can be in while its child is replayed, and the block paths spend with IgnoreLocked; the comment now says that rather than claim parity of order.
|
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
Re-reviewed at 6931f097f. The two commits pushed during the review close the wire drop and the first-attempt-only bless, both confirmed by execution — but one P0 and four P1s are open, and the P0 is a design question rather than a slip.
Everything below was reproduced by execution at 6931f097f against sqlite, a live Postgres 13 container and a live Aerospike 8.0 container running the shipped teranode_v63, unless marked otherwise. Probes used go test -overlay; no tracked file was modified.
P0 — the !item.idempotent rollback exclusion strands a parent output across calls
stores/utxo/sql/sql.go:2009 · stores/utxo/aerospike/spend.go:618
Two variants; 563e622df closes one of them.
- In-call retry: closed. The per-attempt reset in
sendSpendBatchfixes it on SQL — removing only those five lines by overlay failsTestSpendRetryRollsBackSpendWrittenOnRetryon both engines with "P:0 is still spent". The flag's three writers (sql.go:2333,:2602,:2887) are all inside one attempt, everyisDeadlockreturn textually precedes itserrChloop, and it is clean under-race. On Aerospike the variant does not exist: no in-call retry, andmarkIdempotentSpends(spend.go:1012) is fed by the same atomic Lua evaluation that decides whether a write happens. Nothing to reset there — correct as it stands. - Cross-call: open, both engines. On a fresh call the store genuinely records the spend, so the reset clears the flag and the attempt re-derives it as
trueimmediately. Call 1 spends C over P:0 and Q:0 with Q's record absent →ErrTxNotFound, which is not inneedsSpendRollback, so the fresh spend of P:0 stays committed and C is never created. Call 2 classifies P:0 idempotent and fails on frozen Q:0, a rollback-class error;spentSpendsexcludes P:0 and the rollback reverses nothing. P:0 is left permanently spent by a transaction the store does not hold, and the next legitimate spender getsErrSpentnaming a txid the node has never seen. Public API, two plainstore.Spendcalls, no fault injection. Reproduced on sqlite (per-row), Postgres (bulk CTE) and Aerospike (Lua andEnableSpendFilterExpressions=true). A crash or context cancellation between the two calls reaches the same state. The enabling condition is the documented policy of leaving partial spends committed on transient failure (aerospike/spend.go:639-643).
No test in the PR covers the cross-call variant.
This is not a one-line revert. Dropping the guard makes the cross-call probe pass and TestPrunedReplayRollbackPreservesHistoricalSpend fail — verified both directions. A record-level "this output already records exactly this spend" cannot separate a confirmed historical spend from one an earlier failed attempt of the same logical spend committed; they are byte-identical on the record. Closing it needs a signal about the spender rather than the output — record exists / is mined / was written by this pass — which is machinery this PR already builds (spenderCreatedByCaller, utxo.LeftoversAmong).
Knock-on: DeleteCreated's rationale for not reversing a ghost's input spends rests on "the store rolls back whatever fresh sibling spends it made in the same call". This is exactly the case where that premise is false, so the compensating delete does not cover it either.
P1 — ERR_UTXO_SPENDING_TX_PRUNED is unclassified on both client-facing surfaces
errors/errors.go:773-810 (publicCauseCodes) · errors/errors.go:1095-1097 (ErrorCodeToGRPCCode) · services/propagation/Server.go:759-776 (httpStatusForTxError)
ERR 78 is new in this PR and appears in none of the three classifiers. httpStatusForTxError falls to default: 500, ErrorCodeToGRPCCode to codes.Internal, and it is absent from publicCauseCodes so DeepestPublicCause is nil and the response body carries no verdict. A permanent, deterministic rejection is reported to submitters as a server fault — where the same rejection previously answered 200, 403 or 409. Add it to all three; FailedPrecondition / 409 matches the neighbouring terminal codes.
P1 — the idempotent compensation is inoperative on the Aerospike native operate-path
stores/utxo/aerospike/native_op.go:96 · native_op.go:49 · teranode.go:281-284
useNativeForSubOp fences subOpUnspend and nothing else, so with aerospike_use_native_teranode_ops=true spendMulti routes to the C dispatcher and never sees teranode_v63. teranode.go:450 reads idempotent with , ok; absent key leaves res.Idempotent nil, markIdempotentSpends flags nothing, and spend.go:618 sweeps a historical confirmed spend into spentSpends, which :479-480 then unspends. Same txid, so the ownership check matches and the reversal succeeds — a confirmed output is released, which is the P0 the panel found and fixed. Overlay probe of resolveSpendCompletions:
v63 (idempotent present): spentSpends=0
v61/native (absent): spentSpends=1 <-- historical spend would be unspent
probeNativeSpendSemantics does not probe for the field, and demoteNativeOnUnsupported only demotes on PARAMETER_ERROR, so a fork server that accepts sub-op 2 and returns no idempotent is indistinguishable from a correct one. The setting defaults false and the CI job is inert without a fork image, so this is not shipping broken today — but it is unfenced. Either extend useNativeForSubOp to exclude subOpSpendMulti, or add a probe stage that requires the field. The comment at teranode.go:281-284 calling the fallback "as before" should change too: "as before" is the state the panel classified P0.
P1 — the pruner writes replay markers for parents whose outputs the child never spent
stores/utxo/sql/pruner/pruner_service.go:349-357, :448-454 · stores/utxo/aerospike/pruner/pruner_service.go:1134-1146
The marker INSERT joins child → inputs → parent with no join to the parent's output and no exclusion of conflicting candidates, and conflicting losers are pruning candidates on both backends (sql.go:4374-4380; create.go:538-539). Before 3fa6315af this was inert, because the predicate keyed on substr(o.spending_data,1,32) — the winner. Re-keying it on the incoming spender (sql.go:2179, :2733) and de-nesting it above every other check makes the stale marker load-bearing.
Using the real pruner service, two-output parent, mined winner on P:0, conflicting loser that never spent it, on sqlite and Postgres: one (parent, loser) row, and then (A) a rebroadcast of the loser answers 78 instead of ErrSpent (70) naming the winner, and (B) after the winner's spend is released, the loser's genuinely fresh spend of the now-free output answers 78 permanently, until the parent itself is pruned. (B) is the one that matters — a legitimate spend rejected forever on the live above-checkpoint path. Join the marker query to the parent's output, or exclude conflicting candidates.
P1 — IgnoreLocked has no proto representation, and this round made it load-bearing
services/legacy/netsync/handle_block.go:1263 · :1530
createUtxos now creates records locked on the legacy catchup path, but the compensating WithIgnoreLocked(true) cannot reach a remote validator — Options.IgnoreLocked and Options.IgnoreConflicting are still absent from the proto. On the default useLocalValidator=false topology, below-checkpoint legacy blocks with intra-block parent chains should fail their spend phase with TX_LOCKED. Same three-line pattern just applied to spender_created_by_caller.
P2
LeftoversAmong(compensate_created.go:167) classifies onData.Lockedalone — no unmined check, no block-id check, no ownership token — andDeleteCreated(:238-273) is a bareDeleteCompletewith no CAS. A mined record left locked by a failed unlock pass, an abandoned attempt at another block, orProcessConflicting's parent lock is treated as this attempt's and deleted once its parent chain is pruned. Proven; survives with the mechanism neutered.- With
blockvalidation_quick_validate_skip_utxo_lock=truethe leftover mechanism is entirely off and the consumed output becomes spendable again. The longdesc says the replay "can be blessed" — it should say that. spend()single-output UDF emitsidempotent:[nil], response unparseable (teranode.lua:262-276,teranode.go:461). Proven live.- Aerospike Lua still answers six checks ahead of the marker (
teranode.lua:286-342) where SQL was reordered — the rationale written insql.godoes not hold on the shipped backend. The comment added in563e622dfargues the reachable states coincide; that argument is probably right, but it is doing real work and belongs next to the Lua, not only insql.go. - Postgres
upd_idemCTE arm marks success without marking idempotent (sql.go:2511). - Marker rejection past the 10-error
JoinCappedcap is never classified (aerospike/spend.go:84,:534).
Static-read only, not a blocking claim: aerospike-client-go/v8 defaults MaxRetries=2 and batch UDF applies are write commands marked in-doubt on retry (policy.go:189, batch_command_operate.go:83). An apply that committed server-side with a lost ack gets re-sent, the re-apply reports it in FIELD_IDEMPOTENT, and this call's durable write is excluded from the rollback — with no transaction to undo the first apply. I could not force a lost ack against a container, so this one is worth your read rather than my assertion.
Prior findings
| Finding | Status at 6931f097f |
|---|---|
| pass 2 P1 — unbounded master marker | FIXED — master write removed entirely, not gated; reversion-pinned |
wire drop — WithSpenderCreatedByCaller |
FIXED — field 15, no collision, both HTTP shapes, generated code reproducible from the proto, tests mutation-proven |
| first-attempt-only bless of a fully pruned chain | FIXED in the fail-open direction, reversion-pinned via LeftoversAmong; the over-delete mirror is the P2 above |
| icellan P0-3, P0-4, P0-5, P0-6 | closed |
| icellan P1-1, P1-2, P1-3, P1-4, P1-5, P1-6, P1-8, P1-9 | closed |
| icellan P0-1 | closed by compensation rather than a marker check in Create |
| icellan P0-2 | openly not claimed; the locking-read analysis holds — it would order the commits but not close the window |
| icellan P1-7 | benchmark still outstanding |
Twelve of icellan's fifteen are genuinely closed and reversion-pinned, as is pass 2's P1. The blocker is the P0; the four P1s behind it.
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
Round 2, re-verified against HEAD cbd808d5d rather than against the fix claims. 10 of the 15 round-1 findings are genuinely fixed, and several of the fixes are better than what I asked for — re-keying the marker check onto the incoming spender instead of the output's stored spending_data closes the Unspend hole cleanly, the expression fast path got the same guard, LuaPackage was bumped v61→v62 so the changed Lua actually loads, ErrUtxoSpendingTxPruned (code 78) is threaded into both rollback predicates, the master-record marker write is gone, per-record isolation is real, util/cuckoo / PrunedTxSet / extractInputs are deleted, and the weak ErrorIs(ErrUtxoError) assertions are now specific. Build, vet, and the PR's own suites all pass locally.
Pruning still is not safe from reinsertion, though, and round 2 opened two new routes in.
The structural objection first: this PR chose compensation over prevention. Create still never consults the marker; instead the new stores/utxo/compensate_created.go deletes the ghost after the spend phase has already rejected. That is best-effort cleanup with no durability — a crash, a cancelled context, or a single failed delete leaves the record behind, and for descendants it is single-shot by construction (see below). Consulting the marker in Create would make the whole class impossible instead of recoverable.
P0
Aerospike native-op spend never consults the replay marker — stores/utxo/aerospike/spend.go:832, stores/utxo/aerospike/native_op.go:96
The marker is enforced in exactly two places: teranode.lua:394 and the Go filter expression at spend_expressions.go:236-250. Neither runs on a native-ops cluster. createBatchRecords builds every spend as teranodeBatchRecord(..., subOpSpendMulti, "spendMulti", ...) at spend.go:832, and teranodeBatchRecord (native_op.go:203) routes to TeranodeModifyOp whenever useNativeForSubOp(subOp) is true — which is s.useNativeTeranodeOps.Load() && subOp != subOpUnspend. Enforcement then lives in out-of-repo C that this PR does not touch and cannot version-gate: you bumped LuaPackage v61→v62 in teranode.go:77 precisely because a Lua semantics change needs a gate, and there is no equivalent for the native path.
probeNativeSpendSemantics (native_op.go:470-593) only proves first-seen: spend with 0x01, re-spend with 0x02, accept on LuaErrorCodeSpent. It never puts a deletedChildren entry on the probe record, so a server that ignores the bin entirely passes.
The expression guard is self-defeating here. spend_expressions.go:236 FILTERED_OUTs a marker hit and processSpendBatchResultsExpressions re-issues it through executeLuaSpendBatch (:597) → createBatchRecords → subOpSpendMulti → native. So on a native-ops cluster the new guard converts a marker hit into a native re-issue that accepts the spend.
useNativeForSubOp already fences subOpUnspend for exactly this reason — native_op.go:80-95 says the probe "does not exercise" its ownership check and running it native "would be a silent spend-reversal / UTXO-resurrection primitive". That argument now applies verbatim to spendMulti and it is not fenced. aerospike_use_native_teranode_ops defaults false, but the scaling cluster runs it true.
Fix: fence subOpSpendMulti to the UDF path while the marker is load-bearing, or extend the probe to plant a deletedChildren entry and require rejection.
P1
Descendant ghosts are single-shot, so one failed delete or one crash makes them permanent — stores/utxo/compensate_created.go:83
PrunedReplayGhosts adds a rejected transaction unconditionally (:59-66) specifically so a leftover from an earlier failed compensation is re-deleted, and the comment at :36-38 says excluding "already existed" is "what made a transient delete failure permanent". The dependent walk does not carry that reasoning: if !spendsAny(tx, ghosts) || !createdHere(txHash) { continue }, and both callers wire createdHere to "was not ErrTxExists in this attempt's create phase" (quick_validate.go:1356-1360, handle_block.go:892-896).
Block B replays pruned G plus its in-block child D. Attempt 1 creates both, the spend rejects G, DeleteComplete(G) succeeds, DeleteComplete(D) fails all three in-band attempts (:113) — or the pod is killed mid-compensation, or the context is cancelled (:149 returns ContextCanceledError on the first retry). Attempt 2: create returns ErrTxExists for D, so createdHere(D) is false and ghosts is {G} only. G is deleted, D is not, and D never appears in rejected either because its spend of G's freshly recreated output succeeds. D is left stored mined with no delete_at_height — neither the unmined reaper nor the DAH scan reclaims it — and on the netsync path createUtxos writes without WithLocked, so it is unlocked and spendable. A plain crash between createG.Wait() (quick_validate.go:1313) and the delete produces the same permanent D. compensate_created_test.go:88-94 currently pins this as intended.
The same hole opens with no delete failure at all: createUtxos discards its partial results on any create error (handle_block.go:1247), so records written before the failing goroutine are "pre-existing" on the retry and can never be walked.
SQL checks already-spent before the marker, so a pruned replay comes back as ErrSpent and the validator recreates it as conflicting — stores/utxo/sql/sql.go:2272 (bulk), :2795 (per-row)
Both SQL paths run the ErrSpent branch and continue before r.childPruned at :2287 / :2808 is ever consulted. Lua orders it the other way — teranode.lua:394 evaluates deletedChildren before the existingSpendingData block at :412 — so the two backends return different errors for the same state, and only the Aerospike ordering feeds the compensation.
That matters because ErrSpent is the trigger for the conflicting-create fallback: Validator.go:1132 sets saveAsConflicting on errors.Is(err, ErrSpent), legacy netsync sets WithCreateConflicting(true) by default (handle_block.go:1444-1447), the fallback calls CreateInUtxoStore (Validator.go:1157) and returns ErrTxConflicting, which netsync swallows outright (handle_block.go:1468). Concretely on Postgres/SQLite: child C is pruned and markered, a reorg unspends P.vout0, a different transaction X spends it, block B containing C is replayed. The spend of P.vout0 sees spendingDataBytes = X ≠ C, returns NewUtxoSpentError at :2279, and never reaches the marker branch. The validator creates C and returns ErrTxConflicting, appendPrunedReplay (handle_block.go:1519) never matches, no DeleteCreated runs, netsync returns nil, the block is accepted, and C is permanently back.
Fix: hoist the childPruned check above the ErrSpent branch on both SQL paths so it matches Lua.
Adding ErrUtxoSpendingTxPruned to isSpendRollbackError can unspend a confirmed output — stores/utxo/aerospike/spend.go:615
Markers are per-parent-record, not per-transaction, so a transaction can hold a marker on some parents and not others. Child C spends A:0 and B:0. executeBatchParentUpdates succeeds for A's page and fails for B's — RECORD_TOO_BIG, DEVICE_OVERLOAD, or the malformed-bin case your own test injects at pruner_replay_test.go:131. tallyParentUpdateResults holds C back so it is not deleted (pruner_service.go:1656-1663), but A keeps its marker, and C is still live and mined. C is then re-spent (duplicate propagation, catchup re-validation, fork validation): A:0 returns INVALID_SPEND → NewUtxoSpendingTxPrunedError (spend.go:1090), while B:0 hits the idempotent branch at teranode.lua:415-417, returns OK, and is appended to result.spentSpends (spend.go:599). rollbackNeeded is now true, so Unspend(ctx, result.spentSpends) at spend.go:505-506 clears B:0 — the ownership check at teranode.lua:534 passes because the recorded spender is C. B:0 is now unspent while C exists and is mined, so any other transaction can spend it. That is a double-spend of a confirmed output.
Before this PR the same replay returned a plain NewUtxoError, which is not in isSpendRollbackError, so nothing was unspent — unnesting the Lua check and classifying it as a rollback error combine to produce this. pruner_replay_test.go uses a single-input child throughout, so no sibling spend ever exists to be rolled back.
The reinsertion window is the entire prune transaction — stores/utxo/sql/sql.go:2133
The marker INSERT and DELETE are now genuinely atomic with each other (sql/pruner/pruner_service.go:348-357, SERIALIZABLE at :309), and the comment at :230-233 is right that they must commit together. But that is a property of the writer only: both spend paths open with s.db.BeginTx(s.ctx, nil) (sql.go:2133 bulk, :2685 per-row), i.e. READ COMMITTED on Postgres, and SSI only serialises SERIALIZABLE transactions against each other. The reader never sees the uncommitted marker.
The delete at pruner_service.go:337 is an unbounded full-tombstone-set DELETE, so this window is seconds to minutes, not microseconds. Inside it: the spend of C evaluates EXISTS(deleted_children) as false, falls into the idempotent re-spend branch at :2294-2299 which performs no update at all, so there is no lock conflict and no serialization failure on either side; it commits success. The pruner then commits, and Create inserts C. No rejection is raised anywhere, so the compensation machinery never fires either. Closing this needs the marker enforced by something the pruner conflicts with — re-check deleted_children inside the spend's own UPDATE predicate, take a row lock on the parent, or run the spend batch at SERIALIZABLE. Raising the pruner's isolation cannot help; it is already at the top.
The new prune retry never fires on the Postgres case it was written for — stores/utxo/sql/pruner/pruner_service.go:162-190
maxPruneAttempts (:151), isPruneRetryable (:162-190) and deleteTombstoned re-running the whole transaction (:197-228) address the shape of the round-1 finding, but every failure site wraps the driver error first: :348-350 errors.NewStorageError("failed to mark pruned children", err), :352-355, :457-464. errors.New (errors/errors.go:363-370) handles a trailing non-*Error param with case error: wErr = &Error{message: err.Error()} — it keeps the string and discards the concrete type, so *pgconn.PgError is no longer in the Unwrap chain and all three errors.As branches at :167-185 are dead. Checked empirically in-tree: isPruneRetryable(&pgconn.PgError{Code:"40001"}) is true, isPruneRetryable(errors.NewStorageError("failed to mark pruned children", thatSameErr)) is false. The only surviving discriminator is the substring check at :187-189, and the 40001 text ("could not serialize access due to read/write dependencies among transactions (SQLSTATE 40001)") matches neither "database is locked" nor "database table is locked". The SQLite half works by accident, because modernc's message happens to read "database is locked (…)".
Not a reinsertion hole — the rollback is atomic and the same candidates are re-picked at the next height — but under sustained conflict the pruner loses every cycle while the code believes the retry protects it. There are no tests for isPruneRetryable or maxPruneAttempts. Either classify before wrapping, or attach the driver error so errors.As can still reach it.
Still open from round 1
Not retroactive — stores/utxo/sql/pruner/pruner_service.go:235-273. The response is a comment conceding both halves, which is honest, but there is still no migration, no backfill, no startup detection of "table just created", no log, no metric, and no docs change (git diff --name-only <merge-base>...HEAD -- docs/ '*.md' is empty for this cluster). An operator upgrading a node that was already pruning gets no signal that their store carries a permanently unprotected backlog. The unprunable-parent leak at :385-389 / :412-416 is byte-identical after the SQL-literal refactor, and the schema_upgrade subtest (pruner_replay_test.go:85-95) still runs before any Prune, so it exercises CREATE TABLE IF NOT EXISTS rather than the pre-upgrade state.
Worth flagging separately: the backfill recipe the comment recommends at :264-269 is unsafe as written. It marks every output whose spending_data names a txid with no row in transactions, treating "child row absent" as "child was pruned". Pruning is not the only path that produces that state — Validator.go:2401 (unwindShed) calls DeleteComplete first and unspends afterwards, and three arms deliberately stop in between (:2416-2419, :2434-2437, :2495). So: a node sheds tx X under backpressure, DeleteComplete succeeds, Unspend fails, the pod restarts for the upgrade, the operator runs the documented backfill, and (P, X) is inserted. X is a valid unmined transaction the sender will resubmit; every future spend of P:0 by X now returns ErrUtxoSpendingTxPruned forever, and P:0 is unspendable by anyone else because its spending_data still names X. Either restrict the recipe or warn explicitly that it must not run on a store that has ever taken a shed-unwind failure.
Aerospike is partly retroactive where SQL is not, since the pre-PR pruner already wrote the deletedChildren bin in both modes. Its gap is narrower: the removed cuckoo prunedSet skipped the parent update on a false positive, previously documented as having "no behavioural consequence" — a claim this PR invalidates, because the bin is now consulted on the spend path. Also unbackfilled and undocumented.
Smaller
- The marker check silently no-ops when
spend.SpendingDatais nil:sql.go:2169-2172and:2748-2751leavespenderas SQL NULL and= NULLis never true, andspend_expressions.go:235skips the clause. Aerospike rejects nil earlier (spend.go:801-805) and no current caller reaches SQL Spend with nil, but the guard is silent rather than fail-closed. - Five commits in the PR range still carry
Co-Authored-By: Claude Opus 5andClaude-Session:trailers:5ed30d5c1(the round-2 upstream merge),3011099f6,abd997e26,597ca1cba,6304b5a84. The five newest non-merge commits are clean, so this was fixed going forward but the history was not rewritten. Rebase or squash-merge with a hand-written message before this lands.
Pruning a confirmed child could remove its UTXO record without preserving replay protection on its surviving parent. Replaying the same spender could then recreate the child as unmined with unspent outputs. This change preserves parent markers before removing children and rejects replay after pruning across Aerospike, SQLite and PostgreSQL.
Fixes #1701.
PrunedTxSet,util/cuckooand the now-inertpruner_utxoPrunedSetMaxEntriessetting are deleted), write the marker to the output-page record the spend path reads and to that record only, and complete all parent updates before child deletes or TTL touches. There is no copy on the parent's master record in either defensive mode: nothing reads one when it differs from the page key, and it grew by ~70 bytes per pruned child with no eviction until the record hitRECORD_TOO_BIG(reproduced by review at ~15k children on a 16k-output parent). A parent-marker write the server rejects holds back only the children of that parent instead of failing the cycle. Retain records when external input references are unavailable. Preserve native-op routing and COMMIT_ALL for parent updates.utxostore_utxoBatchSize = 1with spend filter expressions enabled): the filter that gates a spend write now also requires thatdeletedChildrendoes not name the incoming spender, keyed exactly asaddDeletedChildrenwrites it. Before, the filter only checked that the element was the bare unspent hash, which is true again after anUnspend, so a replay was written with no Lua evaluation at all. A hit isFILTERED_OUTand re-issued through Lua, which returns the classified rejection.deleted_childrentable automatically during schema initialization, and commit markers and deletion in one transaction. Both bulk and per-row spend paths reject pruned-child replay with a dedicated error code,ERR_UTXO_SPENDING_TX_PRUNED. The marker is checked before the conflicting-spender answer, so a replay whose input has since been taken by a replacement transaction is still identifiable as a replay (Lua already orders it that way). The code joinsneedsSpendRollback/isSpendRollbackError, so a rejected replay does not leave its sibling spends recorded against a transaction that will never exist. Markers cascade away with their parent and are respected by defensive pruning.Createdeliberately does not read the marker: that would put a parent-record read on the hottest write path in the node. Instead, when the spend phase hard-fails, the ghosts the create phase left are deleted.utxo.PrunedReplayGhostsdecides what a ghost is: every transaction the store rejected as a pruned replay, whether or not this attempt created it (a markered transaction cannot legitimately come back, so any record for it is a replay's leftover, including the leftover of an earlier compensating delete that failed), plus, transitively, every transaction in the block that spends one of them and that this attempt created (its own markers went with its pruned parent, and it spent the freshly recreated copy, so it was never rejected). A dependent the store already held is left alone, and unrelated siblings are never touched.utxo.DeleteCreateddeletes those records and deliberately reverses none of their spends: a replayed transaction carries the same identity as its original, so a parent output cannot say whether the spend it records is the confirmed one or a fresh one, and every spend a ghost holds on a surviving output is in fact the historical one (a rejected root's fresh sibling spends are rolled back by the store itself; a dependent that spends a surviving output either hit that output's marker and is a root, or the output has recorded its spend since a pre-marker prune). Deleting the record and leaving the spends is the state a normal prune leaves. It retries a failed delete, because for a leftover dependent the next attempt cannot tell it from a legitimately pre-existing record; that residual is stated onPrunedReplayGhosts.WithSpenderCreatedByCaller); for those the fallback is off (the validator's own copy of it is gated too, though with these stores it is shadowed by the aggregate error they return), the missing parent surfaces, and a created-here transaction whose spend fails on a missing parent is a ghost like a marker hit (utxo.IsPrunedReplayRejection). A pre-existing child with a pruned parent is still blessed, which is what the fallback was for.idempotentlist in the spend response), and the rollback leaves it alone. Before, a replay rejected on one parent's marker had its confirmed spend on an un-markered sibling parent cleared, which handed that confirmed output to any new transaction; review reproduced it on all three stores, including through this PR's own hold-back path (one parent's marker landed, the sibling's failed, the child was retained).LuaPackageisteranode_v63.errors.NewStorageError, which replaces a foreign error with a bare teranode error carrying only its message, so the predicate never saw the code. The attempt now reports statement failures through a small wrapper that keeps the driver error reachable, and the final error is wrapped for the caller after the retry decision.utxo.LeftoversAmongreads the lock for everyErrTxExiststransaction before phase 1.5 clears it, and a locked one is treated as this node's own write: the bless is off for it and it is a ghost if rejected. Without this, a record left by a crash, a cancellation, a delete that failed past its retries, a spend phase that saw only retryable errors, or an Aerospike create batch the client re-sent (answeredKEY_EXISTS) was filed as pre-existing on the retry, and a replay of an end-to-end pruned chain was then blessed on the strength of it. The legacy netsync path created unlocked and so had no mark; it now creates locked under the same setting as the quick path (blockvalidation_quick_validate_skip_utxo_lock, which governs both routes), spends withIgnoreLocked, and unlocks onceProcessBlockhas committed, chunked. That is one extra write per transaction on the non-unified legacy route, the cost the quick path already pays by default.WithSpenderCreatedByCallertravels over the validator's gRPC and HTTP transports (request field 15), so a remote validator gets the same answer as the in-process one.Store.DeleteComplete, which MvP-4694 Queue-age-driven Kafka ingest backpressure for the validator #1502 added upstream: master record, pagination records and external blob. This branch had extendedStore.Deleteto do the same job; that extension was dropped when upstream was merged in.spending_data, so anUnspendthat clearsspending_datacannot disarm it.Unspendcontinues to leavedeleted_childrenandBIN_DELETED_CHILDRENalone.LuaPackageis bumped toteranode_v62becauseregisterLuaIfNecessaryonly registers a script when its filename is absent.Limitation: the protection is not retroactive
A marker only exists for a child this code pruned. On a node that was already pruning before this lands, every child the old code removed left no marker, and the child row is gone, so nothing can reconstruct one. Two consequences, both permanent for that backlog:
deleted_childrenescape clause fires only for markered children, so a parent whose child was pruned by the old code keeps failing the unstable-child test and can never be pruned. This is not a regression: the old code had no escape clause at all, so such a parent was already unprunable. It is also not the deployed configuration, sincesettings.confsetspruner_utxoDefensiveEnabled = false.Neither can be backfilled from inside the pruner. "Child row absent" is not a safe substitute for a marker, because
SequentialSpendAndCreatespends before it creates: a legitimately in-flight child is also absent while its parent's output already names it, and inferring "pruned" there would let the pruner delete a parent out from under a child that is about to exist. An operator who wants the backlog cleared can run a one-shot backfill with the node stopped, when no create can be in flight; the SQL is in the comment ondeleteTombstonedTx. It is a full scan ofoutputsand is deliberately not run automatically.Validation:
Unspendhad clearedspending_data.Unspendagainst Aerospike (batch size 1); bothexpressions_*_unspent_parentsubtests fail with the filter clause removed.createUtxosthenValidateTransactionsLegacyMode, and on Aerospike through a create batch delivered twice; all now assert the record is gone and the consumed output refused.Residual: two limits of the lock as the leftover mark
With
blockvalidation_quick_validate_skip_utxo_lockon, nothing writes the mark, and a record left by an interrupted attempt at a block that replays a pruned chain is filed as pre-existing on the retry and can be blessed with its outputs unspent; the setting's description says so. And conflict resolution locks the parents of a conflicting transaction for the few store round trips ofProcessConflicting, so a replay of such a parent's block in exactly that window would misfile it as a leftover and, if its own parent is pruned too, delete it; that fails towards a missing record this node notices when it next validates a spend of it, not towards a double-spendable output. Both onLeftoversAmong.This prevents future unsafe pruning; it does not repair already-corrupted records, and it does not close the window where the pruner commits between a spend and its create (
SpendandCreateare separate transactions and the spender does not conflict with the pruner's SERIALIZABLE transaction). The additional parent writes and SQL transaction work have not been production-throughput benchmarked. Aerospike integration tests use CE with Lua; native-op routing and response contracts are covered by unit tests, and the server-fork nativeaddDeletedChildrenand spend implementations are outside this repo, so neither the Lua changes (marker check, its precedence, theidempotentlist) nor the expression-filter change here reaches them; a native spend that does not report idempotent matches gets the previous rollback behaviour.