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

try-runtime-cli: Add execute-block subcommand #9077

Merged
20 commits merged into from
Jun 22, 2021
Merged

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Jun 11, 2021

This PR adds the execute-block subcomand to the try-runtime command. execute-block allows the user to specify a block to execute against the runtime state of its parent block.

Update 1: this removes the --header-at option, and now conditionally has --block-at as a shared option; thus --block-at is no longer an options for live.

Update 2: --url is now a shared param and is no longer used as arg to the state subcommand.

NOTE: for anyone trying out this tool you may need to bump the --max-rpc-payload on the archive node used for live queries in order to load in some bigger storage items

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jun 11, 2021
@emostov emostov requested a review from coriolinus June 11, 2021 00:54
@emostov emostov added 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. labels Jun 11, 2021
@emostov emostov removed the request for review from coriolinus June 11, 2021 04:05
@emostov emostov marked this pull request as draft June 11, 2021 04:06
@emostov emostov marked this pull request as ready for review June 11, 2021 23:46
@emostov emostov requested review from coriolinus and bkchr June 11, 2021 23:46
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Looks generally good, with some nits. Ping me again when CI is green?

utils/frame/remote-externalities/src/rpc_api.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 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
@emostov
Copy link
Contributor Author

emostov commented Jun 14, 2021

Noting that will likely want to always overwrite code for offchain-worker, but I need to double check this cc @kianenigma

@emostov emostov requested a review from coriolinus June 16, 2021 23:09
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Listed a few minor potential improvements above, but they all can be considered optional; none of them are real blockers.

builder.inject_key_value(&[(code_key, code)]).build().await?
} else {
builder
.inject_hashed_key(well_known_keys::CODE)
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 is fine, but i don't 100% understand the case when you would not fetch state from the all the runtime modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kianenigma, perhaps obvious I also think its worth noting that this will have no affect on the builder if we are using State::Snap

Copy link
Contributor

Choose a reason for hiding this comment

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

this is fine, but i don't 100% understand the case when you would not fetch state from the all the runtime modules?

For example of the offchain worker, you are only interested in executing the OCW of one pallet. Albeit, you must be sure that the OCW of all other pallets won't panic or sth if they don't have any data set in their pallet.

Copy link
Contributor Author

@emostov emostov Jun 22, 2021

Choose a reason for hiding this comment

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

@kianenigma but does execute block run the offchain worker? my point here is that i don't see why you would run execute_block without downloading the whole state because otherwise you get a storage root mismatch

however maybe some edgecase i don't know about

Copy link
Contributor Author

@emostov emostov left a comment

Choose a reason for hiding this comment

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

@kianenigma your changes look good to me

@emostov emostov added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Jun 22, 2021
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.

LGMT

@emostov
Copy link
Contributor Author

emostov commented Jun 22, 2021

bot merge

@ghost
Copy link

ghost commented Jun 22, 2021

Trying merge.

@ghost ghost merged commit 6242d37 into master Jun 22, 2021
@ghost ghost deleted the zeke-try-runtime-exec-block branch June 22, 2021 20:36
athei pushed a commit that referenced this pull request Jun 25, 2021
* Refactor remote_externalities::rpc_api

* try-runtime-cli: Adde `execute-block` subcommand

* Trivial

* Address some comments

* Use required_if & remove header-at usage

* Improve doc

* Update comment

* small tweaks

* add overwrite-code to shared params

* Update utils/frame/try-runtime/cli/src/lib.rs

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

* make url a shared param

* add helper for block_at (#9153)

* add helper for block_at

* remove redundant bound

* doc for fn block_at

* Update error message

Co-authored-by: kianenigma <kian@parity.io>
Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
kianenigma added a commit that referenced this pull request Jul 5, 2021
* Refactor remote_externalities::rpc_api

* try-runtime-cli: Adde `execute-block` subcommand

* Trivial

* Address some comments

* Use required_if & remove header-at usage

* Improve doc

* Update comment

* small tweaks

* add overwrite-code to shared params

* Update utils/frame/try-runtime/cli/src/lib.rs

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

* make url a shared param

* add helper for block_at (#9153)

* add helper for block_at

* remove redundant bound

* doc for fn block_at

* Update error message

Co-authored-by: kianenigma <kian@parity.io>
Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants