From 9d5f3375e714f75bdd30753c211123f2cafc5bd2 Mon Sep 17 00:00:00 2001 From: Sergey Shulepov Date: Fri, 24 Dec 2021 17:58:36 +0000 Subject: [PATCH] paras: fix upgrade restriction signal Closes #3971 Read the linked issue. Apart from that, this addresses the concern raised in this [comment] by just adding a test. I couldn't find a clean way to reconcile a block number delay with a PVF voting TTL, so I just resorted to rely on the test. Should be fine for now. [comment]: https://github.com/paritytech/polkadot/pull/4457#discussion_r770517562 --- runtime/parachains/src/configuration.rs | 14 +++ runtime/parachains/src/dmp.rs | 4 +- runtime/parachains/src/hrmp.rs | 2 +- runtime/parachains/src/inclusion/tests.rs | 2 +- runtime/parachains/src/initializer.rs | 4 +- runtime/parachains/src/paras.rs | 119 ++++++++++++++++++++-- runtime/parachains/src/scheduler.rs | 4 +- 7 files changed, 130 insertions(+), 19 deletions(-) diff --git a/runtime/parachains/src/configuration.rs b/runtime/parachains/src/configuration.rs index dc303ef40672..9a2e740bda82 100644 --- a/runtime/parachains/src/configuration.rs +++ b/runtime/parachains/src/configuration.rs @@ -73,12 +73,26 @@ pub struct HostConfiguration { pub hrmp_max_message_num_per_candidate: u32, /// The minimum period, in blocks, between which parachains can update their validation code. /// + /// This number is used to prevent parachains from spamming the relay chain with validation code + /// upgrades. The only thing it controls is the number of blocks the `UpgradeRestrictionSignal` + /// is set for the parachain in question. + /// /// If PVF pre-checking is enabled this should be greater than the maximum number of blocks /// PVF pre-checking can take. Intuitively, this number should be greater than the duration /// specified by [`pvf_voting_ttl`]. Unlike, [`pvf_voting_ttl`], this parameter uses blocks /// as a unit. pub validation_upgrade_frequency: BlockNumber, /// The delay, in blocks, before a validation upgrade is applied. + /// + /// The delay is counted from the relay-parent of the candidate that signalled the upgrade. The + /// sum of that block and the this delay is dubbed `expected_at`. The first parachain block with + /// relay-parent >= `expected_at` will run the current code and will apply the upgrade. The next + /// parachain block will run the upgraded validation code. + /// + /// At some point we are going to change this to be counted from the moment when the candidate + /// that signalled the upgrade is enacted. See the issue [#4601]. + /// + /// [#4601]: https://github.com/paritytech/polkadot/issues/4601 pub validation_upgrade_delay: BlockNumber, /** diff --git a/runtime/parachains/src/dmp.rs b/runtime/parachains/src/dmp.rs index 08def77f9c35..f06ecdb89b82 100644 --- a/runtime/parachains/src/dmp.rs +++ b/runtime/parachains/src/dmp.rs @@ -238,7 +238,7 @@ mod tests { pub(crate) fn run_to_block(to: BlockNumber, new_session: Option>) { while System::block_number() < to { let b = System::block_number(); - Paras::initializer_finalize(); + Paras::initializer_finalize(b); Dmp::initializer_finalize(); if new_session.as_ref().map_or(false, |v| v.contains(&(b + 1))) { Dmp::initializer_on_new_session(&Default::default(), &Vec::new()); @@ -248,7 +248,7 @@ mod tests { System::on_initialize(b + 1); System::set_block_number(b + 1); - Paras::initializer_finalize(); + Paras::initializer_finalize(b + 1); Dmp::initializer_initialize(b + 1); } } diff --git a/runtime/parachains/src/hrmp.rs b/runtime/parachains/src/hrmp.rs index f132f584a0cf..161f2b0dfe61 100644 --- a/runtime/parachains/src/hrmp.rs +++ b/runtime/parachains/src/hrmp.rs @@ -1281,7 +1281,7 @@ mod tests { // NOTE: this is in reverse initialization order. Hrmp::initializer_finalize(); - Paras::initializer_finalize(); + Paras::initializer_finalize(b); ParasShared::initializer_finalize(); if new_session.as_ref().map_or(false, |v| v.contains(&(b + 1))) { diff --git a/runtime/parachains/src/inclusion/tests.rs b/runtime/parachains/src/inclusion/tests.rs index 4f727348d22f..f0acb8ff9999 100644 --- a/runtime/parachains/src/inclusion/tests.rs +++ b/runtime/parachains/src/inclusion/tests.rs @@ -170,7 +170,7 @@ pub(crate) fn run_to_block( let b = System::block_number(); ParaInclusion::initializer_finalize(); - Paras::initializer_finalize(); + Paras::initializer_finalize(b); ParasShared::initializer_finalize(); if let Some(notification) = new_session(b + 1) { diff --git a/runtime/parachains/src/initializer.rs b/runtime/parachains/src/initializer.rs index ebb2e4b9185f..4255d7a448e0 100644 --- a/runtime/parachains/src/initializer.rs +++ b/runtime/parachains/src/initializer.rs @@ -168,7 +168,7 @@ pub mod pallet { total_weight } - fn on_finalize(_: T::BlockNumber) { + fn on_finalize(now: T::BlockNumber) { // reverse initialization order. hrmp::Pallet::::initializer_finalize(); ump::Pallet::::initializer_finalize(); @@ -177,7 +177,7 @@ pub mod pallet { session_info::Pallet::::initializer_finalize(); inclusion::Pallet::::initializer_finalize(); scheduler::Pallet::::initializer_finalize(); - paras::Pallet::::initializer_finalize(); + paras::Pallet::::initializer_finalize(now); shared::Pallet::::initializer_finalize(); configuration::Pallet::::initializer_finalize(); diff --git a/runtime/parachains/src/paras.rs b/runtime/parachains/src/paras.rs index 8fdef24ec227..9b221f800834 100644 --- a/runtime/parachains/src/paras.rs +++ b/runtime/parachains/src/paras.rs @@ -923,7 +923,9 @@ impl Pallet { } /// Called by the initializer to finalize the configuration pallet. - pub(crate) fn initializer_finalize() {} + pub(crate) fn initializer_finalize(now: T::BlockNumber) { + Self::process_scheduled_upgrade_cooldowns(now); + } /// Called by the initializer to note that a new session has started. /// @@ -1151,10 +1153,13 @@ impl Pallet { } /// Process the timers related to upgrades. Specifically, the upgrade go ahead signals toggle - /// and the upgrade cooldown restrictions. - /// - /// Takes the current block number and returns the weight consumed. + /// and the upgrade cooldown restrictions. However, this function does not actually unset + /// the upgrade restriction, that will happen in the `initializer_finalize` function. However, + /// this function does count the number of cooldown timers expired so that we can reserve weight + /// for the `initializer_finalize` function. fn process_scheduled_upgrade_changes(now: T::BlockNumber) -> Weight { + // account weight for `UpcomingUpgrades::mutate`. + let mut weight = T::DbWeight::get().reads_writes(1, 1); let upgrades_signaled = ::UpcomingUpgrades::mutate( |upcoming_upgrades: &mut Vec<(ParaId, T::BlockNumber)>| { let num = upcoming_upgrades.iter().take_while(|&(_, at)| at <= &now).count(); @@ -1164,17 +1169,35 @@ impl Pallet { num }, ); - let cooldowns_expired = ::UpgradeCooldowns::mutate( + weight += T::DbWeight::get().writes(upgrades_signaled as u64); + + // account weight for `UpgradeCooldowns::get`. + weight += T::DbWeight::get().reads(1); + let cooldowns_expired = ::UpgradeCooldowns::get() + .iter() + .take_while(|&(_, at)| at <= &now) + .count(); + + // reserve weight for `initializer_finalize`: + // - 1 read and 1 write for `UpgradeCooldowns::mutate`. + // - 1 write per expired cooldown. + weight += T::DbWeight::get().reads_writes(1, 1); + weight += T::DbWeight::get().reads(cooldowns_expired as u64); + + weight + } + + /// Actually perform unsetting the expired upgrade restrictions. + /// + /// See `process_scheduled_upgrade_changes` for more details. + fn process_scheduled_upgrade_cooldowns(now: T::BlockNumber) { + ::UpgradeCooldowns::mutate( |upgrade_cooldowns: &mut Vec<(ParaId, T::BlockNumber)>| { - let num = upgrade_cooldowns.iter().take_while(|&(_, at)| at <= &now).count(); - for (para, _) in upgrade_cooldowns.drain(..num) { + for &(para, _) in upgrade_cooldowns.iter().take_while(|&(_, at)| at <= &now) { ::UpgradeRestrictionSignal::remove(¶); } - num }, ); - - T::DbWeight::get().reads_writes(2, upgrades_signaled as u64 + cooldowns_expired as u64) } /// Goes over all PVF votes in progress, reinitializes ballots, increments ages and prunes the @@ -1896,7 +1919,7 @@ mod tests { while System::block_number() < to { let b = System::block_number(); - Paras::initializer_finalize(); + Paras::initializer_finalize(b); ParasShared::initializer_finalize(); if new_session.as_ref().map_or(false, |v| v.contains(&(b + 1))) { let mut session_change_notification = SessionChangeNotification::default(); @@ -2421,6 +2444,7 @@ mod tests { // We expect that if an upgrade is signalled while there is already one pending we just // ignore it. Note that this is only true from perspective of this module. run_to_block(2, None); + assert!(!Paras::can_upgrade_validation_code(para_id)); Paras::schedule_code_upgrade(para_id, newer_code.clone(), 2, &Configuration::config()); assert_eq!( ::FutureCodeUpgrades::get(¶_id), @@ -2431,6 +2455,79 @@ mod tests { }); } + #[test] + fn upgrade_restriction_elapsed_doesnt_mean_can_upgrade() { + // Situation: parachain scheduled upgrade but it doesn't produce any candidate after + // `expected_at`. When `validation_upgrade_frequency` elapsed the parachain produces a + // candidate that tries to upgrade the code. + // + // In the current code this is not allowed: the upgrade should be consumed first. This is + // rather an artifact of the current implementation and not necessarily something we want + // to keep in the future. + // + // This test exists that this is not accidentially changed. + + let code_retention_period = 10; + let validation_upgrade_delay = 7; + let validation_upgrade_frequency = 30; + + let paras = vec![( + 0u32.into(), + ParaGenesisArgs { + parachain: true, + genesis_head: dummy_head_data(), + validation_code: vec![1, 2, 3].into(), + }, + )]; + + let genesis_config = MockGenesisConfig { + paras: GenesisConfig { paras, ..Default::default() }, + configuration: crate::configuration::GenesisConfig { + config: HostConfiguration { + code_retention_period, + validation_upgrade_delay, + validation_upgrade_frequency, + pvf_checking_enabled: false, + ..Default::default() + }, + ..Default::default() + }, + ..Default::default() + }; + + new_test_ext(genesis_config).execute_with(|| { + let para_id = 0u32.into(); + let new_code = ValidationCode(vec![4, 5, 6]); + let newer_code = ValidationCode(vec![4, 5, 6, 7]); + + run_to_block(1, None); + Paras::schedule_code_upgrade(para_id, new_code.clone(), 0, &Configuration::config()); + Paras::note_new_head(para_id, dummy_head_data(), 0); + assert_eq!( + ::UpgradeRestrictionSignal::get(¶_id), + Some(UpgradeRestriction::Present), + ); + assert_eq!( + ::FutureCodeUpgrades::get(¶_id), + Some(0 + validation_upgrade_delay) + ); + assert!(!Paras::can_upgrade_validation_code(para_id)); + + run_to_block(31, None); + assert!(::UpgradeRestrictionSignal::get(¶_id).is_none()); + + // Note the para still cannot upgrade the validation code. + assert!(!Paras::can_upgrade_validation_code(para_id)); + + // And scheduling another upgrade does not do anything. `expected_at` is still the same. + Paras::schedule_code_upgrade(para_id, newer_code.clone(), 30, &Configuration::config()); + assert_eq!( + ::FutureCodeUpgrades::get(¶_id), + Some(0 + validation_upgrade_delay) + ); + }); + } + #[test] fn full_parachain_cleanup_storage() { let code_retention_period = 20; diff --git a/runtime/parachains/src/scheduler.rs b/runtime/parachains/src/scheduler.rs index 984db51d8902..0719165f0f18 100644 --- a/runtime/parachains/src/scheduler.rs +++ b/runtime/parachains/src/scheduler.rs @@ -797,7 +797,7 @@ mod tests { let b = System::block_number(); Scheduler::initializer_finalize(); - Paras::initializer_finalize(); + Paras::initializer_finalize(b); if let Some(notification) = new_session(b + 1) { let mut notification_with_session_index = notification; @@ -831,7 +831,7 @@ mod tests { run_to_block(to, &new_session); Scheduler::initializer_finalize(); - Paras::initializer_finalize(); + Paras::initializer_finalize(to); if let Some(notification) = new_session(to + 1) { Paras::initializer_on_new_session(¬ification);