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

Add Limit to Tranasctional Layers #10808

Merged
merged 15 commits into from
Apr 2, 2022

Conversation

shawntabrizi
Copy link
Member

@shawntabrizi shawntabrizi commented Feb 7, 2022

Breaking down: #10744

This PR simply adds a limit to the number of tranactional layers allowed within FRAME.

It sets the limit to 255.

It requires that all uses of with_transactional can return a Result where the error can be mapped to DispatchError.

We also provide a with_transactional_unchecked which is has the same behavior as before, and can be used (hopefully only temporarily), where it is not easy to update to the new required signature.

polkadot companion: paritytech/polkadot#5232

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Feb 7, 2022
@stale
Copy link

stale bot commented Mar 9, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 9, 2022
@stale stale bot closed this Mar 23, 2022
@shawntabrizi shawntabrizi reopened this Mar 24, 2022
@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 24, 2022
@shawntabrizi shawntabrizi added E5-breaksapi C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Mar 24, 2022
frame/contracts/src/exec.rs Outdated Show resolved Hide resolved
@kianenigma
Copy link
Contributor

sorry I had to force push because I pushed to commits here by mistake, and if I would only revert them, then the diff of any branch on top of this branch would have been messed up.

@athei
Copy link
Member

athei commented Apr 1, 2022

This might be totally naive but couldn't we create something like this for use within the runtime where we know hat we are single threaded?

use core::cell::RefCell;

struct RuntimeMutex<T>(RefCell<T>);

unsafe impl<T> Sync for RuntimeMutex<T>;

impl Deref<T> for RuntimeMutex<T> {
   type Target = RefCell<T>;
   fn deref(&self) -> &Self::Target {
      &self.0
   }
}

frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved
frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/transactional.rs Outdated Show resolved Hide resolved
Co-authored-by: Alexander Theißen <[email protected]>
thread_local! {
static TRANSACTION_LEVEL: RefCell<u32> = RefCell::new(0);
pub fn get_transaction_level() -> Layer {
crate::storage::unhashed::get_or_default::<Layer>(TRANSACTION_LEVEL_KEY)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use a global variable, well, for a non persistent global variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean something like this? master...kiz-shawntabrizi-transactional-rework2

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Member

@athei athei Apr 2, 2022

Choose a reason for hiding this comment

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

I think environmental is very cumbersome in this case. Using storage is clearly not the right way to do it but it works for now. In this case (simple counter) we could just use atomics which compile into plain arithmetic for wasm.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

LGTM. I explored using environmental (akin to thread_local! globals) a bit instead of temp storage, but it seemed to imply that we have to wrap the outermost dispatch call, somewhere in executive, with a function to bring the global reference into scope, and this seemed like a bad design choice to me. The nice thing about using a temp storage is that it need not expose this detail to executive, and the existence of the transactional layer is only contained in frame-support and storage crates.

I think using the environmental might become more and more sensible over time if we have more global variable that we want to exist in scope inside the runtime. So, instead of environmental!(transaction_layers: u32), we would have environmental!(temp_storage: BTreeMap<Vec<u8>, Vec<u8>>), and this environmental reference is initialized in executive, and can store any temporary data that the runtime needs only within a single invocation.

Alternatively, I wonder if it is too much of a bad pattern to make the assumption that a FRAME runtime will always be single-threaded, thus we can create a global static mut variable and mutate it on the fly. Of course, this will be unsafe.

@xlc
Copy link
Contributor

xlc commented Apr 2, 2022

I don't really think we are going to have multi threaded execution within a single WASM runtime?
We do have experimental support of multi threaded execution and that's executing multiple WASM runtime concurrently so the assumption will still hold true.

I do agree we need some global variable solution here as we already have many cases are using storage for exact that purpose and I will assume simple in memory global variable will be faster than the in memory change overlay by avoiding the ecode/decode and hashes.

@athei
Copy link
Member

athei commented Apr 2, 2022

I don't really think we are going to have multi threaded execution within a single WASM runtime?

I agree. The only mechanism that exists constitutes a pure function (no shared memory with the spawner). Apart from that the current thinking is that parallelism happens on the validator level (validating multiple shards at once) rather than per runtime.

Alternatively, I wonder if it is too much of a bad pattern to make the assumption that a FRAME runtime will always be single-threaded, thus we can create a global static mut variable and mutate it on the fly. Of course, this will be unsafe.

I think this is a fair assumption. Still need to care for re-entry, though. This is why we can't just unsafely access mutable globals but must use RefCell and Cell. We should newtype it and implement Sync on it.

@athei
Copy link
Member

athei commented Apr 2, 2022

I pushed a change that uses atomics to implement the counter. They compile to just basic arithmetic in wasm. Native runtime will still use the more expensive synchronization instructions but we shouldn't optimize for this case. The proper solution would probably be having Sync wrappers around Cell and RefCell.

Just revert my commit if you don't like it.

@shawntabrizi
Copy link
Member Author

Thank you @athei

@shawntabrizi
Copy link
Member Author

bot merge

@paritytech-processbot
Copy link

Error: Approval criteria was not satisfied in paritytech/polkadot#5232.

The following errors might have affected the outcome of this attempt:

Merge failed. Check out the criteria for merge. If you're not meeting the approval count, check if the approvers are team members of substrateteamleads or core-devs.

@shawntabrizi
Copy link
Member Author

bot merge

use core::sync::atomic::{AtomicU32, Ordering};

type Layer = u32;
static NUM_LEVELS: AtomicU32 = AtomicU32::new(0);
Copy link
Member

Choose a reason for hiding this comment

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

The native runtime can be executed from multiple threads....

Why don't we just have used the environmental crate then?

Copy link
Member

@athei athei Apr 3, 2022

Choose a reason for hiding this comment

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

It is hard to do here because there is no easy common function where to place it. Shawn tried that.

Copy link
Member

Choose a reason for hiding this comment

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

@kianenigma had a solution that looked reasonable to me.

@athei athei mentioned this pull request Apr 3, 2022
@shawntabrizi
Copy link
Member Author

fixed up in #11163 (comment)

@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jun 7, 2022
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* introduce hard limit to transactional

* add single layer transactional

* remove single_transactional

* Update mod.rs

* add tests

* maybe fix contracts cc @athei

* fmt

* fix contract logic

* Update frame/contracts/src/exec.rs

Co-authored-by: Alexander Theißen <[email protected]>

* Update exec.rs

* add unchecked and custom errors

* Update lib.rs

* Apply suggestions from code review

Co-authored-by: Alexander Theißen <[email protected]>

* Replace storage access by atomics

Co-authored-by: Alexander Theißen <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* introduce hard limit to transactional

* add single layer transactional

* remove single_transactional

* Update mod.rs

* add tests

* maybe fix contracts cc @athei

* fmt

* fix contract logic

* Update frame/contracts/src/exec.rs

Co-authored-by: Alexander Theißen <[email protected]>

* Update exec.rs

* add unchecked and custom errors

* Update lib.rs

* Apply suggestions from code review

Co-authored-by: Alexander Theißen <[email protected]>

* Replace storage access by atomics

Co-authored-by: Alexander Theißen <[email protected]>
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. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants