transports/quic: Refactor listener handling (updated) #19

elenaf9 · 2022-07-10T12:12:17Z

Description

Replaces #17. Rebased onto latest kpp::libp2p-quic and merged latest libp2p::main. Apart form that, the only other change compared to #17 is to remove the Mutex in InAddr (ef7b823).

Copying over the description from #17:

Discussed in libp2p#2289 (comment).
This PR adapts the quic transport to the upstream listener changes of libp2p#2652. Additionally, it now allows the quic transport to listen on multiple addresses / endpoints. The changes include:

QuicTransport now owns and drives multiple endpoints. An endpoint is created each time Transport::listen_on is called.
When dialing a remote, a random existing listening endpoint is used. If none exists a new one is created that only supports outbound connections (i.e. server config is None). If then a new listening endpoint is created any following dials will use the new endpoint, but existing connections are not migrated.

Open Questions

If a first listening endpoint is created and we already have an endpoint used for dialing (without server capabilities enabled): should we, instead of creating a new endpoint, enable server capabilities on the old endpoint and rebind it to the new listening address? The benefit of this would be that the remote peers could then update their address information once they notice the address change. For later dial attempts they would then know the correct address to dial.

Quoting response from @mxinden:

This sounds very neat to me, especially the part where the remote is notified of the address change. I am fine with this landing in a later iteration in case it is a big undertaking.

-> I will look into it and come back to this.

Rare case: if the user starts listening on a socket address that we already randomly chose for an existing dialer-endpoint, what should happen?

Quoting again response from @mxinden:

I would be fine with not handling this case, that is erroring on the listen_on call, forcing the user to handle the case.

I am guessing re-using the existing dialer-endpoint is complicated, especially when handling listen requests across interfaces (i.e. 0.0.0.0) as we would need to do an additional listen_on call for the remaining interfaces with the specified port, rig

-> If we decide to add the logic mentioned in question 1. to migrate a dialing endpoint to be the new listener, we have to deal the case of one interface vs multiple anyway. So it probably makes sense to solve this together. Will look into it.

