Draft benchmark sanity weight test #1493

Daanvdplas · 2023-09-11T12:47:21Z

This PR introduces a sanity weight check during the benchmark execution. It checks whether the max weight of a given extrinsic exceeds that of the (DispatchClass::Normal) max_extrinsic_weight obtained from the runtime. The max weight of an extrinsic can be default (no complexity parameter) or else, the max component is used.

I have included the benchmark_sanity_check in the frame-benchmarking-cli because the component range is available. In addition, a flag: sanity-check is included to have the cli user choose whether the benchmark execution should fail when the max_extrinsic_weight exceeds. A warning message will always be printed when the max_extrinsic_weight is exceeded. Warning example:

This is a draft PR to get confirmation on my approach. If approved, things that are left to do:

Include benchmark_sanity_check for other benchmark operations than benchmarking a pallet.
Check for corresponding Polkadot/Cumulus PR
Finalise PR

Closes #152

ggwpez · 2023-09-15T15:54:34Z

Incident where we needed exactly this: #1589
So the default behaviour should actually be to ERROR out at the end of the program, after successfully writing all weight files.

Daanvdplas · 2023-09-18T13:49:47Z

@ggwpez should I still include a flag that can turn the error into a warning?

ggwpez · 2023-09-18T14:20:12Z

@ggwpez should I still include a flag that can turn the error into a warning?

Yea maybe something like: --weight-sanity-check=warn | error | ignore where error is the default.

0xmovses · 2023-10-14T08:41:12Z

@Daanvdplas are you still working on this? I'm looking for issues to pick up.

Daanvdplas · 2023-10-14T15:04:11Z

Yes! I have been blocked for a while but will continue working on it tomorrow!

substrate/client/cli/src/arg_enums.rs

substrate/client/cli/src/params/shared_params.rs

ggwpez · 2023-10-25T12:34:38Z

substrate/frame/benchmarking/src/utils.rs

@@ -235,7 +238,7 @@ sp_api::decl_runtime_apis! {
 		/// Parameters
 		/// - `extra`: Also list benchmarks marked "extra" which would otherwise not be
 		///            needed for weight calculation.


I know the docs here are pretty bad, but could you please explain the return args that you added?

substrate/utils/frame/benchmarking-cli/src/pallet/command.rs

substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs

ggwpez · 2023-10-25T12:43:23Z

substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs

+	let mut sanity_weight_check_passed = true;
+	// Loop through all benchmark results.
+	for ((pallet, instance), results) in all_results.iter() {
+		println!("Pallet: {}\nInstance: {}\n", pallet, instance);


Maybe it would make sense to add another level to SanityWeightCheck: Info.
Since here you print things that are neither a warning nor an error and are not really important as long as the whole check passes.

I only do not see a reason for only outputting Info (the above line and weight results, without a warning) when Warning only adds a warning line above the failing weight results and a line regarding passing / failing the sanity weight check at the end.

If someone would like to inspect its weight results, personally I would like an indicator that clearly indicates which weight function is failing and if no failing weight results I would like formatting that gives a clear view to inspect my weight results. The above line is just to format the sanity weight check results the most clear to my opinion.

Please let me know if I'm understanding your incorrectly and/or what you think of the above :)

ggwpez · 2023-10-25T12:45:12Z

polkadot/runtime/rococo/src/lib.rs

-			return (list, storage_info)
+			let max_extrinsic_weight = BlockWeights::get().per_class.get(DispatchClass::Normal).max_extrinsic.unwrap();
+			let db_weight: frame_support::weights::RuntimeDbWeight = <Self as frame_system::Config>::DbWeight::get();
+			(list, storage_info, max_extrinsic_weight, db_weight)


I hoped that we could somehow around doing a breaking change for all runtimes, but it seems unavoidable, unless we want to squeeze this into into any of the vectors.
Maybe you can create a single struct that holds the Weight and RuntimeDbWeight. Then we can extend that in the future and avoid more breaking changes.

Creating a single struct for Weight and RuntimeDbWeight is either way a better solution. Would you prefer to have it included in BenchmarkList / StorageInfo and make it backwards compatible or add an additional parameter which results in a breaking change?

I think it would need one breaking change, yes. But from then on we can avoid further breaking changes by using the following pattern:

struct AllBenchmarkData { // not a good name list: Vec<frame_benchmarking::BenchmarkList>, info: Vec<frame_support::traits::StorageInfo>, max_ext_weight: Option<..>, db_weights: Option<..> }

and having Default on that thing. The runtime devs can use ..Default::default() to stay forward compatible and we can keep adding Options without directly breaking their code.

chevdor · 2023-10-31T16:24:38Z

Nice !
You are introducing a check against an upper bound. Did you consider a min as well ?
A weight that is anormally high can be problematic but a weight that is too low opens a door to a range of security issues incl. spamming.

kianenigma · 2023-10-31T18:52:36Z

substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs

+// value) and compares it with the max. extrinsic weight allowed in a single block.
+//
+// `max_extrinsic_weight` & `db_weight` are obtained from the runtime configuration.
+pub(crate) fn sanity_weight_check(


Conceptually, the way to avoid all of these breaking changes is to do this check in the runtime. Is there a reason why it was chosen in the way that it is now?

As in, there is a single new runtime API through which you pass in all the computed weights, and it would do any checking necessary for it.

Ofc this is also a breaking change differently, but possibly an alternative design.

I think the current design is also good, but something to keep in mind.

Reading the code, I guess the runtime also doesn't know the maximum value for each component..

After looking at it again, the component ranges are coming from the runtime actually: parameter list

I'm investigating another approach

kianenigma · 2023-10-31T19:55:33Z

substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs

+				total_weight = total_weight.saturating_add(
+					Weight::from_parts(component.slope.try_into().unwrap(), 0)
+						.saturating_mul(max_component(&component, &result.component_ranges)),
+				);


Suggested change

total_weight = total_weight.saturating_add(

Weight::from_parts(component.slope.try_into().unwrap(), 0)

.saturating_mul(max_component(&component, &result.component_ranges)),

);

total_weight.saturating_accrue(

Weight::from_parts(component.slope.try_into().unwrap(), 0)

.saturating_mul(max_component(&component, &result.component_ranges)),

);

kianenigma · 2023-10-31T19:56:43Z

substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs

+					Weight::from_parts(0, component.slope.try_into().unwrap())
+						.saturating_mul(max_component(&component, &result.component_ranges)),
+				);
+			}


kinda baffled that all of this math to calculate the final weight of a weight-fn, for a given parameter, needs to be re-created here. Is this really the only place where we have this logic?

This logic only exists in the benchmark templates (i.e. here & here)

kianenigma

All in all looks good, modulo some comments from me and Oliver. I would consider making the default in fact be Warn or something that lets as many teams out there know about their misconfig.

Daanvdplas · 2023-11-01T10:36:32Z

You are introducing a check against an upper bound. Did you consider a min as well ?

@chevdor I'm not sure how to determine such minimum_extrinsic_weight because from my understanding the weight / fee system is the protection against attacks such as spamming. A warning when a benchmark result is suspiciously low is not a bad idea though. A suggestion; I could add a check which compares the weight result against the weight of the remark extrinsic`. Curious what others think about this!

chevdor · 2023-11-02T08:24:05Z

I am not convinced you can use the remark as reference.
An idea is to track the previous values and create an "acceptable range" around that previous value(s) (per extrinsic). That means finding a way to keep track of the previous value(s).

Daanvdplas · 2023-11-02T09:24:28Z

@chevdor sorry I do not follow what you mean with "previous values", could you kindly elaborate?

chevdor · 2023-11-02T09:27:34Z

I am afraid my suggestion goes beyond the scope of this PR.
An idea that has been floating around for a while is to keep track of historical weights per extrsinsic and ensure that new weights remain "fairly close" to the weight median value. This is simple but requires storing this historical data (which could even be limited to a single value at first).

kianenigma · 2024-02-05T16:10:39Z

@Daanvdplas do you intend to finish this? happy to delegate to someone else if otherwise.

kianenigma · 2024-02-05T16:19:35Z

PR also needs a better description that is targeted toward externals rather than internals + prdoc.

paritytech-cicd-pr · 2024-02-05T16:24:09Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5122166

kianenigma · 2024-02-28T13:48:10Z

still keen on this, but still missing the CI to be green :)

ggwpez · 2024-02-28T14:13:41Z

@philoniare this one could also be a good issue for you 😄

kianenigma · 2024-03-31T09:30:42Z

Also raising this again as something that we'd be highly interested in having. I would suggest a large tip for the champion to finish this :)

dastansam · 2024-04-05T19:45:41Z

Also raising this again as something that we'd be highly interested in having. I would suggest a large tip for the champion to finish this :)

hey @kianenigma

would like to work on this. can you please assign this to me?

dastansam · 2024-04-08T00:57:25Z

I completed resolving open comments and fixing CI errors. Opened a new PR to this branch since I don't have write access. let me know if you have any more comments

cc @kianenigma @ggwpez

kianenigma · 2024-07-10T09:01:25Z

Closing in fav of the one above. Thank you @Daanvdplas.

Draft benchmark sanity weight test

0ceee42

Daanvdplas added C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. labels Sep 11, 2023

Daanvdplas requested review from kianenigma and ggwpez September 11, 2023 12:47

ggwpez mentioned this pull request Sep 15, 2023

Disallow usage of from_ref_time for weights after the relevant polkadot version #1589

Closed

maltekliemann mentioned this pull request Sep 19, 2023

Reduce length of MarketsCollectingSubsidy zeitgeistpm/zeitgeist#1118

Merged

Daanvdplas added 12 commits October 17, 2023 08:55

error || warning

81165e3

allow enable, warning & ignore + output improvement

8fc1fca

Merge master

7d64b60

fmt

d476d7f

spelling & ci tests

a698258

fix failing ci tests

6081db0

more ci tests

540ce57

more...

df4f1c4

Merge branch 'master' into daan/built_in_weight_test

ac0bc11

add to gitlab pipeline

d64188f

fix: unused import

eace65b

fmt

98485c5

ggwpez reviewed Oct 25, 2023

View reviewed changes

Daanvdplas added 2 commits October 27, 2023 13:13

improve based on first comments (1/2)

8795558

add Cargo.lock

741329f

Daanvdplas marked this pull request as ready for review October 31, 2023 15:42

Daanvdplas requested a review from a team October 31, 2023 15:42

Daanvdplas requested a review from a team as a code owner October 31, 2023 15:42

paritytech-ci requested review from a team October 31, 2023 15:43

kianenigma reviewed Oct 31, 2023

View reviewed changes

kianenigma approved these changes Oct 31, 2023

View reviewed changes

ggwpez removed the C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. label Feb 5, 2024

bkchr requested a review from ggwpez February 5, 2024 11:38

Master.into()

0f76c55

Daanvdplas requested a review from a team as a code owner February 5, 2024 16:17

kianenigma mentioned this pull request Apr 2, 2024

🧑‍💻 Wishlist: Developers #3900

Open

ggwpez assigned dastansam Apr 5, 2024

dastansam mentioned this pull request Apr 8, 2024

benchmarking: complete automatic weight sanity check PR #4020

Open

kianenigma closed this Jul 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft benchmark sanity weight test #1493

Draft benchmark sanity weight test #1493

Daanvdplas commented Sep 11, 2023 •

edited

Loading

ggwpez commented Sep 15, 2023

Daanvdplas commented Sep 18, 2023 •

edited by ggwpez

Loading

ggwpez commented Sep 18, 2023

0xmovses commented Oct 14, 2023

Daanvdplas commented Oct 14, 2023

ggwpez Oct 25, 2023

ggwpez Oct 25, 2023

Daanvdplas Oct 26, 2023

ggwpez Oct 25, 2023

Daanvdplas Oct 26, 2023

ggwpez Feb 5, 2024 •

edited

Loading

chevdor commented Oct 31, 2023

kianenigma Oct 31, 2023

kianenigma Oct 31, 2023

Daanvdplas Nov 2, 2023

kianenigma Oct 31, 2023

kianenigma Oct 31, 2023

Daanvdplas Nov 2, 2023

kianenigma left a comment

Daanvdplas commented Nov 1, 2023

chevdor commented Nov 2, 2023

Daanvdplas commented Nov 2, 2023

chevdor commented Nov 2, 2023

kianenigma commented Feb 5, 2024

kianenigma commented Feb 5, 2024

paritytech-cicd-pr commented Feb 5, 2024

kianenigma commented Feb 28, 2024

ggwpez commented Feb 28, 2024

kianenigma commented Mar 31, 2024

dastansam commented Apr 5, 2024

dastansam commented Apr 8, 2024

kianenigma commented Jul 10, 2024

Draft benchmark sanity weight test #1493

Draft benchmark sanity weight test #1493

Conversation

Daanvdplas commented Sep 11, 2023 • edited Loading

ggwpez commented Sep 15, 2023

Daanvdplas commented Sep 18, 2023 • edited by ggwpez Loading

ggwpez commented Sep 18, 2023

0xmovses commented Oct 14, 2023

Daanvdplas commented Oct 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

chevdor commented Oct 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

Daanvdplas commented Nov 1, 2023

chevdor commented Nov 2, 2023

Daanvdplas commented Nov 2, 2023

chevdor commented Nov 2, 2023

kianenigma commented Feb 5, 2024

kianenigma commented Feb 5, 2024

paritytech-cicd-pr commented Feb 5, 2024

kianenigma commented Feb 28, 2024

ggwpez commented Feb 28, 2024

kianenigma commented Mar 31, 2024

dastansam commented Apr 5, 2024

dastansam commented Apr 8, 2024

kianenigma commented Jul 10, 2024

Daanvdplas commented Sep 11, 2023 •

edited

Loading

Daanvdplas commented Sep 18, 2023 •

edited by ggwpez

Loading

ggwpez Feb 5, 2024 •

edited

Loading