From a6712c5f2bc9408acdb04f0c7e6c4a17edf3ef35 Mon Sep 17 00:00:00 2001 From: Dmitry Sinyavin Date: Sun, 12 Nov 2023 16:52:18 +0100 Subject: [PATCH 01/10] Try to remove offchain storage with a mock pallet --- polkadot/runtime/rococo/src/lib.rs | 49 ++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index 697d22c311ae..4eb74b5fe16c 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -1253,6 +1253,52 @@ impl pallet_asset_rate::Config for Runtime { type BenchmarkHelper = runtime_common::impls::benchmarks::AssetRateArguments; } +#[frame_support::pallet] +pub mod im_online_remover { + use frame_support::pallet_prelude::*; + use frame_system::pallet_prelude::*; + + #[pallet::pallet] + pub struct Pallet(_); + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::hooks] + impl Hooks> for Pallet { + fn on_initialize(n: BlockNumberFor) -> Weight { + if RemoveAtBlock::::get() == None { + RemoveAtBlock::::set(Some(n)); + } + Weight::zero() + } + + fn offchain_worker(n: BlockNumberFor) { + const DB_PREFIX: &[u8] = b"parity/im-online-heartbeat/"; + if let Some(remove_at) = RemoveAtBlock::::get() { + if remove_at == n { + let validator_set_size = + pallet_session::Pallet::::validators().len() as u32; + (0..validator_set_size).for_each(|idx| { + let key = { + let mut key = DB_PREFIX.to_vec(); + key.extend(idx.encode()); + key + }; + // FIXME: `StorageLock` needed? + sp_runtime::offchain::storage::StorageValueRef::persistent(&key).clear(); + }); + } + } + } + } + + #[pallet::storage] + pub(super) type RemoveAtBlock = StorageValue<_, BlockNumberFor, OptionQuery>; +} + +impl im_online_remover::Config for Runtime {} + construct_runtime! { pub enum Runtime { @@ -1370,6 +1416,8 @@ construct_runtime! { // Pallet for sending XCM. XcmPallet: pallet_xcm::{Pallet, Call, Storage, Event, Origin, Config} = 99, + ImOnlineRemover: im_online_remover::{Pallet, Storage} = 100, + ParasSudoWrapper: paras_sudo_wrapper::{Pallet, Call} = 250, AssignedSlots: assigned_slots::{Pallet, Call, Storage, Event, Config} = 251, @@ -1557,6 +1605,7 @@ mod benches { [runtime_parachains::paras_inherent, ParaInherent] [runtime_parachains::paras, Paras] [runtime_parachains::assigner_on_demand, OnDemandAssignmentProvider] + [im_online_remover, ImOnlineRemover] // Substrate [pallet_balances, Balances] [pallet_balances, NisCounterpartBalances] From 5a44e59758ece74aa7b8d9e008554be39503c6ad Mon Sep 17 00:00:00 2001 From: Dmitry Sinyavin Date: Sun, 21 Jan 2024 17:49:34 +0100 Subject: [PATCH 02/10] Use simpler strategy for storage cleanup --- polkadot/runtime/rococo/src/lib.rs | 55 +++------------------- substrate/frame/im-online/Cargo.toml | 1 + substrate/frame/im-online/src/migration.rs | 12 +++++ 3 files changed, 19 insertions(+), 49 deletions(-) diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index 377cb7f4994b..4ba7f8b53a75 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -1307,52 +1307,6 @@ impl pallet_asset_rate::Config for Runtime { type BenchmarkHelper = runtime_common::impls::benchmarks::AssetRateArguments; } -#[frame_support::pallet] -pub mod im_online_remover { - use frame_support::pallet_prelude::*; - use frame_system::pallet_prelude::*; - - #[pallet::pallet] - pub struct Pallet(_); - - #[pallet::config] - pub trait Config: frame_system::Config {} - - #[pallet::hooks] - impl Hooks> for Pallet { - fn on_initialize(n: BlockNumberFor) -> Weight { - if RemoveAtBlock::::get() == None { - RemoveAtBlock::::set(Some(n)); - } - Weight::zero() - } - - fn offchain_worker(n: BlockNumberFor) { - const DB_PREFIX: &[u8] = b"parity/im-online-heartbeat/"; - if let Some(remove_at) = RemoveAtBlock::::get() { - if remove_at == n { - let validator_set_size = - pallet_session::Pallet::::validators().len() as u32; - (0..validator_set_size).for_each(|idx| { - let key = { - let mut key = DB_PREFIX.to_vec(); - key.extend(idx.encode()); - key - }; - // FIXME: `StorageLock` needed? - sp_runtime::offchain::storage::StorageValueRef::persistent(&key).clear(); - }); - } - } - } - } - - #[pallet::storage] - pub(super) type RemoveAtBlock = StorageValue<_, BlockNumberFor, OptionQuery>; -} - -impl im_online_remover::Config for Runtime {} - construct_runtime! { pub enum Runtime { @@ -1469,8 +1423,6 @@ construct_runtime! { // Pallet for sending XCM. XcmPallet: pallet_xcm::{Pallet, Call, Storage, Event, Origin, Config} = 99, - ImOnlineRemover: im_online_remover::{Pallet, Storage} = 100, - // Pallet for migrating Identity to a parachain. To be removed post-migration. IdentityMigrator: identity_migrator::{Pallet, Call, Event} = 248, @@ -1768,7 +1720,6 @@ mod benches { [runtime_parachains::paras_inherent, ParaInherent] [runtime_parachains::paras, Paras] [runtime_parachains::assigner_on_demand, OnDemandAssignmentProvider] - [im_online_remover, ImOnlineRemover] // Substrate [pallet_balances, Balances] [pallet_balances, NisCounterpartBalances] @@ -1865,6 +1816,12 @@ sp_api::impl_runtime_apis! { impl offchain_primitives::OffchainWorkerApi for Runtime { fn offchain_worker(header: &::Header) { + use sp_runtime::{traits::Header, DigestItem}; + + if header.digest().logs().iter().find(|&di| *di == DigestItem::RuntimeEnvironmentUpdated).is_some() { + pallet_im_online::migration::clear_offchain_storage::() + } + Executive::offchain_worker(header) } } diff --git a/substrate/frame/im-online/Cargo.toml b/substrate/frame/im-online/Cargo.toml index b5b01858c898..e3b7af102aa0 100644 --- a/substrate/frame/im-online/Cargo.toml +++ b/substrate/frame/im-online/Cargo.toml @@ -23,6 +23,7 @@ frame-benchmarking = { path = "../benchmarking", default-features = false, optio frame-support = { path = "../support", default-features = false } frame-system = { path = "../system", default-features = false } pallet-authorship = { path = "../authorship", default-features = false } +pallet-session = { path = "../session", default-features = false } sp-application-crypto = { path = "../../primitives/application-crypto", default-features = false, features = ["serde"] } sp-core = { path = "../../primitives/core", default-features = false, features = ["serde"] } sp-io = { path = "../../primitives/io", default-features = false } diff --git a/substrate/frame/im-online/src/migration.rs b/substrate/frame/im-online/src/migration.rs index 84652965972e..a7ff98f5d4ed 100644 --- a/substrate/frame/im-online/src/migration.rs +++ b/substrate/frame/im-online/src/migration.rs @@ -116,6 +116,18 @@ pub mod v1 { } } +pub fn clear_offchain_storage() { + let validator_set_size = pallet_session::Pallet::::validators().len() as u32; + (0..validator_set_size).for_each(|idx| { + let key = { + let mut key = DB_PREFIX.to_vec(); + key.extend(idx.encode()); + key + }; + sp_runtime::offchain::storage::StorageValueRef::persistent(&key).clear(); + }); +} + #[cfg(all(feature = "try-runtime", test))] mod test { use super::*; From c5971e94f23df1163e819e105fda0744c2ffb85b Mon Sep 17 00:00:00 2001 From: Dmitry Sinyavin Date: Sun, 21 Jan 2024 19:19:27 +0100 Subject: [PATCH 03/10] Make clippy happy --- polkadot/runtime/rococo/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index ad13071f88de..e290cbbce93b 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -1828,7 +1828,7 @@ sp_api::impl_runtime_apis! { fn offchain_worker(header: &::Header) { use sp_runtime::{traits::Header, DigestItem}; - if header.digest().logs().iter().find(|&di| *di == DigestItem::RuntimeEnvironmentUpdated).is_some() { + if header.digest().logs().iter().any(|di| di == &DigestItem::RuntimeEnvironmentUpdated) { pallet_im_online::migration::clear_offchain_storage::() } From 654cd7214273da94c7a8a6934c525a6481eb7b76 Mon Sep 17 00:00:00 2001 From: Dmitry Sinyavin Date: Mon, 22 Jan 2024 14:59:33 +0100 Subject: [PATCH 04/10] Add prdoc --- prdoc/pr_2290.prdoc | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 prdoc/pr_2290.prdoc diff --git a/prdoc/pr_2290.prdoc b/prdoc/pr_2290.prdoc new file mode 100644 index 000000000000..9ea599066c92 --- /dev/null +++ b/prdoc/pr_2290.prdoc @@ -0,0 +1,11 @@ +title: im-online pallet offcain storage cleanup + +doc: + - audience: Runtime Dev + description: | + This is a follow-up of #2265. It blindly removes all the offchain storage values added to + the database by now retired im-online pallet. In absence of such values, does nothing. + +crates: + - name: pallet-im-online + - name: rococo-runtime From c0f9820148262d67466c8922be9cffd6584f2bbc Mon Sep 17 00:00:00 2001 From: Dmitry Sinyavin Date: Sun, 18 Feb 2024 17:29:54 +0100 Subject: [PATCH 05/10] Implement for Westend --- polkadot/runtime/westend/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 166c2d0d96e0..17c011f172df 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1794,6 +1794,12 @@ sp_api::impl_runtime_apis! { impl offchain_primitives::OffchainWorkerApi for Runtime { fn offchain_worker(header: &::Header) { + use sp_runtime::{traits::Header, DigestItem}; + + if header.digest().logs().iter().any(|di| di == &DigestItem::RuntimeEnvironmentUpdated) { + pallet_im_online::migration::clear_offchain_storage::() + } + Executive::offchain_worker(header) } } From 048399b4031ca09e18c93ce8d20926d39adb6236 Mon Sep 17 00:00:00 2001 From: Dmitry Sinyavin Date: Sun, 18 Feb 2024 20:32:25 +0100 Subject: [PATCH 06/10] Address discussions --- polkadot/runtime/rococo/src/lib.rs | 2 +- polkadot/runtime/westend/src/lib.rs | 2 +- prdoc/pr_2290.prdoc | 5 ++--- substrate/frame/im-online/Cargo.toml | 1 - substrate/frame/im-online/src/migration.rs | 3 +-- 5 files changed, 5 insertions(+), 8 deletions(-) diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index 3aa991c90531..03e96ab388b7 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -1827,7 +1827,7 @@ sp_api::impl_runtime_apis! { use sp_runtime::{traits::Header, DigestItem}; if header.digest().logs().iter().any(|di| di == &DigestItem::RuntimeEnvironmentUpdated) { - pallet_im_online::migration::clear_offchain_storage::() + pallet_im_online::migration::clear_offchain_storage(Session::validators().len() as u32); } Executive::offchain_worker(header) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 17c011f172df..03bd5b18b5be 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1797,7 +1797,7 @@ sp_api::impl_runtime_apis! { use sp_runtime::{traits::Header, DigestItem}; if header.digest().logs().iter().any(|di| di == &DigestItem::RuntimeEnvironmentUpdated) { - pallet_im_online::migration::clear_offchain_storage::() + pallet_im_online::migration::clear_offchain_storage(Session::validators().len() as u32); } Executive::offchain_worker(header) diff --git a/prdoc/pr_2290.prdoc b/prdoc/pr_2290.prdoc index 9ea599066c92..9f0476e91526 100644 --- a/prdoc/pr_2290.prdoc +++ b/prdoc/pr_2290.prdoc @@ -3,9 +3,8 @@ title: im-online pallet offcain storage cleanup doc: - audience: Runtime Dev description: | - This is a follow-up of #2265. It blindly removes all the offchain storage values added to - the database by now retired im-online pallet. In absence of such values, does nothing. + Adds a function `clear_offchain_storage` to `pallet-im-online`. This function can be used + after the pallet was removed to clear its offchain storage. crates: - name: pallet-im-online - - name: rococo-runtime diff --git a/substrate/frame/im-online/Cargo.toml b/substrate/frame/im-online/Cargo.toml index 825ca99a1d2b..038cbbcd678c 100644 --- a/substrate/frame/im-online/Cargo.toml +++ b/substrate/frame/im-online/Cargo.toml @@ -23,7 +23,6 @@ frame-benchmarking = { path = "../benchmarking", default-features = false, optio frame-support = { path = "../support", default-features = false } frame-system = { path = "../system", default-features = false } pallet-authorship = { path = "../authorship", default-features = false } -pallet-session = { path = "../session", default-features = false } sp-application-crypto = { path = "../../primitives/application-crypto", default-features = false, features = ["serde"] } sp-core = { path = "../../primitives/core", default-features = false, features = ["serde"] } sp-io = { path = "../../primitives/io", default-features = false } diff --git a/substrate/frame/im-online/src/migration.rs b/substrate/frame/im-online/src/migration.rs index 78337d247032..ae22a9cc7bc2 100644 --- a/substrate/frame/im-online/src/migration.rs +++ b/substrate/frame/im-online/src/migration.rs @@ -116,8 +116,7 @@ pub mod v1 { } } -pub fn clear_offchain_storage() { - let validator_set_size = pallet_session::Pallet::::validators().len() as u32; +pub fn clear_offchain_storage(validator_set_size: u32) { (0..validator_set_size).for_each(|idx| { let key = { let mut key = DB_PREFIX.to_vec(); From 77dc46302904872eb462cead200c3b16802a5b5c Mon Sep 17 00:00:00 2001 From: Dmitry Sinyavin Date: Sun, 18 Feb 2024 20:38:46 +0100 Subject: [PATCH 07/10] Add docs --- substrate/frame/im-online/src/migration.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/substrate/frame/im-online/src/migration.rs b/substrate/frame/im-online/src/migration.rs index ae22a9cc7bc2..5cd573d727a4 100644 --- a/substrate/frame/im-online/src/migration.rs +++ b/substrate/frame/im-online/src/migration.rs @@ -116,6 +116,8 @@ pub mod v1 { } } +/// Clears the pallet's offchain storage. Should be used in [offchain_primitives::OffchainWorkerApi] +/// implementation in runtime after the pallet was removed. pub fn clear_offchain_storage(validator_set_size: u32) { (0..validator_set_size).for_each(|idx| { let key = { From 2162d8dbfccab1f75f593bc7e93795567a22f630 Mon Sep 17 00:00:00 2001 From: Dmitry Sinyavin Date: Mon, 19 Feb 2024 10:03:06 +0100 Subject: [PATCH 08/10] Fix docs --- substrate/frame/im-online/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/im-online/src/migration.rs b/substrate/frame/im-online/src/migration.rs index 5cd573d727a4..b23101e770c5 100644 --- a/substrate/frame/im-online/src/migration.rs +++ b/substrate/frame/im-online/src/migration.rs @@ -116,7 +116,7 @@ pub mod v1 { } } -/// Clears the pallet's offchain storage. Should be used in [offchain_primitives::OffchainWorkerApi] +/// Clears the pallet's offchain storage. Should be used in `OffchainWorkerApi` /// implementation in runtime after the pallet was removed. pub fn clear_offchain_storage(validator_set_size: u32) { (0..validator_set_size).for_each(|idx| { From 0848a89d7276979ec0c5f39b0473c61dbbb573f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 19 Feb 2024 10:10:03 +0100 Subject: [PATCH 09/10] Update substrate/frame/im-online/src/migration.rs --- substrate/frame/im-online/src/migration.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/substrate/frame/im-online/src/migration.rs b/substrate/frame/im-online/src/migration.rs index b23101e770c5..34fa365b976d 100644 --- a/substrate/frame/im-online/src/migration.rs +++ b/substrate/frame/im-online/src/migration.rs @@ -116,8 +116,10 @@ pub mod v1 { } } -/// Clears the pallet's offchain storage. Should be used in `OffchainWorkerApi` -/// implementation in runtime after the pallet was removed. +/// Clears the pallet's offchain storage. +/// +/// Must be put in `OffchainWorkerApi::offchain_worker` after +/// the pallet was removed. pub fn clear_offchain_storage(validator_set_size: u32) { (0..validator_set_size).for_each(|idx| { let key = { From dccf8907b104c76c22b95398adbf37bc5300e590 Mon Sep 17 00:00:00 2001 From: Dmitry Sinyavin Date: Mon, 19 Feb 2024 10:16:39 +0100 Subject: [PATCH 10/10] `cargo fmt` --- substrate/frame/im-online/src/migration.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/im-online/src/migration.rs b/substrate/frame/im-online/src/migration.rs index 34fa365b976d..0d2c0a055b6d 100644 --- a/substrate/frame/im-online/src/migration.rs +++ b/substrate/frame/im-online/src/migration.rs @@ -116,9 +116,9 @@ pub mod v1 { } } -/// Clears the pallet's offchain storage. +/// Clears the pallet's offchain storage. /// -/// Must be put in `OffchainWorkerApi::offchain_worker` after +/// Must be put in `OffchainWorkerApi::offchain_worker` after /// the pallet was removed. pub fn clear_offchain_storage(validator_set_size: u32) { (0..validator_set_size).for_each(|idx| {