Skip to content
  • Liam Aharon's avatar
    Initialise on-chain `StorageVersion` for pallets added after genesis (#1297) · c4211b65
    Liam Aharon authored
    
    
    Original PR https://github.com/paritytech/substrate/pull/14641
    
    ---
    
    Closes https://github.com/paritytech/polkadot-sdk/issues/109
    
    ### Problem
    Quoting from the above issue:
    
    > When adding a pallet to chain after genesis we currently don't set the
    StorageVersion. So, when calling on_chain_storage_version it returns 0
    while the pallet is maybe already at storage version 9 when it was added
    to the chain. This could lead to issues when running migrations.
    
    ### Solution
    
    - Create a new trait `BeforeAllRuntimeMigrations` with a single method
    `fn before_all_runtime_migrations() -> Weight` trait with a noop default
    implementation
    - Modify `Executive` to call
    `BeforeAllRuntimeMigrations::before_all_runtime_migrations` for all
    pallets before running any other hooks
    - Implement `BeforeAllRuntimeMigrations` in the pallet proc macro to
    initialize the on-chain version to the current pallet version if the
    pallet has no storage set (indicating it has been recently added to the
    runtime and needs to have its version initialised).
    
    ### Other changes in this PR
    
    - Abstracted repeated boilerplate to access the `pallet_name` in the
    pallet expand proc macro.
    
    ### FAQ
    
    #### Why create a new hook instead of adding this logic to the pallet
    `pre_upgrade`?
    
    `Executive` currently runs `COnRuntimeUpgrade` (custom migrations)
    before `AllPalletsWithSystem` migrations. We need versions to be
    initialized before the `COnRuntimeUpgrade` migrations are run, because
    `COnRuntimeUpgrade` migrations may use the on-chain version for critical
    logic. e.g. `VersionedRuntimeUpgrade` uses it to decide whether or not
    to execute.
    
    We cannot reorder `COnRuntimeUpgrade` and `AllPalletsWithSystem` so
    `AllPalletsWithSystem` runs first, because `AllPalletsWithSystem` have
    some logic in their `post_upgrade` hooks to verify that the on-chain
    version and current pallet version match. A common use case of
    `COnRuntimeUpgrade` migrations is to perform a migration which will
    result in the versions matching, so if they were reordered these
    `post_upgrade` checks would fail.
    
    #### Why init the on-chain version for pallets without a current storage
    version?
    
    We must init the on-chain version for pallets even if they don't have a
    defined storage version so if there is a future version bump, the
    on-chain version is not automatically set to that new version without a
    proper migration.
    
    e.g. bad scenario:
    
    1. A pallet with no 'current version' is added to the runtime
    2. Later, the pallet is upgraded with the 'current version' getting set
    to 1 and a migration is added to Executive Migrations to migrate the
    storage from 0 to 1
        a. Runtime upgrade occurs
        b. `before_all` hook initializes the on-chain version to 1
    c. `on_runtime_upgrade` of the migration executes, and sees the on-chain
    version is already 1 therefore think storage is already migrated and
    does not execute the storage migration
    Now, on-chain version is 1 but storage is still at version 0.
    
    By always initializing the on-chain version when the pallet is added to
    the runtime we avoid that scenario.
    
    ---------
    
    Co-authored-by: default avatarKian Paimani <[email protected]>
    Co-authored-by: default avatarBastian Köcher <[email protected]>
    c4211b65