Remove requirement on Hash = H256, make Proposer return StorageChanges and Proof (#3860)

* Extend `Proposer` to optionally generate a proof of the proposal

* Something

* Refactor sr-api to not depend on client anymore

* Fix benches

* Apply suggestions from code review

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* Apply suggestions from code review

* Introduce new `into_storage_changes` function

* Switch to runtime api for `execute_block` and don't require `H256`
anywhere in the code

* Put the `StorageChanges` into the `Proposal`

* Move the runtime api error to its own trait

* Adds `StorageTransactionCache` to the runtime api

This requires that we add `type NodeBlock = ` to the
`impl_runtime_apis!` macro to work around some bugs in rustc :(

* Remove `type NodeBlock` and switch to a "better" hack

* Start using the transaction cache from the runtime api

* Make it compile

* Move `InMemory` to its own file

* Make all tests work again

* Return block, storage_changes and proof from Blockbuilder::bake()

* Make sure that we use/set `storage_changes` when possible

* Add test

* Fix deadlock

* Remove accidentally added folders

* Introduce `RecordProof` as argument type to be more explicit

* Update client/src/client.rs

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* Update primitives/state-machine/src/ext.rs

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* Integrates review feedback

* Remove `unsafe` usage

* Update client/block-builder/src/lib.rs

Co-Authored-By: Benjamin Kampmann <ben@gnunicorn.org>

* Update client/src/call_executor.rs

* Bump versions

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
Co-authored-by: Benjamin Kampmann <ben.kampmann@googlemail.com>

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

@@ -24,12 +24,13 @@ use hash_db::Hasher;
 use codec::{Decode, Encode, Codec};
 use sp_core::{
 	storage::{well_known_keys, ChildInfo}, NativeOrEncoded, NeverNativeValue,
-	traits::CodeExecutor, hexdisplay::HexDisplay, hash::H256,
+	traits::CodeExecutor, hexdisplay::HexDisplay
 };
 use overlayed_changes::OverlayedChangeSet;
 use sp_externalities::Extensions;
 
 pub mod backend;
+mod in_memory_backend;
 mod changes_trie;
 mod error;
 mod ext;
@@ -56,8 +57,9 @@ pub use changes_trie::{
 	key_changes, key_changes_proof, key_changes_proof_check,
 	prune as prune_changes_tries,
 	oldest_non_pruned_trie as oldest_non_pruned_changes_trie,
+	BlockNumber as ChangesTrieBlockNumber,
 };
-pub use overlayed_changes::OverlayedChanges;
+pub use overlayed_changes::{OverlayedChanges, StorageChanges, StorageTransactionCache};
 pub use proving_backend::{
 	create_proof_check_backend, create_proof_check_backend_storage, merge_storage_proofs,
 	ProofRecorder, ProvingBackend, ProvingBackendRecorder, StorageProof,
@@ -65,6 +67,7 @@ pub use proving_backend::{
 pub use trie_backend_essence::{TrieBackendStorage, Storage};
 pub use trie_backend::TrieBackend;
 pub use error::{Error, ExecutionError};
+pub use in_memory_backend::InMemory as InMemoryBackend;
 
 type CallResult<R, E> = Result<NativeOrEncoded<R>, E>;
 
@@ -140,8 +143,10 @@ impl ExecutionStrategy {
 				warn!(
 					"Consensus error between wasm {:?} and native {:?}. Using wasm.",
 					wasm_result,
-					native_result
+					native_result,
 				);
+				warn!("   Native result {:?}", native_result);
+				warn!("   Wasm result {:?}", wasm_result);
 				wasm_result
 			}),
 		}
@@ -164,7 +169,12 @@ fn always_untrusted_wasm<E, R: Decode>() -> ExecutionManager<DefaultHandler<R, E
 }
 
 /// The substrate state machine.
-pub struct StateMachine<'a, B, H, N, T, Exec> where H: Hasher<Out=H256>, B: Backend<H> {
+pub struct StateMachine<'a, B, H, N, T, Exec>
+	where
+		H: Hasher,
+		B: Backend<H>,
+		N: ChangesTrieBlockNumber,
+{
 	backend: &'a B,
 	exec: &'a Exec,
 	method: &'a str,
@@ -173,10 +183,12 @@ pub struct StateMachine<'a, B, H, N, T, Exec> where H: Hasher<Out=H256>, B: Back
 	extensions: Extensions,
 	changes_trie_storage: Option<&'a T>,
 	_marker: PhantomData<(H, N)>,
+	storage_transaction_cache: Option<&'a mut StorageTransactionCache<B::Transaction, H, N>>,
 }
 
 impl<'a, B, H, N, T, Exec> StateMachine<'a, B, H, N, T, Exec> where
-	H: Hasher<Out=H256>,
+	H: Hasher,
+	H::Out: Ord + 'static + codec::Codec,
 	Exec: CodeExecutor,
 	B: Backend<H>,
 	T: ChangesTrieStorage<H, N>,
@@ -201,51 +213,61 @@ impl<'a, B, H, N, T, Exec> StateMachine<'a, B, H, N, T, Exec> where
 			overlay,
 			changes_trie_storage,
 			_marker: PhantomData,
+			storage_transaction_cache: None,
 		}
 	}
 
