multi: relax trailing byte handling for DB blocks and PSBT readers#2575
Open
Roasbeef wants to merge 3 commits into
Open
multi: relax trailing byte handling for DB blocks and PSBT readers#2575Roasbeef wants to merge 3 commits into
Roasbeef wants to merge 3 commits into
Conversation
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.
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.
In this commit, we extend the lenient database block parsing to the two remaining call sites that re-parse blocks read back from the node's own database with the strict btcutil.NewBlockFromBytes. The getblock RPC fetched the raw block bytes directly, so a legacy block with trailing bytes would fail with an internal error at verbosity >= 1, while verbosity 0 served the dirty bytes (which any hardened consumer now rejects) and reported a size that included them. We now parse via the newly exported blockchain.DBBlockFromBytes and serve the exact block serialization at all verbosity levels. The index manager's init path loads orphaned blocks directly from the database when rolling an index tip back to the main chain, and would fail startup permanently on a legacy block with trailing bytes. It now uses the same lenient parse.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this PR, we address a few follow-ups to the trailing byte hardening that landed in #2558.
Tolerate trailing bytes in the node's own database
Databases written by older versions of btcd may have persisted blocks with trailing bytes: the old
submitblockpath and the pre-hardening p2p path both accepted them, and ffldb stores the raw bytes verbatim. With the strict loading added in #2558, such a block at the chain tip would prevent the node from ever starting, and a block deeper in the chain would failgetblock, reorg handling, and indexer catch-up, with no recovery path short of a full resync.We now load database blocks via a new
blockchain.DBBlockFromByteshelper (used byinitChainStateanddbFetchBlockByNode) that parses leniently: any trailing bytes are logged, ignored, and stripped from the serialization cached on the returned block, so downstream consumers of the raw bytes never observe them. The external ingress paths (RPC, p2p, PSBT) remain strict, this only relaxes reads from the node's own database.The final commit extends the same treatment to the two remaining call sites that re-parse own-DB blocks strictly: the
getblockRPC (which also served the dirty bytes verbatim at verbosity 0, and counted them inSize) and the index manager's init path, which loads orphaned blocks directly from the database when rolling an index tip back to the main chain. Both now go through the shared helper, sogetblockserves the exact block serialization at every verbosity level. We can't routegetblockthroughChain.BlockByHashhere since that rejects side chain blocks, whichgetblockserves today.Fix blocking reads and unbounded allocations in PSBT parsing
The trailing data check in
NewFromRawBytesprobed the caller supplied reader with a blocking one byte read, so a reader without aLenmethod (anet.Conn, anio.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, and we document the reader contract onNewFromRawBytes.The base64 path also 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.MaxMessagePayloadexpanded by the base64 encoding overhead.Steps to Test
Each fix ships with a regression test: startup with a trailing byte tip block now succeeds (with the trailing byte excluded from the recorded block size),
DBBlockFromBytesstrips trailing bytes from the cached serialization while still rejecting truncated blocks, a stream reader is never probed past the packet, and oversized base64 input is rejected (pinned with an erroring sentinel reader so the size bound itself is exercised). Rungo test ./blockchain/ ./blockchain/indexers/in the root module andgo test ./...in the psbt module.