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

Benchmark promote_fast and others in pallet-core-fellowship Generates Ranks Exceeding MaxRank #7517

Open
2 tasks done
Doordashcon opened this issue Feb 10, 2025 · 3 comments · May be fixed by #7720
Open
2 tasks done
Labels
I2-bug The node fails to follow expected behavior.

Comments

@Doordashcon
Copy link
Contributor

Doordashcon commented Feb 10, 2025

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Description of bug

When running the promote_fast benchmark in pallet_core_fellowship, I encountered an issue where the generated rank (r) exceeded the configured MaxRank. Specifically, even when T::MaxRank::get() == 1, the benchmark framework sometimes assigned r = 2, causing the extrinsic call to fail with an InvalidRank error.

Steps to Reproduce

  • Configure the runtime with T::MaxRank::get() == 1.
  • Run the benchmark for promote_fast.
  • Add log::info!("Benchmark promote_fast: r={} MaxRank={}", r, T::MaxRank::get());
  • Observe that r is sometimes set to 2, leading to an InvalidRank error.

Expected Behavior
The benchmark framework should respect the constraint that r must be in the range [1, T::MaxRank::get()], ensuring r never exceeds MaxRank.

Actual Behavior
The benchmark framework assigns r = 2 even when MaxRank == 1.
This causes the promote_fast extrinsic to fail validation.

Are you willing to work on this?
yes

cc @ggwpez

@Doordashcon Doordashcon added I10-unconfirmed Issue might be valid, but it's not yet known. I2-bug The node fails to follow expected behavior. labels Feb 10, 2025
@Doordashcon
Copy link
Contributor Author

Doordashcon commented Feb 10, 2025

This relates to collectives with one rank(i.e. potoc collective)

@Doordashcon Doordashcon changed the title Benchmarks promote_fast and others should support configurable ranks Benchmark promote_fast and others in pallet-core-fellowship Generates Ranks Exceeding MaxRank Feb 14, 2025
@ggwpez
Copy link
Member

ggwpez commented Feb 17, 2025

Hm, it should respect it actually.

fn promote_fast(r: Linear<1, { T::MaxRank::get() as u32 }>) -> Result<(), BenchmarkError> {

@ggwpez ggwpez removed the I10-unconfirmed Issue might be valid, but it's not yet known. label Feb 17, 2025
@Doordashcon
Copy link
Contributor Author

cargo build -p collectives-polkadot-runtime --features runtime-benchmarks --release

frame-omni-bencher v1 benchmark pallet \
--pallet pallet-core-fellowship --extrinsic "" \
--runtime ./target/release/wbuild/collectives-polkadot-runtime/collectives_polkadot_runtime.compact.compressed.wasm \
2025-02-18T00:35:27.899534Z  INFO pallet_core_fellowship::benchmarking::benchmarks: Benchmark promote_fast: r=1 MaxRank=1    
2025-02-18T00:35:27.900543Z  INFO pallet_core_fellowship::benchmarking::benchmarks: Benchmark promote_fast: r=2 MaxRank=9    
2025-02-18T00:35:27.901147Z  INFO pallet_core_fellowship::benchmarking::benchmarks: Benchmark promote_fast: r=2 MaxRank=9    
2025-02-18T00:35:27.901723Z  INFO pallet_core_fellowship::benchmarking::benchmarks: Benchmark promote_fast: r=2 MaxRank=1

cc @ggwpez

@Doordashcon Doordashcon linked a pull request Mar 2, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@ggwpez @Doordashcon and others