Skip to content
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

Fix benchmark failures when using insecure_zero_ed flag #5354

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions prdoc/pr_5354.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# 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: Fix benchmark failures when using insecure_zero_ed flag

doc:
- audience: Runtime Dev
description: |
Currently, when the pallet is compiled with the insecure_zero_ed flag, benchmarks fail because the minimum balance is set to zero.

The PR aims to resolve this issue by implementing a placeholder value for the minimum balance when the insecure_zero_ed flag is active. it ensures that benchmarks run successfully regardless of whether this flag is used or not

crates:
- name: pallet-balances
bump: minor
30 changes: 19 additions & 11 deletions substrate/frame/balances/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ const SEED: u32 = 0;
// existential deposit multiplier
const ED_MULTIPLIER: u32 = 10;

fn minimum_balance<T: Config<I>, I: 'static>() -> T::Balance {
if cfg!(feature = "insecure_zero_ed") {
100u32.into()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really correct, min balance is 0 in this case. I would rather fix specifically failing bench/es. this might lead to a wrong benchmark at least with insecure_zero_ed feature enabled

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check all of them, but most of them are just using this to set some balance or whatever. I don't think there is any problem in using 100 if the ED is zero.

} else {
T::ExistentialDeposit::get()
}
}

#[instance_benchmarks]
mod benchmarks {
use super::*;
Expand All @@ -40,7 +48,7 @@ mod benchmarks {
// * Transfer will create the recipient account.
#[benchmark]
fn transfer_allow_death() {
let existential_deposit = T::ExistentialDeposit::get();
let existential_deposit: T::Balance = minimum_balance::<T, I>();
let caller = whitelisted_caller();

// Give some multiple of the existential deposit
Expand Down Expand Up @@ -75,7 +83,7 @@ mod benchmarks {
<Balances<T, I> as Currency<_>>::make_free_balance_be(&caller, T::Balance::max_value());

// Give the recipient account existential deposit (thus their account already exists).
let existential_deposit = T::ExistentialDeposit::get();
let existential_deposit: T::Balance = minimum_balance::<T, I>();
let _ =
<Balances<T, I> as Currency<_>>::make_free_balance_be(&recipient, existential_deposit);
let transfer_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
Expand All @@ -98,7 +106,7 @@ mod benchmarks {
// Give the sender account max funds, thus a transfer will not kill account.
let _ =
<Balances<T, I> as Currency<_>>::make_free_balance_be(&caller, T::Balance::max_value());
let existential_deposit = T::ExistentialDeposit::get();
let existential_deposit: T::Balance = minimum_balance::<T, I>();
let transfer_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into());

#[extrinsic_call]
Expand All @@ -115,7 +123,7 @@ mod benchmarks {
let user_lookup = T::Lookup::unlookup(user.clone());

// Give the user some initial balance.
let existential_deposit = T::ExistentialDeposit::get();
let existential_deposit: T::Balance = minimum_balance::<T, I>();
let balance_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&user, balance_amount);

Expand All @@ -132,7 +140,7 @@ mod benchmarks {
let user_lookup = T::Lookup::unlookup(user.clone());

// Give the user some initial balance.
let existential_deposit = T::ExistentialDeposit::get();
let existential_deposit: T::Balance = minimum_balance::<T, I>();
let balance_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&user, balance_amount);

Expand All @@ -147,7 +155,7 @@ mod benchmarks {
// * Transfer will create the recipient account.
#[benchmark]
fn force_transfer() {
let existential_deposit = T::ExistentialDeposit::get();
let existential_deposit: T::Balance = minimum_balance::<T, I>();
let source: T::AccountId = account("source", 0, SEED);
let source_lookup = T::Lookup::unlookup(source.clone());

Expand Down Expand Up @@ -175,7 +183,7 @@ mod benchmarks {
#[benchmark(extra)]
fn transfer_increasing_users(u: Linear<0, 1_000>) {
// 1_000 is not very much, but this upper bound can be controlled by the CLI.
let existential_deposit = T::ExistentialDeposit::get();
let existential_deposit: T::Balance = minimum_balance::<T, I>();
let caller = whitelisted_caller();

// Give some multiple of the existential deposit
Expand Down Expand Up @@ -214,7 +222,7 @@ mod benchmarks {
let recipient_lookup = T::Lookup::unlookup(recipient.clone());

// Give some multiple of the existential deposit
let existential_deposit = T::ExistentialDeposit::get();
let existential_deposit: T::Balance = minimum_balance::<T, I>();
let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&caller, balance);

Expand All @@ -231,7 +239,7 @@ mod benchmarks {
let user_lookup = T::Lookup::unlookup(user.clone());

// Give some multiple of the existential deposit
let ed = T::ExistentialDeposit::get();
let ed = minimum_balance::<T, I>();
let balance = ed + ed;
let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&user, balance);

Expand All @@ -257,8 +265,8 @@ mod benchmarks {
.map(|i| -> T::AccountId {
let user = account("old_user", i, SEED);
let account = AccountData {
free: T::ExistentialDeposit::get(),
reserved: T::ExistentialDeposit::get(),
free: minimum_balance::<T, I>(),
reserved: minimum_balance::<T, I>(),
frozen: Zero::zero(),
flags: ExtraFlags::old_logic(),
};
Expand Down
Loading