Skip to content

Conversation

@timwu20
Copy link
Contributor

@timwu20 timwu20 commented Sep 9, 2025

@timwu20 timwu20 changed the title feat: support WebRTC transport option for libp2p client/server implementations feat: support WebRTC transport for libp2p client and server Sep 9, 2025
@lexnv lexnv self-requested a review September 16, 2025 10:05
Copy link
Owner

@lexnv lexnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job here @timwu20! 🙏

Is the WebRTC working as expected for:

  • litep2p <-> litep2p
  • libp2p <-> litep2p?

@timwu20
Copy link
Contributor Author

timwu20 commented Sep 17, 2025

Nice job here @timwu20! 🙏

Is the WebRTC working as expected for:

  • litep2p <-> litep2p
  • libp2p <-> litep2p?

Well since litep2p can't dial via WebRTC we won't be able to test that unless we decide to implement it. I'm getting decode errors on the litep2p/str0m side when running the libp2p client with the litep2p server. After making changes locally to str0m, I'm now getting a panic from the litep2p side after the ICE handshake. Still debugging and will likely make changes to litep2p and str0m after I get it working locally.

@lexnv
Copy link
Owner

lexnv commented Sep 17, 2025

That sounds like great progress! I'm expecting indeed some subtle incompatibility to surface after updating the str0m crate

libp2p-tls = "0.5.0"
libp2p-noise = "0.45.0"
libp2p-yamux = "0.46.0"
libp2p = { git = "https://github.com/timwu20/rust-libp2p", rev = "8bdee16f4dc345eb2a4d63246bb8739f16353cc3", features = ["dns", "identify", "kad", "macros", "mdns", "noise", "ping", "tcp", "tokio", "yamux", "websocket", "request-response"] }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dq: Do we need to introduce any changes to the rust-libp2p crates? Is something not working there? 🤔

I would ask the maintainers for some help with releasing if that's the case 🙏

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need rust-libp2p to use webrtc.rs version 0.13.0 or later. Otherwise the libp2p dialer crashes during connection establishment with this error:

2025-11-12T04:52:01.683603Z  INFO webrtc::peer_connection: ICE connection state changed: connected

thread 'tokio-runtime-worker' (2592172) panicked at /Users/haiko/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/generic-array-0.14.7/src/lib.rs:572:9:
assertion `left == right` failed
  left: 32
 right: 16
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Tim opened libp2p/rust-libp2p#6186 for this but then had to shift focus to something else and the PR wasn't merged due to CI failures. I've taken over all WebRTC related work from Tim and will try to move this forward (or more likely open a new PR since webrtc.rs 0.14.0 has been released in the meantime.)

I'm also currently working on adding WebRTC tests to run_bandwidth.sh.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like great progress! Thanks for tackling this 🙏

futures = "0.3.28"

litep2p = { version = "0.10.0", features = ["websocket", "webrtc"] }
litep2p = { git = "https://github.com/timwu20/litep2p", rev = "9fb09abd77f88450f673c40e51b57aa0c7f4d358", features = ["websocket", "webrtc"] }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a new litep2p including the changes from:

Or is this more of a work-in-progress testing harness? 🤔

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need at least the str0m upgrade from that fork. I'm also trying to move paritytech/litep2p#441 forward but it seems a bit stuck atm.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get libp2p/rust-libp2p#6186 merged and released. The changes look good to me, and we can build upon them in multiple PRs.

Once we have rust-libp2p released and working without panics/issues with litep2p via:

We can bake a new litep2p release and focus on any surfacing issues/smoldot incompatibilities 🙏

@haikoschol
Copy link

@lexnv I've made the following changes to the PR:

Switched to ChainSafe forks of libp2p & litep2p for WebRTC dependencies

The litep2p one is temporary until paritytech/litep2p#465 is merged and a new release is cut. For the libp2p one we agreed to deprioritize getting a release with webrtc.rs 0.13 and that it is acceptable to point to a fork for now.

Set the keep alive timeout in the litep2p WebRTC listener to 5 minutes

I've seen logs saying that this timeout was hit and the connection downgraded. I think there is some interaction with this change I made in my litep2p PR causing the connection to be closed instead of just marked inactive, but I might be wrong. In any case, that change is a temporary one, before we tackle gracefully closing substreams with the flags in the protobuf frame.

Updated run_bandwitdh.sh to include WebRTC runs

I've added libp2p -> libp2p and libp2p -> litep2p and extended the result table. Unfortunately, even after limiting the amount of data being transferred, I've rarely seen the whole script finish without getting stuck. It took many attempts to get the table output.
I guess whatever issues are occurring during script execution are the point of adding WebRTC to this repository; We want to use it to improve reliability and potentially performance. Although I have even seen the script getting stuck during TCP runs. 🤷

Updated Bandwidth Report in README

@lexnv
Copy link
Owner

lexnv commented Nov 18, 2025

Oki doki that sounds about right!

Could you open a few issues in litep2p around those occurrences 🤔 ? I think we'd need to take a closer look in the future as it might affect the connection stability.

While at it, I remember you mentioning a few issues with the current webrtc:

  • memory buffer needs to be set to a lower amount
  • some performance degradation on webrtc substreams
  • tackling the FIN packet as opposed to dropping the substreams

Could you open up a few issues for those as well? So we can link to them in the Webrtc milestone in litep2p 🙏

Once we get the WebRTC fully compatible and stable with smoldot, we should pull the changes via a local litep2p branch into substrate (or release if we get any other pending fixes in the meanwhile), and double-check smoldot can connect to the running substrate node 🙏

@lexnv
Copy link
Owner

lexnv commented Nov 18, 2025

@haikoschol I can see from the rust-libp2p branch that we are not running with the latest webrtc crate, is there something wrong there? In other words, the litep2p implementation should function only with 0.13 as opposed to 0.14? 🤔

@haikoschol
Copy link

@haikoschol I can see from the rust-libp2p branch that we are not running with the latest webrtc crate, is there something wrong there? In other words, the litep2p implementation should function only with 0.13 as opposed to 0.14? 🤔

Yes, I briefly tried 0.14, but with that I couldn't even connect a libp2p dialer to a libp2p listener. I didn't debug this further since I was focused on getting the script to run to completion. I did mean to mention it in my summary comment, but apparently forgot. I think it's some interaction with transitive dependencies, maybe related to DTLS. There's a lot of 0.x stuff being pulled in. :-/ Will look into it again.

@haikoschol
Copy link

Could you open a few issues in litep2p around those occurrences 🤔 ? I think we'd need to take a closer look in the future as it might affect the connection stability.

You mean related to the keep-alive timeout? I'm not sure yet what exactly needs to be done, if anything. It is probably covered by the performance related issue I've linked to below. If I get a better idea of it I'll create another issue if necessary.

While at it, I remember you mentioning a few issues with the current webrtc:

  • memory buffer needs to be set to a lower amount

I'm not sure what you are referring to. Maybe this change related to calling set_buffered_amount_low_threshold()? I'm not entirely sure if and for what we need it, but most likely for logic related to backpressure.

  • some performance degradation on webrtc substreams

opened paritytech/litep2p#477

  • tackling the FIN packet as opposed to dropping the substreams

opened paritytech/litep2p#476

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants