hack around signature verification

paritytech · Sep 21, 2021 · Sep 15, 2021 · Sep 15, 2021 · Sep 15, 2021 · Sep 15, 2021
commit a53043646d1c6709b13daebe948e723973b230ee
diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs
@@ -1587,8 +1587,8 @@ impl_runtime_apis! {
 			(weight, RuntimeBlockWeights::get().max_block)
 		}
 
-		fn execute_block_no_state_root_check(block: Block) -> Weight {
-			Executive::execute_block_no_state_root_check(block)
+		fn execute_block_no_state_root_and_signature_check(block: Block) -> Weight {
+			Executive::execute_block_no_state_root_and_signature_check(block)
 		}
 	}
 

diff --git a/frame/executive/Cargo.toml b/frame/executive/Cargo.toml
@@ -47,4 +47,4 @@ std = [
 	"sp-tracing/std",
 	"sp-std/std",
 ]
-try-runtime = ["frame-support/try-runtime"]
+try-runtime = ["frame-support/try-runtime", "sp-runtime/try-runtime"]
diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs
@@ -224,53 +224,56 @@ where
 	///
 	/// Should only be used for testing.
 	#[cfg(feature = "try-runtime")]
-	pub fn execute_block_no_state_root_check(block: Block) -> frame_support::weights::Weight {
+	pub fn execute_block_no_state_root_and_signature_check(
+		block: Block,
+	) -> frame_support::weights::Weight
+	where
+		Block::Extrinsic: sp_runtime::traits::CheckableNoCheck<Context>,
+		<Block::Extrinsic as sp_runtime::traits::CheckableNoCheck<Context>>::Checked:
+			Applyable<Call = CallOf<Block::Extrinsic, Context>> + GetDispatchInfo,
+	{
 		Self::initialize_block(block.header());
 		Self::initial_checks(&block);
 
 		let (header, extrinsics) = block.deconstruct();
 
-		Self::execute_extrinsics_with_book_keeping(extrinsics, *header.number());
+		Self::execute_extrinsics_with_book_keeping_no_signature_check(extrinsics, *header.number());
 		// don't call `final_checks`, but do finalize the block.
 		let _header = <frame_system::Pallet<System>>::finalize();
 
 		frame_system::Pallet::<System>::block_weight().total()
 	}
 
-	// #[cfg(feature = "try-runtime")]
-	// fn execute_extrinsics_with_book_keeping_no_signature_check(
-	// 	extrinsics: Vec<Block::Extrinsic>,
-	// 	block_number: NumberFor<Block>,
-	// ) {
-	// 	extrinsics.into_iter().for_each(|uxt| {
-	// 		let encoded = uxt.encode();
-	// 		let encoded_len = encoded.len();
-
-	// 		// skip signature verification
-	// 		let xt = Self::blind_check(uxt)?;
-
-	// 		// We don't need to make sure to `note_extrinsic` only after we know it's going to be
-	// 		// executed to prevent it from leaking in storage since at this point, it will either
-	// 		// execute or panic (and revert storage changes).
-	// 		<frame_system::Pallet<System>>::note_extrinsic(encoded);
-
-	// 		// AUDIT: Under no circumstances may this function panic from here onwards.
-
-	// 		// Decode parameters and dispatch
-	// 		let dispatch_info = xt.get_dispatch_info();
-	// 		let r = Applyable::apply::<UnsignedValidator>(xt, &dispatch_info, encoded_len)
-	// 			.map(|_| ())
-	// 			.map_err(|e| e.error)
-	// 			.unwrap();
-
-	// 		<frame_system::Pallet<System>>::note_applied_extrinsic(&r, dispatch_info);
-	// 	});
-
-	// 	// post-extrinsics book-keeping
-	// 	<frame_system::Pallet<System>>::note_finished_extrinsics();
+	#[cfg(feature = "try-runtime")]
+	fn execute_extrinsics_with_book_keeping_no_signature_check(
+		extrinsics: Vec<Block::Extrinsic>,
+		block_number: NumberFor<Block>,
+	) where
+		Block::Extrinsic: sp_runtime::traits::CheckableNoCheck<Context>,
+		<Block::Extrinsic as sp_runtime::traits::CheckableNoCheck<Context>>::Checked:
+			Applyable<Call = CallOf<Block::Extrinsic, Context>> + GetDispatchInfo,
+	{
+		use sp_runtime::traits::CheckableNoCheck;
+
+		extrinsics.into_iter().for_each(|uxt| {
+			let encoded = uxt.encode();
+			let encoded_len = encoded.len();
+
+			// skip signature verification
+			let xt = uxt.check_i_know_i_should_not_be_using_this(&Default::default()).unwrap();
+			<frame_system::Pallet<System>>::note_extrinsic(encoded);
+
+			// dispatch
+			let dispatch_info = xt.get_dispatch_info();
+			let r = Applyable::apply::<UnsignedValidator>(xt, &dispatch_info, encoded_len).unwrap();
+
+			<frame_system::Pallet<System>>::note_applied_extrinsic(&r, dispatch_info);
+		});
 
-	// 	Self::idle_and_finalize_hook(block_number);
-	// }
+		// post-extrinsics book-keeping
+		<frame_system::Pallet<System>>::note_finished_extrinsics();
+		Self::idle_and_finalize_hook(block_number);
+	}
 
 	/// Execute all `OnRuntimeUpgrade` of this runtime, including the pre and post migration checks.
 	///

diff --git a/frame/try-runtime/src/lib.rs b/frame/try-runtime/src/lib.rs
@@ -38,6 +38,6 @@ sp_api::decl_runtime_apis! {
 		///
 		/// This is only sensible where the incoming block is from a different network, yet it has
 		/// the same block format as the runtime implementing this API.
-		fn execute_block_no_state_root_check(block: Block) -> Weight;
+		fn execute_block_no_state_root_and_signature_check(block: Block) -> Weight;
 	}
 }
diff --git a/primitives/runtime/Cargo.toml b/primitives/runtime/Cargo.toml
@@ -58,3 +58,4 @@ std = [
 	"hash256-std-hasher/std",
 	"either/use_std",
 ]
+try-runtime = []
diff --git a/primitives/runtime/src/generic/unchecked_extrinsic.rs b/primitives/runtime/src/generic/unchecked_extrinsic.rs
@@ -159,6 +159,34 @@ where
 	}
 }
 
+#[cfg(feature = "try-runtime")]
+impl<Address, AccountId, Call, Signature, Extra, Lookup> traits::CheckableNoCheck<Lookup>
+	for UncheckedExtrinsic<Address, Call, Signature, Extra>
+where
+	Address: Member + MaybeDisplay,
+	Call: Encode + Member,
+	Signature: Member + traits::Verify,
+	<Signature as traits::Verify>::Signer: IdentifyAccount<AccountId = AccountId>,
+	Extra: SignedExtension<AccountId = AccountId>,
+	AccountId: Member + MaybeDisplay,
+	Lookup: traits::Lookup<Source = Address, Target = AccountId>,
+{
+	type Checked = CheckedExtrinsic<AccountId, Call, Extra>;
+
+	fn check_i_know_i_should_not_be_using_this(
+		self,
+		lookup: &Lookup,
+	) -> Result<Self::Checked, TransactionValidityError> {
+		Ok(match self.signature {
+			Some((signed, _, extra)) => {
+				let signed = lookup.lookup(signed)?;
+				CheckedExtrinsic { signed: Some((signed, extra)), function: self.function }
+			},
+			None => CheckedExtrinsic { signed: None, function: self.function },
+		})
+	}
+}
+
 impl<Address, Call, Signature, Extra> ExtrinsicMetadata
 	for UncheckedExtrinsic<Address, Call, Signature, Extra>
 where

diff --git a/primitives/runtime/src/traits.rs b/primitives/runtime/src/traits.rs
@@ -779,6 +779,21 @@ pub trait Checkable<Context>: Sized {
 	fn check(self, c: &Context) -> Result<Self::Checked, TransactionValidityError>;
 }
 
+/// A variant if `Checkable` that avoids any checks.
+///
+/// Only useful for testing, without breaking the generic assumptions of the runtime.
+#[cfg(feature = "try-runtime")]
+pub trait CheckableNoCheck<Context>: Sized {
+	/// Returned if `check` succeeds.
+	type Checked;
+
+	/// Check self, given an instance of Context.
+	fn check_i_know_i_should_not_be_using_this(
+		self,
+		c: &Context,
+	) -> Result<Self::Checked, TransactionValidityError>;
+}
+
 /// A "checkable" piece of information, used by the standard Substrate Executive in order to
 /// check the validity of a piece of extrinsic information, usually by verifying the signature.
 /// Implement for pieces of information that don't require additional context in order to be

