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

manuelmauro · 2025-01-29T09:44:40Z

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.

Integration

In depth notes about how this PR should be integrated by downstream projects. This part is mandatory, and should be
reviewed by reviewers, if the PR does NOT have the R0-Silent label. In case of a R0-Silent, it can be ignored.

Review Notes

In depth notes about the implementation details of your PR. This should be the main guide for reviewers to
understand your approach and effectively review it. If too long, use
<details>.

Imagine that someone who is depending on the old code wants to integrate your new code and the only information that
they get is this section. It helps to include example usage and default value here, with a diff code-block to show
possibly integration.

Include your leftover TODOs, if any, here.

Checklist

My PR includes a detailed description as outlined in the "Description" and its two subsections above.
My PR follows the labeling requirements of this project (at minimum one label for T required)
- External contributors: ask maintainers to put the right label on your PR.
I have made corresponding changes to the documentation (if applicable)
I have added tests that prove my fix is effective or that my feature works (if applicable)

You can remove the "Checklist" section once all have been checked. Thank you for your contribution!

✄ -----------------------------------------------------------------------------

cla-bot-2021 · 2025-01-29T09:44:47Z

User @manuelmauro, please sign the CLA here.

re-gius · 2025-01-29T14:55:10Z

bot bench substrate-pallet --features=insecure_zero_ed --pallet=pallet_balances

command-bot · 2025-01-29T14:55:16Z

@re-gius https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/8112528 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --features=insecure_zero_ed --pallet=pallet_balances. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-750b5ecf-caad-4fbe-af7f-1bef56eafa29 to cancel this command or bot cancel to cancel all commands in this pull request.

github-actions · 2025-01-29T14:55:23Z

We have migrated the command bot to GHA

Please, see the new usage instructions here or here. Soon the old commands will be disabled.

re-gius · 2025-01-29T14:55:48Z

/cmd bench substrate-pallet --features=insecure_zero_ed --pallet=pallet_balances

github-actions · 2025-01-29T14:56:26Z

Command "bench substrate-pallet --features=insecure_zero_ed --pallet=pallet_balances" has started 🚀 See logs here

command-bot · 2025-01-29T14:57:49Z

@re-gius Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --features=insecure_zero_ed --pallet=pallet_balances has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/8112528 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/8112528/artifacts/download.

re-gius · 2025-01-29T15:07:23Z

/cmd bench substrate-pallet --pallet=pallet_balances

github-actions · 2025-01-29T15:08:12Z

Command "bench substrate-pallet --pallet=pallet_balances" has started 🚀 See logs here

re-gius

LGTM

substrate/frame/balances/src/benchmarking.rs

github-actions · 2025-01-29T15:32:06Z

Review required! Latest push from author must always be reviewed

re-gius · 2025-01-29T15:33:45Z

/cmd bench substrate-pallet --pallet=pallet_balances

github-actions · 2025-01-29T15:35:15Z

Command "bench substrate-pallet --pallet=pallet_balances" has started 🚀 See logs here

bkchr · 2025-01-29T15:50:29Z

substrate/frame/balances/src/benchmarking.rs

+		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());
+		}


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

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

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

Yeah I forgot the runtime-benchmarks flag earlier :D

bkchr · 2025-01-29T15:50:37Z

substrate/frame/balances/src/tests/currency_tests.rs

@@ -1136,6 +1136,7 @@ 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"))]


Only this is required.

RomarQ · 2025-01-29T16:34:58Z

@manuelmauro I think we should also update burn_allow_death burn_keep_alive benchmarks to use let existential_deposit: T::Balance = minimum_balance::<T, I>(); instead of let existential_deposit = T::ExistentialDeposit::get();

I think it is the only thing missing to allow runtimes to use ExistentialDeposit = 0 when running benchmarks.

github-actions · 2025-01-29T16:35:46Z

Command "bench substrate-pallet --pallet=pallet_balances" has failed ❌! See logs here

Command output:

✅ Successful benchmarks of runtimes/pallets:
-- dev: ['pallet_balances']
-- westend: ['pallet_balances']
-- rococo: ['pallet_balances']
-- asset-hub-westend: ['pallet_balances']
-- asset-hub-rococo: ['pallet_balances']
-- bridge-hub-rococo: ['pallet_balances']
-- bridge-hub-westend: ['pallet_balances']
-- collectives-westend: ['pallet_balances']
-- contracts-rococo: ['pallet_balances']
-- coretime-rococo: ['pallet_balances']
-- coretime-westend: ['pallet_balances']
-- people-rococo: ['pallet_balances']
-- people-westend: ['pallet_balances']

github-actions · 2025-01-29T16:35:59Z

Command "bench substrate-pallet --features=insecure_zero_ed --pallet=pallet_balances" has failed ❌! See logs here

Command output:

✅ Successful benchmarks of runtimes/pallets:
-- dev: ['pallet_balances']
-- westend: ['pallet_balances']
-- rococo: ['pallet_balances']
-- asset-hub-westend: ['pallet_balances']
-- asset-hub-rococo: ['pallet_balances']
-- bridge-hub-rococo: ['pallet_balances']
-- bridge-hub-westend: ['pallet_balances']
-- collectives-westend: ['pallet_balances']
-- contracts-rococo: ['pallet_balances']
-- coretime-rococo: ['pallet_balances']
-- coretime-westend: ['pallet_balances']
-- people-rococo: ['pallet_balances']
-- people-westend: ['pallet_balances']

github-actions · 2025-01-29T17:12:42Z

Command "bench substrate-pallet --pallet=pallet_balances" has failed ❌! See logs here

Command output:

✅ Successful benchmarks of runtimes/pallets:
-- dev: ['pallet_balances']
-- westend: ['pallet_balances']
-- rococo: ['pallet_balances']
-- asset-hub-westend: ['pallet_balances']
-- asset-hub-rococo: ['pallet_balances']
-- bridge-hub-rococo: ['pallet_balances']
-- bridge-hub-westend: ['pallet_balances']
-- collectives-westend: ['pallet_balances']
-- contracts-rococo: ['pallet_balances']
-- coretime-rococo: ['pallet_balances']
-- coretime-westend: ['pallet_balances']
-- people-rococo: ['pallet_balances']
-- people-westend: ['pallet_balances']

…nchmarks

RomarQ · 2025-01-29T19:55:56Z

@bkchr just added the missing logic, all calls are now working when insecure_zero_ed feature is enabled and ExistentialDeposit is zero.

