backport: merge bitcoin/bitcoin#25144: refactor: Pass Peer& to Misbehaving()

[knst]

What was done?

After all refactorings to separate network and consensus code it's finally possible to do bitcoin#25144

Depends on #7314, #7332 - both are merged.

How Has This Been Tested?

Run unit & functional tests

Breaking Changes

N/A

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)

[github-actions]

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

• backport: partial Merge bitcoin/bitcoin#26326: net: reduce LOCK(cs_main) scope in GETBLOCKTXN #7350 - fix: reduce GETBLOCKTXN disk read lock scope
• backport: bitcoin#25717, #25960, #25963, #25978, #26012, #26172, #26184, #26355, #26387, #26954, #30761 (anti-DOS headers sync) #7318 - backport: bitcoin#25717, #25960, #25963, #25978, #26012, #26172, #26184, #26355, #26387, #26954, #30761 (anti-DOS headers sync)

Please coordinate with the authors of these PRs to avoid merge conflicts.

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

This pull request has conflicts, please rebase.

[thepastaclaw]

✅ Review complete (commit 89fd0a9)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e1f94ef60f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[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: 4e4c73ed-be6a-4019-9448-0b352c361340

📥 Commits

Reviewing files that changed from the base of the PR and between e1f94ef and 89fd0a9.

📒 Files selected for processing (3)

• src/net_processing.cpp
• src/net_processing.h
• src/test/denialofservice_tests.cpp

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

• src/test/denialofservice_tests.cpp
• src/net_processing.h

---

Walkthrough

The PR replaces node-id based misbehavior handling in net_processing with a peer-based path. PeerManagerImpl now updates peer misbehavior state through Peer&, and block, transaction, header, and message validation paths pass peer objects into the new helper. The public PeerManager interface drops the old Misbehaving(...) virtual and adds UnitTestMisbehaving(...) for tests. Denial-of-service tests were updated to call the new unit-test entry point.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• dashpay/dash#7347 — Also changes src/net_processing.cpp misbehavior handling and ProcessMessage-adjacent penalty flow.

Suggested reviewers

• PastaPastaPasta
• UdjinM6

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% 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 describes the main refactor: passing Peer& to Misbehaving(), with the backport context.
Description check	✅ Passed	The description is clearly related to the refactor, dependencies, and testing, so it passes the lenient relevance check.
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.

✨ 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.}

[coderabbitai]

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@src/net_processing.cpp`:
- Line 3600: The `*Assert(GetPeerRef(node))` use in
`PostProcessMessage`/`PeerPostProcessMessage` can abort if the peer is already
absent, unlike the safer `MaybePunishNodeForBlock`/`MaybePunishNodeForTx` paths.
Update the Dash-specific misbehavior handling to tolerate a missing peer by
checking the result of `GetPeerRef(node)` before calling `Misbehaving`, and
verify all `PeerMisbehaving` callers only report still-connected peers so `node`
cannot be invalid or default to `-1`.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 3bf222ef-10d4-42c7-b356-e33f78fd5707

📥 Commits

Reviewing files that changed from the base of the PR and between 6a051f5 and e1f94ef.

📒 Files selected for processing (3)

• src/net_processing.cpp
• src/net_processing.h
• src/test/denialofservice_tests.cpp

[thepastaclaw]

Code Review

The backport of bitcoin#25144 is mechanically clean and correctly adapts the upstream call sites (MaybePunishNodeForBlock/MaybePunishNodeForTx with if (peer), ProcessMessage with *peer after the null check). However, the two Dash-only entry points that bridge NodeId callers into the new Peer& API — PeerMisbehaving and PostProcessMessage — were adapted using *Assert(GetPeerRef(node)), which is fatal on disconnected peers. PeerMisbehaving in particular is reachable from worker-thread paths (e.g. NetInstantSend::ApplyVerificationResults) where the NodeId can race against disconnect, regressing the prior silent no-op into a node-aborting Assert.

🔴 1 blocking | 🟡 1 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/net_processing.cpp`:
- [BLOCKING] src/net_processing.cpp:6596-6599: PeerMisbehaving aborts the node on disconnected peers — regression vs. prior no-op
  Before this PR, `Misbehaving(NodeId, ...)` started with `PeerRef peer = GetPeerRef(pnode); if (peer == nullptr) return;`, so the Dash-only `PeerMisbehaving` wrapper was a graceful no-op when the peer had already been removed. After this PR, `PeerMisbehaving` is `Misbehaving(*Assert(GetPeerRef(pnode)), howmuch, message);`. `Assert` (src/util/check.h:71) calls `assertion_fail` and aborts the process when its argument is falsy.

  `PeerMisbehaving` is invoked from many Dash subsystems with NodeIds captured earlier, including worker-thread paths where the peer may have disconnected since enqueuing:
  - `src/instantsend/net_instantsend.cpp:195` — `NetInstantSend::ApplyVerificationResults` runs on the isman work thread (see `workInterrupt` loop at line 410) and uses `pending.node_id` captured at enqueue time.
  - `src/instantsend/net_instantsend.cpp:67,100,266`, `src/llmq/net_dkg.cpp:176/183/198/...`, `src/llmq/net_signing.cpp:37/231/275`, `src/llmq/net_quorum.cpp` (many), `src/governance/net_governance.cpp:86/140/197`, `src/coinjoin/server.cpp:139/144/153/179` — all pass NodeId by value.

  Upstream solved this exact race in this same PR for `MaybePunishNodeForBlock`/`MaybePunishNodeForTx` by guarding with `if (peer) Misbehaving(*peer, ...)`. The Dash wrapper needs the same null-tolerant pattern; otherwise a routine disconnect race (or an attacker engineering one during verification latency) turns a benign "peer already gone" event into a node crash.
- [SUGGESTION] src/net_processing.cpp:3597-3611: PostProcessMessage Assert is fragile for NodeId=-1 callers
  `PostProcessMessage` now does `Misbehaving(*Assert(GetPeerRef(node)), result.m_error->score, result.m_error->message);` when `result.m_error` is set. The public `PeerPostProcessMessage` wrapper (line 6651) forwards to `PostProcessMessage(std::move(ret), /*node=*/-1)`, so `GetPeerRef(-1)` would return an empty `PeerRef` and the Assert would abort.

  Today this is safe only because the producers of `MessageProcessingResult` invoked without a peer (e.g. `ChainlockHandler::ProcessNewChainLock` called via `src/chainlock/signing.cpp:286` with `-1`) explicitly guard with `if (from != -1) return MisbehavingError{...};` (src/chainlock/handler.cpp:109). That invariant is not enforced by the type system and is easy to break in future processors that return a `MisbehavingError` without checking `from`.

  Mirroring the `MaybePunishNodeForBlock`/`MaybePunishNodeForTx` adaptation (look up, skip if null) keeps `PostProcessMessage` robust without relying on a cross-file invariant.

[thepastaclaw]

Code Review

Backport of bitcoin#25144 (Pass Peer& to Misbehaving) is clean at HEAD 89fd0a9. Both prior findings (prior-1 PeerMisbehaving abort on disconnected peer, prior-2 PostProcessMessage Assert fragility) are fixed in the latest delta by replacing *Assert(GetPeerRef(...)) with a null-checked PeerRef lookup at both NodeId-based call sites, restoring the pre-PR no-op behavior. Both general reviewers and both backport reviewers converge on no remaining in-scope issues; commit hygiene is clean.

[PastaPastaPasta]

utACK 89fd0a9