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: try-runtime support, and manage author mapping blake2 migration #796

Merged
merged 105 commits into from
Oct 3, 2021

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Sep 9, 2021

What does it do?

This adds try-runtime support in pallet-migrations so that the feature can be used as it would in a normal migration.

What important points reviewers should know?

This PR currently uses notlesh-migration-sketch branch as the target for merging. I did this so we have the option of merging the other without this one.

Is there something left for follow-up PRs?

The motivation for this is, in part, to allow #679 to use both try-runtime and pallet-migrations.

TODO:

runtime/moonbase/src/migrations.rs Outdated Show resolved Hide resolved
runtime/moonbase/src/migrations.rs Outdated Show resolved Hide resolved
notlesh and others added 3 commits September 21, 2021 09:52
Co-authored-by: Joshy Orndorff <JoshOrndorff@users.noreply.github.com>
@notlesh
Copy link
Contributor Author

notlesh commented Sep 21, 2021

This is building and appears to be working now (with Joshy's author_mapping migration).

I was able to test with try-runtime against moonriver, but I needed to make a small mod to substrate:

diff --git a/utils/frame/remote-externalities/src/lib.rs b/utils/frame/remote-externalities/src/lib.rs
index 53c44780a6..8d1a686608 100644
--- a/utils/frame/remote-externalities/src/lib.rs
+++ b/utils/frame/remote-externalities/src/lib.rs
@@ -41,7 +41,7 @@ type KeyPair = (StorageKey, StorageData);

 const LOG_TARGET: &str = "remote-ext";
 const DEFAULT_TARGET: &str = "wss://rpc.polkadot.io";
-const BATCH_SIZE: usize = 1000;
+const BATCH_SIZE: usize = 100;

 jsonrpsee_proc_macros::rpc_client_api! {
    RpcApi<B: BlockT> {

Without this, I found that the requests would consistently time out at the same batch. This was when using a locally-synced archive node (requests going over loopback).

runtime/common/Cargo.toml Show resolved Hide resolved
@@ -237,7 +239,7 @@ pub mod pallet {
#[pallet::getter(fn account_and_deposit_of)]
/// We maintain a mapping from the AuthorIds used in the consensus layer
/// to the AccountIds runtime (including this staking pallet).
type MappingWithDeposit<T: Config> = StorageMap<
pub type MappingWithDeposit<T: Config> = StorageMap<
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are only using this in the crate context, we can make it pub crate only

Copy link
Contributor

@JoshOrndorff JoshOrndorff Oct 2, 2021

Choose a reason for hiding this comment

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

That's a valid point, but our other pallets also have them just pub (eg https://github.com/PureStake/moonbeam/blob/master/pallets/asset-manager/src/lib.rs#L119). And I worry that making stuff too restricted will make it hard to write precompiles in the short term.

Here's a followup issue to treat this throughout the repo. https://purestake.atlassian.net/browse/MOON-933

@JoshOrndorff JoshOrndorff added A8-mergeoncegreen Pull request is reviewed well. and removed A0-pleasereview Pull request needs code review. labels Oct 2, 2021
@JoshOrndorff JoshOrndorff changed the title try-runtime support for pallet-migrations pallet-migrations: try-runtime support, and manage author mapping blake2 migration Oct 2, 2021
@JoshOrndorff JoshOrndorff merged commit feab214 into master Oct 3, 2021
@JoshOrndorff JoshOrndorff deleted the notlesh-migration-pallet-try-runtime branch October 3, 2021 18:44
@notlesh notlesh added the D1-runtime-migration PR introduces code that might require downstream chains to run a runtime upgrade. label Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A8-mergeoncegreen Pull request is reviewed well. B0-silent Changes should not be mentioned in any release notes D1-runtime-migration PR introduces code that might require downstream chains to run a runtime upgrade. I4-tests 🎯 Tests need fixing, improving or augmenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants