swarm/: Patch reporting on banned peer connections #2350

divagant-martian · 2021-11-18T20:05:43Z

Track banned connections in the swarm.
Prevent notifications to the behaviour for banned connections.
Include contract assertions to the CallTraceBehaviour.
Rework ban test to check for verious conditions.
inject_connected and inject_closed are called for the first and last allowed connections, respectively.

divagant-martian · 2021-11-18T23:53:32Z

I added an extremely ugly test to check that inject_connection_closed is not called for a banned connection. The test fails on master, succeeds here. I'm not sure if the test adds value, so please let me know if you'd like to maintain it to try to make it more up to your (and mine) standards

mxinden

Thanks @divagant-martian for the patch!

I would like this change to be contained within libp2p-swarm, i.e. the libp2p-swarm concept of banning a peer should not leak into libp2p-core (here via is_allowed).

Instead of the current is_allowed, could one keep track of the banned connections in Swarm instead? E.g. something along the lines of:

diff --git a/swarm/src/lib.rs b/swarm/src/lib.rs
index 0059a21a..0d422561 100644
--- a/swarm/src/lib.rs
+++ b/swarm/src/lib.rs
@@ -275,6 +275,10 @@ where
     /// List of nodes for which we deny any incoming connection.
     banned_peers: HashSet<PeerId>,
 
+    /// List of banned connections and whether they have been reported as open to the
+    /// [`NetworkBehaviour`].
+    banned_connections: HashMap<ConnectionId, bool>,
+
     /// Pending event to be delivered to connection handlers
     /// (or dropped if the peer disconnected) before the `behaviour`
     /// can be polled again.

Combining the above with @MarcoPolo laws:

If a peer is banned, all active connections are still in the "unbanned state", but the connections are made to disconnect.

Active connections to now banned peer are added to Swarm::banned_connections with true.
If a peer is banned, all new connections will be in the "banned state" and thus not emit any events to the network behavior.

New connections to banned peer are added to Swarm::banned_connections with false.
If a peer is unbanned (and previously banned), all active connections are still in the "banned state".

Active connections stay in Swarm::banned_connections and are only removed from Swarm::banned_connections on closing. On closing check the bool in Swarm::banned_connections to tell whether the closing should be reported to the NetworkBehaviour.
If a peer is unbanned, all new connections will be in the "unbanned state".

New connections for unbanned peer are not added to Swarm::banned_connections.

divagant-martian · 2021-11-19T12:06:03Z

Hey,

I would like this change to be contained within libp2p-swarm, i.e. the libp2p-swarm concept of banning a peer should not leak into libp2p-core (here via is_allowed).

Alright, makes sense. As a library user I still see it all as a single unit but if the ban is contained in the swarms, sounds like the best option.

Instead of the current is_allowed, could one keep track of the banned connections in Swarm instead? E.g. something along the lines of:

diff --git a/swarm/src/lib.rs b/swarm/src/lib.rs
index 0059a21a..0d422561 100644
--- a/swarm/src/lib.rs
+++ b/swarm/src/lib.rs
@@ -275,6 +275,10 @@ where
     /// List of nodes for which we deny any incoming connection.
     banned_peers: HashSet<PeerId>,
 
+    /// List of banned connections and whether they have been reported as open to the
+    /// [`NetworkBehaviour`].
+    banned_connections: HashMap<ConnectionId, bool>,
+
     /// Pending event to be delivered to connection handlers
     /// (or dropped if the peer disconnected) before the `behaviour`
     /// can be polled again.

I think a HashSet would be enough, where !conn.is_allowed() is equivalent to banned_connections.contains(&conn.id()).

divagant-martian · 2021-11-19T18:37:55Z

@mxinden Moved the logic to the swarm. I also removed the ugly test and reworked the existing ban test (see updated PR description) The test however, does not finish every now and then. I'd appreciate if you or @MarcoPolo can give it a look

mxinden

Just one comment. Still need to give this an in-depth review but looking good overall. Thanks for the work @divagant-martian!

swarm/src/lib.rs

divagant-martian · 2021-11-20T23:12:50Z

Not sure why I can't re-request a review from @MarcoPolo. But please both give this a review. I think it's ready

mxinden

Thanks @divagant-martian. This is great work.

I have a bunch of wording suggestions, but overall I think this is the way to go.

core/src/connection/pool.rs

core/src/network/event.rs

swarm/src/lib.rs

swarm/src/test.rs

divagant-martian · 2021-11-22T13:00:56Z

@mxinden all suggestions taken and applied. I think it's a good advice to keep the language consistent across the code 👍

