Basic peer manager based on libp2p peer_store and connection_limits
#126
Conversation
|
I propose we review and merge this, even though upstream has not merged the peer store yet. We need something to test. Very open for other suggestions though |
| .filter_map(|(enr, _)| manager.discovered_peer(enr)) | ||
| .collect::<Vec<_>>(); | ||
| for dial in to_dial { | ||
| let _ = self.swarm.dial(dial); |
There was a problem hiding this comment.
Lighthouse does something like that https://github.com/sigp/lighthouse/blob/unstable/beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs#L113
There was a problem hiding this comment.
The event only allows us to dial a single peer.
There was a problem hiding this comment.
Yes, I think it creates an event for each peer. But honestly, I don't know the reason. Maybe decoupling?
There was a problem hiding this comment.
@jxs: Is there a relevant difference between these approaches to dial?
There was a problem hiding this comment.
We could also encapsulate this code inside the Peer Manager. Similar to https://github.com/sigp/lighthouse/blob/stable/beacon_node/lighthouse_network/src/service/mod.rs#L1845
# Conflicts: # Cargo.lock # anchor/network/src/network.rs
# Conflicts: # anchor/network/src/discovery.rs # anchor/network/src/network.rs
| @@ -12,7 +12,7 @@ ethereum_ssz = { workspace = true } | |||
| ethereum_ssz_derive = { workspace = true } | |||
| futures = { workspace = true } | |||
| hex = "0.4.3" | |||
| libp2p = { version = "0.54", default-features = false, features = [ | |||
| libp2p = { git = "https://github.com/libp2p/rust-libp2p.git", rev = "082eb16", default-features = false, features = [ | |||
There was a problem hiding this comment.
interesting! Didn't know one could this, I suggest we use our own fork or yours Daniel, as Dr Huang may rebase and I fear that commit may disapear, wdyt?
There was a problem hiding this comment.
I don't think that can happen. The commit can disappear from the branch, but it stays in the repo if it was pushed. For example this commit is still accessible via the link: sigp/lighthouse@7f33d35, even though it no longer is in the current commit chain of the PR branch.
| .filter_map(|(enr, _)| manager.discovered_peer(enr)) | ||
| .collect::<Vec<_>>(); | ||
| for dial in to_dial { | ||
| let _ = self.swarm.dial(dial); |
| local_addr: &Multiaddr, | ||
| remote_addr: &Multiaddr, | ||
| ) -> Result<(), ConnectionDenied> { | ||
| self.peer_store.handle_pending_inbound_connection( |
There was a problem hiding this comment.
for the sake of consistency I suggest we always check the connection_limits first as in the handle_pending_outbound connection as priority matters in these checks. By checking first peer_behaviour first we may introduce a peer to the peer-store that then may be reject by connection-limits and therefore we aborting the connection to it
There was a problem hiding this comment.
I put it intentionally this way to add the peer to the peer store regardless (so that we are aware of it and can use it if we need any peers). What do you think?
There was a problem hiding this comment.
ah! Interesting, yeah that seems clever Daniel, can we then leave a comment then explaining it?
| local_addr: &Multiaddr, | ||
| remote_addr: &Multiaddr, | ||
| ) -> Result<(), ConnectionDenied> { | ||
| self.peer_store.handle_pending_inbound_connection( |
There was a problem hiding this comment.
ah! Interesting, yeah that seems clever Daniel, can we then leave a comment then explaining it?
| @@ -12,7 +12,7 @@ ethereum_ssz = { workspace = true } | |||
| ethereum_ssz_derive = { workspace = true } | |||
| futures = { workspace = true } | |||
| hex = "0.4.3" | |||
| libp2p = { version = "0.54", default-features = false, features = [ | |||
| libp2p = { git = "https://github.com/libp2p/rust-libp2p.git", rev = "082eb16", default-features = false, features = [ | |||
|
Thanks everyone! 🎉 |
Draft of basic peer manager using
connection_limitsand a preliminary version ofpeer_store.This is just a draft, as the exact design of
peer_storeis still TBD and a few things are missing.