Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: GoAhead signal only set when runtime upgrade is enacted from parachain side #1176

Merged
merged 7 commits into from
Oct 15, 2023

Conversation

Daanvdplas
Copy link
Contributor

The runtime code of a parachain can be replaced on the relay-chain via:

[cumulus]: enact_authorized_upgrade; this is used for a runtime upgrade when a parachain is not bricked.

[polkadot] (these are used when a parachain is bricked):

  • force_set_current_code: immediately changes the runtime code of a given para without a pvf check (root).
  • force_schedule_code_upgrade: schedules a change to the runtime code of a given para including a pvf check of the new code (root).
  • schedule_code_upgrade: schedules a change to the runtime code of a given para including a pvf check of the new code. Besides root, the parachain or parachain manager can call this extrinsic given that the parachain is unlocked.

Polkadot signals a parachain to be ready for a runtime upgrade through the GoAhead signal.

When in cumulus enact_authorized_upgrade is executed, the same underlying helper function of force_schedule_code_upgrade & schedule_code_upgrade: schedule_code_upgrade, is called on the relay-chain, which sets the GoAhead signal (if the pvf is accepted).

If Cumulus receives the GoAhead signal from polkadot without having the PendingValidationCode ready, it will panic (ref). For enact_authorized_upgrade we know for sure the PendingValidationCode is set. On the contrary, for force_schedule_code_upgrade & schedule_code_upgrade this is not the case.

This PR includes a flag such that the GoAhead signal will only be set when a runtime upgrade is enacted by the parachain (enact_authorized_upgrade).

additional info: paritytech/polkadot#7412

Closes #641

Copy link
Contributor

@BradleyOlson64 BradleyOlson64 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 work Daan!

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Looking mainly good, just some nitpicks.

UmpAcceptanceCheckErr::IsOffboarding =>
write!(fmt, "upward message rejected because the para is off-boarding",),
UmpAcceptanceCheckErr::IsOffboarding => {
write!(fmt, "upward message rejected because the para is off-boarding",)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
write!(fmt, "upward message rejected because the para is off-boarding",)
write!(fmt, "upward message rejected because the para is off-boarding")

@@ -386,6 +386,8 @@ pub(crate) enum PvfCheckCause<BlockNumber> {
///
/// See https://github.com/paritytech/polkadot/issues/4601 for detailed explanation.
included_at: BlockNumber,
/// Whether or not the given para should be sent the `GoAhead` signal.
set_go_ahead: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this an enum:

/// Should the `GoAhead` signal be set after a successful check of the new wasm binary?
enum SetGoAhead {
     Yes,
     No,
}

@@ -1174,7 +1178,7 @@ impl<T: Config> Pallet<T> {
let current_block = frame_system::Pallet::<T>::block_number();
// Schedule the upgrade with a delay just like if a parachain triggered the upgrade.
let upgrade_block = current_block.saturating_add(config.validation_upgrade_delay);
Self::schedule_code_upgrade(id, new_code, upgrade_block, &config);
Self::schedule_code_upgrade(id, new_code, upgrade_block, &config, false);
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should "bubble up" the set_go_ahead parameter to the callers of this function.

@Daanvdplas Daanvdplas requested review from a team September 11, 2023 14:47
@Daanvdplas Daanvdplas requested a review from a team September 11, 2023 14:48
@Daanvdplas Daanvdplas requested review from koute and a team as code owners September 11, 2023 14:48
@paritytech-ci paritytech-ci requested review from a team September 11, 2023 14:53
@svyatonik svyatonik removed request for a team September 11, 2023 15:02
@Daanvdplas Daanvdplas removed request for cheme, a team and koute September 11, 2023 15:24
@Daanvdplas Daanvdplas added the I2-bug The node fails to follow expected behavior. label Sep 11, 2023
@Daanvdplas
Copy link
Contributor Author

Apologies for the mess and the lost history. I didn't see another way than to force push, it won't happen again 😣

@bkchr bkchr added the T8-polkadot This PR/Issue is related to/affects the Polkadot network. label Oct 15, 2023
@bkchr bkchr merged commit 91c4360 into master Oct 15, 2023
108 of 109 checks passed
@bkchr bkchr deleted the daan/fix-go-ahead-signal branch October 15, 2023 21:32
ordian added a commit that referenced this pull request Oct 16, 2023
* master: (54 commits)
  Publish `xcm-emulator` crate (#1881)
  Adding migrations to clean Rococo Gov 1 storage & reserved funds (#1849)
  Arkworks Elliptic Curve utils overhaul (#1870)
  Fix typos (#1878)
  fix: GoAhead signal only set when runtime upgrade is enacted from parachain side (#1176)
  Refactor staking ledger (#1484)
  Paired-key Crypto Scheme (#1705)
  Include polkadot version in artifact path (#1828)
  add link to rfc-0001 in broker README (#1862)
  Discard `Executor` (#1855)
  Macros to use path instead of ident (#1474)
  Remove clippy clone-double-ref lint noise (#1860)
  Refactor alliance benchmarks to v2 (#1868)
  Check executor params coherence (#1774)
  frame: use derive-impl for beefy and mmr pallets (#1867)
  sc-consensus-beefy: improve gossip logic (#1852)
  Adds instance support for composite enums (#1857)
  Fix links to implementers' guide (#1865)
  Disabled validators runtime API (#1257)
  Adding `try_state` hook for `Treasury` pallet (#1820)
  ...
ordian added a commit that referenced this pull request Oct 16, 2023
…ribution

* tsv-disabling-backing: (54 commits)
  Publish `xcm-emulator` crate (#1881)
  Adding migrations to clean Rococo Gov 1 storage & reserved funds (#1849)
  Arkworks Elliptic Curve utils overhaul (#1870)
  Fix typos (#1878)
  fix: GoAhead signal only set when runtime upgrade is enacted from parachain side (#1176)
  Refactor staking ledger (#1484)
  Paired-key Crypto Scheme (#1705)
  Include polkadot version in artifact path (#1828)
  add link to rfc-0001 in broker README (#1862)
  Discard `Executor` (#1855)
  Macros to use path instead of ident (#1474)
  Remove clippy clone-double-ref lint noise (#1860)
  Refactor alliance benchmarks to v2 (#1868)
  Check executor params coherence (#1774)
  frame: use derive-impl for beefy and mmr pallets (#1867)
  sc-consensus-beefy: improve gossip logic (#1852)
  Adds instance support for composite enums (#1857)
  Fix links to implementers' guide (#1865)
  Disabled validators runtime API (#1257)
  Adding `try_state` hook for `Treasury` pallet (#1820)
  ...
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v1-3-0/4614/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior. T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

registerar.schedule_code_upgrade doesn't work
4 participants