Fixed logic of the storage migration to triple reference counting. by remzrn · Pull Request #10337 · paritytech/substrate

remzrn · 2021-11-22T14:06:42Z

The previous behaviour made it impossible for any chain not already upgraded to dual reference counting to upgrade the runtime.

This proposed change is minimal, but attempts to fix this logical issue for runtime upgrades. Currently, migrations from single or u8 reference counting do not seem to be possible even with custom migrations, due to an enforced behaviour of the pallet migration. I tried to detail examples in the issue.

fixes #10336

cla-bot-2021 · 2021-11-22T14:06:47Z

User @remzrn, please sign the CLA here.

gui1117

looks good to me

remzrn · 2021-11-24T14:27:03Z

Not sure whether it is relevant to mention here, but I signed the CLA, and yet, it does not seem to update that status. I cannot sign a second time, since the link does not work anymore.
If there is indeed still an issue with CLA please provide me with another link and I will attempt to sign again.

bkchr · 2021-11-25T12:28:45Z

Not sure whether it is relevant to mention here, but I signed the CLA, and yet, it does not seem to update that status. I cannot sign a second time, since the link does not work anymore. If there is indeed still an issue with CLA please provide me with another link and I will attempt to sign again.

I think the problem is that you created the commit with an email that isn't linked to your github account. So, the tool can not connect you as a user to the commit. Could you please force push with a different email for the commit?

…e previous behaviour made it impossible for any chain not already upgraded to dual reference counting to upgrade the runtime.

kianenigma

