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

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Jun 23, 2023

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.

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.
@bkchr bkchr added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. B1-note_worthy Changes should be noted in the release notes T0-node This PR/Issue is related to the topic “node”. labels Jun 23, 2023
@bkchr bkchr requested a review from a team June 23, 2023 12:47
@bkchr bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Jun 23, 2023
Comment on lines 251 to 253
*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

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.

test-utils/runtime/src/lib.rs Show resolved Hide resolved
@bkchr bkchr requested a review from a team June 24, 2023 20:36
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.

:)

@bkchr bkchr requested a review from a team June 27, 2023 13:18
Comment on lines 251 to 253
*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

@koute koute Jun 27, 2023

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.

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

@bkchr
Copy link
Member Author

bkchr commented Jun 28, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for 3b87f5b

@bkchr bkchr merged commit c14ff62 into master Jun 29, 2023
@bkchr bkchr deleted the bkchr-sp-api-nested-transactions branch June 29, 2023 16:01
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants