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

Bug: max > T::MaxCandidates; you might need to run benchmarks again #1423

Closed
wilwade opened this issue Apr 18, 2023 · 8 comments
Closed

Bug: max > T::MaxCandidates; you might need to run benchmarks again #1423

wilwade opened this issue Apr 18, 2023 · 8 comments
Assignees
Labels
benchmark Extrensic benchmark needed bug Something isn't working good first issue Good for newcomers

Comments

@wilwade
Copy link
Collaborator

wilwade commented Apr 18, 2023

When running benchmarks I saw this error

max > T::MaxCandidates; you might need to run benchmarks again

Example from Benchmark run: https://github.com/LibertyDSNP/frequency/actions/runs/4726913413/jobs/8386962925

Does this error matter? Why is it happening?

Appears to come from set_desired_candidates in cumulus: pallets/collator-selection/src/lib.rs

@wilwade wilwade added bug Something isn't working good first issue Good for newcomers benchmark Extrensic benchmark needed labels Apr 18, 2023
@rlaferla rlaferla self-assigned this Jun 7, 2023
@rlaferla
Copy link
Contributor

rlaferla commented Jun 7, 2023

An observation. The same log file has a script error that occurs earlier:

image

@rlaferla
Copy link
Contributor

rlaferla commented Jun 7, 2023

It only occurs for pallet_collator_selection and below is the specific warning logs:

Running benchmarks for pallet_collator_selection
+ ./scripts/../target/release/frequency benchmark pallet --pallet=pallet_collator_selection --extrinsic '*' --chain=frequency-bench --execution=wasm --heap-pages=4096 --wasm-execution=compiled --steps=50 --repeat=20 --output=./scripts/../runtime/common/src/weights --template=./scripts/../.maintain/runtime-weight-template.hbs
2023-06-07 09:01:36 assembling new collators for new session 0 at #0    
2023-06-07 09:01:36 assembling new collators for new session 1 at #0    
2023-06-07 09:01:36 Starting benchmark: pallet_collator_selection::set_invulnerables    
2023-06-07 09:01:37 Starting benchmark: pallet_collator_selection::set_desired_candidates    
2023-06-07 09:01:37 max > T::MaxCandidates; you might need to run benchmarks again    
2023-06-07 09:01:37 max > T::MaxCandidates; you might need to run benchmarks again    
2023-06-07 09:01:37 max > T::MaxCandidates; you might need to run benchmarks again    
2023-06-07 09:01:37 max > T::MaxCandidates; you might need to run benchmarks again    
2023-06-07 09:01:37 max > T::MaxCandidates; you might need to run benchmarks again    
2023-06-07 09:01:37 max > T::MaxCandidates; you might need to run benchmarks again    
2023-06-07 09:01:37 max > T::MaxCandidates; you might need to run benchmarks again    
2023-06-07 09:01:37 max > T::MaxCandidates; you might need to run benchmarks again    
2023-06-07 09:01:37 max > T::MaxCandidates; you might need to run benchmarks again    
2023-06-07 09:01:37 max > T::MaxCandidates; you might need to run benchmarks again

@rlaferla
Copy link
Contributor

rlaferla commented Jun 7, 2023

	/// Desired number of candidates.
	///
	/// This should ideally always be less than [`Config::MaxCandidates`] for weights to be correct.
	#[pallet::storage]
	#[pallet::getter(fn desired_candidates)]
	pub type DesiredCandidates<T> = StorageValue<_, u32, ValueQuery>;

@rlaferla
Copy link
Contributor

rlaferla commented Jun 7, 2023

pub type CollatorMaxCandidates = ConstU32<50>;
pub type CollatorMinCandidates = ConstU32<1>;

@rlaferla
Copy link
Contributor

rlaferla commented Jun 8, 2023

The problem is a bug in Cumulus' collator-selection pallet where the benchmark to set the desired candidates uses a max value of 999 instead of T::MaxCandidates:get() - 1. I will file an issue and/or PR for the Cumulus project.

@rlaferla
Copy link
Contributor

rlaferla commented Jun 8, 2023

paritytech/cumulus#2715

@rlaferla rlaferla closed this as completed Jun 8, 2023
@rlaferla
Copy link
Contributor

rlaferla commented Jun 8, 2023

paritytech/cumulus#2716

@rlaferla
Copy link
Contributor

rlaferla commented Jun 8, 2023

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Extrensic benchmark needed bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants