-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add libp2p-connection-tracker for simplified peer connection state management #6046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
2725dca
to
74197a5
Compare
Thanks for the PR! |
Because, unfortunately, it's not available inside a |
Yes you're right. But from my own experience, most behaviors want to track some custom state for each connected node anyway. |
Yes, I considered this. But, currently, the |
I can try to remove the behavior part and make it just a component that is part of the |
I would consider On the other hand I do like having small, composable behaviors. I am just afraid that the number of behaviors could explode rather quickly if we add a separate behavior for every minor thing (it also results in larger
Side note: we already have |
IMO |
Thanks for pointing this out. Could it be used for what is described below?
This PR is in the context of sigp/anchor#135 In the current PR I just wanted to draft an initial idea to move this to libp2p |
Hi Elena, I also agree with your vision for the
From my experience a
yes, only the timings of banning are missing, which we can implement |
Thanks for the PR! Im not 100% sure about this since as @elenaf9 mentioned that most behaviours would want to track the connection state of each peer for explicit reasons. However, if we were to have this, wouldn't it be better for it to be apart of |
Thanks a lot, everyone, for the feedback! I removed all the |
d1bd05a
to
31873db
Compare
31873db
to
e6048c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
IMO we should just do the tracking of connections directly in the Behavior
instead of only in the MemoryStore
. That way also other Store
implementations can use it.
PeerConnected { | ||
peer_id: PeerId, | ||
connection_id: ConnectionId, | ||
endpoint: ConnectedPoint, | ||
}, | ||
|
||
/// A peer disconnected (last connection closed). | ||
PeerDisconnected { | ||
peer_id: PeerId, | ||
connection_id: ConnectionId, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need these events. We already have the swarm-events for established and closed connections, that include the info how many other connections to that peer exist.
let is_first_connection = self | ||
.connection_store | ||
.connection_established(peer_id, connection_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just check here if ConnectionEstablished { other_established, .. }
is 0
.
}) => { | ||
trace!(%peer_id, ?connection_id, remaining_established, "Connection closed"); | ||
|
||
let is_last_connection = self.connection_store.connection_closed( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, just check if remaining_established == 0
.
/// Simple storage for connected peers. | ||
#[derive(Debug, Default)] | ||
pub struct ConnectionStore { | ||
/// Currently connected peers with their connection IDs | ||
connected: HashMap<PeerId, HashSet<ConnectionId>>, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs to be a separate module, or even own structure, given that all it does is just adding and removing peers from a hashmap.
Why not just embed the logic directly in Behavior
?
@jxs yes I agree, I am also not super happy with the fact that we now duplicate swarm logic. |
Yeah agree with you Elena, with this being part of the |
I'm still on the fence if this should be part of the |
It's not necessarily about creating a I personally wouldn't expect connection management in a module called |
It's not managing connections, it's just tracking connected peers 🙂 .
Please correct me if I am wrong, I am not really familiar with nim-libp2p, but it seems like the connmanager in nim does a lot more things than just tracking the connected peers (e.g. getting a new stream on a connection, reacting to new connections, etc.). Those are all solved in rust-libp2p in the swarm, so I am not sure if we can really compare this? |
I think #6012 can be an interesting idea, although the maintainers deemed it unnecessary. |
How about we add a |
Then we do something like below instead of keeping an additional data structure:
|
Description
This PR introduces a new libp2p-connection-tracker crate that provides a lightweight
NetworkBehaviour
for tracking connected peers. The primary motivation is to eliminate the need for manual connection state management in application-level behaviours.Notes & open questions
Problem
Currently, behaviours that need to track connected peers must manually maintain connection state using patterns like:
This approach is error-prone and requires each behaviour to correctly handle connection lifecycle events, multiple connections per peer, and state synchronization.
Solution
The new connection-tracker behaviour provides a composable solution following libp2p's architectural patterns:
Design Choices
Implementation
store.rs: Simple storage layer with
HashMap<PeerId, HashSet<ConnectionId>>
behaviour.rs:
NetworkBehaviour
implementation usingdummy::ConnectionHandler
lib.rs: Public API and event definitions
The implementation follows the hierarchical state machine pattern and coding guidelines used throughout rust-libp2p.
Future Extensions
This foundation enables future enhancements like:
Change checklist