Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

swarm/: NetworkBehaviour/ConnectionHandler cross-communication #2680

Closed
mxinden opened this issue May 30, 2022 · 11 comments · Fixed by #3651
Closed

swarm/: NetworkBehaviour/ConnectionHandler cross-communication #2680

mxinden opened this issue May 30, 2022 · 11 comments · Fixed by #3651
Assignees
Labels
difficulty:moderate help wanted priority:important The changes needed are critical for libp2p, or are blocking another project

Comments

@mxinden
Copy link
Member

mxinden commented May 30, 2022

Description

Today there is no way to send generic local and remote node information from one NetworkBehaviour to another NetworkBehaviour, apart from NetworkBehaviour::inject_new_external_addr.

Examples of such generic information could be:

  • Supported protocols by the local peer.
  • Supported protocols of a remote peer.
  • Addresses of a remote peer.

Motivation

One example use-case is the exchange of information betweenlibp2p-identify and libp2p-kad to support Kademlia Client Mode (see #2032).

  1. libp2p-kad needs to know whether a remote peer advertises /ipfs/kad/1.0.0 via the identify protocol to decide whether to include the remote peer in its routing table or not.
  2. libp2p-identify needs to know whether the local node runs in client or in server mode, i.e. supports the /ipfs/kad/1.0.0 or not.

See #2521 which is currently blocked on this.

Proposal

Below is an initial action plan proposal based on a meeting with @MarcoPolo.

  1. Proof of concept 1 (communicating remote metadata across behaviours)
    • Extend NetworkBehaviourAction with NetworkBehaviourAction::ReportRemoteMetaData, containing a delta update.
    • Add NetworkBehaviourAction::inject_remote_metadata.
    • Write a Select NetworkBehaviour which takes two NetworkBehaviour implementations and passes ReportRemoteMetaData emitted by one behaviour to the other via inject_remote_metadata.
    • Showcase how libp2p-kad can learn supported protocols of remote peers from libp2p-identify.
  2. Proof of concept 2 (communicating local metadata across behaviours)
    • Extend NetworkBehaviourAction with NetworkBehaviourAction::ReportSupportedProtocol which each NetworkBehaviour emits when it supports a protocol as a listener and which is passed to other NetworkBehaviours via the Select NetworkBehaviour.
    • Showcase how remote peers can learn that libp2p-kad moved from client mode into server mode via libp2p-identify.
  3. In case proof of concepts looks good, don't merge but consolidate inject_* methods into single inject_event method with a single enum. Motivation here is to not have yet another inject_*** method on NetworkBehaviour.
  4. Adjust proof of concepts, implement Select behaviour logic in libp2p-swarm-derive and merge.
  5. Add method on Swarm (e.g. add_remote_metadata) which would call NetworkBehaviour::inject_event(Event::RemoteMetaData) on the root NetworkBehaviour.
  6. Design AddressBook type which each NetworkBehaviour can instantiate and pass a reference from inject_event into to track peer metadata.

Open questions

Are you planning to do it yourself in a pull request?

No, or at least not any time soon.

@thomaseizinger
Copy link
Contributor

Supported protocols are connection-specific so this should likely be reported on a connection-level by ConnectionHandlers and not NetworkBehaviours. Plus the connection is already tied to a peer so the swarm will simply "know" what we are talking about.

I think there is also a nice symmetry here: A peer changing its supported protocols only concerns that one connection so we should only tell that one ConnectionHandler about it. Whether that is reported up to the NetworkBehaviour is protocol-specific.

@mxinden
Copy link
Member Author

mxinden commented Feb 13, 2023

Supported protocols are connection-specific so this should likely be reported on a connection-level by ConnectionHandlers and not NetworkBehaviours.

I have never thought about it this way. Thus far in libp2p-kad I was planning to include a peer in our routing table if it advertises the Kademlia protocol on any of its connection. I guess we could include the peer under the address of the connection only.

👍 I think this is worth exploring.

@mxinden mxinden added the priority:important The changes needed are critical for libp2p, or are blocking another project label Feb 13, 2023
@thomaseizinger
Copy link
Contributor

Supported protocols are connection-specific so this should likely be reported on a connection-level by ConnectionHandlers and not NetworkBehaviours.

I have never thought about it this way. Thus far in libp2p-kad I was planning to include a peer in our routing table if it advertises the Kademlia protocol on any of its connection. I guess we could include the peer under the address of the connection only.

👍 I think this is worth exploring.

I just realized that the above may be too rust-libp2p specific. In here, protocols are connection-specific (because bound to a ConnectionHandler). This may not be true for other implementations?

Going back to the specific kademlia example: In theory, a node could be publicly reachable on one transport (i.e. QUIC) but not on another (i.e. TCP) so each ConnectionHandler should probably decide for itself whether they underlying transport's address gets included the routing table or not?

@thomaseizinger
Copy link
Contributor

Supported protocols are connection-specific so this should likely be reported on a connection-level by ConnectionHandlers and not NetworkBehaviours.

I have never thought about it this way. Thus far in libp2p-kad I was planning to include a peer in our routing table if it advertises the Kademlia protocol on any of its connection. I guess we could include the peer under the address of the connection only.

👍 I think this is worth exploring.

I just realized that the above may be too rust-libp2p specific. In here, protocols are connection-specific (because bound to a ConnectionHandler). This may not be true for other implementations?

Do we have any recommendations in the specs for this? cc @MarcoPolo

Is a node expected to report (and support) the same set of protocols on each connection?

@mxinden
Copy link
Member Author

mxinden commented Feb 24, 2023

Do we have any recommendations in the specs for this? cc @MarcoPolo

I don't think this is specified anywhere.

Is a node expected to report (and support) the same set of protocols on each connection?

That was my mind-set thus far, with the exception of relayed connections.

All that said, I am in favor of exploring the approach you propose above.

@thomaseizinger
Copy link
Contributor

Do we have any recommendations in the specs for this? cc @MarcoPolo

I don't think this is specified anywhere.

I opened an issue to discuss this: libp2p/specs#525

Is a node expected to report (and support) the same set of protocols on each connection?

That was my mind-set thus far, with the exception of relayed connections.

All that said, I am in favor of exploring the approach you propose above.

Sounds good, I'll work on this next after finishing up generic connection management.

@thomaseizinger
Copy link
Contributor

I think it would be nice to have #2831 for this.

@thomaseizinger
Copy link
Contributor

I've put some more thought into this after looking at the code:

  1. We already call ConnectionHandler::listen_protocol for each stream upgrade. At this point, we know which protocols we support on this connection.
  2. We can cache this list and diff it on every upgrade. If we find a change, we can notify the connection.
  3. Implementations of ConnectionHandler that want to change their supported protocols would have to do so in other "callbacks" to the ConnectionHandler by adjusting state internally and emitting a different set of protocols on the next call to listen_protocol.

@thomaseizinger
Copy link
Contributor

In #3579, an idea came up that touches on this issue. In the same way that #3651 immediately re-injects all protocols gathered from the ConnectionHandler as a list, we could add a command to NetworkBehaviour for reporting a peers address which would immediately get re-injected into the behaviour (and thus down the behaviour tree).

This would allow us to build a standalone address book component that can learn about the addresses of peers from other behaviours such as identify, kademlia or gossipsub. This could make it obsolete for each of these behaviours to participate in storing and emitting addresses upon a new peer being dialed.

@thomaseizinger
Copy link
Contributor

This would allow us to build a standalone address book component that can learn about the addresses of peers from other behaviours such as identify, kademlia or gossipsub. This could make it obsolete for each of these behaviours to participate in storing and emitting addresses upon a new peer being dialed.

Alternatively, we could add a first-class address book to Swarm. What worries me a bit about that idea are requirements like persistent address books or async resolving of addresses. At least the former we wouldn't need to deal with if an address book is just another NetworkBehaviour.

@thomaseizinger thomaseizinger changed the title swarm/: NetworkBehaviour cross-communication swarm/: NetworkBehaviour/ConnectionHandler cross-communication Apr 13, 2023
@mergify mergify bot closed this as completed in #3651 May 8, 2023
mergify bot pushed a commit that referenced this issue May 8, 2023
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 `ConnectionHandler`s 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.

Pull-Request: #3651.
@thomaseizinger
Copy link
Contributor

With #3651, we've merged the necessary support for this to enable kademlia client mode. More importantly, this also sets a direction for how we want to resolve similar problems. In particular, the design to follow is to emit an event and immediately inject it into the component again.

Having this concept in place should make it straight-forward to implement the support for other solutions once the need arises.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty:moderate help wanted priority:important The changes needed are critical for libp2p, or are blocking another project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants