This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
CI friendly fast mode for try state checks #13382
Draft
Ank4n
wants to merge
18
commits into
master
Choose a base branch
from
ankan/try-state-fast
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
7be05b8
ignore slow running tests in CI
Ank4n 63c1f43
choose type of test from cli
Ank4n 17a9911
add fast option to try state checks
Ank4n 2e5b962
remove unused
Ank4n 2af73ee
fmt
Ank4n 25843d9
feature gate import
Ank4n adefb2d
fmt
Ank4n 5be0f9e
Merge branch 'master' into ankan/try-state-fast
Ank4n 83103eb
fix try state tests
Ank4n 858a347
use full paths for tryStateSelect
Ank4n 358aae9
more doc for fast mode
Ank4n 66226e5
documenting params for try_state check hook
Ank4n f23d4d2
fmt
Ank4n ab51f85
choose which checks to run on upgrade
Ank4n a5249f4
fix check
Ank4n dc957a1
add example for fast all check
Ank4n 548862d
fix doc
Ank4n f1fad6b
Merge remote-tracking branch 'origin/master' into ankan/try-state-fast
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -550,7 +550,7 @@ impl ExtBuilder { | |
let mut ext = self.build(); | ||
ext.execute_with(test); | ||
ext.execute_with(|| { | ||
Staking::do_try_state(System::block_number()).unwrap(); | ||
Staking::do_try_state(System::block_number(), false).unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe it would be more clear for the reader to pass one of [ |
||
}); | ||
} | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -17,6 +17,8 @@ | |||||
|
||||||
//! Traits for hooking tasks to events in a blockchain's lifecycle. | ||||||
|
||||||
#[cfg(feature = "try-runtime")] | ||||||
use super::try_runtime::Select as TryStateSelect; | ||||||
use crate::weights::Weight; | ||||||
use impl_trait_for_tuples::impl_for_tuples; | ||||||
use sp_runtime::traits::AtLeast32BitUnsigned; | ||||||
|
@@ -306,9 +308,12 @@ pub trait Hooks<BlockNumber> { | |||||
/// It should focus on certain checks to ensure that the state is sensible. This is never | ||||||
/// executed in a consensus code-path, therefore it can consume as much weight as it needs. | ||||||
/// | ||||||
/// Takes the block number and `TryStateSelect`as a parameter. The `TryStateSelect` is used to | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
/// select which state tests to execute. | ||||||
/// | ||||||
/// This hook should not alter any storage. | ||||||
#[cfg(feature = "try-runtime")] | ||||||
fn try_state(_n: BlockNumber) -> Result<(), TryRuntimeError> { | ||||||
fn try_state(_n: BlockNumber, _s: TryStateSelect) -> Result<(), TryRuntimeError> { | ||||||
Ok(()) | ||||||
} | ||||||
|
||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,8 +34,10 @@ pub struct OnRuntimeUpgradeCmd { | |
/// | ||
/// - `none`: Perform no checks (default when the arg is not present). | ||
/// - `all`: Perform all checks (default when the arg is present). | ||
/// - `fast-all`: Perform fast running state, pre- and post-upgrade checks. | ||
/// - `pre-and-post`: Perform pre- and post-upgrade checks. | ||
/// - `try-state`: Perform the try-state checks. | ||
/// - `fast-try-state`: Perform fast running state checks. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May be a better approach would be to add another option to the |
||
/// | ||
/// Performing any checks will potentially invalidate the measured PoV/Weight. | ||
// NOTE: The clap attributes make it backwards compatible with the previous `--checks` flag. | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is unfortunately not correct.
You are right to pass in something into the hook that helps it understand if it is fast or not, but
TryStateSelect
is not the right type here.TryStateSelect
is meant to identify which pallets to execute, not how much time they should each consume. It is only interpreted by theExecutive
and should not be exposed to the end user here at all.What we instead want is a
TryStateSpeed
which you can for now assume tobool
orenum TryStateSpeed { Slow, Mid, Fast }
.Then, you are capable of selecting which pallets to run, and at what speed.
As it stands now, I see this flaw as welll:
A pallet could possibly see
RoundRobin(7)
as its try-state select. How should it interpret this? Answer: it cannot, because it is not the right audience for it.