Don't reset credits_observed due to stale voters by ryoqun · Pull Request #13836 · solana-labs/solana

ryoqun · 2020-11-26T22:48:18Z

Problem

#13680 resets stake account's credits_observed to 0 (returned from calculate_points_and_credits()) for stale vote accounts. That makes the stake account is incorrectly eligible for inflation rewords:

solana/programs/stake/src/stake_state.rs

Lines 625 to 635 in 0b00a1b

    
           let (points, credits_observed) = self.calculate_points_and_credits( 
        
               vote_state, 
        
               stake_history, 
        
               inflation_point_calc_tracer, 
        
               fix_stake_deactivate, 
        
           ); 
        
           // Drive credits_observed forward unconditionally when rewards are disabled 
        
           if point_value.rewards == 0 && fix_stake_deactivate { 
        
               return Some((0, 0, credits_observed)); 
        
           }

For fun fact, at the next epoch, credits_observed will reset back to vote's credits. Then, yet another next epoch, it'll be again be 0. and so on.

So, when the inflation is activated at the odd-number after the stake_program_v2 activation, our inflation would be skewed. On the other hand, it won't be skewed at the even-number after the activation.

Summary of Changes

return current credits_observed value from calculate_points_and_credits()

todo

write a test
think about gating?

Fixes #

ryoqun · 2020-11-26T22:53:56Z

        // if there is no newer credits since observed, return no point
        if new_vote_state.credits() <= self.credits_observed {
-            return (0, 0);
+            if fix_stake_deactivate {


For testnet, we can freeride on this fix_stake_deactivate flag (backed by already activated stake_program_v2) only after inflation is started.
For mainnet-beta, we are still safe to edit this.

Strictly speaking, we don't need this fix_stake_deactivate flag at all. That's because we can prove we never use the second item in this returned tuple statically if fix_stake_deactivate = false; this becomes relevant only with fix_stake_deactivate = false. Any I'm adding this for consistency and just be in the safe side.

codecov · 2020-11-26T23:13:40Z

Codecov Report

Merging #13836 (b265662) into master (6048342) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #13836   +/-   ##
=======================================
  Coverage    82.0%    82.0%           
=======================================
  Files         381      381           
  Lines       93220    93232   +12     
=======================================
+ Hits        76530    76541   +11     
- Misses      16690    16691    +1

ryoqun · 2020-11-27T02:51:35Z

    ) -> (u128, u64) {
        // if there is no newer credits since observed, return no point
        if new_vote_state.credits() <= self.credits_observed {
-            return (0, 0);


Well, returning (_, 0) was never a good idea to begin with.... Semantically, credits_observed shouldn't be 0 in all cases. Although, it had not been used up to #13680, I really tempted to fix this at #13325. In retrospect, I really wish I just did it right at the time.......

lessons learnt: never violate invariants (= credits_observed can't be 0) (introduced at #10914, https://github.com/solana-labs/solana/pull/10914/files#r531390890) nevertheless at-the-time code was irrelevant of the violation.

it surely will bite future maintainers like this. (hah, past me bit current me...)

Oh dang... so obvious in retrospect! How'd I miss that 🤦‍♂️

ryoqun · 2020-11-27T03:19:55Z

Combined with #13837, I confirmed this pr fixes this stale stake state (pun intended).

$ cat tds-pre-rewrite9.csv | grep -E 'earned_|21yEAinmJfFWFx6QfYcPygvGqUj2vYtQZsrDf6dTNHAN' 
account,owner,old_balance,new_balance,data_size,delegation,delegation_owner,effective_stake,delegated_stake,rent_exempt_reserve,activation_epoch,deactivation_epoch,earned_epochs,epoch,epoch_credits,epoch_points,epoch_stake,old_credits_observed,new_credits_observed,base_rewards,stake_rewards,vote_rewards,commission,cluster_rewards,cluster_points
21yEAinmJfFWFx6QfYcPygvGqUj2vYtQZsrDf6dTNHAN,Stake11111111111111111111111111111111111111,25000000000,25000000000,200,A2Q7aAanJe9vETdYoYdS191YZGoMZ17CGJJuUEZmHX9,Vote111111111111111111111111111111111111111,24997717120,24997717120,2282880,95,N/A,0,N/A,N/A,N/A,N/A,4021838,0,0,0,0,100,N/A,N/A
==> 2020-11-27 12:17:31.166366184|0
$ cat tds-pre-rewrite10.csv | grep -E 'earned_|21yEAinmJfFWFx6QfYcPygvGqUj2vYtQZsrDf6dTNHAN' 
account,owner,old_balance,new_balance,data_size,delegation,delegation_owner,effective_stake,delegated_stake,rent_exempt_reserve,activation_epoch,deactivation_epoch,earned_epochs,epoch,epoch_credits,epoch_points,epoch_stake,old_credits_observed,new_credits_observed,base_rewards,stake_rewards,vote_rewards,commission,cluster_rewards,cluster_points
21yEAinmJfFWFx6QfYcPygvGqUj2vYtQZsrDf6dTNHAN,Stake11111111111111111111111111111111111111,25000000000,25000000000,200,A2Q7aAanJe9vETdYoYdS191YZGoMZ17CGJJuUEZmHX9,Vote111111111111111111111111111111111111111,24997717120,24997717120,2282880,95,N/A,0,N/A,N/A,N/A,N/A,4021838,4021838,0,0,0,100,N/A,N/A

ryoqun · 2020-11-27T03:20:59Z

sad, ci passed without chaining the test while I definitely changed implementation code....

The diff coverage is 100.0%.

vanity metrics here. ;)

ryoqun · 2020-11-27T06:11:13Z

well, I'm regarding that it's very rare for these critical bugs to slip through my eyes. So I went to solo postmortem on this incident.

In short, I think this was just a fluke. To my best, I had several protection to notice this but all of it failed at once to my surprise.

I guess the likelihood is like once in a year... I don't have clear counter-measures against this kind of unnoticed bugs. Alternatively, I think I should be more careful. (too obvious)....

Anyway, concluding this as rather a fluke, I believe there should be no more other similar critical bugs due to my lack of general competence.

As a detail, I failed to notice this bug at these individual opportunities:

I didn't notice the introduction of original smelly code (return (0, 0)) at fix rewards points #10914 on July (well, I was way more focusing on the actual inflation overflow bug; couldn't pay attention enough details like this).
I didn't bother to fix the smelly code even if I was recognizing at Small code cleanup and typo fixes #13325 on Oct (I didn't so because I wanted to lean towards less-aggressive code changes at the time)
I didn't remember about this smelly code when reviewing stake: Don't pay out rewards for epochs where inflation was not enabled #13680 on Nov. (to be honest, I was more paying attention to the validity of the simpler solution than mine; also there is other active stake-related prs...)
I didn't tested stake: Don't pay out rewards for epochs where inflation was not enabled #13680 with testnet ledger, only with mainnet's (I thought testing mainnet-beta itself is just enough; also, I haven't enough time to test before actual testnet activation because of the nonce fire)

t-nelson · 2020-11-28T01:47:56Z

No need to beat yourself @ryoqun! I was all over that code for #13680 and #13712 and managed to miss it as well. The staking logic is major 🤕

ryoqun · 2020-11-28T12:48:05Z

No need to beat yourself @ryoqun! I was all over that code for #13680 and #13712 and managed to miss it as well. The staking logic is major face_with_head_bandage

thanks for saying so. well, I don't have that bitter feeling anymore. I just wanted to learn something from this oversight. ;)

t-nelson

LGTM! Just a comment typo nit.

Nice job catching this one!

t-nelson · 2020-11-29T21:23:06Z

            )
        );
+
+        // credis_observed isn't reset for stale vote accounts while no rewards


Suggested change

// credis_observed isn't reset for stale vote accounts while no rewards

// credits_observed isn't reset for stale vote accounts while no rewards

Here's a suggestion for this comment (which I found confusing):
credits_observed remains at previous level when vote_state credits are not advancing and inflation is disabled

admittedly, I had some difficulty to describe this.. Thank for helping writing. :)

CriesofCarrots

Lgtm too! One suggestion that I think makes that comment a little more clear.

CriesofCarrots · 2020-11-29T22:39:51Z

            )
        );
+
+        // credis_observed isn't reset for stale vote accounts while no rewards


Here's a suggestion for this comment (which I found confusing):
credits_observed remains at previous level when vote_state credits are not advancing and inflation is disabled

mvines · 2020-11-30T01:35:05Z

This needs a v1.4 label right? And just to confirm, we're safe to land this ungated?

ryoqun · 2020-11-30T09:26:18Z

