feat(core)!: make `ConnectionId`s globally unique #3327

thomaseizinger · 2023-01-16T01:34:47Z

Description

Instead of offering a public constructor, users are now no longer able to construct ConnectionIds at all. They only public API exposed are the derived traits. Internally, ConnectionIds are monotonically incremented using a static atomic counter, thus no two connections will ever get assigned the same ID.

Notes

Extracted out of #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

core/src/connection.rs

thomaseizinger · 2023-01-17T03:25:33Z

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

…ibp2p into 2824-unique-connection-ids

thomaseizinger · 2023-01-17T04:47:44Z

This is ready for review despite being draft. Only #3318 needs to merge first.

mergify · 2023-01-18T08:58:27Z

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

…ibp2p into 2824-unique-connection-ids

thomaseizinger · 2023-01-18T10:44:20Z

swarm/src/connection.rs

+    /// A "dummy" [`ConnectionId`].
    ///
-    /// This is primarily useful for creating connection IDs
-    /// in test environments. There is in general no guarantee
-    /// that all connection IDs are based on non-negative integers.
-    pub fn new(id: usize) -> Self {
-        Self(id)
-    }
-}
-
-impl std::ops::Add<usize> for ConnectionId {
-    type Output = Self;
-
-    fn add(self, other: usize) -> Self {
-        Self(self.0 + other)
+    /// Really, you should not use this, not even for testing but it is here if you need it.
+    #[deprecated(
+        since = "0.42.0",
+        note = "Don't use this, it will be removed at a later stage again."
+    )]
+    pub const DUMMY: ConnectionId = ConnectionId(0);


I am not very happy about this but I propose to accept this technical debt to be able to move forward with the connection management PR.

Long-term, we would need to re-write the gossipsub and kademlia tests to not manually call lifecycle functions and thus only have Swarm construct ConnectionIds.

@libp2p/rust-libp2p-maintainers Please voice your opinion on this.

Yeah it would be nice if this was possible. Other than that I don't see a better alternative for here.

thomaseizinger · 2023-01-18T10:53:13Z

swarm/src/connection.rs

 use std::task::Waker;
 use std::time::Duration;
 use std::{fmt, io, mem, pin::Pin, task::Context, task::Poll};

+static NEXT_CONNECTION_ID: AtomicUsize = AtomicUsize::new(1);


We start IDs at 1 so they can never clash with the DUMMY one below.

jxs

LGTM

jxs · 2023-01-18T13:37:30Z

swarm/src/connection.rs

+    /// A "dummy" [`ConnectionId`].
    ///
-    /// This is primarily useful for creating connection IDs
-    /// in test environments. There is in general no guarantee
-    /// that all connection IDs are based on non-negative integers.
-    pub fn new(id: usize) -> Self {
-        Self(id)
-    }
-}
-
-impl std::ops::Add<usize> for ConnectionId {
-    type Output = Self;
-
-    fn add(self, other: usize) -> Self {
-        Self(self.0 + other)
+    /// Really, you should not use this, not even for testing but it is here if you need it.
+    #[deprecated(
+        since = "0.42.0",
+        note = "Don't use this, it will be removed at a later stage again."
+    )]
+    pub const DUMMY: ConnectionId = ConnectionId(0);


Yeah it would be nice if this was possible. Other than that I don't see a better alternative for here.

jxs · 2023-01-18T13:37:50Z

swarm/src/connection.rs

