Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Payout function should refund #8428

Closed
kianenigma opened this issue Mar 23, 2021 · 3 comments · Fixed by #8458
Closed

Payout function should refund #8428

kianenigma opened this issue Mar 23, 2021 · 3 comments · Fixed by #8458
Assignees
@emostov
Labels
I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. U2-some_time_soon Issue is worth doing soon. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.

Comments

@kianenigma
Copy link
Contributor

kianenigma commented Mar 23, 2021 •
edited
Loading

Current payout function of staking assumes that any payout pays exactly T::MaxNominationPerValidator accounts, not to mention that it also assumes the worse case of account target as well: Staked.

We should make this refund along two axes:

  1. If we pay X nominators, we should take this into account.
  2. Each payout variant should have its own weight benchmark, and we must add them up. So the function will still use the upfront worse case weight, but will refund quite a bit along the way.

Once we have this, we can bump the MaxNominationPerValidator to a much larger number safely, effectively nilling the chance of oversubscribed nominators.

related paritytech/polkadot#2637
@kianenigma kianenigma added I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. U2-some_time_soon Issue is worth doing soon. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. and removed Z1-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder labels Mar 23, 2021
@shawntabrizi
Copy link
Member

Each payout variant should have its own weight benchmark, and we must add them up. So the function will still use the upfront worse case weight, but will refund quite a bit along the way.

I think this is probably overkill. Just track how many below the max payouts we actually made and make a refund there?

@kianenigma
Copy link
Contributor Author

Arguably yes, the former is much more important.

@kianenigma
Copy link
Contributor Author

@emostov wanna try this one to show us what you've got? :P

emostov added a commit that referenced this issue Mar 25, 2021
@emostov
[pallet-staking] Refund unused weight for payout_stakers
eb439e4 
fixes #8428
@emostov emostov mentioned this issue Mar 25, 2021
Merged
@ghost ghost closed this as completed in #8458 Mar 28, 2021
ghost pushed a commit that referenced this issue Mar 28, 2021
@emostov
[pallet-staking] Refund unused weight for payout_stakers (#8458)
ec8a661 
* [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>
hirschenberger pushed a commit to hirschenberger/substrate that referenced this issue Apr 14, 2021
@emostov @hirschenberger
[pallet-staking] Refund unused weight for payout_stakers (paritytec…
de9c90c 
…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>
shawntabrizi pushed a commit that referenced this issue Jun 17, 2021
@emostov @shawntabrizi
[pallet-staking] Refund unused weight for payout_stakers (#8458)
898d78a 
* [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>
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@emostov emostov

Labels
I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. U2-some_time_soon Issue is worth doing soon. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.
Projects
(Nominated) Proof of Stake
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[pallet-staking] Refund unused weight for payout_stakers paritytech/substrate
3 participants
@shawntabrizi @kianenigma @emostov