Forcibly reduce credits_observed to the last epoch by ryoqun · Pull Request #13233 · solana-labs/solana

ryoqun · 2020-10-28T16:53:34Z

Problem

The initial inflation reward could be unexpected if inflation has not been enabled since genesis. (i.e. Virtually all of live clusters)
Subsequent inflation rewards are correct, though.

Conceptually, inflation rewards should be a fair distribution of a stake-weighted contribution metrics (called points) across validators for some reasonable recent timespan.

The current implementation fulfils it for all inflation rewards but the first one.
The reason is that the initial inflation reward calculation is based on the full timespan since genesis.
Specifically, the initial inflation calculation uses all epochs since genesis as the timespan for points summation.

This is due to implantation remnant before periodic forced inflation rewards distribution (the solana redeem age)
Technically, this is caused because stake account's credits_observed (key component of points calculation) aren't updated at all, until the very first inflation reward.
After that, as credits_observed is initialized, the inflation is calculated for each last epoch.

This creates the following counter intuitive inflation rewards.
As an extreme example, we reward currently undelegated stake account. (But this happens only once at the first epoch after inflation is enabled)

Currently, HDAhLXUZmFyvxT2gnGv36opsU6ZAXEEmcEzHMAghTBoT is undelegated:

$ ./target/release/solana stake-account HDAhLXUZmFyvxT2gnGv36opsU6ZAXEEmcEzHMAghTBoT
Balance: 0.00228288 SOL
Rent Exempt Reserve: 0.00228288 SOL
Stake account is undelegated
Stake Authority: 9LQhe3fHy9i7RVWMiFiyon3GfC9rCzoXiwc9DjaAY3b7
Withdraw Authority: 9LQhe3fHy9i7RVWMiFiyon3GfC9rCzoXiwc9DjaAY3b7

But, if we would enable the inflation now, the stake account receives +0.000016081 SOL. That's because initial inflation doesn't only look at the last epoch, but the entire blockchain history. So, this matches to the fact the stake account was actually delegating some SOLs (=~ ◎60) from epoch 58 to epoch 65:

        HDAhLXUZmFyvxT2gnGv36opsU6ZAXEEmcEzHMAghTBoT: Delegation {
            voter_pubkey: FKsC411dik9ktS6xPADxs4Fk2SCENvAiuccQHLAPndvk,
            stake: 59987617120,
            activation_epoch: 58,
            deactivation_epoch: 65,
            warmup_cooldown_rate: 0.25,
        },

HDAhLXUZmFyvxT2gnGv36opsU6ZAXEEmcEzHMAghTBoT(Stake11111111111111111111111111111111111111): ◎0.002282880 => ◎0.002298961 (+◎0.000016081 0.704417227%)

For comparison, similarly-sized stake account only receives 0.000000014 SOL.

H6rtteTo41ifz9XwMRx3yHXywssiGQizZYNCDQr4wZj1(Stake11111111111111111111111111111111111111): ◎0.010000000 => ◎0.010000014 (+◎0.000000014 0.000140000%)

In other words, staker will be rewarded proportional to the full delegated duration, not only the last epoch, with current implementation. So, this is a situation of act sooner and earn more.

Summary of Changes

Ideally, I think we should only consider the last epoch?
To realize that, this pr forcibly overwrites credits_observed before calculating inflation.

But this phenomenon is only one-time. and pico-inflation reward is very small. So, might not worth to fix this...

Context

Found via ledger-tool capitalization check.

codecov · 2020-10-28T18:03:25Z

Codecov Report

Merging #13233 (d9a07e7) into master (f5e0adc) will increase coverage by 0.1%.
The diff coverage is 92.5%.

@@            Coverage Diff            @@
##           master   #13233     +/-   ##
=========================================
+ Coverage    82.0%    82.2%   +0.1%     
=========================================
  Files         378      378             
  Lines       90905    91258    +353     
=========================================
+ Hits        74630    75073    +443     
+ Misses      16275    16185     -90

mvines · 2020-10-29T00:04:49Z

But this phenomenon is only one-time. and pico-inflation reward is very small. So, might not worth to fix this...

Yep we want to fix this! The effect is small but we don't need a muddled story for how inflation gets turned on and having to explain why that first epoch after enabling inflation is special.

