-
Notifications
You must be signed in to change notification settings - Fork 993
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
Conversation
This includes the commits of #3991 and I will rebase after it gets merged |
494f482
to
6b854d6
Compare
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.
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.
impl Transport { | ||
pub fn boxed(self) -> Boxed<(PeerId, StreamMuxerBox)> { | ||
self.map(|(peer_id, muxer), _| (peer_id, StreamMuxerBox::new(muxer))) | ||
.boxed() | ||
} | ||
} |
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.
Linking related discussion. No actions required from my end.
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.
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.
928731b
to
a00f118
Compare
5346e01
to
ff3761d
Compare
1932e08
to
231c032
Compare
3f31979
to
c9c9f6c
Compare
// TODO: This should be part libp2p-noise | ||
if let Some(expected_peer_id) = remote_peer { |
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.
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.
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.
Related: #2946
To actually make use of this in our upgrade stack, some more changes are required.
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.
@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 ifid_pk.to_peer_id()
is the same withState::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
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.
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.
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.
What do you think? I can do it in separate PR
Preference for separate pull request.
// TODO: This should be part libp2p-noise | ||
if let Some(expected_peer_id) = remote_peer { |
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.
Related: #2946
To actually make use of this in our upgrade stack, some more changes are required.
70c2268
to
980a12b
Compare
@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? |
@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. |
Approvals have been dismissed because the PR was updated after the send-it
label was applied.
Thanks a lot!! 🎉 |
Btw, @thomaseizinger it looks like my contribution isn't counted by github 😢 is it because it's |
@thomaseizinger @oblique @zvolin in case it is of some use, I can release |
yes please |
I guess there are no chances you amend the message so that my contribution is properly attributed? 😄 |
#4104 Here is the fix for a future contribs |
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'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. |
|
…rt test Introduced in rust-libp2p with libp2p/rust-libp2p#4015.
…rt test Introduced in rust-libp2p with libp2p/rust-libp2p#4015.
btw, any new info from github regarding this @thomaseizinger ? I still have some hope 😄 |
No new information. They referring me to the 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 |
I've captured all the requirements in #4152. |
Introduced in rust-libp2p with libp2p/rust-libp2p#4015.
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.
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.
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 multiaddrWASM 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 bylibp2p_swarm::executor::WasmBindgenExecutor
. However because I needed to satisfySend
, 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
inweb-ext/src/websockets
, but I didn't do it.Documentation will be added soon, but you can review the code.
Change checklist