swarm/: Enable advanced dialing requests #2317

mxinden · 2021-10-29T16:21:50Z

Enable advanced dialing requests both on Swarm and via
NetworkBehaviourAction. Users can now trigger a dial with a specific
set of addresses, optionally extended via
NetworkBehaviour::addresses_of_peer. In addition the whole process is
now modelled in a type safe way via the builder pattern.

Example of a NetworkBehaviour requesting a dial to a specific peer
with a set of addresses additionally extended through
NetworkBehaviour::addresses_of_peer:

NetworkBehaviourAction::Dial {
    opts: DialOpts::peer_id(peer_id)
              .condition(PeerCondition::Always)
              .addresses(addresses)
              .extend_addresses_through_behaviour()
              .build(),
    handler,
}

Example of a user requesting a dial to an unknown peer with a single
address via Swarm:

swarm1.dial(
    DialOpts::unknown_peer_id()
        .address(addr2.clone())
        .build()
)

Fixes swarm/: Enable dialing a specific fixed set of addresses for a single peer #2249.
Enables protocols: Implement Direct Connection Upgrade through Relay (dcutr) #2076 and protocols/: Add basic AutoNAT implementation #2262 to provide a fixed exclusive set of addresses to dial (//CC @elenaf9).

Creating this as a draft for now. I have only updated libp2p-swarm for now.

