Support configuring a different protocol id

[LNSD]

Background

I am working on rust-waku and, more precisely, on the 33/WAKU2-DISCV5 RFC implementation.

The reference implementation, the Nim implementation (nwaku), of the Waku discv5 node discovery mechanism uses a different protocol ID for convenience. This is d5waku.

Description

Right now, in v0.1.0, the PROTOCOL_ID is hardcoded:

https://github.com/sigp/discv5/blob/v0.1.0/src/packet/mod.rs#L32-L33

To make rust-waku compatible with the Waku reference implementation, I would like to add support to configure the protocol ID (similar to what rust-libp2p does with the Gossipsub protocol).

Acceptance criteria

• Add the protocol_id field to Discv5Config. The default value must be "discv5"
• Add a matching protocol_id function to the Discv5ConfigBuilder.
• Add a protocol_id field to packet::{Packet, PacketHeader}.
• Encapsulate the packet::Packet codec logic to support checking the message validity against non-constant (non-static) values.

[AgeManning]

Hey, I have no issues helping support waku.

As you probably know, the protocol-id is currently hard-coded because its "hard-coded" in the discv5 specifications. Technically if we change this protocol-id we will no longer be compliant with the discv5 specifications.

I have a preference to avoid having configurations that allow users to become non-spec compliant. Also some users may tamper with the config and end up sending invalid packets.
I've recently had some issues with boot-nodes sending invalid packets, but because I know there is no configuraiton in rust-discv5 that permits this kind of behaviour, its easier to debug and assume its another software. If we had this configuration, then when a node is sending back invalid packets, an invalid configuration could also be the cause.

As far as I know, this is the only request to change the protocol-id and would therefore be the only use-case for this config. If more people require it, I think maybe we should consider adding the config as you've suggested.

In the meantime, how do you feel about a compile-time feature?

We could add a "waku" feature, which when set, sets the protocol-id to "d5waku".

Let me know what you think

[AgeManning]

This feature-flag could then also be used for various ad-hoc modifications you may need to make to the specification and code logic that may fall outside the regular user config

[LNSD]

As you probably know, the protocol-id is currently hard-coded because its "hard-coded" in the discv5 specifications. Technically if we change this protocol-id we will no longer be compliant with the discv5 specifications.

Absolutely. Makes sense.

If we change the PROTOCOL_ID or the VERSION, we are talking about a different protocol (and, in practice, a different Discv5 network).

In the meantime, how do you feel about a compile-time feature?

It is a possibility that I had in mind. We are doing it with compile-time defines in the Nim implementation of Discv5 (here).

Cargo does not have "compile-time defines", so AFAIK the alternative is to use a compile-time feature. And set it to the discv5 dependency via a feature (e.g., features = ["waku"]).

I think it feels hackish just to tune the PROTOCOL_ID and the VERSION, but following your rationale on the Discv5 spec compliance is the only option I see.

[LNSD]

I have a preference to avoid having configurations that allow users to become non-spec compliant.

When you talk about users, are you thinking about the Lighthouse client users? Or projects that use this crate as a dependency?

Also some users may tamper with the config and end up sending invalid packets. I've recently had some issues with boot-nodes sending invalid packets, but because I know there is no configuraiton in rust-discv5 that permits this kind of behaviour, its easier to debug and assume its another software.

With my suggestion to add a Discv5 configuration parameter, you could always use the default configuration of the rust-discv5 crate (with the discv5 protocol identifier) in the application's discovery service instantiation without exposing it externally in the executable configuration options. In that case, users won't be able to tamper with the protocol Id.

If we had this configuration, then when a node is sending back invalid packets, an invalid configuration could also be the cause.

I understand that you don't want to add additional debug headaches. Me neither 😁

I see that packets are just dropped/marked as invalid if the PROTOCOL_ID or the VERSION does not match with the one hardcoded. I am interested in keeping this logic.

//...

// Check the protocol id
if &static_header[..6] != PROTOCOL_ID.as_bytes() {
  return Err(PacketError::HeaderDecryptionFailed);
}

// ...

Keeping that logic, you should not see any interference. And you can continue assuming, what you said:

I've recently had some issues with boot-nodes sending invalid packets, but because I know there is no configuraiton in rust-discv5 that permits this kind of behaviour, its easier to debug and assume its another software.

[LNSD]

