fix: penalize oversized notfound messages

[thepastaclaw]

fix: penalize oversized notfound messages

Issue being fixed or feature implemented

Oversized notfound inventory vectors were ignored after deserialization. This
change gives them a small misbehavior score, matching the defensive treatment
used for other inventory-vector messages, and avoids holding cs_main while
deserializing the vector.

What was done?

• Deserialize notfound inventory vectors before taking cs_main.
• Return early with a small misbehavior score when a notfound vector exceeds
  the maximum outstanding object/block request count.
• Add functional coverage for the oversized notfound path.

How Has This Been Tested?

Environment: macOS 15.6 arm64, local Dash Core autotools build from this branch.

• git diff --check

• Local build configured with:

  ./configure --without-gui --disable-bench --disable-fuzz-binary \
    --disable-tests --disable-wallet

• make -j8 src/dashd src/dash-cli

  • Initial parallel build hit a generated dependency-file race while building
    dash-cli; dashd linked successfully.

• make -j1 src/dash-cli

• test/functional/p2p_tx_download.py --cachedir=/tmp/dash_func_cache_notfound

Breaking Changes

None.

Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone
  (for repository code-owners and collaborators only)

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2c0e2dcd-64c8-4a09-acbc-e388e5fdcc05

📥 Commits

Reviewing files that changed from the base of the PR and between c4a51eecab3fa50c202ffe5376f520d424d96a4a and a97b7b4.

📒 Files selected for processing (2)

• src/net_processing.cpp
• test/functional/p2p_tx_download.py

🚧 Files skipped from review as they are similar to previous changes (2)

• src/net_processing.cpp
• test/functional/p2p_tx_download.py

---

Walkthrough

This PR adds a defensive validation layer to NOTFOUND message processing. PeerManagerImpl now reads the incoming NOTFOUND vInv and rejects it immediately (Misbehaving score 20) 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=20)
    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

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• UdjinM6

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely identifies the main change: penalizing oversized notfound messages, which is the primary objective of the PR.
Description check	✅ Passed	The description comprehensively explains the issue, the three key changes made, testing approach, and checklist status, all directly related to the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

test/functional/p2p_tx_download.py (1)

147-149: ⚡ Quick win

Avoid hardcoded oversized-count literals in this test.

+ 17 and "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

📥 Commits

Reviewing files that changed from the base of the PR and between 317917a and 641f39b04d13e708666ee89080c2b875493d1d61.

📒 Files selected for processing (2)

• src/net_processing.cpp
• test/functional/p2p_tx_download.py

[thepastaclaw]

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: test/functional/p2p_tx_download.py --timeout-factor=2.

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thepastaclaw]

CI follow-up: the only red check is linux64_multiprocess-test / Test source, and its GitHub check annotation says the hosted runner lost communication with the server. I could not fetch test failure logs, and gh run rerun --failed is blocked for my account because rerun requires repo admin rights. This looks like runner infrastructure rather than a code/test failure; maintainer rerun needed.

[thepastaclaw]

CI is green after the CI-only empty rerun commit 3d6d17ea62. There is no code diff from the previously reviewed head, so the existing CodeRabbit/automation review remains applicable. Marking ready for review.

[thepastaclaw]

✅ Review complete (commit a97b7b4)

[thepastaclaw]

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)

[thepastaclaw]

CI follow-up correction: the red job was linux64_ubsan-test / Test source, failing in feature_governance.py with a vote-propagation race (gobject count reported 24 vs 25). I checked the logs and this happens in governance vote propagation, not in this PR’s oversized-notfound path; another parallel feature_governance instance passed in the same run. Since I do not have permission to rerun the failed job directly, I pushed empty commit af5d8c0e00 to rerun CI.

[thepastaclaw]

Code Review

Both agents independently report no findings on the cumulative head at af5d8c0e. The PR moves NOTFOUND vector deserialization out from under cs_main, adds a 20-point misbehavior penalty for oversized notfound vectors (matching the established pattern for inv/getdata/addr/headers), and includes focused functional test coverage. The latest delta from c4a51eecab is a CI-only rerun commit, so there are no carried-forward prior findings and no resolved findings.

[thepastaclaw]

Code Review

Both agents independently found no in-scope issues in this PR, which adds a misbehavior penalty (score 20) for oversized notfound inventory vectors and reorders deserialization before cs_main. CodeRabbit reported zero inline findings. The change is consistent with existing conventions for other inv-vector messages and includes functional test coverage. No carried-forward prior findings exist.

[UdjinM6]

looks good but you should drop empty "ci retrigger" commits

[thepastaclaw]

Code Review

Both agents found no in-scope issues in this PR, which adds a misbehavior penalty for oversized notfound messages and a functional test covering it. The change mirrors handling for other inventory-vector messages and matches upstream Bitcoin Core. No carried-forward prior findings exist (prior review at b9e5b531 was clean), and CodeRabbit reported 0 inline findings.

[thepastaclaw]

CI follow-up on the current red checks:

• aarch64-linux-build / Build source failed before a code/build error with the GitHub runner annotation System.IO.IOException: No space left on device while writing the runner diagnostic log (/home/runner/actions-runner/.../_diag/Worker_*.log). That is runner disk exhaustion.
• linux64_ubsan-test / Test source failed in feature_asset_locks.py at the existing asset-unlock mempool-size assertion (AssertionError: not(1 == 0) after asset_unlock_tx_too_big_fee). This is in the asset-lock functional test path, not in this PR’s notfound handling changes (net_processing.cpp and p2p_tx_download.py).

I’m leaving the branch unchanged; both failures look unrelated to this PR and need a maintainer rerun if CI needs to be refreshed.

[thepastaclaw]

Done — the empty CI-retrigger commits are no longer on the branch. Current head is a97b7b45, with only the oversized-notfound fix and follow-up code/test adjustment commits.

[thepastaclaw]

CI follow-up on the current red check:

• linux64_ubsan-test / Test source failed in feature_asset_locks.py, not in this PR's notfound handling path.
• The failure is a quorum-mining wait timeout: quorum_hash in self.nodes[0].quorum('list')[llmq_type_name] did not become true within 60s while running mine_quorum_2_nodes() / llmq_test_platform.
• This PR only changes oversized notfound message handling and its targeted P2P coverage, so this looks unrelated to the diff.

Leaving the branch unchanged; maintainers can rerun CI if they want a clean runner result.

[UdjinM6]

utACK a97b7b4

[github-actions]

This pull request has conflicts, please rebase.