Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Migration testing runtime API/Bot #8038

Merged
25 commits merged into from
Feb 19, 2021

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Feb 3, 2021

A quick and dirty, but functioning attempt at making an end to end runtime api that can be triggered by a bot to test all the OnRuntimeUpgrade implementations inside a runtime, including two newly added hooks for some pre-and-post checks. I have one example of how these can work here: paritytech/polkadot@146f1e2#diff-db5840c3c03c740de25aed56b9257559c224a04619c079c94cddbb23f9a855a4R961

Given that it works now, I see this as a leaner and more minimalistic approach compared to #7665, but we can replace parts of this PR with test-runner once it is compatible.

My vision with this work is:

  1. Each substrate PR can request the OnRuntimeUpgrade of a particular pallet to be executed against the live state of any chain: \migration_bot run target:pallet-staking source:remote('http://remote-host:9933').
  2. Prior to each release, we run the same command once more on the polkadot node: \migration_bot run target:all cache('./path-to-cache-file.bin').

Current branch works fine, just needs a clean-up and configurability + making a few things generic (e.g. I assume Hash = H256 which is not good). In its current state, running cargo run -- dry-run --target all -lremote-ext=debug will yield something like:

2021-02-03 16:11:16.421   INFO main staking: 💸 new validator set of size 1 has been elected via ElectionCompute::OnChain for era 0
2021-02-03 16:11:16.443   INFO main remote-ext: scraping keypairs from remote node http://localhost:9933 @ 0x36b8402ce0037f61795cfa50a5615c3b0a30bd2dc2b5c2171faae3622886de12
2021-02-03 16:11:16.443   INFO main remote-ext: downloading data for all modules.
2021-02-03 16:12:18.371   INFO main remote-ext: injecting a total of 328567 keys
2021-02-03 16:15:27.709   INFO main node_runtime: !!! DRYRUN MIGRATION UP AHEAD !!!
2021-02-03 16:15:50.996   INFO main node_runtime: !!! DRYRUN MIGRATION DONE !!!

You can try the same, if you have a locally running node.

If this line of work is deemed okay, I will continue with cleaning the code, and making a companion PR that adds the same subcommand to polkadot as well.

CC @apopiak @seunlanlege @bkchr @shawntabrizi

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 3, 2021
@kianenigma kianenigma changed the title A clean new attempt Migration testing runtime API/Bot Feb 3, 2021
let heap_pages = Some(1024);
let executor = NativeExecutor::<ExecDispatch>::new(wasm_method, heap_pages, 2);

let ext = remote_externalities::Builder::new().inject(&[(code_key, code)]).build().await;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this .inject here is where the whole crux of the work happens: inject the real code (i.e. the one that is currently in this repo) into the remote fake state. I should double-check that this is what we want.

client/state-db/src/lib.rs Outdated Show resolved Hide resolved
@kianenigma kianenigma marked this pull request as ready for review February 11, 2021 10:57
@kianenigma
Copy link
Contributor Author

kianenigma commented Feb 11, 2021

This is more or less ready for review. A note about naming: I first called everything in this PR along the lines of DryRuntRuntimeUpgrade and such. But this is too specific. I foresee that in the future it will be useful (for me, at least) to add new cli commands and APIs that run the OnInitialize, or a particular Call of a new (not deployed) pallet over the state of a particular chain (or a range of blocks, updating as we go through). So I had to make it a bit more generalized, and I thought the Try prefix, such as TryFrom, TryInto and such are cool enough to go with. It is also much easier to type than dry-run-runtime-upgrade :D so now everything is called try-runtime and TryRuntime. Not perfect, but okay for now.

To test it out, try either of:

cargo remote run -- --release --features try-runtime --manifest-path bin/node/cli/Cargo.toml -- try-runtime --state file://Kusama,0xdd772d86d5cfd2be9a11dd504866c7878ee071aefe02ff7db3d749f98bf890b8,.bin --target Staking

or

cargo run --release --features try-runtime --manifest-path bin/node/cli/Cargo.toml -- try-runtime --state file://Kusama,0xdd772d86d5cfd2be9a11dd504866c7878ee071aefe02ff7db3d749f98bf890b8,.bin --target Staking

and put this file into your substrate root https://drive.google.com/file/d/1WMzYzQREBSlNfPMXcRT33Qq1SKJNqjzf/view?usp=sharing

@kianenigma kianenigma added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 11, 2021
type KeyPair = (StorageKey, StorageData);
type Number = u32;
type Hash = sp_core::H256;
// TODO: make these two generic.
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 would be happy to do this in a future PR. Given that we want to use this initially for known chains, I think it is an overkill for now.

}

