feat: report changes in supported protocols to `ConnectionHandler` #3651

thomaseizinger · 2023-03-20T19:59:33Z

Description

With this patch, implementations of ConnectionHandler (which are typically composed in a tree) can exchange information about the supported protocols of a remote with each other via ConnectionHandlerEvent::ReportRemoteProtocols. The provided ProtocolSupport enum can describe either additions or removals of the remote peer's protocols.

This information is aggregated in the connection and passed down to the ConnectionHandler via ConnectionEvent::RemoteProtocolsChange.

Similarly, if the listen protocols of a connection change, all ConnectionHandlers on the connection will be notified via ConnectionEvent::LocalProtocolsChange. This will allow us to eventually remove PollParameters from NetworkBehaviour.

This pattern allows protocols on a connection to communicate with each other. For example, protocols like identify can share the list of (supposedly) supported protocols by the remote with all other handlers. A protocol like kademlia can accurately add and remove a remote from its routing table as a result.

Resolves: #2680.
Related: #3124.

Notes & open questions

Should we automatically attempt to parse protocols as String inside swarm::Connection?
Deprecate PollParameters::supported_protocols as part of this
~~Extract 586394b as separate PR once we can make breaking changes~~: feat(swarm): report outcome of handling SwarmEvent #3865
Should we land b329a09 separate as well? The base of this PR would have to merge first.
Kademlia client-mode should be a separate PR on top of the base infrastructure for reporting changes to protocols

Dependencies

Dependents

The following PRs depend on this / are enabled by this:

feat(kad): implement automatic client mode #3877

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

thomaseizinger · 2023-03-22T21:14:13Z

@mxinden made me aware that we also need to learn a remote's protocols for client-mode. Working on that next.

thomaseizinger · 2023-03-22T21:39:39Z

@mxinden made me aware that we also need to learn a remote's protocols for client-mode. Working on that next.

f0170b9 lays the foundation for that. Now we need to check if we can implement kademlia client-mode with this.

thomaseizinger · 2023-03-24T15:56:25Z

@mxinden @MarcoPolo This PR now contains a PoC for Kademlia's client-mode based on the new infrastructure for reporting protocols to the connection. Please take a look :)

I've split it into distinct commits, that should make the review fairly easy. It needs some more polishing before we can merge it though!

protocols/kad/src/handler.rs

mxinden · 2023-03-27T11:34:16Z

Great to see this in place. Thanks @thomaseizinger!

I wonder how sophisticated we should make this new mechanism. A couple of possible extensions:

Full vs partial view

In this pull request a single *ProtocolsChange event always contains the full list of protocols. That is enough for libp2p-identify as one always sends and receives the full list of protocols (full state instead of delta).

What if a single ConnectionHandler only has a partial view? E.g. what if a ConnectionHandler learns that the remote supports protocol X which might be one among many protocols.

How would two partial-view ConnectionHandlers be able to co-exist, without one overriding the other on each report?

In- and outbound protocols

In this pull request the list of protocols is the list of supported inbound protocols.

Do we need to design for outbound protocols as well. E.g. should a ConnectionHandler be able to report that the remote peer supports protocol X for outbound streams (inbound from the local perspective)?

I can not think of a use-case for this.

Successful upgrades as source of input

Should we include the information attained from successful upgrades or even UpgradeInfo. E.g. should we inform a ConnectionHandler that the remote supports protocol X because we just negotiated that protocol on a new stream (potentially for a different sub-ConnectionHandler).

Replacement of `PollParameters::supported_protocols`

How do we replace PollParameters::supported_protocols with this mechanism? Does each ConnectionHandler emit a LocalProtocolsChange event with the set of protocols that it supports on first invocation of ConnectionHandler::poll?

Alternative design

Unless I am missing something, the design below would check all boxes above. Note that it is still an open question whether we would need such a sophisticated mechanism or not.

