fix: Fix voucher verification and data transfer limits in Circuit Relay v2 #698
Conversation
|
@guha-rahul : Nice work on the PR, Rahul. Appreciate the initiative and your efforts. Circuit Relay v2 is a very important module of NAT traversal. Wish to recommend some tests that would enable verification of the voucher verification and data transfer limit enforcement in a relay protocol. Please visit #715 Also, CCing @Winter-Soren and @sukhman-sukh if they would like to share feedback and thoughts on both the tests added and the PR itself. They have been collaborating on NAT traversal efforts and will be bootstrapping webrtc transport layer using NAT traversal soon. |
|
hi @guha-rahul nice work, I would like you to give an opinion on those TODOs since you just worked on this module. |
|
@acul71 sure will give you some feedback in the coming days. |
| """Handle a connect request.""" | ||
| peer_id = ID(msg.peer) | ||
| dst_stream: INetStream | None = None | ||
| logger.debug("Handling connect request to peer %s", peer_id) |
There was a problem hiding this comment.
Just a suggestion. Can we have a self.resource_manager._reservations.get(peer_id) check at start, and if it fails, then we can directly reject the request with NO_RESERVATION = 204 status. (similar to other implementations).
|
Also, one doubt. Does the current implementation handle the refresh of the reservation? I mean, if a connection is already made between a relay and a device, it refreshes the reservation to maintain connectivity and stability. Is that present in our version of the circuit relay? |
|
Interestingly, I found one thing in other implementations(go and js): they are issuing vouchers but not verifying them, but out implementation does. Nice to see that. |
|
Also, https://github.com/libp2p/go-libp2p/blob/7b7c3ed4ce048c99a865d4349b5f7484a6a93c04/p2p/protocol/circuitv2/relay/relay.go#L265-L269 here, they are denying the connection from relays with response = |
|
Rest of the code looks good to me. Logic is well covered and looks neat. Great work @guha-rahul. |
|
@sukhman-sukh https://github.com/libp2p/specs/blob/master/relay/circuit-v2.md#connection-initiation I think the note above this should be the reason aka to prevent multi-hop relay chaining. Thanks for the catch |
|
LGTM. |
|
@sukhman-sukh : Thank you Sukhman for the update. Appreciate your and @guha-rahul's efforts to arrive at a good conclusion on the circuit relay issues. |
|
@sukhman-sukh, @guha-rahul : This looks good for a final review + merge. Wish to ask if the circuit relay issues have been resolved. |
|
@guha-rahul : Wish if you could add @sukhman-sukh and @Winter-Soren as a collaborator in this PR. They have made recent contributions to circuit relay and we look forward to completing the circuit relay outstanding issues this week. @sukhman-sukh will be sharing important fixes to circuit relay soon. |
added all of them. |
|
@seetadev @sukhman-sukh is anything blocking the merging of this PR? I'd like to help in the relay circuit if it's needed. |
|
@LogicalGuy77 : Thank you so much Harshit for your initiative. Indeed, please collaborate with @sukhman-sukh and @Winter-Soren and focus on this PR: #767. Once the other PR is fixed, you can collaborate with @Winter-Soren and @sukhman-sukh on this one. @guha-rahul : Wish if you could add @Winter-Soren , @LogicalGuy77 and @sukhman-sukh as collaborators on this PR. This PR was very well done indeed, and was also ready for final review + merge. However, with important changes made in relay directory, we might have to revisit the PR once again. Also, there are some merge conflicts in this PR, which now need to be addressed. Once the merge conflicts are resolved, I'll re-run the CI/CD pipeline too. |
|
@LogicalGuy77 and @Winter-Soren : Hi Harshit. Wish if you could resolve the CI/CD issues and merge conflicts in this PR. Once this is merged, I will discuss with @codemaestro64, @pacrob and re-test his PR, in parallel. |
|
@LogicalGuy77 : Hi Harshit. Wish to share that CI/CD issues are still pending in this PR. Kindly collaborate with @Winter-Soren, @sumanjeet0012, @sukhman-sukh and arrive at a good conclusion on this PR. Wish to share that we don't need to open another PR. We can do it here itself in the same branch as @guha-rahul. @guha-rahul : Could you please add @LogicalGuy77 as a collaborator to this PR, in case he has not been added. |
|
Hi @guha-rahul, |
|
@Winter-Soren Fixed those but also added you |
|
@guha-rahul , @Winter-Soren and @LogicalGuy77 : Merge conflicts have been resolved. However, CI/CD issues need to be fixed. Re-running the CI/CD pipeline once again after updating the branch. |
6 participants
What was wrong?
Issue #697
How was it fixed?
Summary of approach.
To-Do
Cute Animal Picture