…td::error::Error` (libp2p#2710) * core/muxing: Remove `Into<io::Error>` bound from `StreamMuxer::Error` This allows us to preserve the type information of a muxer's concrete error as long as possible. For `StreamMuxerBox`, we leverage `io::Error`'s capability of wrapping any error that implements `Into<Box<dyn Error>>`. * Use `?` in `Connection::poll` * Use `?` in `muxing::boxed::Wrap` * Use `futures::ready!` in `muxing::boxed::Wrap` * Fill PR number into changelog * Put `Error + Send + Sync` bounds directly on `StreamMuxer::Error` * Move `Send + Sync` bounds to higher layers * Use `map_inbound_stream` helper * Update changelog to match new implementation

…p#2726) Print remote peer ID when seeing a second identify push stream coming in.

…p#2727)

…ibp2p#2696) (libp2p#2701)

See e04e95a for the rational. With tokio `v1.19.0` released, `TcStream` exposes `take_error`. This commit applies the same fix from e04e95a to the tokio TCP provider.

…ibp2p#2731) Bumps [styfle/cancel-workflow-action](https://github.com/styfle/cancel-workflow-action) from 0.9.1 to 0.10.0. - [Release notes](https://github.com/styfle/cancel-workflow-action/releases) - [Commits](styfle/cancel-workflow-action@a40b884...bb6001c) --- updated-dependencies: - dependency-name: styfle/cancel-workflow-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

* protocols/gossipsub: Fix a typo in error message * protocols/gossipsub: Make the error message accurately

…ibp2p#2742)

…bp2p#2733)

Allow for custom protocol ID via `GossipsubConfigBuilder::protocol_id()`.

…ect (libp2p#2744) **Summary** of the plot of the `discover_peer_after_disconnect` test: 1. `swarm2` connects to `swarm1`. 2. `swarm2` requests an identify response from `swarm1`. 3. `swarm1` sends the response to `swarm2`. 4. `swarm2` disconnects from `swarm1`. 5. `swarm2` tries to disconnect. **Problem** `libp2p-identify` sets `KeepAlive::No` when it identified the remote. Thus `swarm1` might identify` `swarm2` before `swarm2` identified `swarm1`. `swarm1` then sets `KeepAlive::No` and thus closes the connection to `swarm2` before `swarm2` identified `swarm1`. In such case the unit test `discover_peer_after_disconnect hangs indefinitely. **Solution** Add an initial delay to `swarm1` requesting an identification from `swarm2`, thus ensuring `swarm2` is always able to identify `swarm1`.

…er (libp2p#2652) Remove the concept of individual `Transport::Listener` streams from `Transport`. Instead the `Transport` is polled directly via `Transport::poll`. The `Transport` is now responsible for driving its listeners.

…iple-endpoints-2

Adapt to the transport changes of libp2p#2652. Note: this is only a draft "to make it work", and not a proper implementation. It does not support listening on multiple addresses. The listening logic with multiple Endpoints will need to be supported for the upstream implementation.

Co-authored-by: Max Inden <[email protected]>

elenaf9

Copied over the open discussions from #17. Lmk if I forgot something.

transports/quic/src/endpoint.rs

transports/quic/src/transport.rs

transports/quic/src/endpoint.rs

mxinden · 2022-07-11T04:06:41Z

@elenaf9 thanks for porting all the past discussions over here. As far as I can tell there is no input needed from my side right now. Please correct me if I am wrong. Also please let me know in case you want me to review any of these changes again.

kpp · 2022-07-18T13:35:29Z

I tested it:
libp2p-perf: works 👍
substrate: works but same issue: server is not notified when client drops

Also one minor issue: I got Error while sending on QUIC UDP socket: Os { code: 97, kind: Uncategorized, message: "Address family not supported by protocol" } when sending data from 0.0.0.0 to ::1.

LGTM. Other issues can be solved in follow up PRs.

mxinden · 2022-07-19T01:43:26Z

Also one minor issue: I got Error while sending on QUIC UDP socket: Os { code: 97, kind: Uncategorized, message: "Address family not supported by protocol" } when sending data from 0.0.0.0 to ::1.

Do I understand correctly that you only had the 0.0.0.0 listen address, or also ::/0 @kpp?

kpp · 2022-07-19T11:09:04Z

I had two nodes, one is 0.0.0.0 only, the other is ::1 only.

Review fixes

elenaf9 · 2022-07-23T21:29:08Z

Thank you for the reviews @mxinden @kpp!

Also one minor issue: I got Error while sending on QUIC UDP socket: Os { code: 97, kind: Uncategorized, message: "Address family not supported by protocol" } when sending data from 0.0.0.0 to ::1.

Fixed the logic now for selecting an endpoint for dialing:
Analogous to how socket reuse is done in the tcp transport, we now look in our existing listeners for an endpoint with matching socket family (Ipv4 / Ipv6) and loopback interface (is_loopback). If none is found we create a new dialer that is listening on a wildcard address with the matching socket family. Thus we now have two dialing endpoints, one for Ipv4 and one for Ipv6 addresses.
@kpp thanks for adding the ipv4_dial_ipv6 test! It now passes.

From my side this is ready for merge. I can not merge the latest upstream master because of some more muxer changes. @kpp I can look into fixing them, unless you are already working on it?

Edit: CI is failing because of libp2p#2770 which is independent of libp2p-quic, but came up because we use libp2p-core as dependency.

kpp · 2022-07-24T16:41:17Z

I can look into fixing them, unless you are already working on it?

Sure. I tried but it was a little bit hard because your branch was not merged yet.

You can take this issue while I am working on that issue with disconnects.

elenaf9 · 2022-08-03T14:06:16Z

Sorry for the force-push. I did not change anything, just reverted the merge of upstream master. Will do a separate PR that includes the latest master and adapts the QuicMuxer.

Referencing a comment in libp2p#2722:
@kpp

I believe I should try to move QuicMuxer to a new API to give it a try.

To avoid duplicating work: are you working on this right now? From your above comment (#19 (comment)) I assumed that you are currently working on the disconnect issues, but I'm happy to sync on this in case that you have now started looking into this. Otherwise I'd draft the PR and we can discuss the solution there.

kpp · 2022-08-03T19:26:40Z

To avoid duplicating work: are you working on this right now?

No.

elenaf9 · 2022-08-03T23:00:46Z

Sorry for the force-push. I did not change anything, just reverted the merge of upstream master. Will do a separate PR that includes the latest master and adapts the QuicMuxer.

Referencing a comment in libp2p#2722: @kpp

I believe I should try to move QuicMuxer to a new API to give it a try.

To avoid duplicating work: are you working on this right now? From your above comment (#19 (comment)) I assumed that you are currently working on the disconnect issues, but I'm happy to sync on this in case that you have now started looking into this. Otherwise I'd draft the PR and we can discuss the solution there.

See elenaf9#6.

elenaf9 · 2022-08-19T18:39:24Z

For the sake of reducing the number of parallel open PRs related to quic, and also to ease reviewing:
I think we can already review and merge this PR independently of elenaf9#6, since they target different parts of the implementation. Wdyt @kpp? Even though it does not include all of the latest master changes, I think it is still an improvement.

Additional reviews would also be appreciated (cc @mxinden @thomaseizinger @MarcoPolo; relevent is only the diff within transports/quic).

mxinden

I am in favor of merging here as is. Thanks for the ping and thanks for staying on top of things @elenaf9.

mxinden · 2022-08-20T03:11:38Z

transports/quic/src/endpoint.rs

-        keypair: &libp2p_core::identity::Keypair,
-        multiaddr: Multiaddr,
-    ) -> Result<Self, tls::ConfigError> {
+    pub fn new(keypair: &libp2p_core::identity::Keypair) -> Result<Self, tls::ConfigError> {


🚀 great to no longer having to pass a Multiaddr.

kpp · 2022-08-21T08:15:56Z

I think we should merge it. I will review it today/tomorrow, wait for an additional reviews till the end of Monday and merge it.

kpp · 2022-08-22T12:17:43Z

bench `multiple-endpoints-2#bf06d967`

