[Gossipsub] Inconsistency in mesh peer tracking #2175
Conversation
| @@ -2913,11 +2921,6 @@ where | |||
| connection_id: &ConnectionId, | |||
| endpoint: &ConnectedPoint, | |||
| ) { | |||
| // Ignore connections from blacklisted peers. | |||
There was a problem hiding this comment.
Can you expand on why we no longer ignore blacklisted peers?
There was a problem hiding this comment.
I found that in many cases we are under the assumption that any connected peer exists in connected_peers and their topic subscriptions exist in peer_topics. When peers disconnect we use these mappings to correctly remove them from the mesh and other mappings in the behaviour. If there is any inconsistencies, gossipsub panics (this PR fixes on such instance).
Although I haven't seen a panic from blacklisted peers (I'm not using them atm), i'm concerned about inconsistencies coming from states where users at arbitrary times blacklist and unblacklist (via the public functions) peers.
This change represents a safer approach where we register the connections of all peers, add them to the mappings (so they exist regardless of when the user ad-hoc add/removes them) but we still block all messages for whatever peers are blacklisted at the time we receive them. On disconnects, as all peers connections are registered they are removed from all mappings correctly, regardless of the current state of blacklisting.
There probably is a way we could do this without registering the connection, but I think there a quite a few edge cases and this makes it simpler to reason about.
There was a problem hiding this comment.
Thanks @AgeManning for the details!
|
@AgeManning I don't have permissions to merge latest |
|
Thanks @mxinden. In the future, I'll make these PRs fully accessible to you. |
|
We're on a bit of a tight timeline over at sigp/lighthouse with a hard-fork happening on a public testnet next week. This is the last blocker before we can get a release candidate out. I appreciate that the maintainers have their own priorities, I just wanted to make that known 🙂 As always, thanks for your efforts and I appreciate that reviews can take time 🙏 |
@paulhauner do I understand correctly that you only need this patch to be merged into Patch will be merged through #2189 as I can't merge current |
|
Merged via #2189, thus closing here. |
|
Thank you, much appreciated!
We're fine with targeting |
## Issue Addressed - Resolves #2457 - Resolves #2443 ## Proposed Changes Target the (presently unreleased) head of `libp2p/rust-libp2p:master` in order to obtain the fix from libp2p/rust-libp2p#2175. Additionally: - `libsecp256k1` needed to be upgraded to satisfy the new version of `libp2p`. - There were also a handful of minor changes to `eth2_libp2p` to suit some interface changes. - Two `cargo audit --ignore` flags were remove due to libp2p upgrades. ## Additional Info NA
## Issue Addressed - Resolves sigp#2457 - Resolves sigp#2443 ## Proposed Changes Target the (presently unreleased) head of `libp2p/rust-libp2p:master` in order to obtain the fix from libp2p/rust-libp2p#2175. Additionally: - `libsecp256k1` needed to be upgraded to satisfy the new version of `libp2p`. - There were also a handful of minor changes to `eth2_libp2p` to suit some interface changes. - Two `cargo audit --ignore` flags were remove due to libp2p upgrades. ## Additional Info NA
Peers that GRAFT to us without sending subscriptions for the grafted topic can be added to the mesh. These peers once disconnected, do then not get removed from the mesh. This creates a variety of inconsistencies.
This PR resolves this by adding the topics that a peer send us via grafts, to the
peer_topicsmapping, indicating that they are in fact subscribed to that topic. There should never be a case that a peer grafts us, without locally being subscribed to the topic.In a similar vein, this improves the consistency of blacklisted peers that the user can add/remove at arbitrary times.