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

try-runtime - support all state versions #12089

Merged
merged 8 commits into from
Aug 27, 2022
Merged

try-runtime - support all state versions #12089

merged 8 commits into from
Aug 27, 2022

Conversation

pmikolajczyk41
Copy link
Contributor

@pmikolajczyk41 pmikolajczyk41 commented Aug 23, 2022 •
edited
Loading

Currently, every subcommand of try-runtime operates on externalities with StateVersion(1) (which is the default for RemoteExternalities - https://github.com/paritytech/substrate/blob/master/utils/frame/remote-externalities/src/lib.rs#L269). Although it will work with any remote chain without any warning or check, some problems may rise if chain uses another state version. For example, Polkadot (afaik) and AlephZero use StateVersion(0).

This PR introduces new CLI option which enables user to specify target state version. By default it is StateVersion::1.

cc: @kianenigma

Polkadot address: 15fdtPm7jNN9VZk5LokKcmBVfmYZkCXdjPpaPVL2w7WgCgRY

bkchr
bkchr approved these changes Aug 26, 2022
View reviewed changes
utils/frame/try-runtime/cli/src/parse.rs Outdated
Ok(0) => Ok(StateVersion::V0),
Ok(1) => Ok(StateVersion::V1),
Ok(_) => Err("State version not supported."),
_ => Err("State version couldn't have been parsed."),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_ => Err("State version couldn't have been parsed."),
_ => Err("Invalid state version."),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reworded

@bkchr bkchr requested a review from kianenigma August 26, 2022 09:10
ggwpez
ggwpez approved these changes Aug 26, 2022
View reviewed changes
utils/frame/try-runtime/cli/src/parse.rs Outdated Show resolved Hide resolved
@ggwpez ggwpez 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. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Aug 26, 2022
@pmikolajczyk41
Copy link
Contributor Author

pmikolajczyk41 commented Aug 26, 2022 •
edited
Loading

also, would you @bkchr / @ggwpez / @kianenigma support either of these two ideas?

  1. easier runtime passing - currently, we have to provide whole chainspec (to every subcommand) containing the target runtime; then the whole genesis is built, only to be discarded after retrieving runtime code; I would propose passing path to a compiled wasm blob

  2. keeping continuous connection with the chain - currently, for each block we have to reconnect to the node:

2022-08-22 12:22:43.710  INFO                 main jsonrpsee_client_transport::ws: Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
2022-08-22 12:22:43.710 DEBUG                 main try-runtime::cli: new block event: 0x35143cc9d8afc7be8238cc4f989168bc8c9b237cddea67356e4648acbeeac774 => 65, extrinsics: 1

When the blocktime is long, it makes sense obviously. However, for a short period (like we have in Aleph and possibly in Polkadot) it would probably be better to keep a single connection for the whole interaction. However, I'm not so sure whether this is feasible / worth doing.

of course I'm willing to take up any related tasks

kianenigma
kianenigma approved these changes Aug 27, 2022
View reviewed changes
Copy link
Contributor

@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 think both of your ideas makes sense, but they can and should be follow-up PRs.

@kianenigma
Copy link
Contributor

@pmikolajczyk41 polkadot address for tip?

@kianenigma
Copy link
Contributor

bot merge

@paritytech-processbot paritytech-processbot bot merged commit ea4f532 into paritytech:master Aug 27, 2022
@pmikolajczyk41
Copy link
Contributor Author

@pmikolajczyk41 polkadot address for tip?

@kianenigma I have added to the PR description, thank you!

@pmikolajczyk41 pmikolajczyk41 deleted the piomiko/try-runtime/set-state-version branch August 27, 2022 06:11
@kianenigma
Copy link
Contributor

/tip medium

@substrate-tip-bot
Copy link

@kianenigma A medium tip was successfully submitted for pmikolajczyk41 (15fdtPm7jNN9VZk5LokKcmBVfmYZkCXdjPpaPVL2w7WgCgRY on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

@pmikolajczyk41 pmikolajczyk41 mentioned this pull request Sep 2, 2022
Merged
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@pmikolajczyk41 @ark0f
try-runtime - support all state versions (paritytech#12089)
8680f71 
* Add argument

* Apply

* More verbose parsing

* fmt

* fmt again

* Invalid state version

* reuse parsing

* type mismatch
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ggwpez ggwpez ggwpez approved these changes

@bkchr bkchr bkchr approved these changes

@kianenigma kianenigma kianenigma approved these changes

Assignees
No one assigned
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. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pmikolajczyk41 @kianenigma @bkchr @ggwpez