Flexible payout for validator and all nominators by shawntabrizi · Pull Request #5268 · paritytech/substrate

shawntabrizi · 2020-03-17T02:58:44Z

IN PROGRESS: THIS PR NEEDS TO BE POLISHED WITH ADDITIONAL TESTS.

This is a PR which introduces an alternative payout mechanism somewhere in the middle of automatic payouts and lazy payouts.

Main problems to be solved:

With automatic payouts, we were triggering payouts for all users at a single block. This was a ton of computation, and causing the block production to stall. This was not economically viable for the long term as more and more validators and nominators joined the system.
With lazy payouts every validator and nominator needed to request payouts for all of their rewards. This punished small nominators who may pay more in underlying fees than they would be rewarded. Furthermore, it required all users actively claim their rewards every 84 eras.

Solution:

Create a payout mechanism that pays out to a validator and all nominators for that validator in a single transaction.

Intended Behavior:

Validators are the ones most incentivized to claim rewards as they are the ones most likely to receive the largest cut of the pie. They are also most likely to stay up to date with the network and regularly interact with it through transactions, voting, etc...

Validators want nominators to select them so that they stay in the validator pool.

As a result, we can put the ownership of making payouts to the validators, and allow nominators to get the much preferred "automatic payout"-like system

Changes:

Remove payout_nominator
Combine do_payout_validator and do_payout_nominator
Replace last_reward single value storage tracker with claimed_rewards vector
- claimed_rewards is a vector which keeps track of all the eras a validator has made a claim between the current era and current era - history_depth. This allows for out of order claiming.
Introduce max_payout validator preference, allowing them to select how many users to pay out to in an era.
Make payout_validator callable by anyone where they specify the validator to pay out.
Removed ErasStakersClipped which redundantly kept the same data around. (Although maybe we should keep it as it would reduce storage read at the time of claiming...)

Features:

The main feature of this PR is that nominators need to do nothing in order to get paid by the staking system. For nominators, it feel as if we had automatic payouts.
The burden is then placed on validators, who should regularly claim rewards, and will automatically payout all their validators.
Paying out more nominators will inherently increase the fee of making a payout. So a validator can now signal their intention of the max number of nominators they want to payout. This allows them to control the max fee they will pay for their own payout, and remove any attack vector where micro-nominators would make it too hard for the validator to claim their rewards. This is configurable per validator unlike lazy payouts which is global.
Nominators can now use this max_payout preference to help in their decision making when selecting nominators. If a validator selects max_payout = 1, they are guaranteed to pay a very low fee when claiming rewards, but not many nominators would be incentivized to select them. So a middle ground is good, and the default is currently 64, just like in lazy payouts.
Validators are not strictly required to make a payout transaction. The extrinsic has been updated so that it can be called by anyone. This allows nominators who may have significant rewards to claim those rewards by making a payout on behalf of the validator. Additionally, a validator need not pull out their controller keys to make regular payouts, they can just simply use any burner account to make a transaction to payout their validator account and all their nominators. To support this, we need to support out of order claiming, which I have added by tracking which eras the user has claimed in a truncated vector.

TODO:

Get feedback that this is a reasonable direction.
Polish code to make sure it is well optimized for storage/computation.
Benchmark the new extrinsic to compare performance in expected real world scenarios as compared to lazy payouts.
Write comprehensive tests verifying features and functionality.

cc @jacogr @thiolliere @gavofyork

shawntabrizi · 2020-03-17T03:01:43Z

frame/staking/src/lib.rs

+		ledger.claimed_rewards.retain(|&x| x >= current_era.saturating_sub(history_depth));
+		match ledger.claimed_rewards.binary_search(&era) {
+			Ok(_) => Err(Error::<T>::InvalidEraToReward)?,
+			Err(pos) => ledger.claimed_rewards.insert(pos, era),


This retain + binary search should be combined into a single operation over the vector.

shawntabrizi · 2020-03-17T03:04:15Z

frame/staking/src/lib.rs

-					exposure_clipped.others.sort_unstable_by(|a, b| a.value.cmp(&b.value).reverse());
-					exposure_clipped.others.truncate(clipped_max_len);
-				}
-				<ErasStakersClipped<T>>::insert(&current_era, &c, exposure_clipped);


There is a dilemma here:

Do we want to keep a copy of the stakers information for every staker for a bunch of eras, or do we want to have it so that some validators invoke reading of a vec which is large.

I think removing this redundant storage item is better, but could be wrong.

frame/staking/src/lib.rs

joepetrowski · 2020-03-17T14:34:54Z

frame/staking/src/lib.rs

-
-		if nominator_ledger.last_reward.map(|last_reward| last_reward >= era).unwrap_or(false) {
-			return Err(Error::<T>::InvalidEraToReward.into());
+		let mut ledger = <Ledger<T>>::get(&validator).ok_or_else(|| Error::<T>::NotController)?;


I'm guessing people would actually provide the stash address of the validator, since that's how they nominate them?

joepetrowski · 2020-03-17T14:41:58Z

frame/staking/src/lib.rs

+		let max_payout = validator_prefs.max_payout as usize;
+		let mut nominators_exposure = exposure.others;
+		if nominators_exposure.len() > max_payout {
+			nominators_exposure.sort_unstable_by(|a, b| b.value.cmp(&a.value));
+			nominators_exposure.truncate(max_payout);


I think your intent is to limit the number of stakers that the transactor pays out, but this logic here prevents people outside of validator_prefs.max_payout from ever claiming their reward, even if they are willing to pay a tx fee to get it. This breaks down to some weird incentives, especially for bigger validators, like setting max_payout to 1 so that only you nominate yourself and if someone else out-nominates you then your incentive is to misbehave and slash them.

It would be a slightly difficult to allow for an additional payout nominator. It could not be used by the max_payouts people since we do not separately track which payouts they have claimed, and would be at risk of a double payout.

I could make it so it works for anyone not in the max_payouts group, but then the API is a little shitty since it cannot be used by everyone.

I personally don't think things like max_payout = 1(which is inherently a kind of flawed setting) need to be handled well, since we should discourage a validator from selecting this, but there are def solutions if you think it is necessary.

It's really hard (impossible) to set a proper weight for this function because you don't know max_payout in advance for the validator in question. A validator could set this to be very high and make the nomination accounts to attack the chain.

I would rather see a solution that doesn't give the validator this power, but rather does pay out all nominators by:

Ordering nominators by exposure.

Subtracting some fee from the reward.

Truncate the list by only paying out those for whom reward is greater than zero (or some threshold like ED/10)

Why max_payout - why not just payout all? (Excuse my ignorance, sometimes I need a "lemme ask really stupid question" forum)

I think we are always asking you stupid questions. We are trying to avoid the situation with too many state updates in a single transaction. But I would rather pay out all as long as there is a way to decide if an account is worth paying out.

It's really hard (impossible) to set a proper weight for this function because you don't know max_payout in advance for the validator in question. A validator could set this to be very high and make the nomination accounts to attack the chain.

This is not really correct. We will likely refactor all extrinsics to include information like this. So in the next iteration, max_payouts will be a value that has to be input into the extrinsic, and matched against what is in storage. If it does not match correctly, then we do nothing.

We will charge the appropriate fee for that action.

Why max_payout - why not just payout all? (Excuse my ignorance, sometimes I need a "lemme ask really stupid question" forum)

This is an attack vector. A malicious actor can make many many micro nominators to prevent a validator from doing a payout with a reasonable fee.

This is not really correct. We will likely refactor all extrinsics to include information like this. So in the next iteration, max_payouts will be a value that has to be input into the extrinsic, and matched against what is in storage. If it does not match correctly, then we do nothing.

We will charge the appropriate fee for that action.

I still don't see how this is a superior solution than deducting a fee from each staker's rewards and truncating the list based on that. You are giving validators the power to truncate the list of rewarded accounts rather than using an economic mechanism. Charging a fee per rewarded account would basically add a cost to nominators for dividing their account up into smaller accounts that all nominate the same target, which is exactly the behavior we want to disincent. Just giving this param to validators seems like a hack to me.

Ah, I see what you mean, and generally it makes sense, but our weight/fee system isnt quite smart enough to handle this.

The underlying weight of the call needs to appropriately measured/reported for the purposes of forming blocks, and our fee mechanism depends directly on this weight. So even if we do try to "take fees from the rewards" we will still need to increase the weight of the call proportional to the number of people paid out, and ultimately this would need to be known ahead of time, and the fee associated with this will be charged to the sender.

frame/staking/src/lib.rs

gui1117

So the number of nominator per validator will be limited to the number of account we can reward in one block, but I don't think this is an issue.

gui1117 · 2020-03-18T11:16:06Z

frame/staking/src/lib.rs


-		// Note: if era has no reward to be claimed, era may be future. better not to update
-		// `nominator_ledger.last_reward` in this case.
 		let era_payout = <ErasValidatorReward<T>>::get(&era)
 			.ok_or_else(|| Error::<T>::InvalidEraToReward)?;


you removed the comment but the logic still applies, the ? here will return if the era_payout is None

gui1117 · 2020-03-18T18:33:09Z

additional note: if the fees to claim reward for a validator and its n first nominators is important, maybe nobody wants to claim and one has to, at the end, pay the fee "for other". But this can be solved by paying the fee of claiming reward from inside the reward. A staker would claim the reward, it will first take from the total_reward for the validator and n nominator a small part corresponding to the fee.
Thus the staker at the end pay zero fee from claiming for other.

shawntabrizi · 2020-03-25T00:09:57Z

I have not updated docs, but the code + tests should tell anyone with an opinion if they like the design or not. Feel free to review, and if it is positive, then I will complete the work needed here.

shawntabrizi · 2020-03-26T12:19:32Z

Superseded by #5406

shawntabrizi added 4 commits March 9, 2020 14:25

Update lib.rs

3881632

compiles

cbf5dec

Merge branch 'master' into shawntabrizi-better-payout

e110a3c

Better payout

241d2e0

shawntabrizi added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 17, 2020

shawntabrizi requested a review from kianenigma as a code owner March 17, 2020 02:58

shawntabrizi commented Mar 17, 2020

View reviewed changes

bump

5fc67ed

joepetrowski reviewed Mar 17, 2020

View reviewed changes

Rename and have max_nominator_payouts as input

4bf830d

gui1117 reviewed Mar 18, 2020

View reviewed changes

shawntabrizi added 3 commits March 19, 2020 01:32

Merge branch 'master' into shawntabrizi-better-payout

06534ce

Soft fix compile errors

c9b0433

Add nominator extra payout with tests for everything

82a4b4b

paritytech-ci added the B2-breaksapi label Mar 23, 2020

shawntabrizi added 3 commits March 23, 2020 01:13

Merge branch 'master' into shawntabrizi-better-payout

e9b5695

Update benchmarking.rs

96a3ca6

Use stash for `payout_stakers"

d85100d

shawntabrizi added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 25, 2020

shawntabrizi mentioned this pull request Mar 26, 2020

Simple Staking Payouts #5406

Merged

3 tasks

shawntabrizi closed this Mar 26, 2020

Conversation

shawntabrizi commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawntabrizi Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gui1117 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gui1117 commented Mar 18, 2020

Uh oh!

shawntabrizi commented Mar 25, 2020

Uh oh!

shawntabrizi commented Mar 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shawntabrizi commented Mar 17, 2020 •

edited

Loading

shawntabrizi Mar 18, 2020 •

edited

Loading

gui1117 left a comment •

edited

Loading