Repository navigation
fix(subtreevalidation): follow-ups to the conflict-check verdict: cancellation wrap, over-budget walk, ctx guard - #2066
Conversation
…path even once ctx is done The cached path and blessMissingTransaction already returned it whatever the ctx state. The verdict needs a member that was read and is mined on the candidate chain, so a cancellation can only remove evidence, never make it up.
…n needs as a cancellation A store fault, the walk limit, a frozen output, an input out of range and the fail-fast threshold are retried and never charged to the peer without the CONTEXT_CANCELED wrap, so they now leave without it. A missing record and a TX_INVALID under another code keep it: without it catch-up signals a peer failure for the first, and the peer is reported for the second. A cancelled ctx keeps it too.
…y the walk's roots The partial cone the walk reached was read again in phase 3, up to the budget in store reads on every retry of a runaway cone, for the same UTXO_WALK_LIMIT_EXCEEDED. The conflicting transactions and their direct counter-spenders are still checked, and a mined one still rejects the block. A walk stopped by a store fault keeps its partial cone checked.
|
🤖 Claude Code Review Status: Complete Current Review: One point from an earlier human review is still open (icellan, on Suggest running History:
|
freemans13
left a comment
freemans13
left a comment
There was a problem hiding this comment.
Requesting changes for one case in item 2. The rest of the PR holds up. When the walk overruns its budget in its second pass, which walks the conflicting transactions' own descendants, the counter-spender cone is already complete. The PR still drops it from the mined check. So a block proven invalid by reads that all succeeded is retried until an operator raises utxostore_conflictingChildrenMaxNodes. Details and two ways out are inline.
What I checked and agree with:
- Item 1, the cancellation wrap. I traced every consumer of
CONTEXT_CANCELEDandIsContextErrorthat a settle failure can reach. That covers the Kafka retry,ValidateBlock, catch-up's route andreleaseCatchupLock,subtreeMessageHandler, and the legacy park policy (parkCommitFailure, which reaches it throughprocessBlockFound'sSERVICE_ERRORwrap). None of them turns an unwrapped store fault, walk limit, frozen output or out-of-range input into a peer fault.ValidateSubtreeInternalstill retries the threshold byerrors.Is(err, ErrThresholdExceeded). The two exceptions inkeepsCancellationmatch what the table says they need. - Item 3, no ctx guard on a proven conflict. This is sound.
firstMinedOnChainonly counts a read that succeeded. The walk reads level by level, so a store fault in a level always surfaces before a budget error from adding that level's children. Which error stops the walk therefore does not depend on timing. - The bot's nit on the unwrapped
firstMinedOnChaingodoc line stands. I won't repeat it.
Tests: I ran the PR's race-enabled command locally on 972f9d0 (go test -race over the named tests in services/subtreevalidation, stores/utxo and services/blockvalidation), and all 77 passed. I did not run the full package suites. CI was still running when I posted.
| // fault still has what it reached checked. | ||
| checked := [][]chainhash.Hash{txs, counterSpenders, counterCone} | ||
| if errors.Is(walkErr, errors.ErrUtxoWalkLimitExceeded) { | ||
| checked = checked[:2] |
There was a problem hiding this comment.
This drops the counter-spender cone whenever the walk stops on its budget, but the walk runs two passes on one shared budget: pass 0 walks the counter-spenders' cone, pass 1 walks the conflicting transactions' own cone ([][]chainhash.Hash{counterSpenders, txs} a few lines up). If the budget runs out in pass 1, counterCone is complete, not partial.
Concrete case: a conflicting tx with a large descendant chain of its own (over the shared budget), and a counter-spender with a small cone whose third member is mined on the candidate chain. Pass 0 finishes and reaches the mined member; pass 1 overruns. Before this PR the block is rejected with TX_INVALID. With it, the block gets UTXO_WALK_LIMIT_EXCEEDED and is retried until an operator raises utxostore_conflictingChildrenMaxNodes, although every read the proof needs already succeeded.
The comment's reasoning doesn't cover this case. The cone that was "too large to check" is the own cone, which phase 3 never mined-checks anyway (own-cone members are frozen-checked only). And the counter cone's cost is bounded by what pass 0 already read.
Suggestion: drop counterCone only when the walk stopped in pass 0. GetConflictingChildrenFromRoots doesn't say which pass it stopped in. An empty cones[1] is ambiguous, because pass 1 can stop before adding any child. So it would need to return the pass index, or the batch could run the two passes as two calls sharing a budget. Then a test with budget overrun in the own cone and a mined member in a complete counter cone would pin the case. The existing overBudgetWalkChecksOnlyRoots only overruns in pass 0.
If you'd rather keep it simple and accept the retry, then please widen the comment, the longdesc and the CHANGELOG entry to say the full counter cone is skipped too when it was walked completely. As written they say only "the descendants the walk reached", which reads as the partial cone.
There was a problem hiding this comment.
Looks addressed in 14d5eb3: the counter cone is mined-checked again whichever pass ran out of budget. The new subtest "own cone overflows after a complete counter cone" covers the pass-1 overflow case.
There was a problem hiding this comment.
Fixed in 14d5eb3 by dropping the narrowing: phase 3 again mined-checks whatever counter cone the walk reached, whichever pass ran out, so a complete counter cone is never skipped. overBudgetWalkChecksTheCounterConeItReached/"own cone overflows after a complete counter cone" is your case (counter cone of 3 with its third member mined, own cone over the budget, TX_INVALID); it fails at 972f9d0.
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-10-29 13:51 UTC |
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
One blocker: the over-budget narrowing at check_counter_conflicting_batch.go:208 also drops a counter cone that was walked in full. The other two comments are a question and a doc fix.
| // direct counter-spenders, are mined-checked then. A walk stopped by a store | ||
| // fault still has what it reached checked. | ||
| checked := [][]chainhash.Hash{txs, counterSpenders, counterCone} | ||
| if errors.Is(walkErr, errors.ErrUtxoWalkLimitExceeded) { |
There was a problem hiding this comment.
This narrows whenever the walk hit the limit, whichever pass it stopped in. GetConflictingChildrenFromRoots walks [counterSpenders, txs] in order, with one shared visited set and budget (stores/utxo/process_conflicting.go:1336-1353). If pass 0 finishes and only pass 1 (the own cone) overflows, cones[0] is the complete counter cone, not a partial or oversized one. checked[:2] still drops it, so a mined counter-descendant is never read and the block retries with UTXO_WALK_LIMIT_EXCEEDED instead of failing TX_INVALID. The comment above doesn't cover this case either.
overBudgetWalkChecksOnlyRoots only overflows in pass 0.
Fix: have the walk return the index of the pass it stopped in (nil vs empty can't signal it, because walkPass returns nil on both) and narrow only when it stopped in pass 0. Add a test where the counter cone fits, the own cone overflows, and a counter-cone member is mined; assert TX_INVALID.
There was a problem hiding this comment.
Looks addressed in 14d5eb3: the narrowing is gone, so counterCone (cones[0]) always goes into the mined check. Both overflow orders are covered in Test_checkCounterConflictingOnCurrentChainBatch_overBudgetWalkChecksTheCounterConeItReached.
There was a problem hiding this comment.
Fixed in 14d5eb3 by removing the narrowing instead of returning the pass index: phase 3 checks the counter cone the walk reached, complete or partial, with no change to the walk's API. overBudgetWalkChecksTheCounterConeItReached/"own cone overflows after a complete counter cone" pins the case where the counter cone fits and the own cone overflows; it fails at 972f9d0.
| // fault still has what it reached checked. | ||
| checked := [][]chainhash.Hash{txs, counterSpenders, counterCone} | ||
| if errors.Is(walkErr, errors.ErrUtxoWalkLimitExceeded) { | ||
| checked = checked[:2] |
There was a problem hiding this comment.
Question, not blocking: a mined member inside the partial counter cone is still proof, since every hash the walk returns descends from a counter-spender root. Skipping it trades a definite rejection for retries until an operator raises the budget, and saves only the phase-3 GetMeta reads, because the walk itself runs again on every retry either way. Is that the trade you want? Checking the partial cone would end the retry loop whenever it holds a mined member.
There was a problem hiding this comment.
The latest code settles this the way you suggested: a mined member in a partial counter cone is still treated as proof (subtest "counter cone overflows after reaching a mined member").
There was a problem hiding this comment.
Yes, the partial cone is now checked (14d5eb3). Saving phase-3 reads bounded by what the walk already read isn't worth retrying a provably invalid block until someone raises the budget. overBudgetWalkChecksTheCounterConeItReached/"counter cone overflows after reaching a mined member" pins it: the walk stops inside the counter cone, and the mined member it reached gives TX_INVALID. The test fails at 972f9d0.
| // fetch the block from other peers, for a record this node pruned. | ||
| // - A TX_INVALID under another code, such as a record the aerospike store read | ||
| // but could not decode. Without the wrap the Kafka path would report the | ||
| // announcing peer, and catch-up would report it malicious. |
There was a problem hiding this comment.
This overclaims. The wrap doesn't stop the announcing peer being reported. ValidateBlock's infrastructure guard (BlockValidation.go:~2270) matches context.Canceled, not the ERR_CONTEXT_CANCELED code, so a local decode fault still gets a corrupt verdict and penalizeCorruptBlockPeer. That predates this PR, and proven_conflict_classification_test.go:312-327 already says so. Either fix the comment here, or add errors.Is(err, errors.ErrContextCanceled) to that guard and flip the test case to corrupt: false.
There was a problem hiding this comment.
Looks addressed in a45eb58: the keepsCancellation doc comment now says the wrap does not stop the peer being struck, because the guard matches context.Canceled, not ERR_CONTEXT_CANCELED. It also marks this as pre-existing behaviour.
There was a problem hiding this comment.
Fixed the comment in a45eb58; the guard is unchanged. It now says the wrap keeps the Kafka retry and keeps catch-up from charging the peer or reporting it malicious, and that the decode fault still gets a corrupt verdict and penalizeCorruptBlockPeer, from the Kafka path and from catch-up, because ValidateBlock's guard matches context.Canceled, not ERR_CONTEXT_CANCELED. That is pre-existing. TestValidateBlock_ProvenConflictIsNotACancellation/"a TX_INVALID under another code" pins both halves: corrupt: true, plus the retry and catch-up classification.
…or-classification
…budget walk reached Reverts the narrowing from b098750. When only the walk's second pass, the conflicting transactions' own cone, overran the budget, the counter cone was complete and was still dropped, so a block proven invalid by reads that all succeeded retried until the budget was raised. A partial counter cone is proof too: every member descends from a counter-spender. Phase 3 again mined-checks the transactions, their counter-spenders and whatever counter cone the walk reached, whichever error stopped it. The setting's longdesc, the settings doc and the CHANGELOG go back to the wording bsv-blockchain#2052 merged. Adds a case for each pass running out with a mined counter-cone member; both fail with the narrowing.
…code fault The wrap keeps the Kafka path retrying a TX_INVALID under another code and keeps catch-up from charging the peer or reporting it malicious. It does not stop the strike: ValidateBlock's infrastructure guard matches context.Canceled, not ERR_CONTEXT_CANCELED, so the block still gets a corrupt verdict and penalizeCorruptBlockPeer. That is pre-existing.
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
The revert in 14d5eb3 fixes the over-budget blocker, and a45eb58 fixes the doc comment.
Nits, on lines this PR doesn't change:
processTxMetaUsingStore.go:443: the unbatched branch still passeserrProcessTxMetaContextDonetosettledError. Now that the wrap also covers a pruned record or a decode fault with ctx still live, "context done" sends whoever reads the log after a cancellation that never happened. The batched branch uses a different message for the same thing.CHANGELOG.md:732(the 2052 entry) and theutxostore_conflictingChildrenMaxNodeslongdesc say an over-budget walk rejects once it "reached a mined member". Only counter-conflicting members get the mined check; an own-cone member gets only the frozen check. The Go doc gets this right; the operator text should match.
Follow-ups, already wrong before this PR:
BlockValidation.go:~2368: the infrastructure guard matchescontext.Canceled, notERR_CONTEXT_CANCELED, so a local decode fault still strikes the peer, once per Kafka retry.BlockValidation.go:2419:NewBlockIncompleteTransientError("... subtree validation: %s", hash, err)takeserras the wrapped error, so the message renders%!s(MISSING).isLocalCatchupFaultdoesn't checkIsTransientBlockIncomplete, so a transient missing-data verdict without the cancellation wrap still reachesReportPeerFailure.
…or-classification
… error The unbatched branch passed errProcessTxMetaContextDone to settledError, so a pruned record or a decode fault wrapped while ctx was live logged as "context done". Both branches now share one neutral message; no classification changes. The operator text for utxostore_conflictingChildrenMaxNodes (the setting's longdesc, the settings doc and the bsv-blockchain#2052 CHANGELOG entry) now says only a mined counter-conflicting transaction is proof after an over-budget walk, as the batch's Go doc does: the conflicting transactions' own descendants are only checked for frozen outputs.
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
Re-review at a032644: nothing blocking, and my approval stands. The three comments below are low severity and can be done here or in a follow-up. Opened #2072 for the root cause behind them: wrap the conflict check's store reads as StorageError so the decode-fault peer strike and the legacy-route park go away.
| return ctx.Err() != nil || | ||
| errors.Is(err, errors.ErrContextCanceled) || | ||
| errors.IsContextError(err) || | ||
| errors.Is(err, errors.ErrTxNotFound) || |
There was a problem hiding this comment.
With the ErrTxNotFound and ErrTxInvalid arms, CONTEXT_CANCELED doubles as a 'local, not the peer's' marker. A pruned record then shows up as a 'Context canceled, skipping' warning on the live path and as local_context_cancelled in catch-up. Anyone reading either will look for a cancellation that never happened. A local code is better (see #2072).
The wrap is also decided from the single error fail() kept. If a phase-1 record is missing and the walk also exceeds utxostore_conflictingChildrenMaxNodes, the TxNotFound wins and gets wrapped. Catch-up then records local_context_cancelled instead of the local_processing_fault the CHANGELOG promises for the walk limit, and the operator gets no hint to raise the budget.
There was a problem hiding this comment.
No change in 9f975ec; this moved to #2072. Replacing CONTEXT_CANCELED as the "not the peer's" marker needs a local error code, and that fixes both halves: the "Context canceled" label on a pruned record, and a TxNotFound outranking the walk limit in one batch, which hides local_processing_fault and the hint to raise utxostore_conflictingChildrenMaxNodes. TestValidateBlock_ProvenConflictIsNotACancellation/a pruned parent pins today's labelling until then.
| func keepsCancellation(ctx context.Context, err error) bool { | ||
| return ctx.Err() != nil || | ||
| errors.Is(err, errors.ErrContextCanceled) || | ||
| errors.IsContextError(err) || |
There was a problem hiding this comment.
This arm matches on text: (*Error).Is falls back to strings.Contains for a stdlib target. So a StorageError whose text contains 'context deadline exceeded' is still wrapped as a cancellation while ctx is live, which goes against the CHANGELOG line saying store faults are no longer wrapped. ctx.Err() and errors.Is(err, ErrContextCanceled) already cover real cancellations. The arms also overlap, so it isn't clear which one is load-bearing. Suggest ctx.Err() != nil || errors.Is(err, ErrContextCanceled) || errors.Is(err, ErrTxNotFound) || errors.Is(err, ErrTxInvalid), a one-line reason per arm, and a test that a live ctx plus NewStorageError(..., context.DeadlineExceeded) stays unwrapped.
There was a problem hiding this comment.
Looks addressed in 9f975ec: keepsCancellation no longer calls IsContextError, so it reads codes only (ctx.Err(), ERR_CONTEXT_CANCELED, ERR_TX_NOT_FOUND, ERR_TX_INVALID), and each arm has a one-line reason. New tests pin a live ctx with NewStorageError(..., context.DeadlineExceeded) staying ERR_STORAGE_ERROR, in subtreevalidation and through ValidateBlock (local_storage_fault). A raw stdlib DeadlineExceeded that reaches ValidateBlock is still matched by its infrastructure guard, so it is retried.
There was a problem hiding this comment.
Fixed in 9f975ec: keepsCancellation is now ctx.Err() != nil || Is(ErrContextCanceled) || Is(ErrTxNotFound) || Is(ErrTxInvalid), one line of reason per arm; the settle path derives no contexts of its own, so the IsContextError arm caught nothing real. Pinned by the settledError subtest "a store fault whose text names a deadline is not a cancellation while ctx is live" and the two new "store fault naming a deadline" rows in TestValidateBlock_ProvenConflictIsNotACancellation, which all fail with the arm restored.
| return errors.NewContextCanceledError(message, err) | ||
| } | ||
|
|
||
| return err |
There was a problem hiding this comment.
An unwrapped failure now leaves without the [processTxMetaUsingStore] prefix. ValidateSubtreeInternal still says "Don't wrap the error again, processTxMetaUsingStore returns the correctly formatted error", which is no longer true. A batch-decorate StorageError or a walk-limit error then reaches the log and the gRPC response untagged. Either keep a non-cancellation wrap that carries the prefix and the original code, or update that comment.
There was a problem hiding this comment.
Looks addressed in 9f975ec: prefixed() puts [processTxMetaUsingStore] on top under the failure's own code, so errors.Is and code-based classification are unchanged, and the "returns the correctly formatted error" comment in ValidateSubtreeInternal holds again. A proven conflict now leaves prefixed too, still with TX_INVALID on top, so provenConflict and ValidateBlock see the same code. Only a non-*errors.Error passes through without the prefix, as the doc comment says.
There was a problem hiding this comment.
Fixed in 9f975ec: a failure that isn't wrapped as a cancellation now leaves with [processTxMetaUsingStore] on top under its own code (prefixed), so ValidateSubtreeInternal's comment holds again and no errors.Is result changes. Pinned by the settledError subtest "a failure that is not wrapped keeps the prefix and its code"; the class × classifier table passes unchanged.
freemans13
left a comment
freemans13
left a comment
There was a problem hiding this comment.
Approving at a032644. My blocker from 972f9d0 is fixed. The other two commits since then are text fixes and are correct.
The blocker. 14d5eb3 removes the narrowing, so phase 3 mined-checks whatever counter cone the walk reached, in whichever pass it stopped. That is safe on an early stop: GetConflictingChildrenFromRoots sizes cones to both passes before walking, so cones[1] exists even when pass 0 overflows, and a mined member still wins over the walk-limit error. I checked the new test is not a rubber stamp. With check_counter_conflicting_batch.go put back to its 972f9d0 version, both subtests of Test_checkCounterConflictingOnCurrentChainBatch_overBudgetWalkChecksTheCounterConeItReached fail ("Target error should be in err chain"). On a032644 they pass under -race.
The text commits. a45eb58 makes the keepsCancellation doc say what the wrap does and does not do for a decode fault. a032644 replaces "context done" with a neutral prefix on both settle branches. It also brings the CHANGELOG entry, the setting's longdesc and utxo_settings.md in line with the Go doc: only counter-conflicting members get the mined check, and own-cone members get only the frozen check.
icellan's three comments at a032644. I agree with all three and won't repeat them. I checked the text-match one myself. IsContextError ends in Is(err, context.DeadlineExceeded), which on an *Error falls back to strings.Contains on the message (errors/errors.go:177). So a live-ctx StorageError wrapping an Aerospike deadline is still wrapped as a cancellation. None of the three blocks merge. They fit here or in #2072.
What I did not check. I ran only the new test, not the full subtreevalidation and blockvalidation suites. CI ran them: 37 checks pass on a032644. The Aerospike native-ops job skipped, as it does on every PR.
…or-classification # Conflicts: # services/blockvalidation/proven_conflict_classification_test.go
…r prefix keepsCancellation no longer calls IsContextError. Its DeadlineExceeded check falls back to matching error text, so a store fault whose message named a deadline was wrapped as a cancellation while ctx was live. ctx.Err() and ERR_CONTEXT_CANCELED already cover every real cancellation on the settle path, which derives no contexts of its own. A failure settledError does not wrap as a cancellation now leaves with the [processTxMetaUsingStore] prefix under its own code, so errors.Is and every caller's classification are unchanged, and ValidateSubtreeInternal's "returns the correctly formatted error" holds again.
|
Three follow-ups from icellan's approval of #2052. The two doc items from that review are already merged. Merges to
main; intended for v0.16.1.In short:
TX_INVALIDunder another code keep the wrap, because block validation needs it to treat them as this node's own failure;TX_INVALIDeven oncectxis done, as it already was on the cached path and in bless.Production code:
services/subtreevalidation(processTxMetaUsingStore.go,check_counter_conflicting_batch.go), plus the setting's description insettings/anddocs/. Tests inservices/subtreevalidationandservices/blockvalidation.Terms
From #2052, all in
services/subtreevalidation:checkCounterConflictingOnCurrentChainBatch. It checks whether any of a block's conflicting transactions (ones that lost a double spend on this node) conflicts with a transaction mined on the candidate chain. Phase 1 reads each transaction's counter-spenders (the transactions recorded as spending the same outputs). Phase 2 walks their descendants, the cone, from those roots, under the node budgetutxostore_conflictingChildrenMaxNodes. Phase 3 reads each member (GetMeta) and looks for one mined on the candidate chain.TX_INVALID).processTxMetaUsingStore, for a subtree this node has not validated. Its last step, settle (settleCollectedConflictingTxs), runs the batch. Cached path:CheckBlockSubtreesreusing a subtree already validated. Bless:blessMissingTransaction, for a transaction fetched because the node did not have it.ValidateBlockreturnsBLOCK_CORRUPTfor an invalid transaction in a subtree list not yet bound to the header. The peer is struck and the block re-downloaded rather than remembered as invalid.1. The cancellation wrap, only where block validation needs it
Why
processTxMetaUsingStorewrapped every settle failure except a proven conflict inCONTEXT_CANCELED. icellan pointed out that most of them are retried and never charged to the peer without it, so the wrap only mislabels them. On the live path a store fault was logged as "Context canceled, skipping", and catch-up recorded it aslocal_context_cancelled.What I checked
I traced every settle failure through each caller that reads
CONTEXT_CANCELED:isRecoverableBlockMessageError), atvalidateBlockSubtreesand atValidateBlock;ValidateBlock's branches;isLocalCatchupFault, which gates the route inprocessCatchupChItem, thevalidation_failuremetric and the malicious report, andreleaseCatchupLock's label;subtreeMessageHandler.A processing error with no peer code in its chain is local at every one of them since #2022.
Both settle branches, batched and unbatched, now use the same
[processTxMetaUsingStore]message; the unbatched one used to say "context done" even when ctx was live. Every settle failure leaves with that prefix on top: under the cancellation wrap where it keeps one, otherwise under its own code (prefixed), soerrors.Isand every caller's classification are what they would be without it.Two classes would change verdict without the wrap, so they keep it:
TX_NOT_FOUND). icellan's reading holds atvalidateBlockSubtrees. ButValidateBlockthen turnsTX_NOT_FOUNDinto a transientBLOCK_INCOMPLETE, and that code is incarriesPeerFaultCode. Without the wrap,isLocalCatchupFaultis false. Catch-up would then do three things for a record this node pruned:ReportPeerFailure, which rotates the sync peer;validation_failureagainst the peer;TX_INVALIDunder another code. The aerospike store returns one for a record it read but could not decode (classifyRecordError). The wrap keeps the Kafka path retrying the block (isRecoverableBlockMessageError) and keeps catch-up from charging the peer (isLocalCatchupFault) or reporting it malicious (shouldReportConsensusMalicious). It does not keep the peer from being struck:ValidateBlock's infrastructure guard matchescontext.Canceled, not theCONTEXT_CANCELEDcode, so the decode fault still gets a corrupt verdict andpenalizeCorruptBlockPeer, on the Kafka path and in catch-up alike. That is pre-existing, and this PR does not change it.A cancelled
ctx, or a failure that is itself a cancellation (CONTEXT_CANCELED), keeps the wrap too. Nothing else does (keepsCancellation). It reads codes only:IsContextErroris gone from it, because itsDeadlineExceededcheck falls back to matching error text, which wrapped a store fault whose message named a deadline whilectxwas live. The settle path derives no contexts of its own, soctx.Err()andCONTEXT_CANCELEDcover every real cancellation.Result, per class
Before → after. "Same" means that cell did not change.
CONTEXT_CANCELEDValidateBlockreleaseCatchupLocklabelTX_NOT_FOUND)local_context_cancelled(same)local_context_cancelled→local_processing_faultSTORAGE_ERROR)local_storage_fault(same)ctxlive (check or decorate)local_storage_fault(same)local_context_cancelled→local_processing_faultlocal_context_cancelled→local_processing_faultg.Wait())local_storage_fault(same)g.Wait())TX_INVALIDunder another codelocal_context_cancelled(same)TX_INVALIDvalidation_failureNotes on the table:
processBlockFoundpasses a corrupt or invalid verdict on as it is, and wraps everything else inSERVICE_ERROR, which is retried. So every row except the corrupt ones is retried on Kafka whatever the wrap. The test checks the stricter thing:isRecoverableBlockMessageErroris true on the raw error, at bothvalidateBlockSubtreesandValidateBlock.releaseCatchupLocklabel is the error type of catch-up's last failed attempt, shown on the dashboard. Nothing scores on it.releaseCatchupLocktests storage before context, so their label was alreadylocal_storage_fault. A store'sSERVICE_UNAVAILABLE(the aerospike batch timeout) sits under the same processing wrap, and itslocal_service_unavailablelabel also comes before context. It is not driven in a test.ValidateSubtreeInternalretries the threshold by its code, so the threshold does not reachValidateBlock. The test checks the shape it would arrive in.TX_INVALIDunder another code,ValidateBlockalready returns a corrupt verdict and strikes the peer (penalizeCorruptBlockPeer), wrap or not, because its infrastructure guard testscontext.Canceled, not theCONTEXT_CANCELEDcode. The wrap covers only the Kafka retry and catch-up's classification. The test pins today's behaviour; this PR does not change it.2. The counter cone stays checked after an over-budget walk
The first version of this PR mined-checked only the roots after a walk over its budget. Review (freemans13, icellan) showed that was wrong:
So the narrowing is gone. Phase 3 mined-checks the transactions, their counter-spenders and whatever counter cone the walk reached, whichever error stopped it, as #2052 merged it. The setting's longdesc,
utxo_settings.mdand the #2052 CHANGELOG entry keep their merged meaning, but now say that only a mined counter-conflicting transaction (a conflicting transaction, a counter-spender or a counter-spender's descendant) is proof. The conflicting transactions' own descendants are only checked for frozen outputs. This matches the batch's Go doc.What remains against
main:firstMinedOnChaingodoc says why the other reads are not cancelled after one fails: that would lose a mined member not read yet, and with it a proven conflict. icellan's review of fix(subtreevalidation): a proven conflict rejects the block even when the check failed elsewhere #2052 noted the cancellation was removed; it stays removed, and the wider read set makes that more important, not less.3. A proven conflict on the fresh path ignores
ctxOnly the fresh path checked
ctx.Err()before letting a proven conflict through. The cached path and bless returnedTX_INVALIDwhatever thectxstate, so a block was rejected or retried depending on which path reached its transaction. The guard is gone.This cannot let a partial read through as proof. Each piece of evidence comes from a read that succeeded:
firstMinedOnChainsets the mined member only after aGetMetathat succeeded and lists a block ID of the candidate chain. A failed read, cancelled or not, is recorded as an error and never counted as mined.So a cancellation can only remove evidence.
Tests
All on the real
sqlitememoryUTXO store.TestValidateBlock_ProvenConflictIsNotACancellation(blockvalidation), extended. It drives each failure in the table throughValidateBlockwith a real subtree validation server, and checks for each:validateBlockSubtrees: the class's code, whetherCONTEXT_CANCELEDis there, andisRecoverableBlockMessageError;ValidateBlock: whether it is corrupt, notBLOCK_INVALID,isRecoverableBlockMessageError,isLocalCatchupFault, notshouldReportConsensusMalicious, notisUnboundTxInvalidVerdict, and the labelreleaseCatchupLockrecords.The new rows are a store fault in the check, a store fault in a decorate batch, the same two with a store fault whose text names a deadline (
NewStorageError("store down", context.DeadlineExceeded),ctxlive), a frozen output, an input out of range, and aTX_INVALIDunder another code. The store faults go through a store wrapper given to the subtree validation server (setupWithSubtreeValidationStore). Against the code before this PR, every verdict assertion passed; only the wrap and the label assertions differed, as in the table.Test_checkCounterConflictingOnCurrentChainBatch_overBudgetWalkChecksTheCounterConeItReached: two roots (the conflicting tx and its counter-spender). Each subtest first checks its premise onGetConflictingChildrenFromRootsdirectly: the walk stops on its budget, the mined member is in the counter cone it returned, and that cone is complete or cut short. Then the batch must returnTX_INVALIDnaming the mined member.own cone overflows after a complete counter cone: the reviewers' case. Budget 4 per root; a counter cone of 3 whose third member is mined (not a root, not a direct counter-spender); an own cone of 12.counter cone overflows after reaching a mined member: icellan's question. Budget 2 per root; a counter cone of 10 whose second member is mined; the walk stops in the counter cone.Both fail at
972f9d00a(the narrowing): the premise checks pass, the batch returnsUTXO_WALK_LIMIT_EXCEEDEDinstead ofTX_INVALID.Test_provenConflictWithCancelledCtxIsTxInvalidOnEveryPath: a store wrapper cancels the caller'sctxon the check's first read and serves that read and every later one anyway. A mined counter-spender givesTX_INVALID, withoutCONTEXT_CANCELED, on the fresh path (batched and unbatched), the cached path and bless. The fresh-path subtests fail at7b9037272.Changed tests:
provenTxInvalidWins/"a mined member the walk reached before its budget ran out is proof": its budget case is nowoverBudgetWalkChecksTheCounterConeItReached/"counter cone overflows after reaching a mined member". In its place,"a mined member the walk reached before a store fault stopped it is proof"covers the other way a walk stops. To support it,faultyGetStorenow fails the walk's read of its hash too, not only the phase-1 read.provenConflictIsNotACancellation/"once ctx is done a proven conflict keeps the wrap": item 3 reverses it. The new three-path test replaces it. Its second half became"a TX_INVALID under another code is not a proven conflict".passedConflictCheckKeepsEntries: the threshold no longer carriesCONTEXT_CANCELED.provenConflictIsNotACancellationgains twosettledErrorsubtests: a store fault whose text names a deadline stays unwrapped whilectxis live, and a failure that is not wrapped (a proven conflict, a store fault, the walk limit, the threshold) keeps its code with[processTxMetaUsingStore]on top.TestGetConflictingChildrenFromRoots_StoppedWalkReturnsPartialConesand the other walk-limit tests ("a walk over its budget does not hide a mined direct counter-spender",budgetScalesWithRoots, the blockvalidation walk-limit row) are unchanged, and pass.Mutation results
Each mutation was applied on its own to this branch. The
972f9d00arow puts back that commit'scheck_counter_conflicting_batch.go.keepsCancellationdropsTX_NOT_FOUND(icellan's proposal as written)TestValidateBlock_ProvenConflictIsNotACancellation/a pruned parent. With the wrap assertion off,isRecoverableBlockMessageErroron theValidateBlockerror is false.keepsCancellationdrops theTX_INVALIDunder another code…/a TX_INVALID under another code. With the wrap assertion off,isRecoverableBlockMessageErroratvalidateBlockSubtreesis false.missingRecordsErrorcodes a missing recordNOT_FOUND(a peer code)…/a pruned parentsettledErrorwraps every failure again (the code before this PR)…/a walk over its budget,a store fault in the conflict check,a store fault in a decorate batch,a frozen output…,an input out of range…settledErrorkeeps thectxguard (the code before this PR)Test_provenConflictWithCancelledCtxIsTxInvalidOnEveryPathfresh path, both branches972f9d00a)overBudgetWalkChecksTheCounterConeItReached, both subtestsprovenTxInvalidWins/"…before a store fault stopped it is proof"keepsCancellationgets itsIsContextErrorarm back…/a store fault naming a deadline in the conflict check,…/in a decorate batch, andprovenConflictIsNotACancellation/"a store fault whose text names a deadline…"settledErrorreturns an unwrapped failure without the prefixprovenConflictIsNotACancellation/"a failure that is not wrapped keeps the prefix and its code". The blockvalidation table passes either way: the prefix changes no classification.Commands
All pass. Not run locally: the full package suites, which CI runs.
Breaking
CHANGELOG.md, under "Breaking — behaviour changes on upgrade" (both entries are unreleased):No setting or deployment change is needed. Nothing here is consensus logic: each change decides whether a block is rejected now or retried, never whether an invalid block is accepted.