webrtc: update str0m, fix multistream message decoding and encoding

[timwu20]

Changes

• updates the str0m dependency to the commit which contains fixes I co-authored (PR #1, #2)
  • fixes str0m integration based on breaking public API changes
  • currently the references the latest commit hash, will update again when a release is cut by @algesten
• fixes the decoding of multistream messages to support messages without trailing linebreak
• removes trailing linebreak when encoding multistream message used for WebRTC
• sets the channel low buffer amount threshold to 1024 bytes, and listens on Event::ChannelBufferedAmountLow to close a substream/rtc channel.
• contributes to webrtc: Test the connection stability of the webrtc substreams #420

[lexnv]

Was this observed with smoldot integration?

[timwu20]

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

[haikoschol]

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. :)

[lexnv]

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:

• https://github.com/libp2p/rust-libp2p/blob/b3d31673a0ed3f45f4c95cf4865c8381dba6f471/misc/multistream-select/src/protocol.rs#L224-L227

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

[haikoschol]

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. 🫤

[haikoschol]

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?

[lexnv]

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?

[timwu20]

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.

[haikoschol]

@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.

[lexnv]

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

[haikoschol]

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.

[algesten]

@timwu20 I released 0.11.1

[timwu20]

Added 8cf27e9 which calls Channel.set_buffered_amount_low_threshold to set the threshold to 1024 bytes, and listen on this event to close the channel if the protocol behaviour intends to close the substream.

With this change I'm able to get litep2p-perf to run the performance behaviour using a litep2p server utilizing WebRTC transport to upload 16MB of data to a libp2p WebRTC client. Any byte counts higher than that cause the connection to get dropped because of the lack of ICE keep-alive requests. I believe this has to do with the lack of proper back pressure when writing on the server side. I think we can address this further in issue #45.

[haikoschol]

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.