-
Notifications
You must be signed in to change notification settings - Fork 175
QUIC v1 #763
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
base: main
Are you sure you want to change the base?
QUIC v1 #763
Conversation
Everyone is encouraged to review the PR and suggest changes/improvements/skill-issue etc |
@AkMo3 : Thank you so much for your excellent contribution and for opening the PR on QUIC support — it’s a significant step forward for the As a next step, we’ll be consolidating our previous development efforts and integrating them into your PR. This will ensure all related work is streamlined, reviewed collectively, and built upon consistently. Moreover, your contribution is enabling us to extend transport interoperability with other libp2p modules — an exciting milestone. I've looped in @guha-rahul and @lla-dane to collaborate on this integration and testing effort. We’re also setting up a dedicated I ran the CI/CD pipeline earlier and noticed one of the checks is currently failing. I’ll be sharing a detailed report and suggested fixes on a separate discussion thread so we can troubleshoot collaboratively. In parallel, @lla-dane, @guha-rahul, and I have been actively testing Finally, since this is a major addition to the Thanks once again for your excellent work and for being part of this initiative. We’re excited to keep building this with you :) |
@AkMo3 : I ran the full test suite and noticed that one test case failed — however, it appears unrelated to your QUIC changes. The failing test is: FAILED tests/core/pubsub/test_gossipsub_px_and_backoff.py::test_peer_exchange This is part of the pubsub module and unrelated to transport layers, so we will handle it separately. I’ll open a dedicated discussion to document this and explore the root cause independently. CCing @mystical-prog, @acul71 and @sumanjeet0012 , who could help resolve this issue soon. |
In rust-libp2p, the QUIC multiaddress format is like this: |
Tried to run the ping example with py<>rust sides. I think if no quic-config is provided with if listen_addrs is None:
transport_opt = transport_opt or {}
quic_config: QUICTransportConfig | None = transport_opt.get('quic_config')
if quic_config:
transport = QUICTransport(key_pair.private_key, quic_config)
else:
transport = TCP()
else:
addr = listen_addrs[0]
if addr.__contains__("tcp"):
transport = TCP()
elif addr.__contains__("quic"):
transport_opt = transport_opt or {}
quic_config = transport_opt.get('quic_config', QUICTransportConfig())
transport = QUICTransport(key_pair.private_key, quic_config)
else:
raise ValueError(f"Unknown transport in listen_addrs: {listen_addrs}") and in the default @dataclass
class QUICTransportConfig:
"""Configuration for QUIC transport."""
....
# TLS settings
verify_mode: ssl.VerifyMode = ssl.CERT_NONE
.... I think it means the transport will not run certificate verification. But I ran the py-listner<>rust-dialer on QUIC, and these are the logs: ![]() I seems its failing on certificate verification. What will be the correct way to run ping examples of py<>rust with each other? @AkMo3 |
@AkMo3 : Hi Akash. The CI/CD issue got resolved. Wish to share to please respond to @lla-dane 's feedback on transport interop with rust-libp2p. Kindly also add a newsfragment. We are heading towards a final review + merge stage :) Appreciate your great efforts and initiative. Next, we will be trying transport interop with other libp2p modules and submitting test plans at https://github.com/libp2p/test-plans . We will start with interop with the following libp2p modules: go-libp2p, rust-libp2p, dotnet-libp2p and nim-libp2p. |
signature_payload = b"libp2p-tls-handshake:" + cert_public_key_bytes | ||
|
||
try: | ||
public_key.verify(signature_payload, signature) |
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.
while verification , shouldnt we hash the data before verifying it?
@AkMo3 : Thank you for your continued effort. Please share feedback to the suggestions by @lla-dane and @guha-rahul. @lla-dane and @guha-rahul : Wish if you could work on the test suite with @AkMo3 and help take QUIC to production. CCing @pacrob for his feedback and pointers on the PR. |
Grateful to everyone for all the suggestions and feedback around QUIC. I apologise for not being able to respond to the queries, I am going through a bit of a rough patch around my health and will quickly spin up the wheels as soon I recover. |
@AkMo3 : Hi Akash. Thank you so much for your thoughtful message — and no need to apologize at all! Your work on QUIC has been absolutely wonderful and deeply appreciated by everyone in the community. Please prioritize your health and take all the time you need to recover fully — we’ll be right here when you’re ready to jump back in. Your contributions have already made a big impact, and we're looking forward to collaborating more once you're feeling better. Sending you strength and best wishes for a smooth and speedy recovery. Take care, and thanks again for everything you’ve done so far. |
@AkMo3 , @lla-dane and @guha-rahul : Wish to share that there are some merge conflicts coming up, which need to be resolved. Seems like due to recent merge of PR: #766 (we updated py-multiaddrs). CCing @acul71 and @sumanjeet0012 . |
@AkMo3 take a look to AkMo3#2 (comment) # "multiaddr>=0.0.9",
"multiaddr @ git+https://github.com/multiformats/py-multiaddr.git@db8124e2321f316d3b7d2733c7df11d6ad9c03e6", for py-multiaddr until we can publish to PyPi |
@AkMo3 : Hi Akash. Hope you are doing well. Can you give access to @lla-dane and @guha-rahul so that they can help complete the QUIC PR? Wish to share that there are some merge conflicts that need to be resolved soon. Looking forward to a production version of QUIC in py-libp2p soon. |
Sure, provided access to @lla-dane and @guha-rahul. |
@AkMo3 , @lla-dane , @guha-rahul : Wish if the merge conflicts could be resolved. Was trying to re-run CI/Cd pipeline. |
@AkMo3 : Fantastic progress, Akash. Also, appreciate the support by @lla-dane and @guha-rahul. Great work, friends. Each of these fixes addresses an important piece of the transport layer puzzle: ASN.1 format certificate extension: great fix — making sure we’re using the correct format here is critical for interoperability and for keeping peer identity verification standards-compliant. Peer ID derivation on dial: skipping this when necessary, ensures we don’t run into false negatives or block valid connections while still keeping the verification step intact where it matters. QUIC multiaddr test fixes: tests are always a good indicator of correctness, and fixing the maddr parsing/handling is essential for ensuring real-world dialing works smoothly. Allowing accept stream to wait indefinitely: this is a thoughtful change — it improves stability by making sure we don’t cut off valid streams just because of arbitrary timeouts. Stream opening and connection parameter changes: great to see these adjustments — setting parameters correctly up front will prevent a lot of subtle bugs down the line, especially as QUIC is sensitive to handshake and stream state management. Bringing all of this together, the module is much closer to a production-ready state. The flow of commits shows a good balance of fixing edge cases, improving test coverage, and refining core functionality. Once checks finish running and the branch is rebased cleanly, this will be in excellent shape for deeper review and merge. Wish to also thank @lla-dane and @guha-rahul for their wonderful initiatives too. Great progress everyone :) |
@AkMo3 , @lla-dane and @guha-rahul: Thank you for making improvements and testing QUIC PR. Re-ran the CI/CD pipeline. We do have some new merge issues that need to be resolved. |
libp2p/__init__.py
Outdated
else: | ||
addr = listen_addrs[0] | ||
is_quic = addr.__contains__("quic") or addr.__contains__("quic-v1") |
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.
We have the is_quic_multiaddr
function in utils. Could we use that here instead?
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.
Yup
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.
Yup
libp2p/transport/quic/connection.py
Outdated
self._remote_peer_id = remote_peer_id | ||
self._local_peer_id = local_peer_id | ||
self.peer_id = remote_peer_id or local_peer_id | ||
self.__is_initiator = is_initiator |
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 the double underscore 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.
True, there is no requirement of double underscore any longer. Refractoring.
1. Do not dial to open a new stream, use existing swarm connection in quic transport to open new stream 2. Derive values from quic config for quic stream configuration 3. Set quic-v1 config only if enabled
a335595
to
89cb8c0
Compare
Merge conflicts are fixed @seetadev |
@seetadev Thanks a lot for the constant help through the PR. I believe we are at juncture were we can start performing the final review. @pacrob Looking forward towards more of your reviews! @lla-dane @guha-rahul While the review are going, we can parallely analyse the current code and branch coverage of tests for quic module that we can later expand to entire repo. Let me know your thoughts on this. |
@AkMo3 : Thanks a lot for driving this forward and for the consistent effort you’ve put into shaping the QUIC implementation. 🙌 Great to see the PR reaching the stage where we can begin the final review cycle. I’ll make sure to go through it thoroughly and share my comments soon. Lets now try dialing to rust-libp2p and/or go-libp2p, while the final review process is on. We'll have a separate tracking issue on transport (quic) interop efforts with other libp2p modules. @acul71 (Luca) will help us here. Also, I agree with your suggestion to start looking at branch and code coverage in parallel. Getting clarity on the current test coverage for the QUIC module — and then expanding that discipline repo-wide — will give us both higher confidence in stability and a stronger foundation for future contributions. @pacrob: Thank you so much for your reviews and feedback. Appreciate it. This PR is indeed ready for final review + merge :) Looking forward to merging it after careful review of the code and testing for next 4-5 days. @lla-dane @guha-rahul, your analysis of test coverage can really help us close the loop cleanly. Also, CCing other key members of libp2p community for their thoughts and feedback at the QUIC discussion page. This feels like a key milestone for py-libp2p, and I’m excited to see it landing this week 🚀 |
What was wrong?
This PR aims to bring QUIC transport support
How was it fixed?
Summary of approach:
What's Working
What remaining
To-Do
Cute Animal Picture