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

Make pallet_session benchmarks generic over the staking solution #7522

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nanocryk
Copy link

Description

Current benchmarking code for pallet_session requires the runtime to implement pallet_staking from polkadot-dsk. It prevents registering and running benchmarks in runtimes that don't use that specific staking solution (like our project Tanssi).

This PR thus modifies the benchmarking code to add a StakingAdapter trait and associated type in pallet_session_benchmarking Config trait. Implementors of this trait are responsible for calling the appropriate pallet. The benchmarking crate also include a PalletStaking adapter that implements StakingAdapter to use pallet_staking like before.

Integration

Downstream projects that using pallet_session_benchmarking needs to specify the adapter when implementing the Config trait. If they use pallet_staking, they can change the following code in the dispatch_benchmark function of their runtime:

impl pallet_session_benchmarking::Config for Runtime {
+	type StakingAdapter = pallet_session_benchmarking::PalletStaking<Runtime>;
}

A project that doesn't use pallet_staking wouldn't have been able to use pallet_session_benchmarking in the first place. Now they can implement their own adapter that corresponds to their staking solution and setup pallet_session benchmarking.

Review Notes

pallet_session_benchmarking::Config now has an associated type StakingAdapter that must implement traitpallet_session_benchmarking::StakingAdapter, which contains various functions the benchmark needs to interact with the staking solution. Those functions are in a new trait + associated type to allow writing once the adapter instead of having to copy the function bodies in each runtime code. Adapter can be written once and used in multiple runtime (like PalletStaking that will work for any runtime that use polkadot-sdk pallet_staking).

Checklist

  • My PR includes a detailed description as outlined in the "Description" and its two subsections above.
  • My PR follows the labeling requirements of this project (at minimum one label for T required)
    • External contributors: ask maintainers to put the right label on your PR.
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

You can remove the "Checklist" section once all have been checked. Thank you for your contribution!

@nanocryk nanocryk requested a review from a team as a code owner February 10, 2025 15:10
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

The idea of fixing this is correct. However, I think we can make the benchmarking completely separate from staking.

For the historical benchmarks we probably need something that creates some fullidentification etc. For that we can change the Config trait as you have done it. For set_keys and purge_keys we should be able to setup the state without the staking pallet.

@nanocryk
Copy link
Author

set_keys and purge_keys functions use T::ValidatorIdOf::convert, which in westend runtime is set to pallet_staking::StashOf<Self>. The Convert trait doesn't provide an benchmark only function to circumvent this sadly. Any suggestion how to bench set_keys/purge_keys without this being an issue?

@paritytech-review-bot paritytech-review-bot bot requested a review from a team February 11, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants