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

Conversation

TarekkMA
Copy link
Contributor

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

@TarekkMA TarekkMA requested a review from a team as a code owner August 14, 2024 06:49
@bkchr bkchr requested a review from ggwpez August 14, 2024 20:05
@bkchr bkchr added the T2-pallets This PR/Issue is related to a particular pallet. label Aug 14, 2024
@bkchr
Copy link
Member

bkchr commented Aug 14, 2024

@TarekkMA can you please add a prdoc.

Copy link

Review required! Latest push from author must always be reviewed

@github-actions github-actions bot requested review from bkchr and ggwpez August 15, 2024 07:50
@bkchr bkchr enabled auto-merge August 15, 2024 08:56
@bkchr bkchr added this pull request to the merge queue Aug 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Aug 15, 2024
@bkchr bkchr added this pull request to the merge queue Aug 15, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 15, 2024
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
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 15, 2024
@@ -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.

@bkchr bkchr added this pull request to the merge queue Aug 28, 2024
Merged via the queue into paritytech:master with commit 1c4141a Aug 28, 2024
175 of 178 checks passed
ordian added a commit that referenced this pull request Aug 29, 2024
* master: (39 commits)
  short-term fix for para inherent weight overestimation (#5082)
  CI: Add backporting bot (#4795)
  Fix benchmark failures when using `insecure_zero_ed` flag (#5354)
  Command bot GHA v2 - /cmd <cmd> (#5457)
  Remove pallet::getter usage from treasury (#4962)
  Bump blake2b_simd from 1.0.1 to 1.0.2 (#5404)
  Bump rustversion from 1.0.14 to 1.0.17 (#5405)
  Bridge zombienet tests: remove old command (#5434)
  polkadot-parachain: Add omni-node variant with u64 block number (#5269)
  Refactor verbose test (#5506)
  Use umbrella crate for minimal template (#5155)
  IBP Coretime Polkadot bootnodes (#5499)
  rpc server: listen to `ipv6 socket` if available and `--experimental-rpc-endpoint` CLI option (#4792)
  Update approval-voting-regression-bench (#5504)
  change try-runtime rpc domains (#5443)
  polkadot-parachain-bin: Remove contracts parachain (#5471)
  Add feature to allow Aura collator to use full PoV size (#5393)
  Adding stkd bootnodes (#5470)
  Make `PendingConfigs` storage item public (#5467)
  frame-omni-bencher maintenance (#5466)
  ...
@RomarQ RomarQ deleted the p-tarekkma/use-balances-flag-in-benchmarks branch November 7, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants