Additional state check. (#2915)

* check resulting state root on import.

* get root from last method.

* Fix test runtime (was changing state after root calculation).

* Do reset new authorities (just take before root calculation).

* bump impl runtime version.

* Update core/sr-api-macros/tests/trybuild.rs

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/core/client/src/call_executor.rs

@@ -110,7 +110,7 @@ where
 		manager: ExecutionManager<F>,
 		native_call: Option<NC>,
 		side_effects_handler: Option<&mut O>,
-	) -> Result<(NativeOrEncoded<R>, S::Transaction, Option<MemoryDB<H>>), error::Error>;
+	) -> Result<(NativeOrEncoded<R>, (S::Transaction, H::Out), Option<MemoryDB<H>>), error::Error>;
 
 	/// Execute a call to a contract on top of given state, gathering execution proof.
 	///
@@ -314,7 +314,11 @@ where
 		manager: ExecutionManager<F>,
 		native_call: Option<NC>,
 		side_effects_handler: Option<&mut O>,
-	) -> error::Result<(NativeOrEncoded<R>, S::Transaction, Option<MemoryDB<Blake2Hasher>>)> {
+	) -> error::Result<(
+		NativeOrEncoded<R>,
+		(S::Transaction, <Blake2Hasher as Hasher>::Out),
+		Option<MemoryDB<Blake2Hasher>>,
+	)> {
 		state_machine::new(
 			state,
 			self.backend.changes_trie_storage(),

substrate/core/client/src/client.rs

@@ -1027,7 +1027,11 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 
 				let (top, children) = overlay.into_committed();
 				let children = children.map(|(sk, it)| (sk, it.collect())).collect();
-				Ok((Some(storage_update), Some(changes_update), Some((top.collect(), children))))
+				if import_headers.post().state_root() != &storage_update.1 {
+					return Err(error::Error::InvalidStateRoot);
+				}
+
+				Ok((Some(storage_update.0), Some(changes_update), Some((top.collect(), children))))
 			},
 			None => Ok((None, None, None))
 		}

substrate/core/client/src/error.rs

@@ -94,6 +94,9 @@ pub enum Error {
 	/// Hash that is required for building CHT is missing.
 	#[display(fmt = "Failed to get hash of block for building CHT")]
 	MissingHashRequiredForCHT,
+	/// Invalid calculated state root on block import.
+	#[display(fmt = "Calculated state root does not match.")]
+	InvalidStateRoot,
 	/// A convenience variant for String
 	#[display(fmt = "{}", _0)]
 	Msg(String),

substrate/core/client/src/light/call_executor.rs

@@ -170,7 +170,11 @@ where
 		_m: ExecutionManager<FF>,
 		_native_call: Option<NC>,
 		_side_effects_handler: Option<&mut O>,
-	) -> ClientResult<(NativeOrEncoded<R>, S::Transaction, Option<MemoryDB<Blake2Hasher>>)> {
+	) -> ClientResult<(
+		NativeOrEncoded<R>,
+		(S::Transaction, <Blake2Hasher as Hasher>::Out),
+		Option<MemoryDB<Blake2Hasher>>,
+	)> {
 		Err(ClientError::NotAvailableOnLightClient.into())
 	}
 
@@ -343,7 +347,11 @@ impl<Block, B, Remote, Local> CallExecutor<Block, Blake2Hasher> for
 		_manager: ExecutionManager<FF>,
 		native_call: Option<NC>,
 		side_effects_handler: Option<&mut O>,
-	) -> ClientResult<(NativeOrEncoded<R>, S::Transaction, Option<MemoryDB<Blake2Hasher>>)> {
+	) -> ClientResult<(
+		NativeOrEncoded<R>,
+		(S::Transaction, <Blake2Hasher as Hasher>::Out),
+		Option<MemoryDB<Blake2Hasher>>,
+	)> {
 		// there's no actual way/need to specify native/wasm execution strategy on light node
 		// => we can safely ignore passed values
 

substrate/core/state-machine/src/ext.rs

@@ -115,7 +115,7 @@ where
 	}
 
 	/// Get the transaction necessary to update the backend.
-	pub fn transaction(mut self) -> (B::Transaction, Option<MemoryDB<H>>) {
+	pub fn transaction(mut self) -> ((B::Transaction, H::Out), Option<MemoryDB<H>>) {
 		let _ = self.storage_root();
 
 		let (storage_transaction, changes_trie_transaction) = (
@@ -126,7 +126,7 @@ where
 		);
 
 		(
-			storage_transaction.0,
+			storage_transaction,
 			changes_trie_transaction,
 		)
 	}

substrate/core/state-machine/src/lib.rs

@@ -502,7 +502,7 @@ impl<'a, H, N, B, T, O, Exec> StateMachine<'a, H, N, B, T, O, Exec> where
 	pub fn execute(
 		&mut self,
 		strategy: ExecutionStrategy,
-	) -> Result<(Vec<u8>, B::Transaction, Option<MemoryDB<H>>), Box<dyn Error>> {
+	) -> Result<(Vec<u8>, (B::Transaction, H::Out), Option<MemoryDB<H>>), Box<dyn Error>> {
 		// We are not giving a native call and thus we are sure that the result can never be a native
 		// value.
 		self.execute_using_consensus_failure_handler::<_, NeverNativeValue, fn() -> _>(
@@ -522,7 +522,12 @@ impl<'a, H, N, B, T, O, Exec> StateMachine<'a, H, N, B, T, O, Exec> where
 		compute_tx: bool,
 		use_native: bool,
 		native_call: Option<NC>,
-	) -> (CallResult<R, Exec::Error>, bool, Option<B::Transaction>, Option<MemoryDB<H>>) where
+	) -> (
+		CallResult<R, Exec::Error>,
+		bool,
+		Option<(B::Transaction, H::Out)>,
+		Option<MemoryDB<H>>,
+	) where
 		R: Decode + Encode + PartialEq,
 		NC: FnOnce() -> result::Result<R, &'static str> + UnwindSafe,
 	{
@@ -554,7 +559,7 @@ impl<'a, H, N, B, T, O, Exec> StateMachine<'a, H, N, B, T, O, Exec> where
 		mut native_call: Option<NC>,
 		orig_prospective: OverlayedChangeSet,
 		on_consensus_failure: Handler,
-	) -> (CallResult<R, Exec::Error>, Option<B::Transaction>, Option<MemoryDB<H>>) where
+	) -> (CallResult<R, Exec::Error>, Option<(B::Transaction, H::Out)>, Option<MemoryDB<H>>) where
 		R: Decode + Encode + PartialEq,
 		NC: FnOnce() -> result::Result<R, &'static str> + UnwindSafe,
 		Handler: FnOnce(
@@ -585,7 +590,7 @@ impl<'a, H, N, B, T, O, Exec> StateMachine<'a, H, N, B, T, O, Exec> where
 		compute_tx: bool,
 		mut native_call: Option<NC>,
 		orig_prospective: OverlayedChangeSet,
-	) -> (CallResult<R, Exec::Error>, Option<B::Transaction>, Option<MemoryDB<H>>) where
+	) -> (CallResult<R, Exec::Error>, Option<(B::Transaction, H::Out)>, Option<MemoryDB<H>>) where
 		R: Decode + Encode + PartialEq,
 		NC: FnOnce() -> result::Result<R, &'static str> + UnwindSafe,
 	{
@@ -613,7 +618,11 @@ impl<'a, H, N, B, T, O, Exec> StateMachine<'a, H, N, B, T, O, Exec> where
 		manager: ExecutionManager<Handler>,
 		compute_tx: bool,
 		mut native_call: Option<NC>,
-	) -> Result<(NativeOrEncoded<R>, Option<B::Transaction>, Option<MemoryDB<H>>), Box<dyn Error>> where
+	) -> Result<(
+		NativeOrEncoded<R>,
+		Option<(B::Transaction, H::Out)>,
+		Option<MemoryDB<H>>
+	), Box<dyn Error>> where
 		R: Decode + Encode + PartialEq,
 		NC: FnOnce() -> result::Result<R, &'static str> + UnwindSafe,
 		Handler: FnOnce(

substrate/core/test-runtime/src/system.rs

@@ -110,6 +110,8 @@ fn execute_block_with_state_root_handler(
 		storage::unhashed::kill(well_known_keys::EXTRINSIC_INDEX);
 	});
 
+	let o_new_authorities = <NewAuthorities>::take();
+
 	if let Mode::Overwrite = mode {
 		header.state_root = storage_root().into();
 	} else {
@@ -124,7 +126,7 @@ fn execute_block_with_state_root_handler(
 	if let Some(storage_changes_root) = storage_changes_root(header.parent_hash.into()) {
 		digest.push(generic::DigestItem::ChangesTrieRoot(storage_changes_root.into()));
 	}
-	if let Some(new_authorities) = <NewAuthorities>::take() {
+	if let Some(new_authorities) = o_new_authorities {
 		digest.push(generic::DigestItem::Consensus(*b"aura", new_authorities.encode()));
 		digest.push(generic::DigestItem::Consensus(*b"babe", new_authorities.encode()));
 	}
@@ -203,6 +205,7 @@ pub fn finalize_block() -> Header {
 	let parent_hash = <ParentHash>::take();
 	let mut digest = <StorageDigest>::take().expect("StorageDigest is set by `initialize_block`");
 
+	let o_new_authorities = <NewAuthorities>::take();
 	// This MUST come after all changes to storage are done.  Otherwise we will fail the
 	// “Storage root does not match that calculated” assertion.
 	let storage_root = BlakeTwo256::storage_root();
@@ -211,7 +214,8 @@ pub fn finalize_block() -> Header {
 	if let Some(storage_changes_root) = storage_changes_root {
 		digest.push(generic::DigestItem::ChangesTrieRoot(storage_changes_root));
 	}
-	if let Some(new_authorities) = <NewAuthorities>::take() {
+
+	if let Some(new_authorities) = o_new_authorities {
 		digest.push(generic::DigestItem::Consensus(*b"aura", new_authorities.encode()));
 		digest.push(generic::DigestItem::Consensus(*b"babe", new_authorities.encode()));
 	}