+	/// Use given `cache` as storage transaction cache.
+	///
+	/// The cache will be used to cache storage transactions that can be build while executing a
+	/// function in the runtime. For example, when calculating the storage root a transaction is
+	/// build that will be cached.
+	pub fn with_storage_transaction_cache(
+		mut self,
+		cache: Option<&'a mut StorageTransactionCache<B::Transaction, H, N>>,
+	) -> Self {
+		self.storage_transaction_cache = cache;
+		self
+	}
+
 	/// Execute a call using the given state backend, overlayed changes, and call executor.
-	/// Produces a state-backend-specific "transaction" which can be used to apply the changes
-	/// to the backing store, such as the disk.
 	///
 	/// On an error, no prospective changes are written to the overlay.
 	///
 	/// Note: changes to code will be in place if this call is made again. For running partial
 	/// blocks (e.g. a transaction at a time), ensure a different method is used.
-	pub fn execute(&mut self, strategy: ExecutionStrategy) -> Result<
-		(Vec<u8>, (B::Transaction, H::Out), Option<ChangesTrieTransaction<H, N>>),
-		Box<dyn Error>,
-	> {
+	///
+	/// Returns the SCALE encoded result of the executed function.
+	pub fn execute(&mut self, strategy: ExecutionStrategy) -> Result<Vec<u8>, 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() -> _>(
 			strategy.get_manager(),
-			true,
 			None,
-		)
-		.map(|(result, storage_tx, changes_tx)| (
-			result.into_encoded(),
-			storage_tx.expect("storage_tx is always computed when compute_tx is true; qed"),
-			changes_tx,
-		))
+		).map(NativeOrEncoded::into_encoded)
 	}
 
 	fn execute_aux<R, NC>(
 		&mut self,
-		compute_tx: bool,
 		use_native: bool,
 		native_call: Option<NC>,
 	) -> (
 		CallResult<R, Exec::Error>,
 		bool,
-		Option<(B::Transaction, H::Out)>,
-		Option<ChangesTrieTransaction<H, N>>,
 	) where
 		R: Decode + Encode + PartialEq,
 		NC: FnOnce() -> result::Result<R, String> + UnwindSafe,
 	{
+		let mut cache = StorageTransactionCache::default();
+
+		let cache = match self.storage_transaction_cache.as_mut() {
+			Some(cache) => cache,
+			None => &mut cache,
+		};
+
 		let mut ext = Ext::new(
 			self.overlay,
+			cache,
 			self.backend,
 			self.changes_trie_storage.clone(),
 			Some(&mut self.extensions),
@@ -268,13 +290,6 @@ impl<'a, B, H, N, T, Exec> StateMachine<'a, B, H, N, T, Exec> where
 			native_call,
 		);
 
-		let (storage_delta, changes_delta) = if compute_tx {
-			let (storage_delta, changes_delta) = ext.transaction();
-			(Some(storage_delta), changes_delta)
-		} else {
-			(None, None)
-		};
-
 		trace!(
 			target: "state-trace", "{:04x}: Return. Native={:?}, Result={:?}",
 			id,
@@ -282,37 +297,28 @@ impl<'a, B, H, N, T, Exec> StateMachine<'a, B, H, N, T, Exec> where
 			result,
 		);
 
-		(result, was_native, storage_delta, changes_delta)
+		(result, was_native)
 	}
 
 	fn execute_call_with_both_strategy<Handler, R, NC>(
 		&mut self,
-		compute_tx: bool,
 		mut native_call: Option<NC>,
 		orig_prospective: OverlayedChangeSet,
 		on_consensus_failure: Handler,
-	) -> (
-		CallResult<R, Exec::Error>,
-		Option<(B::Transaction, H::Out)>,
-		Option<ChangesTrieTransaction<H, N>>,
-	) where
-		R: Decode + Encode + PartialEq,
-		NC: FnOnce() -> result::Result<R, String> + UnwindSafe,
-		Handler: FnOnce(
-			CallResult<R, Exec::Error>,
-			CallResult<R, Exec::Error>,
-		) -> CallResult<R, Exec::Error>
+	) -> CallResult<R, Exec::Error>
+		where
+			R: Decode + Encode + PartialEq,
+			NC: FnOnce() -> result::Result<R, String> + UnwindSafe,
+			Handler: FnOnce(
+				CallResult<R, Exec::Error>,
+				CallResult<R, Exec::Error>,
+			) -> CallResult<R, Exec::Error>
 	{
-		let (result, was_native, storage_delta, changes_delta) = self.execute_aux(
-			compute_tx,
-			true,
-			native_call.take(),
-		);
+		let (result, was_native) = self.execute_aux(true, native_call.take());
 
 		if was_native {
 			self.overlay.prospective = orig_prospective.clone();
-			let (wasm_result, _, wasm_storage_delta, wasm_changes_delta) = self.execute_aux(
-				compute_tx,
+			let (wasm_result, _) = self.execute_aux(
 				false,
 				native_call,
 			);
@@ -321,71 +327,62 @@ impl<'a, B, H, N, T, Exec> StateMachine<'a, B, H, N, T, Exec> where
 				&& result.as_ref().ok() == wasm_result.as_ref().ok())
 				|| result.is_err() && wasm_result.is_err()
 			{
-				(result, storage_delta, changes_delta)
+				result
 			} else {
-				(on_consensus_failure(wasm_result, result), wasm_storage_delta, wasm_changes_delta)
+				on_consensus_failure(wasm_result, result)
 			}
 		} else {
-			(result, storage_delta, changes_delta)
+			result
 		}
 	}
 
 	fn execute_call_with_native_else_wasm_strategy<R, NC>(
 		&mut self,
-		compute_tx: bool,
 		mut native_call: Option<NC>,
 		orig_prospective: OverlayedChangeSet,
-	) -> (
-		CallResult<R, Exec::Error>,
-		Option<(B::Transaction, H::Out)>,
-		Option<ChangesTrieTransaction<H, N>>,
-	) where
-		R: Decode + Encode + PartialEq,
-		NC: FnOnce() -> result::Result<R, String> + UnwindSafe,
+	) -> CallResult<R, Exec::Error>
+		where
+			R: Decode + Encode + PartialEq,
+			NC: FnOnce() -> result::Result<R, String> + UnwindSafe,
 	{
-		let (result, was_native, storage_delta, changes_delta) = self.execute_aux(
-			compute_tx,
+		let (result, was_native) = self.execute_aux(
 			true,
 			native_call.take(),
 		);
 
 		if !was_native || result.is_ok() {
-			(result, storage_delta, changes_delta)
+			result
 		} else {
 			self.overlay.prospective = orig_prospective.clone();
-			let (wasm_result, _, wasm_storage_delta, wasm_changes_delta) = self.execute_aux(
-				compute_tx,
+			let (wasm_result, _) = self.execute_aux(
 				false,
 				native_call,
 			);
-			(wasm_result, wasm_storage_delta, wasm_changes_delta)
+			wasm_result
 		}
 	}
 
 	/// Execute a call using the given state backend, overlayed changes, and call executor.
-	/// Produces a state-backend-specific "transaction" which can be used to apply the changes
-	/// to the backing store, such as the disk.
 	///
 	/// On an error, no prospective changes are written to the overlay.
 	///
 	/// Note: changes to code will be in place if this call is made again. For running partial
 	/// blocks (e.g. a transaction at a time), ensure a different method is used.
+	///
+	/// Returns the result of the executed function either in native reprensentation `R` or
+	/// in SCALE encoded representation.
 	pub fn execute_using_consensus_failure_handler<Handler, R, NC>(
 		&mut self,
 		manager: ExecutionManager<Handler>,
-		compute_tx: bool,
 		mut native_call: Option<NC>,
-	) -> Result<(
-		NativeOrEncoded<R>,
-		Option<(B::Transaction, H::Out)>,
-		Option<ChangesTrieTransaction<H, N>>,
-	), Box<dyn Error>> where
-		R: Decode + Encode + PartialEq,
-		NC: FnOnce() -> result::Result<R, String> + UnwindSafe,
-		Handler: FnOnce(
-			CallResult<R, Exec::Error>,
-			CallResult<R, Exec::Error>,
-		) -> CallResult<R, Exec::Error>
+	) -> Result<NativeOrEncoded<R>, Box<dyn Error>>
+		where
+			R: Decode + Encode + PartialEq,
+			NC: FnOnce() -> result::Result<R, String> + UnwindSafe,
+			Handler: FnOnce(
+				CallResult<R, Exec::Error>,
+				CallResult<R, Exec::Error>,
+			) -> CallResult<R, Exec::Error>
 	{
 		// read changes trie configuration. The reason why we're doing it here instead of the
 		// `OverlayedChanges` constructor is that we need proofs for this read as a part of
@@ -405,10 +402,9 @@ impl<'a, B, H, N, T, Exec> StateMachine<'a, B, H, N, T, Exec> where
 		let result = {
 			let orig_prospective = self.overlay.prospective.clone();
 
-			let (result, storage_delta, changes_delta) = match manager {
+			match manager {
 				ExecutionManager::Both(on_consensus_failure) => {
 					self.execute_call_with_both_strategy(
-						compute_tx,
 						native_call.take(),
 						orig_prospective,
 						on_consensus_failure,
@@ -416,7 +412,6 @@ impl<'a, B, H, N, T, Exec> StateMachine<'a, B, H, N, T, Exec> where
 				},
 				ExecutionManager::NativeElseWasm => {
 					self.execute_call_with_native_else_wasm_strategy(
-						compute_tx,
 						native_call.take(),
 						orig_prospective,
 					)
@@ -426,15 +421,12 @@ impl<'a, B, H, N, T, Exec> StateMachine<'a, B, H, N, T, Exec> where
 						BackendTrustLevel::Trusted => None,
 						BackendTrustLevel::Untrusted => Some(sp_panic_handler::AbortGuard::never_abort()),
 					};
-					let res = self.execute_aux(compute_tx, false, native_call);
-					(res.0, res.2, res.3)
+					self.execute_aux(false, native_call).0
 				},
 				ExecutionManager::NativeWhenPossible => {
-					let res = self.execute_aux(compute_tx, true, native_call);
-					(res.0, res.2, res.3)
+					self.execute_aux(true, native_call).0
 				},
-			};
-			result.map(move |out| (out, storage_delta, changes_delta))
+			}
 		};
 
 		if result.is_ok() {
@@ -455,7 +447,8 @@ pub fn prove_execution<B, H, Exec>(
 ) -> Result<(Vec<u8>, StorageProof), Box<dyn Error>>
 where
 	B: Backend<H>,
-	H: Hasher<Out=H256>,
+	H: Hasher,
+	H::Out: Ord + 'static + codec::Codec,
 	Exec: CodeExecutor,
 {
 	let trie_backend = backend.as_trie_backend()
@@ -481,7 +474,8 @@ pub fn prove_execution_on_trie_backend<S, H, Exec>(
 ) -> Result<(Vec<u8>, StorageProof), Box<dyn Error>>
 where
 	S: trie_backend_essence::TrieBackendStorage<H>,
-	H: Hasher<Out=H256>,
+	H: Hasher,
+	H::Out: Ord + 'static + codec::Codec,
 	Exec: CodeExecutor,
 {
 	let proving_backend = proving_backend::ProvingBackend::new(trie_backend);
@@ -489,9 +483,8 @@ where
 		&proving_backend, None, overlay, exec, method, call_data, Extensions::default(),
 	);
 
-	let (result, _, _) = sm.execute_using_consensus_failure_handler::<_, NeverNativeValue, fn() -> _>(
+	let result = sm.execute_using_consensus_failure_handler::<_, NeverNativeValue, fn() -> _>(
 		always_wasm(),
-		false,
 		None,
 	)?;
 	let proof = sm.backend.extract_proof();
@@ -508,9 +501,9 @@ pub fn execution_proof_check<H, Exec>(
 	call_data: &[u8],
 ) -> Result<Vec<u8>, Box<dyn Error>>
 where
-	H: Hasher<Out=H256>,
+	H: Hasher,
 	Exec: CodeExecutor,
-	H::Out: Ord + 'static,
+	H::Out: Ord + 'static + codec::Codec,
 {
 	let trie_backend = create_proof_check_backend::<H>(root.into(), proof)?;
 	execution_proof_check_on_trie_backend(&trie_backend, overlay, exec, method, call_data)
@@ -525,7 +518,8 @@ pub fn execution_proof_check_on_trie_backend<H, Exec>(
 	call_data: &[u8],
 ) -> Result<Vec<u8>, Box<dyn Error>>
 where
-	H: Hasher<Out=H256>,
+	H: Hasher,
+	H::Out: Ord + 'static + codec::Codec,
 	Exec: CodeExecutor,
 {
 	let mut sm = StateMachine::<_, H, _, InMemoryChangesTrieStorage<H, u64>, Exec>::new(
@@ -534,9 +528,8 @@ where
 
 	sm.execute_using_consensus_failure_handler::<_, NeverNativeValue, fn() -> _>(
 		always_untrusted_wasm(),
-		false,
 		None,
-	).map(|(result, _, _)| result.into_encoded())
+	).map(NativeOrEncoded::into_encoded)
 }
 
 /// Generate storage read proof.
@@ -546,8 +539,8 @@ pub fn prove_read<B, H, I>(
 ) -> Result<StorageProof, Box<dyn Error>>
 where
 	B: Backend<H>,
-	H: Hasher<Out=H256>,
-	H::Out: Ord,
+	H: Hasher,
+	H::Out: Ord + Codec,
 	I: IntoIterator,
 	I::Item: AsRef<[u8]>,
 {
@@ -739,7 +732,6 @@ mod tests {
 	use codec::Encode;
 	use overlayed_changes::OverlayedValue;
 	use super::*;
-	use super::backend::InMemory;
 	use super::ext::Ext;
 	use super::changes_trie::{
 		InMemoryStorage as InMemoryChangesTrieStorage,
@@ -825,7 +817,7 @@ mod tests {
 		);
 
 		assert_eq!(
-			state_machine.execute(ExecutionStrategy::NativeWhenPossible).unwrap().0,
+			state_machine.execute(ExecutionStrategy::NativeWhenPossible).unwrap(),
 			vec![66],
 		);
 	}
@@ -852,7 +844,7 @@ mod tests {
 			Default::default(),
 		);
 
-		assert_eq!(state_machine.execute(ExecutionStrategy::NativeElseWasm).unwrap().0, vec![66]);
+		assert_eq!(state_machine.execute(ExecutionStrategy::NativeElseWasm).unwrap(), vec![66]);
 	}
 
 	#[test]
@@ -883,7 +875,6 @@ mod tests {
 					consensus_failed = true;
 					we
 				}),
-				true,
 				None,
 			).is_err()
 		);
@@ -933,7 +924,7 @@ mod tests {
 			b"abc".to_vec() => b"2".to_vec(),
 			b"bbb".to_vec() => b"3".to_vec()
 		];
-		let mut state = InMemory::<Blake2Hasher>::from(initial);
+		let mut state = InMemoryBackend::<Blake2Hasher>::from(initial);
 		let backend = state.as_trie_backend().unwrap();
 		let mut overlay = OverlayedChanges {
 			committed: map![
@@ -949,8 +940,10 @@ mod tests {
 
 		{
 			let changes_trie_storage = InMemoryChangesTrieStorage::<Blake2Hasher, u64>::new();
+			let mut cache = StorageTransactionCache::default();
 			let mut ext = Ext::new(
 				&mut overlay,
+				&mut cache,
 				backend,
 				Some(&changes_trie_storage),
 				None,
@@ -975,12 +968,14 @@ mod tests {
 
 	#[test]
 	fn set_child_storage_works() {
-		let mut state = InMemory::<Blake2Hasher>::default();
+		let mut state = InMemoryBackend::<Blake2Hasher>::default();
 		let backend = state.as_trie_backend().unwrap();
 		let changes_trie_storage = InMemoryChangesTrieStorage::<Blake2Hasher, u64>::new();
 		let mut overlay = OverlayedChanges::default();
+		let mut cache = StorageTransactionCache::default();
 		let mut ext = Ext::new(
 			&mut overlay,
+			&mut cache,
 			backend,
 			Some(&changes_trie_storage),
 			None,
@@ -1104,8 +1099,10 @@ mod tests {
 		let mut transaction = {
 			let backend = test_trie();
 			let changes_trie_storage = InMemoryChangesTrieStorage::<Blake2Hasher, u64>::new();
+			let mut cache = StorageTransactionCache::default();
 			let mut ext = Ext::new(
 				&mut overlay,
+				&mut cache,
 				&backend,
 				Some(&changes_trie_storage),
 				None,
@@ -1113,7 +1110,7 @@ mod tests {
 			ext.set_child_storage(subtrie1, CHILD_INFO_1, b"abc".to_vec(), b"def".to_vec());
 			ext.set_child_storage(subtrie2, CHILD_INFO_2, b"abc".to_vec(), b"def".to_vec());
 			ext.storage_root();
-			(ext.transaction().0).0
+			cache.transaction.unwrap()
 		};
 		let mut duplicate = false;
 		for (k, (value, rc)) in transaction.drain().iter() {