Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move AllPalletsWithSystem::decode_entire_state to own runtime API #2263

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Move AllPalletsWithSystem::decode_entire_state to own runtime API #2263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions cumulus/parachain-template/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,13 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}

}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions cumulus/parachains/runtimes/assets/asset-hub-kusama/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1188,6 +1188,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions cumulus/parachains/runtimes/assets/asset-hub-polkadot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1303,6 +1303,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,6 +1285,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions cumulus/parachains/runtimes/bridge-hubs/bridge-hub-kusama/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions cumulus/parachains/runtimes/bridge-hubs/bridge-hub-polkadot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions cumulus/parachains/runtimes/collectives/collectives-polkadot/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,6 +927,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions cumulus/parachains/runtimes/testing/penpal/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions polkadot/runtime/rococo/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2048,6 +2048,12 @@ sp_api::impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions polkadot/runtime/westend/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2130,6 +2130,12 @@ sp_api::impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
6 changes: 6 additions & 0 deletions substrate/bin/node-template/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).expect("execute-block failed")
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().unwrap()
}
}

impl sp_genesis_builder::GenesisBuilder<Block> for Runtime {
Expand Down
6 changes: 6 additions & 0 deletions substrate/bin/node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2747,6 +2747,12 @@ impl_runtime_apis! {
// have a backtrace here.
Executive::try_execute_block(block, state_root_check, signature_check, select).unwrap()
}

fn decode_entire_state() {
// NOTE: intentional unwrap: we don't want to propagate the error backwards, and want to
// have a backtrace here.
Executive::try_decode_entire_state().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 not just panic inside try_decode_entire_state instead of copying this comment everywhere? 🙈

}
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
16 changes: 6 additions & 10 deletions substrate/frame/executive/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,10 +322,6 @@ where
log::error!(target: LOG_TARGET, "failure: {:?}", e);
e
})?;
if select.any() {
let res = AllPalletsWithSystem::try_decode_entire_state();
Self::log_decode_result(res)?;
}
drop(_guard);

// do some of the checks that would normally happen in `final_checks`, but perhaps skip
Expand Down Expand Up @@ -362,6 +358,12 @@ where
Ok(frame_system::Pallet::<System>::block_weight().total())
}

pub fn try_decode_entire_state() -> Result<(), TryRuntimeError> {
let _ = frame_support::StorageNoopGuard::default();
let res = AllPalletsWithSystem::try_decode_entire_state();
Self::log_decode_result(res)
}

/// Execute all `OnRuntimeUpgrade` of this runtime.
///
/// The `checks` param determines whether to execute `pre/post_upgrade` and `try_state` hooks.
Expand All @@ -375,12 +377,6 @@ where
// Nothing should modify the state after the migrations ran:
let _guard = StorageNoopGuard::default();

// The state must be decodable:
if checks.any() {
let res = AllPalletsWithSystem::try_decode_entire_state();
Self::log_decode_result(res)?;
}
Comment on lines -379 to -382
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have the checks parameter? I mean we could also just add a new variant to check? So no new runtime api function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me answer this question in 2 parts.

  1. Suggest how we could add it to checks parameter, but why I don't think this is the best solution
  2. Suggest an alternative

1. How we could add it to checks

UpgradeCheckSelect is currently an enum:

pub enum UpgradeCheckSelect {
	None,
	All,
	PreAndPost,
	TryState,
}

To keep the enum, we'd need to add a new variant for every combination of checks the user may want to run. Which is obviously not scalable or a good solution.

Instead, I think we'd want to change it to a struct something like this:

struct UpgradeCheckSelect {
	pre_and_post: bool,
	try_state: Select,
	decode_entire_state: bool	
}

However, I don't think this is optimal for 2 reasons:

  1. It's a breaking change every time we want to add a new try-runtime task
  2. It's inflexible in the sense that we can only run checks in combination with an on_runtime_upgrade check or an execute_block check. What if we just want to decode entire state? or run try-state checks?
  3. We need to clutter the configuration to both try_on_runtime_upgrade and try_execute_block with the check options

2. Proposed alternative

  1. Add a new enum describing all the atomic try-runtime related tasks, something like
