From c14ff62f0a2bca89f02f8e1cb7c1347f1d2a0baa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 29 Jun 2023 18:01:45 +0200 Subject: [PATCH] sp-api: Support nested transactions (#14447) * sp-api: Support nested transactions Adds support for nested transactions in `sp-api` by using `execute_in_transaction`. This was working until a recent refactor, but this was actually not intended. However, supporting nested transactions is a worthwhile feature to have. So, this pr "brings it back" and adds a test to ensure it will not break. * Make clippy happy * Assert that the runtime api type is not unwind safe * Count number of transactions --- Cargo.lock | 1 + .../api/proc-macro/src/impl_runtime_apis.rs | 85 ++++++++++--------- primitives/api/test/Cargo.toml | 1 + primitives/api/test/tests/runtime_calls.rs | 54 +++++++++++- test-utils/runtime/src/lib.rs | 10 +++ 5 files changed, 109 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 08be231be3e55..a4eeeab6fc36a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10683,6 +10683,7 @@ dependencies = [ "sp-state-machine", "sp-tracing", "sp-version", + "static_assertions", "substrate-test-runtime-client", "trybuild", ] diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index fa933ceb91797..3883dfa4925e6 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -229,7 +229,7 @@ fn generate_runtime_api_base_structures() -> Result { #crate_::std_enabled! { pub struct RuntimeApiImpl + 'static> { call: &'static C, - commit_on_success: std::cell::RefCell, + transaction_depth: std::cell::RefCell, changes: std::cell::RefCell<#crate_::OverlayedChanges>, storage_transaction_cache: std::cell::RefCell< #crate_::StorageTransactionCache @@ -248,11 +248,15 @@ fn generate_runtime_api_base_structures() -> Result { ) -> R where Self: Sized { self.start_transaction(); - *std::cell::RefCell::borrow_mut(&self.commit_on_success) = false; + *std::cell::RefCell::borrow_mut(&self.transaction_depth) += 1; let res = call(self); - *std::cell::RefCell::borrow_mut(&self.commit_on_success) = true; + std::cell::RefCell::borrow_mut(&self.transaction_depth) + .checked_sub(1) + .expect("Transactions are opened and closed together; qed"); - self.commit_or_rollback(std::matches!(res, #crate_::TransactionOutcome::Commit(_))); + self.commit_or_rollback_transaction( + std::matches!(res, #crate_::TransactionOutcome::Commit(_)) + ); res.into_inner() } @@ -332,7 +336,7 @@ fn generate_runtime_api_base_structures() -> Result { ) -> #crate_::ApiRef<'a, Self::RuntimeApi> { RuntimeApiImpl { call: unsafe { std::mem::transmute(call) }, - commit_on_success: true.into(), + transaction_depth: 0.into(), changes: std::default::Default::default(), recorder: std::default::Default::default(), storage_transaction_cache: std::default::Default::default(), @@ -341,52 +345,47 @@ fn generate_runtime_api_base_structures() -> Result { } impl> RuntimeApiImpl { - fn commit_or_rollback(&self, commit: bool) { + fn commit_or_rollback_transaction(&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 *std::cell::RefCell::borrow(&self.commit_on_success) { - let res = if commit { - let res = if let Some(recorder) = &self.recorder { - #crate_::ProofRecorder::::commit_transaction(&recorder) - } else { - Ok(()) - }; - let res2 = #crate_::OverlayedChanges::commit_transaction( - &mut std::cell::RefCell::borrow_mut(&self.changes) - ); - - // Will panic on an `Err` below, however we should call commit - // on the recorder and the changes together. - std::result::Result::and(res, std::result::Result::map_err(res2, drop)) + let res = if commit { + let res = if let Some(recorder) = &self.recorder { + #crate_::ProofRecorder::::commit_transaction(&recorder) } else { - let res = if let Some(recorder) = &self.recorder { - #crate_::ProofRecorder::::rollback_transaction(&recorder) - } else { - Ok(()) - }; + Ok(()) + }; - let res2 = #crate_::OverlayedChanges::rollback_transaction( - &mut std::cell::RefCell::borrow_mut(&self.changes) - ); + let res2 = #crate_::OverlayedChanges::commit_transaction( + &mut std::cell::RefCell::borrow_mut(&self.changes) + ); - // Will panic on an `Err` below, however we should call commit - // on the recorder and the changes together. - std::result::Result::and(res, std::result::Result::map_err(res2, drop)) + // Will panic on an `Err` below, however we should call commit + // on the recorder and the changes together. + std::result::Result::and(res, std::result::Result::map_err(res2, drop)) + } else { + let res = if let Some(recorder) = &self.recorder { + #crate_::ProofRecorder::::rollback_transaction(&recorder) + } else { + Ok(()) }; - std::result::Result::expect(res, proof); - } + let res2 = #crate_::OverlayedChanges::rollback_transaction( + &mut std::cell::RefCell::borrow_mut(&self.changes) + ); + + // Will panic on an `Err` below, however we should call commit + // on the recorder and the changes together. + std::result::Result::and(res, std::result::Result::map_err(res2, drop)) + }; + + std::result::Result::expect(res, proof); } fn start_transaction(&self) { - if !*std::cell::RefCell::borrow(&self.commit_on_success) { - return - } - #crate_::OverlayedChanges::start_transaction( &mut std::cell::RefCell::borrow_mut(&self.changes) ); @@ -486,7 +485,13 @@ impl<'a> ApiRuntimeImplToApiRuntimeApiImpl<'a> { params: std::vec::Vec, fn_name: &dyn Fn(#crate_::RuntimeVersion) -> &'static str, ) -> std::result::Result, #crate_::ApiError> { - self.start_transaction(); + // If we are not already in a transaction, we should create a new transaction + // and then commit/roll it back at the end! + let transaction_depth = *std::cell::RefCell::borrow(&self.transaction_depth); + + if transaction_depth == 0 { + self.start_transaction(); + } let res = (|| { let version = #crate_::CallApiAt::<__SrApiBlock__>::runtime_version_at( @@ -510,7 +515,9 @@ impl<'a> ApiRuntimeImplToApiRuntimeApiImpl<'a> { ) })(); - self.commit_or_rollback(std::result::Result::is_ok(&res)); + if transaction_depth == 0 { + self.commit_or_rollback_transaction(std::result::Result::is_ok(&res)); + } res } diff --git a/primitives/api/test/Cargo.toml b/primitives/api/test/Cargo.toml index c07050c0efc12..87c5eb6199efe 100644 --- a/primitives/api/test/Cargo.toml +++ b/primitives/api/test/Cargo.toml @@ -30,6 +30,7 @@ criterion = "0.4.0" futures = "0.3.21" log = "0.4.17" sp-core = { version = "21.0.0", path = "../../core" } +static_assertions = "1.1.0" [[bench]] name = "bench" diff --git a/primitives/api/test/tests/runtime_calls.rs b/primitives/api/test/tests/runtime_calls.rs index a591bc0b77938..344c2d31eb0a6 100644 --- a/primitives/api/test/tests/runtime_calls.rs +++ b/primitives/api/test/tests/runtime_calls.rs @@ -15,15 +15,20 @@ // See the License for the specific language governing permissions and // limitations under the License. -use sp_api::{Core, ProvideRuntimeApi}; -use sp_runtime::traits::{HashFor, Header as HeaderT}; +use std::panic::UnwindSafe; + +use sp_api::{ApiExt, Core, ProvideRuntimeApi}; +use sp_runtime::{ + traits::{HashFor, Header as HeaderT}, + TransactionOutcome, +}; use sp_state_machine::{ create_proof_check_backend, execution_proof_check_on_trie_backend, ExecutionStrategy, }; use substrate_test_runtime_client::{ prelude::*, runtime::{Block, Header, TestAPI, Transfer}, - DefaultTestClientBuilderExt, TestClientBuilder, + DefaultTestClientBuilderExt, TestClient, TestClientBuilder, }; use codec::Encode; @@ -187,3 +192,46 @@ fn disable_logging_works() { assert!(output.contains("Logging from native works")); } } + +// Certain logic like the transaction handling is not unwind safe. +// +// Ensure that the type is not unwind safe! +static_assertions::assert_not_impl_any!(>::Api: UnwindSafe); + +#[test] +fn ensure_transactional_works() { + const KEY: &[u8] = b"test"; + + let client = TestClientBuilder::new().build(); + let best_hash = client.chain_info().best_hash; + + let runtime_api = client.runtime_api(); + runtime_api.execute_in_transaction(|api| { + api.write_key_value(best_hash, KEY.to_vec(), vec![1, 2, 3], false).unwrap(); + + api.execute_in_transaction(|api| { + api.write_key_value(best_hash, KEY.to_vec(), vec![1, 2, 3, 4], false).unwrap(); + + TransactionOutcome::Commit(()) + }); + + TransactionOutcome::Commit(()) + }); + + let changes = runtime_api + .into_storage_changes(&client.state_at(best_hash).unwrap(), best_hash) + .unwrap(); + assert_eq!(changes.main_storage_changes[0].1, Some(vec![1, 2, 3, 4])); + + let runtime_api = client.runtime_api(); + runtime_api.execute_in_transaction(|api| { + assert!(api.write_key_value(best_hash, KEY.to_vec(), vec![1, 2, 3], true).is_err()); + + TransactionOutcome::Commit(()) + }); + + let changes = runtime_api + .into_storage_changes(&client.state_at(best_hash).unwrap(), best_hash) + .unwrap(); + assert_eq!(changes.main_storage_changes[0].1, Some(vec![1, 2, 3])); +} diff --git a/test-utils/runtime/src/lib.rs b/test-utils/runtime/src/lib.rs index 8d77439f16455..d02b378e154f2 100644 --- a/test-utils/runtime/src/lib.rs +++ b/test-utils/runtime/src/lib.rs @@ -217,6 +217,8 @@ decl_runtime_apis! { fn do_trace_log(); /// Verify the given signature, public & message bundle. fn verify_ed25519(sig: ed25519::Signature, public: ed25519::Public, message: Vec) -> bool; + /// Write the given `value` under the given `key` into the storage and then optional panic. + fn write_key_value(key: Vec, value: Vec, panic: bool); } } @@ -606,6 +608,14 @@ impl_runtime_apis! { fn verify_ed25519(sig: ed25519::Signature, public: ed25519::Public, message: Vec) -> bool { sp_io::crypto::ed25519_verify(&sig, &message, &public) } + + fn write_key_value(key: Vec, value: Vec, panic: bool) { + sp_io::storage::set(&key, &value); + + if panic { + panic!("I'm just following my master"); + } + } } impl sp_consensus_aura::AuraApi for Runtime {