#[async_std::test]
async fn can_load_cache() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

that binary file (20k) that I bushed will allow us to have at least this one test for this crate.. not sure what to do about it.

bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved
frame/executive/src/lib.rs Outdated Show resolved Hide resolved
frame/support/src/traits.rs Outdated Show resolved Hide resolved
frame/support/src/traits.rs Outdated Show resolved Hide resolved
frame/support/src/traits.rs Outdated Show resolved Hide resolved
frame/try-runtime/src/lib.rs Outdated Show resolved Hide resolved
utils/frame/try-runtime/cli/src/lib.rs Outdated Show resolved Hide resolved
utils/frame/try-runtime/cli/src/lib.rs Outdated Show resolved Hide resolved
utils/frame/try-runtime/cli/src/lib.rs Show resolved Hide resolved
utils/frame/try-runtime/cli/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Alexander Popiak <[email protected]>
Co-authored-by: Alexander Popiak <[email protected]>
Copy link
Contributor Author

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

I would highly appreciate a rather nimble review on this as I can use it quite a lot, and not needing to merge this branch into other works is a great luxury.

Note that I think there are lots of improvements to be made, this PR is just setting the foundation. All in all, I envision the TryRuntime api (all gated by try-runtime feature) to expose a lot of testing functionalities that would allow a runtime to be deeply diagnosed as a black box.

…' of github.com:paritytech/substrate into kiz-finally-finally-finally-finally-migration-testing-2
@arkpar
Copy link
Member

arkpar commented Feb 15, 2021

TryRuntimeCmd stuff looks good to me

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

what is utils/frame/try-runtime/remote-externalities/proxy_test ?

config.task_executor.clone(),
registry,
config.telemetry_span.clone(),
).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, why async here ? instead of sync like we do for benchmarks ?

Copy link
Contributor Author

@kianenigma kianenigma Feb 18, 2021

Choose a reason for hiding this comment

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

remote-externalities is working with async/await, my assumption was that in such case you need to use the async_runner 🤔

