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

Jump to bottom

AssetRegistry: protect asset_registry migration #1594

Merged
merged 1 commit into from
Oct 18, 2023
Merged

AssetRegistry: protect asset_registry migration #1594

merged 1 commit into from
Oct 18, 2023

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Oct 17, 2023 •
edited
Loading

Description

The check for the expected state of asset_registry migration is checked under try-runtime, if the chain state changes, the new state sets an empty vector, removing all currencies.

This PR modifies the migration only to take effect if the chain state is the expected state.

  • Checked manually with real altair environment

@lemunozm lemunozm added Q1-easy Can be done by primarily duplicating and adapting code. P7-asap Issue should be addressed in the next days. D8-migration Pull request touches storage and needs migration code. labels Oct 17, 2023
@lemunozm lemunozm self-assigned this Oct 17, 2023
@lemunozm lemunozm changed the title AssetRegsitry: protect asset_registry migration AssetRegistry: protect asset_registry migration Oct 17, 2023
lemunozm
lemunozm commented Oct 17, 2023
View reviewed changes
runtime/altair/src/migrations.rs
Comment on lines -77 to -80
match (loc_count, meta_count) {
(loc, meta)
if (loc, meta) == (ALTAIR_ASSET_LOC_COUNT, ALTAIR_ASSET_METADATA_COUNT) =>
{
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

runtime/common/src/migrations/asset_registry_xcmv3.rs

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!

runtime/common/src/migrations/asset_registry_xcmv3.rs
@@ -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.

@lemunozm
Copy link
Contributor Author

BTW, not sure if this migration should still be there or can be removed from altair (?)

@lemunozm
Copy link
Contributor Author

Confirmed, it's needed

@wischli
Copy link
Contributor

wischli commented Oct 17, 2023

Confirmed, it's needed

Yes. Next release must include Altair for sure. We have not treated it properly due to limited capacity.

@lemunozm
Copy link
Contributor Author

Checked against altair:

2023-10-17 14:01:08.694  INFO main runtime_common::migrations::asset_registry_xcmv3: 💎 AssetRegistryMultilocationToXCMV3: on_runtime_upgrade: started
2023-10-17 14:01:08.695  INFO main runtime_common::migrations::asset_registry_xcmv3: 💎 AssetRegistryMultilocationToXCMV3: Found 5 LocationToAssetId keys
2023-10-17 14:01:08.695  INFO main runtime_common::migrations::asset_registry_xcmv3: 💎 AssetRegistryMultilocationToXCMV3: Found 5 Metadata keys
2023-10-17 14:01:08.695  INFO main runtime_common::migrations::asset_registry_xcmv3: 💎 AssetRegistryMultilocationToXCMV3: Cleared all LocationToAssetId entries successfully
2023-10-17 14:01:08.695  INFO main runtime_common::migrations::asset_registry_xcmv3: 💎 AssetRegistryMultilocationToXCMV3: LocationToAssetId clearing iteration result. backend: 5 unique: 5 loops: 5
2023-10-17 14:01:08.695  INFO main runtime_common::migrations::asset_registry_xcmv3: Cleared all Metadata entries successfully
2023-10-17 14:01:08.695  INFO main runtime_common::migrations::asset_registry_xcmv3: 💎 AssetRegistryMultilocationToXCMV3: Metadata clearing iteration result. backend: 5 unique: 5 loops: 5
2023-10-17 14:01:08.695  INFO main runtime_common::migrations::asset_registry_xcmv3: 💎 AssetRegistryMultilocationToXCMV3: Starting migration of 4 assets
2023-10-17 14:01:08.695 DEBUG main runtime_common::migrations::asset_registry_xcmv3: Migrating asset: Native
2023-10-17 14:01:08.695 DEBUG main runtime_common::migrations::asset_registry_xcmv3: Migrating asset: ForeignAsset(1)
2023-10-17 14:01:08.695 DEBUG main runtime_common::migrations::asset_registry_xcmv3: Migrating asset: ForeignAsset(2)
2023-10-17 14:01:08.696 DEBUG main runtime_common::migrations::asset_registry_xcmv3: Migrating asset: ForeignAsset(3)
2023-10-17 14:01:08.696  INFO main runtime_common::migrations::asset_registry_xcmv3: 💎 AssetRegistryMultilocationToXCMV3: on_runtime_upgrade: completed!

@lemunozm lemunozm force-pushed the asset-registry/protect-migration branch from cfad34e to 6353ab0 Compare October 17, 2023 15:22
wischli
wischli approved these changes Oct 17, 2023
View reviewed changes
runtime/common/src/migrations/asset_registry_xcmv3.rs

log::info!("💎 AssetRegistryMultilocationToXCMV3: on_runtime_upgrade: completed!");
RocksDbWeight::get().reads_writes(
2,
loc_count.saturating_add(meta_count).into(),
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!

runtime/common/src/migrations/asset_registry_xcmv3.rs
@@ -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

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.

@lemunozm lemunozm enabled auto-merge (squash) October 17, 2023 15:29
@lemunozm
Copy link
Contributor Author

Thanks for your fast review as always @william!

@lemunozm lemunozm merged commit 06f3bc8 into main Oct 18, 2023
10 checks passed
@lemunozm lemunozm deleted the asset-registry/protect-migration branch October 18, 2023 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wischli wischli wischli approved these changes

@mustermeiszer mustermeiszer Awaiting requested review from mustermeiszer mustermeiszer is a code owner

@NunoAlexandre NunoAlexandre Awaiting requested review from NunoAlexandre

@hieronx hieronx Awaiting requested review from hieronx hieronx is a code owner

Assignees

@lemunozm lemunozm

Labels
D8-migration Pull request touches storage and needs migration code. P7-asap Issue should be addressed in the next days. Q1-easy Can be done by primarily duplicating and adapting code.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lemunozm @wischli