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

grandpa: allow authority set hard forks to be forced #10444

Merged
merged 4 commits into from
Dec 7, 2021

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Dec 7, 2021

This PR extends the GRANDPA authority set hard fork functionality to allow defining the changes as forced, i.e. enacted instantly instead of waiting for finality.

polkadot companion: paritytech/polkadot#4486

@andresilva andresilva added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit C1-low PR touches the given topic and has a low impact on builders. labels Dec 7, 2021
Comment on lines 554 to 556
/// The last finalized block number by the network. If defined, then the
/// authority set change will be forced, i.e. the node won't wait for the
/// block above to be finalized before enacting it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment confuses me.

I mean I get what it does by reading the code, but I interpret the comment differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated it. Let me know if it is more understandable now.

/// the node won't wait for the block above to be finalized before enacting
/// the change, and the given finalized number will be used as a base for
/// voting.
pub last_finalized: Option<NumberFor<Block>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry again me :P

Does this mean that block above should be last_finalized + 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't necessarily need to be + 1, but it needs to be a block that is higher than last_finalized yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm completely confused now.

Let's say we set last_finalized to 1 and block is 3. So, the set will be enacted at 3, but it will already be active at 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that scenario the change gets enacted when block #3 is imported, and the GRANDPA voter will act as if block #1 is finalized, i.e. this is the base it will start voting on.

(I think you are forgetting that GRANDPA needs to distinguish between best imported and best finalized, and best finalized is always lower or equal to best imported)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that scenario the change gets enacted when block #3 is imported, and the GRANDPA voter will act as if block #1 is finalized, i.e. this is the base it will start voting on.

With the new set?

@andresilva
Copy link
Contributor Author

@bkchr Cumulus will get fixed by updating polkadot. What do we do here?

@bkchr bkchr merged commit fd350e3 into master Dec 7, 2021
@bkchr bkchr deleted the andre/grandpa-forced-hard-fork branch December 7, 2021 21:31
@bkchr
Copy link
Member

bkchr commented Dec 7, 2021

@bkchr Cumulus will get fixed by updating polkadot. What do we do here?

Ignore.

andresilva added a commit that referenced this pull request Dec 7, 2021
* grandpa: allow authority set hard forks to be forced

* grandpa: fix authority set hard forks in warp proof provider

* grandpa: make AuthoritySetHardFork public

* grandpa: extend comment
mmyyrroonn pushed a commit to crustio/substrate that referenced this pull request Dec 8, 2021
* grandpa: allow authority set hard forks to be forced

* grandpa: fix authority set hard forks in warp proof provider

* grandpa: make AuthoritySetHardFork public

* grandpa: extend comment
mmyyrroonn pushed a commit to crustio/substrate that referenced this pull request Dec 8, 2021
* grandpa: allow authority set hard forks to be forced

* grandpa: fix authority set hard forks in warp proof provider

* grandpa: make AuthoritySetHardFork public

* grandpa: extend comment
seunlanlege pushed a commit to seunlanlege/substrate that referenced this pull request Dec 17, 2021
* grandpa: allow authority set hard forks to be forced

* grandpa: fix authority set hard forks in warp proof provider

* grandpa: make AuthoritySetHardFork public

* grandpa: extend comment
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* grandpa: allow authority set hard forks to be forced

* grandpa: fix authority set hard forks in warp proof provider

* grandpa: make AuthoritySetHardFork public

* grandpa: extend comment
mmyyrroonn pushed a commit to mmyyrroonn/substrate that referenced this pull request Nov 22, 2022
* grandpa: allow authority set hard forks to be forced

* grandpa: fix authority set hard forks in warp proof provider

* grandpa: make AuthoritySetHardFork public

* grandpa: extend comment
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* grandpa: allow authority set hard forks to be forced

* grandpa: fix authority set hard forks in warp proof provider

* grandpa: make AuthoritySetHardFork public

* grandpa: extend comment
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. B0-silent Changes should not be mentioned in any release notes 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
Development

Successfully merging this pull request may close these issues.

2 participants