+
+    /// Returns the next available [`ConnectionId`].
+    pub(crate) fn next() -> Self {
+        Self(NEXT_CONNECTION_ID.fetch_add(1, Ordering::SeqCst))


this is very elegant.

Credits go to @rkuhn !

mergify · 2023-01-19T22:52:13Z

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

thomaseizinger · 2023-01-20T00:43:12Z

Thanks for the review @jxs! @mxinden I'd like you to have a look at this too before I merge it.

Note that this is only really valid once we have libp2p#3327 and the corresponding patch in `libp2p-swarm` `DialOpts`.

mxinden

I am fine with the technical dept. I suggest moving forward here.

mxinden · 2023-01-23T12:48:12Z

rust-libp2p-head x rust-libp2p-head is failing. Error seems to be:

   rust-libp2p-headxrust-libp2p-headwebrtc-dialer-1    | [2023-01-23T12:07:47Z WARN  webrtc::peer_connection::peer_connection_internal] failed to open data channel: there already exists a stream with identifier
  rust-libp2p-headxrust-libp2p-headwebrtc-listener-1  | Error: Failed to wait for dialer conclusion

thomaseizinger · 2023-01-23T12:58:07Z

rust-libp2p-head x rust-libp2p-head is failing. Error seems to be:

   rust-libp2p-headxrust-libp2p-headwebrtc-dialer-1    | [2023-01-23T12:07:47Z WARN  webrtc::peer_connection::peer_connection_internal] failed to open data channel: there already exists a stream with identifier
  rust-libp2p-headxrust-libp2p-headwebrtc-listener-1  | Error: Failed to wait for dialer conclusion

Isn't that the flake that came up a couple of times now?

jxs · 2023-01-23T13:19:54Z

rust-libp2p-head x rust-libp2p-head is failing. Error seems to be:

   rust-libp2p-headxrust-libp2p-headwebrtc-dialer-1    | [2023-01-23T12:07:47Z WARN  webrtc::peer_connection::peer_connection_internal] failed to open data channel: there already exists a stream with identifier
  rust-libp2p-headxrust-libp2p-headwebrtc-listener-1  | Error: Failed to wait for dialer conclusion

Isn't that the flake that came up a couple of times now?

yup exactly

thomaseizinger · 2023-01-24T01:26:15Z

I am tracking removal of the tech-debt in #3371.

…r` (#3384) The gossipsub tests are calling lifecycle functions of the `NetworkBehaviour` that aren't meant to be called outside of `Swarm`. This already surfaced as a problem in #3327 and it is coming up again in #3254 where `new_handler` gets deprecated. Try to mitigate that by constructing a dummy handler instead. Functionally, there is no difference as in both cases, the given handler has never seen a connection.

…r` (libp2p#3384) The gossipsub tests are calling lifecycle functions of the `NetworkBehaviour` that aren't meant to be called outside of `Swarm`. This already surfaced as a problem in libp2p#3327 and it is coming up again in libp2p#3254 where `new_handler` gets deprecated. Try to mitigate that by constructing a dummy handler instead. Functionally, there is no difference as in both cases, the given handler has never seen a connection.

melekes · 2023-03-15T11:56:13Z

A bit late to the party, but we (@paritytech) rely on being able to construct ConnectionId in our tests - https://github.com/paritytech/substrate/blob/master/client/network/src/protocol/notifications/behaviour.rs#L3391

Would you be willing to change your opinion on making ConnectionId constructor private?

thomaseizinger · 2023-03-15T12:34:12Z

A bit late to the party, but we (@paritytech) rely on being able to construct ConnectionId in our tests - paritytech/substrate@master/client/network/src/protocol/notifications/behaviour.rs#L3391

Would you be willing to change your opinion on making ConnectionId constructor private?

We do too in some tests but my current position is that you shouldn't call those lifecycle functions yourself but instead construct two or more Swarms and establish a connection between them, possible using the new libp2p-swarm-test: https://github.com/libp2p/rust-libp2p/tree/master/swarm-test

Does that work for you?

@mxinden I think we should publish libp2p-swarm-test to crates.io.

melekes · 2023-03-17T07:05:04Z

@mxinden I think we should publish libp2p-swarm-test to crates.io.

please do 🙏

We do too in some tests but my current position is that you shouldn't call those lifecycle functions yourself but instead construct two or more Swarms and establish a connection between them, possible using the new libp2p-swarm-test: https://github.com/libp2p/rust-libp2p/tree/master/swarm-test
Does that work for you?

There are over 2000 lines of tests, but I think it may be possible to rewrite them using Swarms.

melekes · 2023-03-17T08:05:58Z

In some tests we need to inject custom handshake and I'm worried it won't be possible with libp2p-swarm-test API

notif.on_connection_handler_event(
			peer,
			conn1,
			conn_yielder.open_substream(peer, 0, connected.clone(), vec![1, 2, 3, 4]),
		);

Overall this change is quite upsetting because we just want to test our state machine built on top of libp2p so spawning libp2p Swarms under it is overcomplicating the issue

thomaseizinger · 2023-03-17T14:38:10Z

Overall this change is quite upsetting because we just want to test our state machine built on top of libp2p so spawning libp2p Swarms under it is overcomplicating the issue

Thank you for sharing.

We can re-introduce a public constructor for ConnectionId.

However, I think these whitebox tests are not a good idea. For example, recently we introduced callbacks for connection management that also involves the behaviour upon pending connections. Is your home-grown test framework calling these correctly?

Going through Swarm for your tests is going to simulate a real network much better than calling these lifecycle functions yourself.

melekes · 2023-03-21T09:30:26Z

Is your home-grown test framework calling these correctly?

Probably not, which is why I agree that we should use Swarms as much as possible. BUT at the same time

a) we want to ship 0.51.1 libp2p, which is currently blocked on this
b) rewriting 2000 lines of tests will take some time
c) libp2p-swarm-test is not even published.

