From 338b26762e1d44e756d4faa6cd04a77dfa8347ff Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Wed, 23 Sep 2020 12:24:17 +1200 Subject: [PATCH 1/8] implement batch_all --- frame/support/procedural/src/transactional.rs | 2 +- frame/utility/src/lib.rs | 57 ++++++++++++++++++- frame/utility/src/tests.rs | 48 +++++++++++++++- 3 files changed, 102 insertions(+), 5 deletions(-) diff --git a/frame/support/procedural/src/transactional.rs b/frame/support/procedural/src/transactional.rs index fbd0c9ca0b3c4..6dedd92028e7d 100644 --- a/frame/support/procedural/src/transactional.rs +++ b/frame/support/procedural/src/transactional.rs @@ -29,7 +29,7 @@ pub fn transactional(_attr: TokenStream, input: TokenStream) -> Result + /// - Base weight: 14.39 + .987 * c µs + /// - Plus the sum of the weights of the `calls`. + /// - Plus one additional event. (repeat read/write) + /// # + #[weight = ( + calls.iter() + .map(|call| call.get_dispatch_info().weight) + .fold(0, |total: Weight, weight: Weight| total.saturating_add(weight)) + .saturating_add(T::WeightInfo::batch(calls.len() as u32)), + { + let all_operational = calls.iter() + .map(|call| call.get_dispatch_info().class) + .all(|class| class == DispatchClass::Operational); + if all_operational { + DispatchClass::Operational + } else { + DispatchClass::Normal + } + }, + )] + #[transactional] + fn batch_all(origin, calls: Vec<::Call>) -> DispatchResultWithPostInfo { + let is_root = ensure_root(origin.clone()).is_ok(); + let mut weight: Weight = 0; + for call in calls.into_iter() { + let info = call.get_dispatch_info(); + let result = if is_root { + call.dispatch_bypass_filter(origin.clone()) + } else { + call.dispatch(origin.clone()) + }; + weight += extract_actual_weight(&result, &info); + result.map_err(|mut err| { + err.post_info = Some(weight).into(); + err + })?; + } + Self::deposit_event(Event::BatchCompleted); + Ok(Some(weight).into()) + } } } diff --git a/frame/utility/src/tests.rs b/frame/utility/src/tests.rs index 8e693b234a939..bfdf30106331f 100644 --- a/frame/utility/src/tests.rs +++ b/frame/utility/src/tests.rs @@ -22,8 +22,11 @@ use super::*; use frame_support::{ - assert_ok, assert_noop, impl_outer_origin, parameter_types, impl_outer_dispatch, - weights::Weight, impl_outer_event, dispatch::DispatchError, traits::Filter, storage, + assert_ok, assert_noop, impl_outer_origin, parameter_types, impl_outer_dispatch, impl_outer_event, + weights::{Weight, Pays}, + dispatch::{DispatchError, DispatchErrorWithPostInfo}, + traits::Filter, + storage, }; use sp_core::H256; use sp_runtime::{Perbill, traits::{BlakeTwo256, IdentityLookup}, testing::Header}; @@ -255,3 +258,44 @@ fn batch_weight_calculation_doesnt_overflow() { assert_eq!(batch_call.get_dispatch_info().weight, Weight::max_value()); }); } + + +#[test] +fn batch_all_works() { + new_test_ext().execute_with(|| { + assert_eq!(Balances::free_balance(1), 10); + assert_eq!(Balances::free_balance(2), 10); + assert_ok!( + Utility::batch_all(Origin::signed(1), vec![ + Call::Balances(BalancesCall::transfer(2, 5)), + Call::Balances(BalancesCall::transfer(2, 5)) + ]), + ); + assert_eq!(Balances::free_balance(1), 0); + assert_eq!(Balances::free_balance(2), 20); + }); +} + +#[test] +fn batch_all_revert() { + new_test_ext().execute_with(|| { + assert_eq!(Balances::free_balance(1), 10); + assert_eq!(Balances::free_balance(2), 10); + assert_noop!( + Utility::batch_all(Origin::signed(1), vec![ + Call::Balances(BalancesCall::transfer(2, 5)), + Call::Balances(BalancesCall::transfer(2, 10)), + Call::Balances(BalancesCall::transfer(2, 5)), + ]), + DispatchErrorWithPostInfo { + post_info: PostDispatchInfo { + actual_weight: Some(381898000), + pays_fee: Pays::Yes + }, + error: pallet_balances::Error::::InsufficientBalance.into() + } + ); + assert_eq!(Balances::free_balance(1), 10); + assert_eq!(Balances::free_balance(2), 10); + }); +} From 7afd502e9165e07861be7a8298ad7f0191b9575a Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Wed, 23 Sep 2020 15:01:18 +1200 Subject: [PATCH 2/8] bump version --- bin/node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 4d2f11d2f9206..74e898219e107 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -109,7 +109,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { // and set impl_version to 0. If only runtime // implementation changes and behavior does not, then leave spec_version as // is and increment impl_version. - spec_version: 259, + spec_version: 260, impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 1, From fa621c25b3eaecae01f95093afdb48d7530f3826 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Thu, 24 Sep 2020 16:08:24 +1200 Subject: [PATCH 3/8] updates --- frame/utility/src/lib.rs | 10 +++------- frame/utility/src/tests.rs | 5 ++++- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/frame/utility/src/lib.rs b/frame/utility/src/lib.rs index 03b68c835f541..2227a38da6636 100644 --- a/frame/utility/src/lib.rs +++ b/frame/utility/src/lib.rs @@ -129,9 +129,7 @@ decl_module! { /// bypassing `frame_system::Trait::BaseCallFilter`). /// /// # - /// - Base weight: 14.39 + .987 * c µs - /// - Plus the sum of the weights of the `calls`. - /// - Plus one additional event. (repeat read/write) + /// - Complexity: O(C) where C is the number of calls to be batched. /// # /// /// This will return `Ok` in all circumstances. To determine the success of the batch, an @@ -208,9 +206,7 @@ decl_module! { /// bypassing `frame_system::Trait::BaseCallFilter`). /// /// # - /// - Base weight: 14.39 + .987 * c µs - /// - Plus the sum of the weights of the `calls`. - /// - Plus one additional event. (repeat read/write) + /// - Complexity: O(C) where C is the number of calls to be batched. /// # #[weight = ( calls.iter() @@ -239,7 +235,7 @@ decl_module! { } else { call.dispatch(origin.clone()) }; - weight += extract_actual_weight(&result, &info); + weight = weight.saturating_add(extract_actual_weight(&result, &info)); result.map_err(|mut err| { err.post_info = Some(weight).into(); err diff --git a/frame/utility/src/tests.rs b/frame/utility/src/tests.rs index bfdf30106331f..c98e8a2917d54 100644 --- a/frame/utility/src/tests.rs +++ b/frame/utility/src/tests.rs @@ -279,6 +279,9 @@ fn batch_all_works() { #[test] fn batch_all_revert() { new_test_ext().execute_with(|| { + let call = Call::Balances(BalancesCall::transfer(2, 5)); + let info = call.get_dispatch_info(); + assert_eq!(Balances::free_balance(1), 10); assert_eq!(Balances::free_balance(2), 10); assert_noop!( @@ -289,7 +292,7 @@ fn batch_all_revert() { ]), DispatchErrorWithPostInfo { post_info: PostDispatchInfo { - actual_weight: Some(381898000), + actual_weight: Some(info.weight * 2), pays_fee: Pays::Yes }, error: pallet_balances::Error::::InsufficientBalance.into() From 8d09b6574fb708917e845cff9382a66226c6fbcc Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 2 Oct 2020 04:01:41 +0200 Subject: [PATCH 4/8] Better weight story for utility --- frame/support/src/lib.rs | 20 +++ frame/utility/src/benchmarking.rs | 14 ++ frame/utility/src/default_weights.rs | 4 + frame/utility/src/lib.rs | 49 +++++- frame/utility/src/tests.rs | 246 ++++++++++++++++++++++++++- 5 files changed, 320 insertions(+), 13 deletions(-) diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index bdbdfc04a31f9..6c0dad0c9f73a 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -319,6 +319,26 @@ macro_rules! assert_err { } } +/// Evaluate an expression, assert it returns an expected `Err` value and that +/// runtime storage has not been mutated (i.e. expression is a no-operation). +/// +/// Used as `assert_noop(expression_to_assert, expected_error_expression)`. +/// +/// This can be used on`DispatchResultWithPostInfo` when the post info should +/// be ignored. +#[macro_export] +#[cfg(feature = "std")] +macro_rules! assert_noop_ignore_postinfo { + ( + $x:expr, + $y:expr $(,)? + ) => { + let h = $crate::storage_root(); + $crate::assert_err_ignore_postinfo!($x, $y); + assert_eq!(h, $crate::storage_root()); + } +} + /// Assert an expression returns an error specified. /// /// This can be used on`DispatchResultWithPostInfo` when the post info should diff --git a/frame/utility/src/benchmarking.rs b/frame/utility/src/benchmarking.rs index 8ca0e216f2887..ad0e2f0b6839e 100644 --- a/frame/utility/src/benchmarking.rs +++ b/frame/utility/src/benchmarking.rs @@ -57,6 +57,19 @@ benchmarks! { let caller_key = frame_system::Account::::hashed_key_for(&caller); frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); }: _(RawOrigin::Signed(caller), u as u16, call) + + batch_all { + let c in 0 .. 1000; + let mut calls: Vec<::Call> = Vec::new(); + for i in 0 .. c { + let call = frame_system::Call::remark(vec![]).into(); + calls.push(call); + } + let caller = whitelisted_caller(); + }: _(RawOrigin::Signed(caller), calls) + verify { + assert_last_event::(Event::BatchCompleted.into()) + } } #[cfg(test)] @@ -70,6 +83,7 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(test_benchmark_batch::()); assert_ok!(test_benchmark_as_derivative::()); + assert_ok!(test_benchmark_batch_all::()); }); } } diff --git a/frame/utility/src/default_weights.rs b/frame/utility/src/default_weights.rs index d023dbddd4f80..903c542f0db47 100644 --- a/frame/utility/src/default_weights.rs +++ b/frame/utility/src/default_weights.rs @@ -31,4 +31,8 @@ impl crate::WeightInfo for () { fn as_derivative() -> Weight { (4086000 as Weight) } + fn batch_all(c: u32, ) -> Weight { + (16461000 as Weight) + .saturating_add((1982000 as Weight).saturating_mul(c as Weight)) + } } diff --git a/frame/utility/src/lib.rs b/frame/utility/src/lib.rs index d6672d0c3e938..3b86a3d0cd1f9 100644 --- a/frame/utility/src/lib.rs +++ b/frame/utility/src/lib.rs @@ -66,7 +66,7 @@ use frame_support::{ dispatch::{PostDispatchInfo, DispatchResultWithPostInfo}, }; use frame_system::{ensure_signed, ensure_root}; -use sp_runtime::{DispatchError, DispatchResult, traits::Dispatchable}; +use sp_runtime::{DispatchError, traits::Dispatchable}; mod tests; mod benchmarking; @@ -75,6 +75,7 @@ mod default_weights; pub trait WeightInfo { fn batch(c: u32, ) -> Weight; fn as_derivative() -> Weight; + fn batch_all(c: u32, ) -> Weight; } /// Configuration trait. @@ -153,20 +154,32 @@ decl_module! { } }, )] - fn batch(origin, calls: Vec<::Call>) { + fn batch(origin, calls: Vec<::Call>) -> DispatchResultWithPostInfo { let is_root = ensure_root(origin.clone()).is_ok(); + let calls_len = calls.len(); + // Track the actual weight of each of the batch calls. + let mut weight: Weight = 0; for (index, call) in calls.into_iter().enumerate() { + let info = call.get_dispatch_info(); + // If origin is root, don't apply any dispatch filters; root can call anything. let result = if is_root { call.dispatch_bypass_filter(origin.clone()) } else { call.dispatch(origin.clone()) }; + // Add the weight of this call. + weight = weight.saturating_add(extract_actual_weight(&result, &info)); if let Err(e) = result { Self::deposit_event(Event::BatchInterrupted(index as u32, e.error)); - return Ok(()); + // Take the weight of this function itself into account. + let base_weight = T::WeightInfo::batch(index.saturating_add(1) as u32); + // Return the actual used weight + base_weight of this call. + return Ok(Some(base_weight + weight).into()); } } Self::deposit_event(Event::BatchCompleted); + let base_weight = T::WeightInfo::batch(calls_len as u32); + return Ok(Some(base_weight + weight).into()); } /// Send a call through an indexed pseudonym of the sender. @@ -189,12 +202,22 @@ decl_module! { .saturating_add(T::DbWeight::get().reads_writes(1, 1)), call.get_dispatch_info().class, )] - fn as_derivative(origin, index: u16, call: Box<::Call>) -> DispatchResult { + fn as_derivative(origin, index: u16, call: Box<::Call>) -> DispatchResultWithPostInfo { let mut origin = origin; let who = ensure_signed(origin.clone())?; let pseudonym = Self::derivative_account_id(who, index); origin.set_caller_from(frame_system::RawOrigin::Signed(pseudonym)); - call.dispatch(origin).map(|_| ()).map_err(|e| e.error) + let info = call.get_dispatch_info(); + let result = call.dispatch(origin); + // Always take into account the base weight of this call. + let mut weight = T::WeightInfo::as_derivative().saturating_add(T::DbWeight::get().reads_writes(1, 1)); + // Add the real weight of the dispatch. + weight = weight.saturating_add(extract_actual_weight(&result, &info)); + result.map_err(|mut err| { + err.post_info = Some(weight).into(); + err + })?; + Ok(Some(weight).into()) } /// Send a batch of dispatch calls and atomically execute them. @@ -214,7 +237,7 @@ decl_module! { calls.iter() .map(|call| call.get_dispatch_info().weight) .fold(0, |total: Weight, weight: Weight| total.saturating_add(weight)) - .saturating_add(T::WeightInfo::batch(calls.len() as u32)), + .saturating_add(T::WeightInfo::batch_all(calls.len() as u32)), { let all_operational = calls.iter() .map(|call| call.get_dispatch_info().class) @@ -229,22 +252,30 @@ decl_module! { #[transactional] fn batch_all(origin, calls: Vec<::Call>) -> DispatchResultWithPostInfo { let is_root = ensure_root(origin.clone()).is_ok(); + let calls_len = calls.len(); + // Track the actual weight of each of the batch calls. let mut weight: Weight = 0; - for call in calls.into_iter() { + for (index, call) in calls.into_iter().enumerate() { let info = call.get_dispatch_info(); + // If origin is root, bypass any dispatch filter; root can call anything. let result = if is_root { call.dispatch_bypass_filter(origin.clone()) } else { call.dispatch(origin.clone()) }; + // Add the weight of this call. weight = weight.saturating_add(extract_actual_weight(&result, &info)); result.map_err(|mut err| { - err.post_info = Some(weight).into(); + // Take the weight of this function itself into account. + let base_weight = T::WeightInfo::batch_all(index.saturating_add(1) as u32); + // Return the actual used weight + base_weight of this call. + err.post_info = Some(base_weight + weight).into(); err })?; } Self::deposit_event(Event::BatchCompleted); - Ok(Some(weight).into()) + let base_weight = T::WeightInfo::batch_all(calls_len as u32); + Ok(Some(base_weight + weight).into()) } } } diff --git a/frame/utility/src/tests.rs b/frame/utility/src/tests.rs index c98e8a2917d54..58284913e3030 100644 --- a/frame/utility/src/tests.rs +++ b/frame/utility/src/tests.rs @@ -23,8 +23,9 @@ use super::*; use frame_support::{ assert_ok, assert_noop, impl_outer_origin, parameter_types, impl_outer_dispatch, impl_outer_event, + assert_noop_ignore_postinfo, weights::{Weight, Pays}, - dispatch::{DispatchError, DispatchErrorWithPostInfo}, + dispatch::{DispatchError, DispatchErrorWithPostInfo, Dispatchable}, traits::Filter, storage, }; @@ -32,6 +33,40 @@ use sp_core::H256; use sp_runtime::{Perbill, traits::{BlakeTwo256, IdentityLookup}, testing::Header}; use crate as utility; +// example module to test behaviors. +pub mod example { + use super::*; + use frame_support::dispatch::WithPostDispatchInfo; + pub trait Trait: frame_system::Trait { } + + decl_module! { + pub struct Module for enum Call where origin: ::Origin { + #[weight = *weight] + fn noop(_origin, weight: Weight) { } + + #[weight = *start_weight] + fn foobar( + origin, + err: bool, + start_weight: Weight, + end_weight: Option, + ) -> DispatchResultWithPostInfo { + let _ = ensure_signed(origin)?; + if err { + let error: DispatchError = "The cake is a lie.".into(); + if let Some(weight) = end_weight { + Err(error.with_weight(weight)) + } else { + Err(error)? + } + } else { + Ok(end_weight.into()) + } + } + } + } +} + impl_outer_origin! { pub enum Origin for Test where system = frame_system {} } @@ -47,6 +82,7 @@ impl_outer_dispatch! { frame_system::System, pallet_balances::Balances, utility::Utility, + example::Example, } } @@ -105,13 +141,19 @@ parameter_types! { pub const MultisigDepositFactor: u64 = 1; pub const MaxSignatories: u16 = 3; } + +impl example::Trait for Test {} + pub struct TestBaseCallFilter; impl Filter for TestBaseCallFilter { fn filter(c: &Call) -> bool { match *c { Call::Balances(_) => true, + Call::Utility(_) => true, // For benchmarking, this acts as a noop call Call::System(frame_system::Call::remark(..)) => true, + // For tests + Call::Example(_) => true, _ => false, } } @@ -123,8 +165,12 @@ impl Trait for Test { } type System = frame_system::Module; type Balances = pallet_balances::Module; +type Example = example::Module; type Utility = Module; +type ExampleCall = example::Call; +type UtilityCall = crate::Call; + use frame_system::Call as SystemCall; use pallet_balances::Call as BalancesCall; use pallet_balances::Error as BalancesError; @@ -152,7 +198,7 @@ fn as_derivative_works() { new_test_ext().execute_with(|| { let sub_1_0 = Utility::derivative_account_id(1, 0); assert_ok!(Balances::transfer(Origin::signed(1), sub_1_0, 5)); - assert_noop!(Utility::as_derivative( + assert_noop_ignore_postinfo!(Utility::as_derivative( Origin::signed(1), 1, Box::new(Call::Balances(BalancesCall::transfer(6, 3))), @@ -167,10 +213,70 @@ fn as_derivative_works() { }); } +#[test] +fn as_derivative_handles_weight_refund() { + new_test_ext().execute_with(|| { + let start_weight = 100; + let end_weight = 75; + let diff = start_weight - end_weight; + + // Full weight when ok + let inner_call = Call::Example(ExampleCall::foobar(false, start_weight, None)); + let call = Call::Utility(UtilityCall::as_derivative(0, Box::new(inner_call))); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_ok!(result); + assert_eq!(extract_actual_weight(&result, &info), info.weight); + + // Refund weight when ok + let inner_call = Call::Example(ExampleCall::foobar(false, start_weight, Some(end_weight))); + let call = Call::Utility(UtilityCall::as_derivative(0, Box::new(inner_call))); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_ok!(result); + // Diff is refunded + assert_eq!(extract_actual_weight(&result, &info), info.weight - diff); + + // Full weight when err + let inner_call = Call::Example(ExampleCall::foobar(true, start_weight, None)); + let call = Call::Utility(UtilityCall::as_derivative(0, Box::new(inner_call))); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_noop!( + result, + DispatchErrorWithPostInfo { + post_info: PostDispatchInfo { + // No weight is refunded + actual_weight: Some(info.weight), + pays_fee: Pays::Yes, + }, + error: DispatchError::Other("The cake is a lie."), + } + ); + + // Refund weight when err + let inner_call = Call::Example(ExampleCall::foobar(true, start_weight, Some(end_weight))); + let call = Call::Utility(UtilityCall::as_derivative(0, Box::new(inner_call))); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_noop!( + result, + DispatchErrorWithPostInfo { + post_info: PostDispatchInfo { + // Diff is refunded + actual_weight: Some(info.weight - diff), + pays_fee: Pays::Yes, + }, + error: DispatchError::Other("The cake is a lie."), + } + ); + }); +} + #[test] fn as_derivative_filters() { new_test_ext().execute_with(|| { - assert_noop!(Utility::as_derivative( + assert_noop_ignore_postinfo!(Utility::as_derivative( Origin::signed(1), 1, Box::new(Call::System(frame_system::Call::suicide())), @@ -259,6 +365,73 @@ fn batch_weight_calculation_doesnt_overflow() { }); } +#[test] +fn batch_handles_weight_refund() { + new_test_ext().execute_with(|| { + let start_weight = 100; + let end_weight = 75; + let diff = start_weight - end_weight; + let batch_len: Weight = 4; + + // Full weight when ok + let inner_call = Call::Example(ExampleCall::foobar(false, start_weight, None)); + let batch_calls = vec![inner_call; batch_len as usize]; + let call = Call::Utility(UtilityCall::batch(batch_calls)); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_ok!(result); + assert_eq!(extract_actual_weight(&result, &info), info.weight); + + // Refund weight when ok + let inner_call = Call::Example(ExampleCall::foobar(false, start_weight, Some(end_weight))); + let batch_calls = vec![inner_call; batch_len as usize]; + let call = Call::Utility(UtilityCall::batch(batch_calls)); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_ok!(result); + // Diff is refunded + assert_eq!(extract_actual_weight(&result, &info), info.weight - diff * batch_len); + + // Full weight when err + let good_call = Call::Example(ExampleCall::foobar(false, start_weight, None)); + let bad_call = Call::Example(ExampleCall::foobar(true, start_weight, None)); + let batch_calls = vec![good_call, bad_call]; + let call = Call::Utility(UtilityCall::batch(batch_calls)); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_ok!(result); + expect_event(Event::BatchInterrupted(1, DispatchError::Other(""))); + // No weight is refunded + assert_eq!(extract_actual_weight(&result, &info), info.weight); + + // Refund weight when err + let good_call = Call::Example(ExampleCall::foobar(false, start_weight, Some(end_weight))); + let bad_call = Call::Example(ExampleCall::foobar(true, start_weight, Some(end_weight))); + let batch_calls = vec![good_call, bad_call]; + let batch_len = batch_calls.len() as Weight; + let call = Call::Utility(UtilityCall::batch(batch_calls)); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_ok!(result); + expect_event(Event::BatchInterrupted(1, DispatchError::Other(""))); + assert_eq!(extract_actual_weight(&result, &info), info.weight - diff * batch_len); + + // Partial batch completion + let good_call = Call::Example(ExampleCall::foobar(false, start_weight, Some(end_weight))); + let bad_call = Call::Example(ExampleCall::foobar(true, start_weight, Some(end_weight))); + let batch_calls = vec![good_call, bad_call.clone(), bad_call]; + let call = Call::Utility(UtilityCall::batch(batch_calls)); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_ok!(result); + expect_event(Event::BatchInterrupted(1, DispatchError::Other(""))); + assert_eq!( + extract_actual_weight(&result, &info), + // Real weight is 2 calls at end_weight + ::WeightInfo::batch(2) + end_weight * 2, + ); + }); +} #[test] fn batch_all_works() { @@ -292,7 +465,7 @@ fn batch_all_revert() { ]), DispatchErrorWithPostInfo { post_info: PostDispatchInfo { - actual_weight: Some(info.weight * 2), + actual_weight: Some(::WeightInfo::batch(2) + info.weight * 2), pays_fee: Pays::Yes }, error: pallet_balances::Error::::InsufficientBalance.into() @@ -302,3 +475,68 @@ fn batch_all_revert() { assert_eq!(Balances::free_balance(2), 10); }); } + +#[test] +fn batch_all_handles_weight_refund() { + new_test_ext().execute_with(|| { + let start_weight = 100; + let end_weight = 75; + let diff = start_weight - end_weight; + let batch_len: Weight = 4; + + // Full weight when ok + let inner_call = Call::Example(ExampleCall::foobar(false, start_weight, None)); + let batch_calls = vec![inner_call; batch_len as usize]; + let call = Call::Utility(UtilityCall::batch_all(batch_calls)); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_ok!(result); + assert_eq!(extract_actual_weight(&result, &info), info.weight); + + // Refund weight when ok + let inner_call = Call::Example(ExampleCall::foobar(false, start_weight, Some(end_weight))); + let batch_calls = vec![inner_call; batch_len as usize]; + let call = Call::Utility(UtilityCall::batch_all(batch_calls)); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_ok!(result); + // Diff is refunded + assert_eq!(extract_actual_weight(&result, &info), info.weight - diff * batch_len); + + // Full weight when err + let good_call = Call::Example(ExampleCall::foobar(false, start_weight, None)); + let bad_call = Call::Example(ExampleCall::foobar(true, start_weight, None)); + let batch_calls = vec![good_call, bad_call]; + let call = Call::Utility(UtilityCall::batch_all(batch_calls)); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_noop_ignore_postinfo!(result, "The cake is a lie."); + // No weight is refunded + assert_eq!(extract_actual_weight(&result, &info), info.weight); + + // Refund weight when err + let good_call = Call::Example(ExampleCall::foobar(false, start_weight, Some(end_weight))); + let bad_call = Call::Example(ExampleCall::foobar(true, start_weight, Some(end_weight))); + let batch_calls = vec![good_call, bad_call]; + let batch_len = batch_calls.len() as Weight; + let call = Call::Utility(UtilityCall::batch_all(batch_calls)); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_noop_ignore_postinfo!(result, "The cake is a lie."); + assert_eq!(extract_actual_weight(&result, &info), info.weight - diff * batch_len); + + // Partial batch completion + let good_call = Call::Example(ExampleCall::foobar(false, start_weight, Some(end_weight))); + let bad_call = Call::Example(ExampleCall::foobar(true, start_weight, Some(end_weight))); + let batch_calls = vec![good_call, bad_call.clone(), bad_call]; + let call = Call::Utility(UtilityCall::batch_all(batch_calls)); + let info = call.get_dispatch_info(); + let result = call.dispatch(Origin::signed(1)); + assert_noop_ignore_postinfo!(result, "The cake is a lie."); + assert_eq!( + extract_actual_weight(&result, &info), + // Real weight is 2 calls at end_weight + ::WeightInfo::batch_all(2) + end_weight * 2, + ); + }); +} From b0ab964f5cfa2c787052c2cc2ae0b2d3c047cb32 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 2 Oct 2020 04:08:54 +0200 Subject: [PATCH 5/8] small fixes --- bin/node/runtime/src/weights/pallet_utility.rs | 4 ++++ frame/support/src/lib.rs | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/bin/node/runtime/src/weights/pallet_utility.rs b/bin/node/runtime/src/weights/pallet_utility.rs index 2c508ed6cd42d..73f49459610c9 100644 --- a/bin/node/runtime/src/weights/pallet_utility.rs +++ b/bin/node/runtime/src/weights/pallet_utility.rs @@ -33,4 +33,8 @@ impl pallet_utility::WeightInfo for WeightInfo { fn as_derivative() -> Weight { (4086000 as Weight) } + fn batch_all(c: u32, ) -> Weight { + (16461000 as Weight) + .saturating_add((1982000 as Weight).saturating_mul(c as Weight)) + } } diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index 6c0dad0c9f73a..532e462d26997 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -322,7 +322,7 @@ macro_rules! assert_err { /// Evaluate an expression, assert it returns an expected `Err` value and that /// runtime storage has not been mutated (i.e. expression is a no-operation). /// -/// Used as `assert_noop(expression_to_assert, expected_error_expression)`. +/// Used as `assert_noop_ignore_postinfo(expression_to_assert, expected_error_expression)`. /// /// This can be used on`DispatchResultWithPostInfo` when the post info should /// be ignored. From 439e324d58557e1d3482c98cb741a3be46e8f33f Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 2 Oct 2020 04:24:22 +0200 Subject: [PATCH 6/8] weights --- bin/node/runtime/src/weights/pallet_utility.rs | 16 +++++++++------- frame/utility/src/default_weights.rs | 16 +++++++++------- frame/utility/src/tests.rs | 2 +- 3 files changed, 19 insertions(+), 15 deletions(-) diff --git a/bin/node/runtime/src/weights/pallet_utility.rs b/bin/node/runtime/src/weights/pallet_utility.rs index 73f49459610c9..618fce058721f 100644 --- a/bin/node/runtime/src/weights/pallet_utility.rs +++ b/bin/node/runtime/src/weights/pallet_utility.rs @@ -1,6 +1,6 @@ // This file is part of Substrate. -// Copyright (C) 2017-2020 Parity Technologies (UK) Ltd. +// Copyright (C) 2020 Parity Technologies (UK) Ltd. // SPDX-License-Identifier: Apache-2.0 // Licensed under the Apache License, Version 2.0 (the "License"); @@ -15,7 +15,9 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0-rc5 +//! Weights for pallet_utility +//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0 +//! DATE: 2020-10-02, STEPS: [50], REPEAT: 20, LOW RANGE: [], HIGH RANGE: [] #![allow(unused_parens)] #![allow(unused_imports)] @@ -26,15 +28,15 @@ use sp_std::marker::PhantomData; pub struct WeightInfo(PhantomData); impl pallet_utility::WeightInfo for WeightInfo { fn batch(c: u32, ) -> Weight { - (16461000 as Weight) - .saturating_add((1982000 as Weight).saturating_mul(c as Weight)) + (20_803_000 as Weight) + .saturating_add((1_984_000 as Weight).saturating_mul(c as Weight)) } // WARNING! Some components were not used: ["u"] fn as_derivative() -> Weight { - (4086000 as Weight) + (5_853_000 as Weight) } fn batch_all(c: u32, ) -> Weight { - (16461000 as Weight) - .saturating_add((1982000 as Weight).saturating_mul(c as Weight)) + (21_104_000 as Weight) + .saturating_add((1_509_000 as Weight).saturating_mul(c as Weight)) } } diff --git a/frame/utility/src/default_weights.rs b/frame/utility/src/default_weights.rs index 903c542f0db47..efc1271210fd4 100644 --- a/frame/utility/src/default_weights.rs +++ b/frame/utility/src/default_weights.rs @@ -1,6 +1,6 @@ // This file is part of Substrate. -// Copyright (C) 2019-2020 Parity Technologies (UK) Ltd. +// Copyright (C) 2020 Parity Technologies (UK) Ltd. // SPDX-License-Identifier: Apache-2.0 // Licensed under the Apache License, Version 2.0 (the "License"); @@ -15,7 +15,9 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0-rc5 +//! Weights for pallet_utility +//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0 +//! DATE: 2020-10-02, STEPS: [50], REPEAT: 20, LOW RANGE: [], HIGH RANGE: [] #![allow(unused_parens)] #![allow(unused_imports)] @@ -24,15 +26,15 @@ use frame_support::weights::{Weight, constants::RocksDbWeight as DbWeight}; impl crate::WeightInfo for () { fn batch(c: u32, ) -> Weight { - (16461000 as Weight) - .saturating_add((1982000 as Weight).saturating_mul(c as Weight)) + (20_803_000 as Weight) + .saturating_add((1_984_000 as Weight).saturating_mul(c as Weight)) } // WARNING! Some components were not used: ["u"] fn as_derivative() -> Weight { - (4086000 as Weight) + (5_853_000 as Weight) } fn batch_all(c: u32, ) -> Weight { - (16461000 as Weight) - .saturating_add((1982000 as Weight).saturating_mul(c as Weight)) + (21_104_000 as Weight) + .saturating_add((1_509_000 as Weight).saturating_mul(c as Weight)) } } diff --git a/frame/utility/src/tests.rs b/frame/utility/src/tests.rs index 58284913e3030..957043013656b 100644 --- a/frame/utility/src/tests.rs +++ b/frame/utility/src/tests.rs @@ -465,7 +465,7 @@ fn batch_all_revert() { ]), DispatchErrorWithPostInfo { post_info: PostDispatchInfo { - actual_weight: Some(::WeightInfo::batch(2) + info.weight * 2), + actual_weight: Some(::WeightInfo::batch_all(2) + info.weight * 2), pays_fee: Pays::Yes }, error: pallet_balances::Error::::InsufficientBalance.into() From fc8249334bfdaaefd23ce952fb6f694b43753407 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 2 Oct 2020 04:32:22 +0200 Subject: [PATCH 7/8] assert_noop_ignore_postinfo doesnt make sense --- frame/support/src/lib.rs | 20 -------------------- frame/utility/src/tests.rs | 12 ++++++------ 2 files changed, 6 insertions(+), 26 deletions(-) diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index 532e462d26997..bdbdfc04a31f9 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -319,26 +319,6 @@ macro_rules! assert_err { } } -/// Evaluate an expression, assert it returns an expected `Err` value and that -/// runtime storage has not been mutated (i.e. expression is a no-operation). -/// -/// Used as `assert_noop_ignore_postinfo(expression_to_assert, expected_error_expression)`. -/// -/// This can be used on`DispatchResultWithPostInfo` when the post info should -/// be ignored. -#[macro_export] -#[cfg(feature = "std")] -macro_rules! assert_noop_ignore_postinfo { - ( - $x:expr, - $y:expr $(,)? - ) => { - let h = $crate::storage_root(); - $crate::assert_err_ignore_postinfo!($x, $y); - assert_eq!(h, $crate::storage_root()); - } -} - /// Assert an expression returns an error specified. /// /// This can be used on`DispatchResultWithPostInfo` when the post info should diff --git a/frame/utility/src/tests.rs b/frame/utility/src/tests.rs index 957043013656b..a3c33bdf2081f 100644 --- a/frame/utility/src/tests.rs +++ b/frame/utility/src/tests.rs @@ -23,7 +23,7 @@ use super::*; use frame_support::{ assert_ok, assert_noop, impl_outer_origin, parameter_types, impl_outer_dispatch, impl_outer_event, - assert_noop_ignore_postinfo, + assert_err_ignore_postinfo, weights::{Weight, Pays}, dispatch::{DispatchError, DispatchErrorWithPostInfo, Dispatchable}, traits::Filter, @@ -198,7 +198,7 @@ fn as_derivative_works() { new_test_ext().execute_with(|| { let sub_1_0 = Utility::derivative_account_id(1, 0); assert_ok!(Balances::transfer(Origin::signed(1), sub_1_0, 5)); - assert_noop_ignore_postinfo!(Utility::as_derivative( + assert_err_ignore_postinfo!(Utility::as_derivative( Origin::signed(1), 1, Box::new(Call::Balances(BalancesCall::transfer(6, 3))), @@ -276,7 +276,7 @@ fn as_derivative_handles_weight_refund() { #[test] fn as_derivative_filters() { new_test_ext().execute_with(|| { - assert_noop_ignore_postinfo!(Utility::as_derivative( + assert_err_ignore_postinfo!(Utility::as_derivative( Origin::signed(1), 1, Box::new(Call::System(frame_system::Call::suicide())), @@ -510,7 +510,7 @@ fn batch_all_handles_weight_refund() { let call = Call::Utility(UtilityCall::batch_all(batch_calls)); let info = call.get_dispatch_info(); let result = call.dispatch(Origin::signed(1)); - assert_noop_ignore_postinfo!(result, "The cake is a lie."); + assert_err_ignore_postinfo!(result, "The cake is a lie."); // No weight is refunded assert_eq!(extract_actual_weight(&result, &info), info.weight); @@ -522,7 +522,7 @@ fn batch_all_handles_weight_refund() { let call = Call::Utility(UtilityCall::batch_all(batch_calls)); let info = call.get_dispatch_info(); let result = call.dispatch(Origin::signed(1)); - assert_noop_ignore_postinfo!(result, "The cake is a lie."); + assert_err_ignore_postinfo!(result, "The cake is a lie."); assert_eq!(extract_actual_weight(&result, &info), info.weight - diff * batch_len); // Partial batch completion @@ -532,7 +532,7 @@ fn batch_all_handles_weight_refund() { let call = Call::Utility(UtilityCall::batch_all(batch_calls)); let info = call.get_dispatch_info(); let result = call.dispatch(Origin::signed(1)); - assert_noop_ignore_postinfo!(result, "The cake is a lie."); + assert_err_ignore_postinfo!(result, "The cake is a lie."); assert_eq!( extract_actual_weight(&result, &info), // Real weight is 2 calls at end_weight From ad73228b84012c41f7184a39c8ffa14c3ab326f2 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 2 Oct 2020 14:48:52 +0200 Subject: [PATCH 8/8] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- frame/utility/src/lib.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/frame/utility/src/lib.rs b/frame/utility/src/lib.rs index 3b86a3d0cd1f9..3aa310c8acb74 100644 --- a/frame/utility/src/lib.rs +++ b/frame/utility/src/lib.rs @@ -179,7 +179,7 @@ decl_module! { } Self::deposit_event(Event::BatchCompleted); let base_weight = T::WeightInfo::batch(calls_len as u32); - return Ok(Some(base_weight + weight).into()); + Ok(Some(base_weight + weight).into()) } /// Send a call through an indexed pseudonym of the sender. @@ -216,8 +216,7 @@ decl_module! { result.map_err(|mut err| { err.post_info = Some(weight).into(); err - })?; - Ok(Some(weight).into()) + }).map(|_| Some(weight).into()) } /// Send a batch of dispatch calls and atomically execute them.