-
Notifications
You must be signed in to change notification settings - Fork 948
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
refactor(*): Transport redesign #4568
Conversation
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.
Great start! I left some high-level comments :)
I have implemented some of the suggestions. However I didn't touch the question on whether DialOpts should be struct or the field should just be passed as arguments. I also moved some methods into a macro. |
Since we are touching |
…n if we already have a listener for outgoing connections.
At the moment I'm pretty pleased with the implementation. But I don't really like having the list of things that we wan't to strip. It feels like something that might cause a bug later on, but I honestly don't have a better idea. |
As long as we cover it with unit tests, that is fine. We can always ship improvements as patch releases later if we find that we are missing something :) |
This pull request has merge conflicts. Could you please resolve them @umgefahren? 🙏 |
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 this is a fine solution. It is known that one of the disadvantages of enabling port reuse is that you can only have a single connection to another peer. Thus, our best way of fulfilling the request to establish another connection is to ignore port reuse and simply allocate a new port. I think that is better than the connection failing.
I have two suggestions for slightly revising the log and bumping its severity.
Co-authored-by: Thomas Eizinger <[email protected]>
I'm glad you are fine with the solution and I implemented your suggestions. I'm ready to merge. |
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.
LGTM
Great to see this being so close to merging. Thanks for pushing this over the finish line @umgefahren. |
Is there something left for me to do for merging? |
Not at this stage. I'll leave it to @jxs to coordinate when it is good to go in! |
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.
LGTM Thanks Hannes for your hard work, and Thomas for the review! ❤️
I will try to release 0.54
on Monday
Thanks to everyone involved, I'm sorry it took so much longer than anticipated. |
Do you not want to release AutoNatV2 with this together? There is another PR coming for that I'd assume. |
In case you do: I have opened #5526. |
Closes: #4524 This is the implementation of the evolved AutoNAT protocol, named AutonatV2 as defined in the [spec](https://github.com/libp2p/specs/blob/03718ef0f2dea4a756a85ba716ee33f97e4a6d6c/autonat/autonat-v2.md). The stabilization PR for the spec can be found under libp2p/specs#538. The work on the Rust implementation can be found in the PR to my fork: umgefahren#1. The implementation has been smoke-tested with the Go implementation (PR: libp2p/go-libp2p#2469). The new protocol addresses shortcomings of the original AutoNAT protocol: - Since the server now always dials back over a newly allocated port, this made #4568 necessary; the client can be sure of the reachability state for other peers, even if the connection to the server was made through a hole punch. - The server can now test addresses different from the observed address (i.e., the connection to the server was made through a `p2p-circuit`). To mitigate against DDoS attacks, the client has to send more data to the server than the dial-back costs. Pull-Request: #5526.
Resolves: libp2p#4226. Resolves: libp2p#3953. Resolves: libp2p#3889. Pull-Request: libp2p#4568.
Closes: libp2p#4524 This is the implementation of the evolved AutoNAT protocol, named AutonatV2 as defined in the [spec](https://github.com/libp2p/specs/blob/03718ef0f2dea4a756a85ba716ee33f97e4a6d6c/autonat/autonat-v2.md). The stabilization PR for the spec can be found under libp2p/specs#538. The work on the Rust implementation can be found in the PR to my fork: umgefahren#1. The implementation has been smoke-tested with the Go implementation (PR: libp2p/go-libp2p#2469). The new protocol addresses shortcomings of the original AutoNAT protocol: - Since the server now always dials back over a newly allocated port, this made libp2p#4568 necessary; the client can be sure of the reachability state for other peers, even if the connection to the server was made through a hole punch. - The server can now test addresses different from the observed address (i.e., the connection to the server was made through a `p2p-circuit`). To mitigate against DDoS attacks, the client has to send more data to the server than the dial-back costs. Pull-Request: libp2p#5526.
Description
Resolves: #4226.
Resolves: #3953.
Resolves: #3889.
Notes & open questions
What do you think of the implementation?
Change checklist