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

Name changes for GrandPa and Beefy notifications protocols #10463

Merged

Conversation

acatangiu
Copy link
Contributor

@acatangiu acatangiu commented Dec 10, 2021

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

@acatangiu acatangiu self-assigned this Dec 10, 2021
@acatangiu 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
@tomaka
Copy link
Contributor

tomaka commented Dec 10, 2021

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
Copy link
Contributor

tomaka commented Dec 10, 2021

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
Copy link
Contributor Author

@tomaka were you thinking something like this: d4b616a ?

@tomaka
Copy link
Contributor

tomaka commented Dec 13, 2021

Yes!

@acatangiu acatangiu added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Dec 13, 2021
@acatangiu acatangiu marked this pull request as ready for review December 13, 2021 13:58
'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
Copy link
Contributor

tomaka commented Dec 13, 2021

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 Outdated Show resolved Hide resolved
bin/node-template/node/src/service.rs Outdated Show resolved Hide resolved
client/finality-grandpa/src/communication/mod.rs Outdated Show resolved Hide resolved
client/network/src/config.rs Show resolved Hide resolved
client/finality-grandpa/src/communication/mod.rs Outdated Show resolved Hide resolved
@tomaka
Copy link
Contributor

tomaka commented Dec 14, 2021

👍 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 Outdated Show resolved Hide resolved
bin/node-template/node/src/service.rs Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Jan 5, 2022

bot merge

@paritytech-processbot
Copy link

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

@bkchr
Copy link
Member

bkchr commented Jan 5, 2022

bot merge

@paritytech-processbot
Copy link

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
Copy link
Contributor

bot merge

@xlc
Copy link
Contributor

xlc commented Jan 5, 2022

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
Copy link
Member

bkchr commented Jan 5, 2022

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.

AurevoirXavier pushed a commit to darwinia-network/substrate that referenced this pull request Jan 27, 2022
…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>
wischli added a commit to KILTprotocol/kilt-node that referenced this pull request Feb 1, 2022
wischli added a commit to KILTprotocol/kilt-node that referenced this pull request Feb 1, 2022
wischli added a commit to KILTprotocol/kilt-node that referenced this pull request Feb 9, 2022
* 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
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
…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>
AurevoirXavier added a commit to darwinia-network/darwinia-common that referenced this pull request Sep 14, 2022
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
6 participants