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.

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

Add awesome avatars multi-block-migration with pallet-migrations #21

Merged
merged 15 commits into from
Apr 25, 2024

Conversation

clangenb
Copy link
Contributor

@clangenb clangenb commented Apr 18, 2024

Tested by cloning all storage values of AAA and running the multiblock migration on a rococo-local setup:

Migration Metadata

Migration duration estimate: 108206 storage keys / 687 (avg migrations/block) = 157.5 blocks

Migration starting block: 118
Migration end estimate block = 118 + 158 = 276
Estimated elapsed time: 158 * 12s = 1896s = 31.6 min

Effective migration end in test = block 261

Post migration tests:

  • Tested a few avatars, they all have the minted_at: 0 field. 👍
  • Auxiliary storage TradeStatsMap is empty after the migration 👍
  • There are meany season stats and they have different values for bought/sold trades, which means that something has been migrated at least 👍.
  • PostMigration AAA-Keys count: 108212. So 6 keys more than pre-migration, which can be explained:
    • Season migration adds 3 new storages each: SeasonMetas, SeasonSchedules, SeasonTradeFilters. We have 2 seasons, hence we get 6 more keys. 👍
  • Executed a subsequent runtime to see if the upgrade process does still work 👍

Todo:

  • Test some specific storage keys manually for correct migration

Reproduce test results

  • Described in the zombienet/rococo-local-with-bajun-runtime-upgrade.toml file.

@clangenb clangenb marked this pull request as draft April 18, 2024 02:46
@clangenb clangenb force-pushed the cl/multiblock-migration branch from 8be8a4c to 9073b1b Compare April 23, 2024 15:42
Comment on lines +408 to +410
parameter_types! {
pub MbmServiceWeight: Weight = Perbill::from_percent(80) * RuntimeBlockWeights::get().max_block;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Todo: Double check if we should be more conservative here. Substrate doesn't have meaningful example value in their test runtime, but I think 80% should be ok.

Copy link
Contributor Author

@clangenb clangenb Apr 24, 2024

Choose a reason for hiding this comment

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

Parity has now introduced the same weight as mine. 80% percent allowed usage of a max value seems to be the rule of thumb for any weight-limited operation: https://github.com/paritytech/polkadot-sdk/pull/4251/files

impl FailedMigrationHandler for MockedFailedMigrationHandler {
fn failed(migration: Option<u32>) -> FailedMigrationHandling {
log::error!("FailedMigrationHandler failed at: {migration:?}");
FailedMigrationHandling::ForceUnstuck
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will make the chain resume normal operation and allow extrinsics again, but it will not continue the migration. Here, I suggest we assume that it works now after testing with the production data. Otherwise we could start integrating the pallet-safe-mode, which is not audited yet though.

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 suggest keeping it at ForceUnstuck. The only thing that could go wrong here is that a few entries are messed up, but no system logic is touched. pallet-safe-mode can be evaluated at another time.

Copy link

@ggwpez ggwpez Apr 26, 2024

Choose a reason for hiding this comment

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

To me it looks like you are actually using the freeze option @clangenb :
type FailedMigrationHandler = frame_support::migrations::FreezeChainOnFailedMigration;.
Anything starting with "Mock" should only be used for testing/benchmarking - not prod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooops, good catch. Fuck up on my end. Thanks for pointing this out!

@clangenb clangenb changed the title Add pallet-multiblock-migration Add awesome avatars multi-block-migration with pallet-migrations Apr 23, 2024
@clangenb clangenb marked this pull request as ready for review April 23, 2024 16:18
@@ -157,13 +158,8 @@ pub type Executive = frame_executive::Executive<
frame_system::ChainContext<Runtime>,
Runtime,
AllPalletsWithSystem,
Migrations,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is obsolete now, the single block migrations are also configured in frame-system now.

@clangenb clangenb merged commit 1ddf52b into develop Apr 25, 2024
4 of 5 checks passed
@clangenb clangenb deleted the cl/multiblock-migration branch April 25, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants