merging donal-ahm with master

[karolk91]

Merging donal-ahm with master

[cla-bot-2021]

User @Stephenlawrence00, please sign the CLA here.

[karolk91]

@ggwpez, Currently my governance tests on donal-ahm branch are failing, with error like:

---- open_gov_on_asset_hub::assethub_can_authorize_upgrade_for_system_chains stdout ----
thread 'open_gov_on_asset_hub::assethub_can_authorize_upgrade_for_system_chains' panicked at /Users/kkdev/Dev/paritytech/polkadot-sdk/substrate/frame/staking/src/pallet/mod.rs:792:5:
Expected Ok(_). Got Err(
    Module(
        ModuleError {
            index: 6,
            error: [
                33,
                0,
                0,
                0,
            ],
            message: Some(
                "Restricted",
            ),
        },
    ),
)


failures:
    open_gov_on_asset_hub::assethub_can_authorize_upgrade_for_itself
    open_gov_on_asset_hub::assethub_can_authorize_upgrade_for_relay_chain
    open_gov_on_asset_hub::assethub_can_authorize_upgrade_for_system_chains
    open_gov_on_relay::relaychain_can_authorize_upgrade_for_itself
    open_gov_on_relay::relaychain_can_authorize_upgrade_for_system_chains

I have narrowed it down to the type Filter = Everything change here:
51e9ad8

[Ank4n]

@sigurpol, @Ank4n

Could you please check why pallet_fast_unstake benchmarks are failing here:

The following 4 benchmarks failed:
- pallet_fast_unstake::deregister
- pallet_fast_unstake::on_idle_check
- pallet_fast_unstake::on_idle_unstake
- pallet_fast_unstake::register_fast_unstake

My best guess is: its a consequence of #8436 and benchmarking code here should be adapted with something like

diff --git a/substrate/frame/fast-unstake/src/benchmarking.rs b/substrate/frame/fast-unstake/src/benchmarking.rs
index db35b1f1b3..11f4bde5ea 100644
--- a/substrate/frame/fast-unstake/src/benchmarking.rs
+++ b/substrate/frame/fast-unstake/src/benchmarking.rs
@@ -51,6 +51,7 @@ fn fund_and_bond_account<T: Config>(account: &T::AccountId) {

        // bond and nominate ourselves, this will guarantee that we are not backing anyone.
        assert_ok!(T::Staking::bond(account, stake, account));
+       assert_ok!(T::Staking::validate(account, Default::default());
        assert_ok!(T::Staking::nominate(account, vec![account.clone()]));
 }

but T::Staking:: doesn't provide validate(..) method. I could probably extend trait StakingInterface with validate() but I lack domain knowledge in this area

Oh shoot, I think this is happening because fast-unstake bench is trying to call pallet-staking (old pallet existing on Westend runtime) but WAH runtime against which this bench is ran does not have pallet-staking, it has pallet-staking-async. I suspect even some other pallet benches (nom pool, delegated staking) will fail on WAH.

What needs to be done is – use new pallets in the bench as well as in the mocks of these pallets. And remove them from Westend.

I was planning to do it here, but currently tied up with other tasks. It makes more sense to make those changes in your PR though. Would you be able take a stab at it?

[sigurpol]

If you don't have buffer, I can try to fix them tomorrow, still need to wrap up a couple of tasks today

[sigurpol]

@sigurpol, @Ank4n
Could you please check why pallet_fast_unstake benchmarks are failing here:

The following 4 benchmarks failed:
- pallet_fast_unstake::deregister
- pallet_fast_unstake::on_idle_check
- pallet_fast_unstake::on_idle_unstake
- pallet_fast_unstake::register_fast_unstake

My best guess is: its a consequence of #8436 and benchmarking code here should be adapted with something like

diff --git a/substrate/frame/fast-unstake/src/benchmarking.rs b/substrate/frame/fast-unstake/src/benchmarking.rs
index db35b1f1b3..11f4bde5ea 100644
--- a/substrate/frame/fast-unstake/src/benchmarking.rs
+++ b/substrate/frame/fast-unstake/src/benchmarking.rs
@@ -51,6 +51,7 @@ fn fund_and_bond_account<T: Config>(account: &T::AccountId) {

        // bond and nominate ourselves, this will guarantee that we are not backing anyone.
        assert_ok!(T::Staking::bond(account, stake, account));
+       assert_ok!(T::Staking::validate(account, Default::default());
        assert_ok!(T::Staking::nominate(account, vec![account.clone()]));
 }

but T::Staking:: doesn't provide validate(..) method. I could probably extend trait StakingInterface with validate() but I lack domain knowledge in this area

Oh shoot, I think this is happening because fast-unstake bench is trying to call pallet-staking (old pallet existing on Westend runtime) but WAH runtime against which this bench is ran does not have pallet-staking, it has pallet-staking-async. I suspect even some other pallet benches (nom pool, delegated staking) will fail on WAH.

What needs to be done is – use new pallets in the bench as well as in the mocks of these pallets. And remove them from Westend.

I was planning to do it here, but currently tied up with other tasks. It makes more sense to make those changes in your PR though. Would you be able take a stab at it?

@Ank4n can you please kindly verify that this fix 07d7925 makes sense ?

My analysis: The benchmarks failed because they attempted to nominate accounts that were not properly registered as validators. #8436 correctly added validation to prevent this, fixing a silent failure. However, this change broke benchmarks with invalid setups. Wdyt?

[Ank4n]

If you don't have buffer, I can try to fix them tomorrow, still need to wrap up a couple of tasks today

Heads up:

@sigurpol, @Ank4n
Could you please check why pallet_fast_unstake benchmarks are failing here:

The following 4 benchmarks failed:
- pallet_fast_unstake::deregister
- pallet_fast_unstake::on_idle_check
- pallet_fast_unstake::on_idle_unstake
- pallet_fast_unstake::register_fast_unstake

My best guess is: its a consequence of #8436 and benchmarking code here should be adapted with something like

diff --git a/substrate/frame/fast-unstake/src/benchmarking.rs b/substrate/frame/fast-unstake/src/benchmarking.rs
index db35b1f1b3..11f4bde5ea 100644
--- a/substrate/frame/fast-unstake/src/benchmarking.rs
+++ b/substrate/frame/fast-unstake/src/benchmarking.rs
@@ -51,6 +51,7 @@ fn fund_and_bond_account<T: Config>(account: &T::AccountId) {

        // bond and nominate ourselves, this will guarantee that we are not backing anyone.
        assert_ok!(T::Staking::bond(account, stake, account));
+       assert_ok!(T::Staking::validate(account, Default::default());
        assert_ok!(T::Staking::nominate(account, vec![account.clone()]));
 }

but T::Staking:: doesn't provide validate(..) method. I could probably extend trait StakingInterface with validate() but I lack domain knowledge in this area

Oh shoot, I think this is happening because fast-unstake bench is trying to call pallet-staking (old pallet existing on Westend runtime) but WAH runtime against which this bench is ran does not have pallet-staking, it has pallet-staking-async. I suspect even some other pallet benches (nom pool, delegated staking) will fail on WAH.
What needs to be done is – use new pallets in the bench as well as in the mocks of these pallets. And remove them from Westend.
I was planning to do it here, but currently tied up with other tasks. It makes more sense to make those changes in your PR though. Would you be able take a stab at it?

@Ank4n can you please kindly verify that this fix 07d7925 makes sense ?

My analysis: The benchmarks failed because they attempted to nominate accounts that were not properly registered as validators. #8436 correctly added validation to prevent this, fixing a silent failure. However, this change broke benchmarks with invalid setups. Wdyt?

If it passes the tests then all good. I might have been wrong in my last assumption, looking at fast-unstake again, it should be fine since the dependency is via trait `StakingInterface. I am trying to replicate this issue locally.

[sigurpol]

If you don't have buffer, I can try to fix them tomorrow, still need to wrap up a couple of tasks today

Heads up:

@sigurpol, @Ank4n
Could you please check why pallet_fast_unstake benchmarks are failing here:

The following 4 benchmarks failed:
- pallet_fast_unstake::deregister
- pallet_fast_unstake::on_idle_check
- pallet_fast_unstake::on_idle_unstake
- pallet_fast_unstake::register_fast_unstake

My best guess is: its a consequence of #8436 and benchmarking code here should be adapted with something like

diff --git a/substrate/frame/fast-unstake/src/benchmarking.rs b/substrate/frame/fast-unstake/src/benchmarking.rs
index db35b1f1b3..11f4bde5ea 100644
--- a/substrate/frame/fast-unstake/src/benchmarking.rs
+++ b/substrate/frame/fast-unstake/src/benchmarking.rs
@@ -51,6 +51,7 @@ fn fund_and_bond_account<T: Config>(account: &T::AccountId) {

        // bond and nominate ourselves, this will guarantee that we are not backing anyone.
        assert_ok!(T::Staking::bond(account, stake, account));
+       assert_ok!(T::Staking::validate(account, Default::default());
        assert_ok!(T::Staking::nominate(account, vec![account.clone()]));
 }

but T::Staking:: doesn't provide validate(..) method. I could probably extend trait StakingInterface with validate() but I lack domain knowledge in this area

Oh shoot, I think this is happening because fast-unstake bench is trying to call pallet-staking (old pallet existing on Westend runtime) but WAH runtime against which this bench is ran does not have pallet-staking, it has pallet-staking-async. I suspect even some other pallet benches (nom pool, delegated staking) will fail on WAH.
What needs to be done is – use new pallets in the bench as well as in the mocks of these pallets. And remove them from Westend.
I was planning to do it here, but currently tied up with other tasks. It makes more sense to make those changes in your PR though. Would you be able take a stab at it?

@Ank4n can you please kindly verify that this fix 07d7925 makes sense ?
My analysis: The benchmarks failed because they attempted to nominate accounts that were not properly registered as validators. #8436 correctly added validation to prevent this, fixing a silent failure. However, this change broke benchmarks with invalid setups. Wdyt?

If it passes the tests then all good. I might have been wrong in my last assumption, looking at fast-unstake again, it should be fine since the dependency is via trait `StakingInterface. I am trying to replicate this issue locally.

But we need to update tests (not benchmarks) under fast-unstake because they are passing JUST because they are relying on staking and not staking-async. Seems out of scope of this PR though so I Would do it in your usual other PR #8807 not to pollute this one

[Ank4n]

@sigurpol thanks, your fix is all we need here. 🚀

[paritytech-workflow-stopper]

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/15906183877
Failed job name: test-linux-stable

[ggwpez]

Will fix the remaining ones in the other MR.