blockchain: tolerate trailing bytes when loading stored blocks

In this commit, we relax the strict block deserialization introduced as
part of the trailing byte hardening. Databases written by older
versions of btcd may have persisted blocks with trailing bytes, so
refusing to load them would prevent a node from ever starting (or
serving such a block) after an upgrade, with no recovery path short of
a full resync.

We instead introduce a new dbBlockFromBytes helper, used by both
initChainState and dbFetchBlockByNode, that deserializes the block
leniently: any trailing bytes are logged, ignored, and excluded from
the serialization cached on the returned block, so downstream consumers
of the raw bytes never observe them.

blockchain/chainio.go

@@ -1277,7 +1277,7 @@ func (b *BlockChain) initChainState() error {
 		if err != nil {
 			return err
 		}
-		block, err := btcutil.NewBlockFromBytes(blockBytes)
+		block, err := DBBlockFromBytes(blockBytes, state.hash)
 		if err != nil {
 			return err
 		}
@@ -1301,8 +1301,15 @@ func (b *BlockChain) initChainState() error {
 			}
 		}
 
-		// Initialize the state related to the best block.
-		blockSize := uint64(len(blockBytes))
+		// Initialize the state related to the best block.  The block
+		// bytes are re-derived from the block itself so any trailing
+		// bytes ignored during deserialization are excluded from the
+		// recorded size.
+		serializedBlock, err := block.Bytes()
+		if err != nil {
+			return err
+		}
+		blockSize := uint64(len(serializedBlock))
 		blockWeight := uint64(GetBlockWeight(block))
 		numTxns := uint64(len(block.MsgBlock().Transactions))
 		b.stateSnapshot = newBestState(tip, blockSize, blockWeight,
@@ -1367,6 +1374,35 @@ func dbFetchHeaderByHeight(dbTx database.Tx, height int32) (*wire.BlockHeader, e
 	return dbFetchHeaderByHash(dbTx, hash)
 }
 
+// DBBlockFromBytes deserializes a block fetched from the local database,
+// tolerating trailing bytes rather than rejecting them outright.  Databases
+// written by older btcd versions may have persisted blocks with trailing
+// bytes, and failing here would make such blocks permanently unreadable.
+// Instead, any trailing bytes are logged, ignored, and excluded from the
+// serialization cached in the returned block.
+//
+// This lenient parsing is only appropriate for blocks read back from the
+// node's own database.  Blocks from external sources (p2p, RPC) should be
+// parsed with the strict btcutil.NewBlockFromBytes instead.
+func DBBlockFromBytes(blockBytes []byte, hash chainhash.Hash) (*btcutil.Block,
+	error) {
+
+	blockReader := bytes.NewReader(blockBytes)
+	var msgBlock wire.MsgBlock
+	if err := msgBlock.Deserialize(blockReader); err != nil {
+		return nil, err
+	}
+	if trailing := blockReader.Len(); trailing > 0 {
+		log.Warnf("Block %v has %d trailing bytes in the database; "+
+			"ignoring them", hash, trailing)
+		blockBytes = blockBytes[:len(blockBytes)-trailing]
+	}
+
+	// Cache the exact serialization on the block so downstream consumers
+	// of the raw bytes never observe the trailing bytes.
+	return btcutil.NewBlockFromBlockAndBytes(&msgBlock, blockBytes), nil
+}
+
 // dbFetchBlockByNode uses an existing database transaction to retrieve the
 // raw block for the provided node, deserialize it, and return a btcutil.Block
 // with the height set.
@@ -1378,7 +1414,7 @@ func dbFetchBlockByNode(dbTx database.Tx, node *blockNode) (*btcutil.Block, erro
 	}
 
 	// Create the encapsulated block and set the height appropriately.
-	block, err := btcutil.NewBlockFromBytes(blockBytes)
+	block, err := DBBlockFromBytes(blockBytes, node.hash)
 	if err != nil {
 		return nil, err
 	}

blockchain/chainio_test.go

@@ -9,10 +9,10 @@ import (
 	"errors"
 	"math/big"
 	"reflect"
-	"strings"
 	"testing"
 
 	"github.com/btcsuite/btcd/btcutil/v2"
+	"github.com/btcsuite/btcd/chaincfg/v2"
 	"github.com/btcsuite/btcd/database"
 	"github.com/btcsuite/btcd/txscript/v2"
 	"github.com/btcsuite/btcd/wire/v2"
@@ -40,11 +40,13 @@ func TestErrNotInMainChain(t *testing.T) {
 	}
 }
 
-// TestInitChainStateRejectsTrailingBestBlockBytes ensures startup rejects a
+// TestInitChainStateToleratesTrailingBestBlockBytes ensures startup loads a
 // stored best block whose bytes contain a valid block plus trailing data.
-func TestInitChainStateRejectsTrailingBestBlockBytes(t *testing.T) {
+// Databases written by older btcd versions may contain such blocks, so
+// rejecting them would prevent the node from ever starting.
+func TestInitChainStateToleratesTrailingBestBlockBytes(t *testing.T) {
 	chain, params, teardown := utxoCacheTestChain(
-		"TestInitChainStateRejectsTrailingBestBlockBytes")
+		"TestInitChainStateToleratesTrailingBestBlockBytes")
 	defer teardown()
 
 	tip := btcutil.NewBlock(params.GenesisBlock)
@@ -72,17 +74,83 @@ func TestInitChainStateRejectsTrailingBestBlockBytes(t *testing.T) {
 		t.Fatalf("failed to process block: %v", err)
 	}
 
-	_, err = New(&Config{
+	restarted, err := New(&Config{
 		DB:          chain.db,
 		ChainParams: params,
 		TimeSource:  NewMedianTime(),
 		SigCache:    txscript.NewSigCache(1000),
 	})
-	if err == nil {
-		t.Fatal("expected trailing best block bytes to fail startup")
+	if err != nil {
+		t.Fatalf("expected trailing best block bytes to be "+
+			"tolerated at startup, got: %v", err)
 	}
-	if !strings.Contains(err.Error(), "trailing bytes") {
-		t.Fatalf("expected trailing byte error, got: %v", err)
+
+	// The best state must reflect the stored block, with the trailing
+	// byte excluded from the recorded block size.
+	snapshot := restarted.BestSnapshot()
+	if snapshot.Hash != *block.Hash() {
+		t.Fatalf("unexpected best block hash - got %v, want %v",
+			snapshot.Hash, block.Hash())
+	}
+	wantSize := uint64(len(serialized.Bytes()))
+	if snapshot.BlockSize != wantSize {
+		t.Fatalf("unexpected best block size - got %d, want %d",
+			snapshot.BlockSize, wantSize)
+	}
+}
+
+// TestDBBlockFromBytes ensures database block parsing strips trailing bytes
+// from the cached serialization, passes exact serializations through
+// untouched, and still rejects truncated blocks.
+func TestDBBlockFromBytes(t *testing.T) {
+	t.Parallel()
+
+	params := &chaincfg.MainNetParams
+	var serialized bytes.Buffer
+	err := params.GenesisBlock.Serialize(&serialized)
+	if err != nil {
+		t.Fatalf("failed to serialize block: %v", err)
+	}
+	cleanBytes := serialized.Bytes()
+	wantHash := params.GenesisBlock.BlockHash()
+
+	// A block with trailing bytes must parse, and the cached
+	// serialization must exclude the trailing data.
+	block, err := DBBlockFromBytes(
+		append(append([]byte(nil), cleanBytes...), 0x00), wantHash,
+	)
+	if err != nil {
+		t.Fatalf("failed to parse block with trailing bytes: %v", err)
+	}
+	gotBytes, err := block.Bytes()
+	if err != nil {
+		t.Fatalf("failed to serialize parsed block: %v", err)
+	}
+	if !bytes.Equal(gotBytes, cleanBytes) {
+		t.Fatal("cached serialization includes trailing bytes")
+	}
+	if *block.Hash() != wantHash {
+		t.Fatalf("unexpected block hash - got %v, want %v",
+			block.Hash(), wantHash)
+	}
+
+	// An exact serialization must pass through untouched.
+	block, err = DBBlockFromBytes(cleanBytes, wantHash)
+	if err != nil {
+		t.Fatalf("failed to parse exact block: %v", err)
+	}
+	gotBytes, err = block.Bytes()
+	if err != nil {
+		t.Fatalf("failed to serialize parsed block: %v", err)
+	}
+	if !bytes.Equal(gotBytes, cleanBytes) {
+		t.Fatal("exact serialization was not preserved")
+	}
+
+	// A truncated block must still fail to parse.
+	_, err = DBBlockFromBytes(cleanBytes[:len(cleanBytes)-1], wantHash)
+	if err == nil {
+		t.Fatal("expected truncated block to fail to parse")
 	}
 }