From e76c1360fad7aab85176810fdfb042d30804771e Mon Sep 17 00:00:00 2001 From: freemans13 Date: Tue, 20 Oct 2026 17:59:14 +0100 Subject: [PATCH 01/10] fix(blockvalidation): reject blocks whose coinbase is not final 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 Claude-Session: https://claude.ai/code/session_01ATTxGCcnoi1LqjvX93AD8W --- model/Block.go | 48 ++- model/block_coinbase_finality.go | 44 +++ model/block_coinbase_finality_test.go | 297 ++++++++++++++++++ services/blockvalidation/quick_validate.go | 11 + .../quick_validate_coinbase_finality_test.go | 37 +++ 5 files changed, 424 insertions(+), 13 deletions(-) create mode 100644 model/block_coinbase_finality.go create mode 100644 model/block_coinbase_finality_test.go create mode 100644 services/blockvalidation/quick_validate_coinbase_finality_test.go diff --git a/model/Block.go b/model/Block.go index bb65f5a25e..67a639802a 100644 --- a/model/Block.go +++ b/model/Block.go @@ -132,8 +132,7 @@ type Block struct { // firstRootMemo caches subtree 0's coinbase-substituted root from the early // binding check, so CheckMerkleRoot does not recompute it for the same // subtree (a full merkle store over up to 1M leaves). - firstRootMemo atomic.Pointer[firstRootMemo] - medianTimestamp uint32 + firstRootMemo atomic.Pointer[firstRootMemo] // nodeAllocator, if non-nil, supplies pooled backing slices for the // per-subtree Node arrays during GetAndValidateSubtrees. Only the // blockvalidation service sets this (via SetNodeAllocator); model-internal @@ -696,14 +695,25 @@ func (b *Block) MedianTimeWindow(currentChain []*BlockHeader) ([]*BlockHeader, e // When currentChain is empty the median-time-past rule is skipped (same as Valid()). Returns nil // when the header passes. func (b *Block) CheckHeaderContextual(currentChain []*BlockHeader, settings *settings.Settings, logger ulogger.Logger) error { + _, err := b.checkHeaderContextual(currentChain, settings, logger) + + return err +} + +// checkHeaderContextual is CheckHeaderContextual, and additionally returns the parent's median time +// past it measured the rule against, or 0 when currentChain is empty and the rule was skipped. Valid +// needs that value again for the coinbase finality check. +func (b *Block) checkHeaderContextual(currentChain []*BlockHeader, settings *settings.Settings, logger ulogger.Logger) (uint32, error) { + var parentMedianTimePast uint32 + // 2. Check that the block timestamp is not more than two hours in the future. twoHoursToTheFutureTimestampUint32, err := safeconversion.Int64ToUint32(time.Now().Add(2 * time.Hour).Unix()) if err != nil { - return errors.NewProcessingError("[BLOCK][%s] failed to convert two hours to the future timestamp to uint32", b.String(), err) + return 0, errors.NewProcessingError("[BLOCK][%s] failed to convert two hours to the future timestamp to uint32", b.String(), err) } if b.Header.Timestamp > twoHoursToTheFutureTimestampUint32 { - return errors.NewBlockInvalidError("[BLOCK][%s] block timestamp is more than two hours in the future", b.String()) + return 0, errors.NewBlockInvalidError("[BLOCK][%s] block timestamp is more than two hours in the future", b.String()) } // 3. Check that the block's timestamp is strictly after the median timestamp of the 11 @@ -713,7 +723,7 @@ func (b *Block) CheckHeaderContextual(currentChain []*BlockHeader, settings *set if len(currentChain) > 0 { lastTimeStamps, err := b.MedianTimeWindow(currentChain) if err != nil { - return err + return 0, err } pruneLength := len(lastTimeStamps) @@ -726,19 +736,19 @@ func (b *Block) CheckHeaderContextual(currentChain []*BlockHeader, settings *set // calculate the median timestamp medianTimestamp, err := CalculateMedianTimestamp(prevTimeStamps) if err != nil { - return err + return 0, err } - b.medianTimestamp, err = safeconversion.Int64ToUint32(medianTimestamp.Unix()) + parentMedianTimePast, err = safeconversion.Int64ToUint32(medianTimestamp.Unix()) if err != nil { - return err + return 0, err } // validate that the block's timestamp is after the median timestamp - if b.Header.Timestamp <= b.medianTimestamp { + if b.Header.Timestamp <= parentMedianTimePast { // if we're not on a chain that allows blocks to be generated quickly then return an error if !settings.ChainCfgParams.GenerateSupported { - return errors.NewBlockInvalidError("block timestamp %d is not after median time past of last %d blocks %d", b.Header.Timestamp, pruneLength, medianTimestamp.Unix()) + return 0, errors.NewBlockInvalidError("block timestamp %d is not after median time past of last %d blocks %d", b.Header.Timestamp, pruneLength, medianTimestamp.Unix()) } // otherwise just warn logger.Warnf("block timestamp %d is not after median time past of last %d blocks %d", b.Header.Timestamp, pruneLength, medianTimestamp.Unix()) @@ -749,10 +759,10 @@ func (b *Block) CheckHeaderContextual(currentChain []*BlockHeader, settings *set // Parity with bitcoin-sv ContextualCheckBlockHeader; must run before body/coinbase checks to // match svnode's bad-version error priority. if err := CheckBlockVersion(b.Header.Version, b.Height, settings.ChainCfgParams); err != nil { - return errors.NewBlockInvalidError("[BLOCK][%s] outdated block version", b.String(), err) + return 0, errors.NewBlockInvalidError("[BLOCK][%s] outdated block version", b.String(), err) } - return nil + return parentMedianTimePast, nil } // Valid runs this function's consensus checks over the block — the header's own proof of work, the @@ -807,7 +817,8 @@ func (b *Block) ValidWithBinding(ctx context.Context, logger ulogger.Logger, sub // must be evaluated at block-receipt time rather than only in the background Valid() goroutine // where time.Now() has drifted past receipt. Factored into CheckHeaderContextual so the // optimistic path can run them synchronously before AddBlock (issue #1149). - if err := b.CheckHeaderContextual(currentChain, settings, logger); err != nil { + parentMedianTimePast, err := b.checkHeaderContextual(currentChain, settings, logger) + if err != nil { return false, false, err } @@ -1023,6 +1034,17 @@ func (b *Block) ValidWithBinding(ctx context.Context, logger ulogger.Logger, sub return false, merkleRootChecked, bindErr("[BLOCK][%s] bad coinbase length", b.String()) } + // 4c. Check that the coinbase is final (bitcoin-sv/teranode#4981). bitcoin-sv's + // ContextualCheckBlock applies IsFinalTx to every transaction, coinbase included, ahead of its + // bad-cb-height check; the coinbase never reaches the validator, so it is checked here. Same + // binding-aware classification as the coinbase checks either side of it. With no ancestor + // 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, b.Height, settings.ChainCfgParams, parentMedianTimePast); reason != "" { + return false, merkleRootChecked, bindErr("[BLOCK][%s] coinbase is not final: %s", b.String(), reason) + } + // BIP34 (https://en.bitcoin.it/wiki/BIP_0034) forces miners to encode the block height in the // coinbase tx; it activates per network at ChainCfgParams.BIP0034Height (skipped below that // height). This check deliberately runs AFTER the merkle binding above (bitcoin-sv/teranode#4692): the coinbase is committed by the merkle root, so only once the body is diff --git a/model/block_coinbase_finality.go b/model/block_coinbase_finality.go new file mode 100644 index 0000000000..920f92a17c --- /dev/null +++ b/model/block_coinbase_finality.go @@ -0,0 +1,44 @@ +package model + +import ( + "github.com/bsv-blockchain/go-bt/v2" + "github.com/bsv-blockchain/go-chaincfg" + "github.com/bsv-blockchain/teranode/util" +) + +// lockTimeThreshold is bitcoin-sv's LOCKTIME_THRESHOLD: an nLockTime below it is a block height, +// at or above it a unix timestamp. +const lockTimeThreshold = 500_000_000 + +// CoinbaseFinalityViolation applies bitcoin-sv's IsFinalTx to a block's coinbase and returns +// bitcoin-sv's reject reason, bad-txns-nonfinal, when the coinbase is not final at height, or "" +// when it is. bitcoin-sv's ContextualCheckBlock applies IsFinalTx to every transaction in the block; +// regular transactions get it from the validator, but the coinbase never goes through the validator, +// so a block with a non-final coinbase was accepted (bitcoin-sv/teranode#4981). +// +// A time-based nLockTime is compared as ContextualCheckBlock does: against the block's own header +// timestamp before CSV activates, against the parent's median time past (BIP113) from CSV onwards. +// parentMedianTimePast is the median time past of the 11 blocks ending at the block's parent, or 0 +// when the caller has no ancestor headers. In that case, from CSV onwards, a time-based nLockTime is +// not evaluated, the same way Valid skips the median-time-past header rule without ancestor headers; +// the sequence-number short-circuit and a height-based nLockTime are still enforced. +// +// Deliberately a reason rather than an error, like CoinbaseCommonRuleViolation: the verdict depends +// on whether the body is bound to its header, which only the caller knows. The caller must guarantee +// a non-nil coinbaseTx with at least one input. +func CoinbaseFinalityViolation(coinbaseTx *bt.Tx, header *BlockHeader, height uint32, params *chaincfg.Params, parentMedianTimePast uint32) string { + lockTimeCutoff := header.Timestamp + if height >= params.CSVHeight { + if parentMedianTimePast == 0 && coinbaseTx.LockTime >= lockTimeThreshold { + return "" + } + + lockTimeCutoff = parentMedianTimePast + } + + if util.IsTransactionFinal(coinbaseTx, height, lockTimeCutoff) != nil { + return "bad-txns-nonfinal" + } + + return "" +} diff --git a/model/block_coinbase_finality_test.go b/model/block_coinbase_finality_test.go new file mode 100644 index 0000000000..e2e34e3362 --- /dev/null +++ b/model/block_coinbase_finality_test.go @@ -0,0 +1,297 @@ +package model + +import ( + "context" + "testing" + + "github.com/bsv-blockchain/go-bt/v2" + "github.com/bsv-blockchain/go-bt/v2/bscript" + "github.com/bsv-blockchain/go-bt/v2/chainhash" + "github.com/bsv-blockchain/go-chaincfg" + txmap "github.com/bsv-blockchain/go-tx-map" + "github.com/bsv-blockchain/teranode/errors" + "github.com/bsv-blockchain/teranode/settings" + blobmemory "github.com/bsv-blockchain/teranode/stores/blob/memory" + "github.com/bsv-blockchain/teranode/ulogger" + "github.com/bsv-blockchain/teranode/util" + "github.com/bsv-blockchain/teranode/util/test" + "github.com/stretchr/testify/require" +) + +const ( + // finalityTestHeight is below regtest's first halving (150), so the 50 BTC output is the full + // subsidy, and below regtest's CSVHeight (576). + finalityTestHeight = uint32(100) + + // regtestGenesisTimestamp is the regtest genesis header's timestamp. With currentChain holding + // only the genesis header, it is also the parent's median time past. + regtestGenesisTimestamp = uint32(1296688602) + + // finalityTestHeaderTime is the header timestamp of the blocks that need the header time and the + // parent's median time past to differ. + finalityTestHeaderTime = regtestGenesisTimestamp + 1000 + + nonFinalSequence = uint32(0) +) + +// finalityCoinbase builds a coinbase that satisfies every rule except possibly finality: one +// consensus-shaped input, a scriptSig that starts with the BIP34 height push, one output of 50 BTC, +// with the given nLockTime and input sequence. +func finalityCoinbase(t *testing.T, height, lockTime, sequence uint32) *bt.Tx { + t.Helper() + + scriptSig := append(util.EncodeCoinbaseHeightPush(height), []byte("/coinbase-finality/")...) + + cb := bt.NewTx() + require.NoError(t, cb.From("0000000000000000000000000000000000000000000000000000000000000000", 0xFFFFFFFF, "", 0)) + cb.Inputs[0].UnlockingScript = bscript.NewFromBytes(scriptSig) + cb.Inputs[0].SequenceNumber = sequence + require.NoError(t, cb.AddP2PKHOutputFromAddress("1DkmRkb5iQFkDu4NBysog5bugnsyx7kwtn", 50*100_000_000)) + cb.LockTime = lockTime + + require.True(t, IsConsensusCoinbase(cb), "fixture precondition: consensus-shaped coinbase") + + return cb +} + +// minedHeaderAt is minedHeaderOnParent with a chosen timestamp. +func minedHeaderAt(t *testing.T, timestamp uint32, merkleRoot, prev *chainhash.Hash) *BlockHeader { + t.Helper() + + nBits, err := NewNBitFromString("207fffff") + require.NoError(t, err) + + hdr := &BlockHeader{ + Version: 4, + HashPrevBlock: prev, + HashMerkleRoot: merkleRoot, + Timestamp: timestamp, + Bits: *nBits, + } + + for { + if ok, _, _ := hdr.HasMetTargetDifficulty(); ok { + return hdr + } + + hdr.Nonce++ + } +} + +// finalityBlock wraps cb in a bound, coinbase-only block on the regtest genesis (header merkle root +// = coinbase txid), so every failure is a verdict on the block hash. +func finalityBlock(t *testing.T, cb *bt.Tx, headerTime uint32) *Block { + t.Helper() + + hdr := minedHeaderAt(t, headerTime, cb.TxIDChainHash(), regtestGenesisHeader(t).Hash()) + + block, err := NewBlock(hdr, cb, []*chainhash.Hash{}, 1, uint64(cb.Size()), finalityTestHeight, 0) //nolint:gosec + require.NoError(t, err) + + return block +} + +// finalitySettings activates BIP34 at height 1 and CSV at csvHeight. +func finalitySettings(t *testing.T, csvHeight uint32) *settings.Settings { + t.Helper() + + params := chaincfg.RegressionNetParams + params.BIP0034Height = 1 + params.CSVHeight = csvHeight + + tSettings := test.CreateBaseTestSettings(t) + tSettings.ChainCfgParams = ¶ms + + return tSettings +} + +func validateFinalityBlock(t *testing.T, block *Block, tSettings *settings.Settings, currentChain []*BlockHeader) (bool, error) { + t.Helper() + + return block.Valid(context.Background(), ulogger.TestLogger{}, blobmemory.New(), createTestUTXOStore(t), + txmap.NewSyncedMap[chainhash.Hash, []uint32](), currentChain, []uint32{0}, tSettings, nil) +} + +func requireBoundNonFinal(t *testing.T, valid bool, err error) { + t.Helper() + + require.Error(t, err, "a non-final coinbase must be rejected (bitcoin-sv bad-txns-nonfinal)") + require.False(t, valid) + require.True(t, errors.Is(err, errors.ErrBlockInvalid), "must be ERR_BLOCK_INVALID on a bound body, got: %v", err) + require.False(t, errors.IsBlockCorrupt(err), "a bound body must not be classified corrupt") + require.Contains(t, err.Error(), "bad-txns-nonfinal") +} + +// TestBlock_Valid_NonFinalCoinbaseRejected is bitcoin-sv/teranode#4981 (GAP-BV-045, BV-BODY-019): +// bitcoin-sv applies IsFinalTx to the coinbase in ContextualCheckBlock, and Teranode's only finality +// check was in the validator, which never sees a coinbase. A bound block at height H whose coinbase +// has nLockTime = H and input sequence 0 is non-final and must be rejected; with sequence 0xFFFFFFFF +// the same coinbase is final. +func TestBlock_Valid_NonFinalCoinbaseRejected(t *testing.T) { + tSettings := finalitySettings(t, chaincfg.RegressionNetParams.CSVHeight) + chain := []*BlockHeader{regtestGenesisHeader(t)} + + t.Run("control: sequence 0xFFFFFFFF makes the coinbase final", func(t *testing.T) { + block := finalityBlock(t, finalityCoinbase(t, finalityTestHeight, finalityTestHeight, 0xFFFFFFFF), regtestGenesisTimestamp) + + valid, err := validateFinalityBlock(t, block, tSettings, chain) + require.NoError(t, err, "fixture must satisfy every other block rule, so the only variable is finality") + require.True(t, valid) + }) + + t.Run("nLockTime == height with sequence 0 is non-final and INVALID", func(t *testing.T) { + block := finalityBlock(t, finalityCoinbase(t, finalityTestHeight, finalityTestHeight, nonFinalSequence), regtestGenesisTimestamp) + + valid, err := validateFinalityBlock(t, block, tSettings, chain) + requireBoundNonFinal(t, valid, err) + }) + + t.Run("nLockTime == height-1 with sequence 0 is final", func(t *testing.T) { + block := finalityBlock(t, finalityCoinbase(t, finalityTestHeight, finalityTestHeight-1, nonFinalSequence), regtestGenesisTimestamp) + + valid, err := validateFinalityBlock(t, block, tSettings, chain) + require.NoError(t, err) + require.True(t, valid) + }) +} + +// TestBlock_Valid_CoinbaseTimeLockCutoff pins which time a time-based coinbase nLockTime is compared +// against: the block's header time before CSV, the parent's median time past from CSV onwards +// (bitcoin-sv ContextualCheckBlock). The header time is 1000s after the parent's median time past, so +// an nLockTime between the two is final before CSV and non-final after it. +func TestBlock_Valid_CoinbaseTimeLockCutoff(t *testing.T) { + chain := []*BlockHeader{regtestGenesisHeader(t)} + between := regtestGenesisTimestamp + 500 + + t.Run("pre-CSV: nLockTime below the header time is final", func(t *testing.T) { + block := finalityBlock(t, finalityCoinbase(t, finalityTestHeight, between, nonFinalSequence), finalityTestHeaderTime) + + valid, err := validateFinalityBlock(t, block, finalitySettings(t, finalityTestHeight+1), chain) + require.NoError(t, err) + require.True(t, valid) + }) + + t.Run("pre-CSV: nLockTime equal to the header time is non-final", func(t *testing.T) { + block := finalityBlock(t, finalityCoinbase(t, finalityTestHeight, finalityTestHeaderTime, nonFinalSequence), finalityTestHeaderTime) + + valid, err := validateFinalityBlock(t, block, finalitySettings(t, finalityTestHeight+1), chain) + requireBoundNonFinal(t, valid, err) + }) + + t.Run("post-CSV: nLockTime between the parent MTP and the header time is non-final", func(t *testing.T) { + block := finalityBlock(t, finalityCoinbase(t, finalityTestHeight, between, nonFinalSequence), finalityTestHeaderTime) + + valid, err := validateFinalityBlock(t, block, finalitySettings(t, finalityTestHeight), chain) + requireBoundNonFinal(t, valid, err) + }) + + t.Run("post-CSV: nLockTime equal to the parent MTP is non-final", func(t *testing.T) { + block := finalityBlock(t, finalityCoinbase(t, finalityTestHeight, regtestGenesisTimestamp, nonFinalSequence), finalityTestHeaderTime) + + valid, err := validateFinalityBlock(t, block, finalitySettings(t, finalityTestHeight), chain) + requireBoundNonFinal(t, valid, err) + }) + + t.Run("post-CSV: nLockTime one below the parent MTP is final", func(t *testing.T) { + block := finalityBlock(t, finalityCoinbase(t, finalityTestHeight, regtestGenesisTimestamp-1, nonFinalSequence), finalityTestHeaderTime) + + valid, err := validateFinalityBlock(t, block, finalitySettings(t, finalityTestHeight), chain) + require.NoError(t, err) + require.True(t, valid) + }) +} + +// TestBlock_Valid_CoinbaseFinalityWithoutChain pins the no-ancestor-headers case (block assembly +// passes none): from CSV onwards the parent's median time past is unknown, so a time-based nLockTime +// is not evaluated, as the median-time-past header rule is skipped. A height-based nLockTime needs no +// chain and is still enforced. +func TestBlock_Valid_CoinbaseFinalityWithoutChain(t *testing.T) { + tSettings := finalitySettings(t, finalityTestHeight) + + t.Run("post-CSV time-based nLockTime is skipped", func(t *testing.T) { + block := finalityBlock(t, finalityCoinbase(t, finalityTestHeight, finalityTestHeaderTime, nonFinalSequence), finalityTestHeaderTime) + + valid, err := validateFinalityBlock(t, block, tSettings, nil) + require.NoError(t, err) + require.True(t, valid) + }) + + t.Run("height-based nLockTime is still enforced", func(t *testing.T) { + block := finalityBlock(t, finalityCoinbase(t, finalityTestHeight, finalityTestHeight, nonFinalSequence), finalityTestHeaderTime) + + valid, err := validateFinalityBlock(t, block, tSettings, nil) + requireBoundNonFinal(t, valid, err) + }) +} + +// TestBlock_Valid_UnboundNonFinalCoinbaseIsCorrupt pins the binding-aware classification: with +// subtrees present but no subtree store, CheckMerkleRoot never runs, so a non-final coinbase on that +// unbound body is corrupt (re-download), never invalid (poisoned). +func TestBlock_Valid_UnboundNonFinalCoinbaseIsCorrupt(t *testing.T) { + tSettings := finalitySettings(t, chaincfg.RegressionNetParams.CSVHeight) + + txHash, err := chainhash.NewHashFromStr("0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206") + require.NoError(t, err) + + coinbase := finalityCoinbase(t, finalityTestHeight, finalityTestHeight, nonFinalSequence) + st, merkleRoot := buildSubtreeAndMerkleRoot(t, coinbase, *txHash) + hdr := minedHeaderVersion(t, 4, merkleRoot) + + block, err := NewBlock(hdr, coinbase, []*chainhash.Hash{st.RootHash()}, 2, 123, finalityTestHeight, 0) + require.NoError(t, err) + + valid, err := block.Valid(context.Background(), ulogger.TestLogger{}, nil, + &panicTxMetaStore{}, txmap.NewSyncedMap[chainhash.Hash, []uint32](), []*BlockHeader{}, []uint32{}, tSettings, nil) + require.False(t, valid) + require.Error(t, err) + require.True(t, errors.IsBlockCorrupt(err), "an unbound non-final coinbase must be corrupt, got: %v", err) + require.False(t, errors.Is(err, errors.ErrBlockInvalid), "an unbound body must never be poisoned") + require.Contains(t, err.Error(), "bad-txns-nonfinal") +} + +func TestCoinbaseFinalityViolation(t *testing.T) { + const ( + height = uint32(100) + headerTime = uint32(1_600_001_000) + mtp = uint32(1_600_000_000) + preCSV = height + 1 + postCSV = height + ) + + header := &BlockHeader{Timestamp: headerTime} + + tests := []struct { + name string + lockTime uint32 + sequence uint32 + csvHeight uint32 + mtp uint32 + want string + }{ + {"nLockTime 0 is final whatever the sequence", 0, nonFinalSequence, postCSV, mtp, ""}, + {"final sequence waives any nLockTime", 0xFFFFFFFF, 0xFFFFFFFF, postCSV, mtp, ""}, + {"height lock below the height is final", height - 1, nonFinalSequence, postCSV, mtp, ""}, + {"height lock equal to the height is non-final", height, nonFinalSequence, postCSV, mtp, "bad-txns-nonfinal"}, + {"height lock above the height is non-final", height + 1, nonFinalSequence, postCSV, mtp, "bad-txns-nonfinal"}, + {"highest height lock is non-final", lockTimeThreshold - 1, nonFinalSequence, postCSV, mtp, "bad-txns-nonfinal"}, + {"pre-CSV: time lock below the header time is final", headerTime - 1, nonFinalSequence, preCSV, mtp, ""}, + {"pre-CSV: time lock equal to the header time is non-final", headerTime, nonFinalSequence, preCSV, mtp, "bad-txns-nonfinal"}, + {"pre-CSV: header time applies even without a parent MTP", headerTime, nonFinalSequence, preCSV, 0, "bad-txns-nonfinal"}, + {"post-CSV: time lock below the parent MTP is final", mtp - 1, nonFinalSequence, postCSV, mtp, ""}, + {"post-CSV: time lock equal to the parent MTP is non-final", mtp, nonFinalSequence, postCSV, mtp, "bad-txns-nonfinal"}, + {"post-CSV: time lock between MTP and header time is non-final", mtp + 500, nonFinalSequence, postCSV, mtp, "bad-txns-nonfinal"}, + {"post-CSV: lowest time lock is final", lockTimeThreshold, nonFinalSequence, postCSV, mtp, ""}, + {"post-CSV: time lock without a parent MTP is not evaluated", headerTime, nonFinalSequence, postCSV, 0, ""}, + {"post-CSV: height lock without a parent MTP is still enforced", height, nonFinalSequence, postCSV, 0, "bad-txns-nonfinal"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + params := chaincfg.RegressionNetParams + params.CSVHeight = tt.csvHeight + + cb := finalityCoinbase(t, height, tt.lockTime, tt.sequence) + require.Equal(t, tt.want, CoinbaseFinalityViolation(cb, header, height, ¶ms, tt.mtp)) + }) + } +} diff --git a/services/blockvalidation/quick_validate.go b/services/blockvalidation/quick_validate.go index 5209c334ce..e6adc5cfda 100644 --- a/services/blockvalidation/quick_validate.go +++ b/services/blockvalidation/quick_validate.go @@ -518,6 +518,13 @@ func (u *BlockValidation) quickValidateBlockAsync(ctx context.Context, block *mo // This is defence-in-depth, not a commonly-reachable path: quick validation only runs for blocks at // or below the highest hash-verified checkpoint for this catchup run (catchup.go, // tryQuickValidation). +// +// It also enforces model.CoinbaseFinalityViolation (bitcoin-sv/teranode#4981), after bad-cb-length as +// in Valid. This route has no ancestor headers to compute the parent's median time past from, so +// from CSV onwards a time-based coinbase nLockTime is not evaluated; the sequence-number rule, a +// height-based nLockTime, and a pre-CSV time-based nLockTime (measured against the block's own +// header time) are. The skipped case is a block below a hash-verified checkpoint, which the network +// already accepted. func (u *BlockValidation) checkQuickValidationCoinbase(block *model.Block, caller string) error { if reason := model.CoinbaseCommonRuleViolation(block.CoinbaseTx, block.Height, u.settings.ChainCfgParams); reason != "" { return errors.NewBlockInvalidError("[%s][%s] coinbase breaks a transaction rule: %s", caller, block.Hash().String(), reason) @@ -527,6 +534,10 @@ func (u *BlockValidation) checkQuickValidationCoinbase(block *model.Block, calle return errors.NewBlockInvalidError("[%s][%s] bad coinbase length", caller, block.Hash().String()) } + if reason := model.CoinbaseFinalityViolation(block.CoinbaseTx, block.Header, block.Height, u.settings.ChainCfgParams, 0); reason != "" { + return errors.NewBlockInvalidError("[%s][%s] coinbase is not final: %s", caller, block.Hash().String(), reason) + } + return nil } diff --git a/services/blockvalidation/quick_validate_coinbase_finality_test.go b/services/blockvalidation/quick_validate_coinbase_finality_test.go new file mode 100644 index 0000000000..ec00f509fc --- /dev/null +++ b/services/blockvalidation/quick_validate_coinbase_finality_test.go @@ -0,0 +1,37 @@ +package blockvalidation + +import ( + "testing" + + "github.com/bsv-blockchain/teranode/errors" + "github.com/bsv-blockchain/teranode/services/blockvalidation/testhelpers" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// TestQuickValidateBlock_NonFinalCoinbase pins bitcoin-sv/teranode#4981 on the quick-validation +// path, which never calls block.Valid: checkQuickValidationCoinbase applies +// model.CoinbaseFinalityViolation. A coinbase whose height-based nLockTime equals the block height, +// with a non-final input sequence, is non-final; the body is bound by the coinbase-only rule, so the +// verdict is invalid and the block is never committed. +func TestQuickValidateBlock_NonFinalCoinbase(t *testing.T) { + suite := NewCatchupTestSuite(t) + defer suite.Cleanup() + + suite.MockBlockchain.On("AssignBlockID", mock.Anything, mock.Anything).Return(uint64(1), nil).Maybe() + suite.MockBlockchain.On("AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + + block := testhelpers.CreateTestBlocks(t, 1)[0] + block.Height = 100 + block.CoinbaseTx.LockTime = block.Height + block.CoinbaseTx.Inputs[0].SequenceNumber = 0 + block.Header.HashMerkleRoot = block.CoinbaseTx.TxIDChainHash() + + err := suite.Server.blockValidation.quickValidateBlock(suite.Ctx, block, "test", "") + require.Error(t, err) + require.True(t, errors.Is(err, errors.ErrBlockInvalid), "a bound non-final coinbase must condemn invalid, got: %v", err) + require.False(t, errors.IsBlockCorrupt(err), "must NOT be corrupt") + require.Contains(t, err.Error(), "bad-txns-nonfinal") + + suite.MockBlockchain.AssertNotCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} From cc436269f0278d98d67c52af0a6a5cceaeb773ba Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 21 Oct 2026 11:50:23 +0100 Subject: [PATCH 02/10] fix(blockassembly): never mine a block whose coinbase is not final 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 Claude-Session: https://claude.ai/code/session_01HwBYs9tELJrTFomcTUPmp6 --- model/Block.go | 2 +- model/block_coinbase_finality.go | 46 +++- model/block_coinbase_finality_test.go | 39 ++- services/blockassembly/Server.go | 35 ++- services/blockassembly/candidate_time.go | 27 ++ ..._mining_solution_coinbase_finality_test.go | 232 ++++++++++++++++++ services/blockvalidation/quick_validate.go | 2 +- 7 files changed, 357 insertions(+), 26 deletions(-) create mode 100644 services/blockassembly/submit_mining_solution_coinbase_finality_test.go diff --git a/model/Block.go b/model/Block.go index 67a639802a..a61d706644 100644 --- a/model/Block.go +++ b/model/Block.go @@ -1041,7 +1041,7 @@ func (b *Block) ValidWithBinding(ctx context.Context, logger ulogger.Logger, sub // 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, b.Height, settings.ChainCfgParams, parentMedianTimePast); reason != "" { + if reason := CoinbaseFinalityViolation(b.CoinbaseTx, b.Header.Timestamp, b.Height, settings.ChainCfgParams, parentMedianTimePast); reason != "" { return false, merkleRootChecked, bindErr("[BLOCK][%s] coinbase is not final: %s", b.String(), reason) } diff --git a/model/block_coinbase_finality.go b/model/block_coinbase_finality.go index 920f92a17c..cef4a48943 100644 --- a/model/block_coinbase_finality.go +++ b/model/block_coinbase_finality.go @@ -11,25 +11,26 @@ import ( const lockTimeThreshold = 500_000_000 // CoinbaseFinalityViolation applies bitcoin-sv's IsFinalTx to a block's coinbase and returns -// bitcoin-sv's reject reason, bad-txns-nonfinal, when the coinbase is not final at height, or "" -// when it is. bitcoin-sv's ContextualCheckBlock applies IsFinalTx to every transaction in the block; -// regular transactions get it from the validator, but the coinbase never goes through the validator, -// so a block with a non-final coinbase was accepted (bitcoin-sv/teranode#4981). +// bitcoin-sv's reject reason, bad-txns-nonfinal, when the coinbase is not final at height in a block +// stamped blockTime, or "" when it is. bitcoin-sv's ContextualCheckBlock applies IsFinalTx to every +// transaction in the block; regular transactions get it from the validator, but the coinbase never +// goes through the validator, so a block with a non-final coinbase was accepted +// (bitcoin-sv/teranode#4981). // -// A time-based nLockTime is compared as ContextualCheckBlock does: against the block's own header -// timestamp before CSV activates, against the parent's median time past (BIP113) from CSV onwards. -// parentMedianTimePast is the median time past of the 11 blocks ending at the block's parent, or 0 -// when the caller has no ancestor headers. In that case, from CSV onwards, a time-based nLockTime is -// not evaluated, the same way Valid skips the median-time-past header rule without ancestor headers; -// the sequence-number short-circuit and a height-based nLockTime are still enforced. +// A time-based nLockTime is compared as ContextualCheckBlock does: against blockTime before CSV +// activates, against the parent's median time past (BIP113) from CSV onwards. parentMedianTimePast +// is the median time past of the 11 blocks ending at the block's parent, or 0 when the caller has no +// ancestor headers. In that case, when the verdict depends on it (CoinbaseFinalityNeedsMedianTimePast), +// the coinbase is passed, the same way Valid skips the median-time-past header rule without ancestor +// headers; the sequence-number short-circuit and a height-based nLockTime are still enforced. // // Deliberately a reason rather than an error, like CoinbaseCommonRuleViolation: the verdict depends // on whether the body is bound to its header, which only the caller knows. The caller must guarantee // a non-nil coinbaseTx with at least one input. -func CoinbaseFinalityViolation(coinbaseTx *bt.Tx, header *BlockHeader, height uint32, params *chaincfg.Params, parentMedianTimePast uint32) string { - lockTimeCutoff := header.Timestamp +func CoinbaseFinalityViolation(coinbaseTx *bt.Tx, blockTime uint32, height uint32, params *chaincfg.Params, parentMedianTimePast uint32) string { + lockTimeCutoff := blockTime if height >= params.CSVHeight { - if parentMedianTimePast == 0 && coinbaseTx.LockTime >= lockTimeThreshold { + if parentMedianTimePast == 0 && CoinbaseFinalityNeedsMedianTimePast(coinbaseTx, height, params) { return "" } @@ -42,3 +43,22 @@ func CoinbaseFinalityViolation(coinbaseTx *bt.Tx, header *BlockHeader, height ui return "" } + +// CoinbaseFinalityNeedsMedianTimePast reports whether CoinbaseFinalityViolation's verdict on +// coinbaseTx at height depends on the parent's median time past. That is only so from CSV onwards, +// for a time-based nLockTime, and when some input's sequence number is not final; in every other +// case the verdict is the same whatever parentMedianTimePast is passed, including 0. A caller that +// has no ancestor headers to hand uses this to fetch the median time past only when it matters. +func CoinbaseFinalityNeedsMedianTimePast(coinbaseTx *bt.Tx, height uint32, params *chaincfg.Params) bool { + if height < params.CSVHeight || coinbaseTx.LockTime < lockTimeThreshold { + return false + } + + for _, input := range coinbaseTx.Inputs { + if input.SequenceNumber != bt.DefaultSequenceNumber { + return true + } + } + + return false +} diff --git a/model/block_coinbase_finality_test.go b/model/block_coinbase_finality_test.go index e2e34e3362..039191cb47 100644 --- a/model/block_coinbase_finality_test.go +++ b/model/block_coinbase_finality_test.go @@ -258,8 +258,6 @@ func TestCoinbaseFinalityViolation(t *testing.T) { postCSV = height ) - header := &BlockHeader{Timestamp: headerTime} - tests := []struct { name string lockTime uint32 @@ -291,7 +289,42 @@ func TestCoinbaseFinalityViolation(t *testing.T) { params.CSVHeight = tt.csvHeight cb := finalityCoinbase(t, height, tt.lockTime, tt.sequence) - require.Equal(t, tt.want, CoinbaseFinalityViolation(cb, header, height, ¶ms, tt.mtp)) + require.Equal(t, tt.want, CoinbaseFinalityViolation(cb, headerTime, height, ¶ms, tt.mtp)) + + // The promise CoinbaseFinalityNeedsMedianTimePast makes to a caller without headers: + // when it says the median time past does not matter, leaving it unknown gives the same + // verdict. + if !CoinbaseFinalityNeedsMedianTimePast(cb, height, ¶ms) { + require.Equal(t, tt.want, CoinbaseFinalityViolation(cb, headerTime, height, ¶ms, 0)) + } + }) + } +} + +func TestCoinbaseFinalityNeedsMedianTimePast(t *testing.T) { + const height = uint32(100) + + tests := []struct { + name string + lockTime uint32 + sequence uint32 + csvHeight uint32 + want bool + }{ + {"post-CSV time lock with a non-final sequence needs it", lockTimeThreshold, nonFinalSequence, height, true}, + {"pre-CSV time lock is measured against the header time", lockTimeThreshold, nonFinalSequence, height + 1, false}, + {"height lock never needs it", lockTimeThreshold - 1, nonFinalSequence, height, false}, + {"nLockTime 0 never needs it", 0, nonFinalSequence, height, false}, + {"final sequence waives the time lock", lockTimeThreshold, 0xFFFFFFFF, height, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + params := chaincfg.RegressionNetParams + params.CSVHeight = tt.csvHeight + + cb := finalityCoinbase(t, height, tt.lockTime, tt.sequence) + require.Equal(t, tt.want, CoinbaseFinalityNeedsMedianTimePast(cb, height, ¶ms)) }) } } diff --git a/services/blockassembly/Server.go b/services/blockassembly/Server.go index 4189ef878c..d59918f9e8 100644 --- a/services/blockassembly/Server.go +++ b/services/blockassembly/Server.go @@ -1940,18 +1940,17 @@ func (ba *BlockAssembly) submitMiningSolution(ctx context.Context, req *BlockSub return nil, errors.NewProcessingError("[BlockAssembly][%s] failed to create coinbase tx", jobID, err) } - // set the new mining parameters on the coinbase (nonce) if req.Version != nil { coinbaseTx.Version = *req.Version } - if req.Time != nil { - coinbaseTx.LockTime = *req.Time - } - - if req.Nonce != 0 { - coinbaseTx.Inputs[0].SequenceNumber = req.Nonce - } + // The solution's time and nonce belong in the header, which is built from req below, as + // 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 + // version copy above has no bitcoin-sv counterpart either, but it does not affect + // finality and is left as it was. } // Final validation: ensure coinbase is valid (defense-in-depth) @@ -1959,6 +1958,26 @@ func (ba *BlockAssembly) submitMiningSolution(ctx context.Context, req *BlockSub return nil, err } + // The coinbase must be final (bitcoin-sv/teranode#4981). block.Valid below applies the rule too, + // but with no ancestor headers it cannot evaluate a time-based nLockTime from CSV onwards, and + // this block goes from here straight to AddBlock, never through block validation. So when the + // verdict depends on the parent's median time past, fetch it, and refuse the solution if it + // cannot be established rather than add a block every peer may reject. + var parentMedianTimePast uint32 + + if model.CoinbaseFinalityNeedsMedianTimePast(coinbaseTx, job.MiningCandidate.Height, ba.blockAssembler.settings.ChainCfgParams) { + parentMedianTimePast, err = ba.blockAssembler.ParentMedianTimePast(ctx, hashPrevBlock) + if err != nil { + return nil, errors.NewProcessingError("[BlockAssembly][%s] cannot check that the coinbase is final: its time-based nLockTime needs the parent's median time past", jobID, err) + } + } + + if reason := model.CoinbaseFinalityViolation(coinbaseTx, nTime, job.MiningCandidate.Height, ba.blockAssembler.settings.ChainCfgParams, parentMedianTimePast); reason != "" { + // A consensus verdict on the miner's coinbase, so the same class block.Valid gives it. The + // lookup failure above is this node's problem and stays a processing error. + return nil, errors.NewBlockInvalidError("[BlockAssembly][%s] coinbase is not final: %s", jobID, reason) + } + coinbaseTxIDHash := coinbaseTx.TxIDChainHash() var sizeInBytes uint64 diff --git a/services/blockassembly/candidate_time.go b/services/blockassembly/candidate_time.go index bf44db4c5e..d36d754605 100644 --- a/services/blockassembly/candidate_time.go +++ b/services/blockassembly/candidate_time.go @@ -6,6 +6,7 @@ import ( "time" "github.com/bsv-blockchain/go-bt/v2/chainhash" + safeconversion "github.com/bsv-blockchain/go-safe-conversion" "github.com/bsv-blockchain/teranode/errors" "github.com/bsv-blockchain/teranode/model" "github.com/bsv-blockchain/teranode/services/blockchain" @@ -173,6 +174,32 @@ func (b *BlockAssembler) MinCandidateTime(parentHash *chainhash.Hash) (int64, bo return 0, false } +// ParentMedianTimePast returns the median time past of the 11 headers ending at parentHash, which +// is what bitcoin-sv measures a time-based coinbase nLockTime against from CSV onwards. The submit +// path uses it for the coinbase finality check (bitcoin-sv/teranode#4981). +// +// Unlike MinCandidateTime it neither stops at a memo miss nor defers to GenerateSupported: finality +// is a consensus rule on every chain, so a miss goes to the blockchain service, and a failed lookup +// is an error the caller refuses on rather than a reason to skip the rule. A failed lookup is also +// logged by computeMtpFloor in candidateTime's wording, which names an unfloored candidate. +func (b *BlockAssembler) ParentMedianTimePast(ctx context.Context, parentHash *chainhash.Hash) (uint32, error) { + entry := b.memoizedMtpFloor(parentHash) + if entry == nil { + // mtpFloor reads the header only for its hash-keyed fallback walk, which refuses a nil one. + parentHeader, _, err := b.blockchainClient.GetBlockHeader(ctx, parentHash) + if err != nil { + return 0, errors.NewProcessingError("failed to fetch parent header %s", parentHash.String(), err) + } + + if entry = b.mtpFloor(ctx, parentHeader, parentHash); entry == nil { + return 0, errors.NewProcessingError("median time past of parent %s could not be established", parentHash.String()) + } + } + + // The memo holds the floor, median time past plus one. + return safeconversion.Int64ToUint32(entry.minTime - 1) +} + // mtpFloor returns the memoized median-time-past floor for parentHash, // computing it on a miss. It returns nil — having logged the cause — when the // floor cannot be established, leaving the caller to decide how to degrade. diff --git a/services/blockassembly/submit_mining_solution_coinbase_finality_test.go b/services/blockassembly/submit_mining_solution_coinbase_finality_test.go new file mode 100644 index 0000000000..85d6e0a141 --- /dev/null +++ b/services/blockassembly/submit_mining_solution_coinbase_finality_test.go @@ -0,0 +1,232 @@ +package blockassembly + +import ( + "context" + "testing" + "time" + + "github.com/bsv-blockchain/go-bt/v2" + "github.com/bsv-blockchain/go-bt/v2/chainhash" + "github.com/bsv-blockchain/go-chaincfg" + "github.com/bsv-blockchain/teranode/errors" + "github.com/bsv-blockchain/teranode/model" + "github.com/bsv-blockchain/teranode/services/blockassembly/blockassembly_api" + "github.com/bsv-blockchain/teranode/util" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// finalityMTPLag is how far the parent's median time past sits below the candidate time in these +// tests: a time lock between the two is final by the header time and non-final by the median time +// past, which is the comparison bitcoin-sv makes from CSV onwards. +const finalityMTPLag = uint32(600) + +// newCoinbaseFinalityHarness is the coinbase-height harness with CSV active at the candidate height, +// so a time-based coinbase nLockTime is measured against the parent's median time past, and with the +// assembler wired to the harness's blockchain mock for the median-time-past lookup. +func newCoinbaseFinalityHarness(t *testing.T) *coinbaseHeightHarness { + t.Helper() + + params := chaincfg.RegressionNetParams + params.CSVHeight = 1 + + h := newCoinbaseHeightHarness(t, ¶ms, postGenesisCandidateHeight, 5_000_000) + h.server.blockAssembler.blockchainClient = h.blockchai + + return h +} + +// parentMTP is the median time past the tests give the candidate's parent. +func (h *coinbaseHeightHarness) parentMTP() uint32 { + return h.candidate.Time - finalityMTPLag +} + +// warmMTP puts the parent's median time past in the assembler's memo, as a candidate built on that +// parent leaves it. +func (h *coinbaseHeightHarness) warmMTP(t *testing.T) { + t.Helper() + + parent, err := chainhash.NewHash(h.candidate.PreviousHash) + require.NoError(t, err) + + h.server.blockAssembler.placeMtpFloor(&mtpFloorEntry{parent: *parent, minTime: int64(h.parentMTP()) + 1}) +} + +// timeLockedCoinbase is a pool coinbase whose time-based nLockTime is enforced: its input sequence is +// not final. +func timeLockedCoinbase(t *testing.T, lockTime uint32) *bt.Tx { + t.Helper() + + coinbase := coinbaseEncoding(t, postGenesisCandidateHeight, 5_000_000) + coinbase.LockTime = lockTime + coinbase.Inputs[0].SequenceNumber = 0 + + return coinbase +} + +// TestSubmitMiningSolution_CoinbaseMustBeFinal pins bitcoin-sv's IsFinalTx rule on the coinbase of a +// block this node mines (bitcoin-sv/teranode#4981). block.Valid on the submit path has no ancestor +// headers, so from CSV onwards it cannot evaluate a time-based nLockTime, and the block goes from the +// submit path straight to AddBlock. The submit path therefore fetches the parent's median time past +// itself, but only when the coinbase's verdict depends on it. +func TestSubmitMiningSolution_CoinbaseMustBeFinal(t *testing.T) { + t.Run("a time lock at or after the parent's median time past is refused", func(t *testing.T) { + h := newCoinbaseFinalityHarness(t) + h.warmMTP(t) + + // Final by the header time, which is what block.Valid would fall back to without headers + // before CSV, and non-final by the median time past bitcoin-sv uses after it. + resp, err := h.submit(t, timeLockedCoinbase(t, h.parentMTP())) + require.Error(t, err) + require.Nil(t, resp) + require.Contains(t, err.Error(), "bad-txns-nonfinal") + h.blockchai.AssertNotCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + + // The pool may submit again with a final coinbase, so the job stays and assembly is not + // reset: nothing local is at fault. + require.NotNil(t, h.server.jobStore.Get(h.id), "the job must survive a refused coinbase") + h.requireNoReset(t) + }) + + t.Run("a time lock before the parent's median time past is accepted", func(t *testing.T) { + h := newCoinbaseFinalityHarness(t) + h.warmMTP(t) + + resp, err := h.submit(t, timeLockedCoinbase(t, h.parentMTP()-1)) + require.NoError(t, err) + require.True(t, resp.Ok) + h.blockchai.AssertCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + h.blockchai.AssertNotCalled(t, "GetBlockHeader", mock.Anything, mock.Anything) + }) + + t.Run("a cold memo is filled from the blockchain service", func(t *testing.T) { + h := newCoinbaseFinalityHarness(t) + + // A chain of eleven headers from the chain start, the parent last, one block every ten + // minutes ending ten minutes before the candidate time. Their median is the sixth, sixty + // minutes before the candidate time. + headers := make([]*model.BlockHeader, 11) + prev := &chainhash.Hash{} + + for i := range headers { + headers[i] = &model.BlockHeader{ + Version: 1, + HashPrevBlock: prev, + HashMerkleRoot: &chainhash.Hash{byte(i)}, + Timestamp: h.candidate.Time - uint32(len(headers)-i)*600, + Bits: h.nBits, + } + prev = headers[i].Hash() + } + + parent := headers[len(headers)-1] + h.candidate.PreviousHash = parent.Hash().CloneBytes() + + // GetBlockHeaders answers newest first. + newestFirst := make([]*model.BlockHeader, len(headers)) + for i, header := range headers { + newestFirst[len(headers)-1-i] = header + } + + h.blockchai.On("GetBlockHeader", mock.Anything, parent.Hash()).Return(parent, &model.BlockHeaderMeta{}, nil) + h.blockchai.On("GetBlockHeaders", mock.Anything, parent.Hash(), mock.Anything).Return(newestFirst, []*model.BlockHeaderMeta{}, nil) + + mtp := h.candidate.Time - 3600 + + resp, err := h.submit(t, timeLockedCoinbase(t, mtp)) + require.Error(t, err) + require.Nil(t, resp) + require.Contains(t, err.Error(), "bad-txns-nonfinal") + + resp, err = h.submit(t, timeLockedCoinbase(t, mtp-1)) + require.NoError(t, err) + require.True(t, resp.Ok) + }) + + t.Run("an unknown median time past refuses the solution", func(t *testing.T) { + h := newCoinbaseFinalityHarness(t) + h.blockchai.On("GetBlockHeader", mock.Anything, mock.Anything).Return(nil, nil, errors.NewServiceUnavailableError("blockchain down")) + + resp, err := h.submit(t, timeLockedCoinbase(t, h.parentMTP()-1)) + require.Error(t, err) + require.Nil(t, resp) + require.Contains(t, err.Error(), "cannot check that the coinbase is final") + h.blockchai.AssertNotCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + h.requireNoReset(t) + }) + + t.Run("a coinbase whose verdict does not depend on the median time past needs no lookup", func(t *testing.T) { + h := newCoinbaseFinalityHarness(t) + + // Cold memo and no GetBlockHeader expectation: a lookup would fail the mock. + coinbase := timeLockedCoinbase(t, h.candidate.Time) + coinbase.Inputs[0].SequenceNumber = bt.DefaultSequenceNumber + + resp, err := h.submit(t, coinbase) + require.NoError(t, err) + require.True(t, resp.Ok) + h.blockchai.AssertNotCalled(t, "GetBlockHeader", mock.Anything, mock.Anything) + }) + + t.Run("a height lock is refused without a lookup", func(t *testing.T) { + h := newCoinbaseFinalityHarness(t) + + resp, err := h.submit(t, timeLockedCoinbase(t, postGenesisCandidateHeight)) + require.Error(t, err) + require.Nil(t, resp) + require.Contains(t, err.Error(), "bad-txns-nonfinal") + h.blockchai.AssertNotCalled(t, "GetBlockHeader", mock.Anything, mock.Anything) + }) +} + +// TestSubmitMiningSolution_RecreatedCoinbaseIsFinal pins the coinbase block assembly builds when a +// solution omits one. The solution's time and nonce used to be copied into that coinbase as its +// nLockTime and input sequence, which made it non-final for any non-zero nonce +// (bitcoin-sv/teranode#4981). They belong in the header only. +func TestSubmitMiningSolution_RecreatedCoinbaseIsFinal(t *testing.T) { + h := newCoinbaseFinalityHarness(t) + h.warmMTP(t) + + nTime := h.candidate.Time + + // The recreated coinbase carries a fresh random extranonce, so the merkle root, and with it + // whether a given nonce meets the target, cannot be known in advance. Regtest's target takes + // about half of all hashes, so resubmit until one does; a miss is a proof-of-work rejection, + // which deletes the job and resets assembly, so put the job back each time. + var ( + resp *blockassembly_api.OKResponse + err error + ) + + for attempt := 0; attempt < 64; attempt++ { + h.server.jobStore.Set(h.id, h.job, time.Minute) + + resp, err = h.server.submitMiningSolution(context.Background(), &BlockSubmissionRequest{ + SubmitMiningSolutionRequest: &blockassembly_api.SubmitMiningSolutionRequest{ + Id: h.idBytes, + Nonce: 7, + Time: &nTime, + }, + }) + if err == nil { + break + } + + require.Contains(t, err.Error(), "target difficulty", "the only acceptable failure is a nonce that misses the target") + + select { + case <-h.resets: + case <-time.After(time.Second): + } + } + + require.NoError(t, err) + require.True(t, resp.Ok) + + block := h.addedBlock(t) + require.Equal(t, uint32(7), block.Header.Nonce, "the nonce belongs in the header") + require.Equal(t, nTime, block.Header.Timestamp, "the time belongs in the header") + require.Zero(t, block.CoinbaseTx.LockTime, "the time must not be copied into the coinbase nLockTime") + require.Equal(t, bt.DefaultSequenceNumber, block.CoinbaseTx.Inputs[0].SequenceNumber, "the nonce must not be copied into the coinbase sequence") + require.NoError(t, util.IsTransactionFinal(block.CoinbaseTx, block.Height, h.parentMTP())) +} diff --git a/services/blockvalidation/quick_validate.go b/services/blockvalidation/quick_validate.go index e6adc5cfda..a71ddaf735 100644 --- a/services/blockvalidation/quick_validate.go +++ b/services/blockvalidation/quick_validate.go @@ -534,7 +534,7 @@ func (u *BlockValidation) checkQuickValidationCoinbase(block *model.Block, calle return errors.NewBlockInvalidError("[%s][%s] bad coinbase length", caller, block.Hash().String()) } - if reason := model.CoinbaseFinalityViolation(block.CoinbaseTx, block.Header, block.Height, u.settings.ChainCfgParams, 0); reason != "" { + if reason := model.CoinbaseFinalityViolation(block.CoinbaseTx, block.Header.Timestamp, block.Height, u.settings.ChainCfgParams, 0); reason != "" { return errors.NewBlockInvalidError("[%s][%s] coinbase is not final: %s", caller, block.Hash().String(), reason) } From db54c9522681ba0bc1c48619cbb31f33ce229688 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 21 Oct 2026 12:09:47 +0100 Subject: [PATCH 03/10] test(blockassembly): read the coinbase finality median time past from 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 Claude-Session: https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz --- CHANGELOG.md | 12 +++ ..._mining_solution_coinbase_finality_test.go | 100 +++++++++++------- 2 files changed, 73 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a118c71c2c..b541bfb8db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -443,6 +443,18 @@ Upgrading from v0.16.0 changes nothing for them; upgrading from an earlier relea shape. The same collapse on the HTTP blob store's own client and server is fixed too; no shipped configuration uses that store. (#1963) +- **A block whose coinbase is not final is rejected.** Affects every node and every miner. + bitcoin-sv applies its `IsFinalTx` rule to the coinbase as to any other transaction, and + Teranode now does too: a block whose coinbase has a non-final input sequence and an + nLockTime not yet reached is invalid (`bad-txns-nonfinal`). From CSV onwards a time-based + nLockTime is measured against the parent's median time past. `submitminingsolution` + refuses such a coinbase instead of mining a block peers reject. When a solution carries no + coinbase, the one block assembly rebuilds no longer gets the solution's time and nonce + copied into its nLockTime and input sequence; they go into the header only, as in + bitcoin-sv, so the rebuilt coinbase, and the merkle root built from it, no longer depend + on the nonce the miner found. A node on an + earlier build accepts blocks this one rejects, so upgrade every node together. (#2017) + ### Not breaking, but worth knowing - **Mining candidates no longer build on the old tip during a reorg.** While block diff --git a/services/blockassembly/submit_mining_solution_coinbase_finality_test.go b/services/blockassembly/submit_mining_solution_coinbase_finality_test.go index 85d6e0a141..489571c3b1 100644 --- a/services/blockassembly/submit_mining_solution_coinbase_finality_test.go +++ b/services/blockassembly/submit_mining_solution_coinbase_finality_test.go @@ -2,15 +2,19 @@ package blockassembly import ( "context" + "net/url" + "sync/atomic" "testing" "time" "github.com/bsv-blockchain/go-bt/v2" "github.com/bsv-blockchain/go-bt/v2/chainhash" "github.com/bsv-blockchain/go-chaincfg" - "github.com/bsv-blockchain/teranode/errors" "github.com/bsv-blockchain/teranode/model" "github.com/bsv-blockchain/teranode/services/blockassembly/blockassembly_api" + "github.com/bsv-blockchain/teranode/services/blockchain" + blockchainstore "github.com/bsv-blockchain/teranode/stores/blockchain" + "github.com/bsv-blockchain/teranode/ulogger" "github.com/bsv-blockchain/teranode/util" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -21,19 +25,58 @@ import ( // past, which is the comparison bitcoin-sv makes from CSV onwards. const finalityMTPLag = uint32(600) +// headerLookupCounter is a real blockchain client that counts the header reads the median-time-past +// lookup makes, so a test can assert that a lookup did or did not happen without mocking the client. +type headerLookupCounter struct { + blockchain.ClientI + lookups atomic.Int32 +} + +func (c *headerLookupCounter) GetBlockHeader(ctx context.Context, hash *chainhash.Hash) (*model.BlockHeader, *model.BlockHeaderMeta, error) { + c.lookups.Add(1) + + return c.ClientI.GetBlockHeader(ctx, hash) +} + +func (c *headerLookupCounter) GetBlockHeaders(ctx context.Context, hash *chainhash.Hash, n uint64) ([]*model.BlockHeader, []*model.BlockHeaderMeta, error) { + c.lookups.Add(1) + + return c.ClientI.GetBlockHeaders(ctx, hash, n) +} + +// coinbaseFinalityHarness is the coinbase-height harness plus the real blockchain client the +// assembler reads the parent's median time past from. +type coinbaseFinalityHarness struct { + *coinbaseHeightHarness + chain *headerLookupCounter +} + // newCoinbaseFinalityHarness is the coinbase-height harness with CSV active at the candidate height, // so a time-based coinbase nLockTime is measured against the parent's median time past, and with the -// assembler wired to the harness's blockchain mock for the median-time-past lookup. -func newCoinbaseFinalityHarness(t *testing.T) *coinbaseHeightHarness { +// assembler reading headers from a sqlitememory blockchain store. That store holds only genesis, so +// the harness's parent is unknown to it until a test stores a chain and points the candidate at it. +// AddBlock still goes to the coinbase-height harness's client, which records the mined block. +func newCoinbaseFinalityHarness(t *testing.T) *coinbaseFinalityHarness { t.Helper() params := chaincfg.RegressionNetParams params.CSVHeight = 1 h := newCoinbaseHeightHarness(t, ¶ms, postGenesisCandidateHeight, 5_000_000) - h.server.blockAssembler.blockchainClient = h.blockchai - return h + storeURL, err := url.Parse("sqlitememory://") + require.NoError(t, err) + + store, err := blockchainstore.NewStore(ulogger.TestLogger{}, storeURL, h.server.blockAssembler.settings) + require.NoError(t, err) + + client, err := blockchain.NewLocalClient(ulogger.TestLogger{}, h.server.blockAssembler.settings, store, nil, nil) + require.NoError(t, err) + + chain := &headerLookupCounter{ClientI: client} + h.server.blockAssembler.blockchainClient = chain + + return &coinbaseFinalityHarness{coinbaseHeightHarness: h, chain: chain} } // parentMTP is the median time past the tests give the candidate's parent. @@ -96,56 +139,35 @@ func TestSubmitMiningSolution_CoinbaseMustBeFinal(t *testing.T) { require.NoError(t, err) require.True(t, resp.Ok) h.blockchai.AssertCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) - h.blockchai.AssertNotCalled(t, "GetBlockHeader", mock.Anything, mock.Anything) + require.Zero(t, h.chain.lookups.Load(), "no median-time-past lookup was needed") }) t.Run("a cold memo is filled from the blockchain service", func(t *testing.T) { h := newCoinbaseFinalityHarness(t) - // A chain of eleven headers from the chain start, the parent last, one block every ten - // minutes ending ten minutes before the candidate time. Their median is the sixth, sixty - // minutes before the candidate time. - headers := make([]*model.BlockHeader, 11) - prev := &chainhash.Hash{} - - for i := range headers { - headers[i] = &model.BlockHeader{ - Version: 1, - HashPrevBlock: prev, - HashMerkleRoot: &chainhash.Hash{byte(i)}, - Timestamp: h.candidate.Time - uint32(len(headers)-i)*600, - Bits: h.nBits, - } - prev = headers[i].Hash() - } - - parent := headers[len(headers)-1] - h.candidate.PreviousHash = parent.Hash().CloneBytes() - - // GetBlockHeaders answers newest first. - newestFirst := make([]*model.BlockHeader, len(headers)) - for i, header := range headers { - newestFirst[len(headers)-1-i] = header - } - - h.blockchai.On("GetBlockHeader", mock.Anything, parent.Hash()).Return(parent, &model.BlockHeaderMeta{}, nil) - h.blockchai.On("GetBlockHeaders", mock.Anything, parent.Hash(), mock.Anything).Return(newestFirst, []*model.BlockHeaderMeta{}, nil) - + // Eleven blocks on the real store stamped base..base+10, so the parent's median time past is + // base+5: an hour before the candidate time. mtp := h.candidate.Time - 3600 + parent := buildFutureChain(t, t.Context(), h.chain.ClientI, mtp-5) + h.candidate.PreviousHash = parent.Hash().CloneBytes() resp, err := h.submit(t, timeLockedCoinbase(t, mtp)) require.Error(t, err) require.Nil(t, resp) require.Contains(t, err.Error(), "bad-txns-nonfinal") + h.blockchai.AssertNotCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + require.NotZero(t, h.chain.lookups.Load(), "a cold memo must be filled from the blockchain service") resp, err = h.submit(t, timeLockedCoinbase(t, mtp-1)) require.NoError(t, err) require.True(t, resp.Ok) + h.blockchai.AssertCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) }) t.Run("an unknown median time past refuses the solution", func(t *testing.T) { + // The harness's parent is not in the store, so the lookup fails as it would against an + // unreachable or out-of-step blockchain service. h := newCoinbaseFinalityHarness(t) - h.blockchai.On("GetBlockHeader", mock.Anything, mock.Anything).Return(nil, nil, errors.NewServiceUnavailableError("blockchain down")) resp, err := h.submit(t, timeLockedCoinbase(t, h.parentMTP()-1)) require.Error(t, err) @@ -158,14 +180,14 @@ func TestSubmitMiningSolution_CoinbaseMustBeFinal(t *testing.T) { t.Run("a coinbase whose verdict does not depend on the median time past needs no lookup", func(t *testing.T) { h := newCoinbaseFinalityHarness(t) - // Cold memo and no GetBlockHeader expectation: a lookup would fail the mock. + // Cold memo and a parent the store does not hold: a lookup would refuse the solution. coinbase := timeLockedCoinbase(t, h.candidate.Time) coinbase.Inputs[0].SequenceNumber = bt.DefaultSequenceNumber resp, err := h.submit(t, coinbase) require.NoError(t, err) require.True(t, resp.Ok) - h.blockchai.AssertNotCalled(t, "GetBlockHeader", mock.Anything, mock.Anything) + require.Zero(t, h.chain.lookups.Load(), "no median-time-past lookup was needed") }) t.Run("a height lock is refused without a lookup", func(t *testing.T) { @@ -175,7 +197,7 @@ func TestSubmitMiningSolution_CoinbaseMustBeFinal(t *testing.T) { require.Error(t, err) require.Nil(t, resp) require.Contains(t, err.Error(), "bad-txns-nonfinal") - h.blockchai.AssertNotCalled(t, "GetBlockHeader", mock.Anything, mock.Anything) + require.Zero(t, h.chain.lookups.Load(), "no median-time-past lookup was needed") }) } From 1acd9f9f2431041a0a4387417c5097cde2c3f98c Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 21 Oct 2026 12:54:12 +0100 Subject: [PATCH 04/10] test(blockvalidation): run the quick-validate coinbase finality test 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 Claude-Session: https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz --- .../quick_validate_coinbase_finality_test.go | 115 ++++++++++++++---- 1 file changed, 93 insertions(+), 22 deletions(-) diff --git a/services/blockvalidation/quick_validate_coinbase_finality_test.go b/services/blockvalidation/quick_validate_coinbase_finality_test.go index ec00f509fc..90dfa5e339 100644 --- a/services/blockvalidation/quick_validate_coinbase_finality_test.go +++ b/services/blockvalidation/quick_validate_coinbase_finality_test.go @@ -3,35 +3,106 @@ package blockvalidation import ( "testing" + "github.com/bsv-blockchain/go-bt/v2" "github.com/bsv-blockchain/teranode/errors" - "github.com/bsv-blockchain/teranode/services/blockvalidation/testhelpers" - "github.com/stretchr/testify/mock" + "github.com/bsv-blockchain/teranode/model" "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" ) -// TestQuickValidateBlock_NonFinalCoinbase pins bitcoin-sv/teranode#4981 on the quick-validation -// path, which never calls block.Valid: checkQuickValidationCoinbase applies -// model.CoinbaseFinalityViolation. A coinbase whose height-based nLockTime equals the block height, -// with a non-final input sequence, is non-final; the body is bound by the coinbase-only rule, so the -// verdict is invalid and the block is never committed. -func TestQuickValidateBlock_NonFinalCoinbase(t *testing.T) { - suite := NewCatchupTestSuite(t) - defer suite.Cleanup() - - suite.MockBlockchain.On("AssignBlockID", mock.Anything, mock.Anything).Return(uint64(1), nil).Maybe() - suite.MockBlockchain.On("AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() - - block := testhelpers.CreateTestBlocks(t, 1)[0] - block.Height = 100 - block.CoinbaseTx.LockTime = block.Height - block.CoinbaseTx.Inputs[0].SequenceNumber = 0 - block.Header.HashMerkleRoot = block.CoinbaseTx.TxIDChainHash() - - err := suite.Server.blockValidation.quickValidateBlock(suite.Ctx, block, "test", "") +// The quick-validation route never calls block.Valid, so checkQuickValidationCoinbase applies +// model.CoinbaseFinalityViolation itself (bitcoin-sv/teranode#4981). These tests run it over real +// stores (newPreBindHarness: sqlitememory UTXO and blockchain stores behind the ordinary +// LocalClient) on both entry points, and assert the end state: whether the block is in the +// blockchain store afterwards, not only which error came back. + +// coinbaseFinalityBlock builds a coinbase-only block at preBindHeight that chains from genesis. +// With final set, the coinbase input carries the final sequence number, so the block is valid and +// must commit. Without it, the sequence is 0 and nLockTime equals the block height, which +// IsTransactionFinal treats as not yet reached, so the coinbase is non-final. +func (h *preBindHarness) coinbaseFinalityBlock(final bool) *model.Block { + h.t.Helper() + + coinbase := preBindCoinbase(h.t, 0x41) + coinbase.LockTime = preBindHeight + + if final { + coinbase.Inputs[0].SequenceNumber = bt.DefaultSequenceNumber + } else { + coinbase.Inputs[0].SequenceNumber = 0 + } + + return h.newPreBindBlock(coinbase, nil, coinbase.TxIDChainHash(), 1) +} + +// requireBlockStored reports whether the block reached the blockchain store. +func (h *preBindHarness) requireBlockStored(block *model.Block, want bool) { + h.t.Helper() + + exists, err := h.chain.GetBlockExists(h.ctx, block.Hash()) + require.NoError(h.t, err) + require.Equal(h.t, want, exists, "block stored in the blockchain store") +} + +func requireNonFinalCoinbaseVerdict(t *testing.T, err error) { + t.Helper() + require.Error(t, err) require.True(t, errors.Is(err, errors.ErrBlockInvalid), "a bound non-final coinbase must condemn invalid, got: %v", err) require.False(t, errors.IsBlockCorrupt(err), "must NOT be corrupt") require.Contains(t, err.Error(), "bad-txns-nonfinal") +} + +func TestQuickValidateBlock_CoinbaseFinality(t *testing.T) { + t.Run("non-final coinbase is rejected and never stored", func(t *testing.T) { + h := newPreBindHarness(t, nil) + block := h.coinbaseFinalityBlock(false) + + requireNonFinalCoinbaseVerdict(t, h.bv.quickValidateBlock(h.ctx, block, "peer", "")) + h.requireBlockStored(block, false) + }) + + t.Run("final coinbase commits", func(t *testing.T) { + h := newPreBindHarness(t, nil) + block := h.coinbaseFinalityBlock(true) + + require.NoError(t, h.bv.quickValidateBlock(h.ctx, block, "peer", "")) + h.requireBlockStored(block, true) + }) +} + +// quickValidateAsync drives the default catch-up entry point the way the catch-up loop does, with a +// running subtree write worker. quickValidateBlockAsync commits the block itself on success. +func (h *preBindHarness) quickValidateAsync(block *model.Block) error { + h.t.Helper() + + writeJobsChan := make(chan *SubtreeWriteJob, 16) + + g, gCtx := errgroup.WithContext(h.ctx) + g.Go(func() error { return h.bv.subtreeWriteWorker(gCtx, writeJobsChan) }) + + _, _, err := h.bv.quickValidateBlockAsync(h.ctx, block, "peer", "", writeJobsChan) + + close(writeJobsChan) + require.NoError(h.t, g.Wait()) + + return err +} + +func TestQuickValidateBlockAsync_CoinbaseFinality(t *testing.T) { + t.Run("non-final coinbase is rejected and never stored", func(t *testing.T) { + h := newPreBindHarness(t, nil) + block := h.coinbaseFinalityBlock(false) + + requireNonFinalCoinbaseVerdict(t, h.quickValidateAsync(block)) + h.requireBlockStored(block, false) + }) + + t.Run("final coinbase commits", func(t *testing.T) { + h := newPreBindHarness(t, nil) + block := h.coinbaseFinalityBlock(true) - suite.MockBlockchain.AssertNotCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + require.NoError(t, h.quickValidateAsync(block)) + h.requireBlockStored(block, true) + }) } From 1ebc56d21e2f65bcb0e3e9b8f87abf6699c71eb2 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 21 Oct 2026 17:03:51 +0100 Subject: [PATCH 05/10] fix(blockassembly): keep the submit path's median-time-past lookup out 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 Claude-Session: https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz --- services/blockassembly/candidate_time.go | 83 ++++++---- ..._mining_solution_coinbase_finality_test.go | 155 +++++++++++++++--- 2 files changed, 178 insertions(+), 60 deletions(-) diff --git a/services/blockassembly/candidate_time.go b/services/blockassembly/candidate_time.go index d36d754605..2d46c29f42 100644 --- a/services/blockassembly/candidate_time.go +++ b/services/blockassembly/candidate_time.go @@ -180,24 +180,28 @@ func (b *BlockAssembler) MinCandidateTime(parentHash *chainhash.Hash) (int64, bo // // Unlike MinCandidateTime it neither stops at a memo miss nor defers to GenerateSupported: finality // is a consensus rule on every chain, so a miss goes to the blockchain service, and a failed lookup -// is an error the caller refuses on rather than a reason to skip the rule. A failed lookup is also -// logged by computeMtpFloor in candidateTime's wording, which names an unfloored candidate. +// is an error the caller refuses on rather than a reason to skip the rule. A hit reads the floor +// memo, but a miss neither fills it nor logs through the floor's latch: a submit is not a candidate, +// so its lookup must not evict a candidate path's entry, and a failure here is a refused submit that +// the caller reports, not an unfloored candidate time. func (b *BlockAssembler) ParentMedianTimePast(ctx context.Context, parentHash *chainhash.Hash) (uint32, error) { - entry := b.memoizedMtpFloor(parentHash) - if entry == nil { - // mtpFloor reads the header only for its hash-keyed fallback walk, which refuses a nil one. - parentHeader, _, err := b.blockchainClient.GetBlockHeader(ctx, parentHash) - if err != nil { - return 0, errors.NewProcessingError("failed to fetch parent header %s", parentHash.String(), err) - } + if entry := b.memoizedMtpFloor(parentHash); entry != nil { + // The memo holds the floor, median time past plus one. + return safeconversion.Int64ToUint32(entry.minTime - 1) + } - if entry = b.mtpFloor(ctx, parentHeader, parentHash); entry == nil { - return 0, errors.NewProcessingError("median time past of parent %s could not be established", parentHash.String()) - } + // The header is needed only for the hash-keyed fallback walk, which refuses a nil one. + parentHeader, _, err := b.blockchainClient.GetBlockHeader(ctx, parentHash) + if err != nil { + return 0, errors.NewProcessingError("failed to fetch parent header %s", parentHash.String(), err) } - // The memo holds the floor, median time past plus one. - return safeconversion.Int64ToUint32(entry.minTime - 1) + median, _, err := b.parentChainMedianTimePast(ctx, parentHeader, parentHash) + if err != nil { + return 0, errors.NewProcessingError("median time past of parent %s could not be established", parentHash.String(), err) + } + + return safeconversion.Int64ToUint32(median.Unix()) } // mtpFloor returns the memoized median-time-past floor for parentHash, @@ -292,6 +296,33 @@ func (b *BlockAssembler) placeMtpFloor(entry *mtpFloorEntry) { // time. It returns nil — having logged the cause — when the floor cannot be // established, leaving the caller to decide how to degrade. func (b *BlockAssembler) computeMtpFloor(ctx context.Context, parentHeader *model.BlockHeader, parentHash *chainhash.Hash) *mtpFloorEntry { + medianTimestamp, walkCause, err := b.parentChainMedianTimePast(ctx, parentHeader, parentHash) + if err != nil { + b.logFloorLookupFailure(parentHash, "%v", err) + + return nil + } + + if walkCause != nil { + // Name the cause of the slower path rather than discarding the error + // silently: the walk costs one round-trip per header, so an operator + // seeing the extra latency needs to know why it engaged. + b.logger.Warnf("[candidateTime] batched header fetch for parent %s was unusable (%v); fell back to the hash-keyed parent-chain walk", parentHash.String(), walkCause) + } + + entry := &mtpFloorEntry{parent: *parentHash, minTime: medianTimestamp.Unix() + 1} + + b.placeMtpFloor(entry) + + return entry +} + +// parentChainMedianTimePast computes the median time past of the headers ending +// at parentHash. It logs nothing and memoizes nothing, so a caller that is not +// building a candidate can use it without spending the floor's log latch or +// evicting a memo entry. When the batched run was unusable and the walk succeeded +// instead, walkCause is why, for the caller to report. +func (b *BlockAssembler) parentChainMedianTimePast(ctx context.Context, parentHeader *model.BlockHeader, parentHash *chainhash.Hash) (median time.Time, walkCause error, err error) { // A transport failure and an unusable result call for opposite responses, so // the fetch and the verification are separate steps. If the call itself fails // the blockchain service is unhealthy, and walking it once per header would @@ -302,9 +333,7 @@ func (b *BlockAssembler) computeMtpFloor(ctx context.Context, parentHeader *mode // walk exists to recover from and its per-hash reads cannot hit it. headers, fetchErr := b.fetchParentChainHeaders(ctx, parentHash) if fetchErr != nil { - b.logFloorLookupFailure(parentHash, "the blockchain service did not answer the batched header fetch (%v); not attempting the %d-read parent-chain walk against a service that is already failing", fetchErr, blockchain.MedianTimeBlocks-1) - - return nil + return time.Time{}, nil, errors.NewProcessingError("the blockchain service did not answer the batched header fetch; not attempting the %d-read parent-chain walk against a service that is already failing", blockchain.MedianTimeBlocks-1, fetchErr) } run := headers @@ -312,16 +341,10 @@ func (b *BlockAssembler) computeMtpFloor(ctx context.Context, parentHeader *mode if verifyErr := verifyParentChainRun(parentHash, headers); verifyErr != nil { walked, walkErr := b.walkParentChain(ctx, parentHeader, blockchain.MedianTimeBlocks) if walkErr != nil { - b.logFloorLookupFailure(parentHash, "the batched header fetch returned an unusable run (%v) and the hash-keyed parent-chain walk also failed (%v)", verifyErr, walkErr) - - return nil + return time.Time{}, nil, errors.NewProcessingError("the batched header fetch returned an unusable run (%v) and the hash-keyed parent-chain walk also failed", verifyErr, walkErr) } - // Name the cause of the slower path rather than discarding the error - // silently: the walk costs one round-trip per header, so an operator - // seeing the extra latency needs to know why it engaged. - b.logger.Warnf("[candidateTime] batched header fetch for parent %s was unusable (%v); fell back to the hash-keyed parent-chain walk", parentHash.String(), verifyErr) - + walkCause = verifyErr run = walked } @@ -334,16 +357,10 @@ func (b *BlockAssembler) computeMtpFloor(ctx context.Context, parentHeader *mode // producers above guarantee at least one header. medianTimestamp, err := model.CalculateMedianTimestamp(timestamps) if err != nil { - b.logFloorLookupFailure(parentHash, "the median timestamp over %d headers could not be calculated (%v)", len(run), err) - - return nil + return time.Time{}, nil, errors.NewProcessingError("the median timestamp over %d headers could not be calculated", len(run), err) } - entry := &mtpFloorEntry{parent: *parentHash, minTime: medianTimestamp.Unix() + 1} - - b.placeMtpFloor(entry) - - return entry + return *medianTimestamp, walkCause, nil } // generateSupported reports whether this chain lets blocks be generated quickly, diff --git a/services/blockassembly/submit_mining_solution_coinbase_finality_test.go b/services/blockassembly/submit_mining_solution_coinbase_finality_test.go index 489571c3b1..72b6d4b251 100644 --- a/services/blockassembly/submit_mining_solution_coinbase_finality_test.go +++ b/services/blockassembly/submit_mining_solution_coinbase_finality_test.go @@ -16,7 +16,6 @@ import ( blockchainstore "github.com/bsv-blockchain/teranode/stores/blockchain" "github.com/bsv-blockchain/teranode/ulogger" "github.com/bsv-blockchain/teranode/util" - "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -44,18 +43,23 @@ func (c *headerLookupCounter) GetBlockHeaders(ctx context.Context, hash *chainha return c.ClientI.GetBlockHeaders(ctx, hash, n) } -// coinbaseFinalityHarness is the coinbase-height harness plus the real blockchain client the -// assembler reads the parent's median time past from. +// coinbaseFinalityHarness is the coinbase-height harness over a real blockchain store: a +// sqlitememory store behind the ordinary LocalClient, holding a chain up to the candidate's parent. +// The server adds the mined block to that store, so a test asserts on what the store holds rather +// than on calls to a mocked client. The assembler reads the same store through headerLookupCounter, +// so a test can also tell whether the median-time-past lookup ran. type coinbaseFinalityHarness struct { *coinbaseHeightHarness chain *headerLookupCounter + store blockchain.ClientI + tip *model.BlockHeader } // newCoinbaseFinalityHarness is the coinbase-height harness with CSV active at the candidate height, -// so a time-based coinbase nLockTime is measured against the parent's median time past, and with the -// assembler reading headers from a sqlitememory blockchain store. That store holds only genesis, so -// the harness's parent is unknown to it until a test stores a chain and points the candidate at it. -// AddBlock still goes to the coinbase-height harness's client, which records the mined block. +// so a time-based coinbase nLockTime is measured against the parent's median time past. The store +// holds a chain from genesis to the candidate's parent, whose last eleven blocks are stamped so that +// the parent's median time past is parentMTP. Regtest has BIP34 off and Genesis active from height +// 100, so the stored chain and the candidate agree on height and on the post-Genesis rules. func newCoinbaseFinalityHarness(t *testing.T) *coinbaseFinalityHarness { t.Helper() @@ -73,10 +77,81 @@ func newCoinbaseFinalityHarness(t *testing.T) *coinbaseFinalityHarness { client, err := blockchain.NewLocalClient(ulogger.TestLogger{}, h.server.blockAssembler.settings, store, nil, nil) require.NoError(t, err) + tip := buildChainToParent(t, client, postGenesisCandidateHeight-1, h.parentMTP()-5) + h.candidate.PreviousHash = tip.Hash().CloneBytes() + chain := &headerLookupCounter{ClientI: client} + h.server.blockchainClient = client h.server.blockAssembler.blockchainClient = chain - return &coinbaseFinalityHarness{coinbaseHeightHarness: h, chain: chain} + return &coinbaseFinalityHarness{coinbaseHeightHarness: h, chain: chain, store: client, tip: tip} +} + +// buildChainToParent stores blocks from genesis up to height and returns the last one. The last +// eleven are stamped base..base+10, so the returned block's median time past is base+5; the ones +// before them carry base, which keeps the chain's timestamps non-decreasing. +func buildChainToParent(t *testing.T, client blockchain.ClientI, height uint32, base uint32) *model.BlockHeader { + t.Helper() + + ctx := t.Context() + + prev, _, err := client.GetBestBlockHeader(ctx) + require.NoError(t, err) + + coinbase, err := bt.NewTxFromString("02000000010000000000000000000000000000000000000000000000000000000000000000ffffffff03510101ffffffff0100f2052a01000000232103656065e6886ca1e947de3471c9e723673ab6ba34724476417fa9fcef8bafa604ac00000000") + require.NoError(t, err) + + nBits, err := model.NewNBitFromString("207fffff") + require.NoError(t, err) + + for i := uint32(1); i <= height; i++ { + timestamp := base + if height-i < 11 { + timestamp = base + 10 - (height - i) + } + + header := &model.BlockHeader{ + Version: 0x20000000, + HashPrevBlock: prev.Hash(), + HashMerkleRoot: &chainhash.Hash{}, + Timestamp: timestamp, + Bits: *nBits, + Nonce: i, + } + + require.NoError(t, client.AddBlock(ctx, &model.Block{ + Header: header, + CoinbaseTx: coinbase, + TransactionCount: 1, + Subtrees: []*chainhash.Hash{}, + }, "")) + + prev = header + } + + return prev +} + +// requireMined asserts whether the server added a block on the candidate's parent to the store, and +// returns it when it did. +func (h *coinbaseFinalityHarness) requireMined(t *testing.T, want bool) *model.Block { + t.Helper() + + best, _, err := h.store.GetBestBlockHeader(t.Context()) + require.NoError(t, err) + + if !want { + require.Equal(t, h.tip.Hash(), best.Hash(), "no block may be added on the candidate's parent") + + return nil + } + + require.Equal(t, h.tip.Hash(), best.HashPrevBlock, "a block must be added on the candidate's parent") + + block, err := h.store.GetBlock(t.Context(), best.Hash()) + require.NoError(t, err) + + return block } // parentMTP is the median time past the tests give the candidate's parent. @@ -123,7 +198,7 @@ func TestSubmitMiningSolution_CoinbaseMustBeFinal(t *testing.T) { require.Error(t, err) require.Nil(t, resp) require.Contains(t, err.Error(), "bad-txns-nonfinal") - h.blockchai.AssertNotCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + h.requireMined(t, false) // The pool may submit again with a final coinbase, so the job stays and assembly is not // reset: nothing local is at fault. @@ -138,55 +213,50 @@ func TestSubmitMiningSolution_CoinbaseMustBeFinal(t *testing.T) { resp, err := h.submit(t, timeLockedCoinbase(t, h.parentMTP()-1)) require.NoError(t, err) require.True(t, resp.Ok) - h.blockchai.AssertCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + require.Equal(t, h.parentMTP()-1, h.requireMined(t, true).CoinbaseTx.LockTime) require.Zero(t, h.chain.lookups.Load(), "no median-time-past lookup was needed") }) - t.Run("a cold memo is filled from the blockchain service", func(t *testing.T) { + t.Run("a cold memo is read from the blockchain service", func(t *testing.T) { h := newCoinbaseFinalityHarness(t) - // Eleven blocks on the real store stamped base..base+10, so the parent's median time past is - // base+5: an hour before the candidate time. - mtp := h.candidate.Time - 3600 - parent := buildFutureChain(t, t.Context(), h.chain.ClientI, mtp-5) - h.candidate.PreviousHash = parent.Hash().CloneBytes() - - resp, err := h.submit(t, timeLockedCoinbase(t, mtp)) + resp, err := h.submit(t, timeLockedCoinbase(t, h.parentMTP())) require.Error(t, err) require.Nil(t, resp) require.Contains(t, err.Error(), "bad-txns-nonfinal") - h.blockchai.AssertNotCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) - require.NotZero(t, h.chain.lookups.Load(), "a cold memo must be filled from the blockchain service") + h.requireMined(t, false) + require.NotZero(t, h.chain.lookups.Load(), "a cold memo must be read from the blockchain service") - resp, err = h.submit(t, timeLockedCoinbase(t, mtp-1)) + resp, err = h.submit(t, timeLockedCoinbase(t, h.parentMTP()-1)) require.NoError(t, err) require.True(t, resp.Ok) - h.blockchai.AssertCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + h.requireMined(t, true) }) t.Run("an unknown median time past refuses the solution", func(t *testing.T) { - // The harness's parent is not in the store, so the lookup fails as it would against an - // unreachable or out-of-step blockchain service. + // A parent the store does not hold, so the lookup fails as it would against an unreachable + // or out-of-step blockchain service. h := newCoinbaseFinalityHarness(t) + h.candidate.PreviousHash = (&chainhash.Hash{9}).CloneBytes() resp, err := h.submit(t, timeLockedCoinbase(t, h.parentMTP()-1)) require.Error(t, err) require.Nil(t, resp) require.Contains(t, err.Error(), "cannot check that the coinbase is final") - h.blockchai.AssertNotCalled(t, "AddBlock", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + h.requireMined(t, false) h.requireNoReset(t) }) t.Run("a coinbase whose verdict does not depend on the median time past needs no lookup", func(t *testing.T) { h := newCoinbaseFinalityHarness(t) - // Cold memo and a parent the store does not hold: a lookup would refuse the solution. coinbase := timeLockedCoinbase(t, h.candidate.Time) coinbase.Inputs[0].SequenceNumber = bt.DefaultSequenceNumber resp, err := h.submit(t, coinbase) require.NoError(t, err) require.True(t, resp.Ok) + h.requireMined(t, true) require.Zero(t, h.chain.lookups.Load(), "no median-time-past lookup was needed") }) @@ -197,10 +267,41 @@ func TestSubmitMiningSolution_CoinbaseMustBeFinal(t *testing.T) { require.Error(t, err) require.Nil(t, resp) require.Contains(t, err.Error(), "bad-txns-nonfinal") + h.requireMined(t, false) require.Zero(t, h.chain.lookups.Load(), "no median-time-past lookup was needed") }) } +// TestParentMedianTimePast_ColdLookupLeavesTheFloorAlone pins that the submit path's lookup is not a +// candidate's: a miss reads the store without filling the floor memo, so it cannot evict an entry a +// candidate path relies on, and a failure is returned to the caller without spending the floor's +// once-per-parent log latch. +func TestParentMedianTimePast_ColdLookupLeavesTheFloorAlone(t *testing.T) { + h := newCoinbaseFinalityHarness(t) + ba := h.server.blockAssembler + + // Fill both memo slots with other parents, as the two candidate paths leave them. + others := []*mtpFloorEntry{{parent: chainhash.Hash{1}, minTime: 1}, {parent: chainhash.Hash{2}, minTime: 2}} + for _, entry := range others { + ba.placeMtpFloor(entry) + } + + mtp, err := ba.ParentMedianTimePast(t.Context(), h.tip.Hash()) + require.NoError(t, err) + require.Equal(t, h.parentMTP(), mtp) + require.NotZero(t, h.chain.lookups.Load(), "a cold memo must be read from the blockchain service") + + require.Nil(t, ba.memoizedMtpFloor(h.tip.Hash()), "a submit lookup must not fill the floor memo") + + for _, entry := range others { + require.Same(t, entry, ba.memoizedMtpFloor(&entry.parent), "a submit lookup must not evict a candidate path's entry") + } + + _, err = ba.ParentMedianTimePast(t.Context(), &chainhash.Hash{9}) + require.Error(t, err) + require.Nil(t, ba.mtpFloorFailureLoggedTip.Load(), "a submit lookup failure must not spend the floor's log latch") +} + // TestSubmitMiningSolution_RecreatedCoinbaseIsFinal pins the coinbase block assembly builds when a // solution omits one. The solution's time and nonce used to be copied into that coinbase as its // nLockTime and input sequence, which made it non-final for any non-zero nonce @@ -245,7 +346,7 @@ func TestSubmitMiningSolution_RecreatedCoinbaseIsFinal(t *testing.T) { require.NoError(t, err) require.True(t, resp.Ok) - block := h.addedBlock(t) + block := h.requireMined(t, true) require.Equal(t, uint32(7), block.Header.Nonce, "the nonce belongs in the header") require.Equal(t, nTime, block.Header.Timestamp, "the time belongs in the header") require.Zero(t, block.CoinbaseTx.LockTime, "the time must not be copied into the coinbase nLockTime") From fa510dd30d87fe28d06b2b6e8ef24c0557308447 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Thu, 22 Oct 2026 10:36:06 +0100 Subject: [PATCH 06/10] docs(changelog): say that a coinbase-less mining solution is still unminable 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 Claude-Session: https://claude.ai/code/session_0191Q6pyDA2snkaJ2JRGJXLz --- CHANGELOG.md | 8 +++++--- .../submit_mining_solution_coinbase_finality_test.go | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b541bfb8db..13196e31ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -451,9 +451,11 @@ Upgrading from v0.16.0 changes nothing for them; upgrading from an earlier relea refuses such a coinbase instead of mining a block peers reject. When a solution carries no coinbase, the one block assembly rebuilds no longer gets the solution's time and nonce copied into its nLockTime and input sequence; they go into the header only, as in - bitcoin-sv, so the rebuilt coinbase, and the merkle root built from it, no longer depend - on the nonce the miner found. A node on an - earlier build accepts blocks this one rejects, so upgrade every node together. (#2017) + bitcoin-sv. That does not make a coinbase-less solution minable: the rebuilt coinbase + still carries a fresh random extranonce, so its merkle root is not the one the miner + hashed, and such a submit still fails the proof-of-work check. Submit the coinbase with + the solution, as every in-repo miner already does. A node on an earlier build accepts + blocks this one rejects, so upgrade every node together. (#2017) ### Not breaking, but worth knowing diff --git a/services/blockassembly/submit_mining_solution_coinbase_finality_test.go b/services/blockassembly/submit_mining_solution_coinbase_finality_test.go index 72b6d4b251..8682946565 100644 --- a/services/blockassembly/submit_mining_solution_coinbase_finality_test.go +++ b/services/blockassembly/submit_mining_solution_coinbase_finality_test.go @@ -316,6 +316,10 @@ func TestSubmitMiningSolution_RecreatedCoinbaseIsFinal(t *testing.T) { // whether a given nonce meets the target, cannot be known in advance. Regtest's target takes // about half of all hashes, so resubmit until one does; a miss is a proof-of-work rejection, // which deletes the job and resets assembly, so put the job back each time. + // + // This loop exists only because of that random rebuild, which is also why a real miner can + // never land a coinbase-less solution. It becomes a single submit once the job keeps the + // candidate's coinbase and reuses it, as bitcoin-sv does; that change is a separate PR. var ( resp *blockassembly_api.OKResponse err error From 8164da4dc6e521db7b80add857c2c338e0405304 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 28 Oct 2026 12:30:24 +0000 Subject: [PATCH 07/10] fix(blockassembly): check proof of work before the coinbase finality 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 Claude-Session: https://claude.ai/code/session_016HzSYaAXVb6mbD9mt4iDds --- services/blockassembly/Server.go | 35 ++++++++++--------- services/blockassembly/candidate_time.go | 17 +++++---- ..._mining_solution_coinbase_finality_test.go | 29 ++++++++++++++- 3 files changed, 56 insertions(+), 25 deletions(-) diff --git a/services/blockassembly/Server.go b/services/blockassembly/Server.go index d59918f9e8..81e9012d95 100644 --- a/services/blockassembly/Server.go +++ b/services/blockassembly/Server.go @@ -1958,23 +1958,9 @@ func (ba *BlockAssembly) submitMiningSolution(ctx context.Context, req *BlockSub return nil, err } - // The coinbase must be final (bitcoin-sv/teranode#4981). block.Valid below applies the rule too, - // but with no ancestor headers it cannot evaluate a time-based nLockTime from CSV onwards, and - // this block goes from here straight to AddBlock, never through block validation. So when the - // verdict depends on the parent's median time past, fetch it, and refuse the solution if it - // cannot be established rather than add a block every peer may reject. - var parentMedianTimePast uint32 - - if model.CoinbaseFinalityNeedsMedianTimePast(coinbaseTx, job.MiningCandidate.Height, ba.blockAssembler.settings.ChainCfgParams) { - parentMedianTimePast, err = ba.blockAssembler.ParentMedianTimePast(ctx, hashPrevBlock) - if err != nil { - return nil, errors.NewProcessingError("[BlockAssembly][%s] cannot check that the coinbase is final: its time-based nLockTime needs the parent's median time past", jobID, err) - } - } - - if reason := model.CoinbaseFinalityViolation(coinbaseTx, nTime, job.MiningCandidate.Height, ba.blockAssembler.settings.ChainCfgParams, parentMedianTimePast); reason != "" { - // A consensus verdict on the miner's coinbase, so the same class block.Valid gives it. The - // lookup failure above is this node's problem and stays a processing error. + // Reject finality violations that need no ancestor lookup now. A post-CSV time lock is checked + // below, after block.Valid has verified proof of work, so junk nonces cannot trigger RPC reads. + if reason := model.CoinbaseFinalityViolation(coinbaseTx, nTime, job.MiningCandidate.Height, ba.blockAssembler.settings.ChainCfgParams, 0); reason != "" { return nil, errors.NewBlockInvalidError("[BlockAssembly][%s] coinbase is not final: %s", jobID, reason) } @@ -2123,6 +2109,21 @@ func (ba *BlockAssembly) submitMiningSolution(ctx context.Context, req *BlockSub return nil, errors.NewProcessingError("[BlockAssembly][%s][%s] invalid block", jobID, block.Hash().String(), err) } + // block.Valid has no ancestor headers here, so it cannot decide post-CSV time-based finality. + // This block goes straight to AddBlock without passing through block validation. Establish the + // parent's median time past only after validating the submitted proof of work, and refuse the + // solution if the lookup fails. A rejected coinbase leaves the job available for a corrected one. + if model.CoinbaseFinalityNeedsMedianTimePast(coinbaseTx, block.Height, ba.blockAssembler.settings.ChainCfgParams) { + parentMedianTimePast, lookupErr := ba.blockAssembler.ParentMedianTimePast(ctx, hashPrevBlock) + if lookupErr != nil { + return nil, errors.NewProcessingError("[BlockAssembly][%s] cannot check that the coinbase is final: its time-based nLockTime needs the parent's median time past", jobID, lookupErr) + } + + if reason := model.CoinbaseFinalityViolation(coinbaseTx, nTime, block.Height, ba.blockAssembler.settings.ChainCfgParams, parentMedianTimePast); reason != "" { + return nil, errors.NewBlockInvalidError("[BlockAssembly][%s] coinbase is not final: %s", jobID, reason) + } + } + // decouple the tracing context to not cancel the context when the block is being saved, even if we cancel the request callerCtx, _, endSpan := tracing.DecoupleTracingSpan(ctx, "blockassembly", "decoupleBlockSaving") defer endSpan() diff --git a/services/blockassembly/candidate_time.go b/services/blockassembly/candidate_time.go index 2d46c29f42..3cd8cc4e84 100644 --- a/services/blockassembly/candidate_time.go +++ b/services/blockassembly/candidate_time.go @@ -190,13 +190,7 @@ func (b *BlockAssembler) ParentMedianTimePast(ctx context.Context, parentHash *c return safeconversion.Int64ToUint32(entry.minTime - 1) } - // The header is needed only for the hash-keyed fallback walk, which refuses a nil one. - parentHeader, _, err := b.blockchainClient.GetBlockHeader(ctx, parentHash) - if err != nil { - return 0, errors.NewProcessingError("failed to fetch parent header %s", parentHash.String(), err) - } - - median, _, err := b.parentChainMedianTimePast(ctx, parentHeader, parentHash) + median, _, err := b.parentChainMedianTimePast(ctx, nil, parentHash) if err != nil { return 0, errors.NewProcessingError("median time past of parent %s could not be established", parentHash.String(), err) } @@ -339,6 +333,15 @@ func (b *BlockAssembler) parentChainMedianTimePast(ctx context.Context, parentHe run := headers if verifyErr := verifyParentChainRun(parentHash, headers); verifyErr != nil { + // Candidate creation already holds this header; a submit lookup only needs to fetch it + // when the batched result is unusable and the hash-keyed walk is necessary. + if parentHeader == nil { + parentHeader, _, err = b.blockchainClient.GetBlockHeader(ctx, parentHash) + if err != nil { + return time.Time{}, nil, errors.NewProcessingError("failed to fetch parent header %s for the hash-keyed walk", parentHash.String(), err) + } + } + walked, walkErr := b.walkParentChain(ctx, parentHeader, blockchain.MedianTimeBlocks) if walkErr != nil { return time.Time{}, nil, errors.NewProcessingError("the batched header fetch returned an unusable run (%v) and the hash-keyed parent-chain walk also failed", verifyErr, walkErr) diff --git a/services/blockassembly/submit_mining_solution_coinbase_finality_test.go b/services/blockassembly/submit_mining_solution_coinbase_finality_test.go index 8682946565..8ce14ce903 100644 --- a/services/blockassembly/submit_mining_solution_coinbase_finality_test.go +++ b/services/blockassembly/submit_mining_solution_coinbase_finality_test.go @@ -188,6 +188,33 @@ func timeLockedCoinbase(t *testing.T, lockTime uint32) *bt.Tx { // submit path straight to AddBlock. The submit path therefore fetches the parent's median time past // itself, but only when the coinbase's verdict depends on it. func TestSubmitMiningSolution_CoinbaseMustBeFinal(t *testing.T) { + t.Run("invalid proof of work does not fetch median time past", func(t *testing.T) { + h := newCoinbaseFinalityHarness(t) + coinbase := timeLockedCoinbase(t, h.parentMTP()-1) + header := &model.BlockHeader{ + Version: h.candidate.Version, HashPrevBlock: h.tip.Hash(), + HashMerkleRoot: coinbase.TxIDChainHash(), Timestamp: h.candidate.Time, Bits: h.nBits, + } + for { + met, _, _ := header.HasMetTargetDifficulty() + if !met { + break + } + header.Nonce++ + } + + resp, err := h.server.submitMiningSolution(t.Context(), &BlockSubmissionRequest{ + SubmitMiningSolutionRequest: &blockassembly_api.SubmitMiningSolutionRequest{ + Id: h.idBytes, CoinbaseTx: coinbase.Bytes(), Nonce: header.Nonce, + }, + }) + require.Error(t, err) + require.Nil(t, resp) + require.Contains(t, err.Error(), "target difficulty") + require.Zero(t, h.chain.lookups.Load(), "an invalid nonce must not trigger blockchain reads") + h.requireMined(t, false) + }) + t.Run("a time lock at or after the parent's median time past is refused", func(t *testing.T) { h := newCoinbaseFinalityHarness(t) h.warmMTP(t) @@ -289,7 +316,7 @@ func TestParentMedianTimePast_ColdLookupLeavesTheFloorAlone(t *testing.T) { mtp, err := ba.ParentMedianTimePast(t.Context(), h.tip.Hash()) require.NoError(t, err) require.Equal(t, h.parentMTP(), mtp) - require.NotZero(t, h.chain.lookups.Load(), "a cold memo must be read from the blockchain service") + require.Equal(t, int32(1), h.chain.lookups.Load(), "a valid batched run needs no separate parent-header read") require.Nil(t, ba.memoizedMtpFloor(h.tip.Hash()), "a submit lookup must not fill the floor memo") From 7d59091b700e9f762140b25d578bce3e8a9717bb Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 28 Oct 2026 12:58:33 +0000 Subject: [PATCH 08/10] refactor(blockassembly): move the coinbase time-lock check into its own 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 Claude-Session: https://claude.ai/code/session_016HzSYaAXVb6mbD9mt4iDds --- services/blockassembly/Server.go | 40 ++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/services/blockassembly/Server.go b/services/blockassembly/Server.go index aa7d1ed0e8..a4319a1c9a 100644 --- a/services/blockassembly/Server.go +++ b/services/blockassembly/Server.go @@ -1880,6 +1880,29 @@ func coinbaseHasP2SHOutput(tx *bt.Tx) bool { return false } +// checkCoinbaseTimeLockFinality applies the coinbase finality rule where its verdict depends on the +// parent's median time past: from CSV onwards, a time-based nLockTime with a non-final sequence. In +// every other case it does nothing, because submitMiningSolution has already applied the rule +// without the lookup. A failed lookup is this node's problem and stays a processing error. A +// non-final coinbase is a consensus verdict on the miner's data, the same class block.Valid gives it. +func (ba *BlockAssembly) checkCoinbaseTimeLockFinality(ctx context.Context, jobID string, coinbaseTx *bt.Tx, nTime, height uint32, hashPrevBlock *chainhash.Hash) error { + params := ba.blockAssembler.settings.ChainCfgParams + if !model.CoinbaseFinalityNeedsMedianTimePast(coinbaseTx, height, params) { + return nil + } + + parentMedianTimePast, err := ba.blockAssembler.ParentMedianTimePast(ctx, hashPrevBlock) + if err != nil { + return errors.NewProcessingError("[BlockAssembly][%s] cannot check that the coinbase is final: its time-based nLockTime needs the parent's median time past", jobID, err) + } + + if reason := model.CoinbaseFinalityViolation(coinbaseTx, nTime, height, params, parentMedianTimePast); reason != "" { + return errors.NewBlockInvalidError("[BlockAssembly][%s] coinbase is not final: %s", jobID, reason) + } + + return nil +} + func (ba *BlockAssembly) submitMiningSolution(ctx context.Context, req *BlockSubmissionRequest) (*blockassembly_api.OKResponse, error) { jobID := util.ReverseAndHexEncodeSlice(req.SubmitMiningSolutionRequest.Id) @@ -2153,18 +2176,11 @@ func (ba *BlockAssembly) submitMiningSolution(ctx context.Context, req *BlockSub } // block.Valid has no ancestor headers here, so it cannot decide post-CSV time-based finality. - // This block goes straight to AddBlock without passing through block validation. Establish the - // parent's median time past only after validating the submitted proof of work, and refuse the - // solution if the lookup fails. A rejected coinbase leaves the job available for a corrected one. - if model.CoinbaseFinalityNeedsMedianTimePast(coinbaseTx, block.Height, ba.blockAssembler.settings.ChainCfgParams) { - parentMedianTimePast, lookupErr := ba.blockAssembler.ParentMedianTimePast(ctx, hashPrevBlock) - if lookupErr != nil { - return nil, errors.NewProcessingError("[BlockAssembly][%s] cannot check that the coinbase is final: its time-based nLockTime needs the parent's median time past", jobID, lookupErr) - } - - if reason := model.CoinbaseFinalityViolation(coinbaseTx, nTime, block.Height, ba.blockAssembler.settings.ChainCfgParams, parentMedianTimePast); reason != "" { - return nil, errors.NewBlockInvalidError("[BlockAssembly][%s] coinbase is not final: %s", jobID, reason) - } + // This block goes straight to AddBlock without passing through block validation, so the time + // lock is checked now, after block.Valid has verified the submitted proof of work. A rejected + // coinbase leaves the job available for a corrected one. + if err = ba.checkCoinbaseTimeLockFinality(ctx, jobID, coinbaseTx, nTime, block.Height, hashPrevBlock); err != nil { + return nil, err } // decouple the tracing context to not cancel the context when the block is being saved, even if we cancel the request From 1999dc299d1b67b3db812d04ee40c50eda3a861f Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 28 Oct 2026 13:26:43 +0000 Subject: [PATCH 09/10] fix(blockassembly): log a coinbase finality refusal after the proof-of-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 Claude-Session: https://claude.ai/code/session_016HzSYaAXVb6mbD9mt4iDds --- services/blockassembly/Server.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/blockassembly/Server.go b/services/blockassembly/Server.go index 7183a0a581..0a46a06175 100644 --- a/services/blockassembly/Server.go +++ b/services/blockassembly/Server.go @@ -1985,6 +1985,8 @@ func (ba *BlockAssembly) checkCoinbaseTimeLockFinality(ctx context.Context, jobI } if reason := model.CoinbaseFinalityViolation(coinbaseTx, nTime, height, params, parentMedianTimePast); reason != "" { + ba.logger.Warnf("[BlockAssembly][%s] rejected mining solution, coinbase is not final: %s (parent median time past %d, coinbase nLockTime %d)", jobID, reason, parentMedianTimePast, coinbaseTx.LockTime) + return errors.NewBlockInvalidError("[BlockAssembly][%s] coinbase is not final: %s", jobID, reason) } From 2f1bf5d5d2a56011fe7f531b9b273434563d2466 Mon Sep 17 00:00:00 2001 From: freemans13 Date: Wed, 28 Oct 2026 13:39:25 +0000 Subject: [PATCH 10/10] fix(blockassembly): name the block hash in the coinbase finality refusal 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 1999dc299. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016HzSYaAXVb6mbD9mt4iDds --- services/blockassembly/Server.go | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/services/blockassembly/Server.go b/services/blockassembly/Server.go index 0a46a06175..eca55f25f3 100644 --- a/services/blockassembly/Server.go +++ b/services/blockassembly/Server.go @@ -1968,26 +1968,27 @@ func coinbaseHasP2SHOutput(tx *bt.Tx) bool { return false } -// checkCoinbaseTimeLockFinality applies the coinbase finality rule where its verdict depends on the -// parent's median time past: from CSV onwards, a time-based nLockTime with a non-final sequence. In -// every other case it does nothing, because submitMiningSolution has already applied the rule -// without the lookup. A failed lookup is this node's problem and stays a processing error. A -// non-final coinbase is a consensus verdict on the miner's data, the same class block.Valid gives it. -func (ba *BlockAssembly) checkCoinbaseTimeLockFinality(ctx context.Context, jobID string, coinbaseTx *bt.Tx, nTime, height uint32, hashPrevBlock *chainhash.Hash) error { +// checkCoinbaseTimeLockFinality applies the coinbase finality rule to block where its verdict +// depends on the parent's median time past: from CSV onwards, a time-based nLockTime with a +// non-final sequence. In every other case it does nothing, because submitMiningSolution has already +// applied the rule without the lookup. A failed lookup is this node's problem and stays a processing +// error. A non-final coinbase is a consensus verdict on the miner's data, the same class block.Valid +// gives it. +func (ba *BlockAssembly) checkCoinbaseTimeLockFinality(ctx context.Context, jobID string, block *model.Block) error { params := ba.blockAssembler.settings.ChainCfgParams - if !model.CoinbaseFinalityNeedsMedianTimePast(coinbaseTx, height, params) { + if !model.CoinbaseFinalityNeedsMedianTimePast(block.CoinbaseTx, block.Height, params) { return nil } - parentMedianTimePast, err := ba.blockAssembler.ParentMedianTimePast(ctx, hashPrevBlock) + parentMedianTimePast, err := ba.blockAssembler.ParentMedianTimePast(ctx, block.Header.HashPrevBlock) if err != nil { - return errors.NewProcessingError("[BlockAssembly][%s] cannot check that the coinbase is final: its time-based nLockTime needs the parent's median time past", jobID, err) + return errors.NewProcessingError("[BlockAssembly][%s][%s] cannot check that the coinbase is final: its time-based nLockTime needs the parent's median time past", jobID, block.Hash().String(), err) } - if reason := model.CoinbaseFinalityViolation(coinbaseTx, nTime, height, params, parentMedianTimePast); reason != "" { - ba.logger.Warnf("[BlockAssembly][%s] rejected mining solution, coinbase is not final: %s (parent median time past %d, coinbase nLockTime %d)", jobID, reason, parentMedianTimePast, coinbaseTx.LockTime) + if reason := model.CoinbaseFinalityViolation(block.CoinbaseTx, block.Header.Timestamp, block.Height, params, parentMedianTimePast); reason != "" { + ba.logger.Warnf("[BlockAssembly][%s][%s] rejected mining solution, coinbase is not final: %s (parent median time past %d, coinbase nLockTime %d)", jobID, block.Hash().String(), reason, parentMedianTimePast, block.CoinbaseTx.LockTime) - return errors.NewBlockInvalidError("[BlockAssembly][%s] coinbase is not final: %s", jobID, reason) + return errors.NewBlockInvalidError("[BlockAssembly][%s][%s] coinbase is not final: %s", jobID, block.Hash().String(), reason) } return nil @@ -2283,7 +2284,7 @@ func (ba *BlockAssembly) submitMiningSolution(ctx context.Context, req *BlockSub // This block goes straight to AddBlock without passing through block validation, so the time // lock is checked now, after block.Valid has verified the submitted proof of work. A rejected // coinbase leaves the job available for a corrected one. - if err = ba.checkCoinbaseTimeLockFinality(ctx, jobID, coinbaseTx, nTime, block.Height, hashPrevBlock); err != nil { + if err = ba.checkCoinbaseTimeLockFinality(ctx, jobID, block); err != nil { return nil, err }