Repository navigation
fix(blockvalidation): reject blocks whose coinbase is not final, and stop block assembly mining them - #2017
Conversation
bitcoin-sv applies IsFinalTx to every transaction in a block, coinbase included, and rejects a non-final one as bad-txns-nonfinal. Teranode's only finality check is in the validator, which never sees the coinbase, so a block with a non-final coinbase was accepted here and rejected by bitcoin-sv. Add model.CoinbaseFinalityViolation and apply it in Block.Valid, after bad-cb-length and before the BIP34 height check (bitcoin-sv's order), and on the quick-validation route. A time-based nLockTime is compared to the header time before CSV and to the parent's median time past from CSV on. Without ancestor headers (block assembly, quick validation) the median time past is unknown, so a post-CSV time lock is skipped there, as the median-time-past header rule already is. checkHeaderContextual now returns the median time past it computed, and the write-only Block.medianTimestamp field is removed. Fixes bitcoin-sv/teranode#4981 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ATTxGCcnoi1LqjvX93AD8W
When a solution omits its coinbase, SubmitMiningSolution rebuilds one and copied the solution's time into its nLockTime and its nonce into the input sequence. For any non-zero nonce that coinbase is non-final, which bitcoin-sv rejects as bad-txns-nonfinal. bitcoin-sv puts those values in the header only, and the header here is already built from them, so the copies are removed. A miner-supplied coinbase could still carry a time lock that block.Valid on the submit path cannot evaluate: without ancestor headers it skips a time-based nLockTime from CSV onwards, and a mined block goes from there straight to AddBlock, never through block validation. The submit path now applies CoinbaseFinalityViolation itself. When the verdict depends on the parent's median time past (new CoinbaseFinalityNeedsMedianTimePast), it reads it from the candidate-time memo, or from the blockchain service on a miss, and refuses the solution if it cannot be established rather than add a block peers may reject. CoinbaseFinalityViolation now takes the block timestamp rather than the header, so the submit path can call it before building one. Refs bitcoin-sv/teranode#4981 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HwBYs9tELJrTFomcTUPmp6
… a real store, and log the consensus change (review round 1) The submit-path finality tests answered the median-time-past lookup from blockchain.Mock, so the cold-memo case checked the code against the mock's idea of header order rather than the store's. They now read headers from a sqlitememory blockchain store through a thin counting wrapper: the cold-memo case stores eleven real blocks, the failed-lookup case uses a parent the store does not hold, and the no-lookup cases assert a zero lookup count. Skipping the lookup in submitMiningSolution fails three of the six subtests. CHANGELOG gains an entry under behaviour changes on upgrade: a block with a non-final coinbase is now rejected, and the coinbase rebuilt for a solution without one no longer carries the solution's time and nonce. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-10-28 13:57 UTC |
|
🤖 Claude Code Review Status: Complete Current Review: One design call is still open for the maintainers: whether the finality check belongs in History:
|
|
Claude Code Review on db54c95: no issues, one suggestion, declined. The suggestion was to warn pool operators in the release notes about the coinbase no longer carrying the time and nonce. The CHANGELOG entry added in db54c95 already does that job. It opens with "Affects every miner", and the release notes are cut from CHANGELOG. A pool could not have depended on the old copy in practice, either. The solution time and nonce went into the coinbase after the miner had already hashed the header, so the rebuilt coinbase changed the merkle root under a header the miner had solved for a different one. |
…over real stores, on both entry points The test ran on the catch-up suite's mocked blockchain client and only checked the returned error. It now uses newPreBindHarness, which puts sqlitememory UTXO and blockchain stores behind the ordinary LocalClient, and asserts the end state: a non-final coinbase leaves no block in the blockchain store. It also covers quickValidateBlockAsync, the default catch-up entry point, which the old test missed. Each entry point has a final-coinbase control that must commit, so a rejection cannot come from a broken fixture. With the finality check disabled in checkQuickValidationCoinbase, both rejection cases fail and both controls still pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
The finality rule matches bitcoin-sv IsFinalTx: block time before CSV, parent MTP from CSV onwards, checked before bad-cb-height. The coinbase-less submit path is still broken, see inline.
| // bitcoin-sv's submitminingsolution puts them. They used to be copied into the coinbase as | ||
| // well, as its nLockTime and input sequence, and that made the coinbase non-final for any | ||
| // non-zero nonce: bitcoin-sv rejects such a block as bad-txns-nonfinal | ||
| // (bitcoin-sv/teranode#4981). CreateCoinbaseTxCandidate's coinbase is final as built. The |
There was a problem hiding this comment.
The coinbase-less path is still unminable. CreateCoinbaseTxCandidate (line 1938) builds a fresh coinbase with a random extranonce (rand.Read), so the merkle root can never match the one the miner hashed. Every coinbase-less submit fails the target check, deletes the job and resets BA. The req.Version copy changes the txid as well. The CHANGELOG entry reads as if this path now works. Either refuse coinbase-less solutions, or store the candidate's coinbase on the job and rebuild from it.
There was a problem hiding this comment.
Agreed, and the cause predates this PR: CreateCoinbase appends 12 random extranonce bytes (model/mining_candidate.go:126), so the rebuilt coinbase never matches the one the miner hashed. Dropping the time and nonce copy removed one source of divergence, not the main one. The paths do not agree with each other either: GetCandidateBlock, getminingcandidate with coinbase=true (a p2pk coinbase block assembly never sees) and submit each build their own random coinbase.
We are taking your second option, the bitcoin-sv shape (mining-fbb.cpp keeps the candidate block and falls back to result->GetBlockCoinbase(), with version going to the header only). It needs the job to own the coinbase and the GetMiningCandidate response to carry it, so it goes in its own PR rather than widening this consensus fix. I will link it here when it is open.
For this PR, fa510dd corrects the CHANGELOG entry: it now says a coinbase-less solution is still unminable and that miners must submit the coinbase, which every in-repo miner (mining.Mine, the RPC submit) already does.
There was a problem hiding this comment.
The bitcoin-sv shape is up as #2027 (draft): the job keeps the candidate coinbase, getminingcandidate coinbase=true and GetCandidateBlock return it, and a coinbase-less solution is mined with it in a single submit over a real store.
There was a problem hiding this comment.
Closed by PR 2027, which is now on main and merged into this branch in 70ea819. A coinbase-less solution is mined with the coinbase the job was issued with, and the solution's version goes into the header only. The merge took main's side of that branch of submitMiningSolution, and this PR's CHANGELOG entry no longer says a coinbase-less solution is unminable.
| err error | ||
| ) | ||
|
|
||
| for attempt := 0; attempt < 64; attempt++ { |
There was a problem hiding this comment.
The 64-attempt retry loop with a reset wait of up to 1s is there to work around the random-extranonce rebuild above. It hides the bug instead of pinning it, and the runtime is unpredictable. Make it single-shot once the rebuild is deterministic.
There was a problem hiding this comment.
Agreed that the loop only exists to get past the random rebuild. It stays in this PR because the rebuild is still random here, and fa510dd says so in the test: it becomes a single submit in the follow-up PR that makes the job keep and reuse the candidate coinbase. Its worst case is bounded: regtest takes about half of all hashes, so 64 misses in a row is roughly 2^-64, and each miss waits only for the reset the harness answers at once, never the full 1s timeout in practice.
There was a problem hiding this comment.
Done in 70ea819, now that PR 2027 is on main and merged in. TestSubmitMiningSolution_RecreatedCoinbaseIsFinal is a single submit with no retry loop. It stores the job through storeJobWithCoinbase, so the job holds its issued coinbase. It finds a nonce for that coinbase in advance, starting at 1 so that a nonce copied into the input sequence would show, and submits once. The block must be stored with that nonce in the header and a final coinbase with nLockTime 0 and a final sequence.
…t of the candidate floor, and test it on a real blockchain store ParentMedianTimePast filled a cold lookup through mtpFloor. That placed the submit's parent in the two-slot floor memo, which could evict the entry a candidate path was using, and a failed lookup logged "serving an unfloored candidate time" for what was a refused submit, spending the per-parent log latch so the real candidate failure on that parent then logged nothing. The fetch, verify, walk and median steps now live in parentChainMedianTimePast, which neither logs nor memoizes. computeMtpFloor wraps it with its logging and memo placement as before. ParentMedianTimePast still reads the memo on a hit, and on a miss calls the helper directly and returns the cause to the submit path. The submit-path finality test no longer asserts AddBlock on a mocked blockchain client. The server adds blocks to a sqlitememory store holding a real 149-block chain to the candidate's parent, and each case asserts whether a block was stored on that parent. A new test pins that a cold lookup leaves both memo slots and the log latch untouched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz
…minable The entry read as if dropping the time and nonce copy made the coinbase-less submit path work. It does not: CreateCoinbase gives the rebuilt coinbase a fresh random extranonce, so its merkle root is never the one the miner hashed. The entry now says so and tells miners to submit the coinbase, which every in-repo miner already does. The recreate test's retry loop says the same; it becomes a single submit once the job keeps the candidate's coinbase, as bitcoin-sv does, which is a separate PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz
| // headers (block assembly passes none) the parent's median time past is unknown, so from CSV | ||
| // onwards a time-based nLockTime is not evaluated, as the median-time-past rule above is | ||
| // skipped; the block is checked again with headers when block validation receives it. | ||
| if reason := CoinbaseFinalityViolation(b.CoinbaseTx, b.Header.Timestamp, b.Height, settings.ChainCfgParams, parentMedianTimePast); reason != "" { |
There was a problem hiding this comment.
Coinbase finality only needs the coinbase, height, header time and parent MTP. No subtrees. Yet it runs in ValidWithBinding, not beside CheckHeaderContextual. With optimistic mining on, a block whose coinbase has a non-final sequence and nLockTime >= height passes CheckHeaderContextual, gets added as the tip and is built on, and only the background Valid then invalidates it, which forces a reorg. bitcoin-sv rejects it in ContextualCheckBlock before accepting it. Move the check into the synchronous contextual step.
There was a problem hiding this comment.
One thing to keep in mind if this moves: at the optimistic pre-add point (BlockValidation.go, just before AddBlock), the coinbase is only bound to the header for a subtree-less body (CheckCoinbaseOnlyBodyBound). For a body with subtrees, a finality failure there is a verdict on an unbound coinbase. It should not mark the block invalid or penalize the peer as a header verdict would. The safe shape is probably: on a violation, skip the optimistic add and fall through to the full Valid path, which classifies it with bindErr. The existing coinbase rules (BIP34 height, bad-cb-length) have the same exposure under optimistic mining today, so whatever is chosen could cover them too.
There was a problem hiding this comment.
I have left the check where it is in this PR and flagged the placement to Stu as a design decision. Here is why.
-
The exposure needs an opt-in that is off by default. The pre-
Validadd only happens when bothblockvalidation_optimistic_mining(default true) andblockvalidation_optimistic_mining_peer_blocks(default false,settings/settings.go:416) are set. Catch-up and the legacy route are never optimistic. On a default node every peer block runs the fullValid, finality included, beforeAddBlock. -
Under the opt-in, finality has the same exposure as the coinbase rules next to it:
CoinbaseCommonRuleViolation, bad-cb-length and the BIP34 height check. They all sit after the merkle binding for the reason github-actions gave. For a body with subtrees, the coinbase is not bound to the header untilCheckMerkleRootruns insideValid. A finality verdict at the optimistic pre-add point (right afterCheckHeaderContextualinBlockValidation.go) would be a verdict on a coinbase the header may not commit to. Marking the block invalid or charging the peer there would let anyone who swaps the coinbase poison a good header. bitcoin-sv can reject inContextualCheckBlockbecause itsCheckBlockhas already checked the merkle root by then. -
The safe shape would be: at the pre-add point, run the coinbase rules with the median time past
checkHeaderContextualalready computes. On a violation, skip the optimistic add and fall through to the full path, which classifies the failure withbindErr. No verdict and no penalty at the pre-add point. That should cover all four coinbase rules, not finality alone. The opt-in's own setting docs say to keep it off until theblock.Validintegrity-floor split lands, and that split is where the coinbase rules would naturally move together with the binding. Doing it for one rule here would leave the other three exposed in the same way.
Stu, this one is yours: fall-through for all coinbase rules in a follow-up, or wait for the block.Valid split.
There was a problem hiding this comment.
Deferring this to #2077, which tracks the block.Valid split that moves the integrity floor ahead of the optimistic add. Moving finality alone would not close the gap. For a body with subtrees the coinbase is not yet bound to the header at that point, and the BIP34 height, bad-cb-length and shared coinbase rules have the same exposure. The flag stays off by default, so a default node fully validates, finality included, before a block joins the chain.
|
Merged main in 20825dd. The only conflict was in CHANGELOG.md: this PR's entry and the one from #2037 landed in the same spot, and I kept both. The markdownlint pre-commit hook fails on main as it stands: MD007 ul-indent on the four nested bullets in the #2037 entry ("Before: / Now: / A batch sample's nodes / Blessing a single missing transaction"). They're indented 2 spaces and |
…lookup Two review findings on the submit path, both about blockchain reads a submission could cause before it had shown any work. The median-time-past lookup ran before block.Valid. A miner could send a time-locked coinbase with a non-final sequence and a junk nonce, and every such submit cost a GetBlockHeader, a GetBlockHeaders and, on the fallback walk, up to ten more reads. submitMiningSolution now runs only the lookup-free part of the finality check up front (parent median time past passed as 0, which CoinbaseFinalityViolation skips for a time lock it cannot judge). The lookup and the time-based check run after block.Valid has accepted the proof of work, so an invalid nonce triggers no reads. The height is block.Height, which is set from job.MiningCandidate.Height, so the verdict is unchanged. A finality refusal still keeps the job: the job's template is sound, only the miner's coinbase is wrong, and a corrected coinbase against the same job is valid. ParentMedianTimePast fetched the parent header on every call, though only the hash-keyed fallback walk needs it. In the common case the batched run verifies and the header was thrown away, and a transient GetBlockHeader error could refuse a submit the batched fetch would have handled. parentChainMedianTimePast now fetches the header only when the batched run fails verification and the caller did not supply one. Tests: a new subtest submits a below-target nonce with a time-locked coinbase and asserts zero blockchain reads; moving the lookup back before block.Valid makes it fail. The cold-lookup test now asserts exactly one read; restoring the unconditional header fetch makes it fail with two. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HzSYaAXVb6mbD9mt4iDds
…wn method The previous commit put the median-time-past lookup and the time-based finality check inline in submitMiningSolution, after block.Valid. That pushed the function's cognitive complexity from under 50 to 52, and SonarCloud flagged it (go:S3776). The lookup and the check now live in checkCoinbaseTimeLockFinality, which submitMiningSolution calls at the same point. The behaviour is unchanged: the same condition gates the lookup, the same errors come back, and the finality tests pass unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HzSYaAXVb6mbD9mt4iDds
icellan
left a comment
icellan
left a comment
There was a problem hiding this comment.
Approving at 7d59091. The finality rule matches bitcoin-sv IsFinalTx, and the submit-side median-time-past lookup now runs after the proof-of-work check.
Follow-up, in #2027:
- A submit without a coinbase still can't be mined, because the rebuilt coinbase gets a random extranonce. Until #2027 lands, it would be cleaner to refuse a coinbase-less solution up front than to let it fail proof of work and delete the job.
TestSubmitMiningSolution_RecreatedCoinbaseIsFinalshould become single-shot once the rebuild is deterministic.
@freemans13, a design decision for you: coinbase finality only needs the coinbase, height, header time and parent median time past, but it runs in ValidWithBinding, not beside CheckHeaderContextual. With optimistic mining on for peer blocks, a block with a non-final coinbase is added and built on before the background Valid invalidates it. bitcoin-sv rejects it in ContextualCheckBlock. Moving it into the synchronous contextual step closes that, here or in a follow-up. The open thread on model/Block.go tracks it.
Minor: the finality refusal after the PoW check returns without a log line, unlike the ErrBlockInvalid branch next to it.
PR 2027 landed on main. It makes the job keep the coinbase it was issued with and mine that one for a coinbase-less solution, checks proof of work before block.Valid, and keeps the job when block.Valid finds a consensus fault. One conflict, in services/blockassembly/Server.go, on the coinbase-less branch of submitMiningSolution. This branch still rebuilt the coinbase there, with the solution's version copied in; main parses the job's issued coinbase. Took main's side: it already leaves the solution's time and nonce out of the coinbase, which is what this PR needs. Adjustments the merge needs: - TestSubmitMiningSolution_RecreatedCoinbaseIsFinal is now a single submit. Its 64-attempt retry loop only existed to get past the random extranonce rebuild that PR 2027 removed. The test stores the job through storeJobWithCoinbase, finds a nonce for the issued coinbase in advance, starting at 1 so a nonce copied into the sequence would show, and submits once. - The invalid-nonce subtest now expects main's "does not meet the target" message, from the proof-of-work check main added before block.Valid. The median-time-past lookup still runs after that check. - The CHANGELOG entry for this PR no longer says a coinbase-less solution is unminable, and says the job is kept after a finality refusal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HzSYaAXVb6mbD9mt4iDds
…f-work check checkCoinbaseTimeLockFinality refused a non-final coinbase without a log line, while the block.Valid ErrBlockInvalid branch just before it logs a warning for every consensus refusal. An operator could see the second kind in the log and not the first. The refusal now logs a warning with the reason, the parent's median time past and the coinbase nLockTime. icellan raised this as a minor point in the approving review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HzSYaAXVb6mbD9mt4iDds
|
Thanks @icellan. Where each point in the approving review stands now:
|
The warning and errors from checkCoinbaseTimeLockFinality carried only the job id. The sibling refusals in submitMiningSolution log the job id and the block hash, which is what an operator uses to match a refusal to a solution. The helper now takes the block it is checking and reads the coinbase, height, timestamp and parent hash from it, so the hash comes for free, and the call site passes one argument instead of four. The values are the same ones as before: the block is built from coinbaseTx, nTime, hashPrevBlock and the job's height just above. The Claude Code Review bot suggested this as an optional nit on 1999dc2. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HzSYaAXVb6mbD9mt4iDds
|
What happened
Teranode accepted blocks whose coinbase is not final, and bitcoin-sv rejects them. That is a chain-split risk.
A transaction is final when every input's sequence number is
0xffffffff, or when itsnLockTimeis already in the past. bitcoin-sv applies this rule (IsFinalTx) to every transaction in a block, coinbase included, and rejects a failure asbad-txns-nonfinal. Teranode's only finality check was in the validator, which never sees a coinbase. So nothing checked the coinbase at all.Block Assembly could also build such a coinbase itself. When a miner submits a solution without a coinbase, Block Assembly rebuilds one and copied the solution's time into its
nLockTimeand its nonce into its input sequence. For any non-zero nonce, that coinbase is non-final.Closes bitcoin-sv/teranode#4981.
Changes
Block validation rejects a non-final coinbase.
model.CoinbaseFinalityViolationapplies bitcoin-sv's rule.Block.Validruns it after the coinbase length check and before the BIP34 height check, which is bitcoin-sv's order. The quick-validation route used below checkpoints runs it too. A time-based lock is compared to the block's own timestamp before CSV activation, and from CSV onwards to the parent's median time past (the median timestamp of the 11 blocks ending at the parent).CheckHeaderContextualalready computes that median, so it now returns it, and the write-onlyBlock.medianTimestampfield is gone.Routes without ancestor headers skip only the one case they cannot judge. Quick validation and Block Assembly's own
Block.Validcall have no headers to compute median time past from. From CSV onwards they don't evaluate a time-based lock, just as they already skip the median-time-past header rule. They still enforce the sequence rule, height-based locks, and pre-CSV time locks.The rebuilt coinbase is final.
SubmitMiningSolutionno longer copies the solution's time and nonce into the coinbase. bitcoin-sv'ssubmitminingsolutionputs them in the header only, and Teranode already builds the header from them.Block Assembly checks a miner's own coinbase for finality, median time past included. A mined block goes from
SubmitMiningSolutionstraight toAddBlock, never through block validation. Without this check, the skipped case above would let a miner's time-locked coinbase through, and peers would reject the block. The submit path now runs the finality check itself.model.CoinbaseFinalityNeedsMedianTimePasttells it when the verdict depends on median time past: from CSV onwards, a time-based lock, and a non-final sequence. Only then does it callBlockAssembler.ParentMedianTimePast, which reads the value the candidate-time code (#1466) already caches for the parent, and asks the blockchain service on a cache miss. If the value can't be established, the solution is refused rather than mined. A non-final coinbase is refused as block-invalid, the same classBlock.Validgives it. A failed lookup is a processing error, because the fault is this node's.Testing
model/block_coinbase_finality_test.go:Block.Validon bound and unbound blocks, every lock-time boundary before and after CSV, and the promise thatCoinbaseFinalityNeedsMedianTimePastmakes to callers without headers.services/blockvalidation/quick_validate_coinbase_finality_test.go: quick validation rejects a non-final coinbase as invalid and never commits the block.services/blockassembly/submit_mining_solution_coinbase_finality_test.go: the submit path refuses a time lock at or after median time past and accepts one before it. It fills a cold cache from the blockchain service, refuses when the lookup fails, and makes no lookup when the verdict doesn't depend on it. A rebuilt coinbase comes out final. With theServer.gochange reversed, four of these tests fail.TestGapBV045_NonFinalCoinbaseRejectedfrom docs(spec): services/blockvalidation #1975, passes on this branch with itsbvgapstag removed.-race, 5,077 tests.make lintreports threeerrcheckfailures, all in test files whose lines are the same onmain.Limits
computeMtpFloorlogs in the candidate-time code's wording ("serving an unfloored candidate time"), which reads oddly on the submit path.🤖 Generated with Claude Code
https://claude.ai/code/session_01HwBYs9tELJrTFomcTUPmp6