mxinden · 2021-11-22T14:00:32Z

Unfortunately this pull request does introduce a breaking change to libp2p-core, thus we will need to release a new minor version of libp2p-core and all of its dependents.

I prepared the corresponding cargo and changelog entries (small hacky Python script). Could you cherry-pick 10f7c18 into this pull request?

Also could you add a changelog entry to libp2p-swarm describing the bug fix?

Final ask: Can you run this on one of your lighthouse nodes for a bit to make sure all panics are fixed?

mxinden

Mind including this diff as well?

diff --git a/swarm/src/lib.rs b/swarm/src/lib.rs
index 0059a21a..2b2262eb 100644
--- a/swarm/src/lib.rs
+++ b/swarm/src/lib.rs
@@ -540,6 +540,10 @@ where
     pub fn ban_peer_id(&mut self, peer_id: PeerId) {
         if self.banned_peers.insert(peer_id) {
             if let Some(peer) = self.network.peer(peer_id).into_connected() {
+                // Note that established connections to the now banned peer are closed but not added
+                // to [`Swarm::banned_peer_connections`]. They have been previously reported as open
+                // to the behaviour and need be reported as closed once closing the connection
+                // finished.
                 peer.disconnect();
             }
         }

core/src/connection/pool.rs

…eer-connections

divagant-martian · 2021-11-22T14:15:16Z

all done. We'll report in a few days with our findings

MarcoPolo · 2021-11-24T21:37:24Z

swarm/src/lib.rs

-                    this.behaviour.inject_event(peer, connection, event);
+                    let conn_id = connection.id();
+                    if this.banned_peer_connections.contains(&conn_id)
+                        || this.banned_peers.contains(&peer)


Why do we do the || this.banned_peers.contains(&peer) check?

My guess: So that if a peer banned we don't inject_event even on connections from before the ban. This is done because the disconnect of existing connections takes a bit and we don't want to get events from banned peers.

That's the reason. That, and every possible condition of peers being banned, unbanned, etc and connections not closing on time

At the end, it's also the safer bet

Sorry in advance for reviving this discussion once more, especially since you already tested this version on your infra @divagant-martian.

Imagine the scenario below:

Local peer is connected to peer A via a single connection C1.

Peer A sends two events to the local peer via C1. The second event depends on the first event. The events are about to be delivered to the local Swarm from the local Network.

User banns peer A.

User polls the Swarm.

The Swarm receives the first event from Network sent via C1.

Swarm drops it due to this.banned_peers.contains(&peer).

User unbanns A.

User polls the Swarm.

The Swarm receives the second event from Network sent via C1.

The Swarm does not drop the second event as this.banned_peers.contains(&peer) is no longer true.

Swarm delivers the second event to the NetworkBehaviour.

Result: NetworkBehaviour receives the second but not the first event from peer A.

I don't think the behavior above is intuitive for an implementor of NetworkBehaviour. In other words, as an implementor of NetworkBehaviour I would expect to either receive all events from a single remote peer in their correct order or not at all.

With the above in mind, would it not be better to remove || this.banned_peers.contains(&peer) @divagant-martian and @MarcoPolo? A NetworkBehaviour would potentially receive events from banned peers until already existing connections are closed, but a NetworkBehaviour would never receive events out of order.

addressed in b43a169

divagant-martian · 2021-11-25T15:52:42Z

Based on runs, I think this looks fine @mxinden
The last CI run failed but it looks like a random infra failure

EDIT: waiting one more day due to changes addressing this comment

divagant-martian · 2021-11-26T15:16:19Z

all looking bright on our side

mxinden

🙏 thanks!

Don't report events of a connection to the `NetworkBehaviour`, if connection has been established while the remote peer was banned. Among other guarantees this upholds that `NetworkBehaviour::inject_event` is never called without a previous `NetworkBehaviour::inject_connection_established` for said connection. Co-authored-by: Max Inden <[email protected]>

patch reporting on banned peer connections

2e73f1d

divagant-martian marked this pull request as ready for review November 18, 2021 23:47

unelegant test

ba4df01

divagant-martian force-pushed the banned-peer-connections branch from 6007c98 to ba4df01 Compare November 18, 2021 23:51

mxinden reviewed Nov 19, 2021

View reviewed changes

divagant-martian added 4 commits November 19, 2021 07:30

move banned connections logic to the swarm

43edaa0

Fix existing ban test.

7aebe91

checkpoint

f8ab8e0

remove log init

eefc25d

mxinden reviewed Nov 19, 2021

View reviewed changes

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

divagant-martian added 4 commits November 20, 2021 17:06

Deal with edge case and fix test

2643b6c

code improvements

c92dff2

remove race condition from test

fd0dd48

address some clippy lints

bdf0ff1

divagant-martian requested a review from mxinden November 20, 2021 23:11

divagant-martian added 2 commits November 20, 2021 18:22

remove debug code

3ccad98

self review updates

5e7750f

mxinden reviewed Nov 22, 2021

View reviewed changes

divagant-martian added 2 commits November 22, 2021 07:45

review suggestions for clarity

48a993b

last review suggestion

8997997

divagant-martian requested a review from mxinden November 22, 2021 12:49

*: Bump libp2p-core to v0.31.0

10f7c18

mxinden reviewed Nov 22, 2021

View reviewed changes

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

remove established_ids everywhere

8b2aa9b

divagant-martian added 3 commits November 22, 2021 09:09

add explanatory note to Swarm::ban_peer_id

e80cfff

Merge commit '10f7c188813da20147e6c84f114352f7170b8e19' into banned-p…

f824064

…eer-connections

add swarm's Changelog entry

891a88b

divagant-martian requested a review from mxinden November 22, 2021 14:17

mxinden and others added 4 commits November 24, 2021 16:48

CHANGELOG: Move v0.42.0 section down replacing v0.41.1

7c317c0

Merge branch 'libp2p/master' into banned-peer-connections

83b1743

fix merge conflicts

18888d9

DeMorgan is hard

1c1ab66

MarcoPolo reviewed Nov 24, 2021

View reviewed changes

address yet another lovely race condition

b43a169

Merge branch 'master' into banned-peer-connections

66c4c1d

mxinden approved these changes Nov 26, 2021

View reviewed changes

mxinden changed the title ~~patch reporting on banned peer connections~~ swarm/: Patch reporting on banned peer connections Nov 26, 2021

mxinden merged commit fd41751 into libp2p:master Nov 26, 2021

mxinden mentioned this pull request Dec 19, 2021

libp2p connections can stick around in zombi mode #2388

Closed

AgeManning deleted the banned-peer-connections branch February 15, 2022 05:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swarm/: Patch reporting on banned peer connections #2350

swarm/: Patch reporting on banned peer connections #2350

divagant-martian commented Nov 18, 2021 •

edited

Loading

divagant-martian commented Nov 18, 2021

mxinden left a comment

divagant-martian commented Nov 19, 2021 •

edited

Loading

divagant-martian commented Nov 19, 2021

mxinden left a comment

divagant-martian commented Nov 20, 2021

mxinden left a comment

divagant-martian commented Nov 22, 2021

mxinden commented Nov 22, 2021

mxinden left a comment

divagant-martian commented Nov 22, 2021

MarcoPolo Nov 24, 2021

divagant-martian Nov 24, 2021

divagant-martian Nov 24, 2021

mxinden Nov 25, 2021 •

edited

Loading

divagant-martian Nov 25, 2021

divagant-martian commented Nov 25, 2021 •

edited

Loading

divagant-martian commented Nov 26, 2021

mxinden left a comment

swarm/: Patch reporting on banned peer connections #2350

swarm/: Patch reporting on banned peer connections #2350

Conversation

divagant-martian commented Nov 18, 2021 • edited Loading

divagant-martian commented Nov 18, 2021

mxinden left a comment

Choose a reason for hiding this comment

divagant-martian commented Nov 19, 2021 • edited Loading

divagant-martian commented Nov 19, 2021

mxinden left a comment

Choose a reason for hiding this comment

divagant-martian commented Nov 20, 2021

mxinden left a comment

Choose a reason for hiding this comment

divagant-martian commented Nov 22, 2021

mxinden commented Nov 22, 2021

mxinden left a comment

Choose a reason for hiding this comment

divagant-martian commented Nov 22, 2021

MarcoPolo Nov 24, 2021

Choose a reason for hiding this comment

divagant-martian Nov 24, 2021

Choose a reason for hiding this comment

divagant-martian Nov 24, 2021

Choose a reason for hiding this comment

mxinden Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

divagant-martian Nov 25, 2021

Choose a reason for hiding this comment

divagant-martian commented Nov 25, 2021 • edited Loading

divagant-martian commented Nov 26, 2021

mxinden left a comment

Choose a reason for hiding this comment

divagant-martian commented Nov 18, 2021 •

edited

Loading

divagant-martian commented Nov 19, 2021 •

edited

Loading

mxinden Nov 25, 2021 •

edited

Loading

divagant-martian commented Nov 25, 2021 •

edited

Loading