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

grandpa: fix early enactment of forced changes #7321

Merged
5 commits merged into from
Oct 26, 2020

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Oct 14, 2020

In GRANDPA there are two mechanisms for switching the authority set:

  • Regular authority set changes - these are announced at a block N and enacted when that block is finalized. This is what should happen if GRANDPA is working properly.
  • Forced authority set changes - these are announced at a block N (with some delay) and they are enacted when the block after the given delay is imported. Forced changes should only be triggered if the current GRANDPA authority set is not finalizing.

While syncing the chain if we encounter a block that triggers a regular authority set change we need to have a justification for it so that we can finalize it. We support having multiple pending regular authority set changes, so that we can keep importing blocks while in the background we try to sync justifications from peers (in case they didn't come attached to the block in the first place).

This can lead to a situation where by the time we encounter a forced change we try to apply it immediately (remember that it is applied on block import rather than finality) without waiting to apply some previous standard changes that we are waiting to import justifications for. In this case we end up with an invalid authority set state and are unable to validate anything going further.

This PR makes it so that when trying to apply a forced change we will verify if there are any dependent standard changes that we should wait on, if there are then we refuse to import such a block. Specifically we wait for any pending change that is an ancestor of the forced changed and its effective block number is lower than the last finalized block (as signaled in the forced change) must be applied beforehand.

Note: my initial implementation made it so that we allowed multiple pending forced changes, which would be applied in-order the same way we do for standard changes. Unfortunately removing the invariant of only having one forced changed per fork at anytime introduced a couple of edge cases that I started fixing but realized wasn't worth it. Forced changes are uncommon and I'd rather err on the side of safety/correctness rather than performance.

Fixes paritytech/polkadot#1755.

@andresilva andresilva added 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. labels Oct 14, 2020
@andresilva andresilva requested review from octol and rphmeier October 14, 2020 10:58
@andresilva andresilva changed the title grandpa: fix early enactment forced changes grandpa: fix early enactment of forced changes Oct 14, 2020
@andresilva andresilva force-pushed the andre/fix-early-enactment-forced-changes branch from b4ba8b9 to 23a90e1 Compare October 14, 2020 11:17
Copy link
Contributor

@octol octol left a comment

Choose a reason for hiding this comment

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

Looks good to me, nice tidy cleanups too!

Mind expanding a little about median_last_finalized? What we use it for and what is solves?

@andresilva
Copy link
Contributor Author

@octol Whenever we apply a forced authority set change we do so because the current set has stopped finalizing (e.g. > 1/3 went offline). As such, when applying a forced authority set change we provide a block number as a hint of the last thing that was finalized, the new set will start finalizing from this point on (i.e. this will be used as the GRANDPA round base). The reason it is called median_last_finalized is because initially we were triggering these forced authority set changes automatically using the on-chain finality-tracker (#7228) and the way it worked is that the runtime would calculate the median last finalized block based on what the different authorities reported. This is no longer the case as we have removed the finality-tracker and instead will trigger forced changes manually (https://github.com/paritytech/substrate/blob/master/frame/grandpa/src/lib.rs#L300) through sudo/governance, so we just pass whatever we "know" is the last finalized block. I will probably update the name of that parameter later on when I expand some documentation.

@andresilva
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Oct 26, 2020

Trying merge.

This pull request was closed.
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FinalizedBlock stop syncing after #1280777 (Westend)
2 participants