Do not make pallet-identity benchmarks signature-dependent #8179

moliholy · 2025-04-08T10:35:01Z

Similar to #4756 for pallet-nfts, the changes proposed in this PR intend to make benchmarks for pallet-identity signature-agnostic by the inclusion of a benchmark helper with sane defaults to handle the signing operations.

bkchr · 2025-04-08T11:56:22Z

substrate/frame/identity/src/lib.rs


+		#[cfg(feature = "runtime-benchmarks")]
+		/// A set of helper functions for benchmarking.
+		type Helper: BenchmarkHelper<


Suggested change

type Helper: BenchmarkHelper<

type BenchmarkHelper: ::BenchmarkHelper<

I do not have a preference at all on how to name this parameter, but in pallet-nfts it's named Helper, so I thought out of consistency it should be named the same. Just to confirm.

BenchmarkHelper is more expressive.

And we also have a lot of other pallets naming it BenchmarkHelper.

Done in 63c1e13.

bkchr · 2025-04-08T12:08:21Z

prdoc/pr_8179.prdoc

+    description: |
+      - Includes a `BenchmarkHelper` configuration item in `pallet-identity` to handle signing operations.
+      - Abstracts away the explicit link with Sr25519 schema in the benchmarks, allowing chains with a different one to be able to run them and calculate the weights.
+      - Adds a default implementation for the empty tuple that leaves the code equivalent.


Suggested change

- Adds a default implementation for the empty tuple that leaves the code equivalent.

- Adds a default implementation for the empty tuple that leaves the code equivalent.

Adding the following to your implementation of the `frame_identity::Config` should be enough:

```diff

#[cfg(feature = "runtime-benchmarks")]

type BenchmarkHelper = ();

```

Will add once we address my previous comment.

Done in 63c1e13.

bkchr · 2025-04-08T12:10:27Z

substrate/frame/identity/src/lib.rs

+		fn signer() -> (Public, AccountId);
+		fn sign(signer: &Public, message: &[u8]) -> Signature;


Suggested change

fn signer() -> (Public, AccountId);

fn sign(signer: &Public, message: &[u8]) -> Signature;

/// Sign the given `message` with a random account.

///

/// Should return the public key of this random account and the signature.

fn sign_message(message: &[u8]) -> (Public, Signature);

Just make this one function. In the benchmarking code you can get the AccountId by calling into_account on the public_key.

I get the intention of reducing the interface to only one function, but still I don't like the idea, mainly because:

Again it differs from pallet-nfts equivalent code.

This prevents from signing twice with the exact same account. Not that it's needed now but adds unnecessary constrains that might bite devs in future iterations of the pallet and likely force to revert the changes and introduce extra noise.

Just my two cents. If you still prefer your proposed approach I'll happily change it, but I thought it was worth discussing.

Done in 00e81d4.

…marks-generic

moliholy · 2025-04-14T12:01:40Z

@bkchr everything addressed.

bkchr

Looks good.

moliholy · 2025-05-05T08:44:15Z

@ggwpez just a friendly ping 😄

moliholy · 2025-05-29T12:01:22Z

@bkchr just another friendly ping 🙏

ggwpez · 2025-06-02T18:24:39Z

prdoc/pr_8179.prdoc

+
+crates:
+    - name: pallet-identity
+      bump: patch


Adding a config item is a breaking change though

Addrressed in 7bb7da3.

The CI says otherwise, by the way.

substrate/frame/identity/src/benchmarking.rs

ggwpez · 2025-06-02T18:27:00Z

substrate/frame/identity/src/lib.rs

 		type MaxUsernameLength: Get<u32>;

+		#[cfg(feature = "runtime-benchmarks")]
+		/// A set of helper functions for benchmarking.


The comment normally goes above the feature gate.

Can you also please include a line what the default config () means?

That was copy-pasted from elsewhere in the repository, so for sure there will be more cases around in other parts not covered in this PR. Just the heads up.

Addressed in 7bb7da3.

Can you also please include a line what the default config () means?

Missed that part. Done in 926e317.

ggwpez

Good modulo the comments, thanks.

bkchr · 2025-06-03T09:49:16Z

@moliholy ping

…marks-generic

moliholy · 2025-06-03T13:45:24Z

@ggwpez thanks for the review! Everything addressed.

@bkchr ping received. Ready to merge 🚀

…marks-generic

Similar to #4756 for `pallet-nfts`, the changes proposed in this PR intend to make benchmarks for `pallet-identity` signature-agnostic by the inclusion of a benchmark helper with sane defaults to handle the signing operations.