I'd humbly propose to reintroduce ConnectionId::new and mark it as deprecated (it was not marked as deprecated before?). This will give us some time to rewrite the tests. Thank you 🙏

thomaseizinger · 2023-03-21T10:27:47Z

c) libp2p-swarm-test is not even published.

We are working on that:

I'd humbly propose to reintroduce ConnectionId::new and mark it as deprecated (it was not marked as deprecated before?). This will give us some time to rewrite the tests. Thank you pray

Yep, we can do that. I'll send a PR.

thomaseizinger · 2023-03-21T10:54:00Z

I'd humbly propose to reintroduce ConnectionId::new and mark it as deprecated (it was not marked as deprecated before?). This will give us some time to rewrite the tests. Thank you pray

Yep, we can do that. I'll send a PR.

Actually, with how we have now modeled DialOpts, there isn't any risk in users creating an already used ConnectionId as part of dialing a peer.

In earlier iterations of the design for generic connection management, we removed the `ConnectionId::new` constructor because it would have allowed users to create `ConnectionId`s that are already taken, thus breaking invariants that `NetworkBehaviour`s rely on. Later, we incorporated the creation of `ConnectionId` in `DialOpts` which mitigates this risk altogether. Thus, it is reasonably safe to introduce a public, non-deprecated constructor for `ConnectionId` that can be used for tests. Related #3327 (comment). Pull-Request: #3652.

thomaseizinger added 4 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

thomaseizinger requested review from jxs and mxinden January 16, 2023 01:34

Add changelog entry

47cf01f

thomaseizinger mentioned this pull request Jan 16, 2023

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

Merged

4 tasks

thomaseizinger marked this pull request as ready for review January 16, 2023 01:36

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

341d096

thomaseizinger mentioned this pull request Jan 16, 2023

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

Merged

4 tasks

thomaseizinger changed the base branch from master to no-run-title-as-command January 16, 2023 10:42

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

b337ec1

mxinden reviewed Jan 16, 2023

View reviewed changes

core/src/connection.rs Outdated Show resolved Hide resolved

core/src/connection.rs Outdated Show resolved Hide resolved

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

thomaseizinger added 2 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

thomaseizinger requested a review from mxinden January 17, 2023 04:47

Base automatically changed from no-run-title-as-command to master January 17, 2023 22:13

thomaseizinger marked this pull request as ready for review January 18, 2023 01:13

Merge branch 'master' into 2824-unique-connection-ids

3d91173

thomaseizinger added 2 commits January 18, 2023 19:58

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

aa19d69

…ibp2p into 2824-unique-connection-ids

Merge branch 'master' into 2824-unique-connection-ids

0f53692

thomaseizinger commented Jan 18, 2023

View reviewed changes

jxs approved these changes Jan 18, 2023

View reviewed changes

Merge branch 'master' into 2824-unique-connection-ids

244c171

mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Jan 23, 2023

Add example for Command from NetworkBehaviour to Swarm

694a09c

Note that this is only really valid once we have libp2p#3327 and the corresponding patch in `libp2p-swarm` `DialOpts`.

mxinden approved these changes Jan 23, 2023

View reviewed changes

thomaseizinger added the send-it label Jan 23, 2023

Merge branch 'master' into 2824-unique-connection-ids

b536773

