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

feat(webtransport): add WebTransport for WASM environments #4015

Merged
merged 24 commits into from
Jun 23, 2023

Conversation

oblique
Copy link
Contributor

@oblique oblique commented May 31, 2023

Description

This PR implements Transport for WebTransport for browsers by using web-sys.

Related: #3846.
Resolves: #3825.

Notes & open questions

This PR depends on #3991 and on latest master branch of multiaddr

WASM in browsers is single threaded and multi-threading is still work in progress with a lot of unknowns. Currently wasm_bindgen_futures crate implements only spawn_local, which is what is used by libp2p_swarm::executor::WasmBindgenExecutor. However because I needed to satisfy Send, I used send_wrapper crate.

If in the future multithreading is introduced in browser's WASM, then this code might need refactoring.

I was thinking to also move the current code of web-ext/src in web-ext/src/websockets, but I didn't do it.

Documentation will be added soon, but you can review the code.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@oblique
Copy link
Contributor Author

oblique commented May 31, 2023

This includes the commits of #3991 and I will rebase after it gets merged

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thank you @oblique for the high quality contribution. I am looking forward to landing this.

Overall this looks good to me. I still need some more time for another review.

transports/wasm-ext/src/webtransport/transport.rs Outdated Show resolved Hide resolved
Comment on lines 39 to 43
impl Transport {
pub fn boxed(self) -> Boxed<(PeerId, StreamMuxerBox)> {
self.map(|(peer_id, muxer), _| (peer_id, StreamMuxerBox::new(muxer)))
.boxed()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Linking related discussion. No actions required from my end.

#3313

transports/wasm-ext/src/webtransport/substream.rs Outdated Show resolved Hide resolved
transports/wasm-ext/src/webtransport/transport.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

I can only 2nd @mxinden ! Very high quality, thanks a lot for this! Super excited to land this.

Left a few minor comments :)

I am also curious about your usecase and perhaps you want to add yourself to https://github.com/libp2p/rust-libp2p#notable-users.

transports/wasm-ext/src/lib.rs Outdated Show resolved Hide resolved
transports/wasm-ext/src/lib.rs Outdated Show resolved Hide resolved
transports/wasm-ext/src/webtransport.rs Outdated Show resolved Hide resolved
transports/wasm-ext/src/lib.rs Outdated Show resolved Hide resolved
transports/wasm-ext/src/webtransport/cached_js_promise.rs Outdated Show resolved Hide resolved
transports/wasm-ext/src/webtransport/error.rs Outdated Show resolved Hide resolved
transports/wasm-ext/src/webtransport/error.rs Outdated Show resolved Hide resolved
transports/wasm-ext/src/webtransport/transport.rs Outdated Show resolved Hide resolved
transports/wasm-ext/src/webtransport/substream.rs Outdated Show resolved Hide resolved
transports/wasm-ext/src/webtransport/connection.rs Outdated Show resolved Hide resolved
@oblique oblique force-pushed the webtransport-wasm-ext branch 3 times, most recently from 928731b to a00f118 Compare June 1, 2023 15:16
@oblique oblique changed the title feat(wasm-ext): Add webtransport::Transport feat(transport): Add WebTransport for WASM environments Jun 1, 2023
@oblique oblique force-pushed the webtransport-wasm-ext branch 5 times, most recently from 5346e01 to ff3761d Compare June 2, 2023 14:53
@oblique oblique force-pushed the webtransport-wasm-ext branch 4 times, most recently from 1932e08 to 231c032 Compare June 3, 2023 18:35
Cargo.toml Outdated Show resolved Hide resolved
@oblique oblique force-pushed the webtransport-wasm-ext branch 3 times, most recently from 3f31979 to c9c9f6c Compare June 4, 2023 20:48
transports/webtransport-websys/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines +73 to +95
// TODO: This should be part libp2p-noise
if let Some(expected_peer_id) = remote_peer {
Copy link
Member

Choose a reason for hiding this comment

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

Fine by me. Though I suggest doing so in a separate pull request. Also note that we do not always know the remote's peer id in advance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Related: #2946

To actually make use of this in our upgrade stack, some more changes are required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thomaseizinger I was actually thinking to do the following:

  • Add libp2p_noise::Config::with_remote_peer_id
  • The above will pass the value at libp2p_noise::io::handshake::State::expected_remote_peer_id
  • In libp2p_noise::io::handshake::State::finish we will verify if id_pk.to_peer_id() is the same with State::expected_remote_peer_id

After this change I would be able to do some like this in the Connection::authenticate:

        let mut noise = libp2p_noise::Config::new(keypair)?;

        if !certhashes.is_empty() {
            noise = noise.with_webtransport_certhashes(certhashes);
        }

        if let Some(expected_peer_id) = remote_peer {
            noise = noise.with_remote_peer_id(expected_peer_id);
        }

What do you think? I can do it in separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes sounds good to me!

I only linked #2946 here because it would be great if we can take advantage of this built-in verification for all connection upgrades and not just here.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think? I can do it in separate PR

Preference for separate pull request.

.cargo/config.toml Outdated Show resolved Hide resolved
transports/webtransport-websys/src/stream.rs Outdated Show resolved Hide resolved
transports/webtransport-websys/src/utils.rs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
transports/webtransport-websys/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +73 to +95
// TODO: This should be part libp2p-noise
if let Some(expected_peer_id) = remote_peer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Related: #2946

To actually make use of this in our upgrade stack, some more changes are required.

@oblique oblique force-pushed the webtransport-wasm-ext branch 3 times, most recently from 70c2268 to 980a12b Compare June 6, 2023 08:25
@mxinden
Copy link
Member

mxinden commented Jun 6, 2023

@oblique on Slack you were asking how to best test this pull request. @jxs suggested the following:

This would test interoperability between a rust-libp2p WASM WebTransport node (this pull request) against any other libp2p based WebTransport server implementation on every pull request.

I know this looks rather complex at first. Please either comment here, ping me on Slack, Matrix or e-mail, or let's schedule a quick call sometime.

What do you think?

@oblique
Copy link
Contributor Author

oblique commented Jun 6, 2023

@mxinden This looks complex indeed and I'm not sure how much time I can devote for it right now. I am not familiar with JavaScript ecosystem which is something that is needed for the Rust WASM case.

I will spend some time to figure some stuff out, but I can not guarantee that I will be able to deliver the interop tests.

In case of the Go echo-server, I was thinking to do much simpler tests and closer to the Transport layer than the Swarm layer.

@mergify mergify bot dismissed mxinden’s stale review June 23, 2023 05:27

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@mergify mergify bot merged commit 85a846a into libp2p:master Jun 23, 2023
66 checks passed
@oblique
Copy link
Contributor Author

oblique commented Jun 23, 2023

Thanks a lot!! 🎉

@zvolin
Copy link
Contributor

zvolin commented Jun 23, 2023

Btw, @thomaseizinger it looks like my contribution isn't counted by github 😢 is it because it's Co-Authored-By instead of Co-authored-by?

@mxinden
Copy link
Member

mxinden commented Jun 23, 2023

@thomaseizinger @oblique @zvolin in case it is of some use, I can release libp2p v0.52.1 and libp2p-webtransport-websys v0.1.0.

@oblique
Copy link
Contributor Author

oblique commented Jun 23, 2023

@thomaseizinger @oblique @zvolin in case it is of some use, I can release libp2p v0.52.1 and libp2p-webtransport-websys v0.1.0.

yes please

@zvolin
Copy link
Contributor

zvolin commented Jun 23, 2023

I guess there are no chances you amend the message so that my contribution is properly attributed? 😄

@zvolin
Copy link
Contributor

zvolin commented Jun 23, 2023

#4104 Here is the fix for a future contribs

@thomaseizinger
Copy link
Contributor

Btw, @thomaseizinger it looks like my contribution isn't counted by github 😢 is it because it's Co-Authored-By instead of Co-authored-by?

I am terribly sorry for this. I am pretty sure, GitHub attributed it correctly for b8ceecc but now that one is also missing.

Maybe GitHub is buggy here? It is kind of insane that the casing matters ..
I'll open an issue.

@thomaseizinger
Copy link
Contributor

I guess there are no chances you amend the message so that my contribution is properly attributed? 😄

I'd rather not but maybe we can get GitHub to fix this and then it should be counted retroactively.

@zvolin
Copy link
Contributor

zvolin commented Jun 23, 2023

I'd rather not but maybe we can get GitHub to fix this and then it should be counted retroactively.

chances are rather poor, but thanks, would appreciate updates how it went out

@mxinden mxinden mentioned this pull request Jun 23, 2023
4 tasks
@thomaseizinger
Copy link
Contributor

I'd rather not but maybe we can get GitHub to fix this and then it should be counted retroactively.

chances are rather poor, but thanks, would appreciate updates how it went out

Just got a reply, apparently the issue are the blank lines but that does not explain why b8ceecc is not correctly attributed. Still waiting for a response to that.

@mxinden
Copy link
Member

mxinden commented Jun 26, 2023

@thomaseizinger @oblique @zvolin in case it is of some use, I can release libp2p v0.52.1 and libp2p-webtransport-websys v0.1.0.

yes please

@oblique @zvolin published ✔️

mxinden added a commit to libp2p/test-plans that referenced this pull request Jul 3, 2023
mxinden added a commit to libp2p/test-plans that referenced this pull request Jul 3, 2023
@zvolin
Copy link
Contributor

zvolin commented Jul 3, 2023

Just got a reply, apparently the issue are the blank lines but that does not explain why b8ceecc is not correctly attributed. Still waiting for a response to that.

btw, any new info from github regarding this @thomaseizinger ? I still have some hope 😄

@thomaseizinger
Copy link
Contributor

Just got a reply, apparently the issue are the blank lines but that does not explain why b8ceecc is not correctly attributed. Still waiting for a response to that.

btw, any new info from github regarding this @thomaseizinger ? I still have some hope 😄

No new information. They referring me to the git interpret-trailers command but running the commit message through that one correctly parses the trailer so I am not sure why the contribution did not work for that one.

Regarding the original commit, they only referred to documentation on how to edit a commit message, which requires force-pushing. Not really a solution in my opinion. Force-pushing master is overall considered an anti-pattern so it is sad that they don't allow for an additional way of specifying co-authorship. After looking into this a bit more, it seems that interpreting trailers is extremely whitespace sensitive which is quite annoying when trying to generate the commit message automatically.

@thomaseizinger
Copy link
Contributor

I've captured all the requirements in #4152.

mxinden added a commit to libp2p/test-plans that referenced this pull request Jul 7, 2023
@thomaseizinger thomaseizinger mentioned this pull request Aug 19, 2023
mergify bot pushed a commit that referenced this pull request May 26, 2024
When you enable Kademlia in a large network that uses multiple type of transports, then this log creates a very noisy console and makes meaningful logs hard to see.

We can also remove the log entirely if you prefer. Check: #4015 (comment), #4072, #4133

Pull-Request: #5396.
TimTinkers pushed a commit to unattended-backpack/rust-libp2p that referenced this pull request Sep 14, 2024
When you enable Kademlia in a large network that uses multiple type of transports, then this log creates a very noisy console and makes meaningful logs hard to see.

We can also remove the log entirely if you prefer. Check: libp2p#4015 (comment), libp2p#4072, libp2p#4133

Pull-Request: libp2p#5396.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wasm-ext: WebTransport
4 participants