Repository navigation
Conversation
… during a reorg A reorg folded every tx in block assembly into MarkTransactionsOnLongestChain(false), 380M records and ~8 minutes per 1-back/2-forward fork at scale with intake stalled. The mark-false set is now the losing txs plus moved-back txs not in a moveForward block that are still in block assembly. The sweep was the only repair for a tip overtaken before block assembly moved onto it: setTxMined had already cleared unmined_since on its txs. setTxMined now re-checks the chain after writing and re-marks the txs that are not mined on the new longest chain, before mined_set is written. The per-tx chain filter is extracted to model.FilterTxsNotOnLongestChain and shared by setTxMined and the invalid-block reorg check, which now reads only the invalid block's txs instead of the whole mempool. Refs bsv-blockchain#2051
A tip that is overtaken before block assembly moves onto it keeps the unmined_since its setTxMined cleared: block assembly reaches the new tip by a pure catch-up and never sees it as a moveBack block. Block assembly's reorg used to cover this by re-marking its whole mempool; with that sweep narrowed, block validation repairs it. Block validation tracks every block setTxMined writes as on the longest chain, recording it before the write. When one has left the chain, its txs that still look mined and are not mined on the new chain are re-marked not on the longest chain. A re-marked block that comes back onto the chain has its txs marked mined again; a block that comes back with nothing re-marked is simply eligible for repair again. Invalid blocks and blocks 1000 or more below the tip are dropped. The repair runs on the setMined worker, between blocks, as bounded steps: one subtree load or one slice of txs (blockvalidation_tipRepairSliceSize, default 250000) per step, holding only the current subtree's tx hashes. No setTxMined write of this process lands inside a step, so a tx the overtaken block shares with another block ends correct in either order. A new setTxMined write of a block abandons any repair in progress for it. Steps are requested by best-chain notifications and completed writes, and by the periodic tick while the last step failed. Transient failures are retried for as long as it takes; a block whose block or subtrees are gone is dropped after ten attempts. After a restart the tracker is seeded, once the process marks a block mined, from the main chain and from mined fork blocks near the tip. The main- chain id set is extended rather than refetched when the best block only extends the previous one. A subtree that is gone or unreadable is skipped with an error log so the rest of the block is still repaired, and marking a block mined again only rewrites the txs that were actually re-marked. Limitations: the repair is serialised with this process's setMined worker only, so it assumes one block validation instance writes setTxMined for the chain. Repair state is in memory; a re-marked block that comes back onto the chain after a restart keeps its txs flagged unmined until loadUnminedTransactions' fix pass, and fork blocks deeper than ten below the tip at restart are not re-seeded. The repair races the pruner, so its writes use a new MarkTransactionsOnLongestChain option, WithSkipMissing, that makes a record pruned since it was read a logged skip instead of fatal. Every other caller keeps the fatal check. The option is variadic: callers are unchanged, and wrappers pass it through. SubtreeProcessor.reset keeps its whole-assembly sweep as the catch-all for a reset that lands before a repair. FilterTxsNotOnLongestChain reads BlockIDs and UnminedSince in batches through BatchDecorate, skips txs that are already unmined, takes the caller's main-chain id set, and resolves each distinct set of block ids with one CheckBlockIsInCurrentChain call, so a repair writes only what changes and a fork storm costs a handful of chain lookups, not one per tx. The test MockStore implements GetLastNBlocks for the seed. Refs bsv-blockchain#2051
…ing-dependent tests Surfaced by -race runs and Aerospike runs on this branch; none is caused by it. - stores/blob/memory: currentBlockHeight was written by SetCurrentBlockHeight and read by the TTL cleaner goroutine without synchronisation. It is now an atomic.Uint32. - TestIntegrationRetryWithMultipleFailures sent all three peer announcements at once. When peer3 stored the block first, the failing peers never fetched and their attempt counters stayed at 0. Announcements now go out one at a time, each after the previous peer has been tried. - The commitBlock harness relied on another test having initialised the prometheus metrics; run on its own, commitBlock dereferenced a nil histogram. - TestUnminedSinceReorgScenarios generated its 1005 setup blocks in one RPC call, which outlasts the 30s RPC timeout on Aerospike. It now generates them in chunks. Refs bsv-blockchain#2051
|
🤖 Claude Code Review Status: Complete Current Review: The only new commit since the last review is The backoff trade-off thread is resolved. The author pointed out that the delay that matters runs from the read that recovers to the next pass. That gap is capped at Notes (non-blocking, unchanged): the Aerospike skip-missing branch still has no test of its own. Suggested verification: History:
|
freemans13
left a comment
freemans13
left a comment
There was a problem hiding this comment.
Approving. I read the three commits at 116eed1 and found no correctness bug. The reorg now marks only what it moved, and the overtaken-tip repair is sound for one block validation instance. The comments below are non-blocking.
What I checked, so you can see where the approval comes from:
- Repair against setTxMined. The repair's read and write both run on the setMined worker, so no setTxMined write can land between them. If block assembly's reorg mark(true) races an unmark slice,
stampedis set before the write, so the next slice starts a remine job and the tx ends mined. I traced this by hand and found no order that leaves a tx looking mined while it is off the chain, short of a restart. - Which direction is safe. A mined tx left flagged unmined is the safe failure. The Aerospike pruner keeps any non-conflicting record with
unminedSinceset (dropUnminedRecords), andfixUnminedSinceInconsistenciesclears it on the next load. The unsafe failure is the other one: a tx that looks mined while off the chain. Its parents can then be pruned from under it, which is the 1768 class of bug. My two comments are both about how that state can still arise. FilterTxsNotOnLongestChainkeeps the oldcheckMarkNotOnLongestChaindecisions. It adds one skip, for records that are already unmined, and a mark would not change those anyway.- Setting wiring.
blockvalidation_tipRepairSliceSizeappears insettings.conf,settings.go, the struct tag and the reference doc, all with the default of 250000.
I agree with the docs finding in the github-actions review: docs/topics/stores/utxo.md section 10 should list MarkTransactionsOnLongestChain for Block Validation. I have not repeated it inline.
What I ran and what I did not: go test -race -count=1 on ./model/, ./services/blockassembly/subtreeprocessor/, ./stores/utxo/sql/ and ./services/blockvalidation/ at 116eed1, all passing. When I posted this, CI on 116eed1 still had test, legacy-sync, three smoketest and sequential shards, and two benches pending. I did not run the Aerospike-tagged tests or the e2e reorg suite.
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-10-29 00:20 UTC |
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
Request changes. The new overtaken-tip repair can miss a partially committed mined-status write after a restart, leaving an off-chain transaction flagged mined across restarts. That transaction is invisible to unmined loading and can be pruned with its parents.
- P1 —
services/blockvalidation/BlockValidation.go:1633(setTxMinedStatus): An earlySetMinedMultibatch can clearunmined_sincebefore a later batch fails, leaving the block withMinedSet=false. If a winning block seeds repair before the losing block's off-chain retry, the seed skips it; the retry leavesunmined_sincecleared. When block assembly moves back from an earlier block, the narrowed mark-false set no longer catches the transaction. Persist or reconstruct repair eligibility for partially written blocks, or restore their status on an off-chain retry. Add a regression test for that ordering. This needs a partialSetMinedMultifailure, a restart, then that ordering and a move-back reorg; it is reasoned from the code, not reproduced end to end. - SonarQube Quality Gate is red at this head.
…t way before a restart A setTxMined that cleared some of a block's txs and then failed leaves the block's mined_set unwritten. After a restart the seed skipped such blocks, and their retry, with onLongestChain=false once overtaken, leaves unmined_since alone, so the cleared txs kept looking mined while off the chain. The seed now tracks a nearby fork block whose mined_set is not written as unwritten, so its retry makes it repairable. Also covers the repair's error paths with tests, documents the remaining restart limitation and the multi-replica race, lists MarkTransactionsOnLongestChain for block validation in the UTXO store write-access docs, and adds the CHANGELOG entry.
|
Fixed in aa08cf6. The restart seed now tracks fork blocks within Still open, now in the godoc and CHANGELOG: fork blocks deeper than |
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
Request changes. The new restart seed and its regression test fix the seed-before-successful-retry failure from my previous review, and CI is green. A separate partial-write path still leaves an off-chain transaction falsely marked mined across restarts while recovery keeps failing.
- P1 —
services/blockvalidation/overtaken_tip_repair.go:428(tipRepairSlice): After an on-chainSetMinedMultichunk commits but a later chunk fails, a near-tip fork block is seeded withwritten=false. If its off-chain retry keeps failing, repair skips that block; periodic retries re-enqueue it but do not restore the committed transactions'unmined_since. The narrower block-assembly reorg sweep no longer catches a still-held transaction from that block in the move-back sequence. Make the committed portion repairable without requiring a complete retry, or restore its status on the failure path. Add a test that keeps the retry failing after restart and overtake. This needs a partialSetMinedMultifailure, an off-chain retry that keeps failing, a restart and an overtake; it is reasoned from the code, not reproduced end to end.
…, stuck jobs and remine - Read a subtree the way setTxMined does: fall back to the .subtreeToCheck file and retry a failed read, so a block marked mined from a .subtreeToCheck file, or a read truncated while the file moves, no longer skips its txs for good. - Set aside a job that fails 10 steps in a row so it stops holding up the repair of every other block; it starts over once they have had their turn. - Remine only txs that still carry the block's id: a tx pruned and created again while the block was off the chain is in no block and stays unmined. - Run the reorg mark-scope tests on a real sqlitememory chain store instead of a mocked blockchain client.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
Request changes. CI is green, and the new tests cover their direct guards, but the repair can now treat a temporary blob-store failure as a completed repair. The partial-write P1 from my last review also remains open. Both can leave an off-chain transaction falsely marked mined.
- P1 —
services/blockvalidation/overtaken_tip_repair.go:715(loadSubtreeNodes): After three transient subtree read errors, this wraps the last error asSubtreeNotFound.jobStepskips the subtree and marks the block done, so recovery of the blob store does not revisit its transactions. A focused repro left an off-chain transaction'sunmined_sincecleared after the store recovered. If the fork is more than ten heights below the tip before restart, the seed also misses it. Preserve transient error classification after the local retries and add a test that succeeds on a later repair slice. This needs three consecutive transient read errors on one subtree; the wrong state is reproduced in a focused test. - P1 —
services/blockvalidation/overtaken_tip_repair.go:464(startNextJob): The!b.writtenguard still excludes a seeded fork block after an earlySetMinedMultichunk commits and a later chunk fails. If its off-chain retry keeps failing, the committed transactions stay falsely mined; the narrower block-assembly reorg sweep no longer catches a still-held transaction in the move-back sequence. Make the committed portion repairable without requiring the entire retry to succeed, or restore its status safely on the failure path. Add a test that keeps the retry failing after restart and overtake. Still reasoned from the code, not reproduced end to end.
ordishs
left a comment
ordishs
left a comment
There was a problem hiding this comment.
The narrower reorg mark set is correct. The overtaken-tip repair handles the case that the old sweep covered, and its serialisation on the setMined worker is sound. The PR documents the restart and single-instance limits.
…t subtree read errors The overtaken-tip repair now repairs a tracked block whose setTxMined write failed part way, instead of waiting for a retry to complete it; a retry that keeps failing left the cleared txs looking mined while off the chain. The restart seed watches main-chain blocks whose mined_set is unwritten as well. A completed write has a finished repair run again. loadSubtreeNodes only treats a subtree that cannot be deserialized as gone. A blob-store error that outlasts the read attempts is returned as is, so the step is retried rather than the subtree skipped for good. Updates the MarkTransactionsOnLongestChain signature in the utxo store reference.
# Conflicts: # services/blockvalidation/integration_retry_test.go
|
Both fixed in d8fb981. |
# Conflicts: # CHANGELOG.md
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
Request changes. The latest commit fixes the partial-write retry and direct blob-store error paths, but a short subtree stream can still make repair treat a transient read failure as permanent. The resulting off-chain transaction can remain falsely mined after storage recovers.
- P0 —
services/blockvalidation/overtaken_tip_repair.go:718(loadSubtreeNodes): Three short subtree streams produceErrProcessingfromreadSubtreeNodes, which this branch turns intoErrSubtreeNotFound.jobStepthen skips the subtree and finishes repair, leaving its off-chain transactions withunmined_since=0. A focused test with three short reads reproduced that state even though the next read was healthy. A later restart outside the ten-height seed window can preserve it. Keep short or failed streams retryable after the local attempts, and add a test that recovers on a later repair slice.
…of skipping it A short read looks the same as a corrupt subtree file, so mapping a deserialize failure that outlasts the read attempts to SubtreeNotFound let a run of short reads skip the subtree for good, leaving its txs looking mined while off the chain. The error is now returned for the next step to retry; a job that keeps failing is set aside. Also removes a race in TestRepairOvertakenTips_RunsOnTheSetMinedWorker: the test wrote the competing block's tx straight to the store after the chain flipped, where the live worker's repair slice could interleave with it.
|
Fixed in a4cfefa. |
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
Request changes. The new retry path fixes the transient short-read case, but a persistent corrupt subtree can now prevent repair of later intact subtrees in the same overtaken block. Their transactions remain falsely marked mined while off chain, and that status can survive a restart after the fork leaves the startup seed range. The initiating blob fault is internal; the later-subtree regression is new in this commit.
- P0 —
services/blockvalidation/overtaken_tip_repair.go:718(loadSubtreeNodes): After a successful on-chain mined-status write, a persistently unreadable earlier subtree makesjobStepretry at the same index forever, even when later subtrees are intact. Ten failed steps only defer and restart the block job. A focused two-subtree repro left the intact subtree's transaction withunmined_since=0after the block was overtaken; restoring the prior error classification repaired it. Let later intact subtrees progress while preserving retries for transient short reads, and add a regression test for both cases. The repro uses a synthetic corrupt subtree; restart survival follows from the ten-height seed bound rather than a separate restart run.
…led repair steps An unreadable subtree was retried forever: the job failed, was set aside, started again from subtree 0 and failed at the same subtree until the block left the window, so the subtrees after it were never repaired. A deserialize failure is now a SubtreeInvalid error; when it is still failing on the job's tipRepairMaxAttempts-th failed step in a row, the subtree is skipped with an error log and the rest of the block is repaired. A short run of short reads still cannot cause a skip. Updates the jobStep godoc.
… block's later subtrees
# Conflicts: # CHANGELOG.md
|
Fixed in 728aa73, which landed just after your review of a4cfefa. A subtree that still cannot be deserialized on the job's tipRepairMaxAttempts-th failed step in a row is skipped with an error log, and the job goes on to the later subtrees; a short run of short reads is still retried, not skipped. Regression tests for both cases: |
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
Request changes. The new ten-step skip lets an overtaken block's repair finish after temporary short subtree reads, leaving an off-chain transaction falsely marked mined even when storage recovers. The previous persistent-corruption stall is fixed for an immediately readable later subtree, but the new failure still needs to be resolved.
- P0 — services/blockvalidation/overtaken_tip_repair.go:575 (jobStep): Ten failed steps caused by finite short reads make this branch skip the subtree and mark the block done. A focused repro returned healthy data after 30 short reads, yet the off-chain transaction still had unmined_since=0; subsequent repair calls did not revisit it. After the fork leaves the ten-height restart seed range, that wrong status can survive restart. The same counter carries across subtrees: after nine failures on corrupt subtree A, one three-short-read step on intact subtree B skips B too. Keep ambiguous read failures eligible for later repair while allowing later subtrees to progress, and count failures per subtree. Add a two-subtree regression test that checks the later transaction's status.
…air, count failures per subtree A subtree skipped after its tenth failed load was never read again, so a long run of short reads left its txs looking mined after storage recovered. The skip counted the job's failed steps, so a subtree right after one that was skipped inherited nine failures and was skipped after one more. Failed loads are now counted per subtree. A subtree that still cannot be deserialized after tipRepairMaxAttempts loads is set aside and the job goes on to the later subtrees; when it ends, the block is not done, holds the set-aside subtrees as pending and is deferred, and its next unmark job reads only those. A completed setTxMined write, or the block coming back onto the chain, clears pending so the whole block is repaired next time.
# Conflicts: # CHANGELOG.md
…rejects as a table-lock deadlock
|
Fixed in 8c5f363. Failed loads are counted per subtree now, so a subtree right after a skipped one starts at zero. A subtree that still cannot be deserialized after 10 loads is set aside, not skipped: the job goes on to the later subtrees, and when it ends the block is not done, holds the set-aside subtrees as pending and is deferred behind other blocks. Its next unmark job reads only those subtrees, so they are repaired once reads are whole again. A completed setTxMined write, or the block coming back onto the chain, clears pending, so the whole block is repaired next time. Two-subtree tests that check the later tx's status:
main merged in. |
…a lock error Under SQLite's shared cache a lock cycle between connections fails one side with SQLITE_LOCKED. When that side was a SetMinedMulti chunk, the error was returned and setTxMined failed the whole block, though the spend batch, the rollback and the parent-output read all retry the same error. Each chunk is its own transaction and is rolled back on failure, so it is now tried up to three times while isDeadlock matches, with a short backoff. Drops the retry the blockvalidation repair tests' store wrapper did for this.
…-aside subtrees A block whose subtree stays unreadable was read again as soon as no other block needed the repair, so a corrupt subtree took the repair slot and logged on every pass until the block left the window. The next pass now waits until the best height has moved on 2, 4, 8, ... up to 128 blocks from the start of the last one, about a dozen passes over the window; it still never gives the subtree up. Clearing pending resets the count. Also corrects the jobStep and loadSubtreeNodes comments: only an unmark job comes back to a set-aside subtree, and the remine job leaving one is the safe direction. The recovery test now uses the 30 short reads its description names.
oskarszoon
left a comment
oskarszoon
left a comment
There was a problem hiding this comment.
Request changes. The pending-subtree change fixes the earlier permanent skip for finite short reads, but its height backoff can still leave a recovered subtree's transaction falsely mined until after the default pruning threshold. The new SQL lock retry also misses PostgreSQL statement deadlocks, and its named test passes with the production retry call removed.
- P0 —
services/blockvalidation/overtaken_tip_repair.go:708(finishJob): Repeated short reads can set a subtree aside at best height 256 and postpone its next read until 384. If reads recover at 257, an off-chain child still has nounmined_sincewhen the default 288-height pruning deadline arrives; its fully spent parent can be deleted before repair. This is an incomplete fix for the prior false-mined-status finding, not a new failure first introduced by this latest commit. Bound pending retries by the earliest pruning deadline or protect the affected records while repair waits, and test recovery across that boundary. - P1 —
stores/utxo/sql/sql.go:3636(SetMinedMulti): PostgreSQL deadlocks from a chunk statement or commit are wrapped beforeisDeadlocksees them. The wrapper drops*pgconn.PgError, so the new chunk retry makes one attempt and returns the error. This behavior existed before the retry commit, but the new shared SQL retry still misses this supported backend. Preserve or classify the driver error before wrapping, and test a PostgreSQL40P01chunk failure. - P1 —
stores/utxo/sql/setmined_lock_retry_test.go:21(TestSetMinedMultiRetriesLockError): The test callsretryWriteOnLockErrordirectly. Removing the production retry call fromSetMinedMultileft the named test and all four subtests passing. Add a test that injects a lock error throughStore.SetMinedMultiand fails when that call-site retry is removed.
|
Closes #2051
A 1-back/2-forward reorg cost block assembly 10–16 minutes on the scaling env, with intake stopped the whole time. About half of that was
MarkTransactionsOnLongestChain(false)over the entire mempool: 380M records, 7.7 min on scale-1. The sweep was there for a real reason: a tip overtaken before block assembly moved onto it keeps theunmined_sinceits setTxMined cleared. But sweeping the whole mempool on every reorg was the wrong place to handle it.What changes
1.
reorgBlocksmarks only what the reorg moved (services/blockassembly/subtreeprocessor/SubtreeProcessor.go)checkMarkNotOnLongestChain, the 992 case) now reads only the invalid block's own txs, not the whole mempool.SubtreeProcessor.resetkeeps its whole-assembly sweep. Reset is rare, and the sweep is the catch-all for anything the repair below hasn't reached yet.2. Block validation repairs overtaken tips (
services/blockvalidation/overtaken_tip_repair.go)blockvalidation_tipRepairSliceSizetxs (default 250000). Because no setTxMined write of this process can land inside a step, a tx shared with another block ends correct in either order, with no gates or re-checks around the write. The slice size bounds how long a step delays the next block's mined_set.3.
MarkTransactionsOnLongestChaingets a variadicWithSkipMissingoption (stores/utxo)Fatalf.4.
FilterTxsNotOnLongestChain(model/)BlockIDs/UnminedSincein batches throughBatchDecorateand skips txs that are already unmined.CheckBlockIsInCurrentChaincall, not one per tx.5. Pre-existing test and race fixes, in their own commit
currentBlockHeightin the in-memory blob store.TestIntegrationRetryWithMultipleFailures.TestUnminedSinceReorgScenariosgenerating 1005 blocks in one RPC call, past the 30s timeout on Aerospike.Limitations
GetBestBlockHeaderover gRPC carries no block id. The repair resolves the id withGetBlockHeaderIDsinstead. e2e caught this; a unit test now pins it.Verification
go build ./...,go vet ./...,golangci-lint --new-from-rev=origin/main: clean.-race:model,services/blockvalidation/...,services/blockassembly/...,services/blockpersister/...,stores/utxo/...(Aerospike included),stores/txmetacache,stores/blockchain/...,stores/blob/memory: all pass. The repair tests also ran 10x under-race.donereset, transient vs permanent failures, skipped subtree, gen check, worker-driven path, gRPC id) fails its test when reverted.TestUnminedSinceReorgScenarios: passed 7 of the last 8 runs. The one failure didn't reproduce in 5 reruns; earlier flakes in this suite were Aerospike test-container network errors.Follow-ups