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.

Sign up for GitHub

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

Jump to bottom

fix benchmarks to handle zero existential deposit #67

Merged
merged 2 commits into from
Aug 18, 2023
Merged

fix benchmarks to handle zero existential deposit #67

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a0aef4
fix benchmarks to handle zero existential deposit
nbaztec Aug 17, 2023
67ed582
minimize changes
nbaztec Aug 18, 2023
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
34 changes: 17 additions & 17 deletions frame/balances/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ const SEED: u32 = 0;
// existential deposit multiplier
const ED_MULTIPLIER: u32 = 10;

const MINIMUM_BALANCE: u32 = 100;

#[instance_benchmarks]
mod benchmarks {
use super::*;
Expand All @@ -40,19 +42,18 @@ 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.into();
let caller = whitelisted_caller();

// Give some multiple of the existential deposit
let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
Copy link

Choose a reason for hiding this comment

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

why removing the multiplier? We can keep it

Copy link
Author

Choose a reason for hiding this comment

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

For this benchmark we need the account to be reaped so we can no longer transfer a part of the balance, rather the entire balance needs to be transferred so the benchmark case is respected, which doesn't really need the multiplier.

Copy link
Author

Choose a reason for hiding this comment

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

I can alternatively also make it to transfer the balance instead of the ED, if that's better.

Copy link

Choose a reason for hiding this comment

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

Even if the multiplier is useless, my point is to minimize the changes, change only what we strictly need to be able to run the benchmarks

Copy link
Author

Choose a reason for hiding this comment

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

Done.

let balance = existential_deposit;
let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&caller, balance);

// Transfer `e - 1` existential deposits + 1 unit, which guarantees to create one account,
// and reap this user.
let recipient: T::AccountId = account("recipient", 0, SEED);
let recipient_lookup = T::Lookup::unlookup(recipient.clone());
let transfer_amount =
existential_deposit.saturating_mul((ED_MULTIPLIER - 1).into()) + 1u32.into();
let transfer_amount = existential_deposit;

#[extrinsic_call]
_(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount);
Expand All @@ -75,7 +76,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.into();
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 +99,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.into();
let transfer_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into());

#[extrinsic_call]
Expand All @@ -115,7 +116,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.into();
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 +133,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.into();
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,20 +148,19 @@ 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.into();
let source: T::AccountId = account("source", 0, SEED);
let source_lookup = T::Lookup::unlookup(source.clone());

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

// Transfer `e - 1` existential deposits + 1 unit, which guarantees to create one account,
// and reap this user.
let recipient: T::AccountId = account("recipient", 0, SEED);
let recipient_lookup = T::Lookup::unlookup(recipient.clone());
let transfer_amount =
existential_deposit.saturating_mul((ED_MULTIPLIER - 1).into()) + 1u32.into();
let transfer_amount = existential_deposit;

#[extrinsic_call]
_(RawOrigin::Root, source_lookup, recipient_lookup, transfer_amount);
Expand All @@ -175,7 +175,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.into();
let caller = whitelisted_caller();

// Give some multiple of the existential deposit
Expand Down Expand Up @@ -214,7 +214,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.into();
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 +231,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.into();
let balance = ed + ed;
let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&user, balance);

Expand All @@ -257,8 +257,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.into(),
reserved: MINIMUM_BALANCE.into(),
frozen: Zero::zero(),
flags: ExtraFlags::old_logic(),
};
Expand Down
8 changes: 5 additions & 3 deletions frame/identity/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ use sp_runtime::traits::Bounded;

const SEED: u32 = 0;

const MINIMUM_BALANCE: u32 = 100;

fn assert_last_event<T: Config>(generic_event: <T as Config>::RuntimeEvent) {
frame_system::Pallet::<T>::assert_last_event(generic_event.into());
}
Expand Down Expand Up @@ -148,7 +150,7 @@ benchmarks! {
for i in 0..r {
let registrar: T::AccountId = account("registrar", i, SEED);
let registrar_lookup = T::Lookup::unlookup(registrar.clone());
let balance_to_use = T::Currency::minimum_balance() * 10u32.into();
let balance_to_use = (MINIMUM_BALANCE * 10u32).into();
let _ = T::Currency::make_free_balance_be(&registrar, balance_to_use);

Identity::<T>::request_judgement(caller_origin.clone(), i, 10u32.into())?;
Expand Down Expand Up @@ -221,7 +223,7 @@ benchmarks! {
// User requests judgement from all the registrars, and they approve
for i in 0..r {
let registrar: T::AccountId = account("registrar", i, SEED);
let balance_to_use = T::Currency::minimum_balance() * 10u32.into();
let balance_to_use = (MINIMUM_BALANCE * 10u32).into();
let _ = T::Currency::make_free_balance_be(&registrar, balance_to_use);

Identity::<T>::request_judgement(caller_origin.clone(), i, 10u32.into())?;
Expand Down Expand Up @@ -378,7 +380,7 @@ benchmarks! {
// User requests judgement from all the registrars, and they approve
for i in 0..r {
let registrar: T::AccountId = account("registrar", i, SEED);
let balance_to_use = T::Currency::minimum_balance() * 10u32.into();
let balance_to_use = (MINIMUM_BALANCE * 10u32).into();
let _ = T::Currency::make_free_balance_be(&registrar, balance_to_use);

Identity::<T>::request_judgement(target_origin.clone(), i, 10u32.into())?;
Expand Down
Loading