Name changes for GrandPa and Beefy notifications protocols #10463

acatangiu · 2021-12-10T12:05:05Z

polkadot companion: paritytech/polkadot#4519
cumulus companion: paritytech/cumulus#870

First commit still uses static protocol name, but removes paritytech.

Following commits add support for building dynamic protocol name using /<genesis-hash> prefix. /<fork-id> is also appended to the prefix if the chain has forked.

Introduces fork_id in the substrate ChainSpec definition.

Fixes #7252
Fixes paritytech/grandpa-bridge-gadget#240
Related to #7746

tomaka · 2021-12-10T13:39:03Z

I think that the protocolId should not be re-purposed as a "hard fork identifier". We should instead introduce a new optional field in the chain spec and deprecate protocolId.

The reasons are:

We've written documentation and guides about this protocolId, and the new role of this hard fork identifier is different enough that it would just be confusing (example)
Most chains will never encounter any hard fork, and I don't see why you'd need to use both the genesis hash plus an arbitrary string, rather than just the genesis hash. So the default value for this new hard fork identifier should be None, meaning it's not used. And we would like all existing chains (assuming they're not a hard fork) to switch to a value of None rather than keep their existing protocolId.

tomaka · 2021-12-10T13:40:26Z

A minor unrelated point is that I think it should be /<hash>/<id> and not /<id>/<hash>
It is possible to have multiple ids with the same hash, but (normally) never multiple hashes with the same id.

acatangiu · 2021-12-13T12:21:02Z

@tomaka were you thinking something like this: d4b616a ?

tomaka · 2021-12-13T12:29:05Z

Yes!

'fork_id' is used to uniquely identify forks of the same chain/network 'ChainSpec' trait provides default 'None' implementation, meaning this chain hasn't been forked.

tomaka · 2021-12-13T16:02:13Z

Could you please not force push? 🙏 I was in the middle of reviewing and this ruins reviews in progress.

client/chain-spec/src/lib.rs

bin/node-template/node/src/service.rs

client/finality-grandpa/src/communication/mod.rs

client/network/src/config.rs

client/finality-grandpa/src/communication/mod.rs

…-protocol-name

tomaka · 2021-12-14T10:48:12Z

👍 This looks good to me from a networking perspective, but I don't know what others think of the changes in bin/* and in Grandpa & Beefy.

bin/node/cli/src/service.rs

bin/node-template/node/src/service.rs

…-protocol-name

bkchr · 2022-01-05T15:32:05Z

bot merge

paritytech-processbot · 2022-01-05T15:32:14Z

Error: It's not possible to meet the minimal approval count of 1 in paritytech/polkadot#4519

bkchr · 2022-01-05T15:35:55Z

bot merge

paritytech-processbot · 2022-01-05T15:36:06Z

Error: Response error (status 404 Not Found):

{"documentation_url":"https://docs.github.com/rest/reference/pulls#get-a-pull-request","message":"Not Found"}

joao-paulo-parity · 2022-01-05T17:01:29Z

bot merge

xlc · 2022-01-05T21:53:25Z

Anything parachain teams needs to be aware of about this? Will this cause any compatibility issues?
Looks like this only touches Grandpa/Beefy which only exists on relaychain so there should be no effect on parachain?
It does introduce a fork id but it should be fully backward compatible if we just leave it to none?

bkchr · 2022-01-05T23:50:03Z

Anything parachain teams needs to be aware of about this? Will this cause any compatibility issues? Looks like this only touches Grandpa/Beefy which only exists on relaychain so there should be no effect on parachain? It does introduce a fork id but it should be fully backward compatible if we just leave it to none?

Nothing changes for parachains. You just use None for fork_id.

And the code is also backwards compatible for the relay chain.

…h#10463) * grandpa: update notif protocol name * grandpa: add chain id prefix to protocol name * grandpa: beautify protocol name handling * grandpa: prepend genesis hash to protocol name * chain-spec: add optional 'fork_id' 'fork_id' is used to uniquely identify forks of the same chain/network 'ChainSpec' trait provides default 'None' implementation, meaning this chain hasn't been forked. * grandpa: protocol_name mod instead of struct * beefy: add genesis hash prefix to protocol name * chainspec: add fork_id * grandpa: simplify protocol name * grandpa: contain protocol name building logic * beefy: contain protocol name building logic * grandpa: fix tests * fix merge damage * fix docs reference visibility Signed-off-by: acatangiu <adrian@parity.io> * Update client/finality-grandpa/src/lib.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Update client/finality-grandpa/src/communication/mod.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Update client/beefy/src/lib.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Update client/beefy/src/lib.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * avoid using hash default, even for protocol names Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

