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

support STORAGE_VERSION for our pallets #726

Merged
merged 12 commits into from
Aug 6, 2022
Merged

support STORAGE_VERSION for our pallets #726

merged 12 commits into from
Aug 6, 2022

Conversation

flame4
Copy link
Contributor

@flame4 flame4 commented Jul 29, 2022

Description

relates to RP: #460

This PR support STORAGE_VERSION feature in our pallets. Since we never used PALLET_VERSION before, this is just a one-time work to add storage_version_prefix_key in node storage.
Otherwise, It could be a good demo for showing runtime upgrading.
In the future, this hooks could be removed to minimize the runtime size, which seems a good first issue for someone new.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Added one line describing your change in <branch>/CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer.

Situational Notes:

  • If adding functionality, write unit tests!
  • If importing a new pallet, choose a proper module index for it, and allow it in BaseFilter. Ensure every extrinsic works from front-end. If there's corresponding tool, ensure both work for each other.
  • If needed, update our Javascript/Typescript APIs. These APIs are officially used by exchanges or community developers.
  • If modifying existing runtime storage items, make sure to implement storage migrations for the runtime and test them with try-runtime. This includes migrations inherited from upstream changes, and you can search the diffs for modifications of #[pallet::storage] items to check for any.
  • If runtime changes, need to update the version numbers properly:
    • authoring_version: The version of the authorship interface. An authoring node will not attempt to author blocks unless this is equal to its native runtime.
    • spec_version: The version of the runtime specification. A full node will not attempt to use its native runtime in substitute for the on-chain Wasm runtime unless all of spec_name, spec_version, and authoring_version are the same between Wasm and native.
    • impl_version: The version of the implementation of the specification. Nodes are free to ignore this; it serves only as an indication that the code is different; as long as the other two versions are the same then while the actual code may be different, it is nonetheless required to do the same thing. Non-consensus-breaking optimizations are about the only changes that could be made which would result in only the impl_version changing.
    • transaction_version: The version of the extrinsics interface. This number must be updated in the following circumstances: extrinsic parameters (number, order, or types) have been changed; extrinsics or pallets have been removed; or the pallet order in the construct_runtime! macro or extrinsic order in a pallet has been changed. You can run the metadata_diff.yml workflow for help. If this number is updated, then the spec_version must also be updated
  • Verify benchmarks & weights have been updated for any modified runtime logics

Signed-off-by: Yi <flame0743@gmail.com>
Signed-off-by: Yi <flame0743@gmail.com>
Signed-off-by: Yi <flame0743@gmail.com>
Signed-off-by: Yi <flame0743@gmail.com>
Signed-off-by: Yi <flame0743@gmail.com>
@stechu
Copy link
Contributor

stechu commented Jul 29, 2022

Hey, please name the PR properly.

@flame4 flame4 changed the title Yi/i460 support STORAGE_VERSION for our pallets Aug 1, 2022
Signed-off-by: Yi <flame0743@gmail.com>
Signed-off-by: Yi <flame0743@gmail.com>
@flame4 flame4 added C-enhancement Category: An issue proposing an enhancement or a PR with one A-runtime Area: Issues and PRs related to Runtimes L-changed Log: Issues and PRs related to changes labels Aug 1, 2022
Copy link
Contributor

@ghzlatarev ghzlatarev left a comment

Choose a reason for hiding this comment

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

Look at how the previous migrations used the try-runtime feature and test this migration with it as well.
try-runtime basically scrapes all the data from a live chain (eg: Calamari on Kusama) and insert it in a local network to test any storage migrations that may happen. There are pre and post migration hooks where you can check the state.
You can use this workflow for that https://github.com/Manta-Network/Manta/actions/workflows/try-runtime-calamari-mainnet.yml or do it locally.

@ghzlatarev ghzlatarev linked an issue Aug 3, 2022 that may be closed by this pull request
Signed-off-by: Yi <flame0743@gmail.com>
Signed-off-by: Yi <flame0743@gmail.com>
@flame4
Copy link
Contributor Author

flame4 commented Aug 5, 2022

Look at how the previous migrations used the try-runtime feature and test this migration with it as well. try-runtime basically scrapes all the data from a live chain (eg: Calamari on Kusama) and insert it in a local network to test any storage migrations that may happen. There are pre and post migration hooks where you can check the state. You can use this workflow for that https://github.com/Manta-Network/Manta/actions/workflows/try-runtime-calamari-mainnet.yml or do it locally.

Nice Advice. I've tried it using like RUST_LOG=runtime=trace,try-runtime::cli=trace,executor=trace cargo run --release --features=try-runtime try-runtime --chain calamari-dev on-runtime-upgrade live --uri=wss://ws.calamari.systems:443.

Yeah. Actually I met some trouble like I can't use format! and print! in no_std, and i found there's already some StorageVersion in state store. But after a lot of struggling, I can pass the try_runtime test and get below logging:

2022-08-05 17:08:58.609 DEBUG                 main runtime: "----PostUpgrade----"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "pallet_asset_manager"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "AssetManager"
2022-08-05 17:08:58.609 DEBUG                 main runtime: StorageVersion(1)
2022-08-05 17:08:58.609 DEBUG                 main runtime: "----PostUpgrade----"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "pallet_tx_pause"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "TransactionPause"
2022-08-05 17:08:58.609 DEBUG                 main runtime: StorageVersion(1)
2022-08-05 17:08:58.609 DEBUG                 main runtime: "----PostUpgrade----"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "manta_collator_selection"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "CollatorSelection"
2022-08-05 17:08:58.609 DEBUG                 main runtime: StorageVersion(1)
2022-08-05 17:08:58.609 DEBUG                 main runtime: "----PostUpgrade----"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "calamari_vesting"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "CalamariVesting"
2022-08-05 17:08:58.609 DEBUG                 main runtime: StorageVersion(1)
2022-08-05 17:08:58.633  INFO                 main try-runtime::cli: TryRuntime_on_runtime_upgrade executed without errors. Consumed weight = 500900000000, total weight = 500000000000 (1.0018)

@ghzlatarev
Copy link
Contributor

Look at how the previous migrations used the try-runtime feature and test this migration with it as well. try-runtime basically scrapes all the data from a live chain (eg: Calamari on Kusama) and insert it in a local network to test any storage migrations that may happen. There are pre and post migration hooks where you can check the state. You can use this workflow for that https://github.com/Manta-Network/Manta/actions/workflows/try-runtime-calamari-mainnet.yml or do it locally.

Nice Advice. I've tried it using like RUST_LOG=runtime=trace,try-runtime::cli=trace,executor=trace cargo run --release --features=try-runtime try-runtime --chain calamari-dev on-runtime-upgrade live --uri=wss://ws.calamari.systems:443.

Yeah. Actually I met some trouble like I can't use format! and print! in no_std, and i found there's already some StorageVersion in state store. But after a lot of struggling, I can pass the try_runtime test and get below logging:

2022-08-05 17:08:58.609 DEBUG                 main runtime: "----PostUpgrade----"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "pallet_asset_manager"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "AssetManager"
2022-08-05 17:08:58.609 DEBUG                 main runtime: StorageVersion(1)
2022-08-05 17:08:58.609 DEBUG                 main runtime: "----PostUpgrade----"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "pallet_tx_pause"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "TransactionPause"
2022-08-05 17:08:58.609 DEBUG                 main runtime: StorageVersion(1)
2022-08-05 17:08:58.609 DEBUG                 main runtime: "----PostUpgrade----"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "manta_collator_selection"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "CollatorSelection"
2022-08-05 17:08:58.609 DEBUG                 main runtime: StorageVersion(1)
2022-08-05 17:08:58.609 DEBUG                 main runtime: "----PostUpgrade----"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "calamari_vesting"
2022-08-05 17:08:58.609 DEBUG                 main runtime: "CalamariVesting"
2022-08-05 17:08:58.609 DEBUG                 main runtime: StorageVersion(1)
2022-08-05 17:08:58.633  INFO                 main try-runtime::cli: TryRuntime_on_runtime_upgrade executed without errors. Consumed weight = 500900000000, total weight = 500000000000 (1.0018)

You should be able to print with log::info! for example, but not everything will show up on wasm.

Signed-off-by: Yi <flame0743@gmail.com>
@flame4 flame4 requested a review from ghzlatarev August 5, 2022 09:50
Copy link
Contributor

@Dengjianping Dengjianping left a comment

Choose a reason for hiding this comment

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

lgtm!

@ghzlatarev ghzlatarev merged commit 0d63cc4 into manta Aug 6, 2022
@ghzlatarev ghzlatarev deleted the Yi/i460 branch August 6, 2022 14:31
Copy link
Contributor

@Garandor Garandor left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the clean PR!

I've noticed you updated our own pallets but none of the dependency pallets we use from parity.

Have you checked them all and ensured that e.g. pallet_treasury in the current version (polkadot-v0.9.26) we use does not provide the const STORAGE_VERSION variable in code? Because if you find any that already support this, part of the issue was to migrate those as well

@flame4
Copy link
Contributor Author

flame4 commented Aug 8, 2022

LGTM, thanks for the clean PR!

I've noticed you updated our own pallets but none of the dependency pallets we use from parity.

Have you checked them all and ensured that e.g. pallet_treasury in the current version (polkadot-v0.9.26) we use does not provide the const STORAGE_VERSION variable in code? Because if you find any that already support this, part of the issue was to migrate those as well

i think it's better to add the dependency checking into the task #463 . so the upstream checking will be there. @Garandor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-runtime Area: Issues and PRs related to Runtimes C-enhancement Category: An issue proposing an enhancement or a PR with one L-changed Log: Issues and PRs related to changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calamari: Migrate all pallets from PalletVersion to StorageVersion
5 participants