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

pallet-migrations Initial Implementation #527

Merged
merged 84 commits into from
Sep 20, 2021
Merged

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Jun 21, 2021

What does it do?

This PR adds a pallet-migrations which allows for composing migrations at runtime-level. Here are some goals of the design along with their status:

Aggregation

pallet-migrations allows for a runtime to aggregate various pallet migrations together such that they can form a persistent list yet only be applied exactly once. For projects (such as Moonbeam) with multiple deployed networks, this alleviates the need to plan runtime upgrades around individual migrations. For example, deployment B might have fewer runtime upgrades than deployment A but both could use the same list of migrations and neither would need to worry about when specific on_runtime_upgrade() implementations are added or removed.

The list of runtime upgrades is also ordered, which allows dependent upgrades to be explicitly handled. If a pallet undergoes several runtime upgrades which each depend on each other, this can be trivially handled through ordering.

One explicit side-effect of this feature is that a project's migrations can be enumerated in one place and in a consistent way. It's safe to leave a migration in the codebase (potentially no-op'ing it), which allows for a full historical context of migrations to exist in code.

Multi-block migrations

This was a feature that was fully implemented and then ripped out. It really needs to be implemented in Substrate to be done properly, especially for a general case.

This was a design feature from the outset, but now exists as an optional feature (see type MultiBlockMigrationsSupported: Get<bool>;). The rationale for including this feature is simply that some migrations may not fit within a single block. For a parachain (which must produce blocks which validators will accept), this is an inevitability. Worse, it may be an attack vector.

This feature is implemented by allowing the pallet to start by flagging that a runtime upgrade has started in on_runtime_upgrade() and then "stepping" through migrations on subsequent blocks (via on_initialize()) until they are done. It tracks weight consumed and provides it to each migration so that block weight can be used efficiently but without exceeding limits.

However, this feature requires more exploration. There currently is no mechanism for preventing a pallet's hooks from running. If these hooks depend on a migration, they might attempt to access inconsistent state. Compounding this problem, some pallets may be required to produce blocks (in Moonbeam's case, parachain-staking and author-mapping are both block-production-critical). Making sure that a pallet can support producing blocks at all points before, during, and after a runtime upgrade would not be trivial.

This PR, therefore, allows multi-block migrations to be optional and configures them as disabled.

/// This module acts as a registry where each migration is defined. Each migration should implement
/// the "Migration" trait declared in this crate.

struct MM_001_AuthorMappingAddDeposit;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

MM stands for Moonbeam Migration here. I'm giving these monotonically increasing numbers, something along the lines of how EIPs are named.

@crystalin
Copy link
Collaborator

So we would have each migration struct implementing the runtime_upgrade and we would include them (or not) in the runtime, is that correct ?

@notlesh
Copy link
Contributor Author

notlesh commented Jun 25, 2021

So we would have each migration struct implementing the runtime_upgrade and we would include them (or not) in the runtime, is that correct ?

The goal would be for each struct to reference the code that can live elsewhere. I'll try to mock that out so that it's clear.

on_runtime_upgrade() would check for each Migration whether or not it has been applied (using storage) and apply it if needed, something like:

for migration in Migrations {
    let has_been_applied = // query storage here to see if we've applied it
    if ! has_been_applied {
        // apply migration
        // update storage to show that we applied it
    }
}

Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me.

pallets/migrations/src/lib.rs Show resolved Hide resolved
log::error!(
"migrations weren't completed in on_runtime_upgrade(), but we're not
configured for multi-block migrations; state is potentially inconsistent!"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we panic in this case? If you panic the block is immediately invalid. But that may be better than getting one that we suspect is corrupted finalized in the relay chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've generally made the assumption that if we fail during a runtime upgrade and can't produce a block that we'll just end up doing the same thing when we try to produce another block.

One reason to allow the block is that we are better off if we at least continue producing blocks (e.g. we could vote to modify storage, do another runtime upgrade, etc.)

I've definitely gone back and forth on this, though. It's a situation where we can't do anything good...

pallets/migrations/src/lib.rs Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
@crystalin
Copy link
Collaborator

Can @4meta5, @JoshOrndorff give a final approval ? Then you can merge Stephen

Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

I like this idea, and I'm eager to get it in.

Here's an idea for a followup. What if your new trait here does't ecompass everything about an upgrade, but rather builds on top of FRAME's existing trait.

pub trait Migration: frame_support::traits::OnRuntimeUpgrade {
  	/// A human-readable name for this migration. Also used as storage key.
	fn friendly_name(&self) -> &str;
	
	/// Perform the required migration and return the weight consumed.
	fn migrate(&self, available_weight: Weight) -> Weight {
		//This is a provided method that calls into the OnRuntimeUpgrade::on_runtime_upgrade method
	}
}

@JoshOrndorff
Copy link
Contributor

I've tried to update this branch. The build fails locally for me with some very strange errors like this.

$ cargo check -p moonbeam-runtime
error: failed to select a version for `pallet-migrations`.
    ... required by package `moonbeam-runtime v0.8.4 (/home/joshy/ProgrammingProjects/moonbeam/runtime/moonbeam)`
versions that meet the requirements `=0.1.0` are: 0.1.0

the package `moonbeam-runtime` depends on `pallet-migrations`, with features: `sp-core` but `pallet-migrations` does not have these features.


failed to select a version for `pallet-migrations` which could resolve this conflict

It gives a similar error for all three runtimes, and it is not always sp-core.

/// Overarching event type
type Event: From<Event<Self>> + IsType<<Self as frame_system::Config>::Event>;
/// The list of migrations that will be performed
type MigrationsList: Get<Vec<Box<dyn Migration>>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with an earlier suggestion to change this trait bound to Migration and allow it to accept multiple migrations by implementing Migration for tuple instead of passing them in as a vec.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. We can do it in a followup though.

@JoshOrndorff JoshOrndorff added A8-mergeoncegreen Pull request is reviewed well. and removed A0-pleasereview Pull request needs code review. labels Sep 20, 2021
@JoshOrndorff JoshOrndorff merged commit c34608b into master Sep 20, 2021
@JoshOrndorff JoshOrndorff deleted the notlesh-migration-sketch branch September 20, 2021 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A1-needsburnin Pull request needs to be tested on a live validator node before merge. A8-mergeoncegreen Pull request is reviewed well. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants