Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AssetRegistry: protect asset_registry migration #1594

Merged
merged 1 commit into from
Oct 18, 2023
Merged

AssetRegistry: protect asset_registry migration #1594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 1 addition & 23 deletions runtime/altair/src/migrations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,31 +62,9 @@ mod asset_registry {
use sp_std::{vec, vec::Vec};
use xcm::{v3::prelude::*, VersionedMultiLocation};

pub const ALTAIR_ASSET_LOC_COUNT: u32 = 5;
pub const ALTAIR_ASSET_METADATA_COUNT: u32 = 5;

pub struct AltairAssets;
impl runtime_common::migrations::asset_registry_xcmv3::AssetsToMigrate for AltairAssets {
fn get_assets_to_migrate(
loc_count: u32,
meta_count: u32,
) -> Vec<(
CurrencyId,
orml_asset_registry::AssetMetadata<Balance, CustomMetadata>,
)> {
match (loc_count, meta_count) {
(loc, meta)
if (loc, meta) == (ALTAIR_ASSET_LOC_COUNT, ALTAIR_ASSET_METADATA_COUNT) =>
{
Comment on lines -77 to -80
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check moved to the migration itself

Self::get_altair_assets()
}
_ => vec![],
}
}
}

impl AltairAssets {
pub fn get_altair_assets() -> Vec<(
fn get_assets_to_migrate() -> Vec<(
CurrencyId,
orml_asset_registry::AssetMetadata<Balance, CustomMetadata>,
)> {
Expand Down
4 changes: 3 additions & 1 deletion runtime/common/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ log = "0.4"
sp-io = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "polkadot-v0.9.38" }

[dev-dependencies]
cfg-mocks = { path = "../../libs/mocks", features = ["runtime-benchmarks", "std"] }
cfg-mocks = { path = "../../libs/mocks" }
pallet-collective = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.38" }
sp-io = { git = "https://github.com/paritytech/substrate", default-features = true, branch = "polkadot-v0.9.38" }

Expand Down Expand Up @@ -157,6 +157,7 @@ runtime-benchmarks = [
"xcm-executor/runtime-benchmarks",
"orml-asset-registry/runtime-benchmarks",
"polkadot-parachain/runtime-benchmarks",
"cfg-mocks/runtime-benchmarks",
]

on-chain-release-build = [
Expand Down Expand Up @@ -190,4 +191,5 @@ try-runtime = [
"pallet-investments/try-runtime",
"parachain-info/try-runtime",
"orml-asset-registry/try-runtime",
"cfg-mocks/try-runtime",
]
69 changes: 36 additions & 33 deletions runtime/common/src/migrations/asset_registry_xcmv3.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ impl<
// Complexity: 2 reads
let (loc_count, meta_count) = Self::get_key_counts();

if Self::check_key_counts(loc_count, meta_count).is_err() {
return RocksDbWeight::get().reads(loc_count.saturating_add(meta_count).into());
}

// Complexity: O(loc_count) writes
let result = orml_asset_registry::LocationToAssetId::<T>::clear(loc_count, None);
match result.maybe_cursor {
Expand Down Expand Up @@ -96,28 +100,26 @@ impl<
result.loops,
);

let assets = Assets::get_assets_to_migrate();
log::info!(
"💎 AssetRegistryMultilocationToXCMV3: Starting migration of {:?} assets",
Assets::get_assets_to_migrate(loc_count, meta_count)
.iter()
.len()
assets.iter().len()
);

// Complexity: O(meta_count + loc_count) writes
Assets::get_assets_to_migrate(loc_count, meta_count)
.into_iter()
.for_each(|(asset_id, asset_metadata)| {
log::debug!("Migrating asset: {:?}", asset_id);
orml_asset_registry::Pallet::<T>::do_register_asset_without_asset_processor(
asset_metadata.into(),
asset_id.into(),
)
.map_err(|e| log::error!("Failed to register asset id: {:?}", e))
.ok();
});
assets.into_iter().for_each(|(asset_id, asset_metadata)| {
log::debug!("Migrating asset: {:?}", asset_id);
orml_asset_registry::Pallet::<T>::do_register_asset_without_asset_processor(
asset_metadata.into(),
asset_id.into(),
)
.map_err(|e| log::error!("Failed to register asset id: {:?}", e))
.ok();
});

log::info!("💎 AssetRegistryMultilocationToXCMV3: on_runtime_upgrade: completed!");
RocksDbWeight::get().reads_writes(
2,
loc_count.saturating_add(meta_count).into(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add the cost of reading the previous keys here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch, I agree. StorageMap::clear(n, None) should have n reads and writes each!

loc_count
.saturating_add(meta_count)
.saturating_mul(2)
Expand All @@ -130,19 +132,7 @@ impl<
log::info!("💎 AssetRegistryMultilocationToXCMV3: pre-upgrade: started");
let (loc_count, meta_count) = Self::get_key_counts();

match (loc_count, meta_count) {
(loc, meta)
if (loc, meta) == (EXPECTED_MAINNET_LOC_COUNT, EXPECTED_MAINNET_META_COUNT) =>
{
Ok(())
}
(loc, meta)
if (loc, meta) == (EXPECTED_TESTNET_LOC_COUNT, EXPECTED_TESTNET_META_COUNT) =>
{
Ok(())
}
_ => Err("💎 AssetRegistryMultilocationToXCMV3: Unexpected counters"),
}?;
Self::check_key_counts(loc_count, meta_count)?;

log::info!("💎 AssetRegistryMultilocationToXCMV3: pre-upgrade: done");
Ok((loc_count, meta_count).encode())
Expand Down Expand Up @@ -185,7 +175,7 @@ impl<
{
fn get_key_counts() -> (u32, u32) {
// let loc_count =
// orml_asset_registry::LocationToAssetId::<T>::iter_keys().count() as u32;
// orml_asset_registry::LocationToAssetId::<T>::iter_keys().count() as u32;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why the commented line doesn't work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember that these commented lines did not work because of the existing decoding failure(s) on Altair related to these storages. Keys which cannot be decoded are not counted whereas count_storage_keys does not try to decode and thus counts correctly.

In the end these commented lines should have not been merged, sorry about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keys which cannot be decoded are not counted whereas count_storage_keys does not try to decode and thus counts correctly.

Good point! Thanks for expanding it.

// let meta_count = orml_asset_registry::Metadata::<T>::iter_keys().count() as
// u32;
let loc_count = Self::count_storage_keys(
Expand All @@ -206,6 +196,22 @@ impl<
(loc_count, meta_count)
}

fn check_key_counts(loc_count: u32, meta_count: u32) -> Result<(), &'static str> {
match (loc_count, meta_count) {
(loc, meta)
if (loc, meta) == (EXPECTED_MAINNET_LOC_COUNT, EXPECTED_MAINNET_META_COUNT) =>
{
Ok(())
}
(loc, meta)
if (loc, meta) == (EXPECTED_TESTNET_LOC_COUNT, EXPECTED_TESTNET_META_COUNT) =>
{
Ok(())
}
_ => Err("💎 AssetRegistryMultilocationToXCMV3: Unexpected counters"),
}
}

pub fn count_storage_keys(prefix: &[u8]) -> u32 {
let mut count = 0;
let mut next_key = prefix.to_vec();
Expand All @@ -225,10 +231,7 @@ impl<
}

pub trait AssetsToMigrate {
fn get_assets_to_migrate(
loc_count: u32,
meta_count: u32,
) -> Vec<(
fn get_assets_to_migrate() -> Vec<(
CurrencyId,
orml_asset_registry::AssetMetadata<Balance, CustomMetadata>,
)>;
Expand Down
Loading