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

Generic nuke migrations #1492

Merged
merged 8 commits into from Aug 17, 2023
Merged

Generic nuke migrations #1492

merged 8 commits into from Aug 17, 2023

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Aug 11, 2023

Description

This PR adds a nuke migration utility agnostic from any pallet to be used when we need to restore the storage of a pallet. Located in runtime-common.

To nuke the migration of a pallet you need to add the following line (supposing we want to nuke Loans from on-chain version 1):

runtime_common::migrations::nuke::Migration<Loans, RocksDbWeight, 1>

This PR do not remove/add any runtime migration

Partial_fix #1461

Testing

Tested in Altair (previously modifying the runtime by adding this migration) with the following:

RUST_LOG=runtime=trace,try-runtime::cli=trace,executor=trace cargo run --release --features try-runtime try-runtime --runtime target/release/wbuild/altair-runtime/altair_runtime.wasm --chain centrifuge on-runtime-upgrade live --uri wss://fullnode.altair.centrifuge.io:443

Done positive (when it works) & negative (when it shouldn't work) testing

@lemunozm lemunozm added P5-soon Issue should be addressed soon. D9-needsaudit Pull request touches sensitiv code parts and must be audited (externally). I6-refactoring Code needs refactoring. labels Aug 11, 2023
@lemunozm lemunozm self-assigned this Aug 11, 2023
@lemunozm lemunozm enabled auto-merge (squash) August 11, 2023 22:10
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

The code LGTM! I was wondering though whether it was desired not to apply the migration to any runtime? Just realized this after I could not get any logs when running against Altair or Centrifuge.

@mustermeiszer
Copy link
Collaborator

I would merge after #1493

@lemunozm
Copy link
Contributor Author

I was wondering though whether it was desired not to apply the migration to any runtime? Just realized this after I could not get any logs when running against Altair or Centrifuge.

@wischli, yeah, it all depends on the chain state at the moment of the runtime upgrade (at least for pallet-loans and pallet-interest-accrual). By now, it's added as a utility. On the release day, we should check those storages and add the nuke migration for them. Or maybe it would be more safe adding them now and remove whatever we found on the runtime upgrade day 🤔

@lemunozm lemunozm requested a review from wischli August 16, 2023 06:34
@wischli
Copy link
Contributor

wischli commented Aug 16, 2023

@wischli, yeah, it all depends on the chain state at the moment of the runtime upgrade (at least for pallet-loans and pallet-interest-accrual). By now, it's added as a utility. On the release day, we should check those storages and add the nuke migration for them. Or maybe it would be more safe adding them now and remove whatever we found on the runtime upgrade day 🤔

If this migration is planned to be part of the next release, I would like to add it now as otherwise the verification of the correctness of the migration will only be done during release and not as part of this PR. I don't have a strong opinion about this.

@lemunozm
Copy link
Contributor Author

For me, the purpose of this PR is only to add this utility for future usage in a common place, without affecting any behavior.

I can add a next PR with the required changes just after merging this in altair/centrifuge runtimes to remove old migrations and add this nuke migration to the required pallets for the next upgrade. WDYT? Just for splitting concerns in different PRs

Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Thanks for the conversation! Definitely in favor of splitting up PRs such that the addition to corresponding runtimes can be done in a subsequent PR.

@lemunozm lemunozm requested a review from wischli August 16, 2023 23:46
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Re-approving

@lemunozm lemunozm merged commit 5ae0caa into main Aug 17, 2023
22 checks passed
@lemunozm lemunozm deleted the nuke-migrations branch August 17, 2023 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D9-needsaudit Pull request touches sensitiv code parts and must be audited (externally). I6-refactoring Code needs refactoring. P5-soon Issue should be addressed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants