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 support for "multi-stream connections" #2352

Merged
merged 41 commits into from
Jun 14, 2022

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jun 7, 2022

cc #1712

Smoldot contains bindings with the JS code. Some functions in these bindings ask the JS code to open a connection to a multiaddress. On success, the JS code reports that the connection has been opened, and smoldot and the JS communicate in order to send and receive data on that connection.

Internally, smoldot performs a protocol negotiation of Noise + Yamux on these connections, and then applies the encryption and multiplexing internally.

The connect function that the JS must implement accepts as parameter a string multiaddress that the JS decodes in order to know the "actual" type of connection to open. At the moment, this is either a plain TCP/IP connection or a WebSocket connection.

We want to add support for WebRTC. The problem with WebRTC is that it is the browser that performs the encryption and multiplexing, and thus it doesn't fit the model of one stream subdivided internally by smoldot.
For this reason, we have to add in smoldot a second type of connection named "multi-stream connections".
In order to not introduce any ambiguity, the current connections have been renamed to "single-stream connections".

This PR adds support for that other type of connection as a new module in src/libp2p/connection/established.
In collection.rs, peers.rs, and service.rs (which are collections of connections), multi-stream connections are handled the same way as single-stream connections except that the function to add a new connection is different.
Inserting a connection previously returned a ConnectionTask that allows reading/writing on that connection. The ConnectionTask struct has been renamed to SingleStreamConnectionTask and there is now a MultiStreamConnectionTask that has a different API.

I have correspondingly updated the Platform trait of the light-base crate and bin/light-base/network_service so that this type of connection is accessible to the Wasm node. I have also updated the JS <-> Wasm bindings, and the JS code can add support for WebRTC right now after this PR. However I've left bin/wasm-node/rust/platform (i.e. the code that "connects" the low-level bindings that pass u32s around and the higher-level objects) full of todo!s because I would like to refactor this module entirely.

As always, this code hasn't been tested and to be honest it would be a bit miraculous if everything worked first try. I'm opening this PR now because I've been working on it for 2 weeks now, and I would like to land it and iterate on top of it. I believe that my API design is good, and only the implementation details are lacking.

Can be reviewed commit by commit.

Copy link
Contributor

@mergify mergify bot left a comment

Choose a reason for hiding this comment

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

Automatically approving tomaka's pull requests. This auto-approval will be removed once more maintainers are active.

@tomaka tomaka mentioned this pull request Jun 7, 2022
2 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

twiggy diff report

Difference in .wasm size before and after this pull request.


 Delta Bytes │ Item
─────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       +7608 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h91ea29f274dd257c
       -7569 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::hd577c4ce39ea0c0e
       -4439 ┊ smoldot::libp2p::collection::ConnectionTask<TNow>::read_write::h5a98749c1f8976ce
       +4097 ┊ smoldot::libp2p::collection::SingleStreamConnectionTask<TNow>::read_write::h4941fc3b8b423bbf
       -2915 ┊ smoldot::libp2p::connection::established::Established<TNow,TRqUd,TNotifUd>::read_write::h19d2afca2e3ff2a1
       +2702 ┊ smoldot::libp2p::connection::established::single_stream::SingleStream<TNow,TRqUd,TNotifUd>::read_write::h364933ca5cb6bf4f
       +2645 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h4852f61409e7b92a
       -2575 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::ha7bec94c2115ef9d
       +2560 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h3f3d43ee50519cb0
       -2456 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::ha8c738376ea0a8fd
       +2372 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h4c742e8762a3aaf3
       +1740 ┊ smoldot::libp2p::connection::established::multi_stream::MultiStream<TNow,TSubId,TRqUd,TNotifUd>::substream_read_write::hdf84bbd7e4172582
       -1607 ┊ smoldot::libp2p::connection::established::Established<TNow,TRqUd,TNotifUd>::process_substream::h8f113b791cc040e4
       +1511 ┊ smoldot::libp2p::connection::established::single_stream::SingleStream<TNow,TRqUd,TNotifUd>::process_substream::hd681daf4c6ec674f
       +1384 ┊ smoldot::libp2p::collection::SingleStreamConnectionTask<TNow>::inject_coordinator_message::ha98d19b6eded6cb3
       -1317 ┊ smoldot::libp2p::collection::ConnectionTask<TNow>::inject_coordinator_message::hb2a3bade5b64dc49
       +1309 ┊ smoldot::libp2p::collection::MultiStreamConnectionTask<TNow,TSubId>::inject_coordinator_message::hec6a94df418a3f91
       +1171 ┊ smoldot::libp2p::collection::MultiStreamConnectionTask<TNow,TSubId>::pull_message_to_coordinator::hf62149bdb9fe8fd5
       +1088 ┊ data[0]
        +984 ┊ hashbrown::raw::RawTable<T,A>::reserve_rehash::h9ee0e5796cd5c71f
      +17146 ┊ ... and 502 more.
      +43417 ┊ Σ [522 Total Rows]

@tomaka tomaka requested a review from melekes June 7, 2022 15:00
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

great work @tomaka 💯 did a first pass and everything looks good. will do another one later today or maybe tomorrow

bin/light-base/src/lib.rs Show resolved Hide resolved
/// Asynchronous task managing a specific multi-stream connection after it's been open.
// TODO: a lot of logging disappeared
async fn multi_stream_connection_task<TPlat: Platform>(
mut websocket: TPlat::Connection,
Copy link
Contributor

Choose a reason for hiding this comment

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

it's no longer a websocket, right? rename to connection or conn or platform_conn whichever makes more sense

bin/light-base/src/network_service.rs Show resolved Hide resolved
}

if has_message {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

there are so many loops in this function. might be good to assign a label to the main one

Suggested change
continue;
continue 'connection_task_loop


/// Reverse mapping.
// TODO: could be user datas in established?
outbound_substreams_mapping2:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
outbound_substreams_mapping2:
outbound_substreams_reverse:

src/libp2p/collection.rs Show resolved Hide resolved
// TODO: check conflicts between protocol names?

let num_expected_substreams =
config.request_protocols.len() + config.notifications_protocols.len() * 2 + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

can u maybe add a comment explaining the logic behind this?

ping_timeout: Duration::from_secs(10), // TODO: hardcoded
first_out_ping: now + Duration::from_secs(2), // TODO: hardcoded
}),
outbound_substreams_mapping: hashbrown::HashMap::with_capacity_and_hasher(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
outbound_substreams_mapping: hashbrown::HashMap::with_capacity_and_hasher(
outbound_substreams_map: hashbrown::HashMap::with_capacity_and_hasher(

to be consistent with the rest of the code

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

@tomaka tomaka mentioned this pull request Jun 14, 2022
@tomaka tomaka added the automerge Automatically merge pull request as soon as possible label Jun 14, 2022
@mergify mergify bot merged commit f1e2a38 into paritytech:main Jun 14, 2022
@tomaka tomaka deleted the multistream-ext branch June 14, 2022 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge pull request as soon as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants