Repository navigation
Count only connected peers in /net_info n_peers - #4500
Conversation
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4500 +/- ##
==========================================
+ Coverage 57.11% 57.22% +0.11%
==========================================
Files 2127 2135 +8
Lines 166521 167077 +556
==========================================
+ Hits 95101 95609 +508
- Misses 71415 71463 +48
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryLow Risk Overview The Adds Reviewed by Cursor Bugbot for commit 691d45b. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
This change makes /net_info build peers and n_peers from Router.ConnInfos(), so they report live connections, which already come deduplicated by node ID, instead of every address learned via PEX; it also removes Router.AllAddrs and adds a test-only KnownAddrs helper. The new logic is correct and nothing blocks it; the tree was checked out at the PR merge but Go is not installed here, so the new test was not run, and the one scout reading (codex) found nothing, so it added no findings and none were dropped.
1 nit, not posted on the code
sei-tendermint/internal/rpc/core/net.go:21— An inbound connection whose handshake carried noSelfAddrnow shows up inpeerswithurl: ""; before this change such a peer was left out.SplitAndTrimEmptydrops empty entries, sopersistent-peersbuilt by scripts that read.peers[].urlis unaffected, but leaving the peer out ofpeerswould keep the output clean for other readers.
seidroid review · decision approve · session 1ba11012dd11415ba145e8fdf7c42349 · turn resp_claude_b624230ebcbfb83a1cc4ca3d9961254c · item fefbd1cd4c7152f79c4eb829be3a6db3
Findings: 0 blocking | 0 non-blocking | 0 posted inline
| PeerConnections: slices.Collect(maps.Values(peerConnections)), | ||
| NPeers: len(peerConnections), | ||
| Peers: peers, | ||
| PeerConnections: peerConnections, |
There was a problem hiding this comment.
Based on what you described, it is probably correct semantics for the tools.
But should we export count of AllAddrs somewhere so we have visibility on that? I think we already exported all peers (whether they are connected or not) before PR#2539?
There was a problem hiding this comment.
I think we should; as a prometheus metric 👍
There was a problem hiding this comment.
Thank you for reviewing; I will action this as a separate follow up PR for metrics.
|
@masih @alexander-sei My AI claimed not being able to merge into main could be because of this: Yes. This job died because the RPC node never got a peer to dial, and that comes from the new /net_info peers list. The merge-queue run for this PR cancelled Integration Test (Chain Operation Test) at the 30-minute limit. The chain-operation tests never started. The step was stuck on seid status inside sei-rpc-node, which got connection refused on port 26657 from 20:59 until 21:27. rpc-node.log shows why the port was closed. State sync runs inside reactor startup, before the RPC server is bound, and it sat at peers=0 for 28 minutes (target=2). The router log is out=0/20 the whole time, with no dial attempts. The RPC node fills persistent-peers from node 0: step1_configure_init.sh Before this PR, AllAddrs still included addresses learned over PEX. Nodes 1–3 each had two outbound connections, so those dialed addresses were in node 0's response and the RPC node could connect. The other integration jobs in this run passed; this one is the case where node 0 accepted every connection inbound. |
|
@wen-coding ty! Yes I am aware and disscussed with @alexander-sei in DMs. This is why I explicitly removed it from the merge queue. We can ship this one in the second patch of v6.7 after migration settles 🙌 ty for your review and letting me take your time on this 🙇 |
|
Taking the path of least resistence: only count connected peers as n_peers. That has the least ecosystem breakage since they read these values as it is understood by cometbft. I will add extra prometheus metrics in a separate PR. |
|
Successfully created backport PR for |
Follow-up to #4500, which changes `/net_info` `n_peers` to count connected peers so it stops reporting the ~180-200 known addresses that operators took for a broken peer cap. The known-address count is still worth watching, for example to see how much the PEX tables grow, so this exports it as a Prometheus gauge rather than an RPC field, as wen suggested on #4500. `tendermint_p2p_known_addresses` is the length of `peerManager.AllAddrs()`: distinct node IDs across the live connections and the persistent and regular PEX tables, the same set `/net_info` `peers` lists. `NewRouter` seeds it and `metricsRoutine` refreshes it every 10s, next to `tendermint_p2p_peers`, which keeps counting live connections and is unchanged. The seed-node Prometheus test now also checks that the new series is served. This only adds a metric, with no consensus or state impact. (cherry picked from commit 2009d55)
) Follow-up to sei-protocol#4500, which changes `/net_info` `n_peers` to count connected peers so it stops reporting the ~180-200 known addresses that operators took for a broken peer cap. The known-address count is still worth watching, for example to see how much the PEX tables grow, so this exports it as a Prometheus gauge rather than an RPC field, as wen suggested on sei-protocol#4500. `tendermint_p2p_known_addresses` is the length of `peerManager.AllAddrs()`: distinct node IDs across the live connections and the persistent and regular PEX tables, the same set `/net_info` `peers` lists. `NewRouter` seeds it and `metricsRoutine` refreshes it every 10s, next to `tendermint_p2p_peers`, which keeps counting live connections and is unchanged. The seed-node Prometheus test now also checks that the new series is served. This only adds a metric, with no consensus or state impact.
/net_infobuildsn_peersfromRouter.AllAddrs(), which merges the live connections with every address in the persistent and regular PEX tables. On pacific-1 that reports roughly 175-200 "peers" on nodes that are actually connected to around 45, which an operator runningmax-connections = 31took for a broken peer cap. CometBFT tooling and dashboards readn_peersas the connected-peer count. The known-peer semantics come from the Tendermint v0.35 fork (PeerManager.Peers()over the peer store); #2539 moved the source to the pool address maps, and #3037 switched it toAllAddrs()while movingpeer_connectionsto live connections, which is why the two now disagree.Environment.NetInfonow setsn_peersto the number of connected peers, the same set aspeer_connections.peerskeeps listing every known address, because the rpcnode init script and the sei-infra state-sync and init scripts seedpersistent-peersfrom.peers[].url. Limiting it to connected peers left those lists empty whenever the source node only had inbound peers without anexternal-address, which is what made the Chain Operation Test time out in the merge queue. Exposing the known-address count as a Prometheus metric is left to a follow-up.This is an RPC-only change with no consensus or state impact.
TestNetInfo_NPeersCountsConnectedPeersandTestNetInfo_PeersIncludesPexAddressesboth fail on main and pass here.