From aa5f68a82759388a489b27fd8cbdde0b041e4a83 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <bkchr@users.noreply.github.com>
Date: Wed, 10 Aug 2022 22:27:01 +0200
Subject: [PATCH] transactional: Wrap `pallet::calls` directly in storage
layers (#11927)
* transactional: Wrap `pallet::calls` directly in storage layers
Before this pr we only wrapped `pallet::calls` into storage layers when executing the calls with
`dispatch`. This pr is solving that by wrapping each call function inside a storage layer.
* Teach `BasicExternalities` transactions support
* Fix crates
* FMT
* Fix benchmarking tests
* Use correct span
* Support old decl macros
* Fix test
* Apply suggestions from code review
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
* Update frame/state-trie-migration/src/lib.rs
* Update frame/state-trie-migration/src/lib.rs
* Update frame/state-trie-migration/src/lib.rs
* Feedback
* Apply suggestions from code review
Co-authored-by: cheme <emericchevalier.pro@gmail.com>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: cheme <emericchevalier.pro@gmail.com>
---
.../frame/state-trie-migration/src/lib.rs | 128 ++++++------
.../procedural/src/pallet/expand/call.rs | 26 ++-
.../procedural/src/pallet/parse/call.rs | 20 +-
substrate/frame/support/src/dispatch.rs | 14 +-
.../support/test/tests/construct_runtime.rs | 178 ++++++++--------
.../support/test/tests/storage_layers.rs | 2 +
.../frame/transaction-storage/src/lib.rs | 4 +-
.../primitives/state-machine/src/basic.rs | 192 +++++++-----------
.../src/overlayed_changes/changeset.rs | 31 ++-
.../src/overlayed_changes/mod.rs | 15 ++
10 files changed, 316 insertions(+), 294 deletions(-)
diff --git a/substrate/frame/state-trie-migration/src/lib.rs b/substrate/frame/state-trie-migration/src/lib.rs
index 94f6c1f223b..353bc93e5ab 100644
--- a/substrate/frame/state-trie-migration/src/lib.rs
+++ b/substrate/frame/state-trie-migration/src/lib.rs
@@ -235,7 +235,10 @@ pub mod pallet {
/// reading a key, we simply cannot know how many bytes it is. In other words, this should
/// not be used in any environment where resources are strictly bounded (e.g. a parachain),
/// but it is acceptable otherwise (relay chain, offchain workers).
- pub fn migrate_until_exhaustion(&mut self, limits: MigrationLimits) -> DispatchResult {
+ pub fn migrate_until_exhaustion(
+ &mut self,
+ limits: MigrationLimits,
+ ) -> Result<(), Error<T>> {
log!(debug, "running migrations on top of {:?} until {:?}", self, limits);
if limits.item.is_zero() || limits.size.is_zero() {
@@ -262,7 +265,7 @@ pub mod pallet {
/// Migrate AT MOST ONE KEY. This can be either a top or a child key.
///
/// This function is *the* core of this entire pallet.
- fn migrate_tick(&mut self) -> DispatchResult {
+ fn migrate_tick(&mut self) -> Result<(), Error<T>> {
match (&self.progress_top, &self.progress_child) {
(Progress::ToStart, _) => self.migrate_top(),
(Progress::LastKey(_), Progress::LastKey(_)) => {
@@ -301,7 +304,7 @@ pub mod pallet {
/// Migrate the current child key, setting it to its new value, if one exists.
///
/// It updates the dynamic counters.
- fn migrate_child(&mut self) -> DispatchResult {
+ fn migrate_child(&mut self) -> Result<(), Error<T>> {
use sp_io::default_child_storage as child_io;
let (maybe_current_child, child_root) = match (&self.progress_child, &self.progress_top)
{
@@ -350,7 +353,7 @@ pub mod pallet {
/// Migrate the current top key, setting it to its new value, if one exists.
///
/// It updates the dynamic counters.
- fn migrate_top(&mut self) -> DispatchResult {
+ fn migrate_top(&mut self) -> Result<(), Error<T>> {
let maybe_current_top = match &self.progress_top {
Progress::LastKey(last_top) => {
let maybe_top: Option<BoundedVec<u8, T::MaxKeyLen>> =
@@ -431,7 +434,7 @@ pub mod pallet {
/// The auto migration task finished.
AutoMigrationFinished,
/// Migration got halted due to an error or miss-configuration.
- Halted,
+ Halted { error: Error<T> },
}
/// The outer Pallet struct.
@@ -516,8 +519,9 @@ pub mod pallet {
pub type SignedMigrationMaxLimits<T> = StorageValue<_, MigrationLimits, OptionQuery>;
#[pallet::error]
+ #[derive(Clone, PartialEq)]
pub enum Error<T> {
- /// max signed limits not respected.
+ /// Max signed limits not respected.
MaxSignedLimits,
/// A key was longer than the configured maximum.
///
@@ -529,12 +533,12 @@ pub mod pallet {
KeyTooLong,
/// submitter does not have enough funds.
NotEnoughFunds,
- /// bad witness data provided.
+ /// Bad witness data provided.
BadWitness,
- /// upper bound of size is exceeded,
- SizeUpperBoundExceeded,
/// Signed migration is not allowed because the maximum limit is not set yet.
SignedMigrationNotAllowed,
+ /// Bad child root provided.
+ BadChildRoot,
}
#[pallet::call]
@@ -617,7 +621,7 @@ pub mod pallet {
let (_imbalance, _remainder) = T::Currency::slash(&who, deposit);
Self::deposit_event(Event::<T>::Slashed { who, amount: deposit });
debug_assert!(_remainder.is_zero());
- return Err(Error::<T>::SizeUpperBoundExceeded.into())
+ return Ok(().into())
}
Self::deposit_event(Event::<T>::Migrated {
@@ -634,13 +638,10 @@ pub mod pallet {
MigrationProcess::<T>::put(task);
let post_info = PostDispatchInfo { actual_weight, pays_fee: Pays::No };
- match migration {
- Ok(_) => Ok(post_info),
- Err(error) => {
- Self::halt(&error);
- Err(DispatchErrorWithPostInfo { post_info, error })
- },
+ if let Err(error) = migration {
+ Self::halt(error);
}
+ Ok(post_info)
}
/// Migrate the list of top keys by iterating each of them one by one.
@@ -679,7 +680,7 @@ pub mod pallet {
let (_imbalance, _remainder) = T::Currency::slash(&who, deposit);
Self::deposit_event(Event::<T>::Slashed { who, amount: deposit });
debug_assert!(_remainder.is_zero());
- Err("wrong witness data".into())
+ Ok(().into())
} else {
Self::deposit_event(Event::<T>::Migrated {
top: keys.len() as u32,
@@ -741,12 +742,9 @@ pub mod pallet {
let (_imbalance, _remainder) = T::Currency::slash(&who, deposit);
debug_assert!(_remainder.is_zero());
Self::deposit_event(Event::<T>::Slashed { who, amount: deposit });
- Err(DispatchErrorWithPostInfo {
- error: "bad witness".into(),
- post_info: PostDispatchInfo {
- actual_weight: Some(T::WeightInfo::migrate_custom_child_fail()),
- pays_fee: Pays::Yes,
- },
+ Ok(PostDispatchInfo {
+ actual_weight: Some(T::WeightInfo::migrate_custom_child_fail()),
+ pays_fee: Pays::Yes,
})
} else {
Self::deposit_event(Event::<T>::Migrated {
@@ -806,7 +804,7 @@ pub mod pallet {
if let Some(limits) = Self::auto_limits() {
let mut task = Self::migration_process();
if let Err(e) = task.migrate_until_exhaustion(limits) {
- Self::halt(&e);
+ Self::halt(e);
}
let weight = Self::dynamic_weight(task.dyn_total_items(), task.dyn_size);
@@ -849,10 +847,10 @@ pub mod pallet {
}
/// Put a stop to all ongoing migrations and logs an error.
- fn halt<E: sp_std::fmt::Debug + ?Sized>(msg: &E) {
- log!(error, "migration halted due to: {:?}", msg);
+ fn halt(error: Error<T>) {
+ log!(error, "migration halted due to: {:?}", error);
AutoLimits::<T>::kill();
- Self::deposit_event(Event::<T>::Halted);
+ Self::deposit_event(Event::<T>::Halted { error });
}
/// Convert a child root key, aka. "Child-bearing top key" into the proper format.
@@ -871,7 +869,7 @@ pub mod pallet {
fn transform_child_key_or_halt(root: &Vec<u8>) -> &[u8] {
let key = Self::transform_child_key(root);
if key.is_none() {
- Self::halt("bad child root key");
+ Self::halt(Error::<T>::BadChildRoot);
}
key.unwrap_or_default()
}
@@ -961,8 +959,16 @@ mod benchmarks {
frame_system::RawOrigin::Signed(caller.clone()).into(),
vec![b"foo".to_vec()],
1,
- ).is_err()
- )
+ ).is_ok()
+ );
+
+ frame_system::Pallet::<T>::assert_last_event(
+ <T as Config>::Event::from(crate::Event::Slashed {
+ who: caller.clone(),
+ amount: T::SignedDepositBase::get()
+ .saturating_add(T::SignedDepositPerItem::get().saturating_mul(1u32.into())),
+ }).into(),
+ );
}
verify {
assert_eq!(StateTrieMigration::<T>::migration_process(), Default::default());
@@ -1005,7 +1011,7 @@ mod benchmarks {
StateTrieMigration::<T>::childify("top"),
vec![b"foo".to_vec()],
1,
- ).is_err()
+ ).is_ok()
)
}
verify {
@@ -1285,18 +1291,16 @@ mod test {
SignedMigrationMaxLimits::<Test>::put(MigrationLimits { size: 1 << 20, item: 50 });
// fails if the top key is too long.
- frame_support::assert_err_with_weight!(
- StateTrieMigration::continue_migrate(
- Origin::signed(1),
- MigrationLimits { item: 50, size: 1 << 20 },
- Bounded::max_value(),
- MigrationProcess::<Test>::get()
- ),
- Error::<Test>::KeyTooLong,
- Some(2000000),
- );
+ frame_support::assert_ok!(StateTrieMigration::continue_migrate(
+ Origin::signed(1),
+ MigrationLimits { item: 50, size: 1 << 20 },
+ Bounded::max_value(),
+ MigrationProcess::<Test>::get()
+ ),);
// The auto migration halted.
- System::assert_last_event(crate::Event::Halted {}.into());
+ System::assert_last_event(
+ crate::Event::Halted { error: Error::<Test>::KeyTooLong }.into(),
+ );
// Limits are killed.
assert!(AutoLimits::<Test>::get().is_none());
@@ -1322,18 +1326,16 @@ mod test {
SignedMigrationMaxLimits::<Test>::put(MigrationLimits { size: 1 << 20, item: 50 });
// fails if the top key is too long.
- frame_support::assert_err_with_weight!(
- StateTrieMigration::continue_migrate(
- Origin::signed(1),
- MigrationLimits { item: 50, size: 1 << 20 },
- Bounded::max_value(),
- MigrationProcess::<Test>::get()
- ),
- Error::<Test>::KeyTooLong,
- Some(2000000),
- );
+ frame_support::assert_ok!(StateTrieMigration::continue_migrate(
+ Origin::signed(1),
+ MigrationLimits { item: 50, size: 1 << 20 },
+ Bounded::max_value(),
+ MigrationProcess::<Test>::get()
+ ));
// The auto migration halted.
- System::assert_last_event(crate::Event::Halted {}.into());
+ System::assert_last_event(
+ crate::Event::Halted { error: Error::<Test>::KeyTooLong }.into(),
+ );
// Limits are killed.
assert!(AutoLimits::<Test>::get().is_none());
@@ -1484,7 +1486,7 @@ mod test {
..Default::default()
}
),
- Error::<Test>::BadWitness
+ Error::<Test>::BadWitness,
);
// migrate all keys in a series of submissions
@@ -1547,14 +1549,11 @@ mod test {
assert_eq!(Balances::free_balance(&1), 1000);
// note that we don't expect this to be a noop -- we do slash.
- frame_support::assert_err!(
- StateTrieMigration::migrate_custom_top(
- Origin::signed(1),
- vec![b"key1".to_vec(), b"key2".to_vec(), b"key3".to_vec()],
- correct_witness - 1,
- ),
- "wrong witness data"
- );
+ frame_support::assert_ok!(StateTrieMigration::migrate_custom_top(
+ Origin::signed(1),
+ vec![b"key1".to_vec(), b"key2".to_vec(), b"key3".to_vec()],
+ correct_witness - 1,
+ ),);
// no funds should remain reserved.
assert_eq!(Balances::reserved_balance(&1), 0);
@@ -1584,13 +1583,12 @@ mod test {
assert_eq!(Balances::free_balance(&1), 1000);
// note that we don't expect this to be a noop -- we do slash.
- assert!(StateTrieMigration::migrate_custom_child(
+ frame_support::assert_ok!(StateTrieMigration::migrate_custom_child(
Origin::signed(1),
StateTrieMigration::childify("chk1"),
vec![b"key1".to_vec(), b"key2".to_vec()],
999999, // wrong witness
- )
- .is_err());
+ ));
// no funds should remain reserved.
assert_eq!(Balances::reserved_balance(&1), 0);
diff --git a/substrate/frame/support/procedural/src/pallet/expand/call.rs b/substrate/frame/support/procedural/src/pallet/expand/call.rs
index fe7589a8275..a9468451ad1 100644
--- a/substrate/frame/support/procedural/src/pallet/expand/call.rs
+++ b/substrate/frame/support/procedural/src/pallet/expand/call.rs
@@ -140,6 +140,24 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream {
let capture_docs = if cfg!(feature = "no-metadata-docs") { "never" } else { "always" };
+ // Wrap all calls inside of storage layers
+ if let Some(syn::Item::Impl(item_impl)) = def
+ .call
+ .as_ref()
+ .map(|c| &mut def.item.content.as_mut().expect("Checked by def parser").1[c.index])
+ {
+ item_impl.items.iter_mut().for_each(|i| {
+ if let syn::ImplItem::Method(method) = i {
+ let block = &method.block;
+ method.block = syn::parse_quote! {{
+ // We execute all dispatchable in a new storage layer, allowing them
+ // to return an error at any point, and undoing any storage changes.
+ #frame_support::storage::with_storage_layer(|| #block)
+ }};
+ }
+ });
+ }
+
quote::quote_spanned!(span =>
#[doc(hidden)]
pub mod __substrate_call_check {
@@ -267,12 +285,8 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream {
#frame_support::sp_tracing::enter_span!(
#frame_support::sp_tracing::trace_span!(stringify!(#fn_name))
);
- // We execute all dispatchable in a new storage layer, allowing them
- // to return an error at any point, and undoing any storage changes.
- #frame_support::storage::with_storage_layer(|| {
- <#pallet_ident<#type_use_gen>>::#fn_name(origin, #( #args_name, )* )
- .map(Into::into).map_err(Into::into)
- })
+ <#pallet_ident<#type_use_gen>>::#fn_name(origin, #( #args_name, )* )
+ .map(Into::into).map_err(Into::into)
},
)*
Self::__Ignore(_, _) => {
diff --git a/substrate/frame/support/procedural/src/pallet/parse/call.rs b/substrate/frame/support/procedural/src/pallet/parse/call.rs
index d8a81d699b8..336e08c3d39 100644
--- a/substrate/frame/support/procedural/src/pallet/parse/call.rs
+++ b/substrate/frame/support/procedural/src/pallet/parse/call.rs
@@ -48,8 +48,8 @@ pub struct CallDef {
pub docs: Vec<syn::Lit>,
}
-#[derive(Clone)]
/// Definition of dispatchable typically: `#[weight...] fn foo(origin .., param1: ...) -> ..`
+#[derive(Clone)]
pub struct CallVariantDef {
/// Function name.
pub name: syn::Ident,
@@ -143,18 +143,18 @@ impl CallDef {
index: usize,
item: &mut syn::Item,
) -> syn::Result<Self> {
- let item = if let syn::Item::Impl(item) = item {
+ let item_impl = if let syn::Item::Impl(item) = item {
item
} else {
return Err(syn::Error::new(item.span(), "Invalid pallet::call, expected item impl"))
};
let instances = vec![
- helper::check_impl_gen(&item.generics, item.impl_token.span())?,
- helper::check_pallet_struct_usage(&item.self_ty)?,
+ helper::check_impl_gen(&item_impl.generics, item_impl.impl_token.span())?,
+ helper::check_pallet_struct_usage(&item_impl.self_ty)?,
];
- if let Some((_, _, for_)) = item.trait_ {
+ if let Some((_, _, for_)) = item_impl.trait_ {
let msg = "Invalid pallet::call, expected no trait ident as in \
`impl<..> Pallet<..> { .. }`";
return Err(syn::Error::new(for_.span(), msg))
@@ -163,8 +163,8 @@ impl CallDef {
let mut methods = vec![];
let mut indices = HashMap::new();
let mut last_index: Option<u8> = None;
- for impl_item in &mut item.items {
- if let syn::ImplItem::Method(method) = impl_item {
+ for item in &mut item_impl.items {
+ if let syn::ImplItem::Method(method) = item {
if !matches!(method.vis, syn::Visibility::Public(_)) {
let msg = "Invalid pallet::call, dispatchable function must be public: \
`pub fn`";
@@ -290,7 +290,7 @@ impl CallDef {
});
} else {
let msg = "Invalid pallet::call, only method accepted";
- return Err(syn::Error::new(impl_item.span(), msg))
+ return Err(syn::Error::new(item.span(), msg))
}
}
@@ -299,8 +299,8 @@ impl CallDef {
attr_span,
instances,
methods,
- where_clause: item.generics.where_clause.clone(),
- docs: get_doc_literals(&item.attrs),
+ where_clause: item_impl.generics.where_clause.clone(),
+ docs: get_doc_literals(&item_impl.attrs),
})
}
}
diff --git a/substrate/frame/support/src/dispatch.rs b/substrate/frame/support/src/dispatch.rs
index 2b05478e0b0..fb0b658cd93 100644
--- a/substrate/frame/support/src/dispatch.rs
+++ b/substrate/frame/support/src/dispatch.rs
@@ -1787,9 +1787,11 @@ macro_rules! decl_module {
$vis fn $name(
$origin: $origin_ty $(, $param: $param_ty )*
) -> $crate::dispatch::DispatchResult {
- $crate::sp_tracing::enter_span!($crate::sp_tracing::trace_span!(stringify!($name)));
- { $( $impl )* }
- Ok(())
+ $crate::storage::with_storage_layer(|| {
+ $crate::sp_tracing::enter_span!($crate::sp_tracing::trace_span!(stringify!($name)));
+ { $( $impl )* }
+ Ok(())
+ })
}
};
@@ -1805,8 +1807,10 @@ macro_rules! decl_module {
) => {
$(#[$fn_attr])*
$vis fn $name($origin: $origin_ty $(, $param: $param_ty )* ) -> $result {
- $crate::sp_tracing::enter_span!($crate::sp_tracing::trace_span!(stringify!($name)));
- $( $impl )*
+ $crate::storage::with_storage_layer(|| {
+ $crate::sp_tracing::enter_span!($crate::sp_tracing::trace_span!(stringify!($name)));
+ $( $impl )*
+ })
}
};
diff --git a/substrate/frame/support/test/tests/construct_runtime.rs b/substrate/frame/support/test/tests/construct_runtime.rs
index 63747a9d560..d388127f29a 100644
--- a/substrate/frame/support/test/tests/construct_runtime.rs
+++ b/substrate/frame/support/test/tests/construct_runtime.rs
@@ -282,94 +282,96 @@ pub type UncheckedExtrinsic = generic::UncheckedExtrinsic<u32, Call, Signature,
#[test]
fn check_modules_error_type() {
- assert_eq!(
- Module1_1::fail(system::Origin::<Runtime>::Root.into()),
- Err(DispatchError::Module(ModuleError {
- index: 31,
- error: [0; 4],
- message: Some("Something")
- })),
- );
- assert_eq!(
- Module2::fail(system::Origin::<Runtime>::Root.into()),
- Err(DispatchError::Module(ModuleError {
- index: 32,
- error: [0; 4],
- message: Some("Something")
- })),
- );
- assert_eq!(
- Module1_2::fail(system::Origin::<Runtime>::Root.into()),
- Err(DispatchError::Module(ModuleError {
- index: 33,
- error: [0; 4],
- message: Some("Something")
- })),
- );
- assert_eq!(
- NestedModule3::fail(system::Origin::<Runtime>::Root.into()),
- Err(DispatchError::Module(ModuleError {
- index: 34,
- error: [0; 4],
- message: Some("Something")
- })),
- );
- assert_eq!(
- Module1_3::fail(system::Origin::<Runtime>::Root.into()),
- Err(DispatchError::Module(ModuleError {
- index: 6,
- error: [0; 4],
- message: Some("Something")
- })),
- );
- assert_eq!(
- Module1_4::fail(system::Origin::<Runtime>::Root.into()),
- Err(DispatchError::Module(ModuleError {
- index: 3,
- error: [0; 4],
- message: Some("Something")
- })),
- );
- assert_eq!(
- Module1_5::fail(system::Origin::<Runtime>::Root.into()),
- Err(DispatchError::Module(ModuleError {
- index: 4,
- error: [0; 4],
- message: Some("Something")
- })),
- );
- assert_eq!(
- Module1_6::fail(system::Origin::<Runtime>::Root.into()),
- Err(DispatchError::Module(ModuleError {
- index: 1,
- error: [0; 4],
- message: Some("Something")
- })),
- );
- assert_eq!(
- Module1_7::fail(system::Origin::<Runtime>::Root.into()),
- Err(DispatchError::Module(ModuleError {
- index: 2,
- error: [0; 4],
- message: Some("Something")
- })),
- );
- assert_eq!(
- Module1_8::fail(system::Origin::<Runtime>::Root.into()),
- Err(DispatchError::Module(ModuleError {
- index: 12,
- error: [0; 4],
- message: Some("Something")
- })),
- );
- assert_eq!(
- Module1_9::fail(system::Origin::<Runtime>::Root.into()),
- Err(DispatchError::Module(ModuleError {
- index: 13,
- error: [0; 4],
- message: Some("Something")
- })),
- );
+ sp_io::TestExternalities::default().execute_with(|| {
+ assert_eq!(
+ Module1_1::fail(system::Origin::<Runtime>::Root.into()),
+ Err(DispatchError::Module(ModuleError {
+ index: 31,
+ error: [0; 4],
+ message: Some("Something")
+ })),
+ );
+ assert_eq!(
+ Module2::fail(system::Origin::<Runtime>::Root.into()),
+ Err(DispatchError::Module(ModuleError {
+ index: 32,
+ error: [0; 4],
+ message: Some("Something")
+ })),
+ );
+ assert_eq!(
+ Module1_2::fail(system::Origin::<Runtime>::Root.into()),
+ Err(DispatchError::Module(ModuleError {
+ index: 33,
+ error: [0; 4],
+ message: Some("Something")
+ })),
+ );
+ assert_eq!(
+ NestedModule3::fail(system::Origin::<Runtime>::Root.into()),
+ Err(DispatchError::Module(ModuleError {
+ index: 34,
+ error: [0; 4],
+ message: Some("Something")
+ })),
+ );
+ assert_eq!(
+ Module1_3::fail(system::Origin::<Runtime>::Root.into()),
+ Err(DispatchError::Module(ModuleError {
+ index: 6,
+ error: [0; 4],
+ message: Some("Something")
+ })),
+ );
+ assert_eq!(
+ Module1_4::fail(system::Origin::<Runtime>::Root.into()),
+ Err(DispatchError::Module(ModuleError {
+ index: 3,
+ error: [0; 4],
+ message: Some("Something")
+ })),
+ );
+ assert_eq!(
+ Module1_5::fail(system::Origin::<Runtime>::Root.into()),
+ Err(DispatchError::Module(ModuleError {
+ index: 4,
+ error: [0; 4],
+ message: Some("Something")
+ })),
+ );
+ assert_eq!(
+ Module1_6::fail(system::Origin::<Runtime>::Root.into()),
+ Err(DispatchError::Module(ModuleError {
+ index: 1,
+ error: [0; 4],
+ message: Some("Something")
+ })),
+ );
+ assert_eq!(
+ Module1_7::fail(system::Origin::<Runtime>::Root.into()),
+ Err(DispatchError::Module(ModuleError {
+ index: 2,
+ error: [0; 4],
+ message: Some("Something")
+ })),
+ );
+ assert_eq!(
+ Module1_8::fail(system::Origin::<Runtime>::Root.into()),
+ Err(DispatchError::Module(ModuleError {
+ index: 12,
+ error: [0; 4],
+ message: Some("Something")
+ })),
+ );
+ assert_eq!(
+ Module1_9::fail(system::Origin::<Runtime>::Root.into()),
+ Err(DispatchError::Module(ModuleError {
+ index: 13,
+ error: [0; 4],
+ message: Some("Something")
+ })),
+ );
+ });
}
#[test]
diff --git a/substrate/frame/support/test/tests/storage_layers.rs b/substrate/frame/support/test/tests/storage_layers.rs
index 05ed60fe901..7404ccace2c 100644
--- a/substrate/frame/support/test/tests/storage_layers.rs
+++ b/substrate/frame/support/test/tests/storage_layers.rs
@@ -276,5 +276,7 @@ fn storage_layer_in_decl_pallet_call() {
let call2 = Call::DeclPallet(decl_pallet::Call::set_value { value: 1 });
assert_noop!(call2.dispatch(Origin::signed(0)), "Revert!");
+ // Calling the function directly also works with storage layers.
+ assert_noop!(decl_pallet::Module::<Runtime>::set_value(Origin::signed(1), 1), "Revert!");
});
}
diff --git a/substrate/frame/transaction-storage/src/lib.rs b/substrate/frame/transaction-storage/src/lib.rs
index f16b8f02966..681cd29af82 100644
--- a/substrate/frame/transaction-storage/src/lib.rs
+++ b/substrate/frame/transaction-storage/src/lib.rs
@@ -245,9 +245,11 @@ pub mod pallet {
let sender = ensure_signed(origin)?;
let transactions = <Transactions<T>>::get(block).ok_or(Error::<T>::RenewedNotFound)?;
let info = transactions.get(index as usize).ok_or(Error::<T>::RenewedNotFound)?;
+ let extrinsic_index =
+ <frame_system::Pallet<T>>::extrinsic_index().ok_or(Error::<T>::BadContext)?;
+
Self::apply_fee(sender, info.size)?;
- let extrinsic_index = <frame_system::Pallet<T>>::extrinsic_index().unwrap();
sp_io::transaction_index::renew(extrinsic_index, info.content_hash.into());
let mut index = 0;
diff --git a/substrate/primitives/state-machine/src/basic.rs b/substrate/primitives/state-machine/src/basic.rs
index 6efc847bfbd..236a515a241 100644
--- a/substrate/primitives/state-machine/src/basic.rs
+++ b/substrate/primitives/state-machine/src/basic.rs
@@ -17,14 +17,13 @@
//! Basic implementation for Externalities.
-use crate::{Backend, StorageKey, StorageValue};
+use crate::{Backend, OverlayedChanges, StorageKey, StorageValue};
use codec::Encode;
use hash_db::Hasher;
use log::warn;
use sp_core::{
storage::{
- well_known_keys::is_child_storage_key, ChildInfo, StateVersion, Storage, StorageChild,
- TrackedStorageKey,
+ well_known_keys::is_child_storage_key, ChildInfo, StateVersion, Storage, TrackedStorageKey,
},
traits::Externalities,
Blake2Hasher,
@@ -35,20 +34,19 @@ use std::{
any::{Any, TypeId},
collections::BTreeMap,
iter::FromIterator,
- ops::Bound,
};
/// Simple Map-based Externalities impl.
#[derive(Debug)]
pub struct BasicExternalities {
- inner: Storage,
+ overlay: OverlayedChanges,
extensions: Extensions,
}
impl BasicExternalities {
/// Create a new instance of `BasicExternalities`
pub fn new(inner: Storage) -> Self {
- BasicExternalities { inner, extensions: Default::default() }
+ BasicExternalities { overlay: inner.into(), extensions: Default::default() }
}
/// New basic externalities with empty storage.
@@ -57,13 +55,34 @@ impl BasicExternalities {
}
/// Insert key/value
- pub fn insert(&mut self, k: StorageKey, v: StorageValue) -> Option<StorageValue> {
- self.inner.top.insert(k, v)
+ pub fn insert(&mut self, k: StorageKey, v: StorageValue) {
+ self.overlay.set_storage(k, Some(v));
}
/// Consume self and returns inner storages
pub fn into_storages(self) -> Storage {
- self.inner
+ Storage {
+ top: self
+ .overlay
+ .changes()
+ .filter_map(|(k, v)| v.value().map(|v| (k.to_vec(), v.to_vec())))
+ .collect(),
+ children_default: self
+ .overlay
+ .children()
+ .map(|(iter, i)| {
+ (
+ i.storage_key().to_vec(),
+ sp_core::storage::StorageChild {
+ data: iter
+ .filter_map(|(k, v)| v.value().map(|v| (k.to_vec(), v.to_vec())))
+ .collect(),
+ child_info: i.clone(),
+ },
+ )
+ })
+ .collect(),
+ }
}
/// Execute the given closure `f` with the externalities set and initialized with `storage`.
@@ -73,13 +92,7 @@ impl BasicExternalities {
storage: &mut sp_core::storage::Storage,
f: impl FnOnce() -> R,
) -> R {
- let mut ext = Self {
- inner: Storage {
- top: std::mem::take(&mut storage.top),
- children_default: std::mem::take(&mut storage.children_default),
- },
- extensions: Default::default(),
- };
+ let mut ext = Self::new(std::mem::take(storage));
let r = ext.execute_with(f);
@@ -108,15 +121,26 @@ impl BasicExternalities {
impl PartialEq for BasicExternalities {
fn eq(&self, other: &BasicExternalities) -> bool {
- self.inner.top.eq(&other.inner.top) &&
- self.inner.children_default.eq(&other.inner.children_default)
+ self.overlay.changes().map(|(k, v)| (k, v.value())).collect::<BTreeMap<_, _>>() ==
+ other.overlay.changes().map(|(k, v)| (k, v.value())).collect::<BTreeMap<_, _>>() &&
+ self.overlay
+ .children()
+ .map(|(iter, i)| (i, iter.map(|(k, v)| (k, v.value())).collect::<BTreeMap<_, _>>()))
+ .collect::<BTreeMap<_, _>>() ==
+ other
+ .overlay
+ .children()
+ .map(|(iter, i)| {
+ (i, iter.map(|(k, v)| (k, v.value())).collect::<BTreeMap<_, _>>())
+ })
+ .collect::<BTreeMap<_, _>>()
}
}
impl FromIterator<(StorageKey, StorageValue)> for BasicExternalities {
fn from_iter<I: IntoIterator<Item = (StorageKey, StorageValue)>>(iter: I) -> Self {
let mut t = Self::default();
- t.inner.top.extend(iter);
+ iter.into_iter().for_each(|(k, v)| t.insert(k, v));
t
}
}
@@ -128,11 +152,8 @@ impl Default for BasicExternalities {
}
impl From<BTreeMap<StorageKey, StorageValue>> for BasicExternalities {
- fn from(hashmap: BTreeMap<StorageKey, StorageValue>) -> Self {
- BasicExternalities {
- inner: Storage { top: hashmap, children_default: Default::default() },
- extensions: Default::default(),
- }
+ fn from(map: BTreeMap<StorageKey, StorageValue>) -> Self {
+ Self::from_iter(map.into_iter())
}
}
@@ -140,7 +161,7 @@ impl Externalities for BasicExternalities {
fn set_offchain_storage(&mut self, _key: &[u8], _value: Option<&[u8]>) {}
fn storage(&self, key: &[u8]) -> Option<StorageValue> {
- self.inner.top.get(key).cloned()
+ self.overlay.storage(key).and_then(|v| v.map(|v| v.to_vec()))
}
fn storage_hash(&self, key: &[u8]) -> Option<Vec<u8>> {
@@ -148,11 +169,7 @@ impl Externalities for BasicExternalities {
}
fn child_storage(&self, child_info: &ChildInfo, key: &[u8]) -> Option<StorageValue> {
- self.inner
- .children_default
- .get(child_info.storage_key())
- .and_then(|child| child.data.get(key))
- .cloned()
+ self.overlay.child_storage(child_info, key).and_then(|v| v.map(|v| v.to_vec()))
}
fn child_storage_hash(&self, child_info: &ChildInfo, key: &[u8]) -> Option<Vec<u8>> {
@@ -160,16 +177,13 @@ impl Externalities for BasicExternalities {
}
fn next_storage_key(&self, key: &[u8]) -> Option<StorageKey> {
- let range = (Bound::Excluded(key), Bound::Unbounded);
- self.inner.top.range::<[u8], _>(range).next().map(|(k, _)| k).cloned()
+ self.overlay.iter_after(key).find_map(|(k, v)| v.value().map(|_| k.to_vec()))
}
fn next_child_storage_key(&self, child_info: &ChildInfo, key: &[u8]) -> Option<StorageKey> {
- let range = (Bound::Excluded(key), Bound::Unbounded);
- self.inner
- .children_default
- .get(child_info.storage_key())
- .and_then(|child| child.data.range::<[u8], _>(range).next().map(|(k, _)| k).cloned())
+ self.overlay
+ .child_iter_after(child_info.storage_key(), key)
+ .find_map(|(k, v)| v.value().map(|_| k.to_vec()))
}
fn place_storage(&mut self, key: StorageKey, maybe_value: Option<StorageValue>) {
@@ -178,14 +192,7 @@ impl Externalities for BasicExternalities {
return
}
- match maybe_value {
- Some(value) => {
- self.inner.top.insert(key, value);
- },
- None => {
- self.inner.top.remove(&key);
- },
- }
+ self.overlay.set_storage(key, maybe_value)
}
fn place_child_storage(
@@ -194,19 +201,7 @@ impl Externalities for BasicExternalities {
key: StorageKey,
value: Option<StorageValue>,
) {
- let child_map = self
- .inner
- .children_default
- .entry(child_info.storage_key().to_vec())
- .or_insert_with(|| StorageChild {
- data: Default::default(),
- child_info: child_info.to_owned(),
- });
- if let Some(value) = value {
- child_map.data.insert(key, value);
- } else {
- child_map.data.remove(&key);
- }
+ self.overlay.set_child_storage(child_info, key, value);
}
fn kill_child_storage(
@@ -215,12 +210,7 @@ impl Externalities for BasicExternalities {
_maybe_limit: Option<u32>,
_maybe_cursor: Option<&[u8]>,
) -> MultiRemovalResults {
- let count = self
- .inner
- .children_default
- .remove(child_info.storage_key())
- .map(|c| c.data.len())
- .unwrap_or(0) as u32;
+ let count = self.overlay.clear_child_storage(child_info);
MultiRemovalResults { maybe_cursor: None, backend: count, unique: count, loops: count }
}
@@ -239,19 +229,7 @@ impl Externalities for BasicExternalities {
return MultiRemovalResults { maybe_cursor, backend: 0, unique: 0, loops: 0 }
}
- let to_remove = self
- .inner
- .top
- .range::<[u8], _>((Bound::Included(prefix), Bound::Unbounded))
- .map(|(k, _)| k)
- .take_while(|k| k.starts_with(prefix))
- .cloned()
- .collect::<Vec<_>>();
-
- let count = to_remove.len() as u32;
- for key in to_remove {
- self.inner.top.remove(&key);
- }
+ let count = self.overlay.clear_prefix(prefix);
MultiRemovalResults { maybe_cursor: None, backend: count, unique: count, loops: count }
}
@@ -262,56 +240,37 @@ impl Externalities for BasicExternalities {
_maybe_limit: Option<u32>,
_maybe_cursor: Option<&[u8]>,
) -> MultiRemovalResults {
- if let Some(child) = self.inner.children_default.get_mut(child_info.storage_key()) {
- let to_remove = child
- .data
- .range::<[u8], _>((Bound::Included(prefix), Bound::Unbounded))
- .map(|(k, _)| k)
- .take_while(|k| k.starts_with(prefix))
- .cloned()
- .collect::<Vec<_>>();
-
- let count = to_remove.len() as u32;
- for key in to_remove {
- child.data.remove(&key);
- }
- MultiRemovalResults { maybe_cursor: None, backend: count, unique: count, loops: count }
- } else {
- MultiRemovalResults { maybe_cursor: None, backend: 0, unique: 0, loops: 0 }
- }
+ let count = self.overlay.clear_child_prefix(child_info, prefix);
+ MultiRemovalResults { maybe_cursor: None, backend: count, unique: count, loops: count }
}
fn storage_append(&mut self, key: Vec<u8>, value: Vec<u8>) {
- let current = self.inner.top.entry(key).or_default();
- crate::ext::StorageAppend::new(current).append(value);
+ let current_value = self.overlay.value_mut_or_insert_with(&key, || Default::default());
+ crate::ext::StorageAppend::new(current_value).append(value);
}
fn storage_root(&mut self, state_version: StateVersion) -> Vec<u8> {
- let mut top = self.inner.top.clone();
- let prefixed_keys: Vec<_> = self
- .inner
- .children_default
- .iter()
- .map(|(_k, v)| (v.child_info.prefixed_storage_key(), v.child_info.clone()))
- .collect();
+ let mut top = self
+ .overlay
+ .changes()
+ .filter_map(|(k, v)| v.value().map(|v| (k.clone(), v.clone())))
+ .collect::<BTreeMap<_, _>>();
// Single child trie implementation currently allows using the same child
// empty root for all child trie. Using null storage key until multiple
// type of child trie support.
let empty_hash = empty_child_trie_root::<LayoutV1<Blake2Hasher>>();
- for (prefixed_storage_key, child_info) in prefixed_keys {
+ for child_info in self.overlay.children().map(|d| d.1.clone()).collect::<Vec<_>>() {
let child_root = self.child_storage_root(&child_info, state_version);
if empty_hash[..] == child_root[..] {
- top.remove(prefixed_storage_key.as_slice());
+ top.remove(child_info.prefixed_storage_key().as_slice());
} else {
- top.insert(prefixed_storage_key.into_inner(), child_root);
+ top.insert(child_info.prefixed_storage_key().into_inner(), child_root);
}
}
match state_version {
- StateVersion::V0 =>
- LayoutV0::<Blake2Hasher>::trie_root(self.inner.top.clone()).as_ref().into(),
- StateVersion::V1 =>
- LayoutV1::<Blake2Hasher>::trie_root(self.inner.top.clone()).as_ref().into(),
+ StateVersion::V0 => LayoutV0::<Blake2Hasher>::trie_root(top).as_ref().into(),
+ StateVersion::V1 => LayoutV1::<Blake2Hasher>::trie_root(top).as_ref().into(),
}
}
@@ -320,10 +279,11 @@ impl Externalities for BasicExternalities {
child_info: &ChildInfo,
state_version: StateVersion,
) -> Vec<u8> {
- if let Some(child) = self.inner.children_default.get(child_info.storage_key()) {
- let delta = child.data.iter().map(|(k, v)| (k.as_ref(), Some(v.as_ref())));
+ if let Some((data, child_info)) = self.overlay.child_changes(child_info.storage_key()) {
+ let delta =
+ data.into_iter().map(|(k, v)| (k.as_ref(), v.value().map(|v| v.as_slice())));
crate::in_memory_backend::new_in_mem::<Blake2Hasher, HashKey<_>>()
- .child_storage_root(&child.child_info, delta, state_version)
+ .child_storage_root(&child_info, delta, state_version)
.0
} else {
empty_child_trie_root::<LayoutV1<Blake2Hasher>>()
@@ -332,15 +292,15 @@ impl Externalities for BasicExternalities {
}
fn storage_start_transaction(&mut self) {
- unimplemented!("Transactions are not supported by BasicExternalities");
+ self.overlay.start_transaction()
}
fn storage_rollback_transaction(&mut self) -> Result<(), ()> {
- unimplemented!("Transactions are not supported by BasicExternalities");
+ self.overlay.rollback_transaction().map_err(drop)
}
fn storage_commit_transaction(&mut self) -> Result<(), ()> {
- unimplemented!("Transactions are not supported by BasicExternalities");
+ self.overlay.commit_transaction().map_err(drop)
}
fn wipe(&mut self) {}
diff --git a/substrate/primitives/state-machine/src/overlayed_changes/changeset.rs b/substrate/primitives/state-machine/src/overlayed_changes/changeset.rs
index ae5d47f6bb9..e5dad7157c7 100644
--- a/substrate/primitives/state-machine/src/overlayed_changes/changeset.rs
+++ b/substrate/primitives/state-machine/src/overlayed_changes/changeset.rs
@@ -57,7 +57,7 @@ pub struct NotInRuntime;
/// Describes in which mode the node is currently executing.
#[derive(Debug, Clone, Copy)]
pub enum ExecutionMode {
- /// Exeuting in client mode: Removal of all transactions possible.
+ /// Executing in client mode: Removal of all transactions possible.
Client,
/// Executing in runtime mode: Transactions started by the client are protected.
Runtime,
@@ -95,7 +95,7 @@ pub type OverlayedChangeSet = OverlayedMap<StorageKey, Option<StorageValue>>;
/// Holds a set of changes with the ability modify them using nested transactions.
#[derive(Debug, Clone)]
-pub struct OverlayedMap<K: Ord + Hash, V> {
+pub struct OverlayedMap<K, V> {
/// Stores the changes that this overlay constitutes.
changes: BTreeMap<K, OverlayedEntry<V>>,
/// Stores which keys are dirty per transaction. Needed in order to determine which
@@ -110,7 +110,7 @@ pub struct OverlayedMap<K: Ord + Hash, V> {
execution_mode: ExecutionMode,
}
-impl<K: Ord + Hash, V> Default for OverlayedMap<K, V> {
+impl<K, V> Default for OverlayedMap<K, V> {
fn default() -> Self {
Self {
changes: BTreeMap::new(),
@@ -121,6 +121,31 @@ impl<K: Ord + Hash, V> Default for OverlayedMap<K, V> {
}
}
+#[cfg(feature = "std")]
+impl From<sp_core::storage::StorageMap> for OverlayedMap<StorageKey, Option<StorageValue>> {
+ fn from(storage: sp_core::storage::StorageMap) -> Self {
+ Self {
+ changes: storage
+ .into_iter()
+ .map(|(k, v)| {
+ (
+ k,
+ OverlayedEntry {
+ transactions: SmallVec::from_iter([InnerValue {
+ value: Some(v),
+ extrinsics: Default::default(),
+ }]),
+ },
+ )
+ })
+ .collect(),
+ dirty_keys: Default::default(),
+ num_client_transactions: 0,
+ execution_mode: ExecutionMode::Client,
+ }
+ }
+}
+
impl Default for ExecutionMode {
fn default() -> Self {
Self::Client
diff --git a/substrate/primitives/state-machine/src/overlayed_changes/mod.rs b/substrate/primitives/state-machine/src/overlayed_changes/mod.rs
index 746599a6768..001b4b656c3 100644
--- a/substrate/primitives/state-machine/src/overlayed_changes/mod.rs
+++ b/substrate/primitives/state-machine/src/overlayed_changes/mod.rs
@@ -638,6 +638,21 @@ impl OverlayedChanges {
}
}
+#[cfg(feature = "std")]
+impl From<sp_core::storage::Storage> for OverlayedChanges {
+ fn from(storage: sp_core::storage::Storage) -> Self {
+ Self {
+ top: storage.top.into(),
+ children: storage
+ .children_default
+ .into_iter()
+ .map(|(k, v)| (k, (v.data.into(), v.child_info)))
+ .collect(),
+ ..Default::default()
+ }
+ }
+}
+
#[cfg(feature = "std")]
fn retain_map<K, V, F>(map: &mut Map<K, V>, f: F)
where
--
GitLab