Merge pull request #2575 from Roasbeef/trailing-relax

multi: relax trailing byte handling for DB blocks and PSBT readers

blockchain/chainio.go

@@ -1168,6 +1168,35 @@ func (b *BlockChain) createChainState() error {
 	return err
 }
 
+// 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.Debugf("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
+}
+
 // initChainState attempts to load and initialize the chain state from the
 // database.  When the db does not yet contain any chain state, both it and the
 // chain state are initialized to the genesis block.
@@ -1277,7 +1306,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 +1330,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,
@@ -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")
 	}
 }
 

blockchain/indexers/manager.go

@@ -309,7 +309,9 @@ func (m *Manager) Init(chain *blockchain.BlockChain, interrupt <-chan struct{})
 				if err != nil {
 					return err
 				}
-				block, err = btcutil.NewBlockFromBytes(blockBytes)
+				block, err = blockchain.DBBlockFromBytes(
+					blockBytes, *hash,
+				)
 				if err != nil {
 					return err
 				}

psbt/psbt.go

@@ -185,15 +185,19 @@ func NewFromUnsignedTx(tx *wire.MsgTx) (*Packet, error) {
 // argument b64 is true, the passed byte slice is decoded from base64 encoding
 // before processing.
 //
+// The parsing is strict: base64 input must not contain whitespace or any
+// characters outside the RFC4648 standard alphabet, and any data after the
+// packet results in ErrInvalidPsbtFormat. For raw input, trailing data is only
+// detected when the reader can report its remaining length without blocking
+// (such as bytes.Reader); a plain raw stream is not probed past the packet, so
+// the reader is left positioned directly after it. Base64 input is decoded
+// incrementally and read through its canonical end.
+//
 // NOTE: To create a Packet from one's own data, rather than reading in a
 // serialization from a counterparty, one should use a psbt.New.
 func NewFromRawBytes(r io.Reader, b64 bool) (*Packet, error) {
 	if b64 {
-		decoded, err := decodeBase64Strict(r)
-		if err != nil {
-			return nil, err
-		}
-		r = bytes.NewReader(decoded)
+		r = newStrictBase64Decoder(r)
 	}
 
 	// The Packet struct does not store the fixed magic bytes, but they
@@ -324,34 +328,91 @@ func NewFromRawBytes(r io.Reader, b64 bool) (*Packet, error) {
 		return nil, err
 	}
 
-	if err := assertFullyConsumed(r); err != nil {
-		return nil, err
+	if b64 {
+		if err := assertBase64FullyConsumed(r); err != nil {
+			return nil, err
+		}
+	} else {
+		// Reject any trailing data after the packet when a raw reader can
+		// report it without an additional read. Plain raw streams are not
+		// probed, as a read for EOF could block forever on an open
+		// connection that has already delivered a complete packet.
+		if lr, ok := r.(interface{ Len() int }); ok && lr.Len() > 0 {
+			return nil, ErrInvalidPsbtFormat
+		}
 	}
 
 	return &newPsbt, nil
 }
 
-// decodeBase64Strict decodes an RFC4648 base64 stream without permitting
-// whitespace and with '=' allowed only as final padding.
-func decodeBase64Strict(r io.Reader) ([]byte, error) {
-	encoded, err := io.ReadAll(r)
-	if err != nil {
-		return nil, err
+// canonicalBase64Reader rejects the CR and LF bytes that encoding/base64
+// otherwise ignores while decoding.
+type canonicalBase64Reader struct {
+	io.Reader
+}
+
+// Read returns encoded bytes only when they use the canonical RFC4648
+// alphabet.
+func (r *canonicalBase64Reader) Read(p []byte) (int, error) {
+	n, err := r.Reader.Read(p)
+	if bytes.ContainsAny(p[:n], "\r\n") {
+		return 0, ErrInvalidPsbtFormat
 	}
 
-	// Go's strict base64 decoder still ignores CR/LF. Reject them before
-	// decoding so base64 PSBT parsing matches the RFC4648 alphabet exactly.
-	if bytes.ContainsAny(encoded, "\r\n") {
-		return nil, ErrInvalidPsbtFormat
+	return n, err
+}
+
+// strictBase64Decoder maps base64 syntax and truncation errors to
+// ErrInvalidPsbtFormat while preserving other errors returned by the
+// caller-supplied reader.
+type strictBase64Decoder struct {
+	io.Reader
+}
+
+// Read returns incrementally decoded bytes.
+func (d *strictBase64Decoder) Read(p []byte) (int, error) {
+	n, err := d.Reader.Read(p)
+	if err == nil || errors.Is(err, io.EOF) {
+		return n, err
 	}
 
-	decoded := make([]byte, base64.StdEncoding.DecodedLen(len(encoded)))
-	n, err := base64.StdEncoding.Strict().Decode(decoded, encoded)
-	if err != nil {
-		return nil, ErrInvalidPsbtFormat
+	var corruptInput base64.CorruptInputError
+	if errors.As(err, &corruptInput) ||
+		errors.Is(err, io.ErrUnexpectedEOF) ||
+		errors.Is(err, ErrInvalidPsbtFormat) {
+
+		return n, ErrInvalidPsbtFormat
 	}
 
-	return decoded[:n], nil
+	return n, err
+}
+
+// newStrictBase64Decoder returns a streaming RFC4648 base64 decoder that
+// rejects whitespace and only accepts '=' as final padding.
+func newStrictBase64Decoder(r io.Reader) io.Reader {
+	canonicalReader := &canonicalBase64Reader{Reader: r}
+	decoder := base64.NewDecoder(
+		base64.StdEncoding.Strict(), canonicalReader,
+	)
+
+	return &strictBase64Decoder{Reader: decoder}
+}
+
+// assertBase64FullyConsumed verifies the end of the base64 envelope and
+// rejects decoded data after the PSBT packet.
+func assertBase64FullyConsumed(r io.Reader) error {
+	var trailing [1]byte
+	_, err := io.ReadFull(r, trailing[:])
+	switch {
+	case err == nil:
+		return ErrInvalidPsbtFormat
+
+	case errors.Is(err, io.EOF):
+		return nil
+
+	default:
+		return err
+	}
 }
 
 // Serialize creates a binary serialization of the referenced Packet struct

psbt/strict_tx_values_test.go

@@ -3,6 +3,7 @@ package psbt
 import (
 	"bytes"
 	"encoding/base64"
+	"io"
 	"testing"
 
 	"github.com/btcsuite/btcd/wire/v2"
@@ -172,6 +173,86 @@ func TestRejectsTrailingDataAfterPacket(t *testing.T) {
 	require.ErrorIs(t, err, ErrInvalidPsbtFormat)
 }
 
+// TestStreamReaderNotProbedPastPacket verifies that a reader that cannot
+// report its remaining length is not read past the end of the packet: the
+// packet parses successfully and any subsequent data remains unread, so
+// parsing never blocks on an open stream.
+func TestStreamReaderNotProbedPastPacket(t *testing.T) {
+	unsignedTx, prevTx := strictnessTxPair(t)
+	rawPacket := strictnessPSBT(
+		t,
+		serializeTxForStrictness(t, unsignedTx, true),
+		serializeTxForStrictness(t, prevTx, false),
+	)
+
+	// io.MultiReader hides the Len method of the underlying bytes.Reader,
+	// mimicking a plain stream.
+	stream := io.MultiReader(bytes.NewReader(
+		append(append([]byte{}, rawPacket...), 0xde, 0xad),
+	))
+
+	_, err := NewFromRawBytes(stream, false)
+	require.NoError(t, err)
+
+	// The bytes following the packet must still be readable from the
+	// stream.
+	trailing, err := io.ReadAll(stream)
+	require.NoError(t, err)
+	require.Equal(t, []byte{0xde, 0xad}, trailing)
+}
+
+// TestAcceptsBase64PacketAboveWirePayloadLimit verifies that base64 parsing
+// accepts valid PSBT packets larger than the peer-to-peer wire message payload
+// limit.
+func TestAcceptsBase64PacketAboveWirePayloadLimit(t *testing.T) {
+	unsignedTx, _ := strictnessTxPair(t)
+	packet, err := NewFromUnsignedTx(unsignedTx)
+	require.NoError(t, err)
+
+	value := bytes.Repeat([]byte{0x01}, MaxPsbtValueLength)
+	for i := range 9 {
+		packet.Unknowns = append(packet.Unknowns, &Unknown{
+			Key:   []byte{0xfc, byte(i)},
+			Value: value,
+		})
+	}
+
+	var rawPacket bytes.Buffer
+	require.NoError(t, packet.Serialize(&rawPacket))
+	require.Greater(t, rawPacket.Len(), wire.MaxMessagePayload)
+
+	rawParsed, err := NewFromRawBytes(
+		bytes.NewReader(rawPacket.Bytes()), false,
+	)
+	require.NoError(t, err)
+	require.Len(t, rawParsed.Unknowns, 9)
+
+	encodedReader, encodedWriter := io.Pipe()
+	encodeDone := make(chan error, 1)
+	go func() {
+		encoder := base64.NewEncoder(
+			base64.StdEncoding, encodedWriter,
+		)
+		_, encodeErr := io.Copy(encoder, bytes.NewReader(
+			rawPacket.Bytes(),
+		))
+		if closeErr := encoder.Close(); encodeErr == nil {
+			encodeErr = closeErr
+		}
+		_ = encodedWriter.CloseWithError(encodeErr)
+		encodeDone <- encodeErr
+	}()
+
+	parsedPacket, parseErr := NewFromRawBytes(encodedReader, true)
+	_ = encodedReader.Close()
+	encodeErr := <-encodeDone
+	if parseErr == nil {
+		require.NoError(t, encodeErr)
+	}
+	require.NoError(t, parseErr)
+	require.Len(t, parsedPacket.Unknowns, 9)
+}
+
 // TestRejectsNonCanonicalBase64Packet verifies that base64 PSBT input rejects
 // whitespace, bad padding, and extra decoded packet bytes.
 func TestRejectsNonCanonicalBase64Packet(t *testing.T) {

psbt/utils.go

@@ -279,27 +279,12 @@ func getKey(r io.Reader) (int, []byte, error) {
 
 // assertFullyConsumed returns ErrInvalidPsbtFormat if r still has bytes
 // available after parsing.
-func assertFullyConsumed(r io.Reader) error {
-	if lr, ok := r.(interface{ Len() int }); ok {
-		if lr.Len() > 0 {
-			return ErrInvalidPsbtFormat
-		}
-
-		return nil
-	}
-
-	var trailing [1]byte
-	_, err := io.ReadFull(r, trailing[:])
-	switch {
-	case err == nil:
+func assertFullyConsumed(r *bytes.Reader) error {
+	if r.Len() > 0 {
 		return ErrInvalidPsbtFormat
-
-	case errors.Is(err, io.EOF):
-		return nil
-
-	default:
-		return err
 	}
+
+	return nil
 }
 
 // readTxOut parses a transaction output value and requires the full value to
@@ -325,9 +310,10 @@ func readTxOut(txout []byte) (*wire.TxOut, error) {
 	return txOut, nil
 }
 
-// readTransaction parses a transaction value and requires the full value to be
-// consumed. PSBT transaction-valued fields contain exactly one network
-// serialized transaction, not a transaction prefix with arbitrary trailing data.
+// readTransaction parses a transaction value and requires the full value to
+// be consumed. PSBT transaction-valued fields contain exactly one network
+// serialized transaction, not a transaction prefix with arbitrary trailing
+// data.
 func readTransaction(txBytes []byte, noWitness bool) (*wire.MsgTx, error) {
 	tx := wire.NewMsgTx(2)
 	reader := bytes.NewReader(txBytes)

rpcserver.go

@@ -1082,9 +1082,17 @@ func handleGetBlock(s *rpcServer, cmd interface{}, closeChan <-chan struct{}) (i
 	}
 	var blkBytes []byte
 	err = s.cfg.DB.View(func(dbTx database.Tx) error {
-		var err error
-		blkBytes, err = dbTx.FetchBlock(hash)
-		return err
+		dbBlockBytes, err := dbTx.FetchBlock(hash)
+		if err != nil {
+			return err
+		}
+
+		// FetchBlock bytes are only valid for the lifetime of the
+		// transaction. Copy them before returning from the view so the
+		// lenient parser below owns its backing memory.
+		blkBytes = append([]byte(nil), dbBlockBytes...)
+
+		return nil
 	})
 	if err != nil {
 		return nil, &btcjson.RPCError{
@@ -1092,6 +1100,21 @@ func handleGetBlock(s *rpcServer, cmd interface{}, closeChan <-chan struct{}) (i
 			Message: "Block not found",
 		}
 	}
+	// Deserialize the block.  Blocks read back from the node's own
+	// database are parsed leniently: trailing bytes persisted by older
+	// btcd versions are stripped rather than treated as an error, so the
+	// bytes served below are always the exact block serialization.
+	blk, err := blockchain.DBBlockFromBytes(blkBytes, *hash)
+	if err != nil {
+		context := "Failed to deserialize block"
+		return nil, internalRPCError(err.Error(), context)
+	}
+	blkBytes, err = blk.Bytes()
+	if err != nil {
+		context := "Failed to serialize block"
+		return nil, internalRPCError(err.Error(), context)
+	}
+
 	// If verbosity is 0, return the serialized block as a hex encoded string.
 	if c.Verbosity != nil && *c.Verbosity == 0 {
 		return hex.EncodeToString(blkBytes), nil
@@ -1099,13 +1122,6 @@ func handleGetBlock(s *rpcServer, cmd interface{}, closeChan <-chan struct{}) (i
 
 	// Otherwise, generate the JSON object and return it.
 
-	// Deserialize the block.
-	blk, err := btcutil.NewBlockFromBytes(blkBytes)
-	if err != nil {
-		context := "Failed to deserialize block"
-		return nil, internalRPCError(err.Error(), context)
-	}
-
 	// Get the block height from chain.
 	blockHeight, err := s.cfg.Chain.BlockHeightByHash(hash)
 	if err != nil {

rpcserver_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/btcsuite/btcd/btcutil/v2"
 	"github.com/btcsuite/btcd/chaincfg/v2"
 	"github.com/btcsuite/btcd/chainhash/v2"
+	"github.com/btcsuite/btcd/database"
 	"github.com/btcsuite/btcd/mempool"
 	"github.com/btcsuite/btcd/wire/v2"
 	"github.com/stretchr/testify/mock"
@@ -111,6 +112,60 @@ func blockHexWithTrailingByte(t *testing.T) string {
 	return hex.EncodeToString(append(block.Bytes(), 0x00))
 }
 
+// invalidatingBlockDB clears fetched block bytes as soon as its managed view
+// ends. This models database backends whose zero-copy buffers are only valid
+// for the lifetime of a transaction.
+type invalidatingBlockDB struct {
+	database.DB
+	blockBytes []byte
+}
+
+// View runs the callback with a transaction backed by the configured block
+// bytes, then invalidates those bytes before returning.
+func (d *invalidatingBlockDB) View(fn func(database.Tx) error) error {
+	err := fn(&invalidatingBlockTx{blockBytes: d.blockBytes})
+	clear(d.blockBytes)
+
+	return err
+}
+
+// invalidatingBlockTx returns the parent database's transaction-scoped block
+// bytes.
+type invalidatingBlockTx struct {
+	database.Tx
+	blockBytes []byte
+}
+
+// FetchBlock returns bytes that are invalidated when the enclosing view ends.
+func (t *invalidatingBlockTx) FetchBlock(*chainhash.Hash) ([]byte, error) {
+	return t.blockBytes, nil
+}
+
+// TestHandleGetBlockCopiesTransactionBytes verifies getblock does not retain
+// transaction-scoped database memory after its managed view ends.
+func TestHandleGetBlockCopiesTransactionBytes(t *testing.T) {
+	t.Parallel()
+
+	var serializedBlock bytes.Buffer
+	err := chaincfg.MainNetParams.GenesisBlock.Serialize(&serializedBlock)
+	require.NoError(t, err)
+
+	wantBytes := serializedBlock.Bytes()
+	dbBytes := append([]byte(nil), wantBytes...)
+	dbBytes = append(dbBytes, 0x00)
+	db := &invalidatingBlockDB{blockBytes: dbBytes}
+
+	verbosity := 0
+	cmd := btcjson.NewGetBlockCmd(
+		chaincfg.MainNetParams.GenesisHash.String(), &verbosity,
+	)
+	result, err := handleGetBlock(
+		&rpcServer{cfg: rpcserverConfig{DB: db}}, cmd, make(chan struct{}),
+	)
+	require.NoError(t, err)
+	require.Equal(t, hex.EncodeToString(wantBytes), result)
+}
+
 // TestHandleSendRawTransactionRejectsTrailingBytes ensures sendrawtransaction
 // rejects byte strings that contain a valid transaction plus trailing data.
 func TestHandleSendRawTransactionRejectsTrailingBytes(t *testing.T) {