fix(blockvalidation): reject blocks whose coinbase is not final, and stop block assembly mining them

CHANGELOG.md

@@ -730,6 +730,19 @@ 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, and keeps the job, so a
+  corrected coinbase can still be submitted against it. The time-based check needs the
+  parent's median time past, which is looked up only after the solution has passed the
+  proof-of-work check. A solution's time and nonce are no longer copied into the coinbase
+  as its nLockTime and input sequence; they go into the header only, as in bitcoin-sv. A
+  node on an earlier build accepts blocks this one rejects, so upgrade every node
+  together. (#2017)
+
 - **A block's conflicting transactions are checked in one walk, and
   `utxostore_conflictingChildrenMaxNodes` becomes a per-root budget for it.** Subtree
   validation used to check a block's conflicting transactions one at a time, re-walking

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.Timestamp, 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

model/block_coinbase_finality.go

@@ -0,0 +1,64 @@
+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 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 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, blockTime uint32, height uint32, params *chaincfg.Params, parentMedianTimePast uint32) string {
+	lockTimeCutoff := blockTime
+	if height >= params.CSVHeight {
+		if parentMedianTimePast == 0 && CoinbaseFinalityNeedsMedianTimePast(coinbaseTx, height, params) {
+			return ""
+		}
+
+		lockTimeCutoff = parentMedianTimePast
+	}
+
+	if util.IsTransactionFinal(coinbaseTx, height, lockTimeCutoff) != nil {
+		return "bad-txns-nonfinal"
+	}
+
+	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
+}