-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[pallet-staking] Refund unused weight for payout_stakers
#8458
Conversation
IMO, no. If we want this optimization, it can happen in the future. |
/benchmark runtime pallet pallet_staking |
Finished benchmark for branch: zeke-staking-payout-fn-refund Benchmark: Benchmark Runtime Pallet cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs ResultsPallet: "pallet_staking", Extrinsic: "bond", Lowest values: [], Highest values: [], Steps: [50], Repeat: 20
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks right to me besides the nits above
…/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs
…ritytech/substrate into zeke-staking-payout-fn-refund
ensure!(era >= current_era.saturating_sub(history_depth), Error::<T>::InvalidEraToReward); | ||
ensure!( | ||
era <= current_era && era >= current_era.saturating_sub(history_depth), | ||
Error::<T>::InvalidEraToReward.with_weight(T::WeightInfo::payout_stakers_alive_staked(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is not really accurate, because by this point you've done 2 DB reads and almost nothing more, while T::WeightInfo::payout_stakers_alive_staked(0)
is probably a lot more.
I am fine with this, since it is still a worse case, which is good.
Also fine with Error::<T>::InvalidEraToReward.with_weight(T::DbWeight::get().read(X))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think incorrect extrinsics are perfectly fine to overcharge a bit. A simplification like the one you are mentioning risks being under valued, and thus opens an attack.
@@ -269,6 +269,8 @@ where | |||
} | |||
|
|||
pub type Extrinsic = TestXt<Call, ()>; | |||
pub(crate) type StakingCall = crate::Call<Test>; | |||
pub(crate) type TestRuntimeCall = <Test as frame_system::Config>::Call; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should really just rename the Call
generated by the construct_runtime! to OuterCall
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not realted to this PR, I am just ranting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue open for this / should we open an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no issue afaik.
@@ -2976,6 +2978,9 @@ fn claim_reward_at_the_last_era_and_no_double_claim_and_invalid_claim() { | |||
// * an invalid era to claim doesn't update last_reward | |||
// * double claim of one era fails | |||
ExtBuilder::default().nominate(true).build_and_execute(|| { | |||
// Consumed weight for all payout_stakers dispatches that fail | |||
let err_weight = weights::SubstrateWeight::<Test>::payout_stakers_alive_staked(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should do the same patter in the main function as well, the code there is quite verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically, it would be one more computation that is not needed if the 0 weight is not used...
Not for now, that would be a lot of gas-metering-like detailed code for very little benefit.
See my comment above -- I think it is fine, since it is always an overestimate. |
@kianenigma I added in the |
@kianenigma you happy? (about this pr and in general) |
bot merge |
Trying merge. |
* master: (84 commits) Duplicate logging to stdout (#8495) Fix sync restart (#8497) client: fix justifications migration (#8489) helper macro to create storage types on the fly (#8456) Make `BlockImport` and `Verifier` async (#8472) Get rid of `test-helpers` feature in sc-consensus-babe (#8486) Enhancement on Substrate Node Template (#8473) Add Social Network (#8065) Prepare UI tests for Rust 1.51 & new CI image (#8474) Benchmarking pallet-example (#8301) Use pathbuf for remote externalities (#8480) Bring back the on_finalize weight of staking. (#8463) Implement `fungible::*` for Balances (#8454) make types within `generate_solution_type` macro explicit (#8447) [pallet-staking] Refund unused weight for `payout_stakers` (#8458) Use `async_trait` in sc-consensus-slots (#8461) Repot frame_support::traits; introduce some new currency stuff (#8435) Fix &mut self -> &self in add_known_address (#8468) Add NetworkService::add_known_address (#8467) Fix companion check (#8464) ...
…h#8458) * [pallet-staking] Refund unused weight for `payout_stakers` fixes paritytech#8428 * Use periods in comments * cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs * Address Shawn's Feedback * Assert monotomic weights && improve test note * Remove stray new line * debug_assert payout_count <= max * Only track payouts to nominators; not validators * Trivial comment update Co-authored-by: Parity Benchmarking Bot <admin@parity.io>
* [pallet-staking] Refund unused weight for `payout_stakers` fixes #8428 * Use periods in comments * cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs * Address Shawn's Feedback * Assert monotomic weights && improve test note * Remove stray new line * debug_assert payout_count <= max * Only track payouts to nominators; not validators * Trivial comment update Co-authored-by: Parity Benchmarking Bot <admin@parity.io>
fixes #8428
Overview
This PR enables
payout_stakers
to refund weight based on how many payout ops where completed. Previously, the validators payout op was not taken into account for the max weight; this updates the logic so the maximum weight reflects payout ops forMaxNominatorRewardedPerValidator
+ the validator.Discussion
From the original issue:
The current iteration of this PR does not discriminate weight used based on payout destination variant and instead just assumes worst case for every payout op.
Questions for review:
WeightInfo::payout_stakers_alive_staked
- is this ok?