allow custom gossipsub protocol id

[bernardoaraujor]

Description

Currently, the only way to set a GossipSub protocol ID is via GossipsubConfigBuilder::protocol_id_prefix(), which uses the input as a mere prefix, and the resulting protocol ID is in the format /{prefix}/1.x.0 .

This PR introduces the possibility of custom GossipSub protocol IDs, to be set via GossipsubConfigBuilder::protocol_id().

If GossipsubConfigBuilder::protocol_id() is called, then GossipsubHandler is created via GossipsubHandler::new_custom and whatever was previously set via GossipsubConfigBuilder::protocol_id_prefix() is simply ignored.

Links to any relevant issues

#2709

Open Questions

The existing test-cases all pass.
I'm also able to successfully test on the use-case described on #2709

Is it necessary to add more test cases?
The only tests I could find that operate on a similar level were create_config_with_message_id_as_*, however that was still not exactly the same thing.

I'm also not sure what are the appropriate crates for a changelog entry.

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

[MarcoPolo]

Haven't looked too closely at this yet, but:

Is it necessary to add more test cases?

Yes, please do.

I'm also not sure what are the appropriate crates for a changelog entry.

https://github.com/libp2p/rust-libp2p/blob/0d9bf4f2e785cf09042b3e147d18a5c7462929ee/protocols/gossipsub/CHANGELOG.md See https://github.com/libp2p/rust-libp2p/blob/master/docs/release.md#development-between-releases for more details.

I wonder if the change can be simplified if you did the prefix logic sooner. Create the protocol id at the beginning and use that later on rather than adding the x_custom variants to functions.

[thomaseizinger]

As far as I know, the current functionality of only configuring the prefix was introduced because that is the minimal amount of configuration needed to allow re-use of the code between floodsub, gossipsub v1 and gossipsub v1.1.

If we now want to support entirely custom protocols, I think a cleaner way of achieving this would be to build up the list of protocol strings in the GossipsubConfigBuilder and pass the final list down to the handler.

Is this what you also meant @MarcoPolo?

Also tagging @AgeManning as the original author.

[MarcoPolo]

If we now want to support entirely custom protocols, I think a cleaner way of achieving this would be to build up the list of protocol strings in the GossipsubConfigBuilder and pass the final list down to the handler.

Is this what you also meant @MarcoPolo?

Yup. Thank you

[AgeManning]

The reason I went with just a prefix is to allow users to form their own networks and not be stuck connecting to other gossipsub peers on the same network. I kept the version and general format because there were live networks hosting a mix of gossipsub versions.

There's a few things I think we need to consider for this change:

1. What versions does a custom protocol-id support? - The current design optionally supports floodsub, gossipsub 1.0 and gossipsub 1.1. It will advertise these protocol-ids such that by default this can connect to networks that have old versions of gossipsub. If we use a custom protocol id without versioning, what versions specifically should this support? There are protobuf message changes between 1.0 and 1.1 (such as peer exchange in pruning) and currently every node that connects to us we identify which version they are (by the protocol-id) so we know what messages we can/cannot send. There is currently work being done on a v 1.2 that is backwards compatible. For 1.2, we can just add a new protocol-id to the list and add these nodes to existing networks and have it work fine. If we say that a custom protocol id only supports 1.1 and nothing else, these networks will not be upgradeable or work well when 1.2 gets released. The reason only the prefix was given to users is to allow for this upgradability of networks. If we want to add a custom protocol id that can be versionless and just uses the latest version of gossipsub, then when gossipsub updates it will not work with previous networks and/or the user must manually adjust the custom protocol-id of the network to distinguish versions. I think we should be explicit of how we want to handle this.
2. We currently use the protocol-id list to identify each node that connects to us. We then preform different logic in the behaviour based on the version of the node. If this PR wants the PeerKind::Custom variant to behave as a v1.1 node, some of the behaviour match statements need to be modified to reflect this.
3. As @MarcoPolo points out, i think the code can be simplified. I dont think we need two functions to instantiate a handler. We just need to make a decision of what versions PeerKind::Custom is actually going to support. You could manually disable floodsub support and only support v1.1 and use the functions that already exist. Again this may run into complications when the versions get updated. I think that if the decision is that the custom protocol id, just behaves as a 1.1 node, then we don't need the Custom variant, and you can just set the protocol id to be a 1.1 node (this would simplify the code).

The fundamental issue I think is that gossipsub needs to be able to distinguish other versions of itself in order for us to upgrade gossipsub and still be backwards compatible with existing networks. If your network removes the gossipsub versioning, you may run into issues with future gossipsub versions working on your network. If this is really what the network wants to do, perhaps the best way is to just set the custom protocol-id to be the protocol-id for the current version (currently PeerKind::Gossipsub1_1, but this will probably change in the future to Gossipsub1_2, when its complete).

[bernardoaraujor]

@AgeManning thanks for the feedback.

I introduced GossipsubVersion enum to config.rs. That way, whenever the user makes the conscious choice of using a custom protocol id (by calling GossipsubConfigBuilder::protocol_id instead of GossipsubConfigBuilder::protocol_id_prefix), they have to provide either GossipsubVersion::V1_0 or GossipsubVersion::V1_1 (or GossipsubVersion::V1_2 in the future) as argument.

During the construction of ProtocolConfig, this enum is matched so that if a custom protocol id is to be set, only a single PeerKind will be used.

[thomaseizinger]

Thanks! I think this is much simpler already :)

One question.

[AgeManning]

Yep this looks good to me. Thanks :)

[mxinden]

Needs a changelog entry. Otherwise looks good to me.

Thanks for the collaboration here everyone.

[bernardoaraujor]

@mxinden done.

[mxinden]

@bernardoaraujor could you commit after running Rust fmt on your changes?

[bernardoaraujor]

@mxinden done.