pub enum TryRuntimeTask {
	OnRuntimeUpgrade(pre_and_post: bool),
	ExecuteBlock(block: Block, state_root_check: bool, signature_check: bool),
	TryState(TryStateSelect),
	TryDecodeEntireState
}

  1. Remove all try-state and try-decode-entire-state logic from try_on_runtime_upgrade and try_execute_block entirely, instead create dedicated methods for running those checks try_state and try_decode_entire_state.

  2. Create a new Executive method and runtime API that accepts a vec of TryRuntimeTasks, and executes them in order

// runtime api
fn execute_try_runtime_tasks(tasks: Vec<TryRuntimeTask>) -> (Weight, Weight) {
	Executive::execute_try_runtime_tasks(tasks)
}

// executive method
pub fn execute_try_runtime_tasks(Vec<TryRuntimeTasks>) -> (Weight, Weight) {
	let agg_weight = Weight::zero();
	for task in tasks {
		match task {
			TryRuntimeTask::OnRuntimeUpgrade(pre_and_post) => {
				let weight = Self::try_on_runtime_upgrade(pre_and_post);
				agg_weight.saturating_acc(weight);
			}
			TryRuntimeTask::ExecuteBlock(block, state_root_check, signature_check) => {
				let weight = Self::try_execute_block(block, state_root_check, signature_check, select).unwrap();
				agg_weight.saturating_acc(weight);
			}
			TryRuntimeTask::TryState(try_state_select) {
				Self::try_state(try_state_select);
			}
			TryRuntimeTask::TryDecodeEntireState {
				Self::try_decode_entire_state();
			}
		}
	}
	(weight, BlockWeights::get().max_block)
}

This allows try_on_runtime_upgrade and try_execute_block to not be concerned about what checks to run or when (before or after) to run them or how (configuration) to run them.

The developer instead, with ultimate flexibility, just specifies a Vec like vec![OnRuntimeUpgrade(OnRuntimeUpgradeConfig), TryState(TryStateConfig), TryDecodeEntireState] describing what they want to run in what order.

Once this is implemented, we can add a deprecation notice to the current try-runtime runtime APIs and eventually remove them.

Copy link
Member

Choose a reason for hiding this comment

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

Okay ty for the writeup.

It's a breaking change every time we want to add a new try-runtime task

Your second proposal has the same issues ;) I don't think in general that this is such a problem, because these are development apis that don't need to stay stable or we need to support them over X years.

It's inflexible in the sense that we can only run checks in combination with an on_runtime_upgrade check or an execute_block check. What if we just want to decode entire state? or run try-state checks?

Not sure why it also requires the on_runtime_upgrade check. However, it will always require that we run the runtimes upgrades. You always need to run the upgrades as the new code you are running could be incompatible to the old runtime that created the state. This actually shows a "flaw" in your current pr here, because you don't run the upgrades before.

Copy link
Contributor Author

@liamaharon liamaharon Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Your second proposal has the same issues ;) I don't think in general that this is such a problem, because these are development apis that don't need to stay stable or we need to support them over X years.

If it's an enum then I think we can add new variants without it being a breaking change to the caller (e.g. try-runtime-cli)? Since try-runtime-cli will just construct the Vec with enums it knows about.

You're right though not a huge deal, and we likely won't change it frequently.

Do you feel I should proceed with the struct approach (described in 1.) then?

Copy link
Member

Choose a reason for hiding this comment

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

If it's an enum then I think we can add new variants without it being a breaking change to the caller (e.g. try-runtime-cli)? Since try-runtime-cli will just construct the Vec with enums it knows about.

Yeah that is a good point.

Maybe we could also just introduce a bitflag, assuming we don't need to pass some data. However, if we need to pass some data, I would probably use your enum approach given your reasoning above.


// Check all storage invariants:
if checks.try_state() {
AllPalletsWithSystem::try_state(
Expand Down
5 changes: 5 additions & 0 deletions substrate/frame/try-runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,10 @@ sp_api::decl_runtime_apis! {
signature_check: bool,
try_state: TryStateSelect,
) -> Weight;

/// Decode the state of all pallets in the runtime.
///
/// Panics and logs info to assist with debugging if any pallets are not decodable.
fn decode_entire_state();
}
}