Repository navigation
fix(blockvalidation): an in-flight tagging run no longer overwrites an invalidation's mined_set reset - #2018
Open
freemans13 wants to merge 9 commits into
Open
freemans13 wants to merge 9 commits into
freemans13 wants to merge 9 commits into
Conversation
…n invalidation's mined_set reset setTxMinedStatus reads the block's invalid flag once, tags or untags the block's transactions, then sets mined_set = true. That write was an unconditional UPDATE. If InvalidateBlock or RevalidateBlock ran in between, they had cleared mined_set so the block would be processed again; the overwrite made the setMined worker's MinedSet guard and the mined-not-set sweep both skip it for good. The result was an invalid block with its transactions still tagged, or a revalidated block with none. SetBlockMinedSet now takes the invalid flag the run started from and only writes while the row still has it. A mismatch returns ErrStateError, and setTxMinedStatus re-queues the block and returns nil, so the refusal does not charge the retry counter. The flag travels in a new expectInvalid field on SetBlockMinedSetRequest. The GAP-BV-058 proof test is now an untagged regression test that also asserts the re-queue and the nil return. New store tests cover both directions of the race on sqlite and on postgres, and a client test checks ErrStateError survives the gRPC round trip. Refs bitcoin-sv/teranode#4989 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013BLSCaXeSCCbSqjX4s5KBe
…ions in TestClientSetBlockMinedSet The repo's test convention is testify require, not assert; the two assertions added for the refused-write case now stop the subtest on failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJ2B4FENo6G5s2TDXXvdKs
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-10-29 12:32 UTC |
freemans13
requested review from
icellan and
ordishs
and removed request for
icellan
October 21, 2026 11:47
freemans13
marked this pull request as ready for review
October 21, 2026 11:47
ordishs
approved these changes
Oct 21, 2026
ordishs
left a comment
ordishs
left a comment
Collaborator
There was a problem hiding this comment.
The fix is correct for the stated scope. Making the mined_set write conditional on invalid, plus the re-queue, closes GAP-BV-058. The tests derive from the requirement, and they cover sqlite and postgres.
Two points to address before merge:
- Upgrade order. A pre-upgrade client sends
expectInvalid = false. If it tags an invalid block against a new server, the server refuses withErrStateError. The old client has no re-queue. It counts a retry, and aftersetMinedMaxRetriesit drops the block. Document the upgrade order, or make the fieldoptionaland skip the condition when unset. - CHANGELOG.
AGENTS.mdrequires an## [Unreleased]entry. This PR adds a proto field and a new refusal onSetBlockMinedSet.
Minor: blockchain_reference.md, blockchainProto.md and the block_validation_set_tx_mined.puml diagram still describe an unconditional write.
icellan
requested changes
Oct 21, 2026
icellan
left a comment
icellan
left a comment
Contributor
There was a problem hiding this comment.
The compare-and-set closes the invalidate race but guards the wrong field; details inline.
Also missing: a CHANGELOG.md entry under ## [Unreleased] for the SetBlockMinedSetRequest.expectInvalid proto change and the new ERR_STATE_ERROR response from SetBlockMinedSet, including the mixed-version upgrade behaviour.
…the invalid flag The guard on invalid missed two cases. ClearBlockMinedSet resets mined_set without touching invalid, so a run that started before a catchup reset could still write mined_set=true over it. An invalidate then revalidate inside one run also put invalid back to its starting value, so the stale result landed. blocks gains mined_set_gen (default 0, added on startup). ClearBlockMinedSet, InvalidateBlock and RevalidateBlock bump it. GetBlockHeader returns it, the tagging run captures it before any work, and SetBlockMinedSet only writes while it is unchanged. A moved generation is still ERR_STATE_ERROR, so the run re-queues without charging a retry. The run also re-queues when the invalid flag the worker passed in differs from the one it reads itself, since the generation is captured after that gap. On the zero-rows path the RowsAffected error is now checked, and an unchanged generation returns a storage error, so a real storage fault is charged retries and cannot loop for free. The request field is now optional uint64 mined_set_generation. A request without it, from an older blockvalidation, gets the old unconditional write, so mixed versions keep working. CHANGELOG entry, proto and reference docs and the set-tx-mined diagram are updated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HzSYZhvqpYZPHF3YjMa3F
CHANGELOG.md conflicted only on adjacent entries; both are kept. The new set_tx_mined_revert_test call to SetBlockMinedSet gains the generation argument. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HzSYZhvqpYZPHF3YjMa3F
Contributor
|
🤖 Claude Code Review Status: Complete Current Review:
Still open from earlier passes, with no change requested: re-queues never count against the retry limit. A block whose generation keeps moving is re-tagged once per backoff interval with no upper bound. Only explicit clear, invalidate and revalidate operations move the generation, so this should be bounded in practice. History:
Worth running locally (not run here): |
Collaborator
Author
|
@ordishs, all three points from your review are addressed in e56bd60 (merged with upstream/main as b5eafe7).
Your review approved d37b022. The design changed since then, so a second look would be worth it. |
Collaborator
Author
|
@icellan, both points from your review are addressed in e56bd60 (merged with upstream/main as b5eafe7).
I left the three inline threads open for you to resolve. |
icellan
approved these changes
Oct 27, 2026
icellan
left a comment
icellan
left a comment
Contributor
There was a problem hiding this comment.
The generation counter fixes both lost-update paths, and the optional field keeps mixed-version deploys working. The earlier comments are addressed. Two non-blocking points inline.
The PR description is stale. It still describes the invalid = $2 / bool expectInvalid design and lists the generation counter as not done, so update it before the squash-merge.
CHANGELOG.md conflicted because this branch and upstream each added an entry at the top of the breaking-changes list. Both entries are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HzSYZUnpxNSz4kXw4T5yR
…koff and keeps the retry counter When the mined_set generation moved under a tagging run, setTxMinedStatus put the block straight back on setMinedChan and returned nil. The worker then took nil as success and deleted the block's retry counter, so a block that had used 9 of its 10 attempts on real storage errors started again from 0 after one refusal. The re-queue also had no wait, so a generation that kept moving drove back-to-back full re-tags. setTxMinedStatus now returns a dedicated re-queue error for both re-queue cases (the refused write and the stale invalid flag). The worker leaves the retry counter as it is, neither charged nor reset, and re-queues through the existing backoff path, at least 1 s. The CHANGELOG entry and the ClearBlockMinedSet test comment no longer imply the catchup fork case is closed. Catchup clears mined_set before the reorg, so the re-queued run still tags the block as on the main chain. That ordering predates this branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HzSYZUnpxNSz4kXw4T5yR
Collaborator
Author
|
@icellan, the PR description is rewritten for the generation design as of da5cf29. It now describes the |
CHANGELOG.md conflicted because this branch and upstream (the mining candidate coinbase change) each added an entry at the top of the breaking-changes list. Both entries are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HzSYZUnpxNSz4kXw4T5yR
Collaborator
Author
|
Claude Code Review note on fd94b38: re-queues are not charged against the retry limit. That is deliberate, and it is what @icellan asked for on the retry-counter thread. A re-queue means the block state moved, which is not a failure, so charging it could drop a block as "manual intervention required" for no fault of its own. Only |
Resolves the CHANGELOG.md conflict by keeping both Unreleased entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HzSYWhzVWctuAFYQTLyXn
…n engines The postgres and sqlite schema builders each carried the same 14-line probe-then-ALTER block for mined_set_gen. SonarQube counted it as 34 of the 37 new lines in sql.go and failed the PR on duplication (3.3% against a 3% limit). Both now call addMinedSetGenColumn with their engine's probe query. Behaviour is unchanged: the postgres upgrade test still drops the column, reopens the store and sees it added back with existing rows at 0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HzSYWhzVWctuAFYQTLyXn
|
Collaborator
Author
|
Claude Code Review on cbfd4d5 found no new issues. The one carried-over note, that re-queues do not count against the retry limit, is unchanged and was answered in the earlier reply on fd94b38. cbfd4d5 moves the duplicated mined_set_gen migration into one helper, which clears the SonarQube duplication gate. No behaviour change. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What happened
Block validation tags a block's transactions as mined in the UTXO store, then sets the block's
mined_setflag to say the tagging is done. The function that does this,setTxMinedStatus, decides at the start whether to tag or untag and whether the block is on the main chain. When it finishes, it setmined_set = truewith an unconditionalUPDATE.Three writers clear
mined_setso the block is processed again under its new state:InvalidateBlock,RevalidateBlock, andClearBlockMinedSet, which catchup calls on old-chain blocks when it finds a fork. If one of them ran while a tagging run was in flight, the run's final write overwrote the clear. After that, two things skipped the block for good. One is the setMined worker's guard that ignores blocks already marked mined. The other is the periodic sweep, which only lists blocks withmined_set = false.The result was an invalid block with its transactions still tagged, or a revalidated block with no tags, both carrying
mined_set = true. The spec's proof test reproduces this on main (GAP-BV-058, issue bitcoin-sv/teranode#4989).Changes
mined_setgeneration. The blockchain store adds amined_set_gencolumn toblocks, created on startup with default 0 on Postgres and SQLite.ClearBlockMinedSet,InvalidateBlockandRevalidateBlockeach add one to it when they clearmined_set. A counter is used rather than theinvalidflag becauseClearBlockMinedSetdoes not touchinvalid, and an invalidate followed by a revalidate inside one run would putinvalidback where it started.SetBlockMinedSetwrites only while the generation is unchanged. A tagging run reads the generation fromGetBlockHeaderbefore any work and passes it back. The update addsAND mined_set_gen = $2. If no row matches, the store reads the block again: a missing block is a storage error, a moved generation isErrStateError, and an unchanged generation is also a storage error, so a fault that is not the guard cannot loop forever as a free re-queue.setTxMinedStatusreturns a dedicated re-queue error. The setMined worker neither charges nor resets the block's retry counter on it, and re-queues through the same backoff path a failed run uses (1 s at least). Charging would push a busy block toward the "manual intervention required" drop. Resetting would let one refusal wipe out the attempts a block with real storage errors has used. The backoff stops a generation that keeps moving from driving back-to-back full re-tags of a large block.invalidflag also re-queues. The worker readsinvalidbefore the run reads the generation. An invalidation that lands between those two reads is already counted in the generation, so the guard cannot catch it. The run compares the flag it was given with the one it read and re-queues without tagging if they differ.SetBlockMinedSetRequestgainsoptional uint64 mined_set_generation = 2, andGetBlockHeaderResponsegainsmined_set_generation = 16.optionallets the server tell "not sent" from 0. An absent field means an older caller, and gets the old unconditional write.Testing
TestGapBV058_*tests inservices/blockvalidation/gap_bv058_test.gopark a real tagging run insideSetMinedMultion the sqlitememory store, run the interfering write, then release it. They coverInvalidateBlock,ClearBlockMinedSet, and the stale-flag window. Each asserts thatmined_setstays false, that no tags are left on an invalid block, and that the run returns the re-queue error rather than nil or a plain failure.TestSetMinedChanWorker_RequeueKeepsRetryCounterAndBacksOffdrives the real worker. It seeds a retry counter of 1, forces a re-queue, and checks that the block waits out a backoff, that the counter stays at 1, and that the block is processed again afterwards. Restoring the immediate re-queue that cleared the counter makes it fail.TestSQL_SetBlockMinedSetcovers the normal write, that each of the three clearing writers bumps the generation, refusal after each of them, invalidate-then-revalidate inside one run, an untagging run on an invalid block, the unconditional write, and an unknown block, on sqlite and on a postgres testcontainer.TestSQL_SetBlockMinedSet_UnchangedGenerationIsStorageErrorcovers the unchanged-generation fault.TestBlockchainSchema_AddsMinedSetGenToExistingBlockschecks the column is added to an existing postgres database.TestClientSetBlockMinedSetandTestSetBlockMinedSetcheck thatErrStateErrorsurvives the gRPC wrap and unwrap, that a generation of 0 is still sent as present, and that a present generation reaches the store.go test -race -tags testtxmetacachepasses on the blockvalidation, blockchain store, blockchain service, blockpersister and legacy netsync packages.golangci-lintis clean on the touched packages.Limits and trade-offs
mined_seton old-chain blocks before the reorg happens (catchup.go, step 5.5). The re-queued run therefore still finds the block on the main chain, tags it withonLongestChain = true, and setsmined_set = true, and nothing clears it again after the real reorg. That ordering predates this PR. Closing it needs a clear after the reorg, which is a separate change.ErrStateError, the code the FSM (the node's run-state machine) also uses. Nothing else on theSetBlockMinedSetpath returns it. A dedicated code would mean editingerrors/error.proto.Refs bitcoin-sv/teranode#4989
🤖 Generated with Claude Code
https://claude.ai/code/session_016HzSYZUnpxNSz4kXw4T5yR