* master: (62 commits) release/build-macos-binaries: add missing FEATURES argument (#8816) Add XCM Precompile to `pallet-xcm` (#8693) [Release|CI/CD] Exclude test runtimes from the runtimes build (#8820) Add freebsd sysinfo for telemetry (#7985) release-reusable-rc-build: add optional `features` input that can be considered for nodes building (#8755) [Staking] Cleanups and some improvements (#8701) Fix typos in 3 files in Implementers Guide (#8799) Update `RemoteExporter` docs to reflect removal of `forward_id_for` (#8795) Snowbridge: enforce fee when registering Polkadot native asset (#8725) Bump the ci_dependencies group across 1 directory with 7 updates (#8788) Docker hub 'master' image short sha (#8790) [Release|CI/CD] Combine branch-off and RC automation flows (#8754) Move Transaction depth limit checks (#8750) Add genesis presets for remaining runtimes in polkadot-parachain-bin (#8426) Do not make pallet-identity benchmarks signature-dependent (#8179) Introduction of Approval Slashes [Disabling Strategy Stage 4] (#6827) [AHM] Prepare For Westend Cleanup (#8715) Actually use RP offset in YAP parachain (#8745) [AHM] Relax the requirement for RC-Client to receive +1 session reports (#8702) Don't read storage items in logging (#8749) ...

Backport #8179 into stable2412. This is to allow runtimes in `stable2412` that don't use SR25519 to correctly benchmark `pallet-identity` --------- Co-authored-by: Egor_P <[email protected]>

@kianenigma

Similar to #4756 for `pallet-nfts`, the changes proposed in this PR intend to make benchmarks for `pallet-identity` signature-agnostic by the inclusion of a benchmark helper with sane defaults to handle the signing operations.

Make pallet-identity benchmarks generic

f628b2c

moliholy requested a review from a team as a code owner April 8, 2025 10:35

José Molina added 2 commits April 8, 2025 12:37

Add prdoc

8e54bb6

Add helper to all instances of pallet-identity

96f4ad7

moliholy changed the title ~~Make pallet-identity benchmarks generic~~ Do not make pallet-identity benchmarks signature-dependent Apr 8, 2025

José Molina added 2 commits April 8, 2025 13:47

Fix tests for pallet-alliance

4753d31

fmt

71bc451

bkchr reviewed Apr 8, 2025

View reviewed changes

José Molina added 2 commits April 14, 2025 13:48

Rename Helper -> BenchmarkHelper

63c1e13

Change BenchmarkHelper trait

00e81d4

paritytech-review-bot bot requested a review from a team April 14, 2025 11:59

Merge remote-tracking branch 'origin/master' into feat/identity-bench…

be9c6ca

…marks-generic

José Molina added 2 commits April 14, 2025 14:18

Update prdoc

32cf21e

Address clippy issues

430f5de

bkchr approved these changes Apr 14, 2025

View reviewed changes

bkchr added the T2-pallets This PR/Issue is related to a particular pallet. label Apr 14, 2025

bkchr requested a review from ggwpez April 14, 2025 21:07

ggwpez reviewed Jun 2, 2025

View reviewed changes

substrate/frame/identity/src/benchmarking.rs Show resolved Hide resolved

ggwpez reviewed Jun 2, 2025

View reviewed changes

ggwpez approved these changes Jun 2, 2025

View reviewed changes

José Molina added 2 commits June 3, 2025 13:46

Merge remote-tracking branch 'origin/master' into feat/identity-bench…

24a3f45

…marks-generic

Address PR review comments

7bb7da3

moliholy force-pushed the feat/identity-benchmarks-generic branch from b6e3da0 to 7bb7da3 Compare June 3, 2025 11:57

Make clippy happy

d001cb1

José Molina added 2 commits June 4, 2025 10:37

Clarify default implementation

926e317

Merge remote-tracking branch 'origin/master' into feat/identity-bench…

40eff65

…marks-generic

bkchr added this pull request to the merge queue Jun 5, 2025

Merged via the queue into paritytech:master with commit 6cb6f5b Jun 5, 2025
274 of 277 checks passed

moliholy deleted the feat/identity-benchmarks-generic branch June 6, 2025 08:29

moliholy mentioned this pull request Jun 27, 2025

Add weights for pallet-identity paritytech/project-mythical#315

Merged

TDemeco mentioned this pull request Jul 3, 2025

[stable2412] Backport #8179 #9099

Merged

acatangiu mentioned this pull request Jul 17, 2025

Upgrade to Polkadot-SDK stable2506 polkadot-fellows/runtimes#817

Merged

21 tasks

github-actions bot mentioned this pull request Oct 24, 2025

Update polkadot-sdk from stable2503 to stable2506 moondance-labs/tanssi#1340

Closed

manuelmauro mentioned this pull request Nov 26, 2025

Upgrade to Polkadot stable2506 moonbeam-foundation/moonbeam#3488

Merged

	type Helper: BenchmarkHelper<
	type BenchmarkHelper: ::BenchmarkHelper<

		fn signer() -> (Public, AccountId);
		fn sign(signer: &Public, message: &[u8]) -> Signature;

-		fn signer() -> (Public, AccountId);
-		fn sign(signer: &Public, message: &[u8]) -> Signature;
+		/// Sign the given `message` with a random account.
+		///
+		/// Should return the public key of this random account and the signature.
+		fn sign_message(message: &[u8]) -> (Public, Signature);

Do not make pallet-identity benchmarks signature-dependent #8179

Do not make pallet-identity benchmarks signature-dependent #8179

Uh oh!

Conversation

moliholy commented Apr 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moliholy Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moliholy commented Apr 14, 2025

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

moliholy commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moliholy commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggwpez left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr commented Jun 3, 2025

Uh oh!

moliholy commented Jun 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moliholy Apr 8, 2025 •

edited

Loading

moliholy commented May 5, 2025 •

edited

Loading