-
Notifications
You must be signed in to change notification settings - Fork 176
QUIC integration using aioquic #554
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
Conversation
@Khwahish29 : Thank you for sharing the pull request. Great work. Wish to share that I re-ran the CI/CD pipeline. All the tests are passing except 1. The error is occurring because Sphinx is treating warnings as errors (-W flag is used in the build command), and there's one specific warning causing the build to fail: could you please check. Seems like we have to add the quic related file to a toctree. Looking forward to testing the PR. |
@Khwahish29 @seetadev It looks like the problem was that the |
@pacrob : Thank you so much, Paul. Appreciate your great support on resolving the issue. The docs build environment was indeed not picking up aioquic because it was not getting installed as expected during the tox run. Thank you for updating tox.ini to ensure it explicitly installs all dependencies. Appreciate it. We will add it as a note for other modules in active development too. The docs are building cleanly, and that all the relevant modules are being pulled in as expected. Appreciate your help. @Khwahish29 : Thank you for the update. I noticed that one test failed: test_readding_after_expiry in tests/core/tools/timed_cache/test_timed_cache.py. Everything else passed (239 tests ✅), so this looks isolated. @pacrob , @acul71 : Wish if you could share pointers, feedback to help us arrive at a good conclusion on this issue. This isolated error seems to be coming up recently with other PRs too. Given that it's in the timed cache logic, there's a chance we're seeing flakiness due to tight timing windows or test environment variability. I’ll review the test and cache implementation to see if there's a deterministic fix or if we need to relax the assertion slightly to account for timing drift. There was a pull request for the same: #562 There was a change introduced lately via this PR: #518 @mystical-prog (Vraj): Thank you for working on adding support for TimedCache module. Since, you worked on #518, any pointers or thoughts to help us resolve it. Copying/pasting the error we are getting on running CI/CD pipeline. FAILED tests/core/tools/timed_cache/test_timed_cache.py::test_readding_after_expiry On the same note, there are 3 different approaches to building QUIC. This is a Python native implementation, which will enable connection management. Working on using napi rust library and msquic to enable interop with other libp2p modules. |
@Khwahish29 : Great work, Khwahish. Appreciate your wonderful efforts and initiative. @pacrob, @dhuseby : Thank you so much for your feedback, pointers and great support on enabling us to arrive at a good milestone achievement. Appreciate it. Please let us know if you would like us to make any final improvements or changes. Happy to collaborate with @Khwahish29 and work on them at the earliest. Thank you once again :) |
Thanks @seetadev I appreciate your kind words! 😊 This PR has been quite an involved one, and I'm glad to see it come together smoothly in the end. That said, I’m happy to keep refining things further if there are specific suggestions. Always open to constructive feedback! 🚀 |
@Khwahish29 : As we start integrating more and running it through its paces, transport interop is going to be a big focus area — not just for robustness within Also worth keeping an eye on related efforts that might inform future refinements:
Let’s keep building on this — amazing progress already, and really excited for what this unlocks. 🚀 |
Thank you so much @seetadev for the incredibly kind words and continuous encouragement throughout this journey! 🙏 The pointers around |
=0.9.20, base58
Outdated
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 looks like these files are created in error.
@pacrob : Thank you so much for your guidance, support and feedback. Appreciate it. Wish to share that QUIC is ready for review. Kindly let us know if any changes are needed. |
@pacrob : Thank you so much for your continued support and feedback. Appreciate it. Wish to share some more context regarding the PR contributed by @Khwahish29. The current approach in the PR is a native Python implementation of QUIC, built using the aioquic library. Wish to share that this is intended as a basic MVP to get things moving, and it does not provide interoperability with all of the other libp2p modules at this stage. I have shared with @Khwahish29 that we will need to move towards interoperability testing and development with the broader libp2p ecosystem as the next step. @Khwahish29 , wish to share the link for interoperability testing framework and plans: The goal is to validate if the native Python QUIC implementation can successfully interoperate with libp2p nodes written in other languages (e.g., Go, Rust, JavaScript). If interoperability succeeds, this would give us a fully native Python approach ready for production. Reza Ameli (rameli), who joined the community call, yesterday will also be participating in this project. I have also shared some notes for the next phase of the project: please visit #578 Finally, wish to share that we will not close this issue yet — it will stay open to allow us to continue building and refining the work as we proceed through interoperability and native integration stages. |
self.transport = transport | ||
self.handler_function = handler_function | ||
|
||
async def listen(self, maddr: Multiaddr, nursery: Nursery) -> bool: |
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.
The nursery
arg is unused here.
libp2p/transport/quic/transport.py
Outdated
"""Start listening for QUIC connections.""" | ||
try: | ||
# Create a UDP endpoint | ||
loop = asyncio.get_event_loop() |
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're aiming to use trio
in place of asyncio
. Is there a reason we need asyncio
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.
None of these are being run. We use trio
instead of asyncio
, so you'll want to mark the tests here and in test_interop.py
with @pytest.mark.trio
.
Off to a great start! My main concern is the use of Also take a look at the test folder configuration. We have tests divided into core, interop, and utils, so you could put The interop tests are currently turned off, so we'll need to turn those back on for yours to run. I'll take a look and get them organized so you can move your interop tests in there. |
Thanks for the review, @pacrob ! I’ll make sure to move the tests to the correct directories as you suggested, I’ll also ensure that the test functions are marked with I'm on it and will make sure everything is cleaned up and organized. Appreciate the thorough feedback! |
OK, @Khwahish29, I've just merged the update to interop test format. We dividing interop tests up by libp2p implementation. |
@pacrob : Thank you so much for your continued support. Appreciate it. @Khwahish29 : We can keep the transport interop tests specific to go-libp2p or any other key libp2p module you are comfortable with as a first pass. Great contribution. |
Great @seetadev |
Hey @Khwahish29 @seetadev @pacrob, I would like to contribute to this PR. Would be cool to learn more about QUIC under the hood. I have a few question I have pointed out in the inline comments and here: I read a little about asyncio aioquic examples and docs here: It seems they have a separate client and server api for quic connection. So I am guessing these two functions would do same kind of thing maybe: but there is no function for server setup like this: https://aioquic.readthedocs.io/en/latest/_modules/aioquic/asyncio/server.html#serve which is used in the aioquic asyncio example that I sent before for setting up listener with quic configs; in these docs which does not seems to use asyncio: https://aioquic.readthedocs.io/en/latest/quic.html. So I was a little bit confused about how the udp socket connection using |
I am glad you did so much research on this, as you already know in libp2p there's no dedicated server or client logic as it's a peer to peer implementation where each node acts as a server and as a client simultaneously - also like I already mentioned we use Trio sockets for handling UDP connection and use aioquic for upgrading that socket to follow QUIC protocol implementation. I'd suggest please go through what was shared on discord by @seetadev and me which could be great starting point to dive deep. |
@Khwahish29 : Hi Khwahish. Wish to take a moment to sincerely thank you for all the thoughtful work and time you’ve invested into the QUIC implementation in As part of the ongoing development, @AkMo3 has opened a promising PR that builds on the QUIC stack and brings it closer to interoperability with other libp2p modules. We’re currently consolidating all prior efforts related to QUIC so that we can test and stabilize the implementation across various modules. In this spirit, it would be wonderful if you could help us by merging or migrating your earlier changes into Akash’s open PR. Your contributions — whether in code, design decisions, or test scaffolding — will continue to play a critical role as we finalize this transport module. The active PR can be found here: please visit #763 If you have any blockers or need support in rebasing or porting code, we’re more than happy to coordinate. We’d also love to have your continued feedback as we proceed to testing, especially with regards to |
Hi @seetadev , thank you so much for the update. I really appreciate the update and the clarity on how the QUIC implementation is progressing, it’s great to see the efforts converging towards interoperability. I’ll review Akash’s PR (#763) shortly and start working on migrating the relevant pieces from my previous contributions. It’s encouraging to see the consolidation happening, and I’d be glad to support the process by porting over any code, tests, or edge case handling that can help stabilize the transport. If I run into any issues with rebasing or compatibility, I’ll reach out. Looking forward to collaborating more closely on this and contributing to the finalization of the QUIC stack in py-libp2p! Thanks again, and kudos to the whole team for pushing this forward 🚀 |
@Khwahish29 : Thank you so much for your thoughtful and encouraging message. We appreciate your proactive approach and your willingness to jump back in to support the QUIC effort—especially with porting over relevant parts from your earlier contributions. Your work has already laid a solid foundation, and having your continued involvement during this phase of consolidation is incredibly valuable. Akash’s PR (#763) brings us much closer to interoperability, and with your help reviewing and aligning previous work, we’re confident we can make the QUIC transport in Please don’t hesitate to reach out if anything comes up during the rebase or integration process—happy to help troubleshoot or coordinate on any blockers. Looking forward to continued collaboration as we push this over the finish line. Thanks again for the energy and support—it really means a lot to the whole team! 🚀 |
We are doing a final review + merge in QUIC's PR: please visit #763. |
What was wrog
WIP PR for QUIC integration using aioquic
Issue #
How was it fixed?
Summary of approach.
To-Do
Cute Animal Picture