Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Don't reset credits_observed due to stale voters #13836

Merged
ryoqun merged 3 commits into from
Nov 30, 2020
Merged

Don't reset credits_observed due to stale voters #13836

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d16eaf
Don't reset credits_observed due to stale voters
ryoqun Nov 26, 2020
3b8a753
Add tests
ryoqun Nov 28, 2020
b265662
Fix comment
ryoqun Nov 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions programs/stake/src/stake_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ impl Stake {
/// for a given stake and vote_state, calculate how many
/// points were earned (credits * stake) and new value
/// for credits_observed were the points paid
pub fn calculate_points_and_credits(
fn calculate_points_and_credits(
Comment thread
ryoqun marked this conversation as resolved.
&self,
new_vote_state: &VoteState,
stake_history: Option<&StakeHistory>,
Expand All @@ -562,7 +562,11 @@ impl Stake {
) -> (u128, u64) {
// if there is no newer credits since observed, return no point
if new_vote_state.credits() <= self.credits_observed {
return (0, 0);
Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Nov 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.......

Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Nov 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Copy link
Copy Markdown
Contributor

@t-nelson t-nelson Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if fix_stake_deactivate {
Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@ryoqun ryoqun Nov 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return (0, self.credits_observed);
} else {
return (0, 0);
}
}

let mut points = 0;
Expand Down Expand Up @@ -3435,7 +3439,7 @@ mod tests {
);

// now one with inflation disabled. no one gets paid, but we still need
// to advance the stake state's observed_credits field to prevent back-
// to advance the stake state's credits_observed field to prevent back-
// paying rewards when inflation is turned on.
assert_eq!(
Some((0, 0, 4)),
Expand All @@ -3450,6 +3454,33 @@ mod tests {
true,
)
);

// credits_observed remains at previous level when vote_state credits are
// not advancing and inflation is disabled
stake.credits_observed = 4;
assert_eq!(
Some((0, 0, 4)),
stake.calculate_rewards(
&PointValue {
rewards: 0,
points: 4
},
&vote_state,
None,
&mut null_tracer(),
true,
)
);

// assert the previous behavior is preserved where fix_stake_deactivate=false
assert_eq!(
(0, 0),
stake.calculate_points_and_credits(&vote_state, None, &mut null_tracer(), false)
);
assert_eq!(
(0, 4),
stake.calculate_points_and_credits(&vote_state, None, &mut null_tracer(), true)
);
}

#[test]
Expand Down