From b8509ed98c36f240af89e158ae3e639c081b32ca Mon Sep 17 00:00:00 2001 From: bgallois Date: Wed, 24 Jan 2024 16:39:41 +0100 Subject: [PATCH 1/6] fix treasury benchmarks when no SpendOrigin --- substrate/frame/treasury/src/benchmarking.rs | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/substrate/frame/treasury/src/benchmarking.rs b/substrate/frame/treasury/src/benchmarking.rs index 0b9999e37fbe..4b492b26a234 100644 --- a/substrate/frame/treasury/src/benchmarking.rs +++ b/substrate/frame/treasury/src/benchmarking.rs @@ -215,6 +215,8 @@ mod benchmarks { Ok(()) } + // This benchmark is short-circuited if `SpendOrigin` cannot provide + // a successful origin, in which case `spend` is un-callable and can use weight=0. #[benchmark] fn spend() -> Result<(), BenchmarkError> { let origin = @@ -248,9 +250,13 @@ mod benchmarks { Ok(()) } + // This benchmark is short-circuited if `SpendOrigin` cannot provide + // a successful origin, in which case there is no spend to payout + // and `payout` is un-callable and can use weight=0. #[benchmark] fn payout() -> Result<(), BenchmarkError> { - let origin = T::SpendOrigin::try_successful_origin().map_err(|_| "No origin")?; + let origin = + T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; let (asset_kind, amount, beneficiary, beneficiary_lookup) = create_spend_arguments::(SEED); T::BalanceConverter::ensure_successful(asset_kind.clone()); @@ -279,9 +285,13 @@ mod benchmarks { Ok(()) } + // This benchmark is short-circuited if `SpendOrigin` cannot provide + // a successful origin, in which case there is no spend to check + // and `check_status` is un-callable and can use weight=0. #[benchmark] fn check_status() -> Result<(), BenchmarkError> { - let origin = T::SpendOrigin::try_successful_origin().map_err(|_| "No origin")?; + let origin = + T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; let (asset_kind, amount, beneficiary, beneficiary_lookup) = create_spend_arguments::(SEED); T::BalanceConverter::ensure_successful(asset_kind.clone()); @@ -311,9 +321,13 @@ mod benchmarks { Ok(()) } + // This benchmark is short-circuited if `SpendOrigin` cannot provide + // a successful origin, in which case there is no spend to void + // and `void_spend` is un-callable and can use weight=0. #[benchmark] fn void_spend() -> Result<(), BenchmarkError> { - let origin = T::SpendOrigin::try_successful_origin().map_err(|_| "No origin")?; + let origin = + T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; let (asset_kind, amount, _, beneficiary_lookup) = create_spend_arguments::(SEED); T::BalanceConverter::ensure_successful(asset_kind.clone()); Treasury::::spend( From f587bf100d29b1d098ba2189ddf338710d8d4d54 Mon Sep 17 00:00:00 2001 From: bgallois Date: Sat, 27 Jan 2024 15:53:54 +0100 Subject: [PATCH 2/6] fix comments --- substrate/frame/treasury/src/benchmarking.rs | 26 ++++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/substrate/frame/treasury/src/benchmarking.rs b/substrate/frame/treasury/src/benchmarking.rs index 4b492b26a234..d73e9889c7fe 100644 --- a/substrate/frame/treasury/src/benchmarking.rs +++ b/substrate/frame/treasury/src/benchmarking.rs @@ -108,8 +108,8 @@ fn create_spend_arguments, I: 'static>( mod benchmarks { use super::*; - // This benchmark is short-circuited if `SpendOrigin` cannot provide - // a successful origin, in which case `spend` is un-callable and can use weight=0. + /// This benchmark is short-circuited if `SpendOrigin` cannot provide + /// a successful origin, in which case `spend` is un-callable and can use weight=0. #[benchmark] fn spend_local() -> Result<(), BenchmarkError> { let (_, value, beneficiary_lookup) = setup_proposal::(SEED); @@ -215,8 +215,8 @@ mod benchmarks { Ok(()) } - // This benchmark is short-circuited if `SpendOrigin` cannot provide - // a successful origin, in which case `spend` is un-callable and can use weight=0. + /// This benchmark is short-circuited if `SpendOrigin` cannot provide + /// a successful origin, in which case `spend` is un-callable and can use weight=0. #[benchmark] fn spend() -> Result<(), BenchmarkError> { let origin = @@ -250,9 +250,9 @@ mod benchmarks { Ok(()) } - // This benchmark is short-circuited if `SpendOrigin` cannot provide - // a successful origin, in which case there is no spend to payout - // and `payout` is un-callable and can use weight=0. + /// This benchmark is short-circuited if `SpendOrigin` cannot provide + /// a successful origin, in which case there is no spend to payout + /// and `payout` is un-callable and can use weight=0. #[benchmark] fn payout() -> Result<(), BenchmarkError> { let origin = @@ -285,9 +285,9 @@ mod benchmarks { Ok(()) } - // This benchmark is short-circuited if `SpendOrigin` cannot provide - // a successful origin, in which case there is no spend to check - // and `check_status` is un-callable and can use weight=0. + /// This benchmark is short-circuited if `SpendOrigin` cannot provide + /// a successful origin, in which case there is no spend to check + /// and `check_status` is un-callable and can use weight=0. #[benchmark] fn check_status() -> Result<(), BenchmarkError> { let origin = @@ -321,9 +321,9 @@ mod benchmarks { Ok(()) } - // This benchmark is short-circuited if `SpendOrigin` cannot provide - // a successful origin, in which case there is no spend to void - // and `void_spend` is un-callable and can use weight=0. + /// This benchmark is short-circuited if `SpendOrigin` cannot provide + /// a successful origin, in which case there is no spend to void + /// and `void_spend` is un-callable and can use weight=0. #[benchmark] fn void_spend() -> Result<(), BenchmarkError> { let origin = From 08e6bd74cc6083f4a6ec1bf62f18ed193b3f0b59 Mon Sep 17 00:00:00 2001 From: bgallois Date: Sun, 28 Jan 2024 16:48:35 +0100 Subject: [PATCH 3/6] fix benchmarks --- substrate/frame/treasury/src/benchmarking.rs | 54 +++++++++++--------- 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/substrate/frame/treasury/src/benchmarking.rs b/substrate/frame/treasury/src/benchmarking.rs index d73e9889c7fe..e4fbbcafd872 100644 --- a/substrate/frame/treasury/src/benchmarking.rs +++ b/substrate/frame/treasury/src/benchmarking.rs @@ -104,6 +104,33 @@ fn create_spend_arguments, I: 'static>( (asset_kind, 100u32.into(), beneficiary, beneficiary_lookup) } +// Use this `spend` insertion to bypass the `SpendOrigin` check. This is helpful +// for benchmarking `payout`, `check_status`, and `void_spend`, which require +// a spend in storage even if `SpendOrigin` cannot provide a successful origin. +fn force_add_spend, I: 'static>( + asset_kind: Box, + amount: AssetBalanceOf, + beneficiary: Box>, +) -> Result<(), &'static str> { + let valid_from = frame_system::Pallet::::block_number(); + let expire_at = valid_from.saturating_add(T::PayoutPeriod::get()); + let beneficiary = T::BeneficiaryLookup::lookup(*beneficiary)?; + let index = SpendCount::::get(); + Spends::::insert( + index, + SpendStatus { + asset_kind: *asset_kind.clone(), + amount, + beneficiary: beneficiary.clone(), + valid_from, + expire_at, + status: PaymentState::Pending, + }, + ); + SpendCount::::put(index + 1); + Ok(()) +} + #[instance_benchmarks] mod benchmarks { use super::*; @@ -250,22 +277,15 @@ mod benchmarks { Ok(()) } - /// This benchmark is short-circuited if `SpendOrigin` cannot provide - /// a successful origin, in which case there is no spend to payout - /// and `payout` is un-callable and can use weight=0. #[benchmark] fn payout() -> Result<(), BenchmarkError> { - let origin = - T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; let (asset_kind, amount, beneficiary, beneficiary_lookup) = create_spend_arguments::(SEED); T::BalanceConverter::ensure_successful(asset_kind.clone()); - Treasury::::spend( - origin, + force_add_spend::( Box::new(asset_kind.clone()), amount, Box::new(beneficiary_lookup), - None, )?; T::Paymaster::ensure_successful(&beneficiary, asset_kind, amount); let caller: T::AccountId = account("caller", 0, SEED); @@ -285,22 +305,15 @@ mod benchmarks { Ok(()) } - /// This benchmark is short-circuited if `SpendOrigin` cannot provide - /// a successful origin, in which case there is no spend to check - /// and `check_status` is un-callable and can use weight=0. #[benchmark] fn check_status() -> Result<(), BenchmarkError> { - let origin = - T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; let (asset_kind, amount, beneficiary, beneficiary_lookup) = create_spend_arguments::(SEED); T::BalanceConverter::ensure_successful(asset_kind.clone()); - Treasury::::spend( - origin, + force_add_spend::( Box::new(asset_kind.clone()), amount, Box::new(beneficiary_lookup), - None, )?; T::Paymaster::ensure_successful(&beneficiary, asset_kind, amount); let caller: T::AccountId = account("caller", 0, SEED); @@ -321,21 +334,14 @@ mod benchmarks { Ok(()) } - /// This benchmark is short-circuited if `SpendOrigin` cannot provide - /// a successful origin, in which case there is no spend to void - /// and `void_spend` is un-callable and can use weight=0. #[benchmark] fn void_spend() -> Result<(), BenchmarkError> { - let origin = - T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; let (asset_kind, amount, _, beneficiary_lookup) = create_spend_arguments::(SEED); T::BalanceConverter::ensure_successful(asset_kind.clone()); - Treasury::::spend( - origin, + force_add_spend::( Box::new(asset_kind.clone()), amount, Box::new(beneficiary_lookup), - None, )?; assert!(Spends::::get(0).is_some()); let origin = From 042159ace0245e5ade8a9cc6c7550be92251cacb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 13 Sep 2024 15:33:45 +0200 Subject: [PATCH 4/6] Fix benchmarks --- substrate/frame/treasury/src/benchmarking.rs | 177 ++++++++++++------- substrate/frame/treasury/src/tests.rs | 19 +- 2 files changed, 128 insertions(+), 68 deletions(-) diff --git a/substrate/frame/treasury/src/benchmarking.rs b/substrate/frame/treasury/src/benchmarking.rs index e4fbbcafd872..e6f715f6ccf6 100644 --- a/substrate/frame/treasury/src/benchmarking.rs +++ b/substrate/frame/treasury/src/benchmarking.rs @@ -26,7 +26,7 @@ use frame_benchmarking::{ v2::*, }; use frame_support::{ - ensure, + assert_err, assert_ok, ensure, traits::{ tokens::{ConversionFromAssetBalance, PaymentStatus}, EnsureOrigin, OnInitialize, @@ -104,33 +104,6 @@ fn create_spend_arguments, I: 'static>( (asset_kind, 100u32.into(), beneficiary, beneficiary_lookup) } -// Use this `spend` insertion to bypass the `SpendOrigin` check. This is helpful -// for benchmarking `payout`, `check_status`, and `void_spend`, which require -// a spend in storage even if `SpendOrigin` cannot provide a successful origin. -fn force_add_spend, I: 'static>( - asset_kind: Box, - amount: AssetBalanceOf, - beneficiary: Box>, -) -> Result<(), &'static str> { - let valid_from = frame_system::Pallet::::block_number(); - let expire_at = valid_from.saturating_add(T::PayoutPeriod::get()); - let beneficiary = T::BeneficiaryLookup::lookup(*beneficiary)?; - let index = SpendCount::::get(); - Spends::::insert( - index, - SpendStatus { - asset_kind: *asset_kind.clone(), - amount, - beneficiary: beneficiary.clone(), - valid_from, - expire_at, - status: PaymentState::Pending, - }, - ); - SpendCount::::put(index + 1); - Ok(()) -} - #[instance_benchmarks] mod benchmarks { use super::*; @@ -282,26 +255,47 @@ mod benchmarks { let (asset_kind, amount, beneficiary, beneficiary_lookup) = create_spend_arguments::(SEED); T::BalanceConverter::ensure_successful(asset_kind.clone()); - force_add_spend::( - Box::new(asset_kind.clone()), - amount, - Box::new(beneficiary_lookup), - )?; + + let spend_exists = if let Ok(origin) = T::SpendOrigin::try_successful_origin() { + Treasury::::spend( + origin, + Box::new(asset_kind.clone()), + amount, + Box::new(beneficiary_lookup), + None, + )?; + + true + } else { + false + }; + T::Paymaster::ensure_successful(&beneficiary, asset_kind, amount); let caller: T::AccountId = account("caller", 0, SEED); - #[extrinsic_call] - _(RawOrigin::Signed(caller.clone()), 0u32); - - let id = match Spends::::get(0).unwrap().status { - PaymentState::Attempted { id, .. } => { - assert_ne!(T::Paymaster::check_payment(id), PaymentStatus::Failure); - id - }, - _ => panic!("No payout attempt made"), - }; - assert_last_event::(Event::Paid { index: 0, payment_id: id }.into()); - assert!(Treasury::::payout(RawOrigin::Signed(caller).into(), 0u32).is_err()); + #[block] + { + let res = Treasury::::payout(RawOrigin::Signed(caller.clone()).into(), 0u32); + + if spend_exists { + assert_ok!(res); + } else { + assert_err!(res, crate::Error::::InvalidIndex); + } + } + + if spend_exists { + let id = match Spends::::get(0).unwrap().status { + PaymentState::Attempted { id, .. } => { + assert_ne!(T::Paymaster::check_payment(id), PaymentStatus::Failure); + id + }, + _ => panic!("No payout attempt made"), + }; + assert_last_event::(Event::Paid { index: 0, payment_id: id }.into()); + assert!(Treasury::::payout(RawOrigin::Signed(caller).into(), 0u32).is_err()); + } + Ok(()) } @@ -309,28 +303,49 @@ mod benchmarks { fn check_status() -> Result<(), BenchmarkError> { let (asset_kind, amount, beneficiary, beneficiary_lookup) = create_spend_arguments::(SEED); + T::BalanceConverter::ensure_successful(asset_kind.clone()); - force_add_spend::( - Box::new(asset_kind.clone()), - amount, - Box::new(beneficiary_lookup), - )?; - T::Paymaster::ensure_successful(&beneficiary, asset_kind, amount); + T::Paymaster::ensure_successful(&beneficiary, asset_kind.clone(), amount); let caller: T::AccountId = account("caller", 0, SEED); - Treasury::::payout(RawOrigin::Signed(caller.clone()).into(), 0u32)?; - match Spends::::get(0).unwrap().status { - PaymentState::Attempted { id, .. } => { - T::Paymaster::ensure_concluded(id); - }, - _ => panic!("No payout attempt made"), + + let spend_exists = if let Ok(origin) = T::SpendOrigin::try_successful_origin() { + Treasury::::spend( + origin, + Box::new(asset_kind), + amount, + Box::new(beneficiary_lookup), + None, + )?; + + Treasury::::payout(RawOrigin::Signed(caller.clone()).into(), 0u32)?; + match Spends::::get(0).unwrap().status { + PaymentState::Attempted { id, .. } => { + T::Paymaster::ensure_concluded(id); + }, + _ => panic!("No payout attempt made"), + }; + + true + } else { + false }; - #[extrinsic_call] - _(RawOrigin::Signed(caller.clone()), 0u32); + #[block] + { + let res = + Treasury::::check_status(RawOrigin::Signed(caller.clone()).into(), 0u32); + + if spend_exists { + assert_ok!(res); + } else { + assert_err!(res, crate::Error::::InvalidIndex); + } + } if let Some(s) = Spends::::get(0) { assert!(!matches!(s.status, PaymentState::Attempted { .. })); } + Ok(()) } @@ -338,17 +353,34 @@ mod benchmarks { fn void_spend() -> Result<(), BenchmarkError> { let (asset_kind, amount, _, beneficiary_lookup) = create_spend_arguments::(SEED); T::BalanceConverter::ensure_successful(asset_kind.clone()); - force_add_spend::( - Box::new(asset_kind.clone()), - amount, - Box::new(beneficiary_lookup), - )?; - assert!(Spends::::get(0).is_some()); + let spend_exists = if let Ok(origin) = T::SpendOrigin::try_successful_origin() { + Treasury::::spend( + origin, + Box::new(asset_kind.clone()), + amount, + Box::new(beneficiary_lookup), + None, + )?; + assert!(Spends::::get(0).is_some()); + + true + } else { + false + }; + let origin = T::RejectOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; - #[extrinsic_call] - _(origin as T::RuntimeOrigin, 0u32); + #[block] + { + let res = Treasury::::void_spend(origin as T::RuntimeOrigin, 0u32); + + if spend_exists { + assert_ok!(res); + } else { + assert_err!(res, crate::Error::::InvalidIndex); + } + } assert!(Spends::::get(0).is_none()); Ok(()) @@ -359,4 +391,15 @@ mod benchmarks { crate::tests::ExtBuilder::default().build(), crate::tests::Test ); + + mod no_spend_origin_tests { + use super::*; + + impl_benchmark_test_suite!( + Treasury, + crate::tests::ExtBuilder::default().spend_origin_succesful_origin_err().build(), + crate::tests::Test, + benchmarks_path = benchmarking + ); + } } diff --git a/substrate/frame/treasury/src/tests.rs b/substrate/frame/treasury/src/tests.rs index 602159262e46..4e3ba1f6c4ed 100644 --- a/substrate/frame/treasury/src/tests.rs +++ b/substrate/frame/treasury/src/tests.rs @@ -108,6 +108,9 @@ thread_local! { pub static PAID: RefCell> = RefCell::new(BTreeMap::new()); pub static STATUS: RefCell> = RefCell::new(BTreeMap::new()); pub static LAST_ID: RefCell = RefCell::new(0u64); + + #[cfg(feature = "runtime-benchmarks")] + pub static TEST_SPEND_ORIGIN_TRY_SUCCESFUL_ORIGIN_ERR: RefCell = RefCell::new(false); } /// paid balance for a given account and asset ids @@ -163,6 +166,7 @@ parameter_types! { pub TreasuryAccount: u128 = Treasury::account_id(); pub const SpendPayoutPeriod: u64 = 5; } + pub struct TestSpendOrigin; impl frame_support::traits::EnsureOrigin for TestSpendOrigin { type Success = u64; @@ -178,7 +182,11 @@ impl frame_support::traits::EnsureOrigin for TestSpendOrigin { } #[cfg(feature = "runtime-benchmarks")] fn try_successful_origin() -> Result { - Ok(RuntimeOrigin::root()) + if TEST_SPEND_ORIGIN_TRY_SUCCESFUL_ORIGIN_ERR.with(|i| *i.borrow()) { + Err(()) + } else { + Ok(frame_system::RawOrigin::Root.into()) + } } } @@ -223,11 +231,20 @@ pub struct ExtBuilder {} impl Default for ExtBuilder { fn default() -> Self { + #[cfg(feature = "runtime-benchmarks")] + TEST_SPEND_ORIGIN_TRY_SUCCESFUL_ORIGIN_ERR.with(|i| *i.borrow_mut() = false); + Self {} } } impl ExtBuilder { + #[cfg(feature = "runtime-benchmarks")] + pub fn spend_origin_succesful_origin_err(self) -> Self { + TEST_SPEND_ORIGIN_TRY_SUCCESFUL_ORIGIN_ERR.with(|i| *i.borrow_mut() = true); + self + } + pub fn build(self) -> sp_io::TestExternalities { let mut t = frame_system::GenesisConfig::::default().build_storage().unwrap(); pallet_balances::GenesisConfig:: { From 6d28fc9c83afe7ce47174e7972b2a7e3d728b69a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 13 Sep 2024 15:41:20 +0200 Subject: [PATCH 5/6] PRDOC --- prdoc/pr_3049.prdoc | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 prdoc/pr_3049.prdoc diff --git a/prdoc/pr_3049.prdoc b/prdoc/pr_3049.prdoc new file mode 100644 index 000000000000..9cead8e2a4e5 --- /dev/null +++ b/prdoc/pr_3049.prdoc @@ -0,0 +1,11 @@ +title: "Fix treasury benchmarks when `SpendOrigin` being `None`" + +doc: + - audience: Runtime Dev + description: | + Fix treasury benchmarks when `SpendOrigin` not returning any succesful origin. + This is for example the case when `SpendOrigin` is set to `NeverOrigin`. + +crates: + - name: pallet-treasury + bump: patch From 44fbb395176b187f51cebb572816702a1fc66187 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 13 Sep 2024 17:36:05 +0200 Subject: [PATCH 6/6] Fix on_init bench Signed-off-by: Oliver Tale-Yazdi --- substrate/frame/treasury/src/benchmarking.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/substrate/frame/treasury/src/benchmarking.rs b/substrate/frame/treasury/src/benchmarking.rs index 4a1cd047451a..0bac78503f41 100644 --- a/substrate/frame/treasury/src/benchmarking.rs +++ b/substrate/frame/treasury/src/benchmarking.rs @@ -73,12 +73,19 @@ fn setup_proposal, I: 'static>( // Create proposals that are approved for use in `on_initialize`. fn create_approved_proposals, I: 'static>(n: u32) -> Result<(), &'static str> { - let origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; + let spender = T::SpendOrigin::try_successful_origin(); + for i in 0..n { let (_, value, lookup) = setup_proposal::(i); - Treasury::::spend_local(origin.clone(), value, lookup)?; + + if let Ok(origin) = &spender { + Treasury::::spend_local(origin.clone(), value, lookup)?; + } + } + + if spender.is_ok() { + ensure!(Approvals::::get().len() == n as usize, "Not all approved"); } - ensure!(Approvals::::get().len() == n as usize, "Not all approved"); Ok(()) }