[AHM] Final Staking Configs for Kusama #883
Conversation
I think it should be: according to the setup: |
|
/cmd bench --runtime asset-hub-kusama --pallet pallet_election_provider_multi_block pallet_election_provider_multi_block_signed pallet_election_provider_multi_block_unsigned pallet_election_provider_multi_block_verifier pallet_staking_async --clean |
|
/cmd bench --runtime asset-hub-kusama --pallet pallet_election_provider_multi_block pallet_election_provider_multi_block_signed pallet_election_provider_multi_block_unsigned pallet_election_provider_multi_block_verifier pallet_staking_async --clean |
|
Command "bench --runtime asset-hub-kusama --pallet pallet_election_provider_multi_block pallet_election_provider_multi_block_signed pallet_election_provider_multi_block_unsigned pallet_election_provider_multi_block_verifier pallet_staking_async --clean" has started 🚀 See logs here |
| RuntimeParameters::Treasury(dynamic_params::treasury::Parameters::BurnPortion( | ||
| dynamic_params::treasury::BurnPortion, | ||
| Some(Permill::from_percent(0)), | ||
| // TODO: @kianenigma/!bkontur - is this ok? |
There was a problem hiding this comment.
Why is this needed to begin with? some benchmarks fail without it? I though the default for MinInflation is already defined above in all cases.
There was a problem hiding this comment.
Why is this needed to begin with? some benchmarks fail without it? I though the default for
MinInflationis already defined above in all cases.
I don't know, this impl Default is kind of confusing, but something on the way requires that.
I just copy this according to actual relay/kusama: https://github.com/polkadot-fellows/runtimes/blob/main/relay/kusama/src/lib.rs#L753-L756
There was a problem hiding this comment.
Benchmarks have a where bound for RuntimeParameters: Default. So you just return any variant, does not matter which. Otherwise the benchmark would have no way to construct this opaque type.
relay/kusama/src/lib.rs
Outdated
| type UnixTime = Timestamp; | ||
| type PointsPerBlock = ConstU32<20>; | ||
| // TODO @kianenigma | ||
| type MaxOffenceBatchSize = ConstU32<50>; |
| let message = SessionReportToXcm::convert(session_report); | ||
| let dest = AssetHubLocation::get(); | ||
| let _ = xcm::prelude::send_xcm::<xcm_config::XcmRouter>(dest, message).inspect_err(|err| { | ||
| log::error!(target: "runtime::ah-client", "Failed to send relay session report: {:?}", err); |
There was a problem hiding this comment.
We can't handle the error before paritytech/polkadot-sdk#9619
We might be able to merge it next week, let's see.
| // TODO: after https://github.com/paritytech/polkadot-sdk/pull/9619, use `XCMSender::send` and handle error | ||
| let _ = send_xcm::<xcm_config::XcmRouter>(AssetHubLocation::get(), message).inspect_err( | ||
| |err| { | ||
| log::error!(target: "runtime::ah-client", "Failed to send relay offence message: {:?}", err); |
There was a problem hiding this comment.
We can't handle the error before paritytech/polkadot-sdk#9619
We might be able to merge it next week, let's see.
| @@ -69,7 +69,7 @@ fn asset_hub_kusama_genesis( | |||
| }, | |||
| "staking": { | |||
| "validatorCount": 1000, | |||
| "devStakers": Some((2_000, 25_000)), | |||
| "devStakers": Some((4_000, 15_000)), | |||
There was a problem hiding this comment.
I had some TODO in a checklist for you to set some Genesis config values for Kusama, is this it? Can i Check that off?
There was a problem hiding this comment.
Yeah this is correct now and no benchmarks shoulf fail.
| /// Parameters used to calculate staking era payouts. | ||
| #[dynamic_pallet_params] | ||
| #[codec(index = 0)] | ||
| pub mod inflation { |
There was a problem hiding this comment.
Not sure if you saw Gavs recent interview, but he basically argued that Inflation is not the right terminology, but rather Interest or Issuance.
| // TODO: @kianenigma/!bkontur - do we want to allow StakingAdmin here? | ||
| Inflation(_) => frame_system::ensure_root(origin.clone()), |
There was a problem hiding this comment.
| // TODO: @kianenigma/!bkontur - do we want to allow StakingAdmin here? | |
| Inflation(_) => frame_system::ensure_root(origin.clone()), | |
| Inflation(_) => frame_system::ensure_root(origin.clone()), |
I think root is okay, there should only be changes needed to this rarely.
There was a problem hiding this comment.
True, inflation should be root-only, will fix.
| // NOTES: | ||
| // * a lot of the parameters are defined as `storage` such that they can be upgraded with a smaller | ||
| // overhead by the root origin, as opposed to a code upgrade. We expect to remove them. If all | ||
| // goes well in Kusama AHM, we can stop using `storage` in Polkadot. |
There was a problem hiding this comment.
They could also be in the parameter pallet
| /// At the time of writing, Kusama has 1000 active validators, and ~2k validator candidates. | ||
| /// | ||
| /// Safety note: This increases the weight of `on_initialize_into_snapshot_msp` weight. Double check TODO @kianenigma. | ||
| pub storage TargetSnapshotPerBlock: u32 = 3000; |
| 5 | ||
| } else { | ||
| if matches!( | ||
| pallet_ah_migrator::AhMigrationStage::<Runtime>::get(), |
There was a problem hiding this comment.
| pallet_ah_migrator::AhMigrationStage::<Runtime>::get(), | |
| pallet_ah_migrator::AhMigrationStage::<Runtime>::get().is_finished() |
There should be a function like this, but it would do the same.
system-parachains/asset-hubs/asset-hub-kusama/src/staking/mod.rs
Outdated
Show resolved
Hide resolved
| use crate::dynamic_params; | ||
|
|
||
| // TODO @kianenigma one sanity check test for this as we had in the RC | ||
| let params = polkadot_runtime_common::impls::EraPayoutParams { |
| /// confused in case AH block times change. Ideally, this value should be updated alongside AH's | ||
| /// block time. If AH blocks progress faster, our eras will become shorter, which is not a | ||
| /// critical issue. | ||
| pub const RelaySessionDuration: BlockNumber = 1 * HOURS; |
There was a problem hiding this comment.
| pub const RelaySessionDuration: BlockNumber = 1 * HOURS; | |
| pub const RelaySessionDuration: BlockNumber = 1 * RC_HOURS; |
Or some renamed import to distinguish please.
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
| for InitiateStakingAsync<EXPECTED_SPEC> | ||
| { | ||
| fn on_runtime_upgrade() -> Weight { | ||
| if crate::VERSION.spec_version == EXPECTED_SPEC { |
There was a problem hiding this comment.
These spec migrations have gone wrong so many time 🙈, would be happy if we can avoid it.
| let minimum_score = sp_npos_elections::ElectionScore { | ||
| minimal_stake: 2957640724907066, | ||
| sum_stake: 3471819933857856584, | ||
| sum_stake_squared: 78133097080615021100202963085417458, |
There was a problem hiding this comment.
So this value should be larger than recent election sum_stake_squared? Recent election event here https://kusama.subscan.io/event/29967817-0 looks like about 2^115.7, whereas the value here is 2^115.9.
Just asking since i thought this should be a lower bound.
There was a problem hiding this comment.
Correct, this value should be larger. The reason being that the lower this value, the better the election is (smaller variance). So the the "lower bound (aka. worst bound)" in this case is a larger value.
| use super::*; | ||
|
|
||
| // TODO: @kianenigma | ||
| mod message_sizes { |
| @@ -803,7 +810,7 @@ impl pallet_staking::EraPayout<Balance> for EraPayout { | |||
|
|
|||
| let params = relay_common::EraPayoutParams { | |||
| total_staked, | |||
| total_stakable: 0, // TODO: double check | |||
| total_stakable: Balances::total_issuance(), | |||
There was a problem hiding this comment.
this used to be Nis total issuance
There was a problem hiding this comment.
correct, and NIS is not part of KAH anymore. Balances TI is the right alternative, and what we have been using in Polkadot all the while.
…' into kiz-staking-ahm-kusama
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
|
Command "bench --runtime asset-hub-kusama --pallet pallet_election_provider_multi_block pallet_election_provider_multi_block_signed pallet_election_provider_multi_block_unsigned pallet_election_provider_multi_block_verifier pallet_staking_async --clean" has failed ❌! See logs here |
|
We should benchmark on top of this #856 |
ggwpez
merged commit 71ace0f
into
oty-dev-asset-hub-migration-2506
Follow up to #883, addressing some review comments. - Inflation -> Issuance rename - More use of parameters pallet for scheduler, MQ, and staking elections - Migration to initialize staking-async is not reliant on spec version --------- Co-authored-by: Oliver Tale-Yazdi <[email protected]>
No description provided.