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
feat: auto relay #723
feat: auto relay #723
Changes from all commits
d4bee23
6c901b9
1dccb7c
02dbdc3
d8f0849
bd613cb
ac430a2
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.
This isn't going to be sufficient, we'll need to try and prioritize public addresses for this. It's still pretty common for peers to advertise private addresses. We can make a note to do this in a followup PR though.
HOP relays should really avoid advertising private addresses (we should document this in a "setting up relays" section of the production guides), but we can't rely on this behavior.
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, we have a milestone for it, but I will add a comment: #699 (comment)
I am not sure yet on the best approach for this, but it is being tracked
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.
Why not just do the add after the listen call in the try? Then the delete isn't required if it fails.
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.
Well, we will block inside the following
try ... catch
block, waiting for thetransportManager.listen
.If we get multiple calls at the same time, all of them will not be blocked from reaching the
transportManager.listen
and we will probably end up with more listenRelays than needed.Perhaps, I can use a
p-queue
instead? Otherwise, I might also cancel others that fail (should not expecttransportManager.listen
to fail, but it can)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 we can leave it like this for now. It really shouldn't fail, we're already connected. If it does fail that should mean the connection was dropped during the listen attempt, which should trigger us to connect to another known relay if it exists.
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.
Something to consider here is that in AutoRelay we are creating a listen addr and then calling
transportManager.listen
to "connect". We will already have a connection per the AutoRelay logic, so that address is going to get thrown out and replaced with whatever this address is. We need to do some reconciliation here, I would find it odd to call listen with one address, and then have my actual address end up being something different.I think this likely won't be a huge issue for initial relay connections, but if we reconnect to known relays, this address could change. The provided address should be the address we end up with, but if it fails for some reason we will dial other known addresses for the peer.
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 is true! I changed this to avoid creating multiple connections to the same peer, but I agree with your point.
So you suggest that we go back and use the
connectToPeer
with a fallback to the dial if we fail?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.
No. We shouldn't care how we're connected to the peer, really, but we need to be careful about the address we're advertising for this. Something like: Prioritize public addresses, if the connected address matches one of those use it, otherwise pick one of the others.
Again, I think we can do a follow PR for this, so we can focus on clear tests for that.