paras: count upgrade delay from inclusion (#7486)

* paras: count upgrade delay from inclusion

* fix warning

* rename check cause block number field

* rename inclusion_parent -> included_at

polkadot/runtime/parachains/src/inclusion/mod.rs

@@ -884,10 +884,13 @@ impl<T: Config> Pallet<T> {
 		// initial weight is config read.
 		let mut weight = T::DbWeight::get().reads_writes(1, 0);
 		if let Some(new_code) = commitments.new_validation_code {
+			// Block number of candidate's inclusion.
+			let now = <frame_system::Pallet<T>>::block_number();
+
 			weight.saturating_add(<paras::Pallet<T>>::schedule_code_upgrade(
 				receipt.descriptor.para_id,
 				new_code,
-				relay_parent_number,
+				now,
 				&config,
 			));
 		}

polkadot/runtime/parachains/src/inclusion/tests.rs

@@ -28,6 +28,7 @@ use crate::{
 };
 use primitives::{SignedAvailabilityBitfields, UncheckedSignedAvailabilityBitfields};
 
+use assert_matches::assert_matches;
 use frame_support::assert_noop;
 use keyring::Sr25519Keyring;
 use parity_scale_codec::DecodeAll;
@@ -44,7 +45,8 @@ use test_helpers::{dummy_collator, dummy_collator_signature, dummy_validation_co
 fn default_config() -> HostConfiguration<BlockNumber> {
 	let mut config = HostConfiguration::default();
 	config.parathread_cores = 1;
-	config.max_code_size = 3;
+	config.max_code_size = 0b100000;
+	config.max_head_data_size = 0b100000;
 	config
 }
 
@@ -1902,3 +1904,141 @@ fn aggregate_origin_decode_regression_check() {
 	let decoded = AggregateMessageOrigin::decode_all(&mut &raw[..]);
 	assert_eq!(decoded, Ok(ump), "Migration needed for AggregateMessageOrigin");
 }
+
+#[test]
+fn para_upgrade_delay_scheduled_from_inclusion() {
+	let chain_a = ParaId::from(1_u32);
+
+	// The block number of the relay-parent for testing.
+	const RELAY_PARENT_NUM: BlockNumber = 4;
+
+	let paras = vec![(chain_a, ParaKind::Parachain)];
+	let validators = vec![
+		Sr25519Keyring::Alice,
+		Sr25519Keyring::Bob,
+		Sr25519Keyring::Charlie,
+		Sr25519Keyring::Dave,
+		Sr25519Keyring::Ferdie,
+	];
+	let keystore: KeystorePtr = Arc::new(LocalKeystore::in_memory());
+	for validator in validators.iter() {
+		Keystore::sr25519_generate_new(
+			&*keystore,
+			PARACHAIN_KEY_TYPE_ID,
+			Some(&validator.to_seed()),
+		)
+		.unwrap();
+	}
+	let validator_public = validator_pubkeys(&validators);
+
+	new_test_ext(genesis_config(paras)).execute_with(|| {
+		shared::Pallet::<Test>::set_active_validators_ascending(validator_public.clone());
+		shared::Pallet::<Test>::set_session_index(5);
+
+		let new_validation_code: ValidationCode = vec![1, 2, 3, 4, 5].into();
+		let new_validation_code_hash = new_validation_code.hash();
+
+		// Otherwise upgrade is no-op.
+		assert_ne!(new_validation_code, dummy_validation_code());
+
+		run_to_block(5, |_| None);
+
+		let signing_context =
+			SigningContext { parent_hash: System::parent_hash(), session_index: 5 };
+
+		let group_validators = |group_index: GroupIndex| {
+			match group_index {
+				group_index if group_index == GroupIndex::from(0) => Some(vec![0, 1, 2, 3, 4]),
+				_ => panic!("Group index out of bounds for 1 parachain"),
+			}
+			.map(|vs| vs.into_iter().map(ValidatorIndex).collect::<Vec<_>>())
+		};
+
+		let core_lookup = |core| match core {
+			core if core == CoreIndex::from(0) => Some(chain_a),
+			_ => None,
+		};
+
+		let chain_a_assignment = CoreAssignment {
+			core: CoreIndex::from(0),
+			para_id: chain_a,
+			kind: AssignmentKind::Parachain,
+			group_idx: GroupIndex::from(0),
+		};
+
+		let mut candidate_a = TestCandidateBuilder {
+			para_id: chain_a,
+			relay_parent: System::parent_hash(),
+			pov_hash: Hash::repeat_byte(1),
+			persisted_validation_data_hash: make_vdata_hash(chain_a).unwrap(),
+			new_validation_code: Some(new_validation_code.clone()),
+			hrmp_watermark: RELAY_PARENT_NUM,
+			..Default::default()
+		}
+		.build();
+		collator_sign_candidate(Sr25519Keyring::One, &mut candidate_a);
+
+		let backed_a = back_candidate(
+			candidate_a.clone(),
+			&validators,
+			group_validators(GroupIndex::from(0)).unwrap().as_ref(),
+			&keystore,
+			&signing_context,
+			BackingKind::Threshold,
+		);
+
+		let ProcessedCandidates { core_indices: occupied_cores, .. } =
+			ParaInclusion::process_candidates(
+				Default::default(),
+				vec![backed_a],
+				vec![chain_a_assignment.clone()],
+				&group_validators,
+			)
+			.expect("candidates scheduled, in order, and backed");
+
+		assert_eq!(occupied_cores, vec![CoreIndex::from(0)]);
+
+		// Run a couple of blocks before the inclusion.
+		run_to_block(7, |_| None);
+
+		let mut bare_bitfield = default_bitfield();
+		*bare_bitfield.0.get_mut(0).unwrap() = true;
+
+		let signed_bitfields = validators
+			.iter()
+			.enumerate()
+			.map(|(i, key)| {
+				sign_bitfield(
+					&keystore,
+					key,
+					ValidatorIndex(i as _),
+					bare_bitfield.clone(),
+					&signing_context,
+				)
+				.into()
+			})
+			.collect::<Vec<_>>();
+
+		let checked_bitfields = simple_sanitize_bitfields(
+			signed_bitfields,
+			DisputedBitfield::zeros(expected_bits()),
+			expected_bits(),
+		);
+
+		let v = process_bitfields(expected_bits(), checked_bitfields, core_lookup);
+		assert_eq!(vec![(CoreIndex(0), candidate_a.hash())], v);
+
+		assert!(<PendingAvailability<Test>>::get(&chain_a).is_none());
+		assert!(<PendingAvailabilityCommitments<Test>>::get(&chain_a).is_none());
+
+		let active_vote_state = paras::Pallet::<Test>::active_vote_state(&new_validation_code_hash)
+			.expect("prechecking must be initiated");
+
+		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 }
+				if id == &chain_a && included_at == &7
+		);
+	});
+}

polkadot/runtime/parachains/src/paras/mod.rs

@@ -367,17 +367,21 @@ impl TypeInfo for ParaKind {
 
 /// This enum describes a reason why a particular PVF pre-checking vote was initiated. When the
 /// PVF vote in question is concluded, this enum indicates what changes should be performed.
-#[derive(Encode, Decode, TypeInfo)]
-enum PvfCheckCause<BlockNumber> {
+#[derive(Debug, Encode, Decode, TypeInfo)]
+pub(crate) enum PvfCheckCause<BlockNumber> {
 	/// PVF vote was initiated by the initial onboarding process of the given para.
 	Onboarding(ParaId),
 	/// PVF vote was initiated by signalling of an upgrade by the given para.
 	Upgrade {
 		/// The ID of the parachain that initiated or is waiting for the conclusion of pre-checking.
 		id: ParaId,
-		/// The relay-chain block number that was used as the relay-parent for the parablock that
-		/// initiated the upgrade.
-		relay_parent_number: BlockNumber,
+		/// The relay-chain block number of **inclusion** of candidate that that initiated the upgrade.
+		///
+		/// It's important to count upgrade enactment delay from the inclusion of this candidate instead
+		/// of its relay parent -- in order to keep PVF available in case of chain reversions.
+		///
+		/// See https://github.com/paritytech/polkadot/issues/4601 for detailed explanation.
+		included_at: BlockNumber,
 	},
 }
 
@@ -400,7 +404,7 @@ enum PvfCheckOutcome {
 
 /// This struct describes the current state of an in-progress PVF pre-checking vote.
 #[derive(Encode, Decode, TypeInfo)]
-struct PvfCheckActiveVoteState<BlockNumber> {
+pub(crate) struct PvfCheckActiveVoteState<BlockNumber> {
 	// The two following vectors have their length equal to the number of validators in the active
 	// set. They start with all zeroes. A 1 is set at an index when the validator at the that index
 	// makes a vote. Once a 1 is set for either of the vectors, that validator cannot vote anymore.
@@ -466,6 +470,11 @@ impl<BlockNumber> PvfCheckActiveVoteState<BlockNumber> {
 			None
 		}
 	}
+
+	#[cfg(test)]
+	pub(crate) fn causes(&self) -> &[PvfCheckCause<BlockNumber>] {
+		self.causes.as_slice()
+	}
 }
 
 pub trait WeightInfo {
@@ -1473,9 +1482,8 @@ impl<T: Config> Pallet<T> {
 				PvfCheckCause::Onboarding(id) => {
 					weight += Self::proceed_with_onboarding(*id, sessions_observed);
 				},
-				PvfCheckCause::Upgrade { id, relay_parent_number } => {
-					weight +=
-						Self::proceed_with_upgrade(*id, code_hash, now, *relay_parent_number, cfg);
+				PvfCheckCause::Upgrade { id, included_at } => {
+					weight += Self::proceed_with_upgrade(*id, code_hash, now, *included_at, cfg);
 				},
 			}
 		}
@@ -1519,10 +1527,10 @@ impl<T: Config> Pallet<T> {
 		// against the new validation code.
 		//
 		// Here we are trying to choose the block number that will have `validation_upgrade_delay`
-		// blocks from the relay-parent of the block that schedule code upgrade but no less than
-		// `minimum_validation_upgrade_delay`. We want this delay out of caution so that when
-		// the last vote for pre-checking comes the parachain will have some time until the upgrade
-		// finally takes place.
+		// blocks from the relay-parent of inclusion of the the block that scheduled code upgrade
+		// but no less than `minimum_validation_upgrade_delay`. We want this delay out of caution
+		// so that when the last vote for pre-checking comes the parachain will have some time until
+		// the upgrade finally takes place.
 		let expected_at = cmp::max(
 			relay_parent_number + cfg.validation_upgrade_delay,
 			now + cfg.minimum_validation_upgrade_delay,
@@ -1765,10 +1773,12 @@ impl<T: Config> Pallet<T> {
 	///
 	/// The new code should not be equal to the current one, otherwise the upgrade will be aborted.
 	/// If there is already a scheduled code upgrade for the para, this is a no-op.
+	///
+	/// Inclusion block number specifies relay parent which enacted candidate initiating the upgrade.
 	pub(crate) fn schedule_code_upgrade(
 		id: ParaId,
 		new_code: ValidationCode,
-		relay_parent_number: BlockNumberFor<T>,
+		inclusion_block_number: BlockNumberFor<T>,
 		cfg: &configuration::HostConfiguration<BlockNumberFor<T>>,
 	) -> Weight {
 		let mut weight = T::DbWeight::get().reads(1);
@@ -1810,7 +1820,7 @@ impl<T: Config> Pallet<T> {
 		UpgradeRestrictionSignal::<T>::insert(&id, UpgradeRestriction::Present);
 
 		weight += T::DbWeight::get().reads_writes(1, 1);
-		let next_possible_upgrade_at = relay_parent_number + cfg.validation_upgrade_cooldown;
+		let next_possible_upgrade_at = inclusion_block_number + cfg.validation_upgrade_cooldown;
 		UpgradeCooldowns::<T>::mutate(|upgrade_cooldowns| {
 			let insert_idx = upgrade_cooldowns
 				.binary_search_by_key(&next_possible_upgrade_at, |&(_, b)| b)
@@ -1819,7 +1829,7 @@ impl<T: Config> Pallet<T> {
 		});
 
 		weight += Self::kick_off_pvf_check(
-			PvfCheckCause::Upgrade { id, relay_parent_number },
+			PvfCheckCause::Upgrade { id, included_at: inclusion_block_number },
 			code_hash,
 			new_code,
 			cfg,
@@ -2120,6 +2130,13 @@ impl<T: Config> Pallet<T> {
 
 		Heads::<T>::insert(&id, &genesis_data.genesis_head);
 	}
+
+	#[cfg(test)]
+	pub(crate) fn active_vote_state(
+		code_hash: &ValidationCodeHash,
+	) -> Option<PvfCheckActiveVoteState<BlockNumberFor<T>>> {
+		PvfActiveVoteMap::<T>::get(code_hash)
+	}
 }
 
 /// An overlay over the `Parachains` storage entry that provides a convenient interface for adding