# Start Rust and Golang servers.
Local peer id: PeerId("Qmcqq9TFaYbb94uwdER1BXyGfCFY4Bb1gKozxNyVvLvTSw")
about to listen on "/ip4/127.0.0.1/udp/9992/quic"
Listening on "/ip4/127.0.0.1/udp/9992/quic".

# Rust -> Rust

Local peer id: PeerId("12D3KooWAmxKDBCLbYU8juAmRhUKGhEYcg79c4hCnvENraAosWkD")
IncomingConnection { local_addr: "/ip4/127.0.0.1/udp/9992/quic", send_back_addr: "/ip4/127.0.0.1/udp/40500/quic" }
ConnectionEstablished { peer_id: PeerId("12D3KooWAmxKDBCLbYU8juAmRhUKGhEYcg79c4hCnvENraAosWkD"), endpoint: Listener { local_addr: "/ip4/127.0.0.1/udp/9992/quic", send_back_addr: "/ip4/127.0.0.1/udp/40500/quic" }, num_established: 1, concurrent_dial_errors: None }
Behaviour(PerfRunDone(10.000694835s, 2232061300))
Interval	Transfer	Bandwidth
0 s - 10.00 s	2232 MBytes	1785.29 MBit/s
ConnectionClosed { peer_id: PeerId("Qmcqq9TFaYbb94uwdER1BXyGfCFY4Bb1gKozxNyVvLvTSw"), endpoint: Dialer { address: "/ip4/127.0.0.1/udp/9992/quic", role_override: Dialer }, num_established: 0, cause: None }

# Rust -> Golang

Local peer id: PeerId("12D3KooWPY3YCErc99qJ74h363qjHHf93DFuuK8U5n2SPGJHooxD")
Interval	Transfer	Bandwidth
0 s - 10.00 s	2056 MBytes	1644.61 MBit/s
ConnectionClosed { peer_id: PeerId("12D3KooWL3XJ9EMCyZvmmGXL2LMiVBtrVa2BuESsJiXkSj7333Jw"), endpoint: Dialer { address: "/ip4/127.0.0.1/udp/9993/quic", role_override: Dialer }, num_established: 0, cause: None }


# Golang -> Rust