In my opinion, it should be the application (in your case, the lighthouse client, in my case rust_waku's node) responsible for configuring the protocol_id parameter, not the final application user.

[LNSD]

I was thinking about this:

As you probably know, the protocol-id is currently hard-coded because its "hard-coded" in the discv5 specifications. Technically if we change this protocol-id we will no longer be compliant with the discv5 specifications.

I think it happens the same with rust-libp2p's gossipsub, but they allow configuring a different protocol id via the GossipsubConfigBuilder: GossipsubConfigBuilder::protocol_id

Changing the protocol_id makes the protocol "diverge" from the Gossipsub spec, but enables other use cases (e.g., extend the protocol by wrapping the current implementation).

What do you think? 🤔

[AgeManning]

There's a few things here. Let me try and be thorough in my current thought process.

Firstly. My terminology of users was referring to applications that use this crate, i.e Lighthouse, Waku, Portal etc, not the end user. So I fully agree that final end-users shouldn't be messing with the protocol-id, my argument was more that potentially projects also shouldn't be messing with the protocol-id, let me give some reasons in this reply.

Firstly let me address some things you've raised:

Cargo does not have "compile-time defines", so AFAIK the alternative is to use a compile-time feature. And set it to the discv5 dependency via a feature (e.g., features = ["waku"]).

Yeah a feature flag would look like that, and you could import discv5 with this feature.

I see that packets are just dropped/marked as invalid if the PROTOCOL_ID or the VERSION does not match with the one hardcoded. I am interested in keeping this logic.

Yep, this is the logic that causes some headaches. The messages are encrypted initially with the node-id you are sending a packet to. So when we receive a packet, it looks like junk, then we decrypt it, assuming it was sent to our node-id. Once decrypted, we can check to see if the decryption worked, we do this by looking for the magic bytes which are the protocol-id. If the protocol-id is not what we expect, then either the message is junk, there is an error in the packet format being sent, or it was encrypted with some other node-id and not meant for us. In the current implementation we can't tell the difference between these cases. Modifying the protocol-id is one way to segregate nodes from communicating with each other.

You brought up libp2p-gossipsub. So this protocol is entirely different and the protocol-id there is used in libp2p's multistream select, which is more-or-less designed to be configurable to identify capabilities of a node. The libp2p gossipsub specs are very quite light: https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.0.md and do not specify the protocol-id. It has been known amongst implementors that the true specification is the go implementation of gossipsub.

Another important difference with gossipsub, is that its not desired to have multiple applications using the one gossipsub network. For this reason, I had added a configurable parameter in the gossipsub configuration called the protocol-id prefix, which prefixed the "meshsub" protocol-id allowing applications to segregate themselves on the gossipsub network. There were some issues about this amongst implementors and it was agreed as a reasonable thing to do in libp2p to customise the protocol-id. So because the specification there is more based on the implementation, and as far as I know there is no specification that fixes it to a specific value, it's apart of the specification to customize it.

Recently, another user wanted to customise the entire protocol-id, which will break the protocol in some weird ways (as we need to identify different versions of gossipsub nodes on the network). I highlight the issues here: libp2p/rust-libp2p#2718 (review) but in the end decided it would be fine if that user wanted to keep the nodes on the same version across the network.

Going back to discv5. The wire-protocol specifications explicitly state the bytes that the protocol-id should be (unlike the gossipsub "specs"). I'm not the author of the discv5 specs, but as I understand the reason for this, is that it is desired use of discv5 is to build a giant DHT of many many nodes from all applications. For kad-like protocols, the larger the network, the more secure. Therefore, I think the goal of discv5 is for all applications to use the one DHT, which is probably why modifying the VERSION and PROTOCOL_ID feels a bit hacky, because I don't think it was designed to allow nodes to segregate themselves from participating in the DHT.

I understand the initial desire to form your own DHT as it can be faster to find nodes that advertise a particular capability (i.e waku nodes). There has been a bunch of research of how best to do this over a very large DHT with many applications. A paper has been written on how to implement "topics" for faster searches of this kind: https://github.com/harnen/service-discovery-paper . We have made some initial implementations of this here: #126

Another way to segregate DHTs is to use this: https://github.com/sigp/discv5/blob/master/src/config.rs#L61. I originally added this for Ethereum, but decided against its use due to the security risks of a decreased DHT size. In ethereum we add an eth2 field in the ENR and using this configuration it would be possible to segregate DHTs by only including ENRs in our routing table that contained this field. Waku could do this also, without having to modify raw protocol rpc messages.

In the end, I think the general idea is that all applications should use the one DHT and therefore keep everything "to-spec". Making the protocol-id in discv5 configureable feels to me to go against the design of the protocol and I'd lean towards making a feature-flag to do it, however if you really want the config, I'm not going to die on this hill :).

I'd be curious to here @fjl thoughts on this (being the primary author of discv5).

[AgeManning]

@LNSD - You can ignore the essay above ^ :p.

I had a quick chat with @fjl.

As I understand the go-implementation now allows the protocol-id to be customized and most of my concerns about the intention of the spec are probably exaggerated. If other nodes are customising this, then makes sense that we should too.

So lets make the config 🎉

[fjl]

I agree with the spirit of @AgeManning's post. Customizing the protocol-id is not intended by the spec. However, I decided to accept the feature in go-ethereum because it can be useful during development or for experiments.

[LNSD]

Thanks for such an elaborate answer. It was very helpful to understand your concerns.

So let's make the config 🎉

Great! In the following days, I will go through the discv5's packet crate in order to see how we can encapsulate the encoding/decoding logic so we can pass the configuration to it.

P.S. I was aware of the libp2p's Gossipsub protocol_id PR since it was motivated by one member of the Waku community that was trying to implement his version of the Waku relay protocol in rust 🙂

[LNSD]

I will go through the discv5's packet crate in order to see how we can encapsulate the encoding/decoding logic so we can pass the configuration to it.

Initially, my idea was to extract the serialization logic into a codec. But with the current code layout, it gets too complicated. So I opted for a "surgical" change, consisting of passing the protocol id down the call stack until reaching the encode and decode functions.

Please, check PR #157, and let me know your thoughts.

[LNSD]

Closing this issue as #158 got merged