From 926e4f6211f11b23b09e12c98ac6895e9afe8e68 Mon Sep 17 00:00:00 2001 From: Liam Date: Fri, 19 Apr 2024 17:30:57 +0400 Subject: [PATCH 1/6] fix: try-runtime checks --- pallets/subtensor/src/lib.rs | 2 +- runtime/src/lib.rs | 125 +---------- .../src/migrations/account_data_migration.rs | 205 ++++++++++++++++++ .../src/migrations/init_storage_versions.rs | 26 +++ runtime/src/migrations/mod.rs | 2 + 5 files changed, 243 insertions(+), 117 deletions(-) create mode 100644 runtime/src/migrations/account_data_migration.rs create mode 100644 runtime/src/migrations/init_storage_versions.rs create mode 100644 runtime/src/migrations/mod.rs diff --git a/pallets/subtensor/src/lib.rs b/pallets/subtensor/src/lib.rs index 55fc5a88a..11cb91277 100644 --- a/pallets/subtensor/src/lib.rs +++ b/pallets/subtensor/src/lib.rs @@ -74,7 +74,7 @@ pub mod pallet { // Tracks version for migrations. Should be monotonic with respect to the // order of migrations. (i.e. always increasing) - const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + const STORAGE_VERSION: StorageVersion = StorageVersion::new(6); #[pallet::pallet] #[pallet::without_storage_info] diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 85043d7d6..ed549436e 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -6,8 +6,11 @@ #[cfg(feature = "std")] include!(concat!(env!("OUT_DIR"), "/wasm_binary.rs")); +mod migrations; + use codec::{Decode, Encode, MaxEncodedLen}; +use migrations::{account_data_migration, init_storage_versions}; use pallet_commitments::CanCommit; use pallet_grandpa::{ fg_primitives, AuthorityId as GrandpaId, AuthorityList as GrandpaAuthorityList, @@ -1147,122 +1150,12 @@ pub type SignedExtra = ( pallet_commitments::CommitmentsSignedExtension, ); -mod account_data_migration { - use super::*; - use frame_support::log; - use pallet_balances::ExtraFlags; - - mod prev { - use super::*; - use frame_support::{pallet_prelude::ValueQuery, storage_alias, Blake2_128Concat}; - - #[derive(Encode, Decode, Clone, PartialEq, Eq, Default, Debug)] - pub struct AccountData { - pub free: Balance, - pub reserved: Balance, - pub misc_frozen: Balance, - pub fee_frozen: Balance, - } - - #[storage_alias] - pub type Account = StorageMap< - frame_system::pallet::Pallet, - Blake2_128Concat, - AccountId, - AccountData, - ValueQuery, - >; - } - - const TARGET: &'static str = "runtime::account_data_migration"; - pub struct Migration; - impl OnRuntimeUpgrade for Migration { - /// Save pre-upgrade account ids to check are decodable post-upgrade. - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { - let account_ids = prev::Account::::iter_keys().collect::>(); - log::info!(target: TARGET, "pre-upgrade"); - - Ok(account_ids.encode()) - } - - /// Ensures post-upgrade that - /// 1. Number of accounts has not changed. - /// 2. Each account exists in storage and decodes. - /// 3. Each account has a provider val >0. - #[cfg(feature = "try-runtime")] - fn post_upgrade(state: Vec) -> Result<(), sp_runtime::TryRuntimeError> { - use frame_support::ensure; - log::info!(target: TARGET, "Running post-upgrade..."); - - let pre_upgrade_account_ids: Vec<::AccountId> = - Decode::decode(&mut &state[..]).expect("pre_upgrade provides a valid state; qed"); - - // Ensure number of accounts has not changed. - let account_ids = prev::Account::::iter_keys().collect::>(); - ensure!( - pre_upgrade_account_ids.len() == account_ids.len(), - "number of accounts has changed" - ); - - for acc in account_ids { - // Ensure account exists in storage and decodes. - match frame_system::pallet::Account::::try_get(&acc) { - Ok(d) => { - ensure!(d.data.free > 0 || d.data.reserved > 0, "account has 0 bal"); - } - _ => { - panic!("account not found") - } - }; - - // Ensure account provider is >0. - ensure!( - frame_system::Pallet::::providers(&acc) > 0, - "provider == 0" - ); - } - - log::info!(target: TARGET, "post-upgrade success ✅"); - Ok(()) - } - - /// Migrates AccountData storage to the new format, bumping providers where required. - fn on_runtime_upgrade() -> Weight { - // Pull the storage in the previous format into memory - let accounts = prev::Account::::iter().collect::>(); - log::info!(target: TARGET, "Migrating {} accounts...", accounts.len()); - - for (acc, data) in accounts.clone().into_iter() { - // Move account to new data format - let new_data = pallet_balances::AccountData { - free: data.free, - reserved: data.reserved, - frozen: data.misc_frozen.saturating_add(data.fee_frozen), - flags: ExtraFlags::old_logic(), - }; - frame_system::pallet::Account::::mutate(acc.clone(), |a| { - a.data = new_data; - }); - - // Ensure provider - if frame_system::Pallet::::providers(&acc) == 0 { - frame_system::Pallet::::inc_providers(&acc); - } - - // Ensure upgraded - pallet_balances::Pallet::::ensure_upgraded(&acc); - } - - log::info!(target: TARGET, "Migrated {} accounts ✅", accounts.len()); - - // R/W not important for solo chain. - return ::DbWeight::get().reads_writes(0u64, 0u64); - } - } -} - -type Migrations = account_data_migration::Migration; +type Migrations = ( + init_storage_versions::Migration, + account_data_migration::Migration, + pallet_multisig::migrations::v1::MigrateToV1, + pallet_preimage::migration::v1::Migration, +); // Unchecked extrinsic type as expected by this runtime. pub type UncheckedExtrinsic = diff --git a/runtime/src/migrations/account_data_migration.rs b/runtime/src/migrations/account_data_migration.rs new file mode 100644 index 000000000..6378de8ea --- /dev/null +++ b/runtime/src/migrations/account_data_migration.rs @@ -0,0 +1,205 @@ +use crate::*; +use frame_support::log; +use pallet_balances::ExtraFlags; + +#[cfg(feature = "try-runtime")] +use sp_std::collections::btree_map::BTreeMap; + +mod prev { + use super::*; + use frame_support::{pallet_prelude::ValueQuery, storage_alias, Blake2_128Concat}; + + #[derive( + Clone, Eq, PartialEq, Default, RuntimeDebug, Encode, Decode, TypeInfo, MaxEncodedLen, + )] + pub struct AccountDataStruct { + pub free: Balance, + pub reserved: Balance, + pub misc_frozen: Balance, + pub fee_frozen: Balance, + } + + #[derive( + Clone, Eq, PartialEq, Default, RuntimeDebug, Encode, Decode, TypeInfo, MaxEncodedLen, + )] + pub struct AccountStruct { + pub nonce: u32, + pub consumers: u32, + pub providers: u32, + pub sufficients: u32, + pub data: AccountDataStruct, + } + + #[storage_alias] + pub type Account = StorageMap< + frame_system::pallet::Pallet, + Blake2_128Concat, + AccountId, + AccountStruct, + ValueQuery, + >; +} + +#[cfg(feature = "try-runtime")] +#[derive(Encode, Decode)] +enum PreUpgradeInfo { + MigrationAlreadyOccured, + MigrationShouldRun( + BTreeMap>>, + ), +} + +const TARGET: &'static str = "runtime::account_data_migration"; +pub struct Migration; +impl OnRuntimeUpgrade for Migration { + /// Save pre-upgrade account ids to check are decodable post-upgrade. + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { + // Skip migration if it already took place. + if migration_already_occured() { + return Ok(PreUpgradeInfo::MigrationAlreadyOccured.encode()); + } + + log::info!(target: TARGET, "pre-upgrade"); + // Save the expected post-migration account state. + let mut expected_account: BTreeMap< + AccountId, + frame_system::AccountInfo>, + > = BTreeMap::new(); + + for (acc_id, acc) in prev::Account::::iter() { + let expected_data = pallet_balances::AccountData { + free: acc.data.free, + reserved: acc.data.reserved, + frozen: acc.data.misc_frozen.saturating_add(acc.data.fee_frozen), + flags: ExtraFlags::default(), + }; + + // `ensure_upgraded` bumps the consumers if there is a non zero reserved balance and no frozen balance. + // https://github.com/paritytech/polkadot-sdk/blob/305d311d5c732fcc4629f3295768f1ed44ef434c/substrate/frame/balances/src/lib.rs#L785 + let expected_consumers = if acc.data.reserved > 0 && expected_data.frozen == 0 { + acc.consumers + 1 + } else { + acc.consumers + }; + let expected_acc = frame_system::AccountInfo { + nonce: acc.nonce, + consumers: expected_consumers, + providers: acc.providers, + sufficients: acc.sufficients, + data: expected_data, + }; + expected_account.insert(acc_id, expected_acc); + } + + Ok(PreUpgradeInfo::MigrationShouldRun(expected_account).encode()) + } + + /// Migrates Account storage to the new format, and calls `ensure_upgraded` for them. + fn on_runtime_upgrade() -> Weight { + // Skip migration if it already took place. + if migration_already_occured() { + log::warn!( + target: TARGET, + "Migration already completed and can be removed.", + ); + return ::DbWeight::get().reads_writes(0u64, 0u64); + } + + // Pull the storage in the previous format into memory + let accounts = prev::Account::::iter().collect::>(); + log::info!(target: TARGET, "Migrating {} accounts...", accounts.len()); + + for (acc_id, acc_info) in accounts.clone().into_iter() { + let prev_data = acc_info.clone().data; + + // Move account to new data format + let new_data = pallet_balances::AccountData { + free: prev_data.free, + reserved: prev_data.reserved, + frozen: prev_data.misc_frozen.saturating_add(prev_data.fee_frozen), + flags: ExtraFlags::old_logic(), + }; + let new_account = frame_system::AccountInfo { + nonce: acc_info.nonce, + consumers: acc_info.consumers, + providers: acc_info.providers, + sufficients: acc_info.sufficients, + data: new_data, + }; + frame_system::pallet::Account::::insert(acc_id.clone(), new_account); + + // Ensure upgraded + pallet_balances::Pallet::::ensure_upgraded(&acc_id); + } + + log::info!(target: TARGET, "Migrated {} accounts ✅", accounts.len()); + + // R/W not important for solo chain. + ::DbWeight::get().reads_writes(0u64, 0u64) + } + + /// Ensures post-upgrade that every Account entry matches what is expected. + #[cfg(feature = "try-runtime")] + fn post_upgrade(state: Vec) -> Result<(), sp_runtime::TryRuntimeError> { + use frame_support::ensure; + + log::info!(target: TARGET, "Running post-upgrade..."); + + let pre_upgrade_data: PreUpgradeInfo = + Decode::decode(&mut &state[..]).expect("decoding state failed"); + + match pre_upgrade_data { + PreUpgradeInfo::MigrationAlreadyOccured => Ok(()), + PreUpgradeInfo::MigrationShouldRun(expected_accounts) => { + // Ensure the actual post-migration state matches the expected + for (acc_id, acc) in frame_system::pallet::Account::::iter() { + let expected = expected_accounts.get(&acc_id).expect("account not found"); + + // New system logic nukes the account if no providers or sufficients. + if acc.providers > 0 || acc.sufficients > 0 { + ensure!(acc.nonce == expected.nonce, "nonce mismatch"); + ensure!(acc.consumers == expected.consumers, "consumers mismatch"); + ensure!(acc.providers == expected.providers, "providers mismatch"); + ensure!( + acc.sufficients == expected.sufficients, + "sufficients mismatch" + ); + ensure!(acc.data.free == expected.data.free, "data.free mismatch"); + ensure!( + acc.data.reserved == expected.data.reserved, + "data.reserved mismatch" + ); + ensure!( + acc.data.frozen == expected.data.frozen, + "data.frozen mismatch" + ); + ensure!(acc.data.flags == expected.data.flags, "data.flags mismatch"); + } + } + + log::info!(target: TARGET, "post-upgrade success ✅"); + Ok(()) + } + } + } +} + +/// Check if the migration already took place. The migration is all-or-nothing, so if one +/// account is migrated we know that the rest also must be migrated. +/// +/// We can use the nonce to check if it's been migrated, because it being zero meant that +/// the decode succeeded and this account has been migrated already. +/// +/// Performance note: While this may appear poor for performance due to touching all keys, +/// these keys will need to be touched anyway, so it's fine to just touch them loading them into +/// the storage overlay here. +fn migration_already_occured() -> bool { + for (_, acc) in frame_system::pallet::Account::::iter() { + if acc.nonce > 0 { + return true; + }; + } + + false +} diff --git a/runtime/src/migrations/init_storage_versions.rs b/runtime/src/migrations/init_storage_versions.rs new file mode 100644 index 000000000..5e0c95e30 --- /dev/null +++ b/runtime/src/migrations/init_storage_versions.rs @@ -0,0 +1,26 @@ +/// Init the on-chain storage versions of pallets added to the runtime prior to this being an automatic process. +use crate::*; + +pub struct Migration; + +impl OnRuntimeUpgrade for Migration { + fn on_runtime_upgrade() -> Weight { + use frame_support::traits::GetStorageVersion; + use frame_support::traits::StorageVersion; + + if Triumvirate::on_chain_storage_version() == StorageVersion::new(0) { + Triumvirate::current_storage_version().put::(); + } + if TriumvirateMembers::on_chain_storage_version() == StorageVersion::new(0) { + TriumvirateMembers::current_storage_version().put::(); + } + if SenateMembers::on_chain_storage_version() == StorageVersion::new(0) { + SenateMembers::current_storage_version().put::(); + } + if Scheduler::on_chain_storage_version() == StorageVersion::new(0) { + Scheduler::current_storage_version().put::(); + } + + ::DbWeight::get().reads_writes(4, 4) + } +} diff --git a/runtime/src/migrations/mod.rs b/runtime/src/migrations/mod.rs new file mode 100644 index 000000000..494f81fa9 --- /dev/null +++ b/runtime/src/migrations/mod.rs @@ -0,0 +1,2 @@ +pub mod account_data_migration; +pub mod init_storage_versions; From 2a76efb1dc8d791cfa991357f9e08d4dc23fe164 Mon Sep 17 00:00:00 2001 From: Liam Date: Fri, 19 Apr 2024 17:38:05 +0400 Subject: [PATCH 2/6] fix: comment --- runtime/src/migrations/init_storage_versions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/src/migrations/init_storage_versions.rs b/runtime/src/migrations/init_storage_versions.rs index 5e0c95e30..9ad0f9b2a 100644 --- a/runtime/src/migrations/init_storage_versions.rs +++ b/runtime/src/migrations/init_storage_versions.rs @@ -1,6 +1,6 @@ -/// Init the on-chain storage versions of pallets added to the runtime prior to this being an automatic process. use crate::*; +/// Init the on-chain storage versions of pallets added to the runtime prior to this being an automatic process. pub struct Migration; impl OnRuntimeUpgrade for Migration { From 4c2d3e333ed4b583700e1f2a6a8175944a75d3e6 Mon Sep 17 00:00:00 2001 From: Liam Date: Fri, 19 Apr 2024 17:43:35 +0400 Subject: [PATCH 3/6] add try-runtime ci --- .github/workflows/check-rust.yml | 42 ++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/.github/workflows/check-rust.yml b/.github/workflows/check-rust.yml index 61178b20a..9309582f9 100644 --- a/.github/workflows/check-rust.yml +++ b/.github/workflows/check-rust.yml @@ -352,3 +352,45 @@ jobs: - name: Check features run: zepter run check + + check-devnet-migrations: + runs-on: ubuntu-22.04 + steps: + - name: Checkout sources + uses: actions/checkout@v3 + + - name: Run Try Runtime Checks + uses: "paritytech/try-runtime-gha@v0.1.0" + with: + runtime-package: "node-subtensor-runtime" + node-uri: "wss://dev.chain.opentensor.ai:443" + checks: "pre-and-post" + extra-args: "--disable-spec-version-check --no-weight-warnings --disable-idempotency-checks" + + check-testnet-migrations: + runs-on: ubuntu-22.04 + steps: + - name: Checkout sources + uses: actions/checkout@v3 + + - name: Run Try Runtime Checks + uses: "paritytech/try-runtime-gha@v0.1.0" + with: + runtime-package: "node-subtensor-runtime" + node-uri: "wss://test.chain.opentensor.ai:443" + checks: "pre-and-post" + extra-args: "--disable-spec-version-check --no-weight-warnings --disable-idempotency-checks" + + check-finney-migrations: + runs-on: ubuntu-22.04 + steps: + - name: Checkout sources + uses: actions/checkout@v3 + + - name: Run Try Runtime Checks + uses: "paritytech/try-runtime-gha@v0.1.0" + with: + runtime-package: "node-subtensor-runtime" + node-uri: "wss://entrypoint-finney.opentensor.ai:443" + checks: "pre-and-post" + extra-args: "--disable-spec-version-check --no-weight-warnings --disable-idempotency-checks" From 5dc9a1a7a6e02d274999c4c8170f47f98d126432 Mon Sep 17 00:00:00 2001 From: Liam Date: Fri, 19 Apr 2024 17:45:46 +0400 Subject: [PATCH 4/6] fix: adds production profile --- Cargo.toml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index e7917c27b..c761877e4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,3 +13,8 @@ panic = "unwind" [profile.test] opt-level = 3 + +[profile.production] +inherits = "release" +lto = true +codegen-units = 1 From 5dc4c7947fdb98ad627cfa0adaa7b9f58d704acc Mon Sep 17 00:00:00 2001 From: Liam Date: Fri, 19 Apr 2024 17:51:28 +0400 Subject: [PATCH 5/6] ci names --- .github/workflows/check-rust.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/check-rust.yml b/.github/workflows/check-rust.yml index 9309582f9..d8501cfb2 100644 --- a/.github/workflows/check-rust.yml +++ b/.github/workflows/check-rust.yml @@ -354,6 +354,7 @@ jobs: run: zepter run check check-devnet-migrations: + name: check devnet migrations runs-on: ubuntu-22.04 steps: - name: Checkout sources @@ -368,6 +369,7 @@ jobs: extra-args: "--disable-spec-version-check --no-weight-warnings --disable-idempotency-checks" check-testnet-migrations: + name: check testnet migrations runs-on: ubuntu-22.04 steps: - name: Checkout sources @@ -382,6 +384,7 @@ jobs: extra-args: "--disable-spec-version-check --no-weight-warnings --disable-idempotency-checks" check-finney-migrations: + name: check finney migrations runs-on: ubuntu-22.04 steps: - name: Checkout sources From a2110af29de24981169037b563b0adae0f73a20f Mon Sep 17 00:00:00 2001 From: Liam Date: Fri, 19 Apr 2024 17:57:14 +0400 Subject: [PATCH 6/6] fix: migrations --- .github/workflows/check-rust.yml | 66 +++++++++++++++++--------------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/.github/workflows/check-rust.yml b/.github/workflows/check-rust.yml index d8501cfb2..266ed177a 100644 --- a/.github/workflows/check-rust.yml +++ b/.github/workflows/check-rust.yml @@ -353,36 +353,6 @@ jobs: - name: Check features run: zepter run check - check-devnet-migrations: - name: check devnet migrations - runs-on: ubuntu-22.04 - steps: - - name: Checkout sources - uses: actions/checkout@v3 - - - name: Run Try Runtime Checks - uses: "paritytech/try-runtime-gha@v0.1.0" - with: - runtime-package: "node-subtensor-runtime" - node-uri: "wss://dev.chain.opentensor.ai:443" - checks: "pre-and-post" - extra-args: "--disable-spec-version-check --no-weight-warnings --disable-idempotency-checks" - - check-testnet-migrations: - name: check testnet migrations - runs-on: ubuntu-22.04 - steps: - - name: Checkout sources - uses: actions/checkout@v3 - - - name: Run Try Runtime Checks - uses: "paritytech/try-runtime-gha@v0.1.0" - with: - runtime-package: "node-subtensor-runtime" - node-uri: "wss://test.chain.opentensor.ai:443" - checks: "pre-and-post" - extra-args: "--disable-spec-version-check --no-weight-warnings --disable-idempotency-checks" - check-finney-migrations: name: check finney migrations runs-on: ubuntu-22.04 @@ -397,3 +367,39 @@ jobs: node-uri: "wss://entrypoint-finney.opentensor.ai:443" checks: "pre-and-post" extra-args: "--disable-spec-version-check --no-weight-warnings --disable-idempotency-checks" + + # ---- + # We can enable devnet and finney migrations once Polkadot v1.0 is deployed to finney, after + # which time all future migrations should be idempotent and won't start failing after the + # upgrade is deployed. + # ---- + # check-devnet-migrations: + # name: check devnet migrations + # runs-on: ubuntu-22.04 + # steps: + # - name: Checkout sources + # uses: actions/checkout@v3 + # + # - name: Run Try Runtime Checks + # uses: "paritytech/try-runtime-gha@v0.1.0" + # with: + # runtime-package: "node-subtensor-runtime" + # node-uri: "wss://dev.chain.opentensor.ai:443" + # checks: "pre-and-post" + # extra-args: "--disable-spec-version-check --no-weight-warnings --disable-idempotency-checks" + # + # check-testnet-migrations: + # name: check testnet migrations + # runs-on: ubuntu-22.04 + # steps: + # - name: Checkout sources + # uses: actions/checkout@v3 + # + # - name: Run Try Runtime Checks + # uses: "paritytech/try-runtime-gha@v0.1.0" + # with: + # runtime-package: "node-subtensor-runtime" + # node-uri: "wss://test.chain.opentensor.ai:443" + # checks: "pre-and-post" + # extra-args: "--disable-spec-version-check --no-weight-warnings --disable-idempotency-checks" + #