Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/common/bloom.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ class CBloomFilter
CBloomFilter(const unsigned int nElements, const double nFPRate, const unsigned int nTweak, unsigned char nFlagsIn);
CBloomFilter() : nHashFuncs(0), nTweak(0), nFlags(0) {}

SERIALIZE_METHODS(CBloomFilter, obj) { READWRITE(obj.vData, obj.nHashFuncs, obj.nTweak, obj.nFlags); }
// Bound vData at MAX_BLOOM_FILTER_SIZE before allocation. Wire format is unchanged;
// IsWithinSizeConstraints() still guards the exact boundary and nHashFuncs.
SERIALIZE_METHODS(CBloomFilter, obj) { READWRITE(LIMITED_VECTOR(obj.vData, MAX_BLOOM_FILTER_SIZE), obj.nHashFuncs, obj.nTweak, obj.nFlags); }

void insert(Span<const unsigned char> vKey);
void insert(const COutPoint& outpoint);
Expand Down
11 changes: 9 additions & 2 deletions src/governance/net_governance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,16 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa
if (!m_node_sync.IsSynced()) return;

uint256 nProp;
CBloomFilter filter;
vRecv >> nProp;
vRecv >> filter;

CBloomFilter filter;
try {
vRecv >> filter;
} catch (const std::ios_base::failure& e) {
// An oversized filter now throws pre-allocation; punish here instead of the outer catch.
m_peer_manager->PeerMisbehaving(peer.GetId(), 100, strprintf("misformatted govsync bloom filter. peer=%d error=%s", peer.GetId(), e.what()));
return;

@knst knst Jul 11, 2026

Copy link
Copy Markdown
Collaborator

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:


LogPrint(BCLog::NET, "%s(%s, %u bytes): Exception '%s' (%s) caught\n", __func__, SanitizeString(msg.m_type), msg.m_message_size, ....

Copy link
Copy Markdown
Author

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 through Misbehaving, so rethrowing would produce a second outer-catch log for the same failure. Returning here is intentional and matches the filterload/filteradd deserialization handlers added by this PR.

}

// The per-object vote-sync path tests this peer-supplied filter against every
// cached vote (CBloomFilter::contains() loops nHashFuncs times). An unbounded
Expand Down
24 changes: 17 additions & 7 deletions src/net_processing.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5398,7 +5398,13 @@ void PeerManagerImpl::ProcessMessage(
return;
}
CBloomFilter filter;
vRecv >> filter;
try {
vRecv >> filter;
} catch (const std::ios_base::failure& e) {
// An oversized filter now throws pre-allocation; punish here instead of the outer catch.
Misbehaving(*peer, 100, strprintf("misformatted bloom filter. peer=%d error=%s", pfrom.GetId(), e.what()));
return;
}

if (!filter.IsWithinSizeConstraints())
{
Expand All @@ -5422,15 +5428,19 @@ void PeerManagerImpl::ProcessMessage(
pfrom.fDisconnect = true;
return;
}
// Nodes must NEVER send a data item > 520 bytes (the max size for a script data object,
// and thus, the maximum size any matched object can have) in a filteradd message. Bound
// the declared length before allocation and punish a bad count here, not the outer catch.
std::vector<unsigned char> vData;
vRecv >> vData;
try {
vRecv >> LIMITED_VECTOR(vData, MAX_SCRIPT_ELEMENT_SIZE);
} catch (const std::ios_base::failure& e) {
Misbehaving(*peer, 100, strprintf("bad filteradd message. peer=%d error=%s", pfrom.GetId(), e.what()));
return;
}

// Nodes must NEVER send a data item > 520 bytes (the max size for a script data object,
// and thus, the maximum size any matched object can have) in a filteradd message
bool bad = false;
if (vData.size() > MAX_SCRIPT_ELEMENT_SIZE) {
bad = true;
} else if (auto tx_relay = peer->GetTxRelay(); tx_relay != nullptr) {
if (auto tx_relay = peer->GetTxRelay(); tx_relay != nullptr) {
LOCK(tx_relay->m_bloom_filter_mutex);
if (tx_relay->m_bloom_filter) {
tx_relay->m_bloom_filter->insert(vData);
Expand Down
28 changes: 28 additions & 0 deletions src/rpc/blockchain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -852,6 +852,34 @@ static RPCHelpMan getmerkleblocks()
CBloomFilter filter;
std::string strFilter = request.params[0].get_str();
CDataStream ssBloomFilter(ParseHex(strFilter), SER_NETWORK, PROTOCOL_VERSION);
// CBloomFilter deserialization is now bounded, so an oversized vData count throws a generic
// length-limit error instead of the historical response. Preserve the old behavior by
// prechecking the leading vData count without consuming the stream: a fully present oversized
// filter (all vData bytes plus the fixed trailing fields) historically deserialized and then
// failed IsWithinSizeConstraints(), while a truncated one raised DataStream end-of-data. Only
// these two well-formed-count cases are handled here; malformed, noncanonical, and
// above-MAX_SIZE prefixes fall through to normal deserialization, which reproduces them.
enum { PRECHECK_OK, OVERSIZED_COMPLETE, OVERSIZED_TRUNCATED } precheck{PRECHECK_OK};
try {
SpanReader prefix{SER_NETWORK, PROTOCOL_VERSION, MakeUCharSpan(ssBloomFilter)};
const uint64_t vdata_size{ReadCompactSize(prefix)};
if (vdata_size > MAX_BLOOM_FILTER_SIZE) {
// Wire size of the fixed fields after vData: nHashFuncs + nTweak (uint32) + nFlags (uint8).
constexpr uint64_t FILTER_TRAILER_SIZE{2 * sizeof(uint32_t) + sizeof(uint8_t)};
const uint64_t remaining{prefix.size()};
const bool complete{remaining >= vdata_size && remaining - vdata_size >= FILTER_TRAILER_SIZE};
precheck = complete ? OVERSIZED_COMPLETE : OVERSIZED_TRUNCATED;
}
} catch (const std::ios_base::failure&) {
// Malformed/noncanonical/above-MAX_SIZE count: leave PRECHECK_OK; deserialization reproduces it.
}
if (precheck == OVERSIZED_COMPLETE) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Filter is not within size constraints");
}
if (precheck == OVERSIZED_TRUNCATED) {
// Reproduce the original end-of-data failure without allocating the oversized vData.
throw std::ios_base::failure("DataStream::read(): end of data");
}
Comment thread
thepastaclaw marked this conversation as resolved.
ssBloomFilter >> filter;
if (!filter.IsWithinSizeConstraints()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Filter is not within size constraints");
Expand Down
15 changes: 15 additions & 0 deletions test/functional/p2p_filter.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@
msg_filteradd,
msg_filterclear,
msg_filterload,
msg_generic,
msg_getdata,
msg_mempool,
msg_version,
ser_compact_size,
)
from test_framework.p2p import (
P2PInterface,
Expand All @@ -34,6 +36,10 @@
getnewdestination,
)

# serialize.h MAX_SIZE: the largest count ReadCompactSize() accepts, so a declared
# vector length of this value reaches the vData/data cap, not the compact-size guard.
MAX_SIZE = 0x02000000


class P2PBloomFilter(P2PInterface):
# This is a P2SH watch-only wallet
Expand Down Expand Up @@ -112,6 +118,11 @@ def test_size_limits(self, filter_peer):
filter_peer.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE)))
filter_peer.send_and_ping(msg_filterclear())

self.log.info('Check that a filterload declaring an oversized vData length with the bytes omitted is rejected before allocation')
# Without the cap this would fall into the outer catch (no Misbehaving) after a large allocation.
with self.nodes[0].assert_debug_log(['Misbehaving']):
filter_peer.send_and_ping(msg_generic(b'filterload', ser_compact_size(MAX_SIZE)))

self.log.info('Check that filter with too many hash functions is rejected')
with self.nodes[0].assert_debug_log(['Misbehaving']):
filter_peer.send_and_ping(msg_filterload(data=b'\xaa', nHashFuncs=MAX_BLOOM_HASH_FUNCS+1))
Expand All @@ -129,6 +140,10 @@ def test_size_limits(self, filter_peer):
with self.nodes[0].assert_debug_log(['Misbehaving']):
filter_peer.send_and_ping(msg_filteradd(data=b'\xcc'*(MAX_SCRIPT_ELEMENT_SIZE+1)))

self.log.info('Check that a filteradd declaring an oversized data length with the bytes omitted is rejected before allocation')
with self.nodes[0].assert_debug_log(['Misbehaving']):
filter_peer.send_and_ping(msg_generic(b'filteradd', ser_compact_size(MAX_SIZE)))

filter_peer.send_and_ping(msg_filterclear())

def test_msg_mempool(self):
Expand Down
14 changes: 13 additions & 1 deletion test/functional/p2p_govsync_bloom.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,16 @@
"""
import struct

from test_framework.messages import ser_string, ser_uint256
from test_framework.messages import msg_generic, ser_compact_size, ser_string, ser_uint256
from test_framework.p2p import P2PInterface
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import force_finish_mnsync

# CBloomFilter size constraints (src/common/bloom.h).
MAX_HASH_FUNCS = 50
# serialize.h MAX_SIZE: the largest count ReadCompactSize() accepts, so a declared
# vData length of this value reaches the vData cap, not the compact-size guard.
MAX_SIZE = 0x02000000


class msg_govsync:
Expand Down Expand Up @@ -65,6 +68,15 @@ def run_test(self):
bad_peer.send_message(msg_govsync(n_hash_funcs=0xFFFFFFFF))
bad_peer.wait_for_disconnect()

self.log.info("A govsync request declaring an oversized filter vData length with the bytes omitted is rejected before allocation")
# nProp (32 bytes) then a CompactSize(MAX_SIZE) vData length with no bytes. Without the
# cap this would fall into net_processing's outer catch (no Misbehaving, no disconnect).
raw_peer = node.add_p2p_connection(P2PInterface())
raw_payload = ser_uint256(0) + ser_compact_size(MAX_SIZE)
with node.assert_debug_log(['Misbehaving']):
raw_peer.send_message(msg_generic(b'govsync', raw_payload))
raw_peer.wait_for_disconnect()


if __name__ == '__main__':
GovsyncBloomCapTest().main()
90 changes: 90 additions & 0 deletions test/functional/rpc_getmerkleblocks.py
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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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 git rebase -i --autosquash (or a manual fixup) so the file lands with the correct copyright year in a single commit. Verified directly: git show 862e7b0e890 shows the file added with '(c) 2024', and git show 5b6ac384d7f shows the sole change is the year correction, with 5b6ac38 not yet reachable from develop.

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()
2 changes: 1 addition & 1 deletion test/functional/test_framework/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -1904,7 +1904,7 @@ def __repr__(self):
# for cases where a user needs tighter control over what is sent over the wire
# note that the user must supply the name of the msgtype, and the data
class msg_generic:
__slots__ = ("data")
__slots__ = ("msgtype", "data")

def __init__(self, msgtype, data=None):
self.msgtype = msgtype
Expand Down
3 changes: 1 addition & 2 deletions test/functional/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@
'wallet_backwards_compatibility.py --descriptors',
'wallet_txn_clone.py --mineblock',
'rpc_getblockfilter.py',
'rpc_getmerkleblocks.py',
'rpc_getblockfrompeer.py',
'rpc_invalidateblock.py',
'feature_txindex.py',
Expand Down Expand Up @@ -936,8 +937,6 @@ def _get_uncovered_rpc_commands(self):
covered_cmds.add('voteraw')
# TODO: implement functional tests for importelectrumwallet
covered_cmds.add('importelectrumwallet')
# TODO: implement functional tests for getmerkleblocks
covered_cmds.add('getmerkleblocks')
Comment thread
knst marked this conversation as resolved.

if not os.path.isfile(coverage_ref_filename):
raise RuntimeError("No coverage reference found")
Expand Down
Loading