Sorry for blocking, the code is correct but at least make a step toward the right direction (#10308 #8713). The correct thing here is to make functions migrate_from_single_to_triple_ref_count and migrate_from_dual_to_triple_ref_count standalone and remove any automatic migration in on_runtime_upgrade. Then, in your own runtime file you can pass either of the above two functions to type Executive to do your upgrade.

remzrn · 2021-11-28T12:59:36Z

Thanks for pointing this out! I think it totally makes sense to require that the migration logic can get controlled fully at runtime level.

I was unaware of those discussions and It just took me a bit of time to grok all this also because I am unfamiliar with substrate. I tried to stick to the example, so I hope I am not misusing anything.

I have tentatively implemented the changes you asked for, but unfortunately, I am unsure about the logic, and I have not been able to find out how to write tests for those. I would need help or pointers about how to do this. For Edgeware, we can try to see if that works during the testnet phase, but I guess there is a cleaner and more reliable way that just iterating version accross all testnet nodes to test and debug this.

One issue I have is that there is now a storage variable that flags whether the migration to triple reference counting has been performed, but it seems to be a renaming of a previous variable that flagged whether the status was upgraded to dual reference counting. Does it mean that it theory I could still fetch out the previous storage flag using generate_storage_alias? In which case I would know which migration to apply with reasonable certainty.
At the moment, I am relying on the fact that the translate() function from storage maps skips the elements it cannot decode, and assuming that if anything has been translated, the migration was the right one, otherwise, trying another one.
This is not ideal but runtime engineers can still choose to apply specific functions related to their exact state if they know where they are.

+Removed the specific migration .anciallaries from the frame-system pallet level. +Introducted a new module that hosts self-contained ancillary functions and logic to perform the storage migration. The current logic attempts to infer the state of the storage based on whether or not a given migration can be conducted.

gui1117 · 2021-11-29T08:53:45Z

One issue I have is that there is now a storage variable that flags whether the migration to triple reference counting has been performed, but it seems to be a renaming of a previous variable that flagged whether the status was upgraded to dual reference counting. Does it mean that it theory I could still fetch out the previous storage flag using generate_storage_alias? In which case I would know which migration to apply with reasonable certainty. At the moment, I am relying on the fact that the translate() function from storage maps skips the elements it cannot decode, and assuming that if anything has been translated, the migration was the right one, otherwise, trying another one. This is not ideal but runtime engineers can still choose to apply specific functions related to their exact state if they know where they are.

I think it is better to just let the user call the correct function from the on_runtime_upgrade implementation when declaring the runtime.

gui1117 · 2021-11-29T09:03:37Z

+/// (u32) to triple reference counting.
+///
+/// If unsure about the behaviour, the standalone functions below have to be preferred.
+pub fn apply<T: V2ToV3>() -> Weight {


personally I would just force user to use the correct migration explicitly.
Also iterating on storage is very heavy so I'm not sure it is good for any chain to try multiple migration.

Right. I removed applied and update the booleans, return weight, etc. on each individual function instead. Is there a reason why the weight should be maxvalue? I saw it done this way everywhere, but I don't understand the logic behind this. Is it just because migrations have a massive cost so we always assume maximum weight regardless of what is done?

Is it just because migrations have a massive cost so we always assume maximum weight regardless of what is done?

Yes this was the assumption, but it is a bad practice, because some chain must not go beyond max_block (like parachains), in those case having the exact weight is important.

remzrn · 2021-11-29T12:22:08Z

I think it is better to just let the user call the correct function from the on_runtime_upgrade implementation when declaring the runtime.

Would you then suggest to remove apply overall and then maybe add weights in each of them?

gui1117 · 2021-11-30T03:27:08Z

I think it is better to just let the user call the correct function from the on_runtime_upgrade implementation when declaring the runtime.

Would you then suggest to remove apply overall and then maybe add weights in each of them?

Yes, I personally think we can remove apply. About weight we can add some weight or let user benchmark the runtime migration on their own. I'm ok with both.

…ed during the runtime implementation of the trait V2ToV3. + Removed apply function. + Made the individual translation function self-sufficient.

gui1117

looks good to me

shawntabrizi · 2021-12-09T04:51:09Z

/tip small

substrate-tip-bot · 2021-12-09T04:51:10Z

Please fix the following problems before calling the tip bot again:

Contributor did not properly post their Polkadot or Kusama address. Make sure the pull request has: "{network} address: {address}".

shawntabrizi · 2021-12-09T05:05:40Z

bot merge

shawntabrizi · 2021-12-09T05:06:10Z

@remzrn you can still get a tip, just update your first post in this PR

…aritytech#10337) * Fixed logic of the storage migration to triple reference counting. The previous behaviour made it impossible for any chain not already upgraded to dual reference counting to upgrade the runtime. * +Removed the on_runtime_upgrade() function from frame-system. +Removed the specific migration .anciallaries from the frame-system pallet level. +Introducted a new module that hosts self-contained ancillary functions and logic to perform the storage migration. The current logic attempts to infer the state of the storage based on whether or not a given migration can be conducted. * Formatting. * + Removed specific AccountData struct. AccountData must now be provided during the runtime implementation of the trait V2ToV3. + Removed apply function. + Made the individual translation function self-sufficient. * + Removed unused decorators.

remzrn mentioned this pull request Nov 22, 2021

Storage migration to triple reference counting will always fail for chains that are not already upgraded to dual reference counting. #10336

Closed

gui1117 approved these changes Nov 23, 2021

View reviewed changes

Fixed logic of the storage migration to triple reference counting. Th…

b6e682f

…e previous behaviour made it impossible for any chain not already upgraded to dual reference counting to upgrade the runtime.

remzrn force-pushed the master branch from 48e0e81 to b6e682f Compare November 25, 2021 19:34

kianenigma changed the title ~~Fixed logic of the storage migration to triple reference counting. Th…~~ Fixed logic of the storage migration to triple reference counting. Nov 26, 2021

kianenigma approved these changes Nov 26, 2021

View reviewed changes

kianenigma suggested changes Nov 26, 2021

View reviewed changes

remzrn force-pushed the master branch from 8eb1c37 to 782f710 Compare November 29, 2021 08:09

Formatting.

5c607d4

remzrn force-pushed the master branch from 782f710 to 5c607d4 Compare November 29, 2021 08:09

gui1117 reviewed Nov 29, 2021

View reviewed changes

+ Removed specific AccountData struct. AccountData must now be provid…

b688798

…ed during the runtime implementation of the trait V2ToV3. + Removed apply function. + Made the individual translation function self-sufficient.

gui1117 approved these changes Dec 1, 2021

View reviewed changes

Comment thread frame/system/src/migrations/mod.rs Outdated

Comment thread frame/system/src/migrations/mod.rs Outdated

Comment thread frame/system/src/migrations/mod.rs Outdated

+ Removed unused decorators.

36fc243

bkchr requested a review from kianenigma December 1, 2021 21:19

shawntabrizi approved these changes Dec 9, 2021

View reviewed changes

kianenigma approved these changes Dec 9, 2021

View reviewed changes

paritytech-processbot Bot merged commit b45359a into paritytech:master Dec 9, 2021

Conversation

remzrn commented Nov 22, 2021 • edited by kianenigma Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cla-bot-2021 Bot commented Nov 22, 2021

Uh oh!

gui1117 left a comment

Choose a reason for hiding this comment

Uh oh!

remzrn commented Nov 24, 2021

Uh oh!

bkchr commented Nov 25, 2021

Uh oh!

kianenigma left a comment

Choose a reason for hiding this comment

Uh oh!

remzrn commented Nov 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gui1117 commented Nov 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gui1117 Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

remzrn Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gui1117 Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

remzrn commented Nov 29, 2021

Uh oh!

gui1117 commented Nov 30, 2021

Uh oh!

gui1117 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shawntabrizi commented Dec 9, 2021

Uh oh!

substrate-tip-bot Bot commented Dec 9, 2021

Uh oh!

shawntabrizi commented Dec 9, 2021

Uh oh!

shawntabrizi commented Dec 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

remzrn commented Nov 22, 2021 •

edited by kianenigma

Loading

remzrn commented Nov 28, 2021 •

edited

Loading

remzrn Nov 30, 2021 •

edited

Loading