diff --git a/utils/frame/try-runtime/cli/src/commands/execute_block.rs b/utils/frame/try-runtime/cli/src/commands/execute_block.rs
@@ -33,7 +33,8 @@ pub struct ExecuteBlockCmd {
 	overwrite_wasm_code: bool,
 
 	/// If set, then the state root check is disabled by the virtue of calling into
-	/// `TryRuntime_execute_block_no_state_root_check` instead of `Core_execute_block`.
+	/// `TryRuntime_execute_block_no_state_root_and_signature_check` instead of
+	/// `Core_execute_block`.
 	#[structopt(long)]
 	no_state_root_check: bool,
 
@@ -155,7 +156,12 @@ where
 	let block = Block::new(header, extrinsics);
 
 	let expected_spec_name = local_spec_name::<Block, ExecDispatch>(&ext, &executor);
-	ensure_matching_spec_name::<Block>(block_uri.clone(), expected_spec_name).await;
+	ensure_matching_spec_name::<Block>(
+		block_uri.clone(),
+		expected_spec_name,
+		shared.no_spec_name_check,
+	)
+	.await;
 
 	let _ = state_machine_call::<Block, ExecDispatch>(
 		&ext,
@@ -164,7 +170,7 @@ where
 		if command.no_state_root_check {
 			"Core_execute_block"
 		} else {
-			"TryRuntime_execute_block_no_state_root_check"
+			"TryRuntime_execute_block_no_state_root_and_signature_check"
 		},
 		block.encode().as_ref(),
 		full_extensions(),

diff --git a/utils/frame/try-runtime/cli/src/commands/follow_chain.rs b/utils/frame/try-runtime/cli/src/commands/follow_chain.rs
@@ -31,6 +31,9 @@ use sp_core::H256;
 use sp_runtime::traits::{Block as BlockT, Header, NumberFor};
 use std::{fmt::Debug, str::FromStr};
 
+const SUB: &'static str = "chain_subscribeFinalizedHeads";
+const UN_SUB: &'static str = "chain_unsubscribeFinalizedHeads";
+
 /// Configurations of the [`Command::FollowChain`].
 #[derive(Debug, Clone, structopt::StructOpt)]
 pub struct FollowChainCmd {
@@ -59,25 +62,34 @@ where
 {
 	let mut maybe_state_ext = None;
 
-	let sub = "chain_subscribeFinalizedHeads";
-	let unsub = "chain_unsubscribeFinalizedHeads";
-
 	let client = WsClientBuilder::default()
 		.connection_timeout(std::time::Duration::new(20, 0))
 		.max_request_body_size(u32::MAX)
 		.build(&command.uri)
 		.await
 		.unwrap();
 
-	log::info!(target: LOG_TARGET, "subscribing to {:?} / {:?}", sub, unsub);
+	log::info!(target: LOG_TARGET, "subscribing to {:?} / {:?}", SUB, UN_SUB);
 	let mut subscription: Subscription<Block::Header> =
-		client.subscribe(&sub, JsonRpcParams::NoParams, &unsub).await.unwrap();
+		client.subscribe(&SUB, JsonRpcParams::NoParams, &UN_SUB).await.unwrap();
 
 	let (code_key, code) = extract_code(&config.chain_spec)?;
 	let executor = build_executor::<ExecDispatch>(&shared, &config);
 	let execution = shared.execution;
 
-	while let Some(header) = subscription.next().await.unwrap() {
+	loop {
+		let header = match subscription.next().await {
+			Ok(Some(header)) => header,
+			Ok(None) => {
+				log::warn!("subscription returned `None`. Probably decoding has failed.");
+				break
+			},
+			Err(why) => {
+				log::warn!("subscription returned error: {:?}.", why);
+				continue
+			},
+		};
+
 		let hash = header.hash();
 		let number = header.number();
 
@@ -109,7 +121,12 @@ where
 			);
 
 			let expected_spec_name = local_spec_name::<Block, ExecDispatch>(&new_ext, &executor);
-			ensure_matching_spec_name::<Block>(command.uri.clone(), expected_spec_name).await;
+			ensure_matching_spec_name::<Block>(
+				command.uri.clone(),
+				expected_spec_name,
+				shared.no_spec_name_check,
+			)
+			.await;
 
 			maybe_state_ext = Some(new_ext);
 		}
@@ -121,7 +138,7 @@ where
 			&state_ext,
 			&executor,
 			execution,
-			"TryRuntime_execute_block_no_state_root_check",
+			"TryRuntime_execute_block_no_state_root_and_signature_check",
 			block.encode().as_ref(),
 			full_extensions(),
 		)?;

diff --git a/utils/frame/try-runtime/cli/src/commands/offchain_worker.rs b/utils/frame/try-runtime/cli/src/commands/offchain_worker.rs
@@ -141,7 +141,8 @@ where
 	};
 
 	let expected_spec_name = local_spec_name::<Block, ExecDispatch>(&ext, &executor);
-	ensure_matching_spec_name::<Block>(header_uri, expected_spec_name).await;
+	ensure_matching_spec_name::<Block>(header_uri, expected_spec_name, shared.no_spec_name_check)
+		.await;
 
 	let _ = state_machine_call::<Block, ExecDispatch>(
 		&ext,

diff --git a/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs b/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs
@@ -59,7 +59,8 @@ where
 
 	if let Some(uri) = command.state.live_uri() {
 		let expected_spec_name = local_spec_name::<Block, ExecDispatch>(&ext, &executor);
-		ensure_matching_spec_name::<Block>(uri, expected_spec_name).await;
+		ensure_matching_spec_name::<Block>(uri, expected_spec_name, shared.no_spec_name_check)
+			.await;
 	}
 
 	let (_, encoded_result) = state_machine_call::<Block, ExecDispatch>(

diff --git a/utils/frame/try-runtime/cli/src/lib.rs b/utils/frame/try-runtime/cli/src/lib.rs
@@ -86,9 +86,19 @@
 //! ## Commands
 //!
 //! See [`Command`] for more info.
-//! 
+//!
+//! ## Spec name check
+//!
+//! A common pitfall is that you might be running some test on top of the state of chain `x`, with
+//! the runtime of chain `y`. To avoid this all commands do a spec-name check before executing
+//! anything by default. This will check the spec name of the remote node your are connected to,
+//! with the spec name of your local runtime and ensure that they match.
+//!
+//! Should you need to disable this on certain occasions, a top level flag of `--no-spec-name-check`
+//! can be used.
+//!
 //! ## Examples
-//! 
+//!
 //! TODO
 
 use parity_scale_codec::Decode;
@@ -156,7 +166,7 @@ pub enum Command {
 	///
 	/// This executes the same runtime api as normal block import, namely `Core_execute_block`. If
 	/// [`ExecuteBlockCmd::no_state_root_check`] is set, it uses a custom, try-runtime-only runtime
-	/// api called `TryRuntime_execute_block_no_state_root_check`.
+	/// api called `TryRuntime_execute_block_no_state_root_and_signature_check`.
 	ExecuteBlock(commands::execute_block::ExecuteBlockCmd),
 
 	/// Executes *the offchain worker hooks* of a given block against some state.
@@ -221,6 +231,10 @@ pub struct SharedParams {
 	/// [`sc_service::Configuration.default_heap_pages`].
 	#[structopt(long)]
 	pub heap_pages: Option<u64>,
+
+	/// When enabled, the spec name check will not panic, and instead only show a warning.
+	#[structopt(long)]
+	pub no_spec_name_check: bool,
 }
 
 /// Our `try-runtime` command.
@@ -408,6 +422,7 @@ where
 pub(crate) async fn ensure_matching_spec_name<Block: BlockT + serde::de::DeserializeOwned>(
 	uri: String,
 	expected_spec_name: String,
+	relaxed: bool,
 ) {
 	match remote_externalities::rpc_api::get_runtime_version::<Block, _>(uri.clone(), None)
 		.await
@@ -418,11 +433,16 @@ pub(crate) async fn ensure_matching_spec_name<Block: BlockT + serde::de::Deseria
 			log::debug!(target: LOG_TARGET, "found matching spec name: {:?}", spec);
 		},
 		Ok(spec) => {
-			panic!(
+			let msg = format!(
 				"version mismatch: remote spec name: '{}', expected (local chain spec, aka. `--chain`): '{}'",
 				spec,
-				expected_spec_name,
+				expected_spec_name
 			);
+			if relaxed {
+				log::warn!(target: LOG_TARGET, "{}", msg);
+			} else {
+				panic!("{}", msg);
+			}
 		},
 		Err(why) => {
 			log::error!(