This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Introduce upgrade go-ahead and upgrade restriction signals #3371
Merged
+368
−32
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pepyakin
added
A0-please_review
Pull request needs code review.
B7-runtimenoteworthy
D5-nicetohaveaudit ⚠️
PR contains trivial changes to logic that should be properly reviewed.
C3-medium
PR touches the given topic and has a medium impact on builders.
labels
Jun 25, 2021
rphmeier
reviewed
Jul 8, 2021
rphmeier
reviewed
Jul 8, 2021
rphmeier
reviewed
Jul 8, 2021
rphmeier
approved these changes
Jul 8, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a welcome refactoring and more elegant solution to the upgrade signal process. Is the plan to migrate Cumulus and eventually phase out some of the other storage members?
Yes, this patch is just a stepping stone. My rough plan is:
|
LGTM after merge addressed |
redzsina
added
D1-audited 👍
PR contains changes to critical logic that has been properly reviewed and externally audited.
and removed
D5-nicetohaveaudit ⚠️
PR contains trivial changes to logic that should be properly reviewed.
labels
Aug 17, 2021
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.
C3-medium
PR touches the given topic and has a medium impact on builders.
D1-audited 👍
PR contains changes to critical logic that has been properly reviewed and externally audited.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch introduces a new mechanism that will be used in future by parachains to apply validation code upgrade.
Currently, the upgrades are assumed to be activated after a fixed number of relay-chain blocks. Also there are configuration values that the parachain should keep an eye on to not signal an upgrade before an upgrade cooldown (aka
validation_upgrade_frequency
) timer elapses. These mechanics do not work well with our planned changes such as #3211 and paritytech/polkadot-sdk#967.Thus as a first step to tackle this problem this patch introduces two new signals that should coordinate an upgrade:
Instead of counting the number of the relay chain blocks passed since the upgrade was signalled the parachain will be looking at the upgrade go ahead signal through merkle proofs.
The logic of upgrades themselves is not changed at this time.
This signal is almost always absent. However, at the relay-chain block the upgrade expected to happen, this signal will be turned on. When a parachain block is created on top of that relay-parent or it's descendant, it will observe the go ahead signal and enact the upgrade. The next parablock will be validated using the upgraded validation code.
As soon as the first block with the activated upgrade is included, the signal is reset.
The upgrade restriction signal is set when validation code upgrades are disallowed. We do not give details on why at this moment. To follow the current upgrade logic, this signal is set as soon as the upgrade is announced. The signal is unset only after
validation_upgrade_frequency
is elapsed.Rationale
Why two signals, it seems that it's possible to get away with only one?
It's indeed was initial thinking that one signal will do.
When I approached this problem I realized that semantically those are different states and cannot be represented by a single state machine. A contrived example: We have a parathread. The parathread doesn't produce blocks too often. The parathread schedules an upgrade. Then the parathread produces a block after
validation_upgrade_frequency
. If the signal had a single state only we would have to signal GoAhead first. We have to delay signalling that the upgrade is allowed to another relay-block.Besides being semantically unsound, it was also harder to implement a single signal model.