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

protocol-select/: Add Protocol Select specification #349

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Jul 12, 2021

This pull request adds the specification for the Protocol Select protocol.

Protocol Select is a protocol negotiation protocol. It is aimed at negotiating libp2p protocols on connections and streams. It replaces the Multistream Select protocol.

Status

Ready for review.

See also protocol/web3-dev-team#119.

This commit adds a first draft of the _Protocol Select_ specification.

> _Protocol Select_ is a protocol negotiation protocol. It is aimed at
negotiating libp2p protocols on connections and streams. It replaces the
_[Multistream Select]_ protocol.
- Remove `Use` and `Offer` message type, embedding the list of protocols
in the `ProtoSelect` message instead.

- Allow non-multiplexer protocols on first protocol negotiation.

- Mention nested stream protocol negotiation

- Send empty protocol list to say that one supports none of the offered
protocols.
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

thank you for putting this together, it is looking quite good.

protocol-select/README.md Outdated Show resolved Hide resolved
Copy link

@lanzafame lanzafame left a comment

Choose a reason for hiding this comment

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

Minor typo

protocol-select/README.md Outdated Show resolved Hide resolved
mxinden and others added 2 commits July 19, 2021 16:25
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>
protocol-select/README.md Show resolved Hide resolved
protocol-select/README.md Show resolved Hide resolved
protocol-select/README.md Outdated Show resolved Hide resolved
protocol-select/README.md Outdated Show resolved Hide resolved
protocol-select/README.md Outdated Show resolved Hide resolved
protocol-select/README.md Outdated Show resolved Hide resolved
protocol-select/README.md Outdated Show resolved Hide resolved
**Protocol Select** will include the option to improve bandwidth efficiency
e.g. around protocol names in the future. While _Protocol Select_ will not
solve this in the first iteration, the protocol is designed with this
optimization in mind, and allows for a smooth upgrade in a future iteration.
Copy link
Member

Choose a reason for hiding this comment

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

Hm. So, I gave this as one of the primary motivations for Protocol Select, but it's not actually specific to Protocol Select, in it's current incarnation.

That is, I could use the same trick of aliasing codec-based protocol names in multistream.

Let's think very carefully about our upgrade path here. I'm now less convinced it's something we can punt because, well, it's kind of the main reason we wanted this protocol in the first place.

protocol-select/README.md Outdated Show resolved Hide resolved
protocol-select/README.md Outdated Show resolved Hide resolved

* TCP: when the cryptographic handshake is started

* QUIC: when the first stream is opened
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to close a connection? I would say in case of QUIC one can open another substream using Multistream Select.

Copy link
Contributor

Choose a reason for hiding this comment

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

In theory that would work, but it might be quite involved implementation-wise.

Copy link
Contributor

@yusefnapora yusefnapora left a comment

Choose a reason for hiding this comment

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

I like how simple this protocol is, and the spec explains things well. Found a couple little typos - otherwise this is looking really nice.

protocol-select/README.md Outdated Show resolved Hide resolved
protocol-select/README.md Outdated Show resolved Hide resolved
protocol-select/README.md Outdated Show resolved Hide resolved
protocol-select/README.md Outdated Show resolved Hide resolved
Co-authored-by: Yusef Napora <yusef@protocol.ai>
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this forward. Excited to see optimizations such as "Early data".

@cheako
Copy link

cheako commented Sep 15, 2021

Why not take this opportunity to make breaking changes not split the network? Supporting multi-versions is a simple solution, there must be others:

syntax = "proto2";
message ProtoSelect {
    uint32 version = 1;
    uint32 max_forbid_version = 2;
    message Protocol {
        oneof protocol {
            string name = 1;
        }
    }
    repeated Protocol protocols = 3;
}

max_forbid_version is set/updated to the oldest/highest offensive version that is hereby banned... because

  1. a change so critical it warrants banning all clients older than
  2. years, perhaps decades and it's just time