What do folks think of the interface? (//CC @thomaseizinger as this follows your second suggestion provided in #2249 (comment).)

Enable advanced dialing requests both on `Swarm` and via `NetworkBehaviourAction`. Users can now trigger a dial with a specific set of addresses, optionally extended via `NetworkBehaviour::addresses_of_peer`. In addition the whole process is now modelled in a type safe way via the builder pattern. Example of a `NetworkBehaviour` requesting a dial to a specific peer with a set of addresses additionally extended through `NetworkBehaviour::addresses_of_peer`: ```rust NetworkBehaviourAction::Dial { opts: DialOpts::peer_id(peer_id) .condition(PeerCondition::Always) .addresses(addresses) .extend_addresses_through_behaviour() .build(), handler, } ``` Example of a user requesting a dial to an unknown peer with a single address via `Swarm`: ```rust swarm1.dial( DialOpts::unknown_peer_id() .address(addr2.clone()) .build() ) ```

elenaf9

I really like the new interface, would be very useful to have this feature!

elenaf9 · 2021-10-30T15:27:06Z

swarm/src/lib.rs

+    pub enum DialOpts {
+        // TODO:  How about folding these structs into the enum variants?
+        WithPeerId(WithPeerId),
+        WithPeerIdWithAddresses(WithPeerIdWithAddresses),
+        WithoutPeerIdWithAddress(WithoutPeerIdWithAddress),
+    }
+
+    impl DialOpts {


Suggested change

pub enum DialOpts {

// TODO: How about folding these structs into the enum variants?

WithPeerId(WithPeerId),

WithPeerIdWithAddresses(WithPeerIdWithAddresses),

WithoutPeerIdWithAddress(WithoutPeerIdWithAddress),

}

impl DialOpts {

enum Opts {

// TODO: How about folding these structs into the enum variants?

WithPeerId(WithPeerId),

WithPeerIdWithAddresses(WithPeerIdWithAddresses),

WithoutPeerIdWithAddress(WithoutPeerIdWithAddress),

}

pub struct DialOpts {

opts: Opts

}

impl DialOpts {

Consider wrapping the enum in a struct to emphasize that the user should not try to build the variants directly, but instead use the methods.

Given the structs WithPeerId, WithPeerIdWithAddresses and WithoutPeerIdWithAddress can not be constructed outside of dial_opts, the user can not build DialOpts themselves. Still I understand that this is not intuitive and that it would be nice to make it clear that the user should go through the methods on DialOpts.

Is wrapping an enum into a struct an idiomatic way in Rust to signal that something should not be constructed directly? I don't recall seeing this in the past. Any prominent examples? I would have just relied on a doc comment on DialOpts.

Is wrapping an enum into a struct an idiomatic way in Rust to signal that something should not be constructed directly? I don't recall seeing this in the past.

I would argue it the other way around: I feel like it is very rare and not rust-idiomatic that an enum is not constructed directly. Of course a user would notice that it is not possible once they try to build the inner structs, but it still may cause some confusion. With a struct, the API would make it clearer that it should be constructed via methods, especially when looking at it on docs.rs.

I don't recall seeing this in the past. Any prominent examples?

I feel like this is just following the general api design pattern of wrapping internal/ inner types in a new struct to hide implementation details. Imo this always makes sense when the inner type is not relevant for the user and should never be constructed directly by them, which is the case here.

Alternatively, if you decide to fold the structs into the enum variants as you wrote in your TODO in l.1786:

// TODO: How about folding these structs into the enum variants

the user would be able to build the enum types directly and the builder pattern would not be needed at all.

Addressed in ad083e9.

elenaf9 · 2021-10-30T15:27:33Z

swarm/src/lib.rs

+        pub(crate) extend_addresses_through_behaviour: bool,
+    }
+
+    impl WithPeerIdWithAddresses {


Suggested change

impl WithPeerIdWithAddresses {

impl WithPeerIdWithAddresses {

pub fn condition(mut self, condition: PeerCondition) -> Self {

self.condition = condition;

self

}

Maybe add this method here as well.

I am a bit reluctant to offer two ways to achieve the same thing. In what use-case would one want to call condition on WithPeerIdWithAddresses and couldn't do so on WithPeerId?

I am also a bit undecided in this aspect.
But the DialOpts are trying to follow the syntax of a builder pattern, and with a builder it normally doesn't matter in which order you specify the paramters, so you would expect that DialOpts::peer_id(...).condition(..).addresses(..) and DialOpts::peer_id(...).addresses(..).condition(..) both work.
That being said, the current implementation doesn't strictly follow the builder pattern anyway (at least not the way it is described in the Rust API Guidelines), therefore I also see the point of avoiding duplicated code.

Addressed in 26dedbe.

elenaf9 · 2021-10-30T16:03:32Z

swarm/src/lib.rs

+        pub fn unknown_peer_id() -> WithoutPeerId {
+            WithoutPeerId {}
+        }


Suggested change

pub fn unknown_peer_id() -> WithoutPeerId {

WithoutPeerId {}

}

pub fn address(address: Multiaddr) -> WithoutPeerIdWithAddress {

WithoutPeerIdWithAddress { address }

}

I do like the consistent syntax of DialOpts::peer_id(..).addresses() and DialOpts::unknown_peer_id(..).address(..).
But this similar syntax suggests that WithPeerId and WithoutPeerId are alike, which is not the case because WithPeerId can be a "final" state from which you can call build(), whereas WithoutPeerId is only intermediate. Not sure about this, but I feel like it could end up confusing the user, since even though it follows the same naming convention, WithoutPeerId is not a valid DialOpt.

Optionally if we follow the above suggestion, we could keep it consist by also adding

impl WithoutPeerIdWithAddress { pub fn peer_id(self, peer_id: PeerId) -> WithPeerIdWithAddresses { WithPeerIdWithAddresses { peer_id, condition: Default::default(),, addresses: vec![self.address], extend_addresses_through_behaviour: false, } } }

The only problem with this is that without a peer id you only have one address, whereas with an peer id there can be multiple.

Say we allowed DialOpts::address(..) instead of DialOpts::unknown_peer_id(..).address(..), would that not confuse a user into thinking that they can then later on add a peer id, i.e. that they can do DialOpts::address(..).peer_id(..)?

The only problem with this is that without a peer id you only have one address, whereas with an peer id there can be multiple.

Yes, this is something that I would like to encode at the type level, thus the differentiation between the two. I am not sure how to model your suggestion without loosing the type safety.

Say we allowed DialOpts::address(..) instead of DialOpts::unknown_peer_id(..).address(..), would that not confuse a user into thinking that they can then later on add a peer id, i.e. that they can do DialOpts::address(..).peer_id(..)?

I agree. But with reference to the above discussion: the pattern currently already expects a fixed order in which the parameters are specified, e.g. as written above: in case of a known peer id you have to specify the condition before specifying the addresses. Isn't that kind of the same issue?

Yes, this is something that I would like to encode at the type level, thus the differentiation between the two. I am not sure how to model your suggestion without loosing the type safety.

You are right, the type safety should be kept; DialOpts::address(..).peer_id(..) should not be possible.

mxinden · 2021-11-03T12:14:06Z

Thanks @elenaf9 for giving this a review and helping to design this interface. I replied to all of your comments. I am curious what you think.

mxinden · 2021-11-09T13:56:19Z

@elenaf9 do you want to take another look?

@thomaseizinger would very much appreciate your input here.

thomaseizinger

Nice work!

Two suggestions but those could also be shipped later!

thomaseizinger · 2021-11-11T06:02:41Z

swarm/src/lib.rs

+    ///      .build()
+    /// );
+    /// ```
+    pub fn dial(&mut self, opts: DialOpts) -> Result<(), DialError> {


Given how common the following is:

DialOpts::unknown_peer_id().address(listen_addr).build()

We could consider accepting impl Into<DialOpts>:

Suggested change

pub fn dial(&mut self, opts: DialOpts) -> Result<(), DialError> {

pub fn dial(&mut self, opts: impl Into<DialOpts>) -> Result<(), DialError> {

and implement:

impl From<MultiAddress> for DialOpts { }

where we do exactly the above.

Totally agree we can get rid of a boilerplate code like this. Another option is to accept an impl DialOptsBuilder and call: .dial(DialOpts::.address(listen_addr)).

Given how common the following is:

DialOpts::unknown_peer_id().address(listen_addr).build()

We could consider accepting impl Into<DialOpts>:

and implement:

impl From<MultiAddress> for DialOpts { }

where we do exactly the above.

Neat! Thanks for bringing it up @thomaseizinger. I will give this a shot.

5e3edb3 implements From<Multiaddr> and changes calls to unknown_peer_id().address(xxx).build() to the address only.

Totally agree we can get rid of a boilerplate code like this. Another option is to accept an impl DialOptsBuilder and call: .dial(DialOpts::.address(listen_addr)).

I think From<Multiaddr> is the more elegant way. Given that the two solve the same issue, I would prefer the earlier over the later suggestion.

thomaseizinger · 2021-11-11T06:06:15Z

swarm/src/dial_opts.rs

+
+impl WithPeerIdWithAddresses {
+    /// Specify a [`PeerCondition`] for the dial.
+    pub fn condition(mut self, condition: PeerCondition) -> Self {


This API requires users to import one more type: PeerCondition. Might be worth providing 3 functions instead that configure the corresponding condition.

Please excuse if that has been discussed and dismissed already!

Modelling 3 exclusive states with 3 methods instead of an enum seems not intuitive to me. Is the additional import of PeerCondition worth the added complexity?

I think there is a misunderstanding. I would still model the states internally with an enum. However, nothing stops anyone from doing this:

DialOpts::address(addr).condition(...).condition(...)

It might give people a hint that the condition is overridden but in theory, it could also be implemented in an additive way.
I don't feel strongly about it so happy to keep the enum :)

I think there is a misunderstanding. I would still model the states internally with an enum.

No misunderstanding :)

I don't feel strongly about it so happy to keep the enum :)

Will keep the enum for now. Not 100% happy but can't think of a better alternative. We can still further iterate on it. Merging for now.

elenaf9

Looks good to me!

kpp · 2021-11-15T10:29:05Z

protocols/identify/src/identify.rs

+
+        // We should still be able to dial now!
+        swarm2
+            .dial(DialOpts::peer_id(swarm1_peer_id).build())


impl From<PeerId> for DialOpts too?

👍 good idea. Done via 09293c4.

mxinden added 2 commits October 29, 2021 18:14

swarm/src/lib: Emit SwarmEvent::Dialing

09688c7

elenaf9 reviewed Oct 30, 2021

View reviewed changes

mxinden added 2 commits November 3, 2021 12:52

Merge branch 'libp2p/master' into advanced-dialing

85b6238

swarm/src/lib: Fold condition check into first match

147c533

mxinden added 17 commits November 3, 2021 13:30

protocols/kad: Adjust to API changes

2bda3b3

swarm/src/lib: Wrap Opts in DialOpts

ad083e9

protocols/identify: Update to latest changes

eea9e12

swarm/src/lib: Allow WithPeerIdWithAddresses::condition

26dedbe

protocols/floodsub: Adjust to latest changes

eb67288

protocols/req-resp: Adjust to latest changes

a04eaa3

protocols/relay/src/behaviour: Update to recent changes

57f0648

protocols/gossipsub: Adjust to latest changes

770e3af

*: Adjust to latest changes

96ac37e

protocols/identify: Wait for connection to close before redialing

9f941fd

swarm/: Add documentation

d4410bb

swarm/src/lib: Deduplicate addresses

163229c

swarm/CHANGELOG: Add entry

8334f36

*: Bump versions

1e561e8

Merge branch 'libp2p/master' into advanced-dialing

2f1ae01

swarm/src/lib: Move dial_opts to separate file

036cf24

swarm/src/dial_opts: Remove outdated comment

eab21a6

mxinden marked this pull request as ready for review November 9, 2021 13:54

mxinden requested a review from thomaseizinger November 9, 2021 13:54

thomaseizinger approved these changes Nov 11, 2021

View reviewed changes

elenaf9 approved these changes Nov 11, 2021

View reviewed changes

mxinden added 2 commits November 14, 2021 22:02

swarm/src/dial_opts: Implement From<Multiaddr> for DialOpts

5e3edb3

Merge branch 'libp2p/master' into advanced-dialing

fed72bf

kpp reviewed Nov 15, 2021

View reviewed changes

mxinden added 4 commits November 15, 2021 11:27

swarm/src/dial_opts: Implement From<PeerId> for DialOpts

09293c4

Merge branch 'master' into advanced-dialing

9a52b30

misc/metrics: Fix type inference

8d9b548

examples: Help type inference

1562159

kpp approved these changes Nov 15, 2021

View reviewed changes

mxinden added 4 commits November 15, 2021 12:23

protocols/.*/examples: Run cargo fix

d11a4ed

*: Run cargo fmt

03ef106

src/tutorial: Fix type inference

8415ba2

swarm/src/lib: Help type inference

9ada868

mxinden merged commit 220f84a into libp2p:master Nov 15, 2021

mxinden mentioned this pull request Nov 15, 2021

protocols/: Add basic AutoNAT implementation #2262

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swarm/: Enable advanced dialing requests #2317

swarm/: Enable advanced dialing requests #2317

mxinden commented Oct 29, 2021

elenaf9 left a comment •

edited

elenaf9 Oct 30, 2021

mxinden Nov 3, 2021

elenaf9 Nov 3, 2021

mxinden Nov 9, 2021

elenaf9 Oct 30, 2021

mxinden Nov 3, 2021

elenaf9 Nov 3, 2021

mxinden Nov 9, 2021

elenaf9 Oct 30, 2021 •

edited

mxinden Nov 3, 2021

elenaf9 Nov 3, 2021

mxinden commented Nov 3, 2021

mxinden commented Nov 9, 2021

thomaseizinger left a comment

thomaseizinger Nov 11, 2021

kpp Nov 11, 2021

mxinden Nov 13, 2021

mxinden Nov 14, 2021

thomaseizinger Nov 11, 2021

mxinden Nov 13, 2021

thomaseizinger Nov 14, 2021

mxinden Nov 15, 2021

elenaf9 left a comment

kpp Nov 15, 2021

mxinden Nov 15, 2021

	pub fn dial(&mut self, opts: DialOpts) -> Result<(), DialError> {
	pub fn dial(&mut self, opts: impl Into<DialOpts>) -> Result<(), DialError> {

swarm/: Enable advanced dialing requests #2317

swarm/: Enable advanced dialing requests #2317

Conversation

mxinden commented Oct 29, 2021

elenaf9 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elenaf9 Oct 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Nov 3, 2021

mxinden commented Nov 9, 2021

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elenaf9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elenaf9 left a comment •

edited

elenaf9 Oct 30, 2021 •

edited