Move create_inherents into the block-builder (#6553)

* Move `create_inherents` into the block-builder

This moves the `create_inherents` call into the block-builder. This has
the advantage that `create_inherents` will be able to reuse the same
context that will be used when applying the extrinsics and we also save
one call to `on_initialize`. To make sure that `create_inherents` does
not modify any state, we execute it in a transaction that is
rolled-back after doing the runtime call.

* Feedback and build fix

* Update primitives/runtime/src/lib.rs

Co-authored-by: Sergei Shulepov <sergei@parity.io>

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

Co-authored-by: Sergei Shulepov <sergei@parity.io>

substrate/Cargo.lock

@@ -5994,6 +5994,7 @@ dependencies = [
  "sp-blockchain",
  "sp-consensus",
  "sp-core",
+ "sp-inherents",
  "sp-runtime",
  "sp-state-machine",
  "sp-trie",

substrate/client/basic-authorship/src/basic_authorship.rs

@@ -26,7 +26,6 @@ use codec::Decode;
 use sp_consensus::{evaluation, Proposal, RecordProof};
 use sp_inherents::InherentData;
 use log::{error, info, debug, trace, warn};
-use sp_core::ExecutionContext;
 use sp_runtime::{
 	generic::BlockId,
 	traits::{Block as BlockT, Hash as HashT, Header as HeaderT, DigestFor, BlakeTwo256},
@@ -200,15 +199,7 @@ impl<A, B, Block, C> Proposer<B, Block, C, A>
 			record_proof,
 		)?;
 
-		// We don't check the API versions any further here since the dispatch compatibility
+		for inherent in block_builder.create_inherents(inherent_data)? {
-		// check should be enough.
-		for inherent in self.client.runtime_api()
-			.inherent_extrinsics_with_context(
-				&self.parent_id,
-				ExecutionContext::BlockConstruction,
-				inherent_data
-			)?
-		{
 			match block_builder.push(inherent) {
 				Err(ApplyExtrinsicFailed(Validity(e))) if e.exhausted_resources() =>
 					warn!("⚠️  Dropping non-mandatory inherent from overweight block."),

substrate/client/block-builder/Cargo.toml

@@ -20,6 +20,7 @@ sp-consensus = { version = "0.8.0-rc4", path = "../../primitives/consensus/commo
 sp-blockchain = { version = "2.0.0-rc4", path = "../../primitives/blockchain" }
 sp-core = { version = "2.0.0-rc4", path = "../../primitives/core" }
 sp-block-builder = { version = "2.0.0-rc4", path = "../../primitives/block-builder" }
+sp-inherents = { version = "2.0.0-rc4", path = "../../primitives/inherents" }
 sc-client-api = { version = "2.0.0-rc4", path = "../api" }
 codec = { package = "parity-scale-codec", version = "1.3.1", features = ["derive"] }
 

substrate/client/block-builder/src/lib.rs

@@ -34,7 +34,10 @@ use sp_runtime::{
 };
 use sp_blockchain::{ApplyExtrinsicFailed, Error};
 use sp_core::ExecutionContext;
-use sp_api::{Core, ApiExt, ApiErrorFor, ApiRef, ProvideRuntimeApi, StorageChanges, StorageProof};
+use sp_api::{
+	Core, ApiExt, ApiErrorFor, ApiRef, ProvideRuntimeApi, StorageChanges, StorageProof,
+	TransactionOutcome,
+};
 use sp_consensus::RecordProof;
 
 pub use sp_block_builder::BlockBuilder as BlockBuilderApi;
@@ -156,17 +159,22 @@ where
 		let block_id = &self.block_id;
 		let extrinsics = &mut self.extrinsics;
 
-		self.api.map_api_result(|api| {
+		self.api.execute_in_transaction(|api| {
 			match api.apply_extrinsic_with_context(
 				block_id,
 				ExecutionContext::BlockConstruction,
 				xt.clone(),
-			)? {
+			) {
-				Ok(_) => {
+				Ok(Ok(_)) => {
 					extrinsics.push(xt);
-					Ok(())
+					TransactionOutcome::Commit(Ok(()))
 				}
-				Err(tx_validity) => Err(ApplyExtrinsicFailed::Validity(tx_validity).into()),
+				Ok(Err(tx_validity)) => {
+					TransactionOutcome::Rollback(
+						Err(ApplyExtrinsicFailed::Validity(tx_validity).into()),
+					)
+				},
+				Err(e) => TransactionOutcome::Rollback(Err(e)),
 			}
 		})
 	}
@@ -212,6 +220,25 @@ where
 			proof,
 		})
 	}
+
+	/// Create the inherents for the block.
+	///
+	/// Returns the inherents created by the runtime or an error if something failed.
+	pub fn create_inherents(
+		&mut self,
+		inherent_data: sp_inherents::InherentData,
+	) -> Result<Vec<Block::Extrinsic>, ApiErrorFor<A, Block>> {
+		let block_id = self.block_id;
+		self.api.execute_in_transaction(move |api| {
+			// `create_inherents` should not change any state, to ensure this we always rollback
+			// the transaction.
+			TransactionOutcome::Rollback(api.inherent_extrinsics_with_context(
+				&block_id,
+				ExecutionContext::BlockConstruction,
+				inherent_data
+			))
+		})
+	}
 }
 
 #[cfg(test)]

substrate/frame/support/src/storage/mod.rs

@@ -21,6 +21,7 @@ use sp_std::{prelude::*, marker::PhantomData};
 use codec::{FullCodec, FullEncode, Encode, EncodeLike, Decode};
 use crate::hash::{Twox128, StorageHasher};
 use sp_runtime::generic::{Digest, DigestItem};
+pub use sp_runtime::TransactionOutcome;
 
 pub mod unhashed;
 pub mod hashed;
@@ -29,14 +30,6 @@ pub mod child;
 pub mod generator;
 pub mod migration;
 
-/// Describes whether a storage transaction should be committed or rolled back.
-pub enum TransactionOutcome<T> {
-	/// Transaction should be committed.
-	Commit(T),
-	/// Transaction should be rolled back.
-	Rollback(T),
-}
-
 /// Execute the supplied function in a new storage transaction.
 ///
 /// All changes to storage performed by the supplied function are discarded if the returned

substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs

@@ -253,18 +253,18 @@ fn generate_runtime_api_base_structures() -> Result<TokenStream> {
 		{
 			type StateBackend = C::StateBackend;
 
-			fn map_api_result<F: FnOnce(&Self) -> std::result::Result<R, E>, R, E>(
+			fn execute_in_transaction<F: FnOnce(&Self) -> #crate_::TransactionOutcome<R>, R>(
 				&self,
-				map_call: F,
+				call: F,
-			) -> std::result::Result<R, E> where Self: Sized {
+			) -> R where Self: Sized {
 				self.changes.borrow_mut().start_transaction();
 				*self.commit_on_success.borrow_mut() = false;
-				let res = map_call(self);
+				let res = call(self);
 				*self.commit_on_success.borrow_mut() = true;
 
-				self.commit_on_ok(&res);
+				self.commit_or_rollback(matches!(res, #crate_::TransactionOutcome::Commit(_)));
 
-				res
+				res.into_inner()
 			}
 
 			fn has_api<A: #crate_::RuntimeApiInfo + ?Sized>(
@@ -380,21 +380,21 @@ fn generate_runtime_api_base_structures() -> Result<TokenStream> {
 					&self.recorder,
 				);
 
-				self.commit_on_ok(&res);
+				self.commit_or_rollback(res.is_ok());
 				res
 			}
 
-			fn commit_on_ok<R, E>(&self, res: &std::result::Result<R, E>) {
+			fn commit_or_rollback(&self, commit: bool) {
 				let proof = "\
 					We only close a transaction when we opened one ourself.
 					Other parts of the runtime that make use of transactions (state-machine)
 					also balance their transactions. The runtime cannot close client initiated
 					transactions. qed";
 				if *self.commit_on_success.borrow() {
-					if res.is_err() {
+					if commit {
-						self.changes.borrow_mut().rollback_transaction().expect(proof);
-					} else {
 						self.changes.borrow_mut().commit_transaction().expect(proof);
+					} else {
+						self.changes.borrow_mut().rollback_transaction().expect(proof);
 					}
 				}
 			}

substrate/primitives/api/proc-macro/src/mock_impl_runtime_apis.rs

@@ -73,11 +73,11 @@ fn implement_common_api_traits(
 		impl #crate_::ApiExt<#block_type> for #self_ty {
 			type StateBackend = #crate_::InMemoryBackend<#crate_::HashFor<#block_type>>;
 
-			fn map_api_result<F: FnOnce(&Self) -> std::result::Result<R, E>, R, E>(
+			fn execute_in_transaction<F: FnOnce(&Self) -> #crate_::TransactionOutcome<R>, R>(
 				&self,
-				map_call: F,
+				call: F,
-			) -> std::result::Result<R, E> where Self: Sized {
+			) -> R where Self: Sized {
-				map_call(self)
+				call(self).into_inner()
 			}
 
 			fn has_api<A: #crate_::RuntimeApiInfo + ?Sized>(

substrate/primitives/api/src/lib.rs

@@ -58,7 +58,7 @@ pub use sp_runtime::{
 		Block as BlockT, GetNodeBlockType, GetRuntimeBlockType, HashFor, NumberFor,
 		Header as HeaderT, Hash as HashT,
 	},
-	generic::BlockId, transaction_validity::TransactionValidity, RuntimeString,
+	generic::BlockId, transaction_validity::TransactionValidity, RuntimeString, TransactionOutcome,
 };
 #[doc(hidden)]
 pub use sp_core::{offchain, ExecutionContext};
@@ -356,15 +356,15 @@ pub trait ApiExt<Block: BlockT>: ApiErrorExt {
 	/// The state backend that is used to store the block states.
 	type StateBackend: StateBackend<HashFor<Block>>;
 
-	/// The given closure will be called with api instance. Inside the closure any api call is
+	/// Execute the given closure inside a new transaction.
-	/// allowed. After doing the api call, the closure is allowed to map the `Result` to a
+	///
-	/// different `Result` type. This can be important, as the internal data structure that keeps
+	/// Depending on the outcome of the closure, the transaction is committed or rolled-back.
-	/// track of modifications to the storage, discards changes when the `Result` is an `Err`.
+	///
-	/// On `Ok`, the structure commits the changes to an internal buffer.
+	/// The internal result of the closure is returned afterwards.
-	fn map_api_result<F: FnOnce(&Self) -> result::Result<R, E>, R, E>(
+	fn execute_in_transaction<F: FnOnce(&Self) -> TransactionOutcome<R>, R>(
 		&self,
-		map_call: F,
+		call: F,
-	) -> result::Result<R, E> where Self: Sized;
+	) -> R where Self: Sized;
 
 	/// Checks if the given api is implemented and versions match.
 	fn has_api<A: RuntimeApiInfo + ?Sized>(

substrate/primitives/runtime/src/lib.rs

@@ -801,6 +801,23 @@ impl Drop for SignatureBatching {
 	}
 }
 
+/// Describes on what should happen with a storage transaction.
+pub enum TransactionOutcome<R> {
+	/// Commit the transaction.
+	Commit(R),
+	/// Rollback the transaction.
+	Rollback(R),
+}
+
+impl<R> TransactionOutcome<R> {
+	/// Convert into the inner type.
+	pub fn into_inner(self) -> R {
+		match self {
+			Self::Commit(r) => r,
+			Self::Rollback(r) => r,
+		}
+	}
+}
 
 #[cfg(test)]
 mod tests {