Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ impl pallet_identity::Config for Runtime {
type UsernameGracePeriod = ConstU32<{ 3 * DAYS }>;
type MaxSuffixLength = ConstU32<7>;
type MaxUsernameLength = ConstU32<32>;
#[cfg(feature = "runtime-benchmarks")]
type Helper = ();
type WeightInfo = weights::pallet_identity::WeightInfo<Runtime>;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ impl pallet_identity::Config for Runtime {
type UsernameGracePeriod = ConstU32<{ 3 * DAYS }>;
type MaxSuffixLength = ConstU32<7>;
type MaxUsernameLength = ConstU32<32>;
#[cfg(feature = "runtime-benchmarks")]
type Helper = ();
type WeightInfo = weights::pallet_identity::WeightInfo<Runtime>;
}

Expand Down
2 changes: 2 additions & 0 deletions polkadot/runtime/common/src/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,8 @@ impl pallet_identity::Config for Test {
type UsernameGracePeriod = ConstU32<10>;
type MaxSuffixLength = ConstU32<7>;
type MaxUsernameLength = ConstU32<32>;
#[cfg(feature = "runtime-benchmarks")]
type Helper = ();
type WeightInfo = ();
}

Expand Down
2 changes: 2 additions & 0 deletions polkadot/runtime/rococo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,8 @@ impl pallet_identity::Config for Runtime {
type UsernameGracePeriod = ConstU32<{ 30 * DAYS }>;
type MaxSuffixLength = ConstU32<7>;
type MaxUsernameLength = ConstU32<32>;
#[cfg(feature = "runtime-benchmarks")]
type Helper = ();
type WeightInfo = weights::pallet_identity::WeightInfo<Runtime>;
}

Expand Down
2 changes: 2 additions & 0 deletions polkadot/runtime/westend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,8 @@ impl pallet_identity::Config for Runtime {
type UsernameGracePeriod = ConstU32<{ 30 * DAYS }>;
type MaxSuffixLength = ConstU32<7>;
type MaxUsernameLength = ConstU32<32>;
#[cfg(feature = "runtime-benchmarks")]
type Helper = ();
type WeightInfo = weights::pallet_identity::WeightInfo<Runtime>;
}

Expand Down
15 changes: 15 additions & 0 deletions prdoc/pr_8179.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Do not make pallet-identity benchmarks signature-dependent

doc:
- audience: Runtime Dev
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = ();
```

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add once we address my previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 63c1e13.


crates:
- name: pallet-identity
bump: minor
2 changes: 2 additions & 0 deletions substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1659,6 +1659,8 @@ impl pallet_identity::Config for Runtime {
type UsernameGracePeriod = ConstU32<{ 30 * DAYS }>;
type MaxSuffixLength = ConstU32<7>;
type MaxUsernameLength = ConstU32<32>;
#[cfg(feature = "runtime-benchmarks")]
type Helper = ();
type WeightInfo = pallet_identity::weights::SubstrateWeight<Runtime>;
}

Expand Down
2 changes: 2 additions & 0 deletions substrate/frame/alliance/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ impl pallet_identity::Config for Test {
type UsernameGracePeriod = UsernameGracePeriod;
type MaxSuffixLength = ConstU32<7>;
type MaxUsernameLength = ConstU32<32>;
#[cfg(feature = "runtime-benchmarks")]
type Helper = ();
type WeightInfo = ();
}

Expand Down
22 changes: 5 additions & 17 deletions substrate/frame/identity/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,7 @@ use frame_support::{
traits::{EnsureOrigin, Get, OnFinalize, OnInitialize},
};
use frame_system::RawOrigin;
use sp_io::crypto::{sr25519_generate, sr25519_sign};
use sp_runtime::{
traits::{Bounded, IdentifyAccount, One},
MultiSignature, MultiSigner,
};
use sp_runtime::traits::{Bounded, One};

const SEED: u32 = 0;

Expand Down Expand Up @@ -131,11 +127,7 @@ fn bounded_username<T: Config>(username: Vec<u8>, suffix: Vec<u8>) -> Username<T
Username::<T>::try_from(full_username).expect("test usernames should fit within bounds")
}

#[benchmarks(
where
<T as frame_system::Config>::AccountId: From<sp_runtime::AccountId32>,
T::OffchainSignature: From<MultiSignature>,
)]
#[benchmarks]
mod benchmarks {
use super::*;

Expand Down Expand Up @@ -625,16 +617,12 @@ mod benchmarks {
let username = bench_username();
let bounded_username = bounded_username::<T>(username.clone(), suffix.clone());

let public = sr25519_generate(0.into(), None);
let who_account: T::AccountId = MultiSigner::Sr25519(public).into_account().into();
let (public, who_account) = T::Helper::signer();
let who_lookup = T::Lookup::unlookup(who_account.clone());

let signature = MultiSignature::Sr25519(
sr25519_sign(0.into(), &public, &bounded_username[..]).unwrap(),
);
let signature = T::Helper::sign(&public, &bounded_username[..]);

// Verify signature here to avoid surprise errors at runtime
assert!(signature.verify(&bounded_username[..], &public.into()));
assert!(signature.verify(&bounded_username[..], &who_account));
let use_allocation = match p {
0 => false,
1 => true,
Expand Down
34 changes: 34 additions & 0 deletions substrate/frame/identity/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,32 @@ pub mod pallet {
use super::*;
use frame_support::pallet_prelude::*;

#[cfg(feature = "runtime-benchmarks")]
pub trait BenchmarkHelper<Public, AccountId, Signature> {
fn signer() -> (Public, AccountId);
fn sign(signer: &Public, message: &[u8]) -> Signature;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Again it differs from pallet-nfts equivalent code.
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 00e81d4.

}
#[cfg(feature = "runtime-benchmarks")]
impl
BenchmarkHelper<
sp_runtime::MultiSigner,
sp_runtime::AccountId32,
sp_runtime::MultiSignature,
> for ()
{
fn signer() -> (sp_runtime::MultiSigner, sp_runtime::AccountId32) {
let public = sp_io::crypto::sr25519_generate(0.into(), None);
let account = sp_runtime::MultiSigner::Sr25519(public).into_account();
(public.into(), account)
}
fn sign(signer: &sp_runtime::MultiSigner, message: &[u8]) -> sp_runtime::MultiSignature {
sp_runtime::MultiSignature::Sr25519(
sp_io::crypto::sr25519_sign(0.into(), &signer.clone().try_into().unwrap(), message)
.unwrap(),
)
}
}

#[pallet::config]
pub trait Config: frame_system::Config {
/// The overarching event type.
Expand Down Expand Up @@ -226,6 +252,14 @@ pub mod pallet {
#[pallet::constant]
type MaxUsernameLength: Get<u32>;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type Helper: BenchmarkHelper<
type BenchmarkHelper: ::BenchmarkHelper<

Copy link
Contributor Author

@moliholy moliholy Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BenchmarkHelper is more expressive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 63c1e13.

Self::SigningPublicKey,
Self::AccountId,
Self::OffchainSignature,
>;

/// Weight information for extrinsics in this pallet.
type WeightInfo: WeightInfo;
}
Expand Down
2 changes: 2 additions & 0 deletions substrate/frame/identity/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ impl pallet_identity::Config for Test {
type UsernameGracePeriod = ConstU64<2>;
type MaxSuffixLength = ConstU32<7>;
type MaxUsernameLength = ConstU32<32>;
#[cfg(feature = "runtime-benchmarks")]
type Helper = ();
type WeightInfo = ();
}

Expand Down
Loading