fix: GoAhead signal only set when runtime upgrade is enacted from par…

…achain side (#1176) The runtime code of a parachain can be replaced on the relay-chain via: [cumulus]: [enact_authorized_upgrade](https://github.com/paritytech/polkadot-sdk/blob/1a38d6d6be42b30e8be3ffccec75a4ec995fef9d/cumulus/pallets/parachain-system/src/lib.rs#L661); 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](https://github.com/paritytech/polkadot-sdk/blob/1a38d6d6be42b30e8be3ffccec75a4ec995fef9d/polkadot/runtime/parachains/src/paras/mod.rs#L823): immediately changes the runtime code of a given para without a pvf check (root). - [force_schedule_code_upgrade](https://github.com/paritytech/polkadot-sdk/blob/1a38d6d6be42b30e8be3ffccec75a4ec995fef9d/polkadot/runtime/parachains/src/paras/mod.rs#L864): schedules a change to the runtime code of a given para including a pvf check of the new code (root). - [schedule_code_upgrade](https://github.com/paritytech/polkadot-sdk/blob/1a38d6d6be42b30e8be3ffccec75a4ec995fef9d/polkadot/runtime/common/src/paras_registrar.rs#L395): 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](https://github.com/paritytech/polkadot-sdk/blob/e49493442a9377be9344c06a4990e17423783d41/polkadot/primitives/src/v5/mod.rs#L1229) 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](https://github.com/paritytech/polkadot/blob/09b61286da11921a3dda0a8e4015ceb9ef9cffca/runtime/parachains/src/paras/mod.rs#L1778), 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](paritytech/polkadot#7412)). 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 --------- Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>
paritytech · Oct 15, 2023 · 91c4360 · 91c4360
1 parent 8ee4042
commit 91c4360
Show file tree

Hide file tree

Showing 8 changed files with 280 additions and 34 deletions.
diff --git a/polkadot/runtime/common/src/paras_registrar/mod.rs b/polkadot/runtime/common/src/paras_registrar/mod.rs
@@ -29,7 +29,7 @@ use frame_system::{self, ensure_root, ensure_signed};
 use primitives::{HeadData, Id as ParaId, ValidationCode, LOWEST_PUBLIC_ID};
 use runtime_parachains::{
 	configuration, ensure_parachain,
-	paras::{self, ParaGenesisArgs},
+	paras::{self, ParaGenesisArgs, SetGoAhead},
 	Origin, ParaLifecycle,
 };
 use sp_std::{prelude::*, result};
@@ -412,7 +412,7 @@ pub mod pallet {
 			new_code: ValidationCode,
 		) -> DispatchResult {
 			Self::ensure_root_para_or_owner(origin, para)?;
-			runtime_parachains::schedule_code_upgrade::<T>(para, new_code)?;
+			runtime_parachains::schedule_code_upgrade::<T>(para, new_code, SetGoAhead::No)?;
 			Ok(())
 		}
 

diff --git a/polkadot/runtime/parachains/src/inclusion/mod.rs b/polkadot/runtime/parachains/src/inclusion/mod.rs
@@ -21,7 +21,8 @@
 
 use crate::{
 	configuration::{self, HostConfiguration},
-	disputes, dmp, hrmp, paras,
+	disputes, dmp, hrmp,
+	paras::{self, SetGoAhead},
 	scheduler::{self, AvailabilityTimeoutStatus},
 	shared::{self, AllowedRelayParentsTracker},
 };
@@ -448,8 +449,9 @@ impl fmt::Debug for UmpAcceptanceCheckErr {
 				"the ump queue would have grown past the max size permitted by config ({} > {})",
 				total_size, limit,
 			),
-			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")
+			},
 		}
 	}
 }
@@ -885,6 +887,7 @@ impl<T: Config> Pallet<T> {
 				new_code,
 				now,
 				&config,
+				SetGoAhead::Yes,
 			));
 		}
 

