ProtocolId re-work

[realbigsean]

Description

ProtocolId contains a single Version enum for all protocols, this doesn't really make sense though because protocols are versioned independently. Places where this isn't great include #3972 where we want a complete match across protocol + version, but currently this means matching on protocol + version combinations that don't actually exist, e.g. LightClientBootstrap + Version::V2. Also handle_v1_response the handle_v2_response end up being very confusing to work with because some v1 protocols end up having functionality more similar to v2 protocols (e.g. fork-awareness in the blobs by range v1 protocol is almost the same as fork-awareness in blocks by range v2 protocol and very unlike the blocks by range v1 protocol) but have to be handled independently.

Options:

• Add variants per-version to Protocol. Maximally strict but maybe makes the most sense.

enum Protocol {
    MetadataV1,
    MetadataV2,
...
}

• Nest version enums inside each protocol. More verbose, but more flexible

enum Protocol {
    Metadata(MetadataVersion),
    BlocksByRange(BlocksByRangeVersion),
...
}

[winksaville]

Is there any interest in solving this on a wider scale. In particular I'd like to be able to uniquely identify items in an agnostic way. The minimum set of groupings I see are:

• Messages
• Protocols - a set of messages
• ProtocolSets - a set of protocols

Each would have a unique identifier and semver. Again, these should be defined in a "language" and "platform" agnostic manner so as to be maximally portable.

[realbigsean]

@divagant-martian @AgeManning what do you guys think?

[divagant-martian]

Thanks @realbigsean and @winksaville

Is there any interest in solving this on a wider scale. In particular I'd like to be able to uniquely identify items in an agnostic way. The minimum set of groupings I see are:

* Messages

* Protocols - a set of messages

* ProtocolSets - a set of protocols

The ProtocolId is an internal model used for protocol negotiation with other peers, so this issue is more of a code quality one than an API one. In this sense I don't think we will be adding a higher hierarchy (ProtocolSets)

Each would have a unique identifier and semver. Again, these should be defined in a "language" and "platform" agnostic manner so as to be maximally portable.

We already model messages as the individual chunks received per protocol and don't support protocols depending on language or platform

ProtocolId contains a single Version enum for all protocols, this doesn't really make sense

Yeah agree, let's change this

[winksaville]

I believe I understand there is one Protocol with a set of messages identified by:

lighthouse/beacon_node/lighthouse_network/src/rpc/protocol.rs

Lines 141 to 158 in 38514c0

	/// Protocol names to be used.
	#[derive(Debug, Clone, Copy, PartialEq, Eq)]
	pub enum Protocol {
	/// The Status protocol name.
	Status,
	/// The Goodbye protocol name.
	Goodbye,
	/// The `BlocksByRange` protocol name.
	BlocksByRange,
	/// The `BlocksByRoot` protocol name.
	BlocksByRoot,
	/// The `Ping` protocol name.
	Ping,
	/// The `MetaData` protocol name.
	MetaData,
	/// The `LightClientBootstrap` protocol name.
	LightClientBootstrap,
	}

I was thinking there were many protocols and many combinations of messages.

[divagant-martian]

That's the Protocol enum that we use to identify which of the different 7 protocols is in use. Messages are handled per request, you can check the InboundRequest and OutboundRequest to check what messages are sent in each. We might be referring to "Protocol" to different things

[winksaville]

SG, txs.

I think we have similar models in mind. Your enum Protocol I call a ProtocolSet. It's a set of Protocols like "Status", "Goodbye", ... and then each of those protocols defines a small set of request/response messages. Because I envision many ProtocolSets there will be a semver, but since you have only one ProtocolSet, I too agree semver is not necessary.

[PatStiles]

I'm interested in taking this on if it hasn't already been taken by @winksaville

[winksaville]

I won't have time, please take it on!

[AgeManning]

My 2 cents on this topic is:
Libp2p doesn't distinguish between any of the abstract values we've added (i.e protocol, version, encoding, string_prefix). It just cares about matching random bytes. So if our abstraction is causing pain, we should modify the abstraction a little.

At the moment, we only really use one encoding, we removed the support of plain ssz. So currently there is pain matching on just protocol and version, and I assume not for the encoding.

We build a list of supported protocols using the raw abstract types (Protocol, Version, Encoding) which we advertise to our peers. I think it might be nicer then, to create a new abstract enum called SupportedProtocol which contains all the variants that are supported (I know some can be turned off/on at runtime but the matching pattern will no longer have crazy variants as described in this issue).

Then when building the supported protocols here:

lighthouse/beacon_node/lighthouse_network/src/rpc/protocol.rs

Line 223 in 38514c0

let mut supported_protocols = vec![

We could use this enum, i..e SupportedProtocol::currently_supported(info: RPCProtocol) and we could implement something like:

impl ProtocolId {
   fn supported_protocol(&self) -> SupportedProtocol { .. }
   }

So when we are checking the match, we convert the raw ProtocolId into a SupportedProtocol and match on that to avoid any non-existent variant.

[divagant-martian]

This can be closed now