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

Author: bkchr

Cargo.lock

@@ -229,7 +229,7 @@ fn generate_runtime_api_base_structures() -> Result<TokenStream> {
 		#crate_::std_enabled! {
 			pub struct RuntimeApiImpl<Block: #crate_::BlockT, C: #crate_::CallApiAt<Block> + 'static> {
 				call: &'static C,
-				commit_on_success: std::cell::RefCell<bool>,
+				transaction_depth: std::cell::RefCell<u16>,
 				changes: std::cell::RefCell<#crate_::OverlayedChanges>,
 				storage_transaction_cache: std::cell::RefCell<
 					#crate_::StorageTransactionCache<Block, C::StateBackend>
@@ -248,11 +248,15 @@ fn generate_runtime_api_base_structures() -> Result<TokenStream> {
 				) -> 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<TokenStream> {
 				) -> #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<TokenStream> {
 			}
 
 			impl<Block: #crate_::BlockT, C: #crate_::CallApiAt<Block>> RuntimeApiImpl<Block, C> {
-				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::<Block>::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::<Block>::commit_transaction(&recorder)
 						} else {
-							let res = if let Some(recorder) = &self.recorder {
-								#crate_::ProofRecorder::<Block>::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::<Block>::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<u8>,
 				fn_name: &dyn Fn(#crate_::RuntimeVersion) -> &'static str,
 			) -> std::result::Result<std::vec::Vec<u8>, #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
 			}

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

@@ -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"

primitives/api/test/Cargo.toml

@@ -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!(<TestClient as ProvideRuntimeApi<_>>::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]));
+}

primitives/api/test/tests/runtime_calls.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<u8>) -> bool;
+		/// Write the given `value` under the given `key` into the storage and then optional panic.
+		fn write_key_value(key: Vec<u8>, value: Vec<u8>, panic: bool);
 	}
 }
 
@@ -606,6 +608,14 @@ impl_runtime_apis! {
 		fn verify_ed25519(sig: ed25519::Signature, public: ed25519::Public, message: Vec<u8>) -> bool {
 			sp_io::crypto::ed25519_verify(&sig, &message, &public)
 		}
+
+		fn write_key_value(key: Vec<u8>, value: Vec<u8>, panic: bool) {
+			sp_io::storage::set(&key, &value);
+
+			if panic {
+				panic!("I'm just following my master");
+			}
+		}
 	}
 
 	impl sp_consensus_aura::AuraApi<Block, AuraId> for Runtime {