Skip to content

fix: penalize oversized notfound messages #7348

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.

Sign up for GitHub

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

Open
thepastaclaw wants to merge 4 commits into
base: develop
Choose a base branch
from
+29 −16
Open

fix: penalize oversized notfound messages #7348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
641f39b
fix: penalize oversized notfound messages
thepastaclaw Jun 8, 2026
56666e9
test: avoid hardcoded oversized notfound count
thepastaclaw Jun 9, 2026
958c97d
ci: rerun flaky asset locks test
thepastaclaw Jun 9, 2026
3d6d17e
ci: rerun linux64 multiprocess test
thepastaclaw Jun 10, 2026
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
33 changes: 18 additions & 15 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5464,24 +5464,27 @@ void PeerManagerImpl::ProcessMessage(
}
if (msg_type == NetMsgType::NOTFOUND) {
// Remove the NOTFOUND transactions from the peer
LOCK(cs_main);
CNodeState *state = State(pfrom.GetId());
std::vector<CInv> vInv;
vRecv >> vInv;
if (vInv.size() <= MAX_PEER_OBJECT_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
for (CInv &inv : vInv) {
if (inv.IsKnownType()) {
// If we receive a NOTFOUND message for a txid we requested, erase
// it from our data structures for this peer.
auto in_flight_it = state->m_object_download.m_object_in_flight.find(inv);
if (in_flight_it == state->m_object_download.m_object_in_flight.end()) {
// Skip any further work if this is a spurious NOTFOUND
// message.
continue;
}
state->m_object_download.m_object_in_flight.erase(in_flight_it);
state->m_object_download.m_object_announced.erase(inv);
if (vInv.size() > MAX_PEER_OBJECT_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
Misbehaving(pfrom.GetId(), 10, strprintf("notfound message size = %u", vInv.size()));
return;
Comment on lines +5469 to +5471

@thepastaclaw thepastaclaw Jun 11, 2026

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.

💬 Nitpick: Misbehavior score of 10 is inconsistent with peer inventory-size penalties elsewhere

All other inventory-vector oversize checks in this file use a score of 20: inv (line 4294), getdata (line 4401), and headers (line 5107). Bitcoin Core's equivalent change for notfound (PR bitcoin#19606) also uses 20. The PR's stated goal is to match the defensive treatment used for other inventory-vector messages, so a value of 10 is asymmetric without an explicit rationale. Either align with the existing pattern, or add a brief comment explaining why notfound warrants a lower penalty.

Suggested change
if (vInv.size() > MAX_PEER_OBJECT_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
Misbehaving(pfrom.GetId(), 10, strprintf("notfound message size = %u", vInv.size()));
return;
Misbehaving(pfrom.GetId(), 20, strprintf("notfound message size = %u", vInv.size()));

source: ['claude']

}

LOCK(cs_main);
CNodeState *state = State(pfrom.GetId());
for (CInv &inv : vInv) {
if (inv.IsKnownType()) {
// If we receive a NOTFOUND message for a txid we requested, erase
// it from our data structures for this peer.
auto in_flight_it = state->m_object_download.m_object_in_flight.find(inv);
if (in_flight_it == state->m_object_download.m_object_in_flight.end()) {
// Skip any further work if this is a spurious NOTFOUND
// message.
continue;
}
state->m_object_download.m_object_in_flight.erase(in_flight_it);
state->m_object_download.m_object_announced.erase(inv);
}
}
return;
Expand Down
12 changes: 11 additions & 1 deletion test/functional/p2p_tx_download.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ def on_getdata(self, message):
MAX_GETDATA_RANDOM_DELAY = 2 # seconds
INBOUND_PEER_TX_DELAY = 2 # seconds
MAX_GETDATA_IN_FLIGHT = 100
MAX_BLOCKS_IN_TRANSIT_PER_PEER = 16
MAX_NOTFOUND_SIZE = MAX_GETDATA_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER
TX_EXPIRY_INTERVAL = GETDATA_TX_INTERVAL * 10

# Python test constants
Expand Down Expand Up @@ -142,13 +144,21 @@ def test_spurious_notfound(self):
self.log.info('Check that spurious notfound is ignored')
self.nodes[0].p2ps[0].send_message(msg_notfound(vec=[CInv(1, 1)]))

def test_oversized_notfound(self):
self.log.info('Check that oversized notfound increases misbehavior score')
oversized_notfound_count = MAX_NOTFOUND_SIZE + 1
invs = [CInv(t=1, h=i) for i in range(oversized_notfound_count)]
with self.nodes[0].assert_debug_log(["Misbehaving", f"notfound message size = {oversized_notfound_count}"]):
self.nodes[0].p2ps[0].send_message(msg_notfound(vec=invs))
self.nodes[0].p2ps[0].sync_with_ping()

def run_test(self):
self.wallet = MiniWallet(self.nodes[0])
self.wallet.rescan_utxos()

# Run each test against new bitcoind instances, as setting mocktimes has long-term effects on when
# the next trickle relay event happens.
for test in [self.test_spurious_notfound, self.test_in_flight_max, self.test_inv_block, self.test_tx_requests]:
for test in [self.test_spurious_notfound, self.test_oversized_notfound, self.test_in_flight_max, self.test_inv_block, self.test_tx_requests]:
self.stop_nodes()
self.start_nodes()
self.connect_nodes(1, 0)
Expand Down
Loading