Merge branch 'master' into 2824-unique-connection-ids

64a333d

Merge branch 'master' into 2824-unique-connection-ids

d6b19e4

mergify bot merged commit 778f7a2 into master Jan 23, 2023

mergify bot deleted the 2824-unique-connection-ids branch January 23, 2023 16:29

thomaseizinger mentioned this pull request Jan 24, 2023

gossipsub: Re-write tests to remove technical debt #3371

Open

thomaseizinger mentioned this pull request Jan 25, 2023

refactor(gossipsub): use dummy handler instead of calling new_handler #3384

Merged

4 tasks

thomaseizinger mentioned this pull request Jan 30, 2023

refactor(gossipsub): use dummy handler instead of calling `new_handle… thomaseizinger/rust-libp2p#54

Merged

4 tasks

melekes mentioned this pull request Mar 17, 2023

Upgrade to libp2p 0.51.3 paritytech/substrate#13587

Merged

thomaseizinger mentioned this pull request Mar 21, 2023

feat(swarm): introduce ConnectionId::new_unchecked constructor #3652

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core)!: make `ConnectionId`s globally unique #3327

feat(core)!: make `ConnectionId`s globally unique #3327

thomaseizinger commented Jan 16, 2023 •

edited

Loading

thomaseizinger commented Jan 17, 2023

thomaseizinger commented Jan 17, 2023

mergify bot commented Jan 18, 2023

thomaseizinger Jan 18, 2023

thomaseizinger Jan 18, 2023

jxs Jan 18, 2023

thomaseizinger Jan 18, 2023

jxs left a comment

jxs Jan 18, 2023

jxs Jan 18, 2023

thomaseizinger Jan 18, 2023

mergify bot commented Jan 19, 2023

thomaseizinger commented Jan 20, 2023

mxinden left a comment

mxinden commented Jan 23, 2023

thomaseizinger commented Jan 23, 2023

jxs commented Jan 23, 2023

thomaseizinger commented Jan 24, 2023

melekes commented Mar 15, 2023

thomaseizinger commented Mar 15, 2023

melekes commented Mar 17, 2023 •

edited

Loading

melekes commented Mar 17, 2023 •

edited

Loading

thomaseizinger commented Mar 17, 2023

melekes commented Mar 21, 2023 •

edited

Loading

thomaseizinger commented Mar 21, 2023

thomaseizinger commented Mar 21, 2023

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

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

Conversation

thomaseizinger commented Jan 16, 2023 • edited Loading

Description

Notes

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger commented Jan 17, 2023

thomaseizinger commented Jan 17, 2023

mergify bot commented Jan 18, 2023

thomaseizinger Jan 18, 2023

Choose a reason for hiding this comment

thomaseizinger Jan 18, 2023

Choose a reason for hiding this comment

jxs Jan 18, 2023

Choose a reason for hiding this comment

thomaseizinger Jan 18, 2023

Choose a reason for hiding this comment

jxs left a comment

Choose a reason for hiding this comment

jxs Jan 18, 2023

Choose a reason for hiding this comment

jxs Jan 18, 2023

Choose a reason for hiding this comment

thomaseizinger Jan 18, 2023

Choose a reason for hiding this comment

mergify bot commented Jan 19, 2023

thomaseizinger commented Jan 20, 2023

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Jan 23, 2023

thomaseizinger commented Jan 23, 2023

jxs commented Jan 23, 2023

thomaseizinger commented Jan 24, 2023

melekes commented Mar 15, 2023

thomaseizinger commented Mar 15, 2023

melekes commented Mar 17, 2023 • edited Loading

melekes commented Mar 17, 2023 • edited Loading

thomaseizinger commented Mar 17, 2023

melekes commented Mar 21, 2023 • edited Loading

thomaseizinger commented Mar 21, 2023

thomaseizinger commented Mar 21, 2023

feat(core)!: make `ConnectionId`s globally unique #3327

feat(core)!: make `ConnectionId`s globally unique #3327

thomaseizinger commented Jan 16, 2023 •

edited

Loading

melekes commented Mar 17, 2023 •

edited

Loading

melekes commented Mar 17, 2023 •

edited

Loading

melekes commented Mar 21, 2023 •

edited

Loading