refactor(swarm)!: remove `handler` from `NetworkBehaviourAction::Dial` #3328

thomaseizinger · 2023-01-16T03:25:56Z

Description

We create the ConnectionId for the new connection as part of DialOpts. This allows NetworkBehaviours to accurately track state regarding their own dial attempts.

This patch is the main enabler of #3254. Removing the handler field will allow us to deprecate the NetworkBehaviour::new_handler function in favor of four new ones that give more control over the connection lifecycle.

Notes

Extracted out of feat(swarm)!: allow NetworkBehaviours to manage connections #3254.
Depends-On: feat(core)!: make ConnectionIds globally unique #3327.
Depends-On: refactor(swarm): simplify DialOpts #3335
Depends-On: refactor(relay): directly store actions instead of events #3372

Please review this design critically. I believe it has value on its own but at the end of the day, we only do this for #3254.

Links to any relevant issues

Open Questions

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

swarm/src/behaviour.rs

thomaseizinger · 2023-01-17T03:26:05Z

Converting to draft until #3327 is merged to prevent accidental merges.

…ibp2p into 2824-unique-connection-ids

Instead of carrying around an enum and parsing its contents later, directly set the correct values as we construct the final `DialOpts`.

Otherwise the num-established counter is wrong.

swarm/src/lib.rs

We no longer add connections for banned peers to the pool so we don't have to filter them later.

Now that we no longer add connections to banned peers, we need to: - Reduce the number of expected connections by 1 (i.e. delete line 1954) - Act in `Stage::BannedDial` earlier (i.e. remove the `if`)

mergify · 2023-01-27T04:45:19Z

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

thomaseizinger · 2023-01-31T03:38:26Z

Friendly ping @elenaf9 @jxs @mxinden. This is ready for review.

mergify · 2023-02-01T21:33:26Z

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

jxs

Overall looks good to me Thomas! I am not very familiar with dctur and relayyet so Max and Elena should also chime in.

swarm/src/lib.rs

jxs · 2023-02-02T23:29:13Z

swarm/src/connection/pool.rs

+        Ok(())
+    }
+
+    pub fn spawn_connection(


yeah I think this fits here better, and being handled by the Swarm on Swarm::handle_pool_event after receiving ConnectionEstablished rather than on the Pool before returning it 🚀

Yeah it turned out surprisingly clean and fixes up some weird state tracking where we had to "filter" connection IDs that are banned. Now we just don't spawn the connection in the first place :)

protocols/dcutr/src/behaviour_impl.rs

mxinden

Ready to go from my end.

core/src/muxing/boxed.rs

swarm/src/lib.rs

mxinden

This still needs further investigation:

mxinden · 2023-02-09T20:35:12Z

swarm/src/lib.rs

                concurrent_dial_errors,
                established_in,
            } => {
                if self.banned_peers.contains(&peer_id) {
-                    // Mark the connection for the banned peer as banned, thus withholding any
-                    // future events from the connection to the behaviour.
-                    self.banned_peer_connections.insert(id);
                    self.pool.disconnect(peer_id);


Given that we drop connection here, is the call to pool.disconnect still needed? There should be no connection to the peer in the pool. The previous Swarm::ban_peer_id should have already called pool.disconnect.

Also, if I am not mistaken, we would previously gracefully close connections to banned peers via the pool.disconnect call. Given that connection is now just dropped, this is no longer the case, right?

Given that we drop connection here, is the call to pool.disconnect still needed? There should be no connection to the peer in the pool. The previous Swarm::ban_peer_id should have already called pool.disconnect.

Originally I thought that we might have other connections to this peer but I guess that isn't actually possible because we would have disconnected those at the time of banning the peer.

Also, if I am not mistaken, we would previously gracefully close connections to banned peers via the pool.disconnect call. Given that connection is now just dropped, this is no longer the case, right?

True. I didn't bother much because this code will go away anyway once we implement it via the new NetworkBehaviour callbacks but yeah, it is a change in behaviour so we should probably keep this behaviour.

This made me think though, we should probably gracefully close connections in #3254 that have been rejected by a NetworkBehaviour.

I've pushed some more commits, let me know what you think!

Changes look good to me, thanks!

mxinden · 2023-02-13T16:24:40Z

@thomaseizinger ready to merge from my end. Please trigger mergify once you are ready. Looking forward for this to land.

libp2p#3328) We create the `ConnectionId` for the new connection as part of `DialOpts`. This allows `NetworkBehaviour`s to accurately track state regarding their own dial attempts. This patch is the main enabler of libp2p#3254. Removing the `handler` field will allow us to deprecate the `NetworkBehaviour::new_handler` function in favor of four new ones that give more control over the connection lifecycle.

thomaseizinger added 6 commits January 11, 2023 16:13

Use single quotes to avoid running commands

3a84c14

Use intermediary variable

458aa53

Make connection IDs globally unique

d0d8b77

Deprecate old new function

11e5f74

Add changelog entry

47cf01f

Merge branch 'master' into no-run-title-as-command

341d096

thomaseizinger requested review from jxs, mxinden and elenaf9 January 16, 2023 03:25

thomaseizinger mentioned this pull request Jan 16, 2023

feat(swarm)!: allow NetworkBehaviours to manage connections #3254

Merged

4 tasks

thomaseizinger added 4 commits January 16, 2023 14:40

Remove handler field from NetworkBehaviourAction::Dial

f77e443

Port dcutr protocol

f6cf751

Port relay protocol

08c5160

Add changelog entry to swarm

8e0d7d8

thomaseizinger force-pushed the 2824-no-handler-in-dial branch from e9ed7d6 to 8e0d7d8 Compare January 16, 2023 03:40

thomaseizinger added 2 commits January 16, 2023 21:43

Merge branch 'no-run-title-as-command' into 2824-unique-connection-ids

b337ec1

Merge branch '2824-unique-connection-ids' into 2824-no-handler-in-dial

3c4ab32

mxinden reviewed Jan 16, 2023

View reviewed changes

swarm/src/behaviour.rs Outdated Show resolved Hide resolved

thomaseizinger mentioned this pull request Jan 16, 2023

feat(core)!: make ConnectionIds globally unique #3327

Merged

4 tasks

thomaseizinger marked this pull request as draft January 17, 2023 03:25

thomaseizinger added 5 commits January 17, 2023 15:05

Remove ordering functionality from ConnectionId

1beec8c

Merge branch '2824-unique-connection-ids' of github.com:libp2p/rust-l…

eb48acb

…ibp2p into 2824-unique-connection-ids

Remove unnecessary visibility qualifiers

de9f152

Convert DialOpts to struct variant

7692c77

Eagerly set defaults in DialOpts

b492236

Instead of carrying around an enum and parsing its contents later, directly set the correct values as we construct the final `DialOpts`.

thomaseizinger mentioned this pull request Jan 17, 2023

refactor(swarm): simplify DialOpts #3335

Merged

4 tasks

thomaseizinger added 3 commits January 17, 2023 15:30

Merge branch '2824-refactor-dial-opts' into 2824-no-handler-in-dial

92b2985

Make ConnectionId part of DialOpts

951a0dc

Inline _dial

41e49a5

thomaseizinger added 4 commits January 27, 2023 10:42

Use type alias

55baab1

Remove unnecessary indentation and "quote"-ing

2c84f28

Fix bad changelog entry

cf77246

Spawn connection at right point in time

92a9625

Otherwise the num-established counter is wrong.

thomaseizinger commented Jan 27, 2023

View reviewed changes

swarm/src/lib.rs Show resolved Hide resolved

thomaseizinger added 4 commits January 27, 2023 12:12

Fix rustdoc

d90ac81

One more missing rustdoc link

cfe9f95

Remove banned_peer_connections

3f8f73c

We no longer add connections for banned peers to the pool so we don't have to filter them later.

Fix test

063ef5c

Now that we no longer add connections to banned peers, we need to: - Reduce the number of expected connections by 1 (i.e. delete line 1954) - Act in `Stage::BannedDial` earlier (i.e. remove the `if`)

Merge branch 'master' into 2824-no-handler-in-dial

26f6cf0

Merge branch 'master' into 2824-no-handler-in-dial

c3ba2e3

jxs approved these changes Feb 3, 2023

View reviewed changes

mxinden approved these changes Feb 9, 2023

View reviewed changes

core/src/muxing/boxed.rs Show resolved Hide resolved

swarm/src/lib.rs Show resolved Hide resolved

mxinden reviewed Feb 9, 2023

View reviewed changes

thomaseizinger added 5 commits February 10, 2023 19:16

Inline spawn function

3ecd623

Box future inside spawn

02cbf42

Merge impl blocks

1c906db

Gracefully close connection to banned peer

87cf5fd

Ignore error when closing connection

f204859

mxinden approved these changes Feb 13, 2023

View reviewed changes

thomaseizinger added the send-it label Feb 14, 2023

Merge branch 'master' into 2824-no-handler-in-dial

21f0c46

mergify bot merged commit caed1fe into master Feb 14, 2023

mergify bot deleted the 2824-no-handler-in-dial branch February 14, 2023 01:09

bajtos mentioned this pull request Mar 16, 2023

feat: upgrade libp2p to 0.51.1 filecoin-station/zinnia#111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(swarm)!: remove `handler` from `NetworkBehaviourAction::Dial` #3328

refactor(swarm)!: remove `handler` from `NetworkBehaviourAction::Dial` #3328

thomaseizinger commented Jan 16, 2023 •

edited

Loading

thomaseizinger commented Jan 17, 2023

mergify bot commented Jan 27, 2023

thomaseizinger commented Jan 31, 2023

mergify bot commented Feb 1, 2023

jxs left a comment

jxs Feb 2, 2023

thomaseizinger Feb 3, 2023

mxinden left a comment

mxinden left a comment

mxinden Feb 9, 2023

thomaseizinger Feb 9, 2023

thomaseizinger Feb 10, 2023

mxinden Feb 13, 2023

mxinden commented Feb 13, 2023

refactor(swarm)!: remove handler from NetworkBehaviourAction::Dial #3328

refactor(swarm)!: remove handler from NetworkBehaviourAction::Dial #3328

Conversation

thomaseizinger commented Jan 16, 2023 • edited Loading

Description

Notes

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger commented Jan 17, 2023

mergify bot commented Jan 27, 2023

thomaseizinger commented Jan 31, 2023

mergify bot commented Feb 1, 2023

jxs left a comment

Choose a reason for hiding this comment

jxs Feb 2, 2023

Choose a reason for hiding this comment

thomaseizinger Feb 3, 2023

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

mxinden Feb 9, 2023

Choose a reason for hiding this comment

thomaseizinger Feb 9, 2023

Choose a reason for hiding this comment

thomaseizinger Feb 10, 2023

Choose a reason for hiding this comment

mxinden Feb 13, 2023

Choose a reason for hiding this comment

mxinden commented Feb 13, 2023

refactor(swarm)!: remove `handler` from `NetworkBehaviourAction::Dial` #3328

refactor(swarm)!: remove `handler` from `NetworkBehaviourAction::Dial` #3328

thomaseizinger commented Jan 16, 2023 •

edited

Loading