From a967cbcde0c06e4acd0a1fee4fb9e240c77aad1a Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sat, 5 Feb 2022 17:27:00 +0100 Subject: [PATCH 01/14] introduce hard limit to transactional --- frame/support/procedural/src/transactional.rs | 4 +- frame/support/src/storage/mod.rs | 125 ++++++++++-------- .../support/test/tests/storage_transaction.rs | 47 ++++--- primitives/runtime/src/lib.rs | 4 + 4 files changed, 106 insertions(+), 74 deletions(-) diff --git a/frame/support/procedural/src/transactional.rs b/frame/support/procedural/src/transactional.rs index 66a8d083fb562..b3984cac031f5 100644 --- a/frame/support/procedural/src/transactional.rs +++ b/frame/support/procedural/src/transactional.rs @@ -49,7 +49,9 @@ pub fn require_transactional(_attr: TokenStream, input: TokenStream) -> Result = RefCell::new(0); + pub fn get_transaction_level() -> Layer { + crate::storage::unhashed::get_or_default::(TRANSACTION_LEVEL_KEY) } - pub fn require_transaction() { - let level = TRANSACTION_LEVEL.with(|v| *v.borrow()); - if level == 0 { - panic!("Require transaction not called within with_transaction"); - } + fn set_transaction_level(level: &Layer) { + crate::storage::unhashed::put::(TRANSACTION_LEVEL_KEY, level); } - pub struct TransactionLevelGuard; - - impl Drop for TransactionLevelGuard { - fn drop(&mut self) { - TRANSACTION_LEVEL.with(|v| *v.borrow_mut() -= 1); - } + fn kill_transaction_level() { + crate::storage::unhashed::kill(TRANSACTION_LEVEL_KEY); } - /// Increments the transaction level. + /// Increments the transaction level. Returns an error if levels go past the limit. /// /// Returns a guard that when dropped decrements the transaction level automatically. - pub fn inc_transaction_level() -> TransactionLevelGuard { - TRANSACTION_LEVEL.with(|v| { - let mut val = v.borrow_mut(); - *val += 1; - if *val > 10 { - log::warn!( - "Detected with_transaction with nest level {}. Nested usage of with_transaction is not recommended.", - *val - ); - } - }); + pub fn inc_transaction_level() -> Result { + let existing_levels = get_transaction_level(); + if existing_levels >= TRANSACTIONAL_LIMIT || existing_levels == Layer::MAX { + return Err(()) + } + // Cannot overflow because of check above. + set_transaction_level(&(existing_levels + 1)); + Ok(StorageLayerGuard) + } - TransactionLevelGuard + fn dec_transaction_level() { + let existing_levels = get_transaction_level(); + if existing_levels == 0 { + log::warn!( + "We are underflowing with calculating transactional levels. Not great, but let's not panic...", + ); + } else if existing_levels == 1 { + // Don't leave any trace of this storage item. + kill_transaction_level(); + } else { + // Cannot underflow because of checks above. + set_transaction_level(&(existing_levels - 1)); + } + } + + pub fn is_transactional() -> bool { + get_transaction_level() > 0 + } + + pub struct StorageLayerGuard; + + impl Drop for StorageLayerGuard { + fn drop(&mut self) { + dec_transaction_level() + } } } -/// Assert this method is called within a storage transaction. -/// This will **panic** if is not called within a storage transaction. -/// -/// This assertion is enabled for native execution and when `debug_assertions` are enabled. -pub fn require_transaction() { - #[cfg(all(feature = "std", any(test, debug_assertions)))] - debug_helper::require_transaction(); +/// Check if the current call is within a transactional layer. +pub fn is_transactional() -> bool { + transaction_level_tracker::is_transactional() } /// Execute the supplied function in a new storage transaction. @@ -101,14 +113,19 @@ pub fn require_transaction() { /// outcome is `TransactionOutcome::Rollback`. /// /// Transactions can be nested to any depth. Commits happen to the parent transaction. -pub fn with_transaction(f: impl FnOnce() -> TransactionOutcome) -> R { +pub fn with_transaction(f: impl FnOnce() -> TransactionOutcome>) -> Result +where + E: From, +{ use sp_io::storage::{commit_transaction, rollback_transaction, start_transaction}; use TransactionOutcome::*; - start_transaction(); + // TODO: Return a result here. + let _guard = transaction_level_tracker::inc_transaction_level().map_err(|()| + DispatchError::TransactionalLimit.into() + )?; - #[cfg(all(feature = "std", any(test, debug_assertions)))] - let _guard = debug_helper::inc_transaction_level(); + start_transaction(); match f() { Commit(res) => { @@ -1415,13 +1432,14 @@ pub fn storage_prefix(pallet_name: &[u8], storage_name: &[u8]) -> [u8; 32] { #[cfg(test)] mod test { use super::*; - use crate::{assert_ok, hash::Identity, Twox128}; + use crate::{assert_ok, assert_noop, hash::Identity, Twox128}; use bounded_vec::BoundedVec; use frame_support::traits::ConstU32; use generator::StorageValue as _; use sp_core::hashing::twox_128; use sp_io::TestExternalities; use weak_bounded_vec::WeakBoundedVec; + use sp_runtime::DispatchResult; #[test] fn prefixed_map_works() { @@ -1532,25 +1550,24 @@ mod test { } #[test] - #[should_panic(expected = "Require transaction not called within with_transaction")] - fn require_transaction_should_panic() { + fn is_transaction_should_return_false() { TestExternalities::default().execute_with(|| { - require_transaction(); + assert!(!is_transactional()) }); } #[test] fn require_transaction_should_not_panic_in_with_transaction() { TestExternalities::default().execute_with(|| { - with_transaction(|| { - require_transaction(); - TransactionOutcome::Commit(()) - }); - - with_transaction(|| { - require_transaction(); - TransactionOutcome::Rollback(()) - }); + assert_ok!(with_transaction(|| -> TransactionOutcome { + assert!(is_transactional()); + TransactionOutcome::Commit(Ok(())) + })); + + assert_noop!(with_transaction(|| -> TransactionOutcome { + assert!(is_transactional()); + TransactionOutcome::Rollback(Err("revert".into())) + }), "revert"); }); } diff --git a/frame/support/test/tests/storage_transaction.rs b/frame/support/test/tests/storage_transaction.rs index 0f1c3a2e0c536..34759d2a54f90 100644 --- a/frame/support/test/tests/storage_transaction.rs +++ b/frame/support/test/tests/storage_transaction.rs @@ -16,13 +16,14 @@ // limitations under the License. use frame_support::{ - assert_noop, assert_ok, + assert_noop, assert_ok, assert_storage_noop, dispatch::{DispatchError, DispatchResult}, storage::{with_transaction, TransactionOutcome::*}, transactional, StorageMap, StorageValue, }; use sp_io::TestExternalities; use sp_std::result; +use sp_runtime::TransactionOutcome; pub trait Config: frame_support_test::Config {} @@ -67,13 +68,13 @@ fn storage_transaction_basic_commit() { assert_eq!(Value::get(), 0); assert!(!Map::contains_key("val0")); - with_transaction(|| { + assert_ok!(with_transaction(|| -> TransactionOutcome { Value::set(99); Map::insert("val0", 99); assert_eq!(Value::get(), 99); assert_eq!(Map::get("val0"), 99); - Commit(()) - }); + Commit(Ok(())) + })); assert_eq!(Value::get(), 99); assert_eq!(Map::get("val0"), 99); @@ -86,13 +87,21 @@ fn storage_transaction_basic_rollback() { assert_eq!(Value::get(), 0); assert_eq!(Map::get("val0"), 0); - with_transaction(|| { + assert_noop!(with_transaction(|| -> TransactionOutcome { Value::set(99); Map::insert("val0", 99); assert_eq!(Value::get(), 99); assert_eq!(Map::get("val0"), 99); - Rollback(()) - }); + Rollback(Err("revert".into())) + }), "revert"); + + assert_storage_noop!(assert_ok!(with_transaction(|| -> TransactionOutcome { + Value::set(99); + Map::insert("val0", 99); + assert_eq!(Value::get(), 99); + assert_eq!(Map::get("val0"), 99); + Rollback(Ok(())) + }))); assert_eq!(Value::get(), 0); assert_eq!(Map::get("val0"), 0); @@ -105,12 +114,12 @@ fn storage_transaction_rollback_then_commit() { Value::set(1); Map::insert("val1", 1); - with_transaction(|| { + assert_ok!(with_transaction(|| -> TransactionOutcome { Value::set(2); Map::insert("val1", 2); Map::insert("val2", 2); - with_transaction(|| { + assert_noop!(with_transaction(|| -> TransactionOutcome { Value::set(3); Map::insert("val1", 3); Map::insert("val2", 3); @@ -121,16 +130,16 @@ fn storage_transaction_rollback_then_commit() { assert_eq!(Map::get("val2"), 3); assert_eq!(Map::get("val3"), 3); - Rollback(()) - }); + Rollback(Err("revert".into())) + }), "revert"); assert_eq!(Value::get(), 2); assert_eq!(Map::get("val1"), 2); assert_eq!(Map::get("val2"), 2); assert_eq!(Map::get("val3"), 0); - Commit(()) - }); + Commit(Ok(())) + })); assert_eq!(Value::get(), 2); assert_eq!(Map::get("val1"), 2); @@ -145,12 +154,12 @@ fn storage_transaction_commit_then_rollback() { Value::set(1); Map::insert("val1", 1); - with_transaction(|| { + assert_noop!(with_transaction(|| -> TransactionOutcome { Value::set(2); Map::insert("val1", 2); Map::insert("val2", 2); - with_transaction(|| { + assert_ok!(with_transaction(|| -> TransactionOutcome { Value::set(3); Map::insert("val1", 3); Map::insert("val2", 3); @@ -161,16 +170,16 @@ fn storage_transaction_commit_then_rollback() { assert_eq!(Map::get("val2"), 3); assert_eq!(Map::get("val3"), 3); - Commit(()) - }); + Commit(Ok(())) + })); assert_eq!(Value::get(), 3); assert_eq!(Map::get("val1"), 3); assert_eq!(Map::get("val2"), 3); assert_eq!(Map::get("val3"), 3); - Rollback(()) - }); + Rollback(Err("revert".into())) + }), "revert"); assert_eq!(Value::get(), 1); assert_eq!(Map::get("val1"), 1); diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index 6d32d2322c765..f413edb43e5ee 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -507,6 +507,8 @@ pub enum DispatchError { Token(TokenError), /// An arithmetic error. Arithmetic(ArithmeticError), + /// The number of transactional layers has been reached, or we are not in a transactional layer. + TransactionalLimit, } /// Result of a `Dispatchable` which contains the `DispatchResult` and additional information about @@ -642,6 +644,7 @@ impl From for &'static str { DispatchError::TooManyConsumers => "Too many consumers", DispatchError::Token(e) => e.into(), DispatchError::Arithmetic(e) => e.into(), + DispatchError::TransactionalLimit => "Too many transactional layers", } } } @@ -680,6 +683,7 @@ impl traits::Printable for DispatchError { "Arithmetic error: ".print(); <&'static str>::from(*e).print(); }, + Self::TransactionalLimit => "Too many transactional layers".print(), } } } From 1936c420a5a4af00263deb3bb198232daf1ec4c1 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sat, 5 Feb 2022 17:45:49 +0100 Subject: [PATCH 02/14] add single layer transactional --- frame/support/procedural/src/lib.rs | 8 +++++- frame/support/procedural/src/transactional.rs | 28 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/frame/support/procedural/src/lib.rs b/frame/support/procedural/src/lib.rs index e2233fff72285..8faf9f3edc894 100644 --- a/frame/support/procedural/src/lib.rs +++ b/frame/support/procedural/src/lib.rs @@ -427,7 +427,13 @@ pub fn pallet(attr: TokenStream, item: TokenStream) -> TokenStream { /// ``` #[proc_macro_attribute] pub fn transactional(attr: TokenStream, input: TokenStream) -> TokenStream { - transactional::transactional(attr, input).unwrap_or_else(|e| e.to_compile_error().into()) + // TODO: I think we should do this, but not sure if breaking + transactional::single_transactional(attr, input).unwrap_or_else(|e| e.to_compile_error().into()) +} + +#[proc_macro_attribute] +pub fn single_transactional(attr: TokenStream, input: TokenStream) -> TokenStream { + transactional::single_transactional(attr, input).unwrap_or_else(|e| e.to_compile_error().into()) } /// Derive [`Clone`] but do not bound any generic. Docs are at `frame_support::CloneNoBound`. diff --git a/frame/support/procedural/src/transactional.rs b/frame/support/procedural/src/transactional.rs index b3984cac031f5..cbd85a764b437 100644 --- a/frame/support/procedural/src/transactional.rs +++ b/frame/support/procedural/src/transactional.rs @@ -42,6 +42,34 @@ pub fn transactional(_attr: TokenStream, input: TokenStream) -> Result Result { + let ItemFn { attrs, vis, sig, block } = syn::parse(input)?; + + let crate_ = generate_crate_access_2018("frame-support")?; + let output = quote! { + #(#attrs)* + #vis #sig { + use #crate_::storage::{with_transaction, is_transactional, TransactionOutcome}; + if is_transactional() { + (|| { #block })() + } else { + with_transaction(|| { + let r = (|| { #block })(); + if r.is_ok() { + TransactionOutcome::Commit(r) + } else { + TransactionOutcome::Rollback(r) + } + }) + } + } + }; + + Ok(output.into()) +} + pub fn require_transactional(_attr: TokenStream, input: TokenStream) -> Result { let ItemFn { attrs, vis, sig, block } = syn::parse(input)?; From 1f09be7538912a58befbbfbcfed7640696849626 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 7 Feb 2022 08:44:02 +0100 Subject: [PATCH 03/14] remove single_transactional --- frame/support/procedural/src/lib.rs | 8 +----- frame/support/procedural/src/transactional.rs | 28 ------------------- 2 files changed, 1 insertion(+), 35 deletions(-) diff --git a/frame/support/procedural/src/lib.rs b/frame/support/procedural/src/lib.rs index 8faf9f3edc894..e2233fff72285 100644 --- a/frame/support/procedural/src/lib.rs +++ b/frame/support/procedural/src/lib.rs @@ -427,13 +427,7 @@ pub fn pallet(attr: TokenStream, item: TokenStream) -> TokenStream { /// ``` #[proc_macro_attribute] pub fn transactional(attr: TokenStream, input: TokenStream) -> TokenStream { - // TODO: I think we should do this, but not sure if breaking - transactional::single_transactional(attr, input).unwrap_or_else(|e| e.to_compile_error().into()) -} - -#[proc_macro_attribute] -pub fn single_transactional(attr: TokenStream, input: TokenStream) -> TokenStream { - transactional::single_transactional(attr, input).unwrap_or_else(|e| e.to_compile_error().into()) + transactional::transactional(attr, input).unwrap_or_else(|e| e.to_compile_error().into()) } /// Derive [`Clone`] but do not bound any generic. Docs are at `frame_support::CloneNoBound`. diff --git a/frame/support/procedural/src/transactional.rs b/frame/support/procedural/src/transactional.rs index cbd85a764b437..b3984cac031f5 100644 --- a/frame/support/procedural/src/transactional.rs +++ b/frame/support/procedural/src/transactional.rs @@ -42,34 +42,6 @@ pub fn transactional(_attr: TokenStream, input: TokenStream) -> Result Result { - let ItemFn { attrs, vis, sig, block } = syn::parse(input)?; - - let crate_ = generate_crate_access_2018("frame-support")?; - let output = quote! { - #(#attrs)* - #vis #sig { - use #crate_::storage::{with_transaction, is_transactional, TransactionOutcome}; - if is_transactional() { - (|| { #block })() - } else { - with_transaction(|| { - let r = (|| { #block })(); - if r.is_ok() { - TransactionOutcome::Commit(r) - } else { - TransactionOutcome::Rollback(r) - } - }) - } - } - }; - - Ok(output.into()) -} - pub fn require_transactional(_attr: TokenStream, input: TokenStream) -> Result { let ItemFn { attrs, vis, sig, block } = syn::parse(input)?; From 44f1ddc4e245dd0ea499063dc35c985c5af1843d Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 7 Feb 2022 09:16:47 +0100 Subject: [PATCH 04/14] Update mod.rs --- frame/support/src/storage/mod.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 10297923ed8aa..7a8feaa10dc04 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -46,8 +46,8 @@ pub mod weak_bounded_vec; mod transaction_level_tracker { const TRANSACTION_LEVEL_KEY: &'static [u8] = b":transaction_level:"; - const TRANSACTIONAL_LIMIT: u8 = 255; - type Layer = u8; + const TRANSACTIONAL_LIMIT: u32 = 10; + type Layer = u32; pub fn get_transaction_level() -> Layer { crate::storage::unhashed::get_or_default::(TRANSACTION_LEVEL_KEY) @@ -112,7 +112,9 @@ pub fn is_transactional() -> bool { /// All changes to storage performed by the supplied function are discarded if the returned /// outcome is `TransactionOutcome::Rollback`. /// -/// Transactions can be nested to any depth. Commits happen to the parent transaction. +/// Transactions can be nested up to 10 times; more than that will result in an error. +/// +/// Commits happen to the parent transaction. pub fn with_transaction(f: impl FnOnce() -> TransactionOutcome>) -> Result where E: From, @@ -120,7 +122,6 @@ where use sp_io::storage::{commit_transaction, rollback_transaction, start_transaction}; use TransactionOutcome::*; - // TODO: Return a result here. let _guard = transaction_level_tracker::inc_transaction_level().map_err(|()| DispatchError::TransactionalLimit.into() )?; From c362b702260ca7ae52298e35914b76f874b87810 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 24 Mar 2022 19:09:56 -0400 Subject: [PATCH 05/14] add tests --- frame/support/src/storage/mod.rs | 73 ++++++++++--- .../support/test/tests/storage_transaction.rs | 103 ++++++++++-------- primitives/runtime/src/lib.rs | 3 +- 3 files changed, 118 insertions(+), 61 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index bc88c799720dc..ea9c2f19736b7 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -27,8 +27,11 @@ use crate::{ }; use codec::{Decode, Encode, EncodeLike, FullCodec, FullEncode}; use sp_core::storage::ChildInfo; -use sp_runtime::{DispatchError, generic::{Digest, DigestItem}}; pub use sp_runtime::TransactionOutcome; +use sp_runtime::{ + generic::{Digest, DigestItem}, + DispatchError, +}; use sp_std::prelude::*; pub use types::Key; @@ -46,7 +49,7 @@ pub mod weak_bounded_vec; mod transaction_level_tracker { const TRANSACTION_LEVEL_KEY: &'static [u8] = b":transaction_level:"; - const TRANSACTIONAL_LIMIT: u32 = 10; + const TRANSACTIONAL_LIMIT: u32 = 255; type Layer = u32; pub fn get_transaction_level() -> Layer { @@ -122,9 +125,8 @@ where use sp_io::storage::{commit_transaction, rollback_transaction, start_transaction}; use TransactionOutcome::*; - let _guard = transaction_level_tracker::inc_transaction_level().map_err(|()| - DispatchError::TransactionalLimit.into() - )?; + let _guard = transaction_level_tracker::inc_transaction_level() + .map_err(|()| DispatchError::TransactionalLimit.into())?; start_transaction(); @@ -1439,14 +1441,14 @@ pub fn storage_prefix(pallet_name: &[u8], storage_name: &[u8]) -> [u8; 32] { #[cfg(test)] mod test { use super::*; - use crate::{assert_ok, assert_noop, hash::Identity, Twox128}; + use crate::{assert_noop, assert_ok, hash::Identity, Twox128}; use bounded_vec::BoundedVec; use frame_support::traits::ConstU32; use generator::StorageValue as _; use sp_core::hashing::twox_128; use sp_io::TestExternalities; - use weak_bounded_vec::WeakBoundedVec; use sp_runtime::DispatchResult; + use weak_bounded_vec::WeakBoundedVec; #[test] fn prefixed_map_works() { @@ -1557,24 +1559,67 @@ mod test { } #[test] - fn is_transaction_should_return_false() { + fn is_transactional_should_return_false() { TestExternalities::default().execute_with(|| { - assert!(!is_transactional()) + assert!(!is_transactional()); }); } #[test] - fn require_transaction_should_not_panic_in_with_transaction() { + fn is_transactional_should_not_error_in_with_transaction() { TestExternalities::default().execute_with(|| { assert_ok!(with_transaction(|| -> TransactionOutcome { assert!(is_transactional()); TransactionOutcome::Commit(Ok(())) })); - assert_noop!(with_transaction(|| -> TransactionOutcome { - assert!(is_transactional()); - TransactionOutcome::Rollback(Err("revert".into())) - }), "revert"); + assert_noop!( + with_transaction(|| -> TransactionOutcome { + assert!(is_transactional()); + TransactionOutcome::Rollback(Err("revert".into())) + }), + "revert" + ); + }); + } + + fn recursive_transactional(num: u32) -> DispatchResult { + if num == 0 { + return Ok(()) + } + + with_transaction(|| -> TransactionOutcome { + let res = recursive_transactional(num - 1); + TransactionOutcome::Commit(res) + }) + } + + #[test] + fn transaction_limit_should_work() { + TestExternalities::default().execute_with(|| { + assert_eq!(transaction_level_tracker::get_transaction_level(), 0); + + assert_ok!(with_transaction(|| -> TransactionOutcome { + assert_eq!(transaction_level_tracker::get_transaction_level(), 1); + TransactionOutcome::Commit(Ok(())) + })); + + assert_ok!(with_transaction(|| -> TransactionOutcome { + assert_eq!(transaction_level_tracker::get_transaction_level(), 1); + let res = with_transaction(|| -> TransactionOutcome { + assert_eq!(transaction_level_tracker::get_transaction_level(), 2); + TransactionOutcome::Commit(Ok(())) + }); + TransactionOutcome::Commit(res) + })); + + assert_ok!(recursive_transactional(255)); + assert_noop!( + recursive_transactional(256), + sp_runtime::DispatchError::TransactionalLimit + ); + + assert_eq!(transaction_level_tracker::get_transaction_level(), 0); }); } diff --git a/frame/support/test/tests/storage_transaction.rs b/frame/support/test/tests/storage_transaction.rs index 34759d2a54f90..848a91a7f5a86 100644 --- a/frame/support/test/tests/storage_transaction.rs +++ b/frame/support/test/tests/storage_transaction.rs @@ -22,8 +22,8 @@ use frame_support::{ transactional, StorageMap, StorageValue, }; use sp_io::TestExternalities; -use sp_std::result; use sp_runtime::TransactionOutcome; +use sp_std::result; pub trait Config: frame_support_test::Config {} @@ -87,21 +87,26 @@ fn storage_transaction_basic_rollback() { assert_eq!(Value::get(), 0); assert_eq!(Map::get("val0"), 0); - assert_noop!(with_transaction(|| -> TransactionOutcome { - Value::set(99); - Map::insert("val0", 99); - assert_eq!(Value::get(), 99); - assert_eq!(Map::get("val0"), 99); - Rollback(Err("revert".into())) - }), "revert"); - - assert_storage_noop!(assert_ok!(with_transaction(|| -> TransactionOutcome { - Value::set(99); - Map::insert("val0", 99); - assert_eq!(Value::get(), 99); - assert_eq!(Map::get("val0"), 99); - Rollback(Ok(())) - }))); + assert_noop!( + with_transaction(|| -> TransactionOutcome { + Value::set(99); + Map::insert("val0", 99); + assert_eq!(Value::get(), 99); + assert_eq!(Map::get("val0"), 99); + Rollback(Err("revert".into())) + }), + "revert" + ); + + assert_storage_noop!(assert_ok!(with_transaction( + || -> TransactionOutcome { + Value::set(99); + Map::insert("val0", 99); + assert_eq!(Value::get(), 99); + assert_eq!(Map::get("val0"), 99); + Rollback(Ok(())) + } + ))); assert_eq!(Value::get(), 0); assert_eq!(Map::get("val0"), 0); @@ -119,19 +124,22 @@ fn storage_transaction_rollback_then_commit() { Map::insert("val1", 2); Map::insert("val2", 2); - assert_noop!(with_transaction(|| -> TransactionOutcome { - Value::set(3); - Map::insert("val1", 3); - Map::insert("val2", 3); - Map::insert("val3", 3); + assert_noop!( + with_transaction(|| -> TransactionOutcome { + Value::set(3); + Map::insert("val1", 3); + Map::insert("val2", 3); + Map::insert("val3", 3); - assert_eq!(Value::get(), 3); - assert_eq!(Map::get("val1"), 3); - assert_eq!(Map::get("val2"), 3); - assert_eq!(Map::get("val3"), 3); + assert_eq!(Value::get(), 3); + assert_eq!(Map::get("val1"), 3); + assert_eq!(Map::get("val2"), 3); + assert_eq!(Map::get("val3"), 3); - Rollback(Err("revert".into())) - }), "revert"); + Rollback(Err("revert".into())) + }), + "revert" + ); assert_eq!(Value::get(), 2); assert_eq!(Map::get("val1"), 2); @@ -154,32 +162,35 @@ fn storage_transaction_commit_then_rollback() { Value::set(1); Map::insert("val1", 1); - assert_noop!(with_transaction(|| -> TransactionOutcome { - Value::set(2); - Map::insert("val1", 2); - Map::insert("val2", 2); + assert_noop!( + with_transaction(|| -> TransactionOutcome { + Value::set(2); + Map::insert("val1", 2); + Map::insert("val2", 2); + + assert_ok!(with_transaction(|| -> TransactionOutcome { + Value::set(3); + Map::insert("val1", 3); + Map::insert("val2", 3); + Map::insert("val3", 3); + + assert_eq!(Value::get(), 3); + assert_eq!(Map::get("val1"), 3); + assert_eq!(Map::get("val2"), 3); + assert_eq!(Map::get("val3"), 3); - assert_ok!(with_transaction(|| -> TransactionOutcome { - Value::set(3); - Map::insert("val1", 3); - Map::insert("val2", 3); - Map::insert("val3", 3); + Commit(Ok(())) + })); assert_eq!(Value::get(), 3); assert_eq!(Map::get("val1"), 3); assert_eq!(Map::get("val2"), 3); assert_eq!(Map::get("val3"), 3); - Commit(Ok(())) - })); - - assert_eq!(Value::get(), 3); - assert_eq!(Map::get("val1"), 3); - assert_eq!(Map::get("val2"), 3); - assert_eq!(Map::get("val3"), 3); - - Rollback(Err("revert".into())) - }), "revert"); + Rollback(Err("revert".into())) + }), + "revert" + ); assert_eq!(Value::get(), 1); assert_eq!(Map::get("val1"), 1); diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index f720528bc2cde..5aedfefb8e95c 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -512,7 +512,8 @@ pub enum DispatchError { Token(TokenError), /// An arithmetic error. Arithmetic(ArithmeticError), - /// The number of transactional layers has been reached, or we are not in a transactional layer. + /// The number of transactional layers has been reached, or we are not in a transactional + /// layer. TransactionalLimit, } From 70350eb1b4bbd5115409677836ad93319bc6571c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 24 Mar 2022 19:20:18 -0400 Subject: [PATCH 06/14] maybe fix contracts cc @athei --- frame/contracts/src/exec.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 455665687d973..8fca60087b76c 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -767,13 +767,13 @@ where // All changes performed by the contract are executed under a storage transaction. // This allows for roll back on error. Changes to the cached contract_info are // comitted or rolled back when popping the frame. - let (success, output) = with_transaction(|| { + let (success, output) = with_transaction(|| -> TransactionOutcome> { let output = do_transaction(); match &output { - Ok(result) if !result.did_revert() => TransactionOutcome::Commit((true, output)), - _ => TransactionOutcome::Rollback((false, output)), + Ok(result) if !result.did_revert() => TransactionOutcome::Commit(Ok((true, output))), + _ => TransactionOutcome::Rollback(Ok((false, output))), } - }); + })?; self.pop_frame(success); output } From 41f8622aba479f747a26b079ecd4681c53d1646e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 24 Mar 2022 19:24:21 -0400 Subject: [PATCH 07/14] fmt --- frame/contracts/src/exec.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 8fca60087b76c..dec3132ef48fb 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -767,13 +767,15 @@ where // All changes performed by the contract are executed under a storage transaction. // This allows for roll back on error. Changes to the cached contract_info are // comitted or rolled back when popping the frame. - let (success, output) = with_transaction(|| -> TransactionOutcome> { - let output = do_transaction(); - match &output { - Ok(result) if !result.did_revert() => TransactionOutcome::Commit(Ok((true, output))), - _ => TransactionOutcome::Rollback(Ok((false, output))), - } - })?; + let (success, output) = + with_transaction(|| -> TransactionOutcome> { + let output = do_transaction(); + match &output { + Ok(result) if !result.did_revert() => + TransactionOutcome::Commit(Ok((true, output))), + _ => TransactionOutcome::Rollback(Ok((false, output))), + } + })?; self.pop_frame(success); output } From 224bd6468567720fcf0d324105a697246074658e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 28 Mar 2022 22:34:08 -0400 Subject: [PATCH 08/14] fix contract logic --- frame/contracts/src/exec.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index dec3132ef48fb..79a66d7e7c739 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -766,8 +766,11 @@ where // All changes performed by the contract are executed under a storage transaction. // This allows for roll back on error. Changes to the cached contract_info are - // comitted or rolled back when popping the frame. - let (success, output) = + // committed or rolled back when popping the frame. + // + // `with_transactional` may return an error caused by a limit in the + // transactional storage depth. + let transaction_outcome = with_transaction(|| -> TransactionOutcome> { let output = do_transaction(); match &output { @@ -775,7 +778,15 @@ where TransactionOutcome::Commit(Ok((true, output))), _ => TransactionOutcome::Rollback(Ok((false, output))), } - })?; + }); + + let (success, output) = match transaction_outcome { + // `with_transactional` executed successfully, and we have the expected output. + Ok((success, output)) => (success, output), + // `with_transactional` returned an error, and we propagate that error and note no state + // has changed. + Err(error) => (false, Err(ExecError { error, origin: ErrorOrigin::Callee })), + }; self.pop_frame(success); output } From 7a6b5d87f51a0428c099fcb55dfaa8a47f789d0e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 29 Mar 2022 11:14:27 -0400 Subject: [PATCH 09/14] Update frame/contracts/src/exec.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Alexander Theißen --- frame/contracts/src/exec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 79a66d7e7c739..3d00950a6609d 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -785,7 +785,7 @@ where Ok((success, output)) => (success, output), // `with_transactional` returned an error, and we propagate that error and note no state // has changed. - Err(error) => (false, Err(ExecError { error, origin: ErrorOrigin::Callee })), + Err(error) => (false, Err(error.into()), }; self.pop_frame(success); output From 73d2f4c2b831aadb018a3b59deb6c05ced91e8f2 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 30 Mar 2022 13:54:47 -0400 Subject: [PATCH 10/14] Update exec.rs --- frame/contracts/src/exec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 3d00950a6609d..ba613fd72f39a 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -785,7 +785,7 @@ where Ok((success, output)) => (success, output), // `with_transactional` returned an error, and we propagate that error and note no state // has changed. - Err(error) => (false, Err(error.into()), + Err(error) => (false, Err(error.into())), }; self.pop_frame(success); output From ef21196914cd125df70b5a8e74f49b26ac3b1ae5 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 1 Apr 2022 09:39:18 -0400 Subject: [PATCH 11/14] add unchecked and custom errors --- frame/support/procedural/src/transactional.rs | 2 +- frame/support/src/storage/mod.rs | 49 ++++++++++++++++--- primitives/runtime/src/lib.rs | 34 +++++++++++-- 3 files changed, 74 insertions(+), 11 deletions(-) diff --git a/frame/support/procedural/src/transactional.rs b/frame/support/procedural/src/transactional.rs index b3984cac031f5..ba75fbc9737aa 100644 --- a/frame/support/procedural/src/transactional.rs +++ b/frame/support/procedural/src/transactional.rs @@ -50,7 +50,7 @@ pub fn require_transactional(_attr: TokenStream, input: TokenStream) -> Result Layer { crate::storage::unhashed::get_or_default::(TRANSACTION_LEVEL_KEY) @@ -69,7 +69,7 @@ mod transaction_level_tracker { /// Returns a guard that when dropped decrements the transaction level automatically. pub fn inc_transaction_level() -> Result { let existing_levels = get_transaction_level(); - if existing_levels >= TRANSACTIONAL_LIMIT || existing_levels == Layer::MAX { + if existing_levels >= TRANSACTIONAL_LIMIT { return Err(()) } // Cannot overflow because of check above. @@ -115,7 +115,8 @@ pub fn is_transactional() -> bool { /// All changes to storage performed by the supplied function are discarded if the returned /// outcome is `TransactionOutcome::Rollback`. /// -/// Transactions can be nested up to 10 times; more than that will result in an error. +/// Transactions can be nested up to `TRANSACTIONAL_LIMIT` times; more than that will result in an +/// error. /// /// Commits happen to the parent transaction. pub fn with_transaction(f: impl FnOnce() -> TransactionOutcome>) -> Result @@ -126,7 +127,41 @@ where use TransactionOutcome::*; let _guard = transaction_level_tracker::inc_transaction_level() - .map_err(|()| DispatchError::TransactionalLimit.into())?; + .map_err(|()| TransactionalError::LimitReached.into())?; + + start_transaction(); + + match f() { + Commit(res) => { + commit_transaction(); + res + }, + Rollback(res) => { + rollback_transaction(); + res + }, + } +} + +/// Same as `with_transaction` but without a limit check on nested transactional layers. +/// +/// This is mostly for backwards compatibility before there was a transactional layer limit. +/// It is recommended to only use `with_transaction` to avoid users from generating too many +/// transactional layers. +pub fn with_transaction_unchecked(f: impl FnOnce() -> TransactionOutcome) -> R { + use sp_io::storage::{commit_transaction, rollback_transaction, start_transaction}; + use TransactionOutcome::*; + + let maybe_guard = transaction_level_tracker::inc_transaction_level(); + + if maybe_guard.is_err() { + log::warn!( + "The transactional layer limit has been reached, and new transactional layers are being + spawned with `with_transaction_unchecked`. This could be caused by someone trying to + attack your chain, and you should investigate usage of `with_transaction_unchecked` and + potentially migrate to `with_transaction`, which enforces a transactional limit.", + ); + } start_transaction(); @@ -1616,7 +1651,7 @@ mod test { assert_ok!(recursive_transactional(255)); assert_noop!( recursive_transactional(256), - sp_runtime::DispatchError::TransactionalLimit + sp_runtime::TransactionalError::LimitReached ); assert_eq!(transaction_level_tracker::get_transaction_level(), 0); diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index 5aedfefb8e95c..7bb89f2eba055 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -486,6 +486,31 @@ impl PartialEq for ModuleError { } } +/// Errors related to transactional storage layers. +#[derive(Eq, PartialEq, Clone, Copy, Encode, Decode, Debug, TypeInfo)] +#[cfg_attr(feature = "std", derive(Serialize, Deserialize))] +pub enum TransactionalError { + /// Too many transactional layers have been spawned. + LimitReached, + /// A transactional layer was expected, but does not exist. + NoLayer, +} + +impl From for &'static str { + fn from(e: TransactionalError) -> &'static str { + match e { + TransactionalError::LimitReached => "Too many transactional layers have been spawned", + TransactionalError::NoLayer => "A transactional layer was expected, but does not exist", + } + } +} + +impl From for DispatchError { + fn from(e: TransactionalError) -> DispatchError { + Self::Transactional(e) + } +} + /// Reason why a dispatch call failed. #[derive(Eq, Clone, Copy, Encode, Decode, Debug, TypeInfo, PartialEq)] #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] @@ -514,7 +539,7 @@ pub enum DispatchError { Arithmetic(ArithmeticError), /// The number of transactional layers has been reached, or we are not in a transactional /// layer. - TransactionalLimit, + Transactional(TransactionalError), } /// Result of a `Dispatchable` which contains the `DispatchResult` and additional information about @@ -650,7 +675,7 @@ impl From for &'static str { DispatchError::TooManyConsumers => "Too many consumers", DispatchError::Token(e) => e.into(), DispatchError::Arithmetic(e) => e.into(), - DispatchError::TransactionalLimit => "Too many transactional layers", + DispatchError::Transactional(e) => e.into(), } } } @@ -689,7 +714,10 @@ impl traits::Printable for DispatchError { "Arithmetic error: ".print(); <&'static str>::from(*e).print(); }, - Self::TransactionalLimit => "Too many transactional layers".print(), + Self::Transactional(e) => { + "Transactional error: ".print(); + <&'static str>::from(*e).print(); + } } } } From ddcbcc43a279d4875d927db4297b3b8bc77f9b41 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 1 Apr 2022 09:46:01 -0400 Subject: [PATCH 12/14] Update lib.rs --- primitives/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index 7bb89f2eba055..c09db5124cc1f 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -717,7 +717,7 @@ impl traits::Printable for DispatchError { Self::Transactional(e) => { "Transactional error: ".print(); <&'static str>::from(*e).print(); - } + }, } } } From f26f80e75d4e441e146f1bd5adf2075ce8c28713 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 1 Apr 2022 10:18:43 -0400 Subject: [PATCH 13/14] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Alexander Theißen --- frame/support/src/storage/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 4c5e1593518a5..3c5f27e226c7f 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -143,10 +143,10 @@ where } } -/// Same as `with_transaction` but without a limit check on nested transactional layers. +/// Same as [`with_transaction`] but without a limit check on nested transactional layers. /// /// This is mostly for backwards compatibility before there was a transactional layer limit. -/// It is recommended to only use `with_transaction` to avoid users from generating too many +/// It is recommended to only use [`with_transaction`] to avoid users from generating too many /// transactional layers. pub fn with_transaction_unchecked(f: impl FnOnce() -> TransactionOutcome) -> R { use sp_io::storage::{commit_transaction, rollback_transaction, start_transaction}; From a69b8eb4a28f365a4a4b2fc295a693ec4d3d3cd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Thei=C3=9Fen?= Date: Sat, 2 Apr 2022 15:02:26 +0200 Subject: [PATCH 14/14] Replace storage access by atomics --- frame/support/src/storage/mod.rs | 54 +++++++++++++++----------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/frame/support/src/storage/mod.rs b/frame/support/src/storage/mod.rs index 3c5f27e226c7f..c7a1b7a35066a 100644 --- a/frame/support/src/storage/mod.rs +++ b/frame/support/src/storage/mod.rs @@ -48,48 +48,46 @@ pub mod unhashed; pub mod weak_bounded_vec; mod transaction_level_tracker { + use core::sync::atomic::{AtomicU32, Ordering}; + type Layer = u32; - const TRANSACTION_LEVEL_KEY: &'static [u8] = b":transaction_level:"; + static NUM_LEVELS: AtomicU32 = AtomicU32::new(0); const TRANSACTIONAL_LIMIT: Layer = 255; pub fn get_transaction_level() -> Layer { - crate::storage::unhashed::get_or_default::(TRANSACTION_LEVEL_KEY) - } - - fn set_transaction_level(level: &Layer) { - crate::storage::unhashed::put::(TRANSACTION_LEVEL_KEY, level); - } - - fn kill_transaction_level() { - crate::storage::unhashed::kill(TRANSACTION_LEVEL_KEY); + NUM_LEVELS.load(Ordering::SeqCst) } /// Increments the transaction level. Returns an error if levels go past the limit. /// /// Returns a guard that when dropped decrements the transaction level automatically. pub fn inc_transaction_level() -> Result { - let existing_levels = get_transaction_level(); - if existing_levels >= TRANSACTIONAL_LIMIT { - return Err(()) - } - // Cannot overflow because of check above. - set_transaction_level(&(existing_levels + 1)); + NUM_LEVELS + .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |existing_levels| { + if existing_levels >= TRANSACTIONAL_LIMIT { + return None + } + // Cannot overflow because of check above. + Some(existing_levels + 1) + }) + .map_err(|_| ())?; Ok(StorageLayerGuard) } fn dec_transaction_level() { - let existing_levels = get_transaction_level(); - if existing_levels == 0 { - log::warn!( - "We are underflowing with calculating transactional levels. Not great, but let's not panic...", - ); - } else if existing_levels == 1 { - // Don't leave any trace of this storage item. - kill_transaction_level(); - } else { - // Cannot underflow because of checks above. - set_transaction_level(&(existing_levels - 1)); - } + NUM_LEVELS + .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |existing_levels| { + if existing_levels == 0 { + log::warn!( + "We are underflowing with calculating transactional levels. Not great, but let's not panic...", + ); + None + } else { + // Cannot underflow because of checks above. + Some(existing_levels - 1) + } + }) + .ok(); } pub fn is_transactional() -> bool {