Allow the other end to reply with, sorry, I speak version X and continue accordingly.

@mxinden
Copy link
Member Author

mxinden commented Sep 28, 2021

Hi @cheako, I am sorry for the late reply here.

Why not take this opportunity to make breaking changes not split the network? Supporting multi-versions is a simple solution, there must be others:

I am not quite sure I understand where you are going with this. Could you describe a small scenario where the current versioning scheme would not suffice but your proposal would?

max_forbid_version is set/updated to the oldest/highest offensive version that is hereby banned... because

In particular I am having a hard time understanding why the max_forbid_version needs to be exchanged explicitly.

  1. a change so critical it warrants banning all clients older than

Say client A receives a ProtocolSelect message with a version pre-cricitical-change from B, it could then simply cut off the connection. I expect clients to start with their highest version available, thus there is no need for A to retry as B likely does not support a higher version.

Allow the other end to reply with, sorry, I speak version X and continue accordingly.

Note that this would conflict with Optimistic Protocol Negotiation where the remote might have already sent application data.

@cheako
Copy link

cheako commented Sep 28, 2021

The point is to avoid disconnection because version. So... The lower version should be the one to give up and disconnect when it can't satisfy the required version.

ACID rules should already cover the case where a transaction needs to be rolled back, so it's not a problem that a higher version sends some data that needs to be disregarded.

@mxinden
Copy link
Member Author

mxinden commented Sep 29, 2021

Again, would you mind describing a concrete example where the current versioning scheme would not suffice? I am still having a hard time understanding which problem you are trying to solve @cheako.

@cheako
Copy link

cheako commented Sep 29, 2021

The notion of a sudden disconnect is not expressive enough and is just ambiguous. I don't think it's right to put a lot of effort into the case where a node would be banned. However, it can be done more than the, simple and to the point, max_forbid_version scheme. Another solution is to have a response along with a disconnect(you can try to pad the TCP RST with data) clients are written to understand. There are three things I can think of for such a reply:

  1. That you're being banned, the packet type(obviously the newer node would be sending a legacy packet) expresses this.
  2. The severity of the ban, like should the application close(as in you're a danger to society) after getting a few of these, or is it just this node that it's banned from.
  3. Documentation, a web address for the user to get more information as to why.
    a. The new software may have updated guidance for where documentation lives.

Fields in proto2 have to be either `required`, `optional` or `repeated`. Marking
`version` as `required` as it should be set at all times.
string name = 1;
}
}
repeated Protocol protocols = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned here #353 (comment), we could add a "Optimistic Data" field, that contains data for the first offered protocol.

If the receiver choose the first protocol, he will process that data. Otherwise, the source will have to resend it after the negotiation with the negotiated protocol.

This allow us to generalize "optimistic negotiation", without the risk of closing the stream/connection if we were too optimistic

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 This came up in previous discussions as well.

I would like to keep the first iteration of the protocol as simple as possible. As far as I can tell, this can be added in a backwards compatible way later on. Let me know in case I am missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, though two things to keep in mind:

The "Optimistic Protocol Negotiation" from the current spec should probably be removed, to avoid clashes in the future

And to add backward compatibility in the future, we'll need need to add a "early data processed" bool to the listener -> initiator response, otherwise the initiator has no way to know if the early data has been processed. Not too much of a deal, though (I think)

Copy link
Member Author

Choose a reason for hiding this comment

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

And to add backward compatibility in the future, we'll need need to add a "early data processed" bool to the listener -> initiator response, otherwise the initiator has no way to know if the early data has been processed. Not too much of a deal, though (I think)

That would require the listener to only send a Protocol Select message once it received the initiators message, right? If so, that would prevent all early-data optimizations where the listener side is able to send first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, might be an issue.

In the case where the listener side is sending first, we would have to flip the roles (the listener is becoming initiator)
AFAICT, we always know who should be the "protocol select initiator", even though it may not be the same as the "secure initiator"

But maybe there is a case where we don't know?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

10 participants