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

Configurable multistream-select protocol. V1Lazy variant. #1245

Merged
merged 1 commit into from
Sep 23, 2019

Conversation

romanb
Copy link
Contributor

@romanb romanb commented Sep 13, 2019

This is a proposal for making the multistream-select protocol (version) configurable both on transport upgrades as well as for individual substreams. It adds a new "lazy" version of multistream-select 1.0 that is identical on the wire but delays sending of negotiation protocol frames as much as possible, which is only safe to use under additional assumptions that go beyond what is required by the multistream-select v1 specification (see the inline doc-comment on Version::V1Lazy and the discussion in #1212).

There are a couple of motivations behind this:

  1. It will arguably be necessary eventually to be able to specify the multistream-select protocol version to use, at least as a dialer, to ever support 0-RTT negotiation in the presence of multiple protocol versions to choose from.

  2. By defaulting to the standard V1 version, the optimisations provided by lazy negotiation are still provided as an opt-in per transport and per substream protocol, without losing full compatibility by default (i.e. addressing multistream-select interoperability issue with JS #1241 in a different way than Disable multistream optimization to be compatible #1244). It may thus offer an opt-in fast-path for specific projects or protocols, especially ones that want to make heavy use of the substream-per-request pattern, such as in network: Use "one shot" protocol handler. paritytech/substrate#3520.

  3. The differences between the versions is constrained to the multistream-select library, not requiring outside workarounds for completely safe V1 compatibility like there is currently in libp2p-core with the additional AwaitNegotiated step for dialer upgrades. I.e. the distinction between fully conformant / interoperable V1 and the V1Lazy variant are fully inside multistream-select, with the choice exposed through the choice of Version for the dialer (the listener can always safely support all protocol versions, which also provides a general upgrade path from older to newer versions).

An example for configuring transport upgrades to use lazy negotiation:

            let transport = libp2p_tcp::TcpConfig::new()
                .upgrade(upgrade::Version::V1Lazy)
                .authenticate(libp2p_secio::SecioConfig::new(local_key))
                .multiplex(libp2p_mplex::MplexConfig::new())

An example for configuring outbound substreams on a handler to use lazy negotiation:

let proto = SubstreamProtocol::new(p).with_upgrade_protocol(upgrade::Version::V1Lazy);
OneShotHandler::new(proto, self.config.inactivity_timeout)

The only thing that would still be nice to have is to default the multistream protocol version used for substreams to the one used for the transport upgrades, but I didn't see an easy way to do that kind of propagation at the moment.

Make the multistream-select protocol (version) configurable
on transport upgrades as well as for individual substreams.

Add a "lazy" variant of multistream-select 1.0 that delays
sending of negotiation protocol frames as much as possible
but is only safe to use under additional assumptions that
go beyond what is required by the multistream-select v1
specification.
@@ -77,8 +80,8 @@ where
T::Error: 'static,
{
/// Creates a `Builder` over the given (base) `Transport`.
pub fn new(transport: T) -> Builder<T> {
Builder { inner: transport }
pub fn new(inner: T, version: upgrade::Version) -> Builder<T> {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not so much of a fan of this. I think there should be a default version that the user can override.
However I see that the builder would have to be rewritten to account for this, so it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a default version - Version even implements Default. Would you prefer having two different variants of Transport::upgrade, one without a Version parameter and one with such a parameter, whereby the former delegates to Builder::new(...) with the default version? If so, any name suggestions for the upgrade variant taking a version? upgrade_with? upgrade_using? upgrade_with_protocol? As you see my reason for not doing so was that I struggled with one of the hardest problems in computer science: naming things.

/// The encoded form of a multistream-select 2.0.0 header message.
const MSG_MULTISTREAM_2_0: &[u8] = b"/multistream/2.0.0\n";
/// The encoded form of a multistream-select 1.0.0 header message.
const MSG_MULTISTREAM_1_0_LAZY: &[u8] = b"/multistream-lazy/1\n";
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, I thought the idea was to keep /multistream/1.0.0, but I guess this works as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without a different protocol name, there can be no different Versions based on which both dialer and listener implement different behaviour (e.g. when to flush). I don't see how that could be done otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

From the client's point of view, dialing V1 and V1Lazy is the same (except for the protocol name), no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the client's point of view, dialing V1 and V1Lazy is the same (except for the protocol name), no?

No, for V1 the dialer now never prematurely goes into the Expecting state, it flushes every protocol proposal and reads the reply. This was done previously in libp2p_core::upgrade::apply_outbound anyway by waiting for negotiation to complete between upgrades, as I mentioned in the PR description, to avoid the edge-cases discussed in the other PRs. Now this behaviour is localised to multistream-select and subject to the chosen version.

Also, the listener needs to know whether the dialer wants the lazy variant or not, based on which it decides whether it needs to immediately flush its protocol response (addressing #1241 as mentioned in the PR description). Without a different protocol name on the wire, there can be no parsing into different Versions.

Apart from that, even though V1 and V1Lazy are identical as far as the protocol messages on the wire are concerned, important expectations on the granularity with which requests and responses are sent and received or buffered that can cause incompatibilities if not observed by both peers are part of the protocol and thus such differences deserve a different protocol name from that perspective as well, I think.

@tomaka
Copy link
Member

tomaka commented Sep 18, 2019

I may be mistaken, but isn't it possible to also do the optimization where, in the case of a single protocol, we assume that the remote supports it, and close the connection if it doesn't?

@romanb
Copy link
Contributor Author

romanb commented Sep 19, 2019

I may be mistaken, but isn't it possible to also do the optimization where, in the case of a single protocol, we assume that the remote supports it, and close the connection if it doesn't?

For safety of nested protocol negotiations, only V1Lazy can optimistically assume a protocol proposal to succeed and it does that if the dialer only supports a single protocol (or he gets to his last protocol proposal, i.e. runs out of options). The dialer then expects to get confirmation for that protocol and otherwise fails negotiation. Whether failed negotiation results in closing the connection is of course not up to multistream-select itself, since it operates on an opaque I/O resource which may or may not be a connection, but e.g. reading from a Negotiated<C> (or a graceful shutdown after writing) will fail with an error if negotiation fails.

@romanb romanb merged commit 73e7878 into libp2p:master Sep 23, 2019
@romanb romanb deleted the multistream-select-v1lazy branch September 23, 2019 10:04
tomaka added a commit that referenced this pull request Oct 10, 2019
* Configurable multistream-select protocol. Add V1Lazy variant. (#1245)

Make the multistream-select protocol (version) configurable
on transport upgrades as well as for individual substreams.

Add a "lazy" variant of multistream-select 1.0 that delays
sending of negotiation protocol frames as much as possible
but is only safe to use under additional assumptions that
go beyond what is required by the multistream-select v1
specification.

* Improve the code readability of the chat example (#1253)

* Add bridged chats (#1252)

* Try fix CI (#1261)

* Print Rust version on CI

* Don't print where not appropriate

* Change caching strategy

* Remove win32 build

* Remove win32 from list

* Update libsecp256k1 dep to 0.3.0 (#1258)

* Update libsecp256k1 dep to 0.3.0

* Sign now cannot fail

* Upgrade url and percent-encoding deps to 2.1.0 (#1267)

* Upgrade percent-encoding dep to 2.1.0

* Upgrade url dep to 2.1.0

* Fix more conflicts

* Revert CIPHERS set to null (#1273)
tomaka added a commit that referenced this pull request Oct 28, 2019
* Configurable multistream-select protocol. Add V1Lazy variant. (#1245)

Make the multistream-select protocol (version) configurable
on transport upgrades as well as for individual substreams.

Add a "lazy" variant of multistream-select 1.0 that delays
sending of negotiation protocol frames as much as possible
but is only safe to use under additional assumptions that
go beyond what is required by the multistream-select v1
specification.

* Improve the code readability of the chat example (#1253)

* Add bridged chats (#1252)

* Try fix CI (#1261)

* Print Rust version on CI

* Don't print where not appropriate

* Change caching strategy

* Remove win32 build

* Remove win32 from list

* Update libsecp256k1 dep to 0.3.0 (#1258)

* Update libsecp256k1 dep to 0.3.0

* Sign now cannot fail

* Upgrade url and percent-encoding deps to 2.1.0 (#1267)

* Upgrade percent-encoding dep to 2.1.0

* Upgrade url dep to 2.1.0

* Revert CIPHERS set to null (#1273)

* Update dependency versions (#1265)

* Update versions of many dependencies

* Bump version of rand

* Updates for changed APIs in rand, ring, and webpki

* Replace references to `snow::Session`

`Session` no longer exists in `snow` but the replacement is two structs `HandshakeState` and `TransportState`
Something will have to be done to harmonize `NoiseOutput.session`

* Add precise type for UnparsedPublicKey

* Update data structures/functions to match new snow's API

* Delete diff.diff

Remove accidentally committed diff file

* Remove commented lines in identity/rsa.rs

* Bump libsecp256k1 to 0.3.1

* Implement /plaintext/2.0.0 (#1236)

* WIP

* plaintext/2.0.0

* Refactor protobuf related issues to compatible with the spec

* Rename: new PlainTextConfig -> PlainText2Config

* Keep plaintext/1.0.0 as PlainText1Config

* Config contains pubkey

* Rename: proposition -> exchange

* Add PeerId to Exchange

* Check the validity of the remote's `Exchange`

* Tweak

* Delete unused import

* Add debug log

* Delete unused field: public_key_encoded

* Delete unused field: local

* Delete unused field: exchange_bytes

* The inner instance should not be public

* identity::Publickey::Rsa is not available on wasm

* Delete PeerId from Config as it should be generated from the pubkey

* Catch up for #1240

* Tweak

* Update protocols/plaintext/src/error.rs

Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

* Update protocols/plaintext/src/handshake.rs

Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

* Update protocols/plaintext/src/error.rs

Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

* Update protocols/plaintext/src/error.rs

Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>

* Update protocols/plaintext/src/error.rs

Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>

* Rename: pubkey -> local_public_key

* Delete unused error

* Rename: PeerIdValidationFailed -> InvalidPeerId

* Fix: HandShake -> Handshake

* Use bytes insteadof Publickey to avoid code duplication

* Replace with ProtobufError

* Merge HandshakeContext<()> into HandshakeContext<Local>

* Improve the peer ID validation to simplify the handshake

* Propagate Remote to allow extracting the PeerId from the Remote

* Collapse the same kind of errors into the variant

* [noise]: `sodiumoxide 0.2.5` (#1276)

Fixes rustsec/advisory-db#192

* examples/ipfs-kad.rs: Remove outdated reference to `without_init` (#1280)

* CircleCI Test Fix (#1282)

* Disabling "Docker Layer Caching" because it breaks one of the circleci checks

* Bump to trigger CircleCI build

* unbump

* zeroize: Upgrade to v1.0 (#1284)

v1.0 final release is out. Release notes:

iqlusioninc/crates#279

* *: Consolidate protobuf scripts and update to rust-protobuf 2.8.1 (#1275)

* *: Consolidate protobuf generation scripts

* *: Update to rust-protobuf 2.8.1

* *: Mark protobuf generated modules with '_proto'

* examples: Add distributed key value store (#1281)

* examples: Add distributed key value store

This commit adds a basic distributed key value store supporting GET and
PUT commands using Kademlia and mDNS.

* examples/distributed-key-value-store: Fix typo

* Simple Warning Cleanup (#1278)

* Cleaning up warnings - removing unused `use`

* Cleaning up warnings - unused tuple value

* Cleaning up warnings - removing dead code

* Cleaning up warnings - fixing deprecated name

* Cleaning up warnings - removing dead code

* Revert "Cleaning up warnings - removing dead code"

This reverts commit f18a765.

* Enable the std feature of ring (#1289)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants