Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

sp-api: Support nested transactions #14447

Merged
merged 6 commits into from
Jun 29, 2023
Merged

sp-api: Support nested transactions #14447

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9575424
sp-api: Support nested transactions
bkchr Jun 23, 2023
0985dfa
Merge remote-tracking branch 'origin/master' into bkchr-sp-api-nested…
bkchr Jun 23, 2023
8dc42cf
Make clippy happy
bkchr Jun 23, 2023
8b90805
Assert that the runtime api type is not unwind safe
bkchr Jun 24, 2023
2297278
Count number of transactions
bkchr Jun 28, 2023
3b87f5b
Merge remote-tracking branch 'origin/master' into bkchr-sp-api-nested…
bkchr Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 44 additions & 39 deletions primitives/api/proc-macro/src/impl_runtime_apis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
in_transaction: std::cell::RefCell<bool>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, can't this use Cell instead of RefCell?

changes: std::cell::RefCell<#crate_::OverlayedChanges>,
storage_transaction_cache: std::cell::RefCell<
#crate_::StorageTransactionCache<Block, C::StateBackend>
Expand All @@ -248,11 +248,13 @@ 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;
let old_value = std::cell::RefCell::replace(&self.in_transaction, true);
let res = call(self);
*std::cell::RefCell::borrow_mut(&self.commit_on_success) = true;
*std::cell::RefCell::borrow_mut(&self.in_transaction) = old_value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not matter in practice (?), but commit_or_rollback_transaction can panic, and now we're going to support nested transactions, which means that call can panic, which means that if it does then the in_transaction won't get restored to the old value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I know, but the type should not be unwind safe (which is the case). However, I added a test to ensure this.

Copy link
Contributor

@koute koute Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I know, but the type should not be unwind safe (which is the case). However, I added a test to ensure this.

Hmmm... to be honest, I'm not sure how useful that is considering how borderline useless the UnwindSafe trait is. (Since e.g. even &mut T ends up being not UnwindSafe, which makes it extremely false positive-y, which makes the use of AssertUnwindSafe very common.)

Maybe instead of in_transaction: bool we could have e.g. transaction_depth: usize and increment/decrement it when we enter/leave a transaction? I think that'd be a little nicer. (And in case there's any funny business going to it'd be easier to debug since you'd have a stale non-zero counter.)

But it's up to you.


self.commit_or_rollback(std::matches!(res, #crate_::TransactionOutcome::Commit(_)));
self.commit_or_rollback_transaction(
std::matches!(res, #crate_::TransactionOutcome::Commit(_))
);

res.into_inner()
}
Expand Down Expand Up @@ -332,7 +334,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(),
in_transaction: false.into(),
changes: std::default::Default::default(),
recorder: std::default::Default::default(),
storage_transaction_cache: std::default::Default::default(),
Expand All @@ -341,52 +343,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)
);
Expand Down Expand Up @@ -486,7 +483,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 in_transaction = *std::cell::RefCell::borrow(&self.in_transaction);

if !in_transaction {
self.start_transaction();
}

let res = (|| {
let version = #crate_::CallApiAt::<__SrApiBlock__>::runtime_version_at(
Expand All @@ -510,7 +513,9 @@ impl<'a> ApiRuntimeImplToApiRuntimeApiImpl<'a> {
)
})();

self.commit_or_rollback(std::result::Result::is_ok(&res));
if !in_transaction {
self.commit_or_rollback_transaction(std::result::Result::is_ok(&res));
}

res
}
Expand Down
45 changes: 43 additions & 2 deletions primitives/api/test/tests/runtime_calls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,11 @@
// 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 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,
};
Expand Down Expand Up @@ -187,3 +190,41 @@ fn disable_logging_works() {
assert!(output.contains("Logging from native works"));
}
}

#[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]));
}
10 changes: 10 additions & 0 deletions test-utils/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
bkchr marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

}
}
}

impl sp_consensus_aura::AuraApi<Block, AuraId> for Runtime {
Expand Down