@@ -273,7 +273,7 @@ pub struct TaskManager {
impl TaskManager {
/// If a Prometheus registry is passed, it will be used to report statistics about the
/// service tasks.
pub(super) fn new(
pub fn new(
Copy link
Contributor

@gui1117 gui1117 Feb 18, 2021

Choose a reason for hiding this comment

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

this looks like a structural change but I don't know that much about the client to say.

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 think it is fine but yeah might be worth asking, maybe @cecton?

Copy link
Contributor

Choose a reason for hiding this comment

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

Normally you don't need to instantiate this but I guess in your case it makes sense if you want to benefit from the clean shutdown of the TaskManager 🤔 ... so I'm not opposed to it. @expenses maybe has another opinion on this?

On the other hand I'm worried by the tokio runtime created later on. You are creating and running 2 tokio runtimes.

utils/frame/try-runtime/remote-externalities/Cargo.toml Outdated Show resolved Hide resolved
primitives/core/src/hexdisplay.rs Outdated Show resolved Hide resolved
@kianenigma
Copy link
Contributor Author

what is utils/frame/try-runtime/remote-externalities/proxy_test ?

#8038 (comment)

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

I am pretty happy with this to start with. I expect it may change a bit as it gets used more, but since it is isolated under its own feature flag, seems super reasonable to merge soon.

utils/frame/try-runtime/remote-externalities/src/lib.rs Outdated Show resolved Hide resolved
bin/node/cli/src/command.rs Show resolved Hide resolved
@kianenigma
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Feb 19, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Feb 19, 2021

Checks failed; merge aborted.

@kianenigma
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Feb 19, 2021

Trying merge.

@ghost ghost merged commit 394c52e into master Feb 19, 2021
@ghost ghost deleted the kiz-finally-finally-finally-finally-migration-testing-2 branch February 19, 2021 14:52
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

I don't understand why this was merged with tons of unwraps in non-testing code. (Non-testing is everything that is not behind #[cfg(test)]

trace!(target: LOG_TARGET, "rpc: finalized_head");
let client: sc_rpc_api::chain::ChainClient<Number, Hash, (), ()> =
jsonrpc_core_client::transports::http::connect(&uri).wait().unwrap();
Ok(client.finalized_head().wait().unwrap())
Copy link
Member

Choose a reason for hiding this comment

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

Unwrap

rt.block_on::<_, _, ()>(futures::lazy(move || {
trace!(target: LOG_TARGET, "rpc: finalized_head");
let client: sc_rpc_api::chain::ChainClient<Number, Hash, (), ()> =
jsonrpc_core_client::transports::http::connect(&uri).wait().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Unwrap

rt.block_on::<_, _, ()>(futures::lazy(move || {
trace!(target: LOG_TARGET, "rpc: storage_pairs: {:?} / {:?}", prefix, at);
let client: sc_rpc_api::state::StateClient<Hash> =
jsonrpc_core_client::transports::http::connect(&uri).wait().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

And more and more

/// Relay the request to `state_getPairs` rpc endpoint.
///
/// Note that this is an unsafe RPC.
async fn rpc_get_pairs(&self, prefix: StorageKey, at: Hash) -> Vec<KeyPair> {
Copy link
Member

Choose a reason for hiding this comment

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

And why do we create a tokio runtime inside an async function!?

bin/node/cli/src/command.rs Show resolved Hide resolved
@kianenigma
Copy link
Contributor Author

Sorry, will fix the unwraps in #8123 😄 the async runtime is discussed here: #8038 (comment)

kianenigma added a commit to kianenigma/seeding that referenced this pull request Sep 27, 2022
# Membership Request 

Hi, I am Kian Paimani, known as @kianenigma. I have been working on Polkadot/Kusama through Parity since February 2019 and I can categorize my main contributions to Polkadot's ecosystem as follows: 

1. Maintaining and developing the staking sub-system.
2. General FRAME development, especially testing and quality assurance. 
3. Polkadot-native side-projects. 
4. Education 

> My first contribution to Polkadot is also indeed related to staking: paritytech/substrate#1915

### Staking system

I joke as the Polkadot staking to be both my blessing and my curse over the years. I started working on it since the first days that I joined this ecosystem and the work [is ongoing ever since](https://github.com/orgs/paritytech/projects/33/views/9). In the past, I focused on making sure that the staking system is secure and to some extent scalable. More recently, I coordinated the (imminent) launch of Nomination Pools. Nowadays I also put an extra effort on making sure that this sub-system of Polkadot is *sustainable*, through code refactor and educating other core developers. 

Lastly, I have been the main author of the [Polkadot staking newsletter](https://gist.github.com/kianenigma/aa835946455b9a3f167821b9d05ba376), which is my main attempt at making the entire complexity and development of this part of the protocol transparent to the end-users.

I expect myself to contribute *directly* to the staking system for at least another ~12, if not more, and afterwards having the role of an advisor. 

Some notable contributions: 

- paritytech/substrate#4517
- paritytech/substrate#7910
- paritytech/substrate#6242
- paritytech/substrate#9415
- paritytech/polkadot#3141
- paritytech/substrate#11212
- paritytech/substrate#12129

### FRAME 

Historically, I have contributed a variety of domains in FRAME, namely: 

- Early version of the weight system paritytech/substrate#3816 paritytech/substrate#3157
- Early version of the transaction fee system
- Primitive arithmetic types paritytech/substrate#3456
- Council election pallet paritytech/substrate#3364

Many of which were, admittedly, a PoC at most, if not considered "poor". I am happy that nowadays many of the above have been refactored and are being maintained by new domain experts. 

These days, I put most of my FRAME focus on testing and quality assurance. Through my work in the staking system, I have had to deal with the high sensitivity and liveness requirement of protocol development first hand (I believe I had to do among the [very first storage migrations](paritytech/substrate#3948) in Kusama) and consequently I felt the need to make better testing facilities, all of which have been formulated in https://forum.polkadot.network/t/testing-complex-frame-pallets-discussion-tools/356. Some relevant PRs:

- paritytech/substrate#8038
- paritytech/substrate#9788
- paritytech/substrate#10174

Regardless of wearing the staking hat, I plan to remain a direct contributor to FRAME, namely because I consider it to be an important requirements of successfully delivering more features to Polkadot's ecosystem. 

### Polkadot-Native Side Projects

I have started multiple small, mostly non-RUST projects in the polkadot ecosystem that I am very happy about, and I plan to continue doing so. I have not yet found the time to make a "polished product" out of any of these, but I hope that I can help foster our community such that someday a team will do so. I consider my role, for the time being, to *put ideas out there* through these side projects. 

- https://github.com/substrate-portfolio/polkadot-portfolio/
- https://github.com/kianenigma/polkadot-basic-notification/
- https://github.com/paritytech/polkadot-scripts/
- https://github.com/paritytech/substrate-debug-kit/

### Education 

Lastly, aside from having had a number of educational talks over the years (all of which [are listed](https://hello.kianenigma.nl/talks/) in my personal website), I am a big enthusiast of the newly formed Polkadot Blockchain Academy. I have [been an instructor](https://singular.app/collectibles/statemine/16/2) in the first cohort, and continue to contribute for as long and as much as I can, whilst still attending to the former 3 duties. 

---

With all of that being said and done, I consider myself at the beginning of the path to Dan 4, but happy to start at a lower one as well.
bkchr added a commit to polkadot-fellows/seeding that referenced this pull request Sep 27, 2022
# Membership Request 

Hi, I am Kian Paimani, known as @kianenigma. I have been working on Polkadot/Kusama through Parity since February 2019 and I can categorize my main contributions to Polkadot's ecosystem as follows: 

1. Maintaining and developing the staking sub-system.
2. General FRAME development, especially testing and quality assurance. 
3. Polkadot-native side-projects. 
4. Education 

> My first contribution to Polkadot is also indeed related to staking: paritytech/substrate#1915

### Staking system

I joke as the Polkadot staking to be both my blessing and my curse over the years. I started working on it since the first days that I joined this ecosystem and the work [is ongoing ever since](https://github.com/orgs/paritytech/projects/33/views/9). In the past, I focused on making sure that the staking system is secure and to some extent scalable. More recently, I coordinated the (imminent) launch of Nomination Pools. Nowadays I also put an extra effort on making sure that this sub-system of Polkadot is *sustainable*, through code refactor and educating other core developers. 

Lastly, I have been the main author of the [Polkadot staking newsletter](https://gist.github.com/kianenigma/aa835946455b9a3f167821b9d05ba376), which is my main attempt at making the entire complexity and development of this part of the protocol transparent to the end-users.

I expect myself to contribute *directly* to the staking system for at least another ~12, if not more, and afterwards having the role of an advisor. 

Some notable contributions: 

- paritytech/substrate#4517
- paritytech/substrate#7910
- paritytech/substrate#6242
- paritytech/substrate#9415
- paritytech/polkadot#3141
- paritytech/substrate#11212
- paritytech/substrate#12129

### FRAME 

Historically, I have contributed a variety of domains in FRAME, namely: 

- Early version of the weight system paritytech/substrate#3816 paritytech/substrate#3157
- Early version of the transaction fee system
- Primitive arithmetic types paritytech/substrate#3456
- Council election pallet paritytech/substrate#3364

Many of which were, admittedly, a PoC at most, if not considered "poor". I am happy that nowadays many of the above have been refactored and are being maintained by new domain experts. 

These days, I put most of my FRAME focus on testing and quality assurance. Through my work in the staking system, I have had to deal with the high sensitivity and liveness requirement of protocol development first hand (I believe I had to do among the [very first storage migrations](paritytech/substrate#3948) in Kusama) and consequently I felt the need to make better testing facilities, all of which have been formulated in https://forum.polkadot.network/t/testing-complex-frame-pallets-discussion-tools/356. Some relevant PRs:

- paritytech/substrate#8038
- paritytech/substrate#9788
- paritytech/substrate#10174

Regardless of wearing the staking hat, I plan to remain a direct contributor to FRAME, namely because I consider it to be an important requirements of successfully delivering more features to Polkadot's ecosystem. 

### Polkadot-Native Side Projects

I have started multiple small, mostly non-RUST projects in the polkadot ecosystem that I am very happy about, and I plan to continue doing so. I have not yet found the time to make a "polished product" out of any of these, but I hope that I can help foster our community such that someday a team will do so. I consider my role, for the time being, to *put ideas out there* through these side projects. 

- https://github.com/substrate-portfolio/polkadot-portfolio/
- https://github.com/kianenigma/polkadot-basic-notification/
- https://github.com/paritytech/polkadot-scripts/
- https://github.com/paritytech/substrate-debug-kit/

### Education 

Lastly, aside from having had a number of educational talks over the years (all of which [are listed](https://hello.kianenigma.nl/talks/) in my personal website), I am a big enthusiast of the newly formed Polkadot Blockchain Academy. I have [been an instructor](https://singular.app/collectibles/statemine/16/2) in the first cohort, and continue to contribute for as long and as much as I can, whilst still attending to the former 3 duties. 

---

With all of that being said and done, I consider myself at the beginning of the path to Dan 4, but happy to start at a lower one as well.

Co-authored-by: Bastian Köcher <[email protected]>
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants