Skip to content

Fix peer selection bounds - #237

Open
tolgahanbozkurt wants to merge 1 commit into
Beldex-Coin:devfrom
tolgahanbozkurt:fix/peer-selection-bounds
Open

tolgahanbozkurt wants to merge 1 commit into
Beldex-Coin:devfrom
tolgahanbozkurt:fix/peer-selection-bounds

Conversation

@tolgahanbozkurt

Copy link
Copy Markdown

Peer selection stops after three random draws because its draw limit is never updated from the filtered peer list. Duplicate or unavailable candidates can exhaust those draws before a usable peer is tried. The white-list selector also passes a last index to a helper that expects a count, excluding the final candidate. With two candidates, ordinary selection always picks the first.

Restore the bounded draw allowance and pass the full candidate count. Clamp the helper's result because floating-point rounding can reach its exclusive upper bound. The existing ten-entry limit, pruning and subnet preferences, bans, and failed-host cooldown remain in place.

Validated on an isolated Linux test server: the daemon builds with GCC 11 and passes an offline RPC startup check. All 57 focused checks using the actual selection functions with controlled peer lists and transport pass; the original code fails 15. The rounding case was also reproduced with the actual standard-library random distribution.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 64b48475-4c98-4941-a6ae-0030678352be

📥 Commits

Reviewing files that changed from the base of the PR and between 4cd63fd and 06b6fc3.

📒 Files selected for processing (1)
  • src/p2p/net_node.inl

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


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Improved peer connection selection when choosing from larger groups of eligible peers, helping ensure the full set of available choices can be considered.

Walkthrough

The exponential peer picker now clamps its result to the last valid index. White-list selection passes the full filtered peer count to the picker and applies a separate cap to the retry-loop bound.

Changes

Peerlist Selection

Layer / File(s) Summary
Clamp exponential peer selection
src/p2p/net_node.inl
The picker clamps its result to the last valid index. White-list selection passes the full filtered peer count to the picker and uses a separate cap of 20 for the retry-loop bound.

Priority: ➖ Normal

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

Change: Bug fix

Suggested reviewers: victor-tucci

Merge Risk: ⚪ Minimal · up to 06b6f

Peer selection now samples across the filtered candidates while keeping retries capped. The reviewed bounds have no concrete merge-blocking risk.

Architecture Summary

Architecture risk: 🔵 Low · up to 06b6f

The change affects 1 system.

Changed systems: src

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — src (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in src/p2p/net_node.inl: get_random_exp_index now returns the smaller of the computed index and size - 1, preventing an index equal to or beyond the list size; previously it returned the computed index unchanged.
  • observed — Modified behavior in src/p2p/net_node.inl: The retry bound now uses min(filtered.size() - 1, 20), while white-list selection passes the full filtered.size() to get_random_exp_index. Previously, the 20-item cap was applied to the picker’s size argument.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: correcting peer selection bounds.
Description check ✅ Passed The description directly explains the peer selection defects, the implemented fixes, preserved behavior, and validation results.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
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 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

A rabbit checks the peerlist bounds,
And keeps each picked index safe and sound.
The full list guides the picker’s flight,
While twenty caps retries just right.
Hops onward through the network night.

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

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.

1 participant