fix: penalize oversized notfound messages#7348
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds a defensive validation layer to NOTFOUND message processing. PeerManagerImpl now reads the incoming NOTFOUND vInv and rejects it immediately (Misbehaving score 10) if its size exceeds MAX_PEER_OBJECT_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER; otherwise it proceeds with the existing per-inventory cleanup for known types. A functional test sends an oversized NOTFOUND and asserts the misbehavior log entry. Sequence Diagram(s)sequenceDiagram
participant RemotePeer
participant PeerManager
participant Node
RemotePeer->>PeerManager: SEND msg_notfound (vInv)
PeerManager->>PeerManager: read vInv and compute size
alt size > MAX_PEER_OBJECT_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER
PeerManager->>Node: Misbehaving(peer, score=10)
Note right of PeerManager: return early
else size within limits
PeerManager->>PeerManager: iterate vInv, erase m_object_in_flight/announced entries
PeerManager->>RemotePeer: continue normal processing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/functional/p2p_tx_download.py (1)
147-149: ⚡ Quick winAvoid hardcoded oversized-count literals in this test.
+ 17and"notfound message size = 117"bake in current protocol limits. Derive both from named constants so this stays valid if limits change.Suggested refactor
def test_oversized_notfound(self): self.log.info('Check that oversized notfound increases misbehavior score') - invs = [CInv(t=1, h=i) for i in range(MAX_GETDATA_IN_FLIGHT + 17)] - with self.nodes[0].assert_debug_log(["Misbehaving", "notfound message size = 117"]): + oversized_count = MAX_GETDATA_IN_FLIGHT + 17 # 1 above current C++ notfound limit path + invs = [CInv(t=1, h=i) for i in range(oversized_count)] + with self.nodes[0].assert_debug_log(["Misbehaving", f"notfound message size = {oversized_count}"]): self.nodes[0].p2ps[0].send_message(msg_notfound(vec=invs)) self.nodes[0].p2ps[0].sync_with_ping()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/p2p_tx_download.py` around lines 147 - 149, Replace the hardcoded "+ 17" and "117" by deriving both values from named constants and the actual serialized message size: introduce a small named constant (e.g. EXTRA_NOTFOUND_INV_COUNT) and build invs as CInv(t=1, h=i) for i in range(MAX_GETDATA_IN_FLIGHT + EXTRA_NOTFOUND_INV_COUNT); then compute the expected debug string dynamically from the serialized notfound message (e.g. expected = f"notfound message size = {len(msg_notfound(vec=invs).serialize())}" or equivalent) and use that in self.nodes[0].assert_debug_log instead of the literal "notfound message size = 117", keeping references to CInv, MAX_GETDATA_IN_FLIGHT, msg_notfound and assert_debug_log to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/functional/p2p_tx_download.py`:
- Around line 147-149: Replace the hardcoded "+ 17" and "117" by deriving both
values from named constants and the actual serialized message size: introduce a
small named constant (e.g. EXTRA_NOTFOUND_INV_COUNT) and build invs as CInv(t=1,
h=i) for i in range(MAX_GETDATA_IN_FLIGHT + EXTRA_NOTFOUND_INV_COUNT); then
compute the expected debug string dynamically from the serialized notfound
message (e.g. expected = f"notfound message size =
{len(msg_notfound(vec=invs).serialize())}" or equivalent) and use that in
self.nodes[0].assert_debug_log instead of the literal "notfound message size =
117", keeping references to CInv, MAX_GETDATA_IN_FLIGHT, msg_notfound and
assert_debug_log to locate the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2220f6ad-bdbd-40da-920c-3278cc73bb3b
📒 Files selected for processing (2)
src/net_processing.cpptest/functional/p2p_tx_download.py
|
Addressed CodeRabbit nitpick in follow-up commit: introduced named NOTFOUND-size constants and derived the oversized test count/assertion from those constants instead of hardcoded literals. Validation: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
CI follow-up: the only red check is |
|
CI is green after the CI-only empty rerun commit |
|
✅ Review complete (commit 3d6d17e) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, focused fix that moves NOTFOUND deserialization out of cs_main and applies a misbehavior penalty when vInv exceeds the per-peer in-flight budget. Logic and lock ordering are correct, and the functional test exercises the new path. One minor consistency observation: the chosen penalty (10) is half of what sibling inventory-vector oversize checks use (20).
💬 1 nitpick(s)
| 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; |
There was a problem hiding this comment.
💬 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.
| 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']
1 participant
fix: penalize oversized notfound messages
Issue being fixed or feature implemented
Oversized
notfoundinventory vectors were ignored after deserialization. Thischange gives them a small misbehavior score, matching the defensive treatment
used for other inventory-vector messages, and avoids holding
cs_mainwhiledeserializing the vector.
What was done?
notfoundinventory vectors before takingcs_main.notfoundvector exceedsthe maximum outstanding object/block request count.
notfoundpath.How Has This Been Tested?
Environment: macOS 15.6 arm64, local Dash Core autotools build from this branch.
git diff --checkLocal build configured with:
make -j8 src/dashd src/dash-clidash-cli;dashdlinked successfully.make -j1 src/dash-clitest/functional/p2p_tx_download.py --cachedir=/tmp/dash_func_cache_notfoundBreaking Changes
None.
Checklist
(for repository code-owners and collaborators only)