protocols/dcutr/examples: Wait to tell relay its public addr #2659
Conversation
This file contains 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
As a listening client, when requesting a reservation with a relay, the relay responds with its public addresses. The listening client can then use the public addresses of the relay to advertise itself as reachable under a relayed address (`/<public-relay-addr>/p2p-circuit/p2p/<listening-client-peer-id>`). The above operates under the assumption that the relay knows its public address. A relay learns its public address from remote peers, via the identify protocol. In the case where the relay just started up, the listening client might be the very first node to connect to it. Such scenario allows for a race condition. The listening client requests a reservation from the relay, while the relay requests its public address from the listening client. The former needs to contain the response from the latter. This commit serializes the two requests, making sure, in the case of a freshly started relay, that the listening client tells the relay its public address before requesting a reservation from the relay.
|
//CC @Foemass |
elenaf9
reviewed
May 19, 2022
protocols/dcutr/examples/client.rs
Outdated
Comment on lines
226
to
236
| opts.relay_address | ||
| .clone() | ||
| .with(Protocol::P2pCircuit) | ||
| .with(Protocol::P2p(opts.remote_peer_id.unwrap().into())), | ||
| ) | ||
| .unwrap(); | ||
| } | ||
| Mode::Listen => { | ||
| swarm | ||
| .listen_on(opts.relay_address.clone().with(Protocol::P2pCircuit)) | ||
| .unwrap(); |
There was a problem hiding this comment.
Suggested change
| opts.relay_address | |
| .clone() | |
| .with(Protocol::P2pCircuit) | |
| .with(Protocol::P2p(opts.remote_peer_id.unwrap().into())), | |
| ) | |
| .unwrap(); | |
| } | |
| Mode::Listen => { | |
| swarm | |
| .listen_on(opts.relay_address.clone().with(Protocol::P2pCircuit)) | |
| .unwrap(); | |
| opts.relay_address | |
| .with(Protocol::P2pCircuit) | |
| .with(Protocol::P2p(opts.remote_peer_id.unwrap().into())), | |
| ) | |
| .unwrap(); | |
| } | |
| Mode::Listen => { | |
| swarm | |
| .listen_on(opts.relay_address.with(Protocol::P2pCircuit)) | |
| .unwrap(); |
Nit: don't think we need to clone the address here; it's not used afterwards anymore.
protocols/dcutr/examples/client.rs
Outdated
Comment on lines
238
to
256
| // Wait for relay to accept reservation request. | ||
| block_on(async { | ||
| loop { | ||
| match swarm.next().await.unwrap() { | ||
| SwarmEvent::NewListenAddr { .. } => {} | ||
| SwarmEvent::Dialing { .. } => {} | ||
| SwarmEvent::ConnectionEstablished { .. } => {} | ||
| SwarmEvent::Behaviour(Event::Ping(_)) => {} | ||
| SwarmEvent::Behaviour(Event::Relay( | ||
| client::Event::ReservationReqAccepted { .. }, | ||
| )) => { | ||
| info!("Relay accepted our reservation request."); | ||
| break; | ||
| } | ||
| SwarmEvent::Behaviour(Event::Identify(_)) => {} | ||
| event => panic!("{:?}", event), | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Why do we need to do a separate loop here to wait for the ReservationReqAccepted? We don't initiate any following action, but instead directly jump into the next loop below. Couldn't we just remove this block?
elenaf9
approved these changes
May 19, 2022
thomaseizinger
approved these changes
May 20, 2022
Co-authored-by: Elena Frank <[email protected]>
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.
Description
As a listening client, when requesting a reservation with a relay, the relay
responds with its public addresses. The listening client can then use the public
addresses of the relay to advertise itself as reachable under a relayed
address (
/<public-relay-addr>/p2p-circuit/p2p/<listening-client-peer-id>).The above operates under the assumption that the relay knows its public address.
A relay learns its public address from remote peers, via the identify protocol.
In the case where the relay just started up, the listening client might be the
very first node to connect to it.
Such scenario allows for a race condition. The listening client requests a
reservation from the relay, while the relay requests its public address from the
listening client. The former needs to contain the response from the latter.
This commit serializes the two requests, making sure, in the case of a freshly
started relay, that the listening client tells the relay its public address
before requesting a reservation from the relay.
Links to any relevant issues
Follow up to #2642. Suggested in #2621 (comment).
Note that hole punching is still broken due to #2651.
Open Questions
Change checklist