This needs a v1.4 label right?

Yeah, I added one. Assuming stake program v2 isn't activated on mainnet-beta with v1.3, we can skip v1.3

And just to confirm, we're safe to land this ungated?

Yeah, that should be so. I'll check this now.

=> EDIT: Yeah, I don't need this; I double-checked this again with testnet/mainnet ledgers to justify the lack of gating.

Pull request has been modified.

* Don't reset credits_observed due to stale voters * Add tests * Fix comment (cherry picked from commit e81c2c8)

* Don't reset credits_observed due to stale voters * Add tests * Fix comment (cherry picked from commit e81c2c8) Co-authored-by: Ryo Onodera <ryoqun@gmail.com>

ryoqun marked this pull request as draft November 26, 2020 22:48

ryoqun marked this pull request as ready for review November 26, 2020 22:48

ryoqun marked this pull request as draft November 26, 2020 22:49

ryoqun requested review from CriesofCarrots and t-nelson November 26, 2020 22:50

ryoqun commented Nov 26, 2020

View reviewed changes

Comment thread programs/stake/src/stake_state.rs

ryoqun mentioned this pull request Nov 27, 2020

Enhance ledger-tool for multi-epoch rewards #13837

Merged

ryoqun commented Nov 27, 2020

View reviewed changes

Don't reset credits_observed due to stale voters

5d16eaf

ryoqun force-pushed the really-correct-credits-observed-rewriting branch from aa6891b to 5d16eaf Compare November 27, 2020 04:28

This was referenced Nov 27, 2020

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

Merged

fix rewards points #10914

Merged

Add tests

3b8a753

ryoqun marked this pull request as ready for review November 28, 2020 12:46

t-nelson reviewed Nov 29, 2020

View reviewed changes

CriesofCarrots previously approved these changes Nov 29, 2020

View reviewed changes

ryoqun added v1.3 and removed v1.3 labels Nov 30, 2020

Fix comment

b265662

ryoqun merged commit e81c2c8 into solana-labs:master Nov 30, 2020

mergify Bot pushed a commit that referenced this pull request Nov 30, 2020

Don't reset credits_observed due to stale voters (#13836)

8e26b8e

* Don't reset credits_observed due to stale voters * Add tests * Fix comment (cherry picked from commit e81c2c8)

mergify Bot mentioned this pull request Nov 30, 2020

Don't reset credits_observed due to stale voters (bp #13836) #13860

Merged

	let (points, credits_observed) = self.calculate_points_and_credits(
	vote_state,
	stake_history,
	inflation_point_calc_tracer,
	fix_stake_deactivate,
	);

	// Drive credits_observed forward unconditionally when rewards are disabled
	if point_value.rewards == 0 && fix_stake_deactivate {
	return Some((0, 0, credits_observed));
	}

	// credis_observed isn't reset for stale vote accounts while no rewards
	// credits_observed isn't reset for stale vote accounts while no rewards

Conversation

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

Uh oh!

Problem

Summary of Changes

Uh oh!

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

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

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

Uh oh!

Codecov Report

Uh oh!

ryoqun Nov 27, 2020

Choose a reason for hiding this comment

Uh oh!

ryoqun Nov 27, 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 Nov 28, 2020

Choose a reason for hiding this comment

Uh oh!

ryoqun commented Nov 27, 2020

Uh oh!

ryoqun commented Nov 27, 2020

Uh oh!

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

Uh oh!

Uh oh!

t-nelson commented Nov 28, 2020

Uh oh!

ryoqun commented Nov 28, 2020

Uh oh!

t-nelson left a comment

Choose a reason for hiding this comment

Uh oh!

t-nelson Nov 29, 2020

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Nov 29, 2020

Choose a reason for hiding this comment

Uh oh!

ryoqun Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots left a comment

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Nov 29, 2020

Choose a reason for hiding this comment

Uh oh!

mvines commented Nov 30, 2020

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ryoqun commented Nov 26, 2020 •

edited

Loading

ryoqun Nov 26, 2020 •

edited

Loading

ryoqun Nov 30, 2020 •

edited

Loading

codecov Bot commented Nov 26, 2020 •

edited

Loading

ryoqun Nov 27, 2020 •

edited

Loading

ryoqun commented Nov 27, 2020 •

edited

Loading

ryoqun commented Nov 30, 2020 •

edited

Loading