With *ProtocolsChange ConnectionHandlers report a list of deltas, not the full list. In other words it is additive to the previously announced list of protocols, it does not replace the previous list.
Differentiate *ProtocolsChange in additions and removals. In other words allow ConnectionHandlers to report removal of support for a protocol by the local or remote node. Would require libp2p-identify to keep track of the previously reported protocols of the remote node to report removals.
Aggregate addition and removal events in the Connection to represent the full list of protocols (global view over all sub-ConnectionHandlers). On change of that full list, notify the sub-ConnectionHandlers via Report*Protocols with the full list. Potentially include the deltas to make implementations in ConnectionHandler easier.
Side note, we should keep *ProtocolsChange events idempotent.
Potentially allow reporting support for outbound protocols. (Not sure there is a use-case.)
At start-up, have each ConnectionHandler emit an event that reports the local inbound support for a protocol. Allows us to replace `PollParameters::supported_protocols.
Additionally use the information obtained from UpgradeInfo (local) and successful upgrades (remote) to report local and remote support for protocols.

The thoughts above are not well structured. Feel free to ask for rephrasing / restructuring. Again, I am not sure we need the above level of sophistication.

thomaseizinger · 2023-03-29T12:02:15Z

We should definitely fix #2831 first. cc @efarg Are you still working on that?

Agreed. @thomaseizinger can you tackle this in case @efarg does not have the capacity (no hard feelings)?

Yes. I talked to them and they will push what they have some time this week. I'll take over from there.

thomaseizinger · 2023-04-06T13:54:42Z

We should definitely fix #2831 first. cc @efarg Are you still working on that?

Agreed. @thomaseizinger can you tackle this in case @efarg does not have the capacity (no hard feelings)?

Yes. I talked to them and they will push what they have some time this week. I'll take over from there.

I am working on my own version of #2831 now. It is actually a bit more invasive than I thought but looking nice already. Stay tuned.

thomaseizinger · 2023-04-06T14:03:36Z

How do we replace PollParameters::supported_protocols with this mechanism? Does each ConnectionHandler emit a LocalProtocolsChange event with the set of protocols that it supports on first invocation of ConnectionHandler::poll?

No, I was thinking of calling listen_protocols right at the start of a connection and immediately injecting a LocalProtocolChange event.

Should we include the information attained from successful upgrades or even UpgradeInfo. E.g. should we inform a ConnectionHandler that the remote supports protocol X because we just negotiated that protocol on a new stream (potentially for a different sub-ConnectionHandler).

Hmm, I am not sure this is useful. Upgrades can fail for a variety of ways. Although in general, I'd like this mechanism to be as transparent as possible for the ConnectionHandler and incorporating successful upgrades would make sense if we design the mechanism such that protocols like identify can contribute but don't necessarily have to be active. This would go in the same direction as your other idea of being able to supply partial views.

The question however is, which protocol would benefit from knowing that another protocol successfully upraded / is supported? Why would the relay protocol care that the kademlia is supported by the other node? On the other hand, if we already build a mechanism like this, we might as well enrich it with as much information as we have. I'll keep thinking about it.

swarm/src/handler.rs

swarm/src/connection.rs

mxinden · 2023-05-08T03:36:17Z

protocols/identify/src/behaviour.rs

+        self.listen_addresses
+            .iter()
+            .chain(self.external_addresses.iter())
+            .cloned()
+            .collect()


Just a note for sometime in the future, ideally we would prioritize the addresses we send to a remote, e.g. we should prioritize (in terms of order in the list) external addresses over listening addresses. Again, not for this pull request.

That is an easy enough change!

It actually isn't because we are collecting in a HashSet and thus lose any ordering.

mxinden · 2023-05-08T03:39:07Z

swarm/src/handler.rs

+    ) -> Option<Self> {
+        let mut actually_removed_protocols = existing_protocols.intersection(to_remove).peekable();
+
+        actually_removed_protocols.peek()?;


Are you referring to intersection?

I am referring to the concise early return via peek()?.

thomaseizinger · 2023-05-08T08:49:19Z

Added a changelog entry now, good to go into master again.

mxinden · 2023-05-09T02:04:20Z

🎉 great to see this merged.

Currently, the kademlia behaviour can only learn that the remote node supports kademlia on a particular connection if we successfully negotiate a stream to them. Using the newly introduced abstractions from #3651, we don't have to attempt to establish a stream to the remote to learn whether they support kademlia on a connection but we can directly learn it from the `ConnectionEvent::RemoteProtocolsChange` event. This happens directly once a connection is established which should overall benefit the DHT. Clients do not advertise the kademlia protocol and thus we will immediately learn that a given connection is not suitable for kadmelia requests. We may receive inbound messages from it but this does not affect the routing table. Resolves: #2032. Pull-Request: #3877.

thomaseizinger requested review from jxs and mxinden March 20, 2023 19:59

This was referenced Mar 21, 2023

swarm/: NetworkBehaviour/ConnectionHandler cross-communication #2680

Closed

fix(kad): remove allow_listening handler config #3504

Closed

thomaseizinger mentioned this pull request Mar 23, 2023

swarm: Remove ConnectionHandler::Error #3591

Closed

thomaseizinger added 7 commits March 24, 2023 15:48

Report changes in supported protocols back to ConnectionHandler

f2801fb

Allow reporting of supported protocols by remote

9a28cd2

Consume supported protocols in identify

e536dea

Report a remote's protocols to other handlers

7699a1e

Add test for kademlia client mode

450fc1e

Implement kademlia client-mode

a972b9c

Add tests for two servers connecting

f9cf33f

thomaseizinger force-pushed the 2680-explore branch from f0170b9 to f9cf33f Compare March 24, 2023 15:55

thomaseizinger mentioned this pull request Mar 24, 2023

feat(mdns): change mdns::Event to hold Vec #3621

Merged

4 tasks

mxinden reviewed Mar 27, 2023

View reviewed changes

protocols/kad/src/handler.rs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

thomaseizinger mentioned this pull request Apr 10, 2023

protocols/kad: Support client mode #2032

Closed

thomaseizinger added 2 commits April 13, 2023 19:56

Report additions and removals of protocols instead

deb30f3

Report listen protocols on startup to connection

3c1bf5f

mxinden reviewed Apr 19, 2023

View reviewed changes

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

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

thomaseizinger added 3 commits April 19, 2023 19:40

Introduce SupportedProtocols type

9e70729

Deduplicate code and propagate supported protocols only once

f1328c5

Extend docs

a4fbcda

thomaseizinger added 4 commits May 4, 2023 19:10

Add failing test

a799798

Make test pass, i.e. only report actual changes back to the handler

0260ad1

Extract helpers to make fields crate-private and add docs

bf99654

Return SmallVec from from_full_sets which allows iteration

46f4e96

thomaseizinger requested a review from mxinden May 4, 2023 17:53

thomaseizinger marked this pull request as ready for review May 4, 2023 17:53

thomaseizinger added 2 commits May 4, 2023 19:57

Fix clippy warnings

ae7fc93

Fix rustdoc

fe9a6e3

thomaseizinger mentioned this pull request May 4, 2023

swarm: Remove PollParameters #3124

Closed

mxinden approved these changes May 8, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

mxinden added the send-it label May 8, 2023

Merge branch 'master' into 2680-explore

d7651b4

thomaseizinger removed the send-it label May 8, 2023

Add changelog entry

75681c1

This comment was marked as resolved.

Sign in to view

thomaseizinger added 2 commits May 8, 2023 10:42

Merge branch 'master' into 2680-explore

bdfb04f

Sort imports

88362e5

thomaseizinger added the send-it label May 8, 2023

This comment was marked as resolved.

Sign in to view

Merge branch 'master' into 2680-explore

3c8d326

This comment was marked as resolved.

Sign in to view

Merge branch 'master' into 2680-explore

77e4e5b

This comment was marked as resolved.

Sign in to view

Merge branch 'master' into 2680-explore

5a772e1

mergify bot merged commit b035fc8 into master May 8, 2023

mergify bot deleted the 2680-explore branch May 8, 2023 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: report changes in supported protocols to `ConnectionHandler` #3651

feat: report changes in supported protocols to `ConnectionHandler` #3651

thomaseizinger commented Mar 20, 2023 •

edited

Loading

thomaseizinger commented Mar 22, 2023 •

edited

Loading

thomaseizinger commented Mar 22, 2023

thomaseizinger commented Mar 24, 2023 •

edited

Loading

This comment was marked as resolved.

mxinden commented Mar 27, 2023

thomaseizinger commented Mar 29, 2023

thomaseizinger commented Apr 6, 2023

thomaseizinger commented Apr 6, 2023

mxinden May 8, 2023

thomaseizinger May 8, 2023

thomaseizinger May 8, 2023

mxinden May 8, 2023

thomaseizinger May 8, 2023

mxinden May 9, 2023

This comment was marked as resolved.

This comment was marked as resolved.

thomaseizinger commented May 8, 2023

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

mxinden commented May 9, 2023

feat: report changes in supported protocols to ConnectionHandler #3651

feat: report changes in supported protocols to ConnectionHandler #3651

Conversation

thomaseizinger commented Mar 20, 2023 • edited Loading

Description

Notes & open questions

Dependencies

Dependents

Change checklist

thomaseizinger commented Mar 22, 2023 • edited Loading

thomaseizinger commented Mar 22, 2023

thomaseizinger commented Mar 24, 2023 • edited Loading

This comment was marked as resolved.

mxinden commented Mar 27, 2023

Full vs partial view

In- and outbound protocols

Successful upgrades as source of input

Replacement of PollParameters::supported_protocols

Alternative design

thomaseizinger commented Mar 29, 2023

thomaseizinger commented Apr 6, 2023

thomaseizinger commented Apr 6, 2023

mxinden May 8, 2023

Choose a reason for hiding this comment

thomaseizinger May 8, 2023

Choose a reason for hiding this comment

thomaseizinger May 8, 2023

Choose a reason for hiding this comment

mxinden May 8, 2023

Choose a reason for hiding this comment

thomaseizinger May 8, 2023

Choose a reason for hiding this comment

mxinden May 9, 2023

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

thomaseizinger commented May 8, 2023

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

mxinden commented May 9, 2023

feat: report changes in supported protocols to `ConnectionHandler` #3651

feat: report changes in supported protocols to `ConnectionHandler` #3651

thomaseizinger commented Mar 20, 2023 •

edited

Loading

thomaseizinger commented Mar 22, 2023 •

edited

Loading

thomaseizinger commented Mar 24, 2023 •

edited

Loading

Replacement of `PollParameters::supported_protocols`