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.
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
relay/DCUtR: Add Direct Connection Upgrade through Relay protocol #173
relay/DCUtR: Add Direct Connection Upgrade through Relay protocol #173
Changes from 18 commits
727f8b1
9db77f0
fee2b99
97e5d61
75ed30b
4ccccf5
4b9549a
73064f9
dfc988c
46bd410
4e94481
9d42524
9958df2
4b7c1ce
fe64a21
db9475e
2d8b38f
6530d45
0076c69
b420064
af0b9bb
6f475de
17f6275
5943d3b
6f558f1
f7b43df
85f567d
cab60cc
8001cd9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we see much better numbers than this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they have been in the same ballpark, but I might as well be mistaken. Unfortunately I am unable to access the data from project flare phase 1. Either the data or my access seems to be removed.
@vyzo do you know more here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion continued on #173 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it possible that
A
may also be directly reachable at a private address ifA
andB
are on the same local network?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is possible, but that would have been dialed directly as the private addresses are still advertised with relay addresses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @albrow has a point. @vyzo: while that should be the case, if we want to be resilient and robust, this protocol should not make assumptions about how any other part of the system behaves. Usually those implicit assumptions make systems brittle.
Luckily our spec lifecycle process allows us to add this topic as an active discussion:
from: https://github.com/libp2p/specs/blob/master/00-framework-01-spec-lifecycle.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not making this assumption will make us dial private addresses in vain multiple times.
We already have a problem with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At best, we can consider dialing them in the bidirectional part of the protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if A is public and B is private, we can't possibly be behind the same NAT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore, for the bidirectional part of the protocol we could check the public address of the other node. If that doesn't match our own, we can't possibly be behind the same NAT and dialing private addrs is pointless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to avoid dialing private addrs if we can avoid it though. Perhaps we could still exchange them, but in a separate field. Then they can be ignored unless your public address matches the other node and you infer that you're behind the same NAT. Or your implementation may be able to always ignore them, since they would have been dialed previously.
Anyway, I agree that we could punt on this for this round and discuss when we promote to candidate rec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the protocol name
/libp2p/dcutr
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the added retry logic. Also see FAQ further below for additional reasoning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change makes a lot of sense to me.