diff --git a/polkadot/runtime/parachains/src/inclusion/tests.rs b/polkadot/runtime/parachains/src/inclusion/tests.rs
@@ -1255,7 +1255,13 @@ fn candidate_checks() {
 				let cfg = Configuration::config();
 				let expected_at = 10 + cfg.validation_upgrade_delay;
 				assert_eq!(expected_at, 12);
-				Paras::schedule_code_upgrade(chain_a, vec![1, 2, 3, 4].into(), expected_at, &cfg);
+				Paras::schedule_code_upgrade(
+					chain_a,
+					vec![1, 2, 3, 4].into(),
+					expected_at,
+					&cfg,
+					SetGoAhead::Yes,
+				);
 			}
 
 			assert_noop!(
@@ -2235,7 +2241,7 @@ fn para_upgrade_delay_scheduled_from_inclusion() {
 		let cause = &active_vote_state.causes()[0];
 		// Upgrade block is the block of inclusion, not candidate's parent.
 		assert_matches!(cause,
-			paras::PvfCheckCause::Upgrade { id, included_at }
+			paras::PvfCheckCause::Upgrade { id, included_at, set_go_ahead: SetGoAhead::Yes }
 				if id == &chain_a && included_at == &7
 		);
 	});

diff --git a/polkadot/runtime/parachains/src/lib.rs b/polkadot/runtime/parachains/src/lib.rs
@@ -53,7 +53,7 @@ mod mock;
 mod ump_tests;
 
 pub use origin::{ensure_parachain, Origin};
-pub use paras::ParaLifecycle;
+pub use paras::{ParaLifecycle, SetGoAhead};
 use primitives::{HeadData, Id as ParaId, ValidationCode};
 use sp_runtime::{DispatchResult, FixedU128};
 
@@ -89,8 +89,9 @@ pub fn schedule_parachain_downgrade<T: paras::Config>(id: ParaId) -> Result<(),
 pub fn schedule_code_upgrade<T: paras::Config>(
 	id: ParaId,
 	new_code: ValidationCode,
+	set_go_ahead: SetGoAhead,
 ) -> DispatchResult {
-	paras::Pallet::<T>::schedule_code_upgrade_external(id, new_code)
+	paras::Pallet::<T>::schedule_code_upgrade_external(id, new_code, set_go_ahead)
 }
 
 /// Sets the current parachain head with the given id.

diff --git a/polkadot/runtime/parachains/src/paras/benchmarking.rs b/polkadot/runtime/parachains/src/paras/benchmarking.rs
@@ -124,7 +124,13 @@ benchmarks! {
 		let expired = frame_system::Pallet::<T>::block_number().saturating_sub(One::one());
 		let config = HostConfiguration::<BlockNumberFor<T>>::default();
 		generate_disordered_pruning::<T>();
-		Pallet::<T>::schedule_code_upgrade(para_id, ValidationCode(vec![0]), expired, &config);
+		Pallet::<T>::schedule_code_upgrade(
+			para_id,
+			ValidationCode(vec![0]),
+			expired,
+			&config,
+			SetGoAhead::Yes,
+		);
 	}: _(RawOrigin::Root, para_id, new_head)
 	verify {
 		assert_last_event::<T>(Event::NewHeadNoted(para_id).into());

diff --git a/polkadot/runtime/parachains/src/paras/benchmarking/pvf_check.rs b/polkadot/runtime/parachains/src/paras/benchmarking/pvf_check.rs
@@ -177,6 +177,7 @@ where
 				validation_code,
 				/* relay_parent_number */ 1u32.into(),
 				&configuration::Pallet::<T>::config(),
+				SetGoAhead::Yes,
 			);
 		} else {
 			let r = Pallet::<T>::schedule_para_initialize(

diff --git a/polkadot/runtime/parachains/src/paras/mod.rs b/polkadot/runtime/parachains/src/paras/mod.rs
@@ -386,9 +386,18 @@ 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: SetGoAhead,
 	},
 }
 