paritytech/substrate#10463

@weichweich

* chore: bump Polkadot dependencies to v0.9.16 * refactor: use AllPalletsWithSystem hook * chore: remove crowdloan pallet * fix: apply no DefaultAccount check * fix: switch to default MaxEncodedLen paritytech/substrate#10662 * fix: apply asset quota + runtime state_version quota: paritytech/substrate#10382 state: paritytech/substrate#9732 * fix: Preimage registrar and Scheduler integration Scheduler: paritytech/substrate#10356 OriginPriviligeCmp: paritytech/polkadot#4166 * fix: OrderedSet * fix: apply new fork_id to chainspecs paritytech/substrate#10463 * fix: apply no default account for SudoConfig * fix: apply name changes for GrandPa paritytech/substrate#10463 * fix: EnsureOneOf paritytech/substrate#10379 * fix: preimage weights * fix: parachain client * fix: clippy copied weights * fix: bump deps * tests: attempt staking fix * bench: attempt to fix default accountid for dids * fix: staking unit test pallet order execution * fix: did unit benchmarks * fix: fmt * fix: revert to old hook order execution * bench: run manually for preimage + scheduler * fix: only import TaskManager for try-runtime feature * fix: use correct scheduler migration + add logs * refactor: use explicit para runtime executors * fix: apply code review by @Diiaablo95 * chore: apply suggestion from @weichweich review * chore: bump deps * fix: deps

…h#10463) * grandpa: update notif protocol name * grandpa: add chain id prefix to protocol name * grandpa: beautify protocol name handling * grandpa: prepend genesis hash to protocol name * chain-spec: add optional 'fork_id' 'fork_id' is used to uniquely identify forks of the same chain/network 'ChainSpec' trait provides default 'None' implementation, meaning this chain hasn't been forked. * grandpa: protocol_name mod instead of struct * beefy: add genesis hash prefix to protocol name * chainspec: add fork_id * grandpa: simplify protocol name * grandpa: contain protocol name building logic * beefy: contain protocol name building logic * grandpa: fix tests * fix merge damage * fix docs reference visibility Signed-off-by: acatangiu <adrian@parity.io> * Update client/finality-grandpa/src/lib.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Update client/finality-grandpa/src/communication/mod.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Update client/beefy/src/lib.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Update client/beefy/src/lib.rs Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * avoid using hash default, even for protocol names Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

acatangiu self-assigned this Dec 10, 2021

acatangiu requested a review from tomaka December 10, 2021 12:05

acatangiu added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Dec 10, 2021

acatangiu requested a review from andresilva December 10, 2021 12:48

acatangiu force-pushed the grandpa-protocol-name branch from 9a3978a to d4b616a Compare December 13, 2021 12:19

acatangiu added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Dec 13, 2021

acatangiu mentioned this pull request Dec 13, 2021

Polkadot companion for Substrate#10463 paritytech/polkadot#4519

Merged

acatangiu marked this pull request as ready for review December 13, 2021 13:58

acatangiu added 3 commits December 13, 2021 17:46

grandpa: update notif protocol name

3cf86ef

grandpa: add chain id prefix to protocol name

fbf7cf5

grandpa: beautify protocol name handling

2b22762

acatangiu force-pushed the grandpa-protocol-name branch from d4b616a to fcb9c8a Compare December 13, 2021 15:50

acatangiu added 4 commits December 13, 2021 18:00

grandpa: prepend genesis hash to protocol name

206beaa

chain-spec: add optional 'fork_id'

952e0a7

'fork_id' is used to uniquely identify forks of the same chain/network 'ChainSpec' trait provides default 'None' implementation, meaning this chain hasn't been forked.

grandpa: protocol_name mod instead of struct

758ff84

beefy: add genesis hash prefix to protocol name

b3a0243

acatangiu force-pushed the grandpa-protocol-name branch from fcb9c8a to b3a0243 Compare December 13, 2021 16:01

tomaka suggested changes Dec 13, 2021

View reviewed changes

acatangiu added 3 commits December 14, 2021 11:56

chainspec: add fork_id

44baf42

grandpa: simplify protocol name

0f9d6cc

Merge branch 'master' of github.com:paritytech/substrate into grandpa…

