{core,swarm}/: Allow configuring dial concurrency factor per dial #2404
Conversation
Enable a `NetworkBehaviour` or a user via `Swarm::dial` to override the dial concurrency factor per dial. This is especially relevant in the case of libp2p-autonat where one wants to probe addresses in sequence to reduce the amount of work a remote peer can force onto the local node. To enable the above, this commit also: - Introduces `libp2p_core::DialOpts` mirroring `libp2p_swarm::DialOpts`. Passed as an argument to `Network::dial`. - Removes `Peer::dial` in favor of `Network::dial`. - Simplifies `Swarm::dial_with_handler`. The introduction of `libp2p_core::DialOpts` will be useful beyond this feature, e.g. for libp2p#2363. In the long run I would like to move and merge `libp2p_core::Network` and `libp2p_core::Pool` into `libp2p_swarm::Swarm` thus deduplicating `libp2p_core::DialOpts` and `libp2p_swarm::DialOpts`. Fixes libp2p#2385.
|
@thomaseizinger @elenaf9 @MarcoPolo does one of you have some time to give this a review? |
|
Yep! I'll tal tomorrow morning |
|
|
||
| impl From<Multiaddr> for DialOpts { | ||
| fn from(address: Multiaddr) -> Self { | ||
| DialOpts::unknown_peer_id().address(address).build() |
There was a problem hiding this comment.
Should we check if the multiaddr ends with /p2p/<peerid> and use the withpeerid variant?
| @@ -432,7 +403,7 @@ where | |||
| THandler: IntoConnectionHandler, | |||
| { | |||
| peer_id: PeerId, | |||
| network: &'a mut Network<TTrans, THandler>, | |||
There was a problem hiding this comment.
Why this change? Are you planning on removing this field?
There was a problem hiding this comment.
Mostly for the sake of consistency. Each of the XXXPeer, e.g. ConnectedPeer and DisconnectedPeer, contain a &mut Network. Holding this mutable reference is relevant e.g. for ConnectedPeer::some_connection:
/// Obtains some established connection to the peer.
pub fn some_connection(&mut self) -> EstablishedConnection<THandlerInEvent<THandler>> {
self.connections()
.into_first()
.expect("By `Peer::new` and the definition of `ConnectedPeer`.")
}Given that ConnectedPeer is only created when the peer is actually connected and given that ConnectedPeer holds a &mut Network and thus a user could not disconnect the peer in the meantine, ConnectedPeer::some_connection can do the expect call.
Does that make sense @MarcoPolo?
In the long run I would like to move and merge
libp2p_core::Networkandlibp2p_core::Poolinto libp2p_swarm::Swarmthus deduplicatinglibp2p_core::DialOptsandlibp2p_swarm::DialOpts`.
Following up on the above from the PR description, I would likely suggest getting rid of all of peer.rs in the same go. While I think the Peer construct offers a cool user experience, I don't think it makes sense for an rust-libp2p internal API.
There was a problem hiding this comment.
Yeah, I think that makes sense. Even though we never use _network anywhere, we still want to keep it so that we know that nothing else has a &mut on Network. Is that correct?
Enable a
NetworkBehaviouror a user viaSwarm::dialto override thedial concurrency factor per dial. This is especially relevant in the
case of libp2p-autonat where one wants to probe addresses in sequence to
reduce the amount of work a remote peer can force onto the local node.
To enable the above, this commit also:
libp2p_core::DialOptsmirroringlibp2p_swarm::DialOpts.Passed as an argument to
Network::dial.Peer::dialin favor ofNetwork::dial.Swarm::dial_with_handler.The introduction of
libp2p_core::DialOptswill be useful beyond thisfeature, e.g. for #2363.
In the long run I would like to move and merge
libp2p_core::Networkand
libp2p_core::Poolintolibp2p_swarm::Swarmthus deduplicatinglibp2p_core::DialOptsandlibp2p_swarm::DialOpts.Fixes #2385.
//CC @elenaf9