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

fix try-runtime fast-forward when run from the substrate cli and add test #13888

Closed
wants to merge 6 commits into from

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented Apr 12, 2023

Closes: #13831

Description

fast-forward requires a custom BlockBuildingInfoProvider with the blocktime set (see discussion here #12896).

The custom provider had been set for the node-template cli, but not the substrate cli.

  • Use a custom BlockBuildingInfoProvider with the substrate cli
  • Add clearly labelled block building info providers for node-template, substrate, polkadot, kusama
  • Allow the block building info provider to be configured by cli args
  • Test that follow-chain exits with a success status code, and perform a basic check on the output

@liamaharon liamaharon 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. T0-node This PR/Issue is related to the topic “node”. labels Apr 12, 2023
@liamaharon liamaharon changed the title fix try-runtime fast-forward when run from the substrate cli fix try-runtime fast-forward when run from the substrate cli and add test Apr 12, 2023
Comment on lines -64 to 69
Command::new(cargo_bin("substrate"))
Command::new(cargo_bin("node-template"))
.stdout(process::Stdio::piped())
.stderr(process::Stdio::piped())
.args(&["--dev", "--tmp", "--ws-port=45789", "--no-hardware-benchmarks"])
.args(&["--dev", "--ws-port=45789"])
.spawn()
.unwrap()
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to make things simpler.

--dev makes --tmp redundant, and node-template doesn't run any hardware benchmarks by default.

Let me know if I'm missing something though.

utils/frame/try-runtime/cli/tests/fast_forward.rs Outdated Show resolved Hide resolved
utils/frame/try-runtime/cli/tests/fast_forward.rs Outdated Show resolved Hide resolved
@liamaharon liamaharon requested a review from bkchr April 13, 2023 07:09
@liamaharon
Copy link
Contributor Author

liamaharon commented Apr 13, 2023

I'm converting this PR back to a draft with the intention of getting fast-forward configurable to work with node-template, substrate, polkadot and kusama, and clear documentation for other chains to be supported.

@liamaharon liamaharon marked this pull request as draft April 13, 2023 08:42
@liamaharon liamaharon added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Apr 13, 2023
@@ -32,6 +32,9 @@ use sp_keyring::Sr25519Keyring;

use std::sync::Arc;

#[cfg(feature = "try-runtime")]
use try_runtime_cli::block_building_info::timestamp_with_aura_info;
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally always prefer using the full path where needed, or doing the import more locally, rather than feature gated imports, but up to you.

@stale
Copy link

stale bot commented May 15, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label May 15, 2023
@liamaharon
Copy link
Contributor Author

Closing this for now, need to solve issue with per-chain inherents first.

@liamaharon liamaharon closed this May 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. A3-stale 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. T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

try-runtime fast-forward panics
3 participants