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

Change the name of the GrandPa notifications protocol #7252

Closed
tomaka opened this issue Oct 2, 2020 · 8 comments · Fixed by #10463
Closed

Change the name of the GrandPa notifications protocol #7252

tomaka opened this issue Oct 2, 2020 · 8 comments · Fixed by #10463
Assignees
Labels
Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder

Comments

@tomaka
Copy link
Contributor

tomaka commented Oct 2, 2020

At the moment, the GrandPa protocol is called /paritytech/grandpa/1. It's not great for two reasons:

  1. It shouldn't contain the word paritytech.
  2. Since the messages exchanged are specific to a certain chain, its name should contain the protocol ID.

Reason 2) will be important in the future, when nodes can be connected to multiple chains at the same time.

@tomaka tomaka added this to Triage in Networking (Outdated) via automation Oct 2, 2020
@tomaka
Copy link
Contributor Author

tomaka commented Oct 25, 2020

The fix needs to involve a transition period where the protocol will have two names. As such, this 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
@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
@acatangiu
Copy link
Contributor

  1. It shouldn't contain the word paritytech.

Using a new static /substrate/grandpa/1 protocol name is easy AFAICT:
just use new protocol name, and add old protocol name (/paritytech/grandpa/1) to fallback_names in sc_finality_grandpa::grandpa_peers_set_config()

  1. Since the messages exchanged are specific to a certain chain, its name should contain the protocol ID

If we want to customize the grandpa protocol name to have chain-specific ID (e.g. /polkadot/grandpa/1), then we'd need the chain ID in places such as client/finality-grandpa/src/lib.rs::run_grandpa_voter().

Where is the chain ID specified, what can be used as a chain-specific ID?

@tomaka
Copy link
Contributor Author

tomaka commented Dec 9, 2021

We have this thing called the protocolId in the chain specification. The sync and transaction protocols already use this protocolId in their name.

However we have a plan to use the genesis hash instead of the protocolId, and it probably makes more to transition Grandpa to use the genesis hash: #7746

@tomaka
Copy link
Contributor Author

tomaka commented Dec 9, 2021

Using a new static /substrate/grandpa/1 protocol name is easy AFAICT:
just use new protocol name, and add old protocol name (/paritytech/grandpa/1) to fallback_names in sc_finality_grandpa::grandpa_peers_set_config()

Yes

@burdges
Copy link

burdges commented Dec 9, 2021

Assuming this is not pressing, we might consider merging this transition with a switch to Rabin-Williams signatures or something else faster than ed25519. RustCrypto/RSA#118

This is based upon my understanding that grandpa spends a lot of CPU on signature verification.

@tomaka
Copy link
Contributor Author

tomaka commented Dec 10, 2021

We can always transition again from /blablabla/grandpa/1 to /blablabla/grandpa/2 in the future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants