Improve handling of unset `StorageVersion` #13417

bkchr · 2023-02-19T22:16:36Z

When a user is forgetting to set the storage version in a pallet and calls current_storage_version to compare it against the on_chain_storage_version it will now fail to compile the code. Before the pallet macro just returned StorageVersion::default() for current_storage_version leading to potential issues with migrations. Besides that it also checks in post_upgrade that the pallet storage version was upgraded and thus, no migration was missed.

Closes: #13062

Failing try-runtime tests

After this pr it can happen that try-runtime tests are failing, because pallets don't have the proper StorageVersion set on chain. This can either mean that a migration was missed or that a pallet was added after genesis and it was missed to set the storage version. To fix the later, it just requires a simple migration that does:

fn on_runtime_upgrade() {
    StorageVersion::new(EXPECTED_VERSION).put::<Pallet_That_Is_failing>();
}

After that try-runtime will be happy and everything should work.

When a user is forgetting to set the storage version in a pallet and calls `current_storage_version` to compare it against the `on_chain_storage_version` it will now fail to compile the code. Before the pallet macro just returned `StorageVersion::default()` for `current_storage_version` leading to potential issues with migrations. Besides that it also checks in `post_upgrade` that the pallet storage version was upgraded and thus, no migration was missed.

frame/support/procedural/src/pallet/expand/hooks.rs

frame/support/procedural/src/pallet/expand/pallet_struct.rs

ggwpez

Okay your approach is completely from what I used on rococo 😛

frame/support/src/traits/metadata.rs

frame/support/test/tests/pallet.rs

frame/support/src/migrations.rs

frame/support/test/tests/pallet.rs

ggwpez

Did you test it with the try-runtime CLI yet on a live network?

frame/support/procedural/src/pallet/expand/hooks.rs

ruseinov

Looks good. I’m still worried about the multi-block stuff, but since we can’t properly test it via try-runtime either it seems - makes no difference. The one scenario in which it could become annoying is when we have some CI check that would either fail try-runtime or we’d have to set the new StorageVersion before the storage has been fully migrated.
Just going to leave this concern here, perhaps we’ll get back to it when the solution is needed.

ggwpez · 2023-02-28T16:43:07Z

Ouch… https://substrate.stackexchange.com/questions/7396
Can we merge this soon please?

stale · 2023-04-06T15:23:12Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

Co-authored-by: Roman Useinov <roman.useinov@gmail.com>

ggwpez · 2023-04-28T10:26:38Z

It works now on Polkadot. Did we fix that version mismatch in pallet Historical?

ggwpez · 2023-04-28T12:33:15Z

frame/support/procedural/src/pallet/expand/hooks.rs

+					on_chain_version,
+				);
+
+				return Err("On chain storage version set, while the pallet doesn't \


Just one nit: you could collect all those errors in a vector and return them at the end. Then we can debug multiple erroneous pallets at once, like in Rococo.

frame/support/src/migrations.rs

frame/support/test/tests/pallet_ui/compare_unset_storage_version.stderr

bkchr · 2023-04-29T10:02:47Z

It works now on Polkadot. Did we fix that version mismatch in pallet Historical?

Good point. Should have been done as part of paritytech/polkadot@ccffb73

@gupnik could you please open a pr for your migration to set the storage version to 1 as well?

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

bkchr · 2023-05-04T10:56:21Z

bot rebase

…rage-version

paritytech-processbot · 2023-05-04T10:56:29Z

Rebased

* Improve handling of unset `StorageVersion` When a user is forgetting to set the storage version in a pallet and calls `current_storage_version` to compare it against the `on_chain_storage_version` it will now fail to compile the code. Before the pallet macro just returned `StorageVersion::default()` for `current_storage_version` leading to potential issues with migrations. Besides that it also checks in `post_upgrade` that the pallet storage version was upgraded and thus, no migration was missed. * Use correct `Cargo.lock` * Fixes * Fix test * Update frame/support/test/tests/pallet.rs * Ensure we don't set a storage version when the pallet is missing the attribute * Fix merge conflict * Update frame/support/procedural/src/pallet/expand/hooks.rs Co-authored-by: Roman Useinov <roman.useinov@gmail.com> * Update frame/support/procedural/src/pallet/expand/hooks.rs Co-authored-by: Roman Useinov <roman.useinov@gmail.com> * Fix compilation * Do not run everything with `try-runtime` * Fix test * Apply suggestions from code review Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Fix `no-metadata-docs` --------- Co-authored-by: Roman Useinov <roman.useinov@gmail.com> Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Co-authored-by: parity-processbot <>

bkchr requested a review from ggwpez February 19, 2023 22:16

bkchr requested a review from a team as a code owner February 19, 2023 22:16

Use correct Cargo.lock

35ec109

ruseinov reviewed Feb 19, 2023

View reviewed changes

frame/support/procedural/src/pallet/expand/hooks.rs Show resolved Hide resolved

ruseinov reviewed Feb 19, 2023

View reviewed changes

frame/support/procedural/src/pallet/expand/pallet_struct.rs Show resolved Hide resolved

bkchr added 2 commits February 20, 2023 08:55

Fixes

21595de

Fix test

2467c47

sergejparity approved these changes Feb 20, 2023

View reviewed changes

paritytech-ci requested a review from a team February 20, 2023 09:37

ggwpez reviewed Feb 20, 2023

View reviewed changes

frame/support/src/traits/metadata.rs Show resolved Hide resolved

frame/support/test/tests/pallet.rs Outdated Show resolved Hide resolved

frame/support/src/migrations.rs Show resolved Hide resolved

bkchr commented Feb 20, 2023

View reviewed changes

frame/support/test/tests/pallet.rs Outdated Show resolved Hide resolved

Update frame/support/test/tests/pallet.rs

f0b844b

ggwpez mentioned this pull request Feb 21, 2023

FRAME: Initialize StorageVersion on runtime upgrade #13430

Closed

ggwpez approved these changes Feb 21, 2023

View reviewed changes

frame/support/procedural/src/pallet/expand/hooks.rs Show resolved Hide resolved

ggwpez mentioned this pull request Feb 22, 2023

Depreciate missing pallet::storage_version #13435

Closed

ruseinov self-requested a review February 23, 2023 08:36

ruseinov approved these changes Feb 23, 2023

View reviewed changes

the-right-joyce added A0-please_review Pull request needs code review. and removed A0-pleasereview labels Mar 7, 2023

ggwpez mentioned this pull request Mar 27, 2023

[FRAME Core] Simple Multi-block Migrations paritytech/polkadot-sdk#198

Closed

stale bot added the A3-stale label Apr 6, 2023

bkchr removed the A3-stale label Apr 11, 2023

Update frame/support/procedural/src/pallet/expand/hooks.rs

43ee1a0

Co-authored-by: Roman Useinov <roman.useinov@gmail.com>

ggwpez mentioned this pull request Apr 28, 2023

try-runtime-cli panics on Polkadot #14006

Closed

bkchr added 2 commits April 28, 2023 11:26

Fix compilation

8021dfa

Do not run everything with try-runtime

7f0626b

liamaharon self-requested a review April 28, 2023 11:13

ggwpez approved these changes Apr 28, 2023

View reviewed changes

liamaharon approved these changes Apr 28, 2023

View reviewed changes

bkchr and others added 3 commits May 2, 2023 10:54

Fix test

2babce7

Apply suggestions from code review

8451fc5

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix no-metadata-docs

2ff5428

Merge remote-tracking branch 'origin/master' into bkchr-something-sto…

dd3ccd0

…rage-version

alvicsam approved these changes May 4, 2023

View reviewed changes

altaua approved these changes May 4, 2023

View reviewed changes

bkchr merged commit 133157a into master May 4, 2023

bkchr deleted the bkchr-something-storage-version branch May 4, 2023 19:18

This was referenced May 5, 2023

Backport .38 and .39 migrations to master paritytech/polkadot#7186

Closed

Add missing migrations paritytech/polkadot#7187

Closed

List migrations included in a runtime chevdor/subwasm#68

Closed

ggwpez mentioned this pull request May 18, 2023

Repair Rococo pallets paritytech/polkadot#6657

Closed

9 tasks

kianenigma mentioned this pull request May 29, 2023

Add Ability to Add/Remove Invulnerable Collators paritytech/cumulus#2596

Merged

github-actions bot mentioned this pull request Jun 15, 2023

Update substrate/polkadot/cumulus from v0.9.40 to v0.9.43 moonbeam-foundation/moonbeam#2354

Closed

kiltbot mentioned this pull request Jun 21, 2023

1️⃣ [AUTOMATIC] Update Polkadot dependencies from 0.9.42 to 0.9.43 KILTprotocol/kilt-node#531

Closed

kacperzuk-neti mentioned this pull request Jun 23, 2023

Polkadot v0.9.43 liberland/liberland_substrate#295

Merged

15 tasks

wischli mentioned this pull request Jul 7, 2023

Explicit storage version for all pallets centrifuge/centrifuge-chain#1295

Open

dmitrylavrenov mentioned this pull request Oct 29, 2024

Tracking issue to bump substrate related deps to polkadot-v0.9.43 humanode-network/humanode#923

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of unset `StorageVersion` #13417

Improve handling of unset `StorageVersion` #13417

bkchr commented Feb 19, 2023 •

edited

Loading

ggwpez left a comment •

edited

Loading

ggwpez left a comment

ruseinov left a comment

ggwpez commented Feb 28, 2023

stale bot commented Apr 6, 2023

ggwpez commented Apr 28, 2023

ggwpez Apr 28, 2023

bkchr commented Apr 29, 2023

bkchr commented May 4, 2023

paritytech-processbot bot commented May 4, 2023

Improve handling of unset StorageVersion #13417

Improve handling of unset StorageVersion #13417

Conversation

bkchr commented Feb 19, 2023 • edited Loading

Failing try-runtime tests

ggwpez left a comment • edited Loading

Choose a reason for hiding this comment

ggwpez left a comment

Choose a reason for hiding this comment

ruseinov left a comment

Choose a reason for hiding this comment

ggwpez commented Feb 28, 2023

stale bot commented Apr 6, 2023

ggwpez commented Apr 28, 2023

ggwpez Apr 28, 2023

Choose a reason for hiding this comment

bkchr commented Apr 29, 2023

bkchr commented May 4, 2023

paritytech-processbot bot commented May 4, 2023

Improve handling of unset `StorageVersion` #13417

Improve handling of unset `StorageVersion` #13417

bkchr commented Feb 19, 2023 •

edited

Loading

ggwpez left a comment •

edited

Loading