2022/08/22 15:11:51 failed to sufficiently increase receive buffer size (was: 208 kiB, wanted: 2048 kiB, got: 416 kiB). See https://github.com/lucas-clemente/quic-go/wiki/UDP-Receive-Buffer-Size for details.
IncomingConnection { local_addr: "/ip4/127.0.0.1/udp/9992/quic", send_back_addr: "/ip4/127.0.0.1/udp/32942/quic" }
ConnectionEstablished { peer_id: PeerId("12D3KooWEiWwtDiNQcwynr4TUzY3m1zFVJ9YDVifDb7FQBajCt9Y"), endpoint: Listener { local_addr: "/ip4/127.0.0.1/udp/9992/quic", send_back_addr: "/ip4/127.0.0.1/udp/32942/quic" }, num_established: 1, concurrent_dial_errors: None }
Interval 	Transfer	Bandwidth
0s - 10.00 s 	1672 MBytes	1337.58 MBit/s
Behaviour(PerfRunDone(9.999930943s, 1672064000))

# Golang -> Golang

2022/08/22 15:12:01 failed to sufficiently increase receive buffer size (was: 208 kiB, wanted: 2048 kiB, got: 416 kiB). See https://github.com/lucas-clemente/quic-go/wiki/UDP-Receive-Buffer-Size for details.
Interval 	Transfer	Bandwidth
0s - 10.00 s 	1681 MBytes	1344.74 MBit/s

thomaseizinger · 2022-08-23T06:35:31Z

Additional reviews would also be appreciated (cc @mxinden @thomaseizinger @MarcoPolo; relevent is only the diff within transports/quic).

I am not gonna be able to fit a review in in time but I think this has had a good few pairs of eyes. Happy to see it merged. The numbers look good too :)

mxinden · 2022-08-25T09:05:06Z

Friendly ping @kpp. Would you mind hitting the merge button?

kpp · 2022-08-25T13:50:03Z

@mxinden I am not quite sure whether we have to merge it right now because of elenaf9#6 (comment).

elenaf9 · 2022-08-25T16:13:48Z

@mxinden I am not quite sure whether we have to merge it right now because of elenaf9#6 (comment).

I think we can already merge this PR (if you have reviewed and approved) because it does not depend on elenaf9#6. The issue about not being compatible with the go-impl that appears on that other PR is not an issue here.
We don't per-se need to already merge it, but reducing the overall number of pending PRs related to QUIC makes it easier for folks on the outside to understand what is happening where. Any reason why you would prefer to wait @kpp?

thomaseizinger and others added 25 commits June 24, 2022 08:26

