Skip to content
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

add WebRTC transport #1999

Closed
wants to merge 250 commits into from
Closed

add WebRTC transport #1999

wants to merge 250 commits into from

Conversation

GlenDC
Copy link

@GlenDC GlenDC commented Jan 17, 2023

Successor of original PR: #1655 (hand over of developer)

This PR implements the WebRTC transport spec according to https://github.com/libp2p/specs/tree/cfcf0230b2f5f11ed6dd060f97305faa973abed2/webrtc.

Benchmarks

Benchmarks of how the WebRTC transport compares in terms of resource usage and continuous R/W throughput can be found at https://github.com/libp2p/libp2p-go-webrtc-benchmarks#2-benchmarks (submitted in function of this PR)

Comparison Quic Vs WebRTC transport:

image
image

Interoperability

Compatibility of go-libp2p's WebRTC transport with the Rust and JS implementations are to be validated using the libp2p-testplans:

WebRTC Interoperability Matrix

libp2p-go (listener) libp2p-rs (listener) libp2p-js (listener)
libp2p-go (dialer) /
libp2p-rs (dialer) /
libp2p-js (dialer) /

Note that noise is used for the encryption and yamux for the multiplexer, and this for all of the tests above.

How to run the WebRTC interoperability tests

Executables used for interop tests below:

First make sure to have a redis server running locally, as this is what used in the new interop tests, e.g. you can do this using the command:

docker run --rm -it -p 6379:6379 redis/redis-stack

Wit that in mind there are following (dial > listen) cases to test:

  1. libp2p-go > libp2p-go ✅
# > Client (Dial) @ <root>/multidim-interop/go/v0.24
$ REDIS_ADDR=localhost:6379 ip="0.0.0.0" transport=webrtc security=noise muxer=yamux is_dialer="true" go run ./main.go
# ...
Ping successful:  291.594µs

# > Server (Listen) @ <root>/multidim-interop/go/v0.24
$ REDIS_ADDR=localhost:6379 ip="0.0.0.0" transport=webrtc security=noise muxer=yamux is_dialer="false" go run ./main.go
  1. libp2p-js > libp2p-go ✅
# > Client (Dial) @ <root>/multidim-interop/js/v0.41
$ REDIS_ADDR=localhost:6379 ip="0.0.0.0" transport=webrtc security=noise muxer=yamux is_dialer="true" npm run test -- -t browser
startPing successful: 176

# > Server (Listen) @ <root>/multidim-interop/go/v0.24
$ REDIS_ADDR=localhost:6379 ip="0.0.0.0" transport=webrtc security=noise muxer=yamux is_dialer="false" go run ./main.go
  1. libp2p-rs > libp2p-go ✅
# > Client (Dial) @ <root>/multidim-interop/rust
$ REDIS_ADDR=localhost:6379 ip="0.0.0.0" transport=webrtc security=noise muxer=yamux is_dialer="true" cargo run --bin testplan_0500
Ping successful: 1.14172ms

# > Server (Listen) @ <root>/multidim-interop/go/v0.24
$ REDIS_ADDR=localhost:6379 ip="0.0.0.0" transport=webrtc security=noise muxer=yamux is_dialer="false" go run ./main.go
  1. libp2p-go > libp2p-rs ✅
# > Client (Dial) @ <root>/multidim-interop/go/v0.24
$ REDIS_ADDR=localhost:6379 ip="0.0.0.0" transport=webrtc security=noise muxer=yamux is_dialer="true" go run ./main.go
# ...
Ping successful:  1.710328ms

# > Server (Listen) @ <root>/multidim-interop/rust
$ REDIS_ADDR=localhost:6379 ip="0.0.0.0" transport=webrtc security=noise muxer=yamux is_dialer="false" cargo run --bin testplan_0500

PR-Ready Checklist

Checklist to be completed before we can promote it from Draft to a PR ready for review:

  • make sure all skipped tests pass, such that we can run go test -v ./...
    • make sure TestWebrtcTransport can be ran (might require contribution to Pion)
      • TestWebrtcTransport is disabled on purpose, see comments in code on why this is
  • make sure all CI tests pass
  • integrate benchmarking tool that can be used for profiling and load testing: https://github.com/little-bear-labs/libp2p-webrtc-bench
    • update code to make use of standard tooling only for metrics tracking
  • document and use load testing
    • document how to use golang profiling to optimise
    • document how to track metrics
    • document how to get summary of metrics using go tool
    • document how to get more detailed view of metrics using python script
  • add benchmarks from AWS testing (and include parameters used)
    • add documentation on how we got these benchmarks and add summary of results
  • document and use example (load testing / benchmarking code is a good enough example)
    • Link to this benchmarking tool from WebRTC module
  • iterate on performance of this draft implementation
  • clean up the code base
  • ensure tests pass consistently in CI
  • demonstrate interoperability tests with rust-libp2p and js-libp2p-webrtc pass before merging

@BigLep
Copy link
Contributor

BigLep commented Mar 31, 2023

@ddimaria : It sounds like this was covered in LBL sync but my understanding of the done criteria is:

  1. Test pass
  2. Code follows conventions of the rest of the library
  3. Code is to similar quality bar of the rest of the library
    (2 and 3 are certainly a bit subjective - dependent on maintainer judgment)

I don't think the goalposts should be fluctuating from there. Let me know if there is other major areas of variability.

@ddimaria
Copy link

@BigLep Thanks for following up! This general criteria has been my understanding as well, though the subjectivity has been a blocker in being predictive in any meaningful way. A more ideal scenario for nailing down time estimates would be to have a concrete list of items remaining, though it's OK to progress as we have been knowing that we can't make accurate estimates of "done".

@MarcoPolo
Copy link
Collaborator

@GlenDC can you please allow edits from maintainers? I cannot push to this branch https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork.

Trying to update the multiaddr dep to use the correct multiaddr code.

@GlenDC
Copy link
Author

GlenDC commented Apr 4, 2023

@GlenDC can you please allow edits from maintainers? I cannot push to this branch https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork.

Trying to update the multiaddr dep to use the correct multiaddr code.

As far as I can see that check is enabled, so not sure why you can’t access it…

To be sure @MarcoPolo I've also added you as a collaborator to my fork (you still need to accept it). But AFAIK you shouldn't need this. Only thing that might be is that you are not a maintainer of this Repo, but I guess from your reaction that you are, so in that case, no clue... Either way with the collaborator rights to my fork it should also work.

@Jorropo Jorropo mentioned this pull request May 22, 2023
6 tasks
@marten-seemann
Copy link
Contributor

I'm taking over this PR.

@p-shahi
Copy link
Member

p-shahi commented Jun 6, 2023

closed in favor of #2337

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

webrtc: public/browser-to-public/server webrtc: investigate resource usage
7 participants