* master: Remove warnings by cleaning up the `Cargo.toml` (#7416) [Backport] Version bumps from stable2412-1 + prdocs reorg (#7401) fix pre-dispatch PoV underweight for ParasInherent (#7378) malus-collator: implement malicious collator submitting same collation to all backing groups (#6924) `fatxpool`: use tracing for logging (#6897) Improvements for Weekly bench (#7390) Replace derivative dependency with derive-where (#7324) Add support for feature `pallet_balances/insecure_zero_ed` in benchmarks and testing (#7379) Fix Snowbridge benchmark tests (#7296) Bridges small nits/improvements (#7383) Migrating cumulus-pallet-session-benchmarking to Benchmarking V2 (#6564) [pallet-revive] implement the block author API (#7198) Use checked math in frame-balances named_reserve (#7365) move installation of frame-omni-bencher into a cmd.py itself (#7372) remove old bench & revert the frame-weight-template (#7362) ci: fix workflow permissions (#7366) [net/libp2p] Use raw `Identify` observed addresses to discover external addresses (#7338) Improve `set_validation_data` error message. (#7359) Implement pallet view function queries (#4722)

…rks and testing (paritytech#7379) # 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. ## Integration *In depth notes about how this PR should be integrated by downstream projects. This part is mandatory, and should be reviewed by reviewers, if the PR does NOT have the `R0-Silent` label. In case of a `R0-Silent`, it can be ignored.* ## Review Notes *In depth notes about the **implementation** details of your PR. This should be the main guide for reviewers to understand your approach and effectively review it. If too long, use [`<details>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details)*. *Imagine that someone who is depending on the old code wants to integrate your new code and the only information that they get is this section. It helps to include example usage and default value here, with a `diff` code-block to show possibly integration.* *Include your leftover TODOs, if any, here.* # Checklist * [x] My PR includes a detailed description as outlined in the "Description" and its two subsections above. * [x] My PR follows the [labeling requirements]( https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process ) of this project (at minimum one label for `T` required) * External contributors: ask maintainers to put the right label on your PR. * [x] I have made corresponding changes to the documentation (if applicable) * [x] I have added tests that prove my fix is effective or that my feature works (if applicable) You can remove the "Checklist" section once all have been checked. Thank you for your contribution! ✄ ----------------------------------------------------------------------------- --------- Co-authored-by: Rodrigo Quelhas <[email protected]>

…rks and testing (#7379) # 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. ## Integration *In depth notes about how this PR should be integrated by downstream projects. This part is mandatory, and should be reviewed by reviewers, if the PR does NOT have the `R0-Silent` label. In case of a `R0-Silent`, it can be ignored.* ## Review Notes *In depth notes about the **implementation** details of your PR. This should be the main guide for reviewers to understand your approach and effectively review it. If too long, use [`<details>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details)*. *Imagine that someone who is depending on the old code wants to integrate your new code and the only information that they get is this section. It helps to include example usage and default value here, with a `diff` code-block to show possibly integration.* *Include your leftover TODOs, if any, here.* # Checklist * [x] My PR includes a detailed description as outlined in the "Description" and its two subsections above. * [x] My PR follows the [labeling requirements]( https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process ) of this project (at minimum one label for `T` required) * External contributors: ask maintainers to put the right label on your PR. * [x] I have made corresponding changes to the documentation (if applicable) * [x] I have added tests that prove my fix is effective or that my feature works (if applicable) You can remove the "Checklist" section once all have been checked. Thank you for your contribution! ✄ ----------------------------------------------------------------------------- --------- Co-authored-by: Rodrigo Quelhas <[email protected]>

manuelmauro added 2 commits January 29, 2025 10:23

feat: ✨ support feature insecure_zero_ed in benchmarks and tests

260d204

test: ✨ add balance assertion for insecure_zero_ed case

ca8b63c

manuelmauro marked this pull request as ready for review January 29, 2025 09:53

manuelmauro requested a review from a team as a code owner January 29, 2025 09:53

docs: 📝 add prdoc

438661a

RomarQ approved these changes Jan 29, 2025

View reviewed changes

re-gius added T2-pallets This PR/Issue is related to a particular pallet. T10-tests This PR/Issue is related to tests. labels Jan 29, 2025

re-gius approved these changes Jan 29, 2025

View reviewed changes

substrate/frame/balances/src/benchmarking.rs Outdated Show resolved Hide resolved

refactor: ♻️ use cfg! macro to check for insecure_zero_ed feature

e5e5615

github-actions bot requested review from re-gius and RomarQ January 29, 2025 15:32

re-gius approved these changes Jan 29, 2025

View reviewed changes

bkchr reviewed Jan 29, 2025

View reviewed changes

test: ✅ rely on minimum_balance() instead of ExistentialBalance in be…

b1cf78f

…nchmarks

bkchr approved these changes Jan 29, 2025

View reviewed changes

RomarQ approved these changes Jan 29, 2025

View reviewed changes

bkchr enabled auto-merge January 29, 2025 19:51

fix benchmark when insecure_zero_ed is enabled

eacbcf6

auto-merge was automatically disabled January 29, 2025 19:53
Head branch was pushed to by a user without write access

bkchr enabled auto-merge January 29, 2025 20:01

fix clippy error

cc26620

auto-merge was automatically disabled January 29, 2025 21:19
Head branch was pushed to by a user without write access

bkchr enabled auto-merge January 29, 2025 21:32

bkchr added this pull request to the merge queue Jan 29, 2025

Merged via the queue into paritytech:master with commit 80e30ec Jan 29, 2025
203 of 206 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

manuelmauro commented Jan 29, 2025 •

edited

Loading

cla-bot-2021 bot commented Jan 29, 2025

re-gius commented Jan 29, 2025

command-bot bot commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025

re-gius commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

command-bot bot commented Jan 29, 2025

re-gius commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

re-gius left a comment

github-actions bot commented Jan 29, 2025

re-gius commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

bkchr Jan 29, 2025

manuelmauro Jan 29, 2025

re-gius Jan 29, 2025

bkchr Jan 29, 2025

bkchr Jan 29, 2025

RomarQ commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

RomarQ commented Jan 29, 2025

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

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

Conversation

manuelmauro commented Jan 29, 2025 • edited Loading

Description

Integration

Review Notes

Checklist

cla-bot-2021 bot commented Jan 29, 2025

re-gius commented Jan 29, 2025

command-bot bot commented Jan 29, 2025 • edited Loading

github-actions bot commented Jan 29, 2025

re-gius commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

command-bot bot commented Jan 29, 2025

re-gius commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

re-gius left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2025

re-gius commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

bkchr Jan 29, 2025

Choose a reason for hiding this comment

manuelmauro Jan 29, 2025

Choose a reason for hiding this comment

re-gius Jan 29, 2025

Choose a reason for hiding this comment

bkchr Jan 29, 2025

Choose a reason for hiding this comment

bkchr Jan 29, 2025

Choose a reason for hiding this comment

RomarQ commented Jan 29, 2025 • edited Loading

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

RomarQ commented Jan 29, 2025

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

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

manuelmauro commented Jan 29, 2025 •

edited

Loading

command-bot bot commented Jan 29, 2025 •

edited

Loading

RomarQ commented Jan 29, 2025 •

edited

Loading