ryoqun · 2020-10-29T02:56:19Z

Wow, test passed.

ryoqun · 2020-10-29T02:58:33Z

                    self.get_account(&delegation.voter_pubkey),
                ) {
-                    (Some(stake_account), Some(vote_account)) => {
+                    (Some(mut stake_account), Some(vote_account)) => {


this is least intrusive way. but a hacky way at the same time.

I'll do more commenting

mvines · 2020-10-30T15:08:30Z

This'll need a feature gate so it can be deployed out on testnet/devnet right?

ryoqun · 2020-10-30T16:39:28Z

This'll need a feature gate so it can be deployed out on testnet/devnet right?

Yeah, it seems so. I originally thought that this shouldn't cause any calculation changes for cluster where inflation is already enabled. But it seems not. ;) I'm now working on this.

mvines · 2020-10-31T15:58:59Z

This'll need a feature gate so it can be deployed out on testnet/devnet right?

Yeah, it seems so. I originally thought that this shouldn't cause any calculation changes for cluster where inflation is already enabled. But it seems not. ;) I'm now working on this.

We turned off inflation for devnet/testnet to test the inflation kill switch, and to make it easier to deploy this across all clusters. No gating required now I think!

ryoqun · 2020-11-08T14:04:10Z

+        stake_context: Option<(Epoch, &StakeHistory)>,
        inflation_point_calc_tracer: &mut Option<impl FnMut(&InflationPointCalculationEvent)>,
    ) -> (u128, u64) {
        // if there is no newer credits since observed, return no point


move comment along...

ryoqun · 2020-11-08T14:15:26Z

Also, add comment to note that now that VoteState.epoch_credits can be retired after checking this is really the case?

ryoqun · 2020-11-10T16:58:21Z

+                if epoch != target_epoch {
+                    return (0, 0);
+                }


write a test for this; this bug is later found after manual testing.

well, if we don't exclude historical vote_state's epoch credits by doing like this early return, this way, we might accidentally reward stale delegations using its (last and stale) epoch_credits, which is well past to target_epoch.

t-nelson · 2020-11-18T19:56:44Z

I happened back across this bug while investigating the viability of allowing fully-activated stake accounts to be merged. Would an alternate (and cleaner?) solution be to ensure that the stake account's observed_credits field is updated even when there were no rewards to be paid out, regardless of split? Caveat being that it needs feature gated, but stake-v2 isn't enabled yet 🎉

I think this can be achieved by changing the early return here...

solana/programs/stake/src/stake_state.rs

Lines 598 to 601 in f25c969

    
           if rewards == 0 { 
        
               return None; 
        
           } 
        
           let (voter_rewards, staker_rewards, is_split) = vote_state.commission_split(rewards);

... to something like ...

let (voter_rewards, staker_rewards, is_split) = if rewards == 0 {
    (0, 0, false)
} else {
    vote_state.commission_split(rewards)
};

... then everything resolves itself at the next epoch boundary.

I say, "something like," because I haven't yet checked whether rewards is accounting for a sub-lamport situation here.

This solution has the side-effect that I believe we can safely merge two qualified fully activated stake accounts with just a few extra checks from the deactivated case.

@ryoqun @CriesofCarrots @mvines WDYT?

t-nelson · 2020-11-19T00:16:54Z

        let mut points = 0;
        let mut new_credits_observed = self.credits_observed;

-        for (epoch, final_epoch_credits, initial_epoch_credits) in


Looking into this further... I believe this change can deny small stakes delegated to validators with small, but non-zero commissions ever getting paid out, as per

solana/programs/stake/src/stake_state.rs

Lines 611 to 616 in d9a07e7

if (voter_rewards == 0 || staker_rewards == 0) && is_split {

// don't collect if we lose a whole lamport somewhere

// is_split means there should be tokens on both sides,

// uncool to move credits_observed if one side didn't get paid

return None;

}

yeah! correct! I intentionally done this way now. My two cent is posted here: #13680 (comment)

CriesofCarrots · 2020-11-19T00:17:30Z

I think this can be achieved by changing the early return here...

solana/programs/stake/src/stake_state.rs

Lines 598 to 601 in f25c969

if rewards == 0 {

return None;

}

let (voter_rewards, staker_rewards, is_split) = vote_state.commission_split(rewards);

... to something like ...
let (voter_rewards, staker_rewards, is_split) = if rewards == 0 {
    (0, 0, false)
} else {
    vote_state.commission_split(rewards)
};
... then everything resolves itself at the next epoch boundary.

@t-nelson , it looks to me like your suggestion will resolve the issue motivating this PR nicely. The only caveat, as we discussed in DM, is that we need to activate your fix at least one epoch before we activate inflation (to prevent the additional rewards that @ryoqun describes on the epoch that observed_credits gets bumped).

I will hold off activating stake-program-v2 on testnet so that we don't have to plumb an additional feature.

ryoqun · 2020-11-21T04:00:16Z

hehe. what a baroque work compared to the state of art pr #13680 by @t-nelson.

Closing this, because this has happily superseded. :)

ericlwilliams mentioned this pull request Oct 28, 2020

Use pico inflation for ledger-tool capitalization --enable-inflation #13215

Merged

mvines added v1.3 labels Oct 29, 2020

ryoqun commented Oct 29, 2020

View reviewed changes

ryoqun force-pushed the credits-observed-overwrite branch from 070712d to 24ecc57 Compare October 29, 2020 04:54

ryoqun mentioned this pull request Oct 31, 2020

Small code cleanup and typo fixes #13325

Merged

ryoqun force-pushed the credits-observed-overwrite branch from 24ecc57 to 4065931 Compare November 2, 2020 14:50

ryoqun mentioned this pull request Nov 3, 2020

Show more detailed vote history in shorter format #13374

Merged

ryoqun commented Nov 7, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs

ryoqun force-pushed the credits-observed-overwrite branch from 5e14228 to 9bf5c3e Compare November 8, 2020 14:00

ryoqun commented Nov 8, 2020

View reviewed changes

ryoqun commented Nov 10, 2020

View reviewed changes

ryoqun added 3 commits November 16, 2020 03:49

Only use latest epoch_credits

2abac09

Fix test

6827052

Pass down target_epoch to exclude stale vote acct

8c41fac

ryoqun force-pushed the credits-observed-overwrite branch from 9bf5c3e to 8c41fac Compare November 15, 2020 19:09

t-nelson reviewed Nov 19, 2020

View reviewed changes

t-nelson mentioned this pull request Nov 19, 2020

stake: Don't pay out rewards for epochs where inflation was not enabled #13680

Merged

ryoqun closed this Nov 21, 2020

ryoqun mentioned this pull request Nov 27, 2020

Enhance ledger-tool for multi-epoch rewards #13837

Merged

	if (voter_rewards == 0 \|\| staker_rewards == 0) && is_split {
	// don't collect if we lose a whole lamport somewhere
	// is_split means there should be tokens on both sides,
	// uncool to move credits_observed if one side didn't get paid
	return None;
	}

Conversation

ryoqun commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Context

Uh oh!

codecov Bot commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mvines commented Oct 29, 2020

Uh oh!

ryoqun commented Oct 29, 2020

Uh oh!

ryoqun Oct 29, 2020

Choose a reason for hiding this comment

Uh oh!

mvines commented Oct 30, 2020

Uh oh!

ryoqun commented Oct 30, 2020

Uh oh!

mvines commented Oct 31, 2020

Uh oh!

Uh oh!

ryoqun Nov 8, 2020

Choose a reason for hiding this comment

Uh oh!

ryoqun commented Nov 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryoqun Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

ryoqun Nov 21, 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!

t-nelson commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-nelson Nov 19, 2020

Choose a reason for hiding this comment

Uh oh!

ryoqun Nov 19, 2020

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryoqun commented Nov 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ryoqun commented Oct 28, 2020 •

edited

Loading

codecov Bot commented Oct 28, 2020 •

edited

Loading

ryoqun commented Nov 8, 2020 •

edited

Loading

ryoqun Nov 21, 2020 •

edited

Loading

t-nelson commented Nov 18, 2020 •

edited

Loading

CriesofCarrots commented Nov 19, 2020 •

edited

Loading