Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 22 additions & 14 deletions programs/stake/src/stake_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -407,37 +407,45 @@ impl Stake {
/// for credits_observed were the points paid
pub fn calculate_points_and_credits(
&self,
vote_state: &VoteState,
new_vote_state: &VoteState,
stake_history: Option<&StakeHistory>,
) -> (u128, u64) {
if self.credits_observed >= vote_state.credits() {
// if there is no newer credits since observed, return no point
if new_vote_state.credits() <= self.credits_observed {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added comment, and swapped the condition operands to match with the comment and read fluently (IMO).

return (0, 0);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

}

let mut credits_observed = self.credits_observed;
let mut points = 0u128;
for (epoch, credits, prev_credits) in vote_state.epoch_credits() {
let mut points = 0;
let mut new_credits_observed = self.credits_observed;
Comment on lines +418 to +419
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is subtle, but let's match with the rhythm here: we're using (point, credits) tuple across this fn

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

also, the intention of point (without new_) and new_credits_observed (with new_) is that point is isolated and independent for each inflation distribution. So, it's stateless. But credits_observed is continuously updated thing in stake accounts. So, it's state-full. So, only credits_observed can be said there is newer version and (current/older) version.


for (epoch, final_epoch_credits, initial_epoch_credits) in
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the naming of credits and prev_credits here was a bit unhelpful in this context, imo.

what's important here is that they're together forming an interval because we're calculating overlaps between it and self.credit_observed.

So, I prefixed both of them with prefixes. (they used symmetrically in the logic but the one has a prefix and the other not; that's another point of confution, imo). final_ and initial_ is chosen over last_/first_ and end_/start_:

  • these epoch_credit interval's bounds are rather more rigid, not arbitrary, like they generally must not overlap with each other and properly ordered, etc. For example, this is not like lockout_intervals. So let's use somewhat formal-sounding wording here.
  • it's less odd to see (final, initial) order than (end, start) because the final wording has more semantic significance than initial; usually end and start have equal balance. Otherwise, we have to change ABI to make first, last to happen...

new_vote_state.epoch_credits().iter().copied()
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

copied() is to remove *s prefixed credits and prev_credits.

{
let stake = u128::from(self.delegation.stake(epoch, stake_history));

// figure out how much this stake has seen that
// for which the vote account has a record
let epoch_credits = if self.credits_observed < *prev_credits {
let earned_credits = if self.credits_observed < initial_epoch_credits {
// the staker observed the entire epoch
credits - prev_credits
} else if self.credits_observed < *credits {
final_epoch_credits - initial_epoch_credits
} else if self.credits_observed < final_epoch_credits {
// the staker registered sometime during the epoch, partial credit
credits - credits_observed
final_epoch_credits - new_credits_observed
} else {
// the staker has already observed or been redeemed this epoch
// or was activated after this epoch
0
};

points += u128::from(self.delegation.stake(*epoch, stake_history))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

factor out u128::froms and stake calculation. let's make important definition (point calculation formula) shine by its own: https://github.com/solana-labs/solana/pull/13325/files#r515579738

* u128::from(epoch_credits);
let earned_credits = u128::from(earned_credits);

// don't want to assume anything about order of the iterator...
credits_observed = credits_observed.max(*credits);
new_credits_observed = new_credits_observed.max(final_epoch_credits);

// finally calculate points for this epoch
points += stake * earned_credits;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the main dish of this fn is put at the end, partly preparing for the next actual functional change pr.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, I know this is against https://github.com/solana-labs/solana/pull/13325/files#r515579041 (credits => points here) and the general rule of putting looping-state-related code (maintaining max of new_credits_observed while looping) at the end to avoid intermixed state inside loop body.

}
(points, credits_observed)

(points, new_credits_observed)
}

/// for a given stake and vote_state, calculate what distributions and what updates should be made
Expand Down
6 changes: 2 additions & 4 deletions programs/vote/src/vote_state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ pub const MAX_LOCKOUT_HISTORY: usize = 31;
pub const INITIAL_LOCKOUT: usize = 2;

// Maximum number of credits history to keep around
// smaller numbers makes
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  • incomplete comment
  • pub isn't needed (at least for now)

pub const MAX_EPOCH_CREDITS_HISTORY: usize = 64;
const MAX_EPOCH_CREDITS_HISTORY: usize = 64;

#[frozen_abi(digest = "Ch2vVEwos2EjAVqSHCyJjnN2MNX1yrpapZTGhMSCjWUH")]
#[derive(Serialize, Default, Deserialize, Debug, PartialEq, Eq, Clone, AbiExample)]
Expand Down Expand Up @@ -392,8 +391,7 @@ impl VoteState {
self.epoch_credits.last_mut().unwrap().0 = epoch;
}

// if stakers do not claim before the epoch goes away they lose the
// credits...
Comment on lines -395 to -396
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

claim-redeem is past thing.

// Remove too old epoch_credits
if self.epoch_credits.len() > MAX_EPOCH_CREDITS_HISTORY {
self.epoch_credits.remove(0);
}
Expand Down
8 changes: 4 additions & 4 deletions runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1230,7 +1230,7 @@ impl Bank {
let validator_rewards =
(validator_rate * capitalization as f64 * epoch_duration_in_years) as u64;

let vote_balance_and_staked = self.stakes.read().unwrap().vote_balance_and_staked();
let old_vote_balance_and_staked = self.stakes.read().unwrap().vote_balance_and_staked();

let validator_point_value = self.pay_validator_rewards(validator_rewards);

Expand All @@ -1247,8 +1247,8 @@ impl Bank {
});
}

let validator_rewards_paid =
self.stakes.read().unwrap().vote_balance_and_staked() - vote_balance_and_staked;
let new_vote_balance_and_staked = self.stakes.read().unwrap().vote_balance_and_staked();
let validator_rewards_paid = new_vote_balance_and_staked - old_vote_balance_and_staked;
assert_eq!(
validator_rewards_paid,
u64::try_from(
Expand Down Expand Up @@ -6006,7 +6006,7 @@ mod tests {
// The same reward should be distributed given same credits
let expected_capitalization = do_test_bank_update_rewards_determinism();
// Repeat somewhat large number of iterations to expose possible different behavior
// depending on the randamly-seeded HashMap ordering
// depending on the randomly-seeded HashMap ordering
for _ in 0..30 {
let actual_capitalization = do_test_bank_update_rewards_determinism();
assert_eq!(actual_capitalization, expected_capitalization);
Expand Down