feat(rpc): add eth_flashblocksPeerStatus API for P2P peer visibility#258
feat(rpc): add eth_flashblocksPeerStatus API for P2P peer visibility#258louisliu2048 merged 4 commits intomainfrom
Conversation
Expose real-time flashblocks P2P peer state via a new JSON-RPC method, enabling operators to monitor connected, disconnected, and never-connected static peers with timing information for debugging broadcast issues. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@claude review for me |
XLayer-Bot
left a comment
There was a problem hiding this comment.
Clean implementation overall — the design is well-structured, the event-hook wiring through the libp2p swarm loop is correct, and the test suite covers the key state transitions. Three concerns below.
| let connected = peers.iter().filter(|p| p.connection_state == "connected").count(); | ||
| let disconnected = peers.iter().filter(|p| p.connection_state == "disconnected").count(); | ||
| let never_connected = | ||
| peers.iter().filter(|p| p.connection_state == "never_connected").count(); |
There was a problem hiding this comment.
The summary counts are computed by string-comparing PeerInfo::connection_state (a String) against hard-coded literals:
let connected = peers.iter().filter(|p| p.connection_state == "connected").count();
let disconnected = peers.iter().filter(|p| p.connection_state == "disconnected").count();
let never_connected = peers.iter().filter(|p| p.connection_state == "never_connected").count();The internal PeerConnectionState enum already models these variants exhaustively, so a typo in either the match arm string (line ~119–124) or these filter literals would silently produce wrong summary counts with no compile-time error. The tests embed the same string literals, so a consistent rename would slip past them too.
Suggestion: make PeerConnectionState public, add #[derive(Serialize)] with #[serde(rename_all = "snake_case")], and use it as the type of PeerInfo::connection_state. The summary can then match on the enum and the compiler catches any divergence.
There was a problem hiding this comment.
Resolved in c8060fa.
PeerConnectionState is now a public ConnectionState enum with #[derive(Serialize)] and #[serde(rename_all = "snake_case")]. PeerInfo::connection_state is typed as ConnectionState instead of String, and summary counts use enum comparison. Wire format is unchanged.
|
|
||
| // Register X Layer RPC | ||
| let xlayer_rpc = DefaultRpcExt::new(flashblocks_state); | ||
| let xlayer_rpc = DefaultRpcExt::new(flashblocks_state, fb_p2p_status.get().cloned()); |
There was a problem hiding this comment.
fb_p2p_status.get().cloned() silently returns None if the OnceLock was never set. The OnceLock is filled inside try_build() (called from spawn_payload_service), which runs during component setup. In practice this happens before extend_rpc_modules fires, but this ordering is implicit and undocumented.
If the ordering ever drifts — or if a future code path skips the payload builder — the sequencer's eth_flashblocksPeerStatus would silently return null with no log message indicating a misconfiguration.
Suggest adding a warning log (or a debug-mode assertion) in sequencer mode when the tracker is absent at RPC init time, so the failure is immediately visible:
let peer_status = fb_p2p_status.get().cloned();
if xlayer_args.sequencer_mode && peer_status.is_none() {
warn!(target: "reth::cli", "fb_p2p_status OnceLock not set — eth_flashblocksPeerStatus will return null");
}
let xlayer_rpc = DefaultRpcExt::new(flashblocks_state, peer_status);There was a problem hiding this comment.
Resolved in c8060fa.
Added a tracing::warn! when sequencer_mode && peer_status.is_none() at RPC init time, so the misconfiguration is immediately visible in logs rather than silently returning null.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…vict non-static peers - Replace String-based connectionState with serializable ConnectionState enum to eliminate stringly-typed summary counting (compile-time safety) - Add warning log when sequencer mode but peer status tracker is uninitialized - Remove non-static peers on disconnect to prevent unbounded accumulation from transient mDNS/DHT connections 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@claude review again for me with the new fixes, as well as the enhanced unit test coverages and e2e test coverage. |
XLayer-Bot
left a comment
There was a problem hiding this comment.
All three previously raised concerns have been fully addressed in c8060fa:
1. Type-safe connection state — is now a public enum with and . is typed as ; summary counts use enum equality. No more stringly-typed comparisons.
2. OnceLock ordering visibility — A is emitted when at RPC init time, making the misconfiguration immediately observable in logs rather than silently returning null.
3. Non-static peer accumulation — now removes non-static peers entirely from the tracker (static peers retain their state). This is covered by the new disconnect_non_static_peer_removes_entry unit test.
Unit test coverage is thorough: 12 tests in peer_status.rs covering static/dynamic peer lifecycle, reconnection, broadcast failure/success semantics, snapshot sorting, and no-op operations on unknown peers. The NLL borrow-checker pattern used in on_disconnected (reading entry.is_static then calling HashMap::remove) is valid — the borrow ends at its last-use point under non-lexical lifetimes.
E2E test coverage adds three tests with appropriate #[ignore] annotations on the two tests that require a conductor-enabled devnet with P2P keys, while the null-on-RPC-node test runs unconditionally.
bd2b2ee to
c8060fa
Compare
|
Design overview:
Overall the design is clean and the approach makes sense. |
JimmyShi22
left a comment
There was a problem hiding this comment.
LGTM. One thing worth keeping in mind: the multiaddr field in the response includes internal IPs and ports, which could be sensitive if this API is ever exposed publicly. For internal-only deployments that's fine, but if this endpoint gets added to any public-facing RPC allowlist, it probably shouldn't be included.
|
🔧 CI running — workflow run |
|
✅ XLayer Reth CI passed for commit
|
Summary
Expose the current flashblocks builder p2p peers status, via a new JSON-RPC method
eth_flashblocksPeerStatus, enabling operators to monitor connected, disconnected, and never-connected static peers with timing information for debugging broadcast issues.Note that the static peers configured inside
flashblocks.p2p_known_peersare always shown first in the returned response of the api call.🤖 Generated with Claude Code