diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index 540ddaa4a1..9ea9b89057 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -5994,6 +5994,7 @@ dependencies = [ "sp-blockchain", "sp-consensus", "sp-core", + "sp-inherents", "sp-runtime", "sp-state-machine", "sp-trie", diff --git a/substrate/client/basic-authorship/src/basic_authorship.rs b/substrate/client/basic-authorship/src/basic_authorship.rs index 383d0ea6fc..7343b13c04 100644 --- a/substrate/client/basic-authorship/src/basic_authorship.rs +++ b/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 Proposer record_proof, )?; - // We don't check the API versions any further here since the dispatch compatibility - // check should be enough. - for inherent in self.client.runtime_api() - .inherent_extrinsics_with_context( - &self.parent_id, - ExecutionContext::BlockConstruction, - inherent_data - )? - { + for inherent in block_builder.create_inherents(inherent_data)? { match block_builder.push(inherent) { Err(ApplyExtrinsicFailed(Validity(e))) if e.exhausted_resources() => warn!("⚠️ Dropping non-mandatory inherent from overweight block."), diff --git a/substrate/client/block-builder/Cargo.toml b/substrate/client/block-builder/Cargo.toml index 1e733355f7..a56ff61cd0 100644 --- a/substrate/client/block-builder/Cargo.toml +++ b/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"] } diff --git a/substrate/client/block-builder/src/lib.rs b/substrate/client/block-builder/src/lib.rs index af40b33662..904667b1af 100644 --- a/substrate/client/block-builder/src/lib.rs +++ b/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, ApiErrorFor> { + 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)] diff --git a/substrate/frame/support/src/storage/mod.rs b/substrate/frame/support/src/storage/mod.rs index c2d7ceef0f..b8b08c5dc0 100644 --- a/substrate/frame/support/src/storage/mod.rs +++ b/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 { - /// 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 diff --git a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs index a4c35dcf42..97b159e6f0 100644 --- a/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -253,18 +253,18 @@ fn generate_runtime_api_base_structures() -> Result { { type StateBackend = C::StateBackend; - fn map_api_result std::result::Result, R, E>( + fn execute_in_transaction #crate_::TransactionOutcome, R>( &self, - map_call: F, - ) -> std::result::Result where Self: Sized { + call: F, + ) -> 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( @@ -380,21 +380,21 @@ fn generate_runtime_api_base_structures() -> Result { &self.recorder, ); - self.commit_on_ok(&res); + self.commit_or_rollback(res.is_ok()); res } - fn commit_on_ok(&self, res: &std::result::Result) { + 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() { - self.changes.borrow_mut().rollback_transaction().expect(proof); - } else { + if commit { self.changes.borrow_mut().commit_transaction().expect(proof); + } else { + self.changes.borrow_mut().rollback_transaction().expect(proof); } } } diff --git a/substrate/primitives/api/proc-macro/src/mock_impl_runtime_apis.rs b/substrate/primitives/api/proc-macro/src/mock_impl_runtime_apis.rs index 028ef57939..0e8f18e3e6 100644 --- a/substrate/primitives/api/proc-macro/src/mock_impl_runtime_apis.rs +++ b/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 std::result::Result, R, E>( + fn execute_in_transaction #crate_::TransactionOutcome, R>( &self, - map_call: F, - ) -> std::result::Result where Self: Sized { - map_call(self) + call: F, + ) -> R where Self: Sized { + call(self).into_inner() } fn has_api( diff --git a/substrate/primitives/api/src/lib.rs b/substrate/primitives/api/src/lib.rs index 0aaf72e2d2..bad6c03058 100644 --- a/substrate/primitives/api/src/lib.rs +++ b/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: ApiErrorExt { /// The state backend that is used to store the block states. type StateBackend: StateBackend>; - /// The given closure will be called with api instance. Inside the closure any api call is - /// 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 - /// 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. - fn map_api_result result::Result, R, E>( + /// Execute the given closure inside a new transaction. + /// + /// Depending on the outcome of the closure, the transaction is committed or rolled-back. + /// + /// The internal result of the closure is returned afterwards. + fn execute_in_transaction TransactionOutcome, R>( &self, - map_call: F, - ) -> result::Result where Self: Sized; + call: F, + ) -> R where Self: Sized; /// Checks if the given api is implemented and versions match. fn has_api( diff --git a/substrate/primitives/runtime/src/lib.rs b/substrate/primitives/runtime/src/lib.rs index b27cb0c633..02031a2df9 100644 --- a/substrate/primitives/runtime/src/lib.rs +++ b/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 { + /// Commit the transaction. + Commit(R), + /// Rollback the transaction. + Rollback(R), +} + +impl TransactionOutcome { + /// 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 {