fix(p2p,docs): handle pair-peer peer accounting#2280
fix(p2p,docs): handle pair-peer peer accounting#2280gzliudan wants to merge 1 commit intoXinFinOrg:dev-upgradefrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
There was a problem hiding this comment.
Pull request overview
This PR fixes p2p “pair peer” disconnect accounting so that dropping the pair connection doesn’t incorrectly remove the primary peer entry or cause shutdown bookkeeping/logging to underflow. It also clarifies in code comments and docs that public peer reporting (RPC and APIs) is based on unique remote NodeID (not physical connection count).
Changes:
- Track total active peer connections separately inside the p2p server loop and centralize disconnect bookkeeping via
removePeerTracking. - Add a unit test to ensure dropping a pair peer preserves the primary peer entry and clears the
PairPeerpointer. - Update RPC/client/docs wording to clarify peer counts/views are keyed by unique remote node identity.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| p2p/server.go | Adds connection-count tracking and safe removal logic for pair peers; updates public API comments. |
| p2p/server_test.go | Adds a regression test for removePeerTracking when dropping a pair peer. |
| internal/ethapi/api.go | Clarifies net_peerCount comment to “connected remote nodes”. |
| ethclient/ethclient.go | Clarifies client PeerCount docstring to match RPC semantics. |
| docs/xdc/net/net.md | Documents that net_peerCount reports unique NodeIDs, not physical connections. |
| docs/xdc/admin/admin.md | Documents that admin_peers is keyed by unique node identity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gzliudan
force-pushed
the
fix-peers-count
branch
from
54a5b3b to
3418ea3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Track pair-peer disconnects with a separate connection count so removal logs and shutdown bookkeeping do not underflow or drop the primary peer entry. Clarify that PeerCount, Peers, net_peerCount, and admin_peers are reported by unique remote NodeID rather than physical connection count.
gzliudan
force-pushed
the
fix-peers-count
branch
from
3418ea3 to
7642729
Compare
Proposed changes
Track pair-peer disconnects with a separate connection count so removal logs and shutdown bookkeeping do not underflow or drop the primary peer entry. Clarify that PeerCount, Peers, net_peerCount, and admin_peers are reported by unique remote NodeID rather than physical connection count.
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that