-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Improve charge_transaction_payment benchmark ergonomics
#10444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 | ||
| # See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json | ||
|
|
||
| title: Improve `charge_transaction_payment benchmark` ergonomics | ||
|
|
||
| doc: | ||
| - audience: Runtime Dev | ||
| description: | | ||
| Adds a `setup_benchmark_environment()` hook to allow runtimes to configure | ||
| required state before running the benchmark (e.g., setting block author for | ||
| fee distribution). Also fixes `amount_to_endow` calculation to use actual | ||
| computed fee and ensure it meets the existential deposit. | ||
|
|
||
| crates: | ||
| - name: pallet-transaction-payment | ||
| bump: patch |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,13 +20,29 @@ | |
| extern crate alloc; | ||
|
|
||
| use super::*; | ||
| use crate::Pallet; | ||
| use frame_benchmarking::v2::*; | ||
| use frame_support::dispatch::{DispatchInfo, PostDispatchInfo}; | ||
| use frame_system::{EventRecord, RawOrigin}; | ||
| use sp_runtime::traits::{AsTransactionAuthorizedOrigin, DispatchTransaction, Dispatchable}; | ||
|
|
||
| fn assert_last_event<T: Config>(generic_event: <T as Config>::RuntimeEvent) { | ||
| /// Re-export the pallet for benchmarking with custom Config trait. | ||
| pub struct Pallet<T: Config>(crate::Pallet<T>); | ||
|
|
||
| /// Benchmark configuration trait. | ||
| /// | ||
| /// This extends the pallet's Config trait to allow runtimes to set up any | ||
| /// required state before running benchmarks. For example, runtimes that | ||
| /// distribute fees to block authors may need to set the author before | ||
| /// the benchmark runs. | ||
| pub trait Config: crate::Config { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we not just name this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follows a similar pattern as https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/pallet-xcm/src/benchmarking.rs#L27
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah could you try to just implement the benchmarking trait for the "normal Pallet"? Maybe it doesn't work, but right now I don't see the need for declaring this extra pallet struct.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The downside of this approach is that everyone would need to implement the new For instance we'd need to add the following to #[cfg(feature = "runtime-benchmarks")]
impl crate::benchmarking::Config for Runtime {} |
||
| /// Called at the start of each benchmark to set up any required state. | ||
| /// | ||
| /// The default implementation is a no-op. Runtimes can override this | ||
| /// to perform setup like setting the block author for fee distribution. | ||
| fn setup_benchmark_environment() {} | ||
| } | ||
|
|
||
| fn assert_last_event<T: crate::Config>(generic_event: <T as crate::Config>::RuntimeEvent) { | ||
| let events = frame_system::Pallet::<T>::events(); | ||
| let system_event: <T as frame_system::Config>::RuntimeEvent = generic_event.into(); | ||
| // compare to the last event record | ||
|
|
@@ -44,19 +60,17 @@ mod benchmarks { | |
|
|
||
| #[benchmark] | ||
| fn charge_transaction_payment() { | ||
| T::setup_benchmark_environment(); | ||
|
|
||
| let caller: T::AccountId = account("caller", 0, 0); | ||
| let existential_deposit = | ||
| <T::OnChargeTransaction as OnChargeTransaction<T>>::minimum_balance(); | ||
|
|
||
| let (amount_to_endow, tip) = if existential_deposit.is_zero() { | ||
| let min_tip: <<T as pallet::Config>::OnChargeTransaction as payment::OnChargeTransaction<T>>::Balance = 1_000_000_000u32.into(); | ||
| (min_tip * 1000u32.into(), min_tip) | ||
|
Comment on lines
-51
to
-53
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you change this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The amount to endow is computed dynamically later on, while the |
||
| } else { | ||
| (existential_deposit * 1000u32.into(), existential_deposit) | ||
| }; | ||
|
|
||
| <T::OnChargeTransaction as OnChargeTransaction<T>>::endow_account(&caller, amount_to_endow); | ||
| // Use a reasonable minimum tip that works for most runtimes | ||
| let min_tip: BalanceOf<T> = 1_000_000_000u32.into(); | ||
| let tip = if existential_deposit.is_zero() { min_tip } else { existential_deposit }; | ||
|
|
||
| // Build the call and dispatch info first so we can compute the actual fee | ||
| let ext: ChargeTransactionPayment<T> = ChargeTransactionPayment::from(tip); | ||
| let inner = frame_system::Call::remark { remark: alloc::vec![] }; | ||
| let call = T::RuntimeCall::from(inner); | ||
|
|
@@ -72,18 +86,32 @@ mod benchmarks { | |
| pays_fee: Pays::Yes, | ||
| }; | ||
|
|
||
| // Calculate the actual fee that will be charged, then endow enough to cover it | ||
| // with a 10x buffer to account for any fee multiplier variations. | ||
| // Ensure we endow at least the existential deposit so the account can exist. | ||
| let len: u32 = 10; | ||
| let expected_fee = crate::Pallet::<T>::compute_fee(len, &info, tip); | ||
| let amount_to_endow = expected_fee.max(existential_deposit).saturating_mul(10u32.into()); | ||
|
|
||
| <T::OnChargeTransaction as OnChargeTransaction<T>>::endow_account(&caller, amount_to_endow); | ||
|
|
||
| #[block] | ||
| { | ||
| assert!(ext | ||
| .test_run(RawOrigin::Signed(caller.clone()).into(), &call, &info, 10, 0, |_| Ok( | ||
| post_info | ||
| )) | ||
| .test_run( | ||
| RawOrigin::Signed(caller.clone()).into(), | ||
| &call, | ||
| &info, | ||
| len as usize, | ||
| 0, | ||
| |_| Ok(post_info) | ||
| ) | ||
| .unwrap() | ||
| .is_ok()); | ||
| } | ||
|
|
||
| post_info.actual_weight.as_mut().map(|w| w.saturating_accrue(extension_weight)); | ||
| let actual_fee = Pallet::<T>::compute_actual_fee(10, &info, &post_info, tip); | ||
| let actual_fee = crate::Pallet::<T>::compute_actual_fee(len, &info, &post_info, tip); | ||
| assert_last_event::<T>( | ||
| Event::<T>::TransactionFeePaid { who: caller, actual_fee, tip }.into(), | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,7 +78,7 @@ mod mock; | |
| mod tests; | ||
|
|
||
| #[cfg(feature = "runtime-benchmarks")] | ||
| mod benchmarking; | ||
| pub mod benchmarking; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably enough to just export the config, instead of making the entire module public
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly to https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/pallet-xcm/src/lib.rs#L22 but I am happy to change it |
||
|
|
||
| mod payment; | ||
| mod types; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can see and example use case here: moonbeam-foundation/moonbeam#3558