393c8cf

…-protocol-name

acatangiu requested a review from tomusdrw December 14, 2021 10:55

bkchr reviewed Jan 4, 2022

View reviewed changes

bin/node/cli/src/service.rs Outdated Show resolved Hide resolved

bin/node-template/node/src/service.rs Outdated Show resolved Hide resolved

acatangiu added 3 commits January 4, 2022 16:39

avoid using hash default, even for protocol names

d70ed35

Merge branch 'master' of github.com:paritytech/substrate into grandpa…

d5802f6

…-protocol-name

Merge branch 'master' of github.com:paritytech/substrate into grandpa…

3c7f699

…-protocol-name

joao-paulo-parity mentioned this pull request Jan 5, 2022

Incorrect API request paritytech/parity-processbot#349

Closed

paritytech-processbot bot merged commit 0f070c8 into paritytech:master Jan 5, 2022

Networking (Outdated) automation moved this from Triage to Done Jan 5, 2022

This was referenced Jan 5, 2022

Improve error origin reporting for companion-related errors paritytech/parity-processbot#351

Closed

Also update companions of companions before merge paritytech/parity-processbot#352

Closed

This was referenced Jan 5, 2022

BEEFY protocol name should change in accordance to GRANDPA protocol name paritytech/grandpa-bridge-gadget#240

Closed

Idea: Replace protocolId with the genesis hash #7746

Closed

tomaka mentioned this pull request Jan 26, 2022

Backport protocol name changes and forkId paritytech/smoldot#1955

Open

tomaka mentioned this pull request Jan 28, 2022

Add attributes and documentation to fork_id #10746

Merged

wischli added a commit to KILTprotocol/kilt-node that referenced this pull request Feb 1, 2022

fix: apply new fork_id to chainspecs

42cde95

paritytech/substrate#10463

wischli added a commit to KILTprotocol/kilt-node that referenced this pull request Feb 1, 2022

fix: apply name changes for GrandPa

4130db3

paritytech/substrate#10463

librelois mentioned this pull request Feb 7, 2022

Update substrate/polkadot/cumulus from v0.9.15 to v0.9.16 moonbeam-foundation/moonbeam#1259

Closed

tomaka mentioned this pull request Mar 3, 2022

Wrong Grandpa protocol name #10969

Closed

acatangiu mentioned this pull request Mar 3, 2022

BEEFY and GRANDPA protocol names should use full genesis hash #10974

Merged

AurevoirXavier added a commit to darwinia-network/darwinia-common that referenced this pull request Sep 14, 2022

Companion of paritytech/substrate#10463, !

9ef5b5a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Name changes for GrandPa and Beefy notifications protocols #10463

Name changes for GrandPa and Beefy notifications protocols #10463

acatangiu commented Dec 10, 2021 •

edited

tomaka commented Dec 10, 2021 •

edited

tomaka commented Dec 10, 2021

acatangiu commented Dec 13, 2021

tomaka commented Dec 13, 2021

tomaka commented Dec 13, 2021 •

edited

tomaka commented Dec 14, 2021

bkchr commented Jan 5, 2022

paritytech-processbot bot commented Jan 5, 2022

bkchr commented Jan 5, 2022

paritytech-processbot bot commented Jan 5, 2022

joao-paulo-parity commented Jan 5, 2022

xlc commented Jan 5, 2022 •

edited

bkchr commented Jan 5, 2022

Name changes for GrandPa and Beefy notifications protocols #10463

Name changes for GrandPa and Beefy notifications protocols #10463

Conversation

acatangiu commented Dec 10, 2021 • edited

tomaka commented Dec 10, 2021 • edited

tomaka commented Dec 10, 2021

acatangiu commented Dec 13, 2021

tomaka commented Dec 13, 2021

tomaka commented Dec 13, 2021 • edited

tomaka commented Dec 14, 2021

bkchr commented Jan 5, 2022

paritytech-processbot bot commented Jan 5, 2022

bkchr commented Jan 5, 2022

paritytech-processbot bot commented Jan 5, 2022

joao-paulo-parity commented Jan 5, 2022

xlc commented Jan 5, 2022 • edited

bkchr commented Jan 5, 2022

acatangiu commented Dec 10, 2021 •

edited

tomaka commented Dec 10, 2021 •

edited

tomaka commented Dec 13, 2021 •

edited

xlc commented Jan 5, 2022 •

edited