Skip to content

Commit

Permalink
feat(swarm): introduce ConnectionId::new_unchecked constructor
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
thomaseizinger committed Mar 21, 2023
1 parent ab9555c commit 3fa10be
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 26 deletions.
39 changes: 15 additions & 24 deletions protocols/gossipsub/src/behaviour/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,17 +222,19 @@ where
}
};

#[allow(deprecated)]
gs.on_swarm_event(FromSwarm::ConnectionEstablished(ConnectionEstablished {
peer_id: peer,
connection_id: ConnectionId::DUMMY,
connection_id: ConnectionId::new_unchecked(0),
endpoint: &endpoint,
failed_addresses: &[],
other_established: 0, // first connection
}));
if let Some(kind) = kind {
#[allow(deprecated)]
gs.on_connection_handler_event(peer, ConnectionId::DUMMY, HandlerEvent::PeerKind(kind));
gs.on_connection_handler_event(
peer,
ConnectionId::new_unchecked(0),
HandlerEvent::PeerKind(kind),
);
}
if explicit {
gs.add_explicit_peer(&peer);
Expand Down Expand Up @@ -582,10 +584,9 @@ fn test_join() {
for _ in 0..3 {
let random_peer = PeerId::random();
// inform the behaviour of a new peer
#[allow(deprecated)]
gs.on_swarm_event(FromSwarm::ConnectionEstablished(ConnectionEstablished {
peer_id: random_peer,
connection_id: ConnectionId::DUMMY,
connection_id: ConnectionId::new_unchecked(0),
endpoint: &ConnectedPoint::Dialer {
address: "/ip4/127.0.0.1".parse::<Multiaddr>().unwrap(),
role_override: Endpoint::Dialer,
Expand Down Expand Up @@ -965,10 +966,7 @@ fn test_get_random_peers() {
*p,
PeerConnections {
kind: PeerKind::Gossipsubv1_1,
connections: vec![
#[allow(deprecated)]
ConnectionId::DUMMY,
],
connections: vec![ConnectionId::new_unchecked(0)],
},
)
})
Expand Down Expand Up @@ -2998,8 +2996,7 @@ fn test_ignore_rpc_from_peers_below_graylist_threshold() {
//receive from p1
gs.on_connection_handler_event(
p1,
#[allow(deprecated)]
ConnectionId::DUMMY,
ConnectionId::new_unchecked(0),
HandlerEvent::Message {
rpc: Rpc {
messages: vec![raw_message1],
Expand All @@ -3025,8 +3022,7 @@ fn test_ignore_rpc_from_peers_below_graylist_threshold() {
//receive from p2
gs.on_connection_handler_event(
p2,
#[allow(deprecated)]
ConnectionId::DUMMY,
ConnectionId::new_unchecked(0),
HandlerEvent::Message {
rpc: Rpc {
messages: vec![raw_message3],
Expand Down Expand Up @@ -3632,8 +3628,7 @@ fn test_scoring_p4_invalid_signature() {

gs.on_connection_handler_event(
peers[0],
#[allow(deprecated)]
ConnectionId::DUMMY,
ConnectionId::new_unchecked(0),
HandlerEvent::Message {
rpc: Rpc {
messages: vec![],
Expand Down Expand Up @@ -4114,11 +4109,10 @@ fn test_scoring_p6() {
}

//add additional connection for 3 others with addr
#[allow(deprecated)]
for id in others.iter().take(3) {
gs.on_swarm_event(FromSwarm::ConnectionEstablished(ConnectionEstablished {
peer_id: *id,
connection_id: ConnectionId::DUMMY,
connection_id: ConnectionId::new_unchecked(0),
endpoint: &ConnectedPoint::Dialer {
address: addr.clone(),
role_override: Endpoint::Dialer,
Expand All @@ -4137,10 +4131,9 @@ fn test_scoring_p6() {

//add additional connection for 3 of the peers to addr2
for peer in peers.iter().take(3) {
#[allow(deprecated)]
gs.on_swarm_event(FromSwarm::ConnectionEstablished(ConnectionEstablished {
peer_id: *peer,
connection_id: ConnectionId::DUMMY,
connection_id: ConnectionId::new_unchecked(0),
endpoint: &ConnectedPoint::Dialer {
address: addr2.clone(),
role_override: Endpoint::Dialer,
Expand Down Expand Up @@ -4168,10 +4161,9 @@ fn test_scoring_p6() {
);

//two times same ip doesn't count twice
#[allow(deprecated)]
gs.on_swarm_event(FromSwarm::ConnectionEstablished(ConnectionEstablished {
peer_id: peers[0],
connection_id: ConnectionId::DUMMY,
connection_id: ConnectionId::new_unchecked(0),
endpoint: &ConnectedPoint::Dialer {
address: addr,
role_override: Endpoint::Dialer,
Expand Down Expand Up @@ -5179,8 +5171,7 @@ fn test_subscribe_and_graft_with_negative_score() {

let (mut gs2, _, _) = inject_nodes1().create_network();

#[allow(deprecated)]
let connection_id = ConnectionId::DUMMY;
let connection_id = ConnectionId::new_unchecked(0);

let topic = Topic::new("test");

Expand Down
3 changes: 1 addition & 2 deletions protocols/kad/src/behaviour/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1295,8 +1295,7 @@ fn network_behaviour_on_address_change() {
let local_peer_id = PeerId::random();

let remote_peer_id = PeerId::random();
#[allow(deprecated)]
let connection_id = ConnectionId::DUMMY;
let connection_id = ConnectionId::new_unchecked(0);
let old_address: Multiaddr = Protocol::Memory(1).into();
let new_address: Multiaddr = Protocol::Memory(2).into();

Expand Down
4 changes: 4 additions & 0 deletions swarm/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
- Deprecate `ConnectionLimits` in favor of `libp2p::connection_limits`.
See [PR 3386].

- Introduce `ConnectionId::new_unchecked` to allow for more sophisticated, manual tests of `NetworkBehaviour`.
See [PR 3652].

[PR 3386]: https://github.com/libp2p/rust-libp2p/pull/3386
[PR 3652]: https://github.com/libp2p/rust-libp2p/pull/3652

# 0.42.0

Expand Down
10 changes: 10 additions & 0 deletions swarm/src/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,16 @@ impl ConnectionId {
)]
pub const DUMMY: ConnectionId = ConnectionId(0);

/// Creates an _unchecked_ [`ConnectionId`].
///
/// [`Swarm`](crate::Swarm) enforces that [`ConnectionId`]s are unique and not reused.
/// This constructor does not, hence the _unchecked_.
///
/// It is primarily meant for allowing manual tests of [`NetworkBehaviour`](crate::NetworkBehaviour)s.
pub fn new_unchecked(id: usize) -> Self {
Self(id)
}

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

0 comments on commit 3fa10be

Please sign in to comment.