feat: enforce protocols to always be valid utf8 strings by thomaseizinger · Pull Request #3745 · libp2p/rust-libp2p

thomaseizinger · 2023-04-06T18:34:30Z

Description

Previously, a protocol could be any sequence of bytes as long as it started with /. Now, we directly parse a protocol as String which enforces it to be valid UTF8. This has a significant API impact. Instead of any type being able to represent a protocol, we use a Protocol newtype now that can only be constructed from a String or a &'static str.

Technically, this would not be necessary to for protocols to always be strings. We could also only change the ProtocolName trait to just return &str instead of &[u8]. However, both of these are breaking changes and I am of the opinion that there are currently too many abstraction layers in regards to stream upgrades. This change removes some of these layers by introducing the UpgradeProtocols trait which replaces the UpgradeInfo trait.

We will eventually lose a bit of type safety with this approach. In particular, once we also ship #2863, types like DeniedUpgrade won't work anymore because each protocol is represented as the same type.

Notes & open questions

Is the change in abstraction layers good? Should we go this far or retain the type-safety of upgrades we get from protocols being able to be any type and just having to satisfy a trait? I am going to prototype that too and see what it is like.

This is a draft because currently, only the multistream-select tests compile and work. I've only touched the code necessary for that to work.

Dependencies

In order to make the implementation of this PR simpler, I've extracted several others. They are all deprecations, meaning we can release them first, then delete the code and finally merge this PR (once it is ready in itself).

feat(noise): deprecate all handshake patterns apart from XX #3768
feat(core): deprecate {In,Out}boundUpgradeExt #3807
feat(core): deprecate OptionalUpgrade #3806
Remove all deprecated items from libp2p-request-response

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

thomaseizinger · 2023-04-06T19:25:56Z

@mxinden please review despite broken build, multistream-select by itself passes.

mxinden

Preliminary review. Still need more time.

mxinden · 2023-04-10T14:44:32Z

core/src/upgrade.rs

@@ -158,7 +196,7 @@ pub trait InboundUpgrade<C>: UpgradeInfo {
    /// method is called to start the handshake.
    ///
    /// The `info` is the identifier of the protocol, as produced by `protocol_info`.


Needs updating.

core/src/upgrade.rs

thomaseizinger · 2023-04-19T20:38:18Z

This is turning out to be pretty invasive but I think it is worth the breaking change. We reduce complexity by removing one layer of abstraction. This looses us some type safety as mentioned in the description but in reality, this just means we can no longer do void::unreachable in the event handler in some edge cases. I don't think the type safety in these edge cases is worth the complexity of these abstractions for our users.

thomaseizinger · 2023-04-19T21:55:04Z

I'd be hoping that we can ship deprecations like #3807 first such that I can delete all this code and don't have to touch it in these breaking changes :)

mxinden

Overall direction looks good to me.

mxinden · 2023-04-20T19:16:46Z

core/src/upgrade/select.rs

