Skip to content

Conversation

@PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

m_peer_mutex, acquired in GetPeerRef, is relatively highly contended. Move the pnode->CanRelay up higher to bail out early if possible and avoid the need to acquire m_peer_mutex.

What was done?

How Has This Been Tested?

Builds

Breaking Changes

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • 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)

@PastaPastaPasta PastaPastaPasta added this to the 23.1 milestone Oct 21, 2025
@github-actions
Copy link

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Oct 21, 2025

Walkthrough

The pull request modifies relay filtering logic in src/net_processing.cpp for the RelayInvFiltered function family. In the first variant accepting a transaction object, the code now performs an upfront CanRelay() check for each node before evaluating transaction relay conditions, removing a previously redundant check. In the second variant accepting a transaction hash, the explicit CanRelay() check is removed entirely, allowing relays to peers with non-null tx_relay objects regardless of relay capability. These changes alter the control flow and gating conditions for peer relay filtering, creating divergent behavior between the two code paths.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The changes involve logic modifications to network relay filtering in a critical subsystem. While contained to a single file and two related functions, the reviewer must carefully verify that the divergent behaviors are intentional: one path tightens filtering with an upfront capability check, while the other relaxes it by removing the check. Understanding the rationale for these opposing changes and their broader implications on relay behavior requires careful analysis despite the limited file scope.

Pre-merge checks and finishing touches

❌ 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%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "perf: bail out in RelayInvFiltered early if !CanRelay" directly references the function being modified (RelayInvFiltered), the optimization strategy (early bailout), and the condition triggering it (!CanRelay). This aligns with the primary change described in the raw summary: adding CanRelay checks to guard peer iteration and bail out before acquiring the highly contended m_peer_mutex. The title is specific, concise, and clearly conveys the performance-focused nature of the changeset.
Description Check ✅ Passed The description explains the motivation for the change: m_peer_mutex contention in GetPeerRef. It clearly states the solution—moving the pnode->CanRelay check higher to bail out early and avoid acquiring the mutex when the condition is false. This is directly related to the code changes detailed in the raw summary, which adds CanRelay checks in RelayInvFiltered. While the "What was done?" section is blank and the description could be more detailed, it provides sufficient context connecting the performance issue to the implemented solution, meeting the lenient criteria that the description simply needs to be related to the changeset in some way.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e5ce1f0 and a425211.

📒 Files selected for processing (1)
  • src/net_processing.cpp (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

  • src/net_processing.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build container / Build container

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

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

{
// TODO: Migrate to iteration through m_peer_map
m_connman.ForEachNode([&](CNode* pnode) {
if (!pnode->CanRelay()) return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider TODO above, if you have touched this code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point, I may try to, but it's non-trivial

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM a425211

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK a425211

@PastaPastaPasta PastaPastaPasta merged commit 88121b0 into dashpay:develop Oct 23, 2025
34 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants