fix(utxo): tolerate a dangling spender reference under a recent parent (#1214) - #1681
fix(utxo): tolerate a dangling spender reference under a recent parent (#1214)#1681ordishs wants to merge 2 commits into
Conversation
bsv-blockchain#1214) SequentialSpendAndCreate records a transaction's spends on its parents before it creates the transaction's own record. A crash in that window, or a rollback that exhausts its retries, leaves a parent output slot naming a spender the store has no record of. GetCounterConflictingTxHashes then walks that absent spender, GetConflictingChildren fails on its root read, and the error propagates out of checkCounterConflictingOnCurrentChain. Block validation wedges on a block that SVNode-following peers accept. Gate the tolerance on the parent's confirmation depth. An absent record has no BlockIDs, so it is not mined on our chain. When every parent output slot naming the spender is confirmed within the UTXO store retention window, no counter mined on those slots could have been pruned yet, so the absence must be a never-created loser: drop the spender from the counter set and continue. Below that window a mined-then-pruned counter cannot be ruled out, so fail closed — SVNode would reject a block double-spending a confirmed output. A parent whose depth is unknown, and retention 0, both fail closed. The guard is scoped to ErrTxNotFound/ErrNotFound; every other walk error still propagates. The tip height is read lazily, only when an absent record is hit, so stores that carry no dangling reference are never asked for it. Tolerated absences increment teranode_utxo_dangling_spender_ref_tolerated_total, so the inconsistency stays visible rather than silent.
|
🤖 Claude Code Review Status: Complete The guard is carefully reasoned and well-tested (real-store e2e for both tolerate/fail-closed, plus prune-horizon boundary tests on the real pruner). One correctness concern worth verifying, plus existing open threads. Current Review:
Everything else — the retention arithmetic, the ANDed per-slot tolerance, lazy tip read, metric visibility, and the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| parentTxMeta, err := s.Get(ctx, parentTxHash, fields.Utxos) | ||
| // fields.BlockIDs is requested alongside fields.BlockHeights because the SQL | ||
| // backend only populates BlockHeights when the block-id join is loaded; the | ||
| // parent-depth guard below relies on BlockHeights being present for a mined parent. |
There was a problem hiding this comment.
[Minor] The stated rationale here is slightly imprecise. needsBlockIDsQuery (stores/utxo/sql/sql.go:1784) already returns true when fields.BlockHeights alone is requested, so the SQL backend loads the block_ids join and populates BlockHeights without fields.BlockIDs being explicitly requested. Aerospike likewise populates BlockHeights directly from its own bin (get.go:977). Requesting fields.BlockIDs here is therefore redundant — harmless and arguably good defensive practice, but the comment overstates it as required. Consider rewording to reflect it is belt-and-suspenders rather than necessary.
|
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-09-04 17:45 UTC |
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
The wedge is real and the shape of the guard (depth-gated, ANDed across slots, scoped to not-found) is defensible. But the safety proof — "an absent record has no BlockIDs, so it is definitionally not mined on our chain" — holds only for a miss on the walk root, and the code applies it to a not-found from anywhere in GetConflictingChildren's BFS cone. That turns a liveness fix into a consensus fail-open. Three further paths reach the same fail-open, the tolerance is also armed on the ProcessConflicting demotion path where it converts a clean abort into a half-committed mutation, and both end-to-end tests pass an empty blockIds map, so the branch that actually decides accept/reject is unreachable in them — every finding below passes the new tests unchanged.
Blocking
-
stores/utxo/process_conflicting.go:1297— root and descendant not-found are indistinguishable.GetConflictingChildrenreads the root and every descendant through the sames.Get(...)(:1126) and returns the raw error. Parent P mined at 900, tip 1000, retention 288; P:0 names counter X whose record exists withBlockIDs=[7]; X names a grandchild Y that is absent. The walk 404s on Y, the guard checks only P's depth andcontinues, X never enterscounterConflictingMap, socheckCounterConflictingOnCurrentChain(SubtreeValidation.go:527-534) never matchesblockIds[7]and the node accepts a block double-spending an output already spent by a confirmed tx. Reproduced independently twice. Only tolerate when the root read is the not-found. -
process_conflicting.go:1407—withinRetentionhas no zero/lagging-tip guard, and a low tip is the permissive direction:minHeight+retention > 0is true for every mined parent. Reproduced with the PR's own buried case (parent at height 10,GetBlockHeight()==0) — tolerated,err == nil. The tip is published only bystores/utxo/factory/utxo.go:139-157, which logs-and-skips on error or 0, never starts withstartBlockchain=false, and refreshes only on block notifications, so it lags during catchup — and lagging low widens the window.set_mined_expressions.go:301-305already stopped trusting this cached value for DAH stamping for the same reason. Minimum:if tipHeight == 0 { return false }. -
process_conflicting.go:1368—newParentDepthInforeturnsunmined:trueonUnminedSince != 0before looking atBlockHeights, and unmined tolerates unconditionally at any depth. A record legitimately carries both: AerospikeMarkTransactionsOnLongestChain(false)writes only the unminedSince bin (longest_chain.go:62) and never clears blockHeights; SQL does the same (sql.go:4590-4598). After any reorg or fork promotion a parent hasBlockHeights=[100]andUnminedSince=95at tip 1000, and the guard declares it top-of-chain and tolerates a counter mined on that buried slot and pruned 600 blocks ago. Prefer the mined height wheneverlen(BlockHeights) > 0. -
process_conflicting.go:1325— tolerance is also armed on the demotion path (aerospike/conflicting.go:28,sql/sql.go:4260), where dropping the loser does not avoid the wedge, it relocates it after two committed mutation steps.MarkConflictingRecursivelyderives affected spends from W's own inputs;Unspendmatches onAND spending_data = $3(sql.go:3014-3028) so the slot holding X is a no-op;SpendAndCreate(W, WithSpendOnly, ...)then hitssql.go:2255-2264"already spent by a different transaction" and returnsUtxoSpentError— there is no ignore-spent flag (Interface.go:187-194). Pre-PR that input aborted cleanly with zero mutation; now it fires the compensating rollback and, on rollback failure, "MANUAL INTERVENTION REQUIRED" (:213) — on the path whose own comment says a failure there wedges block assembly on the block forever. No test coversProcessConflictingwith the new retention argument.
Should fix before merge
-
process_conflicting.go:1328— the toleratecontinueskips the frozen-sentinel scan over the spender's cone, and since #1393 this loop is the only place counter-spender cones are frozen-checked (SubtreeValidation.go:513-518states that invariant explicitly). Combined with the first finding the cone is partially walkable and can hold an alert-system frozen sentinel, so a block spending a frozen UTXO is accepted. -
process_conflicting.go:1297—errors.Ison teranode errors matches a code anywhere in the wrap chain (errors/errors.go:180-200), so aStorageErrorwrapping a blobNotFound— exactly whataerospike/get.go:1938-1941returns when a large tx's external blob is unreadable or pruned — is classified as a never-created loser and tolerated. That is a data-availability fault swallowed as absence.DoesNotTolerateNonNotFoundWalkErroruses a bareProcessingErrorthat wraps nothing, so it does not cover this. -
process_conflicting.go:1392— the premise "a mined spender's DAH ismined_height + retention" is false for conflicting-marked records, which is the normal state for cone members afterMarkConflictingRecursively.teranode.lua:990-1001sets DAH once for conflicting records and never raises it,buildDeleteAtHeightExpressionmirrors that guard (set_mined_expressions.go:184-187), and SQLSetConflictingstamps from the cached tip (sql.go:4280). A record marked while the cache reads 500 against a real tip of 1200 gets DAH 789 and is prune-eligible immediately — a record mined on the current chain and pruned inside the parent's window, which is what the guard claims cannot exist. This is the residual risk the description flags; the audit fails. -
process_conflicting.go:1297—GetConflictingChildrenfans a BFS level overconflictingWalkFanOutconcurrent Gets and errgroup keeps only the first error, so whether the walk surfaces as NotFound (tolerated) or a storage error (propagated) is a scheduling race when a level mixes a transient Aerospike failure with a genuine dangling ref. Same store state, different accept/reject across runs, and the racy outcome is the fail-open one. -
services/subtreevalidation/counter_conflicting_dangling_test.go:103— both end-to-end tests passmap[uint32]bool{}, so the branch that returnsErrTxInvalid(SubtreeValidation.go:530-536) can never fire: the tolerate test asserts only that the walk did not error, the fail-closed test only that it did. Missing cases: a counter whose record is present and mined in a block insideblockIds(must reject); a not-found originating from a descendant rather than the spender (must reject);ProcessConflictingover a dangling slot. Alsoutil/test/helpers.go:28setsGlobalBlockHeightRetention = 10, so these tests run a 10-block window while the chosen heights and comments read as though 288 applied, and the retention actually in force is never asserted. -
process_conflicting.go:1224— addingfields.BlockHeightsto the parent Get introduces a new hard failure on Aerospike:processBlockHeightsreturns aStorageErrorwhen the bin is absent (get.go:1195-1199), whereprocessBlockIDsreturns empty and nil for the same condition (get.go:1149-1153). A parent record from an older node version or a snapshot restore previously read fine underfields.Utxosalone and now failsERR_STORAGE, which is not in the tolerated class, so the block wedges. A change made to enable a fail-open guard adds a new fail-closed wedge on the production backend, and both new test files are sqlitememory/mock only. -
stores/utxo/sql/sql.go:4260— unguardeds.settings.GetUtxoStoreBlockHeightRetention().sql.CreateMockStore(sql/mock.go:155-161) builds&Store{db, logger}with nil settings, andsql.go:3337-3339already guards the identical call. Unreached today, so CI stays green over a live nil-deref path.
Also worth a look, non-blocking: retention == 0 is documented as the off switch but cannot be set in production without disabling UTXO pruning cluster-wide, and is semantically inverted (in that deployment every absent record really is a never-created dangling ref, yet the guard rejects all of them); the comment at :1221 justifying the extra fields.BlockIDs is wrong — needsBlockIDsQuery (sql/sql.go:1784-1788) already returns true for BlockHeights alone, and on Aerospike addAbstractedBins (get.go:703-711) drags SubtreeIdxs off the wire on every parent read; spenderParents (:1269) appends one hash per input with no dedup, so a large fan-in tx builds megabytes of duplicates and repeats the same map lookup, paid on every call including the common no-absence case.
freemans13
left a comment
freemans13
left a comment
There was a problem hiding this comment.
Not ready to merge. I worked this independently of the review already sitting on it, so I will skip everything icellan raised and cover two things instead: what I could actually reproduce of the central objection, and one consequence I have not seen raised anywhere.
The root-versus-descendant confusion is real, and here is a repro
icellan's first blocking point is that GetConflictingChildren reads the spender and every one of its descendants through the same s.Get call and hands back the raw error, so the guard cannot tell "this spender was never created" from "something three levels down is missing". I did not take that on trust. I wrote a throwaway test against MockUtxostore on this commit:
- parent P mined at height 900, node tip 1000, retention 288, so the guard is armed
- P's output 0 names spender X
- X's own record reads fine and carries
BlockIDs = [7] - X names a child Y, and Y is absent
Result:
err = <nil>
result = [27ad8841f3b101a25986fef816a5c72425a5a16c0bff8cc1f0d687e7db8b37a3] // the tx itself, nothing else
spenderX (mined in block 7) present in counter set? false
X is a transaction that exists and is mined in block 7, and the walk dropped it. checkCounterConflictingOnCurrentChain never fetches its meta, so the blockIds[7] test at SubtreeValidation.go:530 never runs, and the block is accepted. That is a double-spend getting through, not a theoretical hole. Confirmed.
The guard has a shelf life, and nothing repairs the reference
This is the part I have not seen raised. Tolerance is computed against the node's current tip, so one unchanged store state gives different answers as the chain grows. Parent mined at 900, retention 288, measured on this commit:
tip=1000 -> ACCEPT (tolerated)
tip=1187 -> ACCEPT (tolerated)
tip=1188 -> REJECT
tip=1200 -> REJECT
Nothing here removes the dangling reference from the parent's output slot. The PR counts it in dangling_spender_ref_tolerated_total and moves on. So the reference is still sitting there at height 1188, and every later conflicting transaction touching that parent hits the same absent record, now outside the window, and block validation stops dead again. The fix buys retention blocks. At 288 on mainnet that is roughly two days.
Past that point the failure does not even reach the guard. Once a parent's outputs are all spent it gets a delete-at-height of mined_height + retention, which is the same boundary withinRetention uses, so the pruner deletes the parent at the moment tolerance expires. The next walk then fails at the parent read, and that read has no guard on it:
parent itself pruned -> err = TX_NOT_FOUND (30): parent pruned (guard never consulted)
That path is unchanged from before this PR, so it is not a regression. It does mean the fail-closed branch the whole design is built around is not where the recurrence lands.
I would rather see the spend-first window closed at source (#1355), or a repair that clears the orphaned slot, than a time-limited tolerance that hides the inconsistency until it runs out.
What I did not check
I ran only the two packages this PR touches, ./stores/utxo/ and ./services/subtreevalidation/, both green on 8bd866a, plus the two throwaway probes above, which I have deleted. No Aerospike tests and no long tests, so icellan's Aerospike-specific claims about the demotion path are unverified by me. I did read processBlockHeights at stores/utxo/aerospike/get.go:1195 and it does return a StorageError where processBlockIDs returns empty and nil for the same missing bin, so that asymmetry is at least real.
I have not run any of this on a node.
Posting as a comment rather than a second changes-requested, since icellan's block already stands on this commit.
|
|
||
| // Equivalent to minHeight > tipHeight - retention, written as addition to | ||
| // avoid unsigned underflow when tipHeight < retention. | ||
| return d.minHeight+retention > tipHeight |
There was a problem hiding this comment.
This is where the shelf life comes from. tipHeight is the validating node's current tip, so the accept-or-reject answer for one unchanged store state flips as the chain grows. Measured on this commit with the parent mined at 900 and retention 288: tolerated at tip 1000 and 1187, rejected at 1188 and 1200.
Because nothing clears the dangling reference from the parent's output slot, a parent that validates fine today simply stops validating once the tip passes 1188, and stays that way.
| // fields.BlockIDs is requested alongside fields.BlockHeights because the SQL | ||
| // backend only populates BlockHeights when the block-id join is loaded; the | ||
| // parent-depth guard below relies on BlockHeights being present for a mined parent. | ||
| parentTxMeta, err := s.Get(ctx, parentTxHash, fields.Utxos, fields.BlockHeights, fields.BlockIDs, fields.UnminedSince) |
There was a problem hiding this comment.
Worth noting where the recurrence actually lands. A parent whose outputs are all spent gets a delete-at-height of mined_height + retention, the same boundary withinRetention uses below, so the pruner deletes the parent at the exact moment tolerance expires.
This read then returns TX_NOT_FOUND and line 1226 returns it raw, with no guard on it at all. That is unchanged from before the PR, but it means the fail-closed branch further down is not the path the recurrence takes.
The tolerance added in the previous commit is sound only if the store never deletes a transaction mined on the longest chain before mined_height + retention. A counter mined on a parent's output slot has mined_height >= parent_height, so its deletion height is at or beyond the parent's; that is what lets a recent parent stand in for the absent counter. Pin the horizon at its boundary, on a real store driven by the real pruner service. The earliest stamp the code can produce for a mined transaction comes from SetMinedMulti (newDAH = minedBlockInfo.BlockHeight + retention); the spend path stamps tip + 1 + retention, which is strictly later. So the worst case is a transaction mined with its outputs already spent, and that is what the tests build: it survives pruning at mined_height + retention - 1 and is deleted at mined_height + retention. The third test covers the one path that could stamp a mined transaction early: a transaction flagged conflicting while unmined carries DAH = flag_height + retention, and flag_height can precede the mined height. SetMinedMulti bumps that stale stamp forward, so the horizon holds. The conflicting flag is written with direct SQL because Store.SetConflicting deadlocks on SQLite — it opens a transaction at sql.go:4325 and then calls s.GetSpend on the pool at sql.go:4383, which cannot get a connection until the transaction commits. That is pre-existing and untouched here; the existing sql tests never reach it because they all pass an empty hash slice.
| // SVNode-following peers accept it. Below that window we cannot rule out | ||
| // a mined-then-pruned counter, so we fail closed: SVNode would reject a | ||
| // block double-spending a confirmed output. | ||
| if errors.Is(err, errors.ErrTxNotFound) || errors.Is(err, errors.ErrNotFound) { |
There was a problem hiding this comment.
[Major] The tolerance keys off the root spender's parent depth, but GetConflictingChildren returns ErrTxNotFound/ErrNotFound for any absent node in the BFS, not just the root. The descendant reads at process_conflicting.go:1127 propagate their error unchanged through g.Wait() (:1136-1138), so this branch cannot distinguish "root spender record absent" from "root spender exists but one of its descendants is a dangling ref".
In the second case the premise the comment relies on — "an absent record has no BlockIDs, so it is definitionally not mined on our chain" — does not hold: the record actually being dropped from the counter set (spendingTxID) may exist and be mined on our chain. If spendingTxID is a genuinely mined-on-chain counter (exactly the confirmed double-spend this check exists to reject) and merely one of its descendants is an absent dangling ref, tolerance fires on spenderParents[spendingTxID] being recent, spendingTxID is silently excluded, and checkCounterConflictingOnCurrentChain never inspects its BlockIDs (SubtreeValidation.go:531-537) — so a block double-spending a confirmed output is accepted rather than rejected. A descendant dangling ref is producible by the same spend-first window (#1214) at any depth, so this is not purely hypothetical.
Suggest confirming the guard's premise applies to the thing being dropped: e.g. before tolerating, do a direct s.Get(ctx, &spendingTxID, fields.Tx) and only tolerate when the root spender's own record is NotFound; if the root exists, the NotFound came from a descendant and should propagate (or be handled on its own evidence). This is a static-analysis concern — I could not execute the tests to confirm reachability; worth verifying with a targeted case (mined root + absent descendant).
freemans13
left a comment
freemans13
left a comment
There was a problem hiding this comment.
Still not mergeable, and the update does not move it. The new commit is test-only. git diff --name-only 8bd866aa f79e2ce9 lists one file, stores/utxo/sql/prune_horizon_test.go, so every blocking finding on 8bd866a stands untouched, including the root-versus-descendant fail-open that icellan, the review bot and I have now each found separately. The bot's note on that one says it could not execute anything and asks for "a targeted case (mined root + absent descendant)". That case is already on this PR, with output, in my 3 September review.
What the new commit does address is the residual risk paragraph in the description, the claim that the pruner never deletes a mined transaction before mined_height + retention. I ran the three tests on f79e2ce and they pass. They do not settle the question, for two reasons.
The audit is SQL-only, and its result inverts on Aerospike
The third test proves that SetMinedMulti bumps a stale flag-height stamp forward, so a transaction flagged conflicting and then mined keeps a deletion height at or beyond mined_height + retention. That is true of the SQL store. It is not true of Aerospike, which is the production backend, and the test comment itself points at the Lua branch where it breaks.
The mined path calls setDeleteAtHeight(rec, blockHeight, blockHeightRetention) at teranode.lua:663. The first thing that function does with a conflicting record is this:
if rec[BIN_CONFLICTING] then
if not existingDeleteAtHeight then
rec[BIN_DELETE_AT_HEIGHT] = newDeleteHeight
...
end
return "", nil
endIt sets the stamp only when there is no stamp, then returns unconditionally. There is no bump. So the exact behaviour the third test relies on to close the hazard is absent on the backend that matters, and the tests cannot catch it because they run against SQL. Worth noting the Aerospike native-ops job on this run reports skipping.
Mined first, then flagged conflicting, is not covered
The three tests all flag conflicting before mining. The reverse order behaves differently on SQL too, because SetConflicting stamps GetBlockHeight() + 1 + retention from the store's cached tip, and that cache lags. It is published on block notifications only and sits at 0 when the store starts with startBlockchain false.
Probe on f79e2ce, retention 10, real store, real pruner service:
after SetMinedMulti(height=1500, unspent outputs): delete_at_height = <nil>
after conflicting flag with cached tip 500: delete_at_height = 511
the guard assumes this tx survives to mined_height + retention = 1510
PRUNED at tip 515, which is 995 blocks BEFORE mined_height+retention (1510)
A transaction mined on the longest chain at 1500 is deleted at 515. The outputs are left unspent on purpose, because that is what leaves delete_at_height null, and null is the one condition under which the COALESCE(delete_at_height, $3) in SetConflicting writes anything at all. Nothing bumps it afterwards, since SetMinedMulti already ran.
Whether a counter-conflicting transaction specifically can reach that ordering is the question the audit needs to answer. A reorg demoting a previously mined transaction is the obvious candidate. The audit as written does not answer it either way.
The SQLite deadlock is real, and it needs its own issue
I checked the claim in the test comment rather than taking it. Store.SetConflicting opens a write transaction with s.db.Begin(), issues the UPDATE through it, and then calls s.GetSpend on the pool, which cannot get a connection until that transaction commits. Called with one real hash it does not return within 5 seconds.
That is pre-existing and this PR is right not to fix it here. But a production-code deadlock currently exists only as a comment inside a test file on an unrelated PR, and the existing SQL tests miss it because they all pass an empty hash slice. It should be an issue in its own right, otherwise it is lost the moment this branch merges or closes.
What I did not check
I ran ./stores/utxo/sql/ on f79e2ce, the three new tests pass, plus two throwaway probes which I have deleted. Nothing on Aerospike, so the Lua reading above is static only, not executed. Nothing on a node.
The one red check, legacy-sync, fails because an SVNode container could not bind RPC port 48332 on the runner. That is a port collision, unrelated to this branch.
|
|
||
| require.NoError(t, store.db.QueryRowContext(ctx, | ||
| `SELECT delete_at_height FROM transactions WHERE hash = $1`, txHash[:]).Scan(&stamped)) | ||
| require.GreaterOrEqual(t, stamped, int64(mineHeight+retention), |
There was a problem hiding this comment.
This holds for the SQL store. It does not hold for Aerospike, and the Lua branch named two lines up is where it stops holding.
The mined path calls setDeleteAtHeight(rec, blockHeight, blockHeightRetention) at teranode.lua:663, and that function opens with:
if rec[BIN_CONFLICTING] then
if not existingDeleteAtHeight then
rec[BIN_DELETE_AT_HEIGHT] = newDeleteHeight
...
end
return "", nil
endIt writes a stamp only when there is none, then returns. No bump. So the bump this test proves, and that the guard depends on, does not happen on the production backend for exactly the records at issue. The Aerospike native-ops job on this run reports skipping, so nothing here would catch it.
| // sql.go:4325 and then calls s.GetSpend on the pool at sql.go:4383, which | ||
| // deadlocks on SQLite. What is under test here is the DAH arithmetic in | ||
| // SetMinedMulti, and this sets up its precondition exactly. | ||
| func TestPruneHorizon_ConflictingThenMinedDoesNotKeepEarlierStamp(t *testing.T) { |
There was a problem hiding this comment.
All three tests flag conflicting before mining. The reverse order is the one that bites, and it is uncovered.
SetConflicting stamps GetBlockHeight() + 1 + retention from the store's cached tip, which lags (published on block notifications only, and 0 when the store starts with startBlockchain false). Probe against a real store and the real pruner, retention 10:
after SetMinedMulti(height=1500, unspent outputs): delete_at_height = <nil>
after conflicting flag with cached tip 500: delete_at_height = 511
PRUNED at tip 515, which is 995 blocks BEFORE mined_height+retention (1510)
Outputs left unspent on purpose: that is what keeps delete_at_height null, and null is the only case where the COALESCE(delete_at_height, $3) in SetConflicting writes anything. SetMinedMulti has already run by then, so nothing bumps it back.
|
Re-reviewed at f79e2ce. The new commit is 211 lines of test in On the one they do engage with — the prune-horizon premise — the result holds for SQL and not for Aerospike. SQL bumps a stale stamp forward, as the commit message says: Aerospike refuses that bump on both paths:
The comment at So the horizon is pinned on the backend where it already held, and the commit message states the conclusion generally. The test is Separately, worth its own issue rather than anything here: the deadlock you documented in the commit message is real and pre-existing — |
3 participants
The wedge
SequentialSpendAndCreateis spend-first. It records a transaction's spends on its parents, then creates the transaction's own record:A crash in that window, or a rollback that exhausts its three
unspendWithRetryattempts, leaves a parent output slot naming a spender the store has no record of — a dangling spender reference.GetCounterConflictingTxHashesthen walks that absent spender.GetConflictingChildrenfails on its root read, the error propagates out ofcheckCounterConflictingOnCurrentChain, and block validation wedges on a block that SVNode-following peers accept.Create-first (#1355) would close the window at source, but it is still open and unmerged. #1393 bounded and deduped the walk; it did not touch the absent-record case.
The guard
An absent record has no
BlockIDs, so it is definitionally not mined on our chain. Tolerance is gated on the parent's confirmation depth:retention == 0→ fail closed.Tolerance is ANDed across every slot naming the spender, not taken from the first seen, so one unprovable slot is enough to reject.
The guard is scoped to
ErrTxNotFound/ErrNotFound; every other walk error still propagates. The tip height is read lazily, only when an absent record is hit, so a store carrying no dangling reference is never asked for it.Tolerated absences increment
teranode_utxo_dangling_spender_ref_tolerated_total, so the inconsistency stays visible rather than silent.Proof
The end-to-end tests build the dangling reference on a real
sqlitememorystore —WithSpendOnly()with no matchingWithCreateOnly()is exactly the spend-first window — then runcheckCounterConflictingOnCurrentChain.With the guard disabled, identical inputs, pre-fix semantics:
With the guard:
The fail-closed case passes in both configurations: the guard does not weaken the consensus check, it only narrows what wedges.
Verification
The
go vetand lint output that remains is pre-existing, intest/utilsand older test files this PR does not touch.Prune horizon — checked
The tolerance is sound only if the store never deletes a transaction mined on the longest chain before
mined_height + retention. That assumption is now verified rather than assumed, andstores/utxo/sql/prune_horizon_test.gopins it against the real pruner service.The DAH arithmetic agrees across both backends: the Aerospike Go expression (
set_mined_expressions.go) and the Lua UDF (teranode.lua:988) both computecurrentBlockHeight + blockHeightRetention, and SQL'sSetMinedMultiusesminedBlockInfo.BlockHeight + retention(sql.go:3346). The pruner deletes ondelete_at_height <= tip. The spend path stampstip + 1 + retention, which is strictly later, so the earliest stamp a mined transaction can carry comes fromSetMinedMulti.Tests, on a real store with the real pruner:
1000 + retention - 11000 + retention1000 + retention), then mined at 1500 →SetMinedMultibumps the stale stamp forward, and it survives to1500 + retention - 1The boundary is exact, with no slack: the guard tolerates while
parent_height + retention > tip, and a counter mined ath >= parent_heightsurvives whileh + retention > tip.Residual risk
Aerospike does not bump a stale conflicting stamp.
teranode.luareturns early in its conflicting branch (if rec[BIN_CONFLICTING] then ... return "", nil), and the Go expression yieldsExpUnknownfor a conflicting record that already has a DAH. SQL bumps it forward; Aerospike does not. For that to shorten the horizon a record would have to be conflicting-flagged and mined on the longest chain at once, which is contradictory in the node's own model — conflicting means "we consider this a loser". I could not construct the state (the store refuses to spend a conflicting transaction's outputs withTX_CONFLICTING), but I did not prove it unreachable, and I did not write the Aerospike-side equivalent of these tests.Unrelated pre-existing bug found while testing.
Store.SetConflictingdeadlocks on SQLite: it opens a transaction atsql.go:4325, then callss.GetSpendon the pool atsql.go:4383, which cannot get a connection until that transaction commits. Reproduced as a hang until the 600s test timeout. Every existingsql_test.gocall passes an empty hash slice, so no current test reaches it. Not touched by this PR — the horizon test writes the conflicting flag with direct SQL to route around it. Worth its own issue.