Skip to content

Add support for feature pallet_balances/insecure_zero_ed in benchmarks and testing #7379

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

Merged
bkchr merged 7 commits into from
Jan 29, 2025
Merged

Add support for feature pallet_balances/insecure_zero_ed in benchmarks and testing #7379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
260d204
feat: :sparkles: support feature insecure_zero_ed in benchmarks and t…
manuelmauro Jan 28, 2025
ca8b63c
test: :sparkles: add balance assertion for insecure_zero_ed case
manuelmauro Jan 28, 2025
438661a
docs: :memo: add prdoc
manuelmauro Jan 29, 2025
e5e5615
refactor: :recycle: use cfg! macro to check for insecure_zero_ed feature
manuelmauro Jan 28, 2025
b1cf78f
test: :white_check_mark: rely on minimum_balance() instead of Existen…
manuelmauro Jan 29, 2025
eacbcf6
fix benchmark when insecure_zero_ed is enabled
RomarQ Jan 29, 2025
cc26620
fix clippy error
RomarQ Jan 29, 2025
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
13 changes: 13 additions & 0 deletions prdoc/pr_7379.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# 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: "Add support for feature pallet_balances/insecure_zero_ed in benchmarks and testing"

doc:
- audience: Runtime Dev
description: |
Currently benchmarks and tests on pallet_balances would fail when the feature insecure_zero_ed is enabled. This PR allows to run such benchmark and tests keeping into account the fact that accounts would not be deleted when their balance goes below a threshold.

crates:
- name: pallet-balances
bump: patch
31 changes: 25 additions & 6 deletions substrate/frame/balances/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,12 @@ mod benchmarks {
#[extrinsic_call]
_(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount);

assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
if cfg!(feature = "insecure_zero_ed") {
assert_eq!(Balances::<T, I>::free_balance(&caller), balance - transfer_amount);
} else {
assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
}
Comment on lines +68 to +72
Copy link
Copy Markdown
Member

@bkchr bkchr Jan 29, 2025

Choose a reason for hiding this comment

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

These changes are not required, since this change: 1c4141a

Copy link
Copy Markdown
Contributor Author

@manuelmauro manuelmauro Jan 29, 2025

Choose a reason for hiding this comment

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

Unfortunately, the linked change was not enough as some benchmarks/tests rely on the side-effect of an account being removed as its balance goes below the ExistentialBalance threshold.

E.g., https://github.com/paritytech/polkadot-sdk/pull/7379/files/e5e561501393e4384bac52b2c8febae49591b1c9#diff-bbca4fed8efbc441432d5d1772f445f49b1f4a25e0920dbcd41ccba2dfbc6520L209-L213

To reproduce the issue:

cargo test -p pallet-balances --features runtime-benchmarks,insecure_zero_ed

Copy link
Copy Markdown
Contributor

@re-gius re-gius Jan 29, 2025

Choose a reason for hiding this comment

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

I double-checked it on master and it's right, those tests fail with the insecure_zero_ed feature flag

Copy link
Copy Markdown
Member

@bkchr bkchr Jan 29, 2025

Choose a reason for hiding this comment

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

Yeah I forgot the runtime-benchmarks flag earlier :D


assert_eq!(Balances::<T, I>::free_balance(&recipient), transfer_amount);
}

Expand Down Expand Up @@ -173,7 +178,12 @@ mod benchmarks {
#[extrinsic_call]
_(RawOrigin::Root, source_lookup, recipient_lookup, transfer_amount);

assert_eq!(Balances::<T, I>::free_balance(&source), Zero::zero());
if cfg!(feature = "insecure_zero_ed") {
assert_eq!(Balances::<T, I>::free_balance(&source), balance - transfer_amount);
} else {
assert_eq!(Balances::<T, I>::free_balance(&source), Zero::zero());
}

assert_eq!(Balances::<T, I>::free_balance(&recipient), transfer_amount);
}

Expand Down Expand Up @@ -208,7 +218,12 @@ mod benchmarks {
#[extrinsic_call]
transfer_allow_death(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount);

assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
if cfg!(feature = "insecure_zero_ed") {
assert_eq!(Balances::<T, I>::free_balance(&caller), balance - transfer_amount);
} else {
assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
}

assert_eq!(Balances::<T, I>::free_balance(&recipient), transfer_amount);
}

Expand Down Expand Up @@ -308,7 +323,7 @@ mod benchmarks {
/// Benchmark `burn` extrinsic with the worst possible condition - burn kills the account.
#[benchmark]
fn burn_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 All @@ -321,13 +336,17 @@ mod benchmarks {
#[extrinsic_call]
burn(RawOrigin::Signed(caller.clone()), burn_amount, false);

assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
if cfg!(feature = "insecure_zero_ed") {
assert_eq!(Balances::<T, I>::free_balance(&caller), balance - burn_amount);
} else {
assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
}
}

// Benchmark `burn` extrinsic with the case where account is kept alive.
#[benchmark]
fn burn_keep_alive() {
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
4 changes: 3 additions & 1 deletion substrate/frame/balances/src/tests/currency_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use frame_support::{
BalanceStatus::{Free, Reserved},
Currency,
ExistenceRequirement::{self, AllowDeath, KeepAlive},
Hooks, InspectLockableCurrency, LockIdentifier, LockableCurrency, NamedReservableCurrency,
InspectLockableCurrency, LockIdentifier, LockableCurrency, NamedReservableCurrency,
ReservableCurrency, WithdrawReasons,
},
StorageNoopGuard,
Expand Down Expand Up @@ -1136,7 +1136,9 @@ fn operations_on_dead_account_should_not_change_state() {

#[test]
#[should_panic = "The existential deposit must be greater than zero!"]
#[cfg(not(feature = "insecure_zero_ed"))]
Copy link
Copy Markdown
Member

@bkchr bkchr Jan 29, 2025

Choose a reason for hiding this comment

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

Only this is required.

fn zero_ed_is_prohibited() {
use frame_support::traits::Hooks;
// These functions all use `mutate_account` which may introduce a storage change when
// the account never existed to begin with, and shouldn't exist in the end.
ExtBuilder::default().existential_deposit(0).build_and_execute_with(|| {
Expand Down