+/// Should the `GoAhead` signal be set after a successful check of the new wasm binary?
+#[derive(Debug, Copy, Clone, PartialEq, TypeInfo, Decode, Encode)]
+pub enum SetGoAhead {
+	Yes,
+	No,
+}
+
 impl<BlockNumber> PvfCheckCause<BlockNumber> {
 	/// Returns the ID of the para that initiated or subscribed to the pre-checking vote.
 	fn para_id(&self) -> ParaId {
@@ -888,7 +897,13 @@ pub mod pallet {
 		) -> DispatchResult {
 			ensure_root(origin)?;
 			let config = configuration::Pallet::<T>::config();
-			Self::schedule_code_upgrade(para, new_code, relay_parent_number, &config);
+			Self::schedule_code_upgrade(
+				para,
+				new_code,
+				relay_parent_number,
+				&config,
+				SetGoAhead::No,
+			);
 			Self::deposit_event(Event::CodeUpgradeScheduled(para));
 			Ok(())
 		}
@@ -1186,14 +1201,15 @@ impl<T: Config> Pallet<T> {
 	pub(crate) fn schedule_code_upgrade_external(
 		id: ParaId,
 		new_code: ValidationCode,
+		set_go_ahead: SetGoAhead,
 	) -> DispatchResult {
 		// Check that we can schedule an upgrade at all.
 		ensure!(Self::can_upgrade_validation_code(id), Error::<T>::CannotUpgradeCode);
 		let config = configuration::Pallet::<T>::config();
 		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, set_go_ahead);
 		Self::deposit_event(Event::CodeUpgradeScheduled(id));
 		Ok(())
 	}
@@ -1534,8 +1550,15 @@ impl<T: Config> Pallet<T> {
 				PvfCheckCause::Onboarding(id) => {
 					weight += Self::proceed_with_onboarding(*id, sessions_observed);
 				},
-				PvfCheckCause::Upgrade { id, included_at } => {
-					weight += Self::proceed_with_upgrade(*id, code_hash, now, *included_at, cfg);
+				PvfCheckCause::Upgrade { id, included_at, set_go_ahead } => {
+					weight += Self::proceed_with_upgrade(
+						*id,
+						code_hash,
+						now,
+						*included_at,
+						cfg,
+						*set_go_ahead,
+					);
 				},
 			}
 		}
@@ -1568,6 +1591,7 @@ impl<T: Config> Pallet<T> {
 		now: BlockNumberFor<T>,
 		relay_parent_number: BlockNumberFor<T>,
 		cfg: &configuration::HostConfiguration<BlockNumberFor<T>>,
+		set_go_ahead: SetGoAhead,
 	) -> Weight {
 		let mut weight = Weight::zero();
 
@@ -1591,12 +1615,15 @@ impl<T: Config> Pallet<T> {
 		weight += T::DbWeight::get().reads_writes(1, 4);
 		FutureCodeUpgrades::<T>::insert(&id, expected_at);
 
-		UpcomingUpgrades::<T>::mutate(|upcoming_upgrades| {
-			let insert_idx = upcoming_upgrades
-				.binary_search_by_key(&expected_at, |&(_, b)| b)
-				.unwrap_or_else(|idx| idx);
-			upcoming_upgrades.insert(insert_idx, (id, expected_at));
-		});
+		// Only set an upcoming upgrade if `GoAhead` signal should be set for the respective para.
+		if set_go_ahead == SetGoAhead::Yes {
+			UpcomingUpgrades::<T>::mutate(|upcoming_upgrades| {
+				let insert_idx = upcoming_upgrades
+					.binary_search_by_key(&expected_at, |&(_, b)| b)
+					.unwrap_or_else(|idx| idx);
+				upcoming_upgrades.insert(insert_idx, (id, expected_at));
+			});
+		}
 
 		let expected_at = expected_at.saturated_into();
 		let log = ConsensusLog::ParaScheduleUpgradeCode(id, *code_hash, expected_at);
@@ -1835,6 +1862,7 @@ impl<T: Config> Pallet<T> {
 		new_code: ValidationCode,
 		inclusion_block_number: BlockNumberFor<T>,
 		cfg: &configuration::HostConfiguration<BlockNumberFor<T>>,
+		set_go_ahead: SetGoAhead,
 	) -> Weight {
 		let mut weight = T::DbWeight::get().reads(1);
 
@@ -1884,7 +1912,7 @@ impl<T: Config> Pallet<T> {
 		});
 
 		weight += Self::kick_off_pvf_check(
-			PvfCheckCause::Upgrade { id, included_at: inclusion_block_number },
+			PvfCheckCause::Upgrade { id, included_at: inclusion_block_number, set_go_ahead },
 			code_hash,
 			new_code,
 			cfg,