Make the notification protocols handshake message configurable through the API

[tomaka]

At the moment, the handshake is hard-coded to be empty. After paritytech/substrate#5665, it is hard-coded to be a single byte containing the Role of the node.

The intent, however, is to make this configurable by whatever code has registered a notifications protocol.

The difficult part in this is to figure out a good API for this.
I also don't think it's possible before paritytech/substrate#5670, because it is possible for the legacy substream to be opened before the notifications substream.

[tomaka]

In order to achieve this, I think we move peerset out of behaviour.rs, and add APIs to behaviour.rs so that it can be controlled by the outside.

[stale]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

[tomaka]

Issue still relevant and important.

[altonen]

I have a proposal. We could define the following enum that would provide three different handshake validation schemes:

enum Handshake {
    /// Use default handshake mechanism, i.e., checking `Role`.
    Default,

    /// Supply protocol-defined set of bytes and a function that does some simple check against
    /// received handshake (or against our and their handshake together).
    Function(Vec<u8>, Box<dyn Fn(Vec<u8>) -> bool>),

    /// Send these bytes as the handshake and when a remote peer opens a substream for this
    /// protocol, don't perform any validation on it but instead emit `UncheckedNotificationStreamOpened`.
    ///
    /// `UncheckedNotificationStreamOpened` is otherwise identical to `NotificationStreamOpened`
    /// but instead of `Roles` field, it would contain a field for the received handshake which
    /// the protocol would then check itself. If the handshake is valid, the substream is simply
    /// kept open and if the handshake is invalid, the protocol would report and disconnect the peer.
    Unchecked(Vec<u8>),
}

The names are dumb but hopefully still convey the general idea.i When the notification protocol is installed, it would select one of these three methods for handling handshake and apart from block announcement protocol, all protocols would default to the Role-based checking for now. Currently there is no use for the second handshake type so that could be scraped all together. Allowing the protocol to do more complex validation, as it is with the block announcement protocol and just emitting this unchecked event, would keep the handshake validation simple and flexible.

We could also keep the handshake validation completely in Protocol but that would require async ping-ponging between Protocol and whatever the installed protocol is, because at least the block announcement protocol requires information about current syncing state which Protocol would not have.

The NetworkNotification trait is also updated to allow protocols to change the handshake they are currently using or simply update the current handshake bytes, as is the case for the block announcement protocol.

pub trait NetworkNotification {
    /// Set new handshake for protocol.
    fn set_notification_handshake(&self, protocol: ProtocolName, handshake: Handshake);

    // rest of the functions
}

Thoughts @tomaka @bkchr ?

[tomaka]

The biggest difficulty in this issue is that we want the handshake generation and validation to be asynchronous.

The main objective of this issue isn't really to check whether a handshake is valid or not, but more to accept/deny the substream based on the context, and if you can't even do any asynchronous operation then you're very limited.

Also, ideally, you could generate a different handshake based on the context and the peer, instead of sending the same handshake all the time.

[bkchr]

Can we not move the generation and validation of handshakes to the specific notification protocol implementation?

[tomaka]

Currently there is no use for the second handshake type so that could be scraped all together.

I didn't catch that sentence, but the reason why it's not in use is because it's not possible. If it was, we'd clearly use this feature. Making this possible is the entire point of the issue.

[bkchr]

But moving the validation to the notification protocol should be fairly simple or not? (I mean I don't know the exact details) But can you not put the handshake alongside the connected (or whatever the name is) event?

[altonen]

@tomaka

I think this scheme would make it asynchronous as the handshake would be sent to the protocol for validation and result of the validation would basically be that the substream is kept open or it's closed.

Also, ideally, you could generate a different handshake based on the context and the peer, instead of sending the same handshake all the time.

Could you elaborate on this? This scheme would allow the protocol to update the handshake bytes and that is what the block announcement protocol is currently doing by calling NetworkNotification::set_notification_handshake()

I didn't catch that sentence, but the reason why it's not in use is because it's not possible. If it was, we'd clearly use this feature. Making this possible is the entire point of the issue.

It is possible but it's not very useful, like it could check if the handshake contains some protocol-defined magic bytes. I was playing around with some code that would make this function asynchronous so Protocol would call this with the received handshake and push the function to FuturesUnordered and wait for the protocol to validate and send the response back but I felt it was unnecessarily complicated when compared to sending the handshake bytes to the protocol and letting it do the checking and closing the substream using NetworkPeers::disconnect_peer() if the validation failed.

@bkchr yes we can and that is what I'm proposing here

[bkchr]

@bkchr yes we can and that is what I'm proposing here

Okay thank you. This wasn't totally clear for me, given your first post. Especially as you had included a variant that would have the validation function attached to it.

[tomaka]

But moving the validation to the notification protocol should be fairly simple or not? (I mean I don't know the exact details) But can you not put the handshake alongside the connected (or whatever the name is) event?

It is fairly simple to move it away, but it's very hard to make it asynchronous.

If this issue was simply about passing a Box<Fn(&[u8]) -> bool to the networking, I would have done it myself a long time ago.

I think this scheme would make it asynchronous as the handshake would be sent to the protocol for validation and result of the validation would basically be that the substream is kept open or it's closed.

This is not very useful, as the remote will believe that it has been accepted while it has in reality been denied.

What is the remote supposed do if it has already started sending notifications?
How would it differentiate whether the substream has been refused, in which case the notifications it has sent haven't been received, or if the substream has been gracefully closed, in which case the notifications it has sent have been received?

The objective of this issue isn't to check an arbitrary checkmark "is the handshake configurable?". The objective is to make things properly, and making it properly involves making the handshake configurable, which is why the issue has been opened.

It is possible but it's not very useful

If I remember correctly, the collation protocol currently sends a second handshake (they don't call it like that, but it's effectively a second handshake) after the substream has been opened precisely because it isn't possible to customize the actual handshake.

[altonen]

I looked into the implementation and what seems to happen, and correct me if I'm wrong, is that peer opens a substream, that gets validated on remote side and it's either accepted (the initiator is not informed) or it is rejected and the substream is closed. This proposal just moves the decoding of bytes to the actual protocol implementation and if it fails there, it protocol is closed just the same way. The decision whether to keep the substream open or closed is moved up in the stack and handshake bytes are modified to contain whatever the protocol wants, Protocol just acts as the intermediator.

[tomaka]

and it's either accepted (the initiator is not informed) or it is rejected and the substream is closed

The substream is accepted by sending back a handshake (the handshake can be empty, in which case a single 0 byte indicating the length is sent back). The substream is rejected by closing the substream without sending back a handshake.

I have trouble interpreting your sentence because the words "open" and "closed" are ambiguous here.
First, the "low-level substream" is opened but the "notifications substream" (as a higher level API concept) isn't opened yet, and only after a handshake has been sent back by the receiver the "notifications substream" is now opened.
If the "low-level substream" is closed before the "notifications substream" is opened, then the "notifications substream" isn't closed in the sense that it was never opened.

[bkchr]

The substream is accepted by sending back a handshake (the handshake can be empty, in which case a single 0 byte indicating the length is sent back)

But can we not defer sending back the handshake until the upper layers have accepted the handshake?

[tomaka]

But can we not defer sending back the handshake until the upper layers have accepted the handshake?

Yes, that's exactly the objective of this issue. This is different from what I understand @altonen suggested which is to always send back the handshake to the remote and then later close the substream if its handshake fails validation.

All I'm saying is: either you make the handshake validation synchronous, in which case it's relatively easy to implement but not very useful, or you make the handshake validation asynchronous, in which case it's very complicated to implement.

[altonen]

I still need some time to look into this but when I'm looking at this code, it seems me that the handshake is already either sent back at that point and we close the substream which leads me to me believe this is incorrect if I've understood your point @tomaka correctly. If this code is correct, however, I don't see why we couldn't move the decoding and checking to a separate protocol and close the substream there.

[tomaka]

Indeed, in the code that you've linked we have already sent back the handshake and we close the substream. The reason why we do that is precisely because it's not possible to do it correctly. So yes this code is not correct (and the fact that it is done this way actually causes a cascade of small issues which are a bit long to explain here).

The place where substreams are accepted/denied is the behavior.rs file. When a substream is opened by the remote, it notifies the peerset, and the peerset will later accept or reject it.

So it's already asynchronous, the only problem is that it's the peerset which decides to accept/reject instead of the outer API layers. It does so based purely on whether there is an available slot or whether the peer is reserved.

I see two ways to implement this:

• Add another step before accepting/refusing. When the peerset accepts a substream, we then ask the outer API layers, and only when the outer API layers accept the substream we truly accept it.
• Replace the concept of peerset entirely with the outer API layers. I think it wouldn't be a bad idea if it was also the outer API layers which decide whether there is any slot available, in addition to validating the handshake.

The first option is complicated as you would need to touch the behavior.rs file quite a lot, and it has caused a lot of bugs in the past whenever it has been modified. The second option looks more elegant, however it is really difficult to design a public API for that. I don't remember the details, but there are a lot of race conditions that you would need to handle.

[altonen]

@tomaka Thanks a lot for this information! I'll probably explore both options to see which is the better path forward

[Polkadot-Forum]

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v1-5-0/5358/1