-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(net): bound bloom message vectors before allocation #7444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| #!/usr/bin/env python3 | ||
| # Copyright (c) 2026 The Dash Core developers | ||
|
Comment on lines
+1
to
+2
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Squash the copyright-year fixup into the commit that introduced the file Commit 5b6ac38 ("test: fix getmerkleblocks copyright year") only changes the header of test/functional/rpc_getmerkleblocks.py from '# Copyright (c) 2024' to '# Copyright (c) 2026'. That file was created one commit earlier in this same, unmerged PR by 862e7b0 ("test: cover getmerkleblocks oversized bloom-filter prechecks") with the wrong year — it has never shipped to develop. Since this only corrects a mistake made earlier in the same stack rather than being an independently reviewable change, and Dash merges commits without squashing (so this becomes permanent git log/blame/bisect noise on a file born one commit earlier), fold it into 862e7b0 via source: ['codex-dash-core-commit-history', 'sonnet5-dash-core-commit-history', 'opus-dash-core-commit-history'] |
||
| # Distributed under the MIT software license, see the accompanying | ||
| # file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
| """Test the getmerkleblocks RPC bloom-filter size prechecks. | ||
|
|
||
| Now that CBloomFilter deserialization is bounded (LIMITED_VECTOR on vData), | ||
| getmerkleblocks parses the leading CompactSize vData count by hand so that an | ||
| oversized filter still reproduces the historical RPC errors instead of a | ||
| generic length-limit failure (see the precheck in src/rpc/blockchain.cpp). | ||
|
|
||
| This pins that byte-level boundary behaviour: | ||
|
|
||
| - a fully present oversized filter (all vData bytes plus the fixed trailer) | ||
| historically deserialized and then failed IsWithinSizeConstraints(), so it | ||
| must still raise RPC_INVALID_PARAMETER ("Filter is not within size | ||
| constraints"); | ||
| - an oversized declaration with the vData bytes omitted, or with the trailing | ||
| fixed fields short by even one byte, historically raised a DataStream | ||
| end-of-data failure, so it must still surface that RPC_MISC_ERROR; | ||
| - a well-formed in-bounds filter is accepted (the precheck falls through) and | ||
| returns an array. | ||
| """ | ||
|
|
||
| from test_framework.messages import ser_compact_size | ||
| from test_framework.test_framework import BitcoinTestFramework | ||
| from test_framework.util import assert_equal, assert_raises_rpc_error | ||
|
|
||
| # Keep in sync with MAX_BLOOM_FILTER_SIZE in src/common/bloom.h. | ||
| MAX_BLOOM_FILTER_SIZE = 36000 | ||
| # Wire size of the fixed fields serialized after vData: | ||
| # nHashFuncs (uint32) + nTweak (uint32) + nFlags (uint8). | ||
| FILTER_TRAILER_SIZE = 9 | ||
|
|
||
|
|
||
| def raw_filter(declared_vdata_size, vdata_bytes, trailer_bytes): | ||
| """Build a raw bloom-filter wire blob with an arbitrary declared vData | ||
| CompactSize count and an arbitrary number of vData/trailer bytes actually | ||
| present, so truncation boundaries can be exercised directly.""" | ||
| return ( | ||
| ser_compact_size(declared_vdata_size) | ||
| + b"\x00" * vdata_bytes | ||
| + b"\x00" * trailer_bytes | ||
| ).hex() | ||
|
|
||
|
|
||
| class GetMerkleBlocksTest(BitcoinTestFramework): | ||
| def set_test_params(self): | ||
| self.setup_clean_chain = True | ||
| self.num_nodes = 1 | ||
|
|
||
| def run_test(self): | ||
| node = self.nodes[0] | ||
| genesis_hash = node.getblockhash(0) | ||
| oversized = MAX_BLOOM_FILTER_SIZE + 1 | ||
|
|
||
| # Fully present oversized filter: deserialization would succeed, so the | ||
| # historical response is the IsWithinSizeConstraints() rejection. | ||
| complete = raw_filter(oversized, oversized, FILTER_TRAILER_SIZE) | ||
| assert_raises_rpc_error(-8, "Filter is not within size constraints", | ||
| node.getmerkleblocks, complete, genesis_hash, 1) | ||
|
|
||
| # Oversized declaration with the vData bytes omitted: reading vData hits | ||
| # end-of-data. | ||
| truncated_body = raw_filter(oversized, 0, 0) | ||
| assert_raises_rpc_error(-1, "DataStream::read(): end of data", | ||
| node.getmerkleblocks, truncated_body, genesis_hash, 1) | ||
|
|
||
| # All vData present but the trailer short by one byte: exercises the | ||
| # FILTER_TRAILER_SIZE boundary of the precheck, still end-of-data. | ||
| truncated_trailer = raw_filter(oversized, oversized, FILTER_TRAILER_SIZE - 1) | ||
| assert_raises_rpc_error(-1, "DataStream::read(): end of data", | ||
| node.getmerkleblocks, truncated_trailer, genesis_hash, 1) | ||
|
|
||
| # A well-formed in-bounds filter falls through the precheck and is | ||
| # handled normally. Build one explicitly (3-byte all-zero vData, | ||
| # nHashFuncs=1, nTweak=0, nFlags=0); it matches nothing, so the genesis | ||
| # block yields no merkleblocks. | ||
| valid = ( | ||
| ser_compact_size(3) | ||
| + b"\x00\x00\x00" | ||
| + (1).to_bytes(4, "little") # nHashFuncs | ||
| + (0).to_bytes(4, "little") # nTweak | ||
| + b"\x00" # nFlags | ||
| ).hex() | ||
| assert_equal(node.getmerkleblocks(valid, genesis_hash, 1), []) | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| GetMerkleBlocksTest().main() | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PastaPastaPasta
nit: why won't re-throw exception here? instead
return;?So this exception will be caught in the call-stack higher and logged:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PeerMisbehaving(..., e.what())already logs the exception detail throughMisbehaving, so rethrowing would produce a second outer-catch log for the same failure. Returning here is intentional and matches thefilterload/filteradddeserialization handlers added by this PR.