From a3bed5e308999f28e8c16d4692f488511d04b85d Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Mon, 13 Jul 2026 20:54:04 -0700 Subject: [PATCH 1/4] 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 | 44 +++++++++++++++++-- blockchain/chainio_test.go | 86 ++++++++++++++++++++++++++++++++++---- 2 files changed, 117 insertions(+), 13 deletions(-) diff --git a/blockchain/chainio.go b/blockchain/chainio.go index 9ce58b12..9d183a72 100644 --- a/blockchain/chainio.go +++ b/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 } diff --git a/blockchain/chainio_test.go b/blockchain/chainio_test.go index 272bf61d..7fb961b4 100644 --- a/blockchain/chainio_test.go +++ b/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") } } From 2ddf73f39e72cfd57df0a5862434782b8a25cc8c Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Mon, 13 Jul 2026 20:54:15 -0700 Subject: [PATCH 2/4] psbt: avoid blocking reads and unbounded allocations in NewFromRawBytes In this commit, we address two issues with the strict parsing recently added to NewFromRawBytes. First, the trailing data check probed the caller supplied reader with a blocking one byte read. A reader without a Len method (net.Conn, io.Pipe) that stays open after delivering a complete packet would hang the parser forever. We now only enforce the check when the reader can report its remaining length without an additional read, which covers in-memory readers along with the decoded base64 path. Plain streams are left positioned directly after the packet, and the reader contract is now documented on NewFromRawBytes. Second, the base64 path read the entire input into memory before any validation ran, so a very large input could force an arbitrarily large allocation before the first validity check. We now bound the read to wire.MaxMessagePayload expanded by the base64 encoding overhead. Along the way, we simplify assertFullyConsumed down to the bytes.Reader case that all remaining callers use. --- psbt/psbt.go | 35 +++++++++++++++++++----- psbt/strict_tx_values_test.go | 50 +++++++++++++++++++++++++++++++++++ psbt/utils.go | 30 ++++++--------------- 3 files changed, 87 insertions(+), 28 deletions(-) diff --git a/psbt/psbt.go b/psbt/psbt.go index d8e39f8c..5a3f7783 100644 --- a/psbt/psbt.go +++ b/psbt/psbt.go @@ -185,6 +185,13 @@ 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. Trailing data is only detected +// when the reader can report its remaining length without blocking (such as +// bytes.Reader, or the base64 path); a plain stream is not probed past the +// packet, so the reader is left positioned directly after it. +// // 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) { @@ -324,20 +331,37 @@ func NewFromRawBytes(r io.Reader, b64 bool) (*Packet, error) { return nil, err } - if err := assertFullyConsumed(r); err != nil { - return nil, err + // Reject any trailing data after the packet when the reader is able + // to report it without an additional read. This covers in-memory + // readers as well as the decoded base64 path above. Plain 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 } +// maxBase64PsbtSize is the maximum number of base64 characters accepted when +// decoding a PSBT. It is wire.MaxMessagePayload, the largest payload the +// wire protocol will carry, expanded by the 4/3 base64 encoding overhead. It +// bounds the memory allocated for a caller-supplied reader before any +// validation runs. +const maxBase64PsbtSize = 4 * ((wire.MaxMessagePayload + 2) / 3) + // 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) + // Bound the read so an unbounded stream cannot force an arbitrarily + // large allocation before validation. + encoded, err := io.ReadAll(io.LimitReader(r, maxBase64PsbtSize+1)) if err != nil { return nil, err } + if len(encoded) > maxBase64PsbtSize { + return nil, ErrInvalidPsbtFormat + } // Go's strict base64 decoder still ignores CR/LF. Reject them before // decoding so base64 PSBT parsing matches the RFC4648 alphabet exactly. @@ -345,13 +369,12 @@ func decodeBase64Strict(r io.Reader) ([]byte, error) { return nil, ErrInvalidPsbtFormat } - decoded := make([]byte, base64.StdEncoding.DecodedLen(len(encoded))) - n, err := base64.StdEncoding.Strict().Decode(decoded, encoded) + decoded, err := base64.StdEncoding.Strict().AppendDecode(nil, encoded) if err != nil { return nil, ErrInvalidPsbtFormat } - return decoded[:n], nil + return decoded, nil } // Serialize creates a binary serialization of the referenced Packet struct diff --git a/psbt/strict_tx_values_test.go b/psbt/strict_tx_values_test.go index 83ed6bdd..9122aa2d 100644 --- a/psbt/strict_tx_values_test.go +++ b/psbt/strict_tx_values_test.go @@ -3,7 +3,10 @@ package psbt import ( "bytes" "encoding/base64" + "errors" + "io" "testing" + "testing/iotest" "github.com/btcsuite/btcd/wire/v2" "github.com/stretchr/testify/require" @@ -172,6 +175,53 @@ 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) +} + +// TestRejectsOversizedBase64Packet verifies that base64 input larger than +// the maximum accepted size is rejected instead of being fully decoded. +func TestRejectsOversizedBase64Packet(t *testing.T) { + oversized := bytes.Repeat([]byte{'A'}, maxBase64PsbtSize+1) + + // The erroring sentinel after the oversized bytes pins the bound + // itself: with the size limit in place the reader is never read past + // maxBase64PsbtSize+1 bytes, so the sentinel stays untouched. Without + // the limit, the full read would surface the sentinel error instead + // of ErrInvalidPsbtFormat. + stream := io.MultiReader( + bytes.NewReader(oversized), + iotest.ErrReader(errors.New("read past size bound")), + ) + + _, err := NewFromRawBytes(stream, true) + require.ErrorIs(t, err, ErrInvalidPsbtFormat) +} + // TestRejectsNonCanonicalBase64Packet verifies that base64 PSBT input rejects // whitespace, bad padding, and extra decoded packet bytes. func TestRejectsNonCanonicalBase64Packet(t *testing.T) { diff --git a/psbt/utils.go b/psbt/utils.go index baf75583..289e596c 100644 --- a/psbt/utils.go +++ b/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 @@ -318,9 +303,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) From 4ac2e42511c720cd882d26d96738a5b98413fd8f Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Mon, 13 Jul 2026 21:17:44 -0700 Subject: [PATCH 3/4] multi: parse own-DB blocks leniently in getblock and indexer init In this commit, we extend the lenient database block parser to the remaining call sites that re-read blocks from the node's own database. The getblock RPC now copies FetchBlock bytes before its read transaction ends, since database buffers are not valid outside that lifetime. We then strip any legacy trailing data and serve the exact block serialization at every verbosity level. A regression database invalidates its buffer as View returns, pinning the required copy. The index manager uses the same parser while rolling an index tip back to the main chain. We also place DBBlockFromBytes before its first caller and log legacy trailing data at debug level, since a frequently fetched dirty block should not flood operator logs. --- blockchain/chainio.go | 58 +++++++++++++++++----------------- blockchain/indexers/manager.go | 4 ++- rpcserver.go | 36 +++++++++++++++------ rpcserver_test.go | 55 ++++++++++++++++++++++++++++++++ 4 files changed, 113 insertions(+), 40 deletions(-) diff --git a/blockchain/chainio.go b/blockchain/chainio.go index 9d183a72..be5c9a20 100644 --- a/blockchain/chainio.go +++ b/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. @@ -1374,35 +1403,6 @@ 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. diff --git a/blockchain/indexers/manager.go b/blockchain/indexers/manager.go index 28f608fb..533f0073 100644 --- a/blockchain/indexers/manager.go +++ b/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 } diff --git a/rpcserver.go b/rpcserver.go index fb5f0665..dcc97fc9 100644 --- a/rpcserver.go +++ b/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 { diff --git a/rpcserver_test.go b/rpcserver_test.go index e4625642..d3e9da06 100644 --- a/rpcserver_test.go +++ b/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) { From e333817d9dbb1e5ecf1ee556c057b4c9f5833770 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 21 Jul 2026 16:06:30 -0700 Subject: [PATCH 4/4] psbt: decode base64 packets incrementally In this commit, we decode base64 PSBT packets as a stream instead of reading the full encoded input into memory first. The previous aggregate bound reused wire.MaxMessagePayload, even though BIP-174 doesn't bind PSBT packet size to the P2P message limit. This made the base64 path reject packets that the raw path accepted. We now feed decoded bytes through the same per-field parser used for raw packets, while retaining strict base64 and trailing-data checks. We also add a regression test with nine individually valid 4,000,000-byte unknown values to pin raw and base64 parsing to the same behavior. --- psbt/psbt.go | 120 ++++++++++++++++++++++------------ psbt/strict_tx_values_test.go | 63 +++++++++++++----- 2 files changed, 126 insertions(+), 57 deletions(-) diff --git a/psbt/psbt.go b/psbt/psbt.go index 5a3f7783..4f61858a 100644 --- a/psbt/psbt.go +++ b/psbt/psbt.go @@ -187,20 +187,17 @@ func NewFromUnsignedTx(tx *wire.MsgTx) (*Packet, error) { // // 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. Trailing data is only detected -// when the reader can report its remaining length without blocking (such as -// bytes.Reader, or the base64 path); a plain stream is not probed past the -// packet, so the reader is left positioned directly after it. +// 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 @@ -331,50 +328,91 @@ func NewFromRawBytes(r io.Reader, b64 bool) (*Packet, error) { return nil, err } - // Reject any trailing data after the packet when the reader is able - // to report it without an additional read. This covers in-memory - // readers as well as the decoded base64 path above. Plain 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 + 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 } -// maxBase64PsbtSize is the maximum number of base64 characters accepted when -// decoding a PSBT. It is wire.MaxMessagePayload, the largest payload the -// wire protocol will carry, expanded by the 4/3 base64 encoding overhead. It -// bounds the memory allocated for a caller-supplied reader before any -// validation runs. -const maxBase64PsbtSize = 4 * ((wire.MaxMessagePayload + 2) / 3) +// canonicalBase64Reader rejects the CR and LF bytes that encoding/base64 +// otherwise ignores while decoding. +type canonicalBase64Reader struct { + io.Reader +} -// decodeBase64Strict decodes an RFC4648 base64 stream without permitting -// whitespace and with '=' allowed only as final padding. -func decodeBase64Strict(r io.Reader) ([]byte, error) { - // Bound the read so an unbounded stream cannot force an arbitrarily - // large allocation before validation. - encoded, err := io.ReadAll(io.LimitReader(r, maxBase64PsbtSize+1)) - if err != nil { - return nil, err - } - if len(encoded) > maxBase64PsbtSize { - return nil, ErrInvalidPsbtFormat +// 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, err := base64.StdEncoding.Strict().AppendDecode(nil, 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, 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 diff --git a/psbt/strict_tx_values_test.go b/psbt/strict_tx_values_test.go index 9122aa2d..25ac2e4d 100644 --- a/psbt/strict_tx_values_test.go +++ b/psbt/strict_tx_values_test.go @@ -3,10 +3,8 @@ package psbt import ( "bytes" "encoding/base64" - "errors" "io" "testing" - "testing/iotest" "github.com/btcsuite/btcd/wire/v2" "github.com/stretchr/testify/require" @@ -203,23 +201,56 @@ func TestStreamReaderNotProbedPastPacket(t *testing.T) { require.Equal(t, []byte{0xde, 0xad}, trailing) } -// TestRejectsOversizedBase64Packet verifies that base64 input larger than -// the maximum accepted size is rejected instead of being fully decoded. -func TestRejectsOversizedBase64Packet(t *testing.T) { - oversized := bytes.Repeat([]byte{'A'}, maxBase64PsbtSize+1) +// 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) - // The erroring sentinel after the oversized bytes pins the bound - // itself: with the size limit in place the reader is never read past - // maxBase64PsbtSize+1 bytes, so the sentinel stays untouched. Without - // the limit, the full read would surface the sentinel error instead - // of ErrInvalidPsbtFormat. - stream := io.MultiReader( - bytes.NewReader(oversized), - iotest.ErrReader(errors.New("read past size bound")), + 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) - _, err := NewFromRawBytes(stream, true) - require.ErrorIs(t, err, ErrInvalidPsbtFormat) + 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