[PastaPastaPasta]

Issue being fixed or feature implemented

PeerManager exposed the same check twice: IsBanned(NodeId) on PeerManager and PeerIsBanned(NodeId) on PeerManagerInternal, which only forwarded to the former. Neither name describes what the check does. It does not consult BanMan; it only reports whether the live Peer has crossed the discouragement threshold and SendMessages() has not yet consumed m_should_discourage, and it returns false for peers that are unknown or already finalized. Both declarations also required cs_main, which the lookup never uses (it only takes m_peer_mutex and the peer's m_misbehavior_mutex).

This PR originally also changed LLMQ cleanup to drop pending recovered sigs and sig-share state for peers that disconnected. That part is superseded by #7563, which purges pending recovered sigs eagerly in NetSigning::BanNode and deliberately keeps queued recovered sigs from peers that disconnect without misbehaving. Sig-share state for vanished peers is already cleaned by CSigSharesManager::Cleanup(). The behavior change has been dropped; this PR is now a behavior-neutral refactor.

What was done?

• Replaced PeerManager::IsBanned() and PeerManagerInternal::PeerIsBanned() with a single PeerManagerInternal::PeerShouldBeDiscouraged(), documented with its actual semantics.
• Removed the cs_main requirement from the annotation; it keeps !m_peer_mutex.
• Renamed NetSigning::RemoveBannedNodeStates() to RemoveDiscouragedNodeStates().
• Kept the existing semantics, including returning false for missing peers.
• Added peer_discouragement assertions for the flag before and after crossing the threshold, after SendMessages() consumes it, below the threshold, and after FinalizeNode().

How Has This Been Tested?

macOS arm64, depends toolchain, Apple clang with -Wthread-safety, --enable-werror --disable-bench --without-gui:

• make -j6 (clean build, no new warnings)
• ./src/test/test_dash --run_test=denialofservice_tests, net_tests, llmq_utils_tests, llmq_qgetdata_tests, llmq_invalid_type_tests, llmq_chainlock_tests
• test/functional/test_runner.py feature_llmq_signing.py "feature_llmq_signing.py --spork21" p2p_disconnect_ban.py
• test/lint/lint-whitespace.py, lint-includes.py, lint-circular-dependencies.py, contrib/devtools/clang-format-diff.py on the diff
• test/lint/all-lint.py: only lint-cppcheck-dash.py failed, on src/governance/object.cpp:854 (untouched by this PR, identical to develop); codespell, vulture and flake8 were skipped because they are not installed locally.

Breaking Changes

None. The renamed interface is internal to the networking and LLMQ implementation.

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

🤖 Generated with Claude Code

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used

📚 Code guidelines (1)

doc/developer-notes.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 4dcb747a-e506-4fd6-8f98-22492bf2f2ff

📥 Commits

Reviewing files that changed from the base of the PR and between 315f683 and fed2f49.

📒 Files selected for processing (5)

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

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

Walkthrough

The peer manager replaces banned-state queries with PeerShouldBeDiscouraged. The query returns false for an absent peer and checks the discouragement flag under the misbehavior mutex. NetSigning uses this query to remove share-manager state for discouraged peers. Tests check the state at and below the threshold, after SendMessages, and during node finalization.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to fed2f

This refactor merges two peer discouragement queries and renames a cleanup helper. No concrete defect was found, so the change looks safe to merge after the usual checks.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to fed2f

The refactor preserves peer selection, discouragement timing, and signing-state cleanup. The reviewed changes introduce no new remote capability or weakened enforcement control.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The reviewed exposure remains peer-scoring and signing bookkeeping within the node. Cleanup selects NodeId-keyed state; address-wide discouragement remains a separate enforcement outcome. The renamed query adds no remote entrypoint or broader cleanup authority.

Trust Boundaries and Controls

• observed — Misbehavior scoring sets the pending flag only when the score crosses the discouragement threshold. The query reads that peer-owned flag under the same misbehavior mutex; its caller cannot supply a replacement policy or arbitrary removal predicate through the network.

Resilience and Maintainability Implications

• observed — Flag observation and share-state deletion are not a cross-component transaction: concurrent flag consumption can defer predicate-based deletion. That timing is inherited. The existing cleanup worker repeats cleanup, and the share manager separately removes states for peers absent from CConnman, including their requested-share keys. This PR introduces no new durable transition or recovery owner.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files.	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 summarizes the main change: merging duplicate peer-discouragement queries.
Description check	✅ Passed	The description explains the query refactor, related renaming, added tests, and reported validation.
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.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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

[thepastaclaw]

✅ Final review complete — no blockers (commit fed2f49) · triage: low

[PastaPastaPasta]

@coderabbitai review\n\n---\n🤖 Posted autonomously by Codex on behalf of pasta.

[coderabbitai]

@PastaPastaPasta I will review pull request #7584.

✅ 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]

This pull request has conflicts, please rebase.

[PastaPastaPasta]

Reshaped this PR after rebasing onto develop (8819507); new head fed2f49.

The fix part is superseded by #7563, which purges pending recovered sigs in NetSigning::BanNode. That PR deliberately keeps queued recovered sigs from peers that disconnect without misbehaving, because those sigs may be the only copy we receive. The old version of this PR deleted them on disconnect, so that change has been dropped. Treating a missing peer as "disconnected" in the sig-share sweep would also have changed behavior (it would duplicate what CSigSharesManager::Cleanup() already does on its own schedule), so that is dropped too.

What remains is a behavior-neutral refactor(net): single commit:

• IsBanned() and PeerIsBanned() are merged into PeerManagerInternal::PeerShouldBeDiscouraged(), which still returns false for unknown peers.
• The unused cs_main lock requirement is removed.
• RemoveBannedNodeStates() is renamed to RemoveDiscouragedNodeStates().
• peer_discouragement assertions are added.

The build is clean with clang thread-safety analysis and -Werror. denialofservice_tests, the net/llmq unit suites, feature_llmq_signing.py (both variants) and p2p_disconnect_ban.py pass. The title and description have been updated to match.

---

🤖 Posted autonomously by Codex on behalf of pasta.

[thepastaclaw]

Final validation — Phase 1 + Phase 2

Verified the supplied reviewer assessments against the complete diff and relevant callers at fed2f49. The consolidated query preserves the discouragement flag and unknown-peer behavior, requires no cs_main-protected state, and leaves LLMQ cleanup semantics unchanged; no actionable findings were identified. This was a static review only: the supplied CI snapshot reports successful lint, formatting, and Linux64/aarch64 source builds, while tests and several other builds remain queued or in progress.

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — This small, behavior-neutral refactor consolidates a peer-state query, removes an unused locking requirement, updates LLMQ callers, and adds focused assertions whose correctness is straightforward to confirm.
• Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer
• Phase 1 model: glm-5.3-flash — zai quota: 5h 99% left, weekly 69% left; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer

[PastaPastaPasta]

approved