-            EitherName::A(info) => EitherFuture::First(self.0.upgrade_inbound(sock, info)),
-            EitherName::B(info) => EitherFuture::Second(self.1.upgrade_inbound(sock, info)),
+    fn upgrade_inbound(self, sock: C, selected_protocol: Protocol) -> Self::Future {
+        if self.0.protocols().any(|p| p == selected_protocol) {


As described in your pull request, having to do this now is a bummer. This is in the hot-path. With our move to one-stream-per-request this will be executed often.

All that said, I am fine moving forward here. Based on intuition, this warrants a benchmark before introducing any optimization.

One issue is that SelectUpgrade is constructed for each upgrade, meaning it is pointless to try and cache something.

Once we get rid of upgrades for protocols, we can try and cache some information in the handler, i.e. a HashMap of Protocol to sub-handler.

I'd suggest we defer it until then. Despite being the hotpath, I think iterating a handful of protocols (which should be allocation-free) shouldn't be very slow?

An alternative that came to my mind:

Introduce a ToProtocol trait which Protocol implements by default. Our ToProtocolsIter would require an Iterator where Item implements ToProtocol.

With this abstraction we still have the simplicity for the user, namely that they can just use Protocol and that they can't mistake a Protocol for a str given that the latter is wrapped in a newtype. At the same time we maintain the functionality for e.g. the SelectUpgrade.

Just a thought. I don't feel strongly about this.

That is effectively the same solution as we had before, just with a different name. It doesn't remove a "layer" for the user because they still have to specify the Item type.

But perhaps we should go with this path until we fully tackle: #2863

At that point, we will also be able to implement performance optimizations in the Select handler.

With the introduction of separate newtypes in multistream-select and libp2p-core, this is proving to be difficult.

On the one hand, I'd like to maintain this separation, I think multistream-select should be an implementation detail of libp2p-core and libp2p-swarm.

But, with the two new-types in place, I cannot re-create the type-safety we had before because multistream-select doesn't know about the type used in libp2p-core.

Perhaps it is too early to try and remove this abstraction. Here is a different idea:

Resurrect feat: replace ProtocolName with AsRef<str> #3746

Still introduce a Protocol newtype but have it in libp2p-swarm. This means protocols can use it and we can use it within our event types and commands

misc/multistream-select/src/dialer_select.rs

mxinden · 2023-04-20T19:22:10Z

misc/multistream-select/src/protocol.rs

-#[derive(Clone, Debug, PartialEq, Eq)]
-pub struct Protocol(Bytes);
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+pub struct Protocol(Cow<'static, str>); // TODO: Instead of `Cow`, we should probably be storing `Arc<str>`


How about we start with as is and introduce an Arc<str> if necessary? None of it is exposed, thus easy to change in a non-breaking way.

I am not fully sure. It improves performance for cases where we dynamically construct a Protocol. Ideally, users use from_static as much as possible so it is kind of a niche usecase but I still want it to be performant.

It is not much work so I think I am just gonna do it. I don't see why a protocol string should ever be mutable which is the only thing that becomes impossible with Arc<str>.

An Arc<str> would not be enough, right? Arc would require a heap allocation, no? So you would have Either<&'static str, Arc<String>> instead?

Yes, we would have an Either of &'static str and Arc<String>.

misc/multistream-select/src/protocol.rs

misc/metrics/src/identify.rs

misc/multistream-select/src/dialer_select.rs

core/src/upgrade.rs

mxinden · 2023-04-25T06:19:03Z

core/src/upgrade/select.rs

-            EitherName::A(info) => EitherFuture::First(self.0.upgrade_inbound(sock, info)),
-            EitherName::B(info) => EitherFuture::Second(self.1.upgrade_inbound(sock, info)),
+    fn upgrade_inbound(self, sock: C, selected_protocol: Protocol) -> Self::Future {
+        if self.0.protocols().any(|p| p == selected_protocol) {


An alternative that came to my mind:

Introduce a ToProtocol trait which Protocol implements by default. Our ToProtocolsIter would require an Iterator where Item implements ToProtocol.

With this abstraction we still have the simplicity for the user, namely that they can just use Protocol and that they can't mistake a Protocol for a str given that the latter is wrapped in a newtype. At the same time we maintain the functionality for e.g. the SelectUpgrade.

Just a thought. I don't feel strongly about this.

mxinden · 2023-04-25T06:26:04Z

misc/multistream-select/src/protocol.rs

-#[derive(Clone, Debug, PartialEq, Eq)]
-pub struct Protocol(Bytes);
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+pub struct Protocol(Cow<'static, str>); // TODO: Instead of `Cow`, we should probably be storing `Arc<str>`


An Arc<str> would not be enough, right? Arc would require a heap allocation, no? So you would have Either<&'static str, Arc<String>> instead?

protocols/kad/src/protocol_priv.rs

transports/webrtc/src/tokio/upgrade/noise.rs

These functions were only used for some code in the interop-tests which is easily mitigated and perhaps even easier to understand now. We can thus deprecate these functions and their related types and thereby reduce the API surface of `libp2p-core` and the maintenance burden. This change is motivated by the work around making protocols always strings which requires/required updates to all these upgrades. Related #3806. Related #3271. Related #3745. Pull-Request: #3807.

mxinden · 2023-04-26T07:04:44Z

@thomaseizinger please ping me when I should take another look.

thomaseizinger · 2023-04-26T11:07:06Z

@thomaseizinger please ping me when I should take another look.

Will do!

mergify · 2023-04-26T12:56:40Z

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

thomaseizinger · 2023-04-26T14:39:59Z

I am going with a different approach now: #3746

thomaseizinger added 5 commits April 5, 2023 11:03

Introduce new UpgradeProtocols trait

e9de619

WIP

ad4767e

Make multistream-select tests pass

3c3398f

Remove commented code

1a6ff9a

Add TODO

50edaee

thomaseizinger requested a review from mxinden April 6, 2023 18:34

thomaseizinger mentioned this pull request Apr 6, 2023

feat: replace ProtocolName with AsRef<str> #3746

Merged

4 tasks

Use iterators instead of vecs to reduce allocations

c139b6c

thomaseizinger mentioned this pull request Apr 10, 2023

protocols/kad: Support client mode #2032

Closed

mxinden reviewed Apr 10, 2023

View reviewed changes

thomaseizinger added 3 commits April 19, 2023 20:02

Merge branch 'master' into feat/string-protocols-2

abda420

Make from_static const

c148ed6

Introduce dedicated Protocol struct in libp2p-core

83fa4c3

thomaseizinger mentioned this pull request Apr 19, 2023

feat: report changes in supported protocols to ConnectionHandler #3651

Merged

9 tasks

thomaseizinger mentioned this pull request Apr 19, 2023

feat(core): deprecate {In,Out}boundUpgradeExt #3807

Merged

4 tasks

mxinden reviewed Apr 20, 2023

View reviewed changes

thomaseizinger marked this pull request as ready for review April 23, 2023 10:56

This comment was marked as resolved.

Sign in to view

thomaseizinger added 2 commits April 24, 2023 19:18

Start deprecating things

244fc7f

Make it compile

f909139

thomaseizinger force-pushed the feat/string-protocols-2 branch from f1a9349 to f909139 Compare April 24, 2023 19:01

Rename UpgradeProtocols type

0c63687

mxinden reviewed Apr 25, 2023

View reviewed changes

thomaseizinger added 3 commits April 25, 2023 22:36

Merge branch 'master' into feat/string-protocols-2

4671126

Delete deprecated upgrades

bf24183

Don't preemptively collect iterator

885854a

thomaseizinger added 5 commits April 25, 2023 23:17

Fix typo

24b77ba

Convert protocols in identify

0762c46

Revert hard-coded protocol in webrtc

b71e517

Reduce diff

2274e02

Fix bad TLS protocol

5d40167

This comment was marked as resolved.

Sign in to view

thomaseizinger added 3 commits April 26, 2023 13:08

Merge branch 'master' into feat/string-protocols-2

19c0ddf

Delete dead code

dc4ce72

Fix docs

ea872f0

This comment was marked as outdated.

Sign in to view

thomaseizinger closed this Apr 26, 2023

Conversation

thomaseizinger commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes & open questions

Dependencies

Change checklist

Uh oh!

thomaseizinger commented Apr 6, 2023

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thomaseizinger commented Apr 19, 2023

Uh oh!

thomaseizinger commented Apr 19, 2023

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

mxinden commented Apr 26, 2023

Uh oh!

thomaseizinger commented Apr 26, 2023

Uh oh!

This comment was marked as outdated.

mergify bot commented Apr 26, 2023

Uh oh!

thomaseizinger commented Apr 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thomaseizinger commented Apr 6, 2023 •

edited

Loading

thomaseizinger Apr 25, 2023 •

edited

Loading