protocols/identify: Extend log message on second identify push (libp2…

2c70c59

…p#2726) Print remote peer ID when seeing a second identify push stream coming in.

swarm/CHANGELOG: Merge unreleased section v0.36.2 with v0.37.0 (libp2…

072d736

…p#2727)

protocols/identify/: Add example (libp2p#2689)

40744be

protocols/relay: Don't duplicate p2p protocol in relay reservation (l…

f814b21

…ibp2p#2696) (libp2p#2701)

transports/tcp/: Call take_error on tokio TcpStream (libp2p#2725)

7190952

See e04e95a for the rational. With tokio `v1.19.0` released, `TcStream` exposes `take_error`. This commit applies the same fix from e04e95a to the tokio TCP provider.

protocols/identify: Fix dev deps for example (libp2p#2737)

423adca

protocols/gossipsub: Fix a typo in error message (libp2p#2739)

748588e

* protocols/gossipsub: Fix a typo in error message * protocols/gossipsub: Make the error message accurately

protocols/rendezvous/src/client: Fix clippy warning let-unit-value (l…

862ae14

…ibp2p#2742)

Cargo.toml: Undo version bump to v0.47.0 as v0.46.0 is unreleased (li…

31f1d66

…bp2p#2733)

protocols/gossipsub/: Allow custom protocol id (libp2p#2718)

6db5712

Allow for custom protocol ID via `GossipsubConfigBuilder::protocol_id()`.

*: Prepare v0.46.0 (libp2p#2730)

7df6bae

*: Bump swarm-derive version and prepare v0.46.1 (libp2p#2747)

2f2b7cb

swarm/: Set default dial concurrency factor to 8 (libp2p#2741)

d0da3a0

Merge branch 'master' of github.com:libp2p/rust-libp2p into quic/mult…

6b758e3

…iple-endpoints-2

transports/quic: support multiple listening endpoints

57743ef

transports/quic: re-use endpoints for dialing

e5e5b34

transports/quic: test endpoint re-use

39d855c

transports/quic: use intra-doc links in docs

0e797cf

Co-authored-by: Max Inden <[email protected]>

transports/quic: remove mutex in InAddr

ef7b823

transports/quic: fix clippy

a375524

elenaf9 mentioned this pull request Jul 10, 2022

transports/quic: Refactor listener handling #17

Closed

elenaf9 commented Jul 10, 2022

View reviewed changes

transports/quic/src/endpoint.rs Outdated Show resolved Hide resolved

transports/quic/src/transport.rs Outdated Show resolved Hide resolved

transports/quic/src/transport.rs Outdated Show resolved Hide resolved

transports/quic/src/endpoint.rs Show resolved Hide resolved

transport/quic: add test for closing a listener

917bccc

kpp and others added 6 commits July 21, 2022 17:07

Add an ignored test to dial ipv6 from ipv4

5fe3e1c

Get rid of recursion in poll_if_addr

574d534

cargo fmt

9e1cc81

Merge pull request #5 from kpp/kpp-multiple-endpoints

2c41f70

Review fixes

quic: match socket family of endpoint for dialing

1a4b8ac

quic: fix outdated docs & intra-doc links

bf06d96

mxinden mentioned this pull request Jul 26, 2022

feat: Add WebRTC transport libp2p/rust-libp2p#2622

Merged

4 tasks

elenaf9 force-pushed the quic/multiple-endpoints-2 branch from 7019d49 to bf06d96 Compare August 3, 2022 14:01

elenaf9 mentioned this pull request Aug 3, 2022

Re-design the StreamMuxer trait libp2p/rust-libp2p#2722

Closed

24 tasks

elenaf9 mentioned this pull request Aug 3, 2022

transports/quic: Adapt QuicMuxer to upstream StreamMuxer changes elenaf9/rust-libp2p#6

Closed

mxinden approved these changes Aug 20, 2022

View reviewed changes

kpp merged commit e0684ab into kpp:libp2p-quic Aug 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transports/quic: Refactor listener handling (updated) #19

transports/quic: Refactor listener handling (updated) #19

elenaf9 commented Jul 10, 2022 •

edited

Loading

elenaf9 left a comment

mxinden commented Jul 11, 2022

kpp commented Jul 18, 2022

mxinden commented Jul 19, 2022

kpp commented Jul 19, 2022

elenaf9 commented Jul 23, 2022 •

edited

Loading

kpp commented Jul 24, 2022

elenaf9 commented Aug 3, 2022 •

edited

Loading

kpp commented Aug 3, 2022

elenaf9 commented Aug 3, 2022

elenaf9 commented Aug 19, 2022

mxinden left a comment

mxinden Aug 20, 2022

kpp commented Aug 21, 2022 •

edited

Loading

kpp commented Aug 22, 2022

thomaseizinger commented Aug 23, 2022

mxinden commented Aug 25, 2022

kpp commented Aug 25, 2022

elenaf9 commented Aug 25, 2022

transports/quic: Refactor listener handling (updated) #19

transports/quic: Refactor listener handling (updated) #19

Conversation

elenaf9 commented Jul 10, 2022 • edited Loading

Description

Open Questions

elenaf9 left a comment

Choose a reason for hiding this comment

mxinden commented Jul 11, 2022

kpp commented Jul 18, 2022

mxinden commented Jul 19, 2022

kpp commented Jul 19, 2022

elenaf9 commented Jul 23, 2022 • edited Loading

kpp commented Jul 24, 2022

elenaf9 commented Aug 3, 2022 • edited Loading

kpp commented Aug 3, 2022

elenaf9 commented Aug 3, 2022

elenaf9 commented Aug 19, 2022

mxinden left a comment

Choose a reason for hiding this comment

mxinden Aug 20, 2022

Choose a reason for hiding this comment

kpp commented Aug 21, 2022 • edited Loading

kpp commented Aug 22, 2022

thomaseizinger commented Aug 23, 2022

mxinden commented Aug 25, 2022

kpp commented Aug 25, 2022

elenaf9 commented Aug 25, 2022

elenaf9 commented Jul 10, 2022 •

edited

Loading

elenaf9 commented Jul 23, 2022 •

edited

Loading

elenaf9 commented Aug 3, 2022 •

edited

Loading

kpp commented Aug 21, 2022 •

edited

Loading