Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Idea: Replace protocolId with the genesis hash #7746

Closed
tomaka opened this issue Dec 17, 2020 · 15 comments · Fixed by #12545
Closed

Idea: Replace protocolId with the genesis hash #7746

tomaka opened this issue Dec 17, 2020 · 15 comments · Fixed by #12545
Assignees
Labels
J0-enhancement An additional feature request.

Comments

@tomaka
Copy link
Contributor

tomaka commented Dec 17, 2020

At the moment, most networking protocols contain what is called the protocolId, defined in the chain specs.
For example, the syncing protocol of Polkadot is named /dot/sync/2, because Polkadot has protocolId: "dot" in its chain specs.

Instead of using a user-chosen protocolId, we could name the protocol using the hash of the genesis block.
The sync protocol could for example become /91b171bb158e2d3848fa23a9f1c25182fb8e20313b2c1eb49219da7a70ce90c3/sync/2.

The advantage of doing so is that things become "automatic". No need to define your own protocolId anymore, and no need to verify whether the genesis hash is the same between two nodes during the blocks announce handshake.

The drawback is obviously that this is longer than just dot, so more bytes are transmitted on the network. The multistream-select protocol requires protocol names to be string.
To mitigate this, we could use an encoding other than hex encoding of the hash. Using a different encoding might however make things confusing.

This also depends on how we tackle #7458. After #7458, the definition of "genesis block" might become a bit blurry.

@tomaka tomaka added the J0-enhancement An additional feature request. label Dec 17, 2020
@tomaka
Copy link
Contributor Author

tomaka commented Mar 9, 2021

Depends on #6605

@tomaka tomaka added the Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder label May 3, 2021
@tomaka
Copy link
Contributor Author

tomaka commented May 3, 2021

Some remark: using just the genesis hash would make it impossible to do a hard fork.
We should use the combination of the genesis hash and some optional value found in the chain specs.

Adding an extra field also makes it flexible enough to not run into an issue if a chain transitions from standalone to parachain.

@stale
Copy link

stale bot commented Jul 7, 2021

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.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 7, 2021
@tomaka
Copy link
Contributor Author

tomaka commented Jul 8, 2021

Issue still relevant and important.

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 8, 2021
@nuke-web3
Copy link
Contributor

nuke-web3 commented Oct 9, 2021

I am writing some guides on parachain configuration, and especially in that context, having unique network protocol IDs seems more important, and as you mention, accounting for the possibility of hard forks and changes in paraID and such.

We have seen solo-chains run into improper discovery & attempted peering because of this, and my hunch at least is in a common-relay chain context, this will be exacerbated.

@acatangiu
Copy link
Contributor

At the network level, when looking at scale wouldn't it be expensive to string match against such a long protocol id?

Would protocolId + truncatedGenesisHash like /dot/91b171bb1/sync/2 also work?

@tomaka
Copy link
Contributor Author

tomaka commented Dec 9, 2021

There's no need to truncate. We don't care about the performance of comparing strings of 50 bytes.

@acatangiu
Copy link
Contributor

Done in #10463

@acatangiu acatangiu self-assigned this Jan 5, 2022
@tomaka
Copy link
Contributor Author

tomaka commented Jan 6, 2022

This was done for Grandpa/Beefy, but not for the syncing and transactions protocols.

@tomaka tomaka reopened this Jan 6, 2022
@acatangiu acatangiu removed their assignment Jan 6, 2022
@andresilva andresilva moved this to Backlog 🗒 in SDK Node Apr 26, 2022
@the-right-joyce the-right-joyce removed the Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder label Apr 26, 2022
@tomaka
Copy link
Contributor Author

tomaka commented Aug 19, 2022

One last thing to do is change the Kademlia protocol name:

config.set_protocol_name(proto_name);

Unfortunately, this is maybe more complicated, because libp2p doesn't allow setting backwards compatible names for this protocol.

@bkchr
Copy link
Member

bkchr commented Aug 20, 2022

One last thing to do is change the Kademlia protocol name:

config.set_protocol_name(proto_name);

Unfortunately, this is maybe more complicated, because libp2p doesn't allow setting backwards compatible names for this protocol.

@dmitry-markin could you check with upstream if adding support for backwards compatible names would be accepted? If yes, could you please do this.

@dmitry-markin
Copy link
Contributor

@dmitry-markin could you check with upstream if adding support for backwards compatible names would be accepted? If yes, could you please do this.

Done in libp2p/rust-libp2p#2846.

@bkchr
Copy link
Member

bkchr commented Sep 3, 2022

@dmitry-markin could you check with upstream if adding support for backwards compatible names would be accepted? If yes, could you please do this.

Done in libp2p/rust-libp2p#2846.

Nice! So we will be able to close this with the next libp2p update.

@mxinden
Copy link
Contributor

mxinden commented Sep 6, 2022

with the next libp2p update

For the protocol: libp2p/rust-libp2p#2869

@bkchr
Copy link
Member

bkchr commented Sep 6, 2022

@dmitry-markin please keep track on this and when it is released, please update libp2p. :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
J0-enhancement An additional feature request.
Projects
Status: done
Development

Successfully merging a pull request may close this issue.

8 participants