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

Remove some multistream-select round-trips #2984

Merged
merged 4 commits into from
Nov 11, 2022

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Nov 8, 2022

cc #2983

This PR removes some extra networking round-trips caused by multistream-select.

The way multistream-select works is: we tell the remote that we would like to use a specific protocol, and then the remote answers either yes or no. No is only ever returned if the remote doesn't support the protocol at all, which isn't supposed to happen unless we're talking to a buggy node or to a libp2p-compatible-but-not-Substrate node.
Before this PR, we wait for the remote to send back yes or no. After this PR, we don't wait and simply start sending the protocol-specific data immediately after the request for a protocol, and assume that the remote is going to answer yes.

The negotiation is still properly finished afterwards, so if it turns out that the remote answers no, then we'll still get an error locally. The drawback is that, if the remote answers no, the protocol-specific data that we have eagerly sent will be interpreted as being from the multistream-select protocol, which can lead to confusing decoding errors. However, the trade-off is worth it.

I've done this change only for substreams after the connection has been opened. There are two other unnecessary round-trips during the connection opening, but considering that we don't open connections that often they're actually less important. For this reason, I'll leave #2983 open.

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
Copy link
Contributor Author

tomaka commented Nov 8, 2022

Note that this optimization is implemented in rust-libp2p. It is optional and disabled by default, but in Substrate we enable it.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

twiggy diff report

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


 Delta Bytes │ Item
─────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       -8656 ┊ smoldot::libp2p::connection::established::substream::Substream<TNow,TRqUd,TNotifUd>::read_write2::h6afd28191e4960fb
       +7782 ┊ smoldot::libp2p::connection::established::substream::Substream<TNow,TRqUd,TNotifUd>::read_write2::hae7d3f223e29d3f0
        -372 ┊ core::ptr::drop_in_place<smoldot::libp2p::connection::established::substream::SubstreamInner<smoldot_light_wasm::Instant,smoldot::libp2p::collection::SubstreamId,()>>::haae316d88bbb650c
        -346 ┊ <alloc::collections::vec_deque::iter::Iter<T> as core::iter::traits::iterator::Iterator>::try_fold::h5142d5a1cb54c2d7
        +316 ┊ core::ptr::drop_in_place<smoldot::libp2p::connection::established::substream::SubstreamInner<smoldot_light_wasm::Instant,smoldot::libp2p::collection::SubstreamId,()>>::h99da7da6d773ce82
        -303 ┊ <alloc::collections::vec_deque::VecDeque<T,A> as alloc::collections::vec_deque::spec_extend::SpecExtend<T,I>>::spec_extend::h71b3f1c9388c16be
        +171 ┊ smoldot::libp2p::connection::established::substream::Substream<TNow,TRqUd,TNotifUd>::request_out::hf22bf503083fd452
        +126 ┊ smoldot::libp2p::connection::established::substream::Substream<TNow,TRqUd,TNotifUd>::notifications_out::h405143471ea7ab89
        -123 ┊ hashbrown::raw::RawTable<T,A>::fallible_with_capacity::hb56b2a0b7dd0b5a2
        -117 ┊ <alloc::collections::vec_deque::iter::Iter<T> as core::iter::traits::iterator::Iterator>::fold::ha1d6aba3b80d79bf
        -107 ┊ core::ops::function::impls::<impl core::ops::function::FnMut<A> for &mut F>::call_mut::h0c9b7fc464bae429
        -103 ┊ core::iter::traits::iterator::Iterator::try_fold::h52a21bfc24ed7add
        -102 ┊ <core::iter::adapters::cloned::Cloned<I> as core::iter::traits::iterator::Iterator>::fold::hc6ed628de23d2f5e
        -102 ┊ <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::fold::h571507e1a0cae5aa
         -99 ┊ <core::iter::adapters::cloned::Cloned<I> as core::iter::traits::iterator::Iterator>::try_fold::hcc89b838d455a14b
         -99 ┊ smoldot::libp2p::connection::established::substream::Substream<TNow,TRqUd,TNotifUd>::reset::hc87c63ce6becfa4a
         -96 ┊ <alloc::collections::vec_deque::VecDeque<T,A> as core::clone::Clone>::clone::h6732c1aa30e949ee
         -76 ┊ core::iter::traits::iterator::Iterator::fold::h75f7b2aa62c050eb
         -72 ┊ core::ops::function::impls::<impl core::ops::function::FnMut<A> for &mut F>::call_mut::h53d23bfa68d9fcb2
         -72 ┊ hashbrown::raw::RawTable<T,A>::with_capacity_in::hf9f9459d3c171509
         +40 ┊ ... and 17 more.
       -3930 ┊ Σ [37 Total Rows]

@tomaka tomaka added the automerge Automatically merge pull request as soon as possible label Nov 11, 2022
@mergify mergify bot merged commit b848439 into paritytech:main Nov 11, 2022
@tomaka tomaka deleted the multistream-rt branch November 11, 2022 14:25
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.

1 participant