fix(blockvalidation): check difficulty of every catch-up header before fetching blocks

CHANGELOG.md

@@ -877,6 +877,20 @@ Upgrading from v0.16.0 changes nothing for them; upgrading from an earlier relea
 
 ### Not breaking, but worth knowing
 
+- **Catch-up checks the difficulty of every offered header before fetching any
+  block.** The header precheck used to skip the first ~147 headers after the
+  common ancestor, every pre-DAA header and chains that never adjust difficulty,
+  leaving them to block validation after the subtree prewarm had already fetched
+  their data. A short fork mined against a target the peer chose could therefore
+  make the node download block data. Every header is now checked against the
+  same calculator block validation uses, reading the window below the ancestor
+  from the blockchain store. A peer offering a wrong `nBits` is reported as
+  malicious (`invalid_difficulty`) before any body is requested, and catch-up
+  fails without blaming the peer when the expected difficulty cannot be read.
+  Expect a few hundred extra blockchain store reads per catch-up. Block
+  validation's own check is unchanged. No action is required.
+  (#2084)
+
 - **Peer subtrees announced during a catchup are validated afterwards instead of
   lost.** While catching up, the subtree validator used to drop every
   peer-announced subtree, so the next block arrived with all of them unvalidated

services/blockchain/Difficulty.go

@@ -359,21 +359,8 @@ func (d *Difficulty) calcHistoricalWorkRequired(ctx context.Context, src HeaderS
 //   - suitableFirstBlock: First block in the difficulty adjustment window
 //   - suitableLastBlock: Last block in the difficulty adjustment window
 func (d *Difficulty) computeTarget(suitableFirstBlock *model.SuitableBlock, suitableLastBlock *model.SuitableBlock) (*model.NBit, error) {
-	return ComputeTarget(d.settings, suitableFirstBlock, suitableLastBlock)
-}
+	tSettings := d.settings
 
-// ComputeTarget calculates the DAA target difficulty from the first and last suitable
-// blocks of the difficulty adjustment window. It is the pure arithmetic core shared by
-// the store-backed Difficulty.CalcNextWorkRequired path and by callers that reconstruct
-// the window from headers not yet persisted (e.g. block validation catchup). It reads
-// only the ChainWork and Time of the two suitable blocks plus the chain parameters, so
-// it makes no store access and holds no state.
-//
-// Parameters:
-//   - tSettings: chain settings (TargetTimePerBlock, PowLimit, NoDifficultyAdjustment)
-//   - suitableFirstBlock: first (older) suitable block in the window
-//   - suitableLastBlock: last (newer) suitable block in the window
-func ComputeTarget(tSettings *settings.Settings, suitableFirstBlock *model.SuitableBlock, suitableLastBlock *model.SuitableBlock) (*model.NBit, error) {
 	lastSuitableBits, _ := model.NewNBitFromSlice(suitableLastBlock.NBits)
 	// If regtest we don't adjust the difficulty
 	if tSettings.ChainCfgParams.NoDifficultyAdjustment {

services/legacy/netsync/header_source.go → services/blockchain/branch_source.go

@@ -1,52 +1,59 @@
-package netsync
+package blockchain
 
 import (
 	"context"
 
 	"github.com/bsv-blockchain/go-bt/v2/chainhash"
 	"github.com/bsv-blockchain/teranode/errors"
 	"github.com/bsv-blockchain/teranode/model"
-	"github.com/bsv-blockchain/teranode/services/blockchain"
 	"github.com/bsv-blockchain/teranode/util"
 )
 
-// cachedHeader is what the header cache keeps about one header it holds above
-// the committed tip, so the difficulty calculator can read it as it reads a
-// stored block: the header, its height and its cumulative chain work, 32 bytes
-// big-endian, the form the store keeps chain_work in.
-type cachedHeader struct {
-	header    *model.BlockHeader
-	height    int32
-	chainWork [32]byte
+// BranchHeader is what a branch holds about one header above the trunk, so the
+// difficulty calculator can read it as it reads a stored block: the header, its
+// height and its cumulative chain work, 32 bytes big-endian, the form the store
+// keeps chain_work in.
+type BranchHeader struct {
+	Header    *model.BlockHeader
+	Height    uint32
+	ChainWork []byte
 }
 
-// branchSource is the blockchain.HeaderSource the header rules judge a cached
-// header with: lookup answers for headers held above the committed tip, and
-// trunk, the store or the blockchain client, answers for everything committed.
-// A walk that starts in the cache crosses into the trunk at the first parent the
-// cache does not hold, which is where every cached run is rooted.
+// branchSource is a HeaderSource over headers that are not stored: lookup
+// answers for the headers the branch holds, and trunk, the store or the
+// blockchain client, answers for everything committed. A walk that starts in
+// the branch crosses into the trunk at the first parent the branch does not
+// hold, which is where every branch is rooted.
 type branchSource struct {
-	trunk  blockchain.HeaderSource
-	lookup func(hash chainhash.Hash) (*cachedHeader, bool)
+	trunk  HeaderSource
+	lookup func(hash chainhash.Hash) (*BranchHeader, bool)
 }
 
-var _ blockchain.HeaderSource = (*branchSource)(nil)
+var _ HeaderSource = (*branchSource)(nil)
 
-func (s *branchSource) meta(c *cachedHeader) *model.BlockHeaderMeta {
-	return &model.BlockHeaderMeta{Height: uint32(c.height), ChainWork: append([]byte(nil), c.chainWork[:]...)} //nolint:gosec // a cached height is above the tip, never negative
+// NewBranchSource returns a HeaderSource that answers from lookup for the
+// headers it holds and from trunk for the rest. It lets CalcNextWorkRequiredFrom
+// judge a header whose ancestry is partly not stored yet, such as one in the
+// legacy header cache or one fetched from a peer during catchup.
+func NewBranchSource(trunk HeaderSource, lookup func(hash chainhash.Hash) (*BranchHeader, bool)) HeaderSource {
+	return &branchSource{trunk: trunk, lookup: lookup}
 }
 
-// GetBlockHeader answers from the cache, then the trunk.
+func (s *branchSource) meta(c *BranchHeader) *model.BlockHeaderMeta {
+	return &model.BlockHeaderMeta{Height: c.Height, ChainWork: append([]byte(nil), c.ChainWork...)}
+}
+
+// GetBlockHeader answers from the branch, then the trunk.
 func (s *branchSource) GetBlockHeader(ctx context.Context, hash *chainhash.Hash) (*model.BlockHeader, *model.BlockHeaderMeta, error) {
 	if c, ok := s.lookup(*hash); ok {
-		return c.header, s.meta(c), nil
+		return c.Header, s.meta(c), nil
 	}
 
 	return s.trunk.GetBlockHeader(ctx, hash)
 }
 
 // GetBlockHeaders returns hash and then its ancestors, newest first, taking
-// what the cache holds and asking the trunk for the rest in one call.
+// what the branch holds and asking the trunk for the rest in one call.
 func (s *branchSource) GetBlockHeaders(ctx context.Context, hash *chainhash.Hash, count uint64) ([]*model.BlockHeader, []*model.BlockHeaderMeta, error) {
 	headers := make([]*model.BlockHeader, 0, count)
 	metas := make([]*model.BlockHeaderMeta, 0, count)
@@ -59,9 +66,9 @@ func (s *branchSource) GetBlockHeaders(ctx context.Context, hash *chainhash.Hash
 			break
 		}
 
-		headers = append(headers, c.header)
+		headers = append(headers, c.Header)
 		metas = append(metas, s.meta(c))
-		cur = *c.header.HashPrevBlock
+		cur = *c.Header.HashPrevBlock
 	}
 
 	if remaining := count - uint64(len(headers)); remaining > 0 {
@@ -77,7 +84,7 @@ func (s *branchSource) GetBlockHeaders(ctx context.Context, hash *chainhash.Hash
 	return headers, metas, nil
 }
 
-// GetHashOfAncestorBlock walks depth parents back through the cache and hands
+// GetHashOfAncestorBlock walks depth parents back through the branch and hands
 // whatever depth is left to the trunk, which returns errors.ErrNotFound when the
 // chain is shorter, exactly as it would for a stored block.
 func (s *branchSource) GetHashOfAncestorBlock(ctx context.Context, hash *chainhash.Hash, depth int) (*chainhash.Hash, error) {
@@ -89,17 +96,17 @@ func (s *branchSource) GetHashOfAncestorBlock(ctx context.Context, hash *chainha
 			return s.trunk.GetHashOfAncestorBlock(ctx, &cur, depth)
 		}
 
-		cur = *c.header.HashPrevBlock
+		cur = *c.Header.HashPrevBlock
 		depth--
 	}
 
 	return &cur, nil
 }
 
 // GetSuitableBlock is the store's GetSuitableBlock (stores/blockchain/sql/
-// GetSuitableBlock.go) over the cache: hash and its two parents, in the store's
+// GetSuitableBlock.go) over the branch: hash and its two parents, in the store's
 // order (oldest first, its ORDER BY depth DESC), sorted by time with the same
-// sorting network, and the middle one returned. A hash the cache does not hold
+// sorting network, and the middle one returned. A hash the branch does not hold
 // is the trunk's to answer whole.
 func (s *branchSource) GetSuitableBlock(ctx context.Context, hash *chainhash.Hash) (*model.SuitableBlock, error) {
 	if _, ok := s.lookup(*hash); !ok {

services/blockvalidation/BlockValidation.go

@@ -2228,12 +2228,13 @@ func (u *BlockValidation) ValidateBlockWithOptions(ctx context.Context, block *m
 		// is true for EVERY height in 1..highest — and the two places that were supposed to cover
 		// the gap do not:
 		//
-		//   - Catch-up's header precheck (validateCatchupHeaderDifficulty) deliberately DEFERS every
-		//     header whose full 144-block window is not inside the fetched run — the first header
-		//     and roughly its first 146 successors — plus every pre-DAA header, and it returns
-		//     immediately on chains with no difficulty adjustment. Its own doc comment names this
-		//     check as the downstream cover for exactly those headers, so skipping here removed the
-		//     only check they ever got.
+		//   - Catch-up's header precheck (validateCatchupHeaderDifficulty) used to DEFER every header
+		//     whose full 144-block window was not inside the fetched run — the first header and
+		//     roughly its first 146 successors — plus every pre-DAA header, and it returned
+		//     immediately on chains with no difficulty adjustment, naming this check as the cover for
+		//     exactly those headers. It now judges every header against the store
+		//     (#2084), but only for blocks that arrive through catch-up, and this
+		//     check stays authoritative for the rest.
 		//   - Catch-up routes blocks certified by a checkpoint verified IN THIS RUN to
 		//     quickValidateBlock, which gates on catchupCtx.highestCheckpointHeight — the verified
 		//     height, not the configured one. That routing is not absolute: tryQuickValidation falls

services/blockvalidation/catchup.go

@@ -350,8 +350,10 @@ func (u *Server) catchup(ctx context.Context, blockUpTo *model.Block, peerID, ba
 
 	// Step 9.5: Validate header-chain difficulty (DAA) before fetching full blocks.
 	// Header-fetch validation only checks each header's PoW against its own nBits; this
-	// recomputes the DAA-required nBits from the header chain and rejects a peer that
-	// supplied a self-consistent low-difficulty chain.
+	// computes the required nBits for every header, reading the window below the common
+	// ancestor from the store, and rejects a peer that supplied a header mined against a
+	// target of its own choosing. It must stay ahead of step 10, whose subtree prewarm
+	// fetches block data before any block is validated.
 	if err = u.validateCatchupHeaderDifficulty(ctx, catchupCtx); err != nil {
 		return err
 	}

services/blockvalidation/catchup_ancestor_link_test.go

@@ -102,6 +102,7 @@ type ancestorLinkRun struct {
 	err             error
 	headersRequests int
 	blocksRequests  int
+	otherRequests   int // requests for anything but headers and /blocks/, such as /subtree_data
 	minedClears     int64
 	breakerFailures int    // failures counted on the peer's circuit breaker after the catchup
 	errorType       string // previousCatchupAttempt.ErrorType, how releaseCatchupLock classified err
@@ -130,6 +131,9 @@ const (
 	// header the batch before it ended on. The target is f2 on f1, which the peer never serves, so
 	// reaching the target never ends the fetch.
 	offerReplayBetweenBatches
+	// offerWrongBits: h1..h5 linked on b5, where h1 is mined against bits other than its parent's.
+	// Regtest never adjusts difficulty, so the bits every block must carry are its parent's.
+	offerWrongBits
 )
 
 // runAncestorLinkCatchup runs catchup() against a real sqlitememory blockchain store holding the
@@ -191,6 +195,11 @@ func runAncestorLinkCatchup(t *testing.T, shape offerShape) ancestorLinkRun {
 		offered = forkHeadersFrom(t, ancestor, 5, 6, 1000)
 	case offerFirstOnOther:
 		offered = forkHeadersFrom(t, otherBlockB, 5, 6, 1000)
+	case offerWrongBits:
+		h1 := forkHeadersFrom(t, ancestor, 1, 6, 1000)[0]
+		h1.Bits = nBitsFrom(t, "1f7fffff")
+		testhelpers.MineHeader(h1)
+		offered = append([]*model.BlockHeader{h1}, forkHeadersFrom(t, h1, 4, 7, 1000)...)
 	case offerBreakAtSecond:
 		offered = append(forkHeadersFrom(t, ancestor, 1, 6, 1000), forkHeadersFrom(t, otherBlockB, 4, 7, 2000)...)
 	case offerBreakBetweenBatches, offerReplayBetweenBatches:
@@ -259,6 +268,13 @@ func runAncestorLinkCatchup(t *testing.T, shape offerShape) ancestorLinkRun {
 		})
 	httpmock.RegisterResponder("GET", `=~^http://peer/blocks/.*`, httpmock.NewBytesResponder(500, nil))
 
+	var otherRequests atomic.Int64
+
+	httpmock.RegisterNoResponder(func(req *http.Request) (*http.Response, error) {
+		otherRequests.Add(1)
+		return httpmock.ConnectionFailure(req)
+	})
+
 	// Add the headers only the later batches carry, so the target is the newest header offered and
 	// the end-state check covers every offered block.
 	switch shape {
@@ -295,6 +311,7 @@ func runAncestorLinkCatchup(t *testing.T, shape offerShape) ancestorLinkRun {
 		errorType:       errorType,
 		headersRequests: int(headersRequests.Load()),
 		blocksRequests:  blocksRequests,
+		otherRequests:   int(otherRequests.Load()),
 		minedClears:     recorder.clears.Load(),
 		client:          client,
 		mainChain:       mainChain,
@@ -488,3 +505,19 @@ func TestBuildHeaderCache_RejectsFirstHeaderNotLinkedToAncestor(t *testing.T) {
 		require.Contains(t, err.Error(), "does not link to common ancestor")
 	})
 }
+
+// TestCatchup_HeaderWithWrongDifficultyIsRejectedBeforeAnyBodyFetch covers the #2084 fix
+// against a real sqlitememory blockchain store. The peer offers a fork of depth 5 on b5 whose first
+// header declares bits other than the ones the chain requires. Its whole adjustment window is in
+// the store, which the header precheck used to skip, leaving the subtree prewarm and the block
+// fetch to run first. Catchup must reject it at the header precheck, charge the peer as malicious,
+// and request nothing but headers.
+func TestCatchup_HeaderWithWrongDifficultyIsRejectedBeforeAnyBodyFetch(t *testing.T) {
+	run := runAncestorLinkCatchup(t, offerWrongBits)
+
+	require.Error(t, run.err)
+	require.True(t, errors.IsMaliciousResponseError(run.err), "wrong difficulty bits are a malicious peer: %v", run.err)
+	require.Contains(t, run.err.Error(), "incorrect difficulty bits")
+	require.Zero(t, run.otherRequests, "nothing but headers may be requested for a rejected offer")
+	requireRejectedWithoutSideEffects(t, run)
+}