Skip to content

webrtc: update str0m, fix multistream message decoding and encoding #441

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.

Sign up for GitHub

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

Closed
timwu20 wants to merge 4 commits into from
Closed

webrtc: update str0m, fix multistream message decoding and encoding #441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a64e64c
update strom dependency which fixes disable remote fingerprint verifi…
timwu20 Oct 7, 2025
a855ce5
change str0m to 0.11.1
timwu20 Oct 20, 2025
c97bc5d
fix lint
timwu20 Oct 21, 2025
8cf27e9
set and listen on ChannelBufferedAmountLow after stream closure
timwu20 Oct 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 15 additions & 19 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ rcgen = { version = "0.14.5", optional = true }
# End of Quic related dependencies.

# WebRTC related dependencies. WebRTC is an experimental feature flag. The dependencies must be updated.
str0m = { version = "0.9.0", optional = true }
str0m = { version = "0.11.1", optional = true }
# End of WebRTC related dependencies.

# Fuzzing related dependencies.
Expand Down
26 changes: 22 additions & 4 deletions src/multistream_select/protocol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,10 @@ impl Message {

// Skip ahead to the next protocol.
remaining = &tail[len..];
if remaining.is_empty() {
// During negotiation the remote may not append a trailing newline.
break;
Copy link
Collaborator

@lexnv lexnv Oct 7, 2025

Choose a reason for hiding this comment

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

Was this observed with smoldot integration?

Copy link
Contributor Author

@timwu20 timwu20 Oct 7, 2025

Choose a reason for hiding this comment

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

This was observed when testing against the libp2p native WebRTC transport. During negotiation, the libp2p native WebRTC transport doesn't append an extra newline.

Copy link
Contributor

@haikoschol haikoschol Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The problem is that Message::decode() conflates ls responses with protocol requests and confirmation messages. As I mentioned in another comment, there are small differences.

ls response:

<total length><header length>/multistream/1.0.0\n
<proto1 length>/foo/bar/proto1\n
<proto2 length>/foo/bar/proto2\n
\n

protocol request with fallback protocols:

<header length>/multistream/1.0.0\n
<proto1 length>/foo/bar/proto1\n
<proto2 length>/foo/bar/proto2\n

protocol confirmation:

<header length>/multistream/1.0.0\n
<proto1 length>/foo/bar/proto1\n

When using webrtc transport, there is another length prefix for the protobuf frame. But that is independent of whether the payload is a multistream-select message or not.

edit: @timwu20 pointed me towards #75 which in turn links to this spec for multistream-select that specifies a back-and-forth process for providing fallback protocols. I've been working off this spec, which glosses over this. So I have to correct my example above: The "protocol request with fallback protocols" does not exist in this form, which means that Message::decode() does not need to support it. :)

Copy link
Collaborator

@lexnv lexnv Nov 6, 2025

Choose a reason for hiding this comment

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

I would not add this code due to possible incompatibilities for current transports.
I see rust-libp2p is using a similar code path to what we did before:

Instead, we should get in touch with smoldot to adjust any multistream-select inconsistencies (or impl ls similarly to litep2p/libp2p 🙏 )

Copy link
Contributor

@haikoschol haikoschol Nov 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

According to Tim's comment, he added this when testing webrtc transport between litep2p and rust-libp2p.

I just tested litep2p <-> smoldot without this change and it doesn't work either. The reason why rust-libp2p <-> smoldot works without this change is that MessageIO yields one Message at a time, even if the remote sent both header and protocol name in a single webrtc protobuf frame.

@lexnv I completely agree with your concern that we must not break backwards compatibility for other transports. I see two alternatives to including this change:

  1. Write webrtc custom code for decoding multistream-select messages analogous to webrtc_encode_multistream_message.
  2. Get rid of all the webrtc custom code and use MessageIO, DialerSelectFuture, ListenerSelectFuture, etc.

I know that you prefer option 2 based on your comment here and I'm happy to work on that. But I'm also expected to make progress on litep2p <-> smoldot interop and don't have a clear picture of how to implement option 2 yet. 🫤

Copy link
Contributor

@haikoschol haikoschol Nov 6, 2025

Choose a reason for hiding this comment

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

There's a third option: Adding the trailing newline in webrtc_listener_negotiate() before passing it to Message::decode().

pub fn webrtc_listener_negotiate<'a>(
    supported_protocols: &'a mut impl Iterator<Item = &'a ProtocolName>,
    payload: Bytes,
) -> crate::Result<ListenerSelectResult> {
    let payload = if payload.len() > 2 && payload[0..payload.len() - 2] != b"\n\n"[..] {
        let mut buf = BytesMut::from(payload);
        buf.extend_from_slice(b"\n");
        buf.freeze()
    } else {
        payload
    };

    let Message::Protocols(protocols) = Message::decode(payload).map_err(|_| Error::InvalidData)?

@lexnv Is this acceptable to you as a short-term solution if we implement option 2 once we have achieved interoperability?

Copy link
Collaborator

@lexnv lexnv Nov 12, 2025

Choose a reason for hiding this comment

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

The third option is adding the quick workaround without modifying the MultiStream Select core logic.

I'm mostly concerned about subtle changes that will lead to litep2p <-> old litep2p validators misbehaving and causing wide connectivity issues. When this happens, usually a higher level starts to manifest undefined behaviors. The load we are experiencing in kusama / polkadot is not reproducible by our testing setups yet, and spawning large test nets is unsustainable budget-wise 🙏

}
}

Ok(Message::Protocols(protocols))
Expand All @@ -230,7 +234,7 @@ impl Message {
///
/// # Note
///
/// This is implementation is not compliant with the multistream-select protocol spec.
/// This implementation may not be compliant with the multistream-select protocol spec.
/// The only purpose of this was to get the `multistream-select` protocol working with smoldot.
pub fn webrtc_encode_multistream_message(
messages: impl IntoIterator<Item = Message>,
Expand All @@ -249,9 +253,6 @@ pub fn webrtc_encode_multistream_message(
header.append(&mut proto_bytes);
}

// For the `Message::Protocols` to be interpreted correctly, it must be followed by a newline.
header.push(b'\n');
Copy link
Collaborator

@lexnv lexnv Oct 7, 2025

Choose a reason for hiding this comment

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

We could reach out to https://github.com/smol-dot/smoldot to align on the spec, then we could remove/simplify these functions.

We have diverged a bit from the spec, I wonder if this may break things down the line. Would the next message be interpreted correctly?

Copy link
Contributor Author

@timwu20 timwu20 Oct 7, 2025

Choose a reason for hiding this comment

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

We have @freddyli7 working on utilizing smoldot as a client to test against the libp2p server, as well as the litep2p server. We plan on getting it into litep2p-perf to run the performance behaviour. If things are incorrect in smoldot, we will create PRs upstream to fix them.

Copy link
Contributor

@haikoschol haikoschol Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@lexnv Message::Protocols is the response to an ls command, right? According to the spec, supporting ls is optional and smoldot currently does not implement it.

I made a quick attempt to remove this function and it was easy for WebRtcDialerState::propose() (by misusing Message::Protocols 😅), but it's also used on the listener side. It's less clear what the benefit of rewriting that code would be since it would be yet another place where length prefixes are handled.

This is my (simplified?) implementation of WebRtcDialerState::propose():

pub fn propose(
    protocol: ProtocolName,
    fallback_names: Vec<ProtocolName>,
) -> crate::Result<(Self, Vec<u8>)> {
    let header = Protocol::try_from(&b"/multistream/1.0.0"[..])
        .expect("valid multitstream-select header");

    let mut protocols = vec![header];
    protocols.extend(std::iter::once(protocol.clone())
        .chain(fallback_names.clone())
        .filter_map(|protocol| Protocol::try_from(protocol.as_ref()).ok())
    );

    let mut buf = BytesMut::with_capacity(64);
    Message::Protocols(protocols).encode(&mut buf).map_err(|_| Error::InvalidData)?;

    Ok((
        Self {
            protocol,
            fallback_names,
            state: HandshakeState::WaitingResponse,
        },
        buf.freeze().to_vec(),
    ))
}

There are two differences between an ls response message and a protocol request or confirmation message with the header included. The ls response has:

  1. a length-prefix for the entire message at the beginning
  2. a trailing \n

Number 1 is not added by Message::encode() for Message::Protocols. Number 2 is added, but does not throw off smoldot when it expects a protocol request. But strictly speaking using Message::Protocols to encode a protocol request or confirmation message is not spec compliant.

Copy link
Collaborator

@lexnv lexnv Nov 6, 2025

Choose a reason for hiding this comment

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

Similar to https://github.com/paritytech/litep2p/pull/441/files#r2498425079, I believe the path forward is to align both litep2p <-> smoldot to the expected multistream-select behavior.

From the litep2p perspective, it would mean removing this webrtc_encode_multistream_message entirely and relying on the normal DialerState, as with TCP/WebSocket.

The WebRTC code was written quickly as a PoC to validate litep2p interoperability with smoldot; however, it used a few hackish methods that deviated from the spec, which should not be present in production environments (unless technically impossible to avoid).

From the smoldot perspective, we'd need several patches to align with the WebSocket behavior

cc @dmitry-markin

Copy link
Contributor

@haikoschol haikoschol Nov 6, 2025

Choose a reason for hiding this comment

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

What concretely would you like to see changed on the smoldot side? I believe its current behaviour is in line with the spec.

IMHO, the custom code in litep2p is necessary due to the protobuf framing used in webrtc transport. I agree that it would be nice to handle this in a cleaner way, but that shouldn't require changes in smoldot.


Ok(BytesMut::from(&header[..]))
}

Expand Down Expand Up @@ -542,4 +543,21 @@ mod tests {
ProtocolError::InvalidMessage
);
}

#[test]
fn test_decode_multiple_protocols_no_trailing_newline() {
let raw: [u8; 38] = [
19, 47, 109, 117, 108, 116, 105, 115, 116, 114, 101, 97, 109, 47, 49, 46, 48, 46, 48,
10, 17, 47, 105, 112, 102, 115, 47, 112, 105, 110, 103, 47, 49, 46, 48, 46, 48, 10,
];
let bytes = Bytes::copy_from_slice(&raw);

assert_eq!(
Message::decode(bytes).unwrap(),
Message::Protocols(vec![
Protocol::try_from(Bytes::from_static(b"/multistream/1.0.0")).unwrap(),
Protocol::try_from(Bytes::from_static(b"/ipfs/ping/1.0.0")).unwrap(),
])
);
}
}
19 changes: 16 additions & 3 deletions src/transport/webrtc/connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,8 @@ impl WebRtcConnection {
protocol: protocol.to_string(),
});

self.rtc.channel(channel_id).unwrap().set_buffered_amount_low_threshold(1024);
Copy link
Contributor

@haikoschol haikoschol Oct 27, 2025

Choose a reason for hiding this comment

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

When testing smoldot against a minimal litep2p-server, this caused a panic for me because self.rtc.channel(channel_id) returned None. Moving this line to WebRtcConnection::on_channel_opened() prevented the panic.


tracing::trace!(
target: LOG_TARGET,
peer = ?self.peer,
Expand Down Expand Up @@ -742,6 +744,20 @@ impl WebRtcConnection {

continue;
}
Event::ChannelBufferedAmountLow(channel_id) => {
if let Some(ChannelState::Closing) = self.channels.get(&channel_id) {
tracing::trace!(
target: LOG_TARGET,
peer = ?self.peer,
?channel_id,
"buffer drained, closing channel",
);
self.rtc.direct_api().close_data_channel(channel_id);
self.handles.remove(&channel_id);
}

continue;
}
event => {
tracing::debug!(
target: LOG_TARGET,
Expand Down Expand Up @@ -794,10 +810,7 @@ impl WebRtcConnection {
?channel_id,
"channel closed",
);

self.rtc.direct_api().close_data_channel(channel_id);
self.channels.insert(channel_id, ChannelState::Closing);
self.handles.remove(&channel_id);
}
Some((channel_id, Some(SubstreamEvent::Message(data)))) => {
if let Err(error) = self.on_outbound_data(channel_id, data) {
Expand Down
8 changes: 4 additions & 4 deletions src/transport/webrtc/opening.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,14 +176,14 @@ impl OpeningWebRtcConnection {
.rtc
.direct_api()
.remote_dtls_fingerprint()
.clone()
.expect("fingerprint to exist");
.expect("fingerprint to exist")
.clone();
Self::fingerprint_to_bytes(&fingerprint)
}

/// Get local fingerprint as bytes.
fn local_fingerprint(&mut self) -> Vec<u8> {
Self::fingerprint_to_bytes(&self.rtc.direct_api().local_dtls_fingerprint())
Self::fingerprint_to_bytes(self.rtc.direct_api().local_dtls_fingerprint())
}

/// Convert `Fingerprint` to bytes.
Expand Down Expand Up @@ -268,8 +268,8 @@ impl OpeningWebRtcConnection {
.rtc
.direct_api()
.remote_dtls_fingerprint()
.clone()
.expect("fingerprint to exist")
.clone()
.bytes;

const MULTIHASH_SHA256_CODE: u64 = 0x12;
Expand Down