Full support for multiple connections per peer in libp2p-swarm. #1519
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 4 commits
March 25, 2020 15:08
This commit makes the notion of multiple connections per peer first-class in the API of libp2p-swarm, introducing the new callbacks `inject_connection_established` and `inject_connection_closed`. The `endpoint` parameter from `inject_connected` and `inject_disconnected` is removed, since the first connection to open may not be the last connection to close, i.e. it cannot be guaranteed, as was previously the case, that the endpoints passed to these callbacks match up.
So that identify requests can be answered with the correct observed address of the connection on which the request arrives.
twittner
approved these changes
Mar 26, 2020
tomaka
suggested changes
Mar 26, 2020
|
The only thing that I think should be resolved before merging is the |
added 6 commits
March 30, 2020 11:41
To retain the possibility of not re-exposing all network configuration choices, thereby providing a more convenient API on the \`SwarmBuilder\`.
This was referenced Mar 31, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up to #1440 that introduces "first class" support for multiple connections per peer to
libp2p-swarm. Concretely:inject_connection_establishedandinject_connection_closedare added to theNetworkBehaviourwith empty default impls.inject_connectedandinject_disconnectedno longer get theendpointas an argument. This is primarily because these callbacks are invoked for the first established connection, respectively last closed connection, whose endpoints may now differ, as opposed to the previous behaviour. Network behaviours that made use of the endpoint will naturally have to implement the new callbacks and split the code previously ininject_connected/inject_disconnected, as is demonstrated in some of the behaviours updated in this PR.NetworkBehaviourAction::DialPeeras well esExpandedSwarm::dialnow take another parameter, aDialPeerCondition, which specifies the conditions under which a new dialing attempt should be initiated. The default (and what most behaviours migrating old code will want to use) isDialPeerCondition::Disconnected.NetworkConfigof a swarm (which includes connection limits) can now be fully customised viaSwarmBuilder::network_config, in order to avoid code duplication. For example, substrate will probably want to configure the maximum connections per peer to2(just to permit concurrent dialing attempts) and possibly also configure limits for concurrent inbound and outbound connections.It is worth mentioning that I took the opportunity to change
inject_connectedto take the peer ID by reference, rather than by value, just likeinject_disconnectedand most other methods. This simplifies things a bit in the derive-macro code and can avoid a bit of unnecessary cloning. I further did this because the signature ofinject_connectedchanged anyway in this PR.This PR is expected to be rebased over #1515 eventually.