Skip to content

List and count peers by identity - #273

Merged
Bronek merged 4 commits into
mainfrom
bronek/peer_inventory_by_identity
Sep 18, 2026
Merged

Bronek merged 4 commits into
mainfrom
bronek/peer_inventory_by_identity

Conversation

@Bronek

@Bronek Bronek commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Count and list unique peer identities, keeping a peer visible while any observed connection remains. Use the latest observed connection for its address and direction.

Add coverage for duplicate connections and both disconnect orders, simplify response assertions, and document peer inventory limitations in ADR 0004

@Bronek
Bronek requested a review from ninaiiad September 16, 2026 14:15
@Bronek
Bronek added this pull request to stack #270 September 16, 2026 14:15
@Bronek
Bronek force-pushed the bronek/peer_inventory_by_identity branch from 22be4c1 to af1051f Compare September 16, 2026 15:10
@Bronek
Bronek marked this pull request as ready for review September 16, 2026 15:11
Comment thread crates/beacon_api/src/peers.rs Outdated
let entry = latest.entry(peer.id).or_insert((*arrival, peer));
if *arrival > entry.0 {
*entry = (*arrival, peer);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

so you are deduplicating from one map into another map and updating other the latest arrival time?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why are there duplicates in the peer table? and if it is possible wouldn't it be better to replace on insertion rather than having 2 entries?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Simplest way this may happen - outgoing and incoming connection to/from the same peer.

Comment thread crates/beacon_api/src/peers.rs Outdated
*entry = (*arrival, peer);
}
}
latest.into_values().map(|(_, peer)| peer).collect()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

and the discarding the arrival time and collecting into a vec?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if you didn't have to track arrival times and multiple connection per Peer (i.e. deduplicate on insertion), then you could just return an impl Iterator<Item = &Peer> from self.connections.values()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

and so avoid allocating multiple collections

@Bronek Bronek Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

See 012aca0

Comment thread crates/beacon_api/src/peers.rs Outdated
pub(crate) fn len(&self) -> usize {
self.0.len()
pub(crate) fn connected(&self) -> usize {
self.by_identity().len()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this allocates the peer table twice

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, see 012aca0

Comment thread crates/beacon_api/src/peers.rs Outdated

pub(crate) fn matching<'a>(&'a self, filter: &'a PeerFilter) -> impl Iterator<Item = &'a Peer> {
self.0.values().filter(move |peer| filter.admits(peer))
self.by_identity().into_iter().filter(move |peer| filter.admits(peer))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

as does this

@Bronek Bronek Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

See 012aca0

Comment thread crates/beacon_api/src/peers.rs Outdated
pub(crate) fn insert(&mut self, connection: usize, peer: Peer) {
self.0.insert(connection, peer);
self.arrivals += 1;
self.connections.insert(connection, (self.arrivals, peer));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

you have the Peer here so you could deduplicate by PeerId here. And it is perfectly possible to handle event orders like Connected, Connected, Disconnected, Disconnected.

For example:

self.connections = FxHashMap::<PeerId, (Peer, u8)>::default();

fn insert(&mut self, peer: Peer) {
    self.connections.entry(peer.id).and_modify(|(_, count)| (peer, *count += 1)).or_insert((peer, 1));
}

// decrement count on `remove` and actually remove if `count == 0`
// then to return peers

fn peers(&self) -> impl Iterator<Item = &Peer> {
    self.connections.values().map(|(peer, _)| peer)
}

@Bronek Bronek Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

See 012aca0

Note, this version is storing connections for each peer peers: FxHashMap<PeerId, Vec<Connection>> - this is to enable API to keep accounting of peers. There probably are alternative solutions, exploring this space.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The reason why we need to retain Connection is that a failed handshake can produce a disconnect without a preceding connect. We need to be able to filter our such disconnects. The current implementation in PeerTable::remove does handle it gracefully; the alternative is to let PeerManager publish its selected peers - a new message, only for Beacon API. I think that would be worse.

@Bronek
Bronek force-pushed the bronek/peer_inventory_by_identity branch from af1051f to 37c27a4 Compare September 17, 2026 08:54
@Bronek
Bronek force-pushed the bronek/peer_inventory_by_identity branch from 37c27a4 to 6e4fa14 Compare September 17, 2026 10:08
@Bronek
Bronek force-pushed the bronek/peer_inventory_by_identity branch 2 times, most recently from d076272 to 4b95ded Compare September 17, 2026 12:35
@Bronek
Bronek force-pushed the bronek/peer_inventory_by_identity branch from 04d7d8e to 012aca0 Compare September 17, 2026 16:26
Base automatically changed from bronek/control_stale_following_fix to main September 18, 2026 09:39
@Bronek
Bronek force-pushed the bronek/peer_inventory_by_identity branch 2 times, most recently from 0c091cb to 1807b97 Compare September 18, 2026 09:58
Deduplicate peer API responses while retaining each connection until its
disconnect event. Use the latest observed connection for each peer's
address and direction, tracking arrival order independently of reusable
connection handles.

Cover duplicate identities, decreasing handles, and both disconnect
orders. Check peer response JSON values and invalid-filter HTTP status
without requiring exact body bytes or header order.

Document the connected-only inventory, null ENRs, and missed connection
events before the API's first read in ADR 0004.

Assisted-by: Claude:claude-fable-5-1
Assisted-by: Codex:gpt-6-astra
Keep each identity's connections in arrival order and select the latest
survivor for API output. Count identities directly and list peers through
an iterator, removing per-query deduplication and its temporary map and
vector.

Use the identity and handle in disconnect events to remove the closed
connection. Preserve the surviving connection's address and direction,
including when the newest connection closes first.

Extend tests for direction filters, three overlapping connections, and
unknown or repeated events. Update ADR 0004's description of connection
storage.

Assisted-by: Codex:gpt-6-astra
@Bronek
Bronek force-pushed the bronek/peer_inventory_by_identity branch from 3b2e7d9 to 739d7e0 Compare September 18, 2026 11:33
@Bronek
Bronek merged commit 66e5039 into main Sep 18, 2026
3 checks passed
@Bronek
Bronek deleted the bronek/peer_inventory_by_identity branch September 18, 2026 11:41
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.

2 participants