From dcf45d2317717fb69dcd23cf00136905f4e91af2 Mon Sep 17 00:00:00 2001 From: Rob Walker Date: Fri, 3 Jul 2020 16:59:00 -0700 Subject: [PATCH] fix rewards points --- programs/stake/src/stake_state.rs | 97 +++++++++++++++++++++-------- runtime/src/bank.rs | 100 +++++++++++++++++------------- runtime/src/stakes.rs | 98 +---------------------------- 3 files changed, 132 insertions(+), 163 deletions(-) diff --git a/programs/stake/src/stake_state.rs b/programs/stake/src/stake_state.rs index b1d6ae497e..38d9eb0d97 100644 --- a/programs/stake/src/stake_state.rs +++ b/programs/stake/src/stake_state.rs @@ -374,31 +374,36 @@ impl Stake { stake_history: Option<&StakeHistory>, ) -> Option<(u64, u64)> { self.calculate_rewards(point_value, vote_state, stake_history) - .map(|(voters_reward, stakers_reward, credits_observed)| { + .map(|(stakers_reward, voters_reward, credits_observed)| { self.credits_observed = credits_observed; self.delegation.stake += stakers_reward; - (voters_reward, stakers_reward) + (stakers_reward, voters_reward) }) } - /// for a given stake and vote_state, calculate what distributions and what updates should be made - /// returns a tuple in the case of a payout of: - /// * voter_rewards to be distributed - /// * staker_rewards to be distributed - /// * new value for credits_observed in the stake - // returns None if there's no payout or if any deserved payout is < 1 lamport - pub fn calculate_rewards( + pub fn calculate_points( &self, - point_value: f64, vote_state: &VoteState, stake_history: Option<&StakeHistory>, - ) -> Option<(u64, u64, u64)> { + ) -> u128 { + self.calculate_points_and_credits(vote_state, stake_history) + .0 + } + + /// 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( + &self, + vote_state: &VoteState, + stake_history: Option<&StakeHistory>, + ) -> (u128, u64) { if self.credits_observed >= vote_state.credits() { - return None; + return (0, 0); } let mut credits_observed = self.credits_observed; - let mut total_rewards = 0f64; + let mut points = 0u128; for (epoch, credits, prev_credits) in vote_state.epoch_credits() { // figure out how much this stake has seen that // for which the vote account has a record @@ -414,18 +419,41 @@ impl Stake { 0 }; - total_rewards += - (self.delegation.stake(*epoch, stake_history) * epoch_credits) as f64 * point_value; + points += self.delegation.stake(*epoch, stake_history) as u128 * epoch_credits as u128; // don't want to assume anything about order of the iterator... credits_observed = credits_observed.max(*credits); } - // don't bother trying to collect fractional lamports - if total_rewards < 1f64 { + (points, credits_observed) + } + + /// for a given stake and vote_state, calculate what distributions and what updates should be made + /// returns a tuple in the case of a payout of: + /// * staker_rewards to be distributed + /// * voter_rewards to be distributed + /// * new value for credits_observed in the stake + // returns None if there's no payout or if any deserved payout is < 1 lamport + pub fn calculate_rewards( + &self, + point_value: f64, + vote_state: &VoteState, + stake_history: Option<&StakeHistory>, + ) -> Option<(u64, u64, u64)> { + if self.credits_observed >= vote_state.credits() { return None; } - let (voter_rewards, staker_rewards, is_split) = vote_state.commission_split(total_rewards); + let (points, credits_observed) = + self.calculate_points_and_credits(vote_state, stake_history); + + let rewards = points as f64 * point_value; + + // don't bother trying to collect fractional lamports + if rewards < 1f64 { + return None; + } + + let (voter_rewards, staker_rewards, is_split) = vote_state.commission_split(rewards); if (voter_rewards < 1f64 || staker_rewards < 1f64) && is_split { // don't bother trying to collect fractional lamports @@ -433,8 +461,8 @@ impl Stake { } Some(( - voter_rewards as u64, staker_rewards as u64, + voter_rewards as u64, credits_observed, )) } @@ -836,6 +864,7 @@ impl<'a> StakeAccount for KeyedAccount<'a> { } // utility function, used by runtime +// returns a tuple of (stakers_reward,voters_reward) pub fn redeem_rewards( stake_account: &mut Account, vote_account: &mut Account, @@ -846,7 +875,7 @@ pub fn redeem_rewards( let vote_state: VoteState = StateMut::::state(vote_account)?.convert_to_current(); - if let Some((voters_reward, stakers_reward)) = + if let Some((stakers_reward, voters_reward)) = stake.redeem_rewards(point_value, &vote_state, stake_history) { stake_account.lamports += stakers_reward; @@ -863,6 +892,22 @@ pub fn redeem_rewards( } } +// utility function, used by runtime +pub fn calculate_points( + stake_account: &Account, + vote_account: &Account, + stake_history: Option<&StakeHistory>, +) -> Result { + if let StakeState::Stake(_meta, stake) = stake_account.state()? { + let vote_state: VoteState = + StateMut::::state(vote_account)?.convert_to_current(); + + Ok(stake.calculate_points(&vote_state, stake_history)) + } else { + Err(InstructionError::InvalidAccountData) + } +} + // utility function, used by runtime::Stakes, tests pub fn new_stake_history_entry<'a, I>( epoch: Epoch, @@ -2180,7 +2225,7 @@ mod tests { // this one should be able to collect exactly 2 assert_eq!( - Some((0, stake_lamports * 2)), + Some((stake_lamports * 2, 0)), stake.redeem_rewards(1.0, &vote_state, None) ); @@ -2216,14 +2261,14 @@ mod tests { // this one should be able to collect exactly 2 assert_eq!( - Some((0, stake.delegation.stake * 2, 2)), + Some((stake.delegation.stake * 2, 0, 2)), stake.calculate_rewards(1.0, &vote_state, None) ); stake.credits_observed = 1; // this one should be able to collect exactly 1 (already observed one) assert_eq!( - Some((0, stake.delegation.stake, 2)), + Some((stake.delegation.stake, 0, 2)), stake.calculate_rewards(1.0, &vote_state, None) ); @@ -2233,7 +2278,7 @@ mod tests { stake.credits_observed = 2; // this one should be able to collect the one just added assert_eq!( - Some((0, stake.delegation.stake, 3)), + Some((stake.delegation.stake, 0, 3)), stake.calculate_rewards(1.0, &vote_state, None) ); @@ -2241,7 +2286,7 @@ mod tests { vote_state.increment_credits(2); // this one should be able to collect 2 now assert_eq!( - Some((0, stake.delegation.stake * 2, 4)), + Some((stake.delegation.stake * 2, 0, 4)), stake.calculate_rewards(1.0, &vote_state, None) ); @@ -2250,10 +2295,10 @@ mod tests { // (2 credits at stake of 1) + (1 credit at a stake of 2) assert_eq!( Some(( - 0, stake.delegation.stake * 2 // epoch 0 + stake.delegation.stake // epoch 1 + stake.delegation.stake, // epoch 2 + 0, 4 )), stake.calculate_rewards(1.0, &vote_state, None) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 0d726e7e77..b9950062aa 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -900,9 +900,8 @@ impl Bank { (*inflation).validator(year) * self.capitalization() as f64 * period }; - let validator_points = self.stakes.write().unwrap().claim_points(); - let validator_point_value = - self.check_point_value(validator_rewards / validator_points as f64); + let validator_point_value = self.pay_validator_rewards(validator_rewards as u64); + self.update_sysvar_account(&sysvar::rewards::id(), |account| { sysvar::rewards::create_account( self.inherit_sysvar_account_balance(account), @@ -910,65 +909,82 @@ impl Bank { ) }); - let validator_rewards = self.pay_validator_rewards(validator_point_value); - self.capitalization .fetch_add(validator_rewards as u64, Ordering::Relaxed); } /// iterate over all stakes, redeem vote credits for each stake we can /// successfully load and parse, return total payout - fn pay_validator_rewards(&mut self, point_value: f64) -> u64 { + fn pay_validator_rewards(&mut self, validator_rewards: u64) -> f64 { let stake_history = self.stakes.read().unwrap().history().clone(); - let mut validator_rewards = HashMap::new(); - let total_validator_rewards = self - .stake_delegations() + let stake_delegations = self.stake_delegations(); + + // first, count total points + let points: u128 = stake_delegations .iter() .map(|(stake_pubkey, delegation)| { match ( self.get_account(&stake_pubkey), self.get_account(&delegation.voter_pubkey), ) { - (Some(mut stake_account), Some(mut vote_account)) => { - let rewards = stake_state::redeem_rewards( - &mut stake_account, - &mut vote_account, - point_value, - Some(&stake_history), - ); - if let Ok((stakers_reward, voters_reward)) = rewards { - self.store_account(&stake_pubkey, &stake_account); - self.store_account(&delegation.voter_pubkey, &vote_account); - - if voters_reward > 0 { - *validator_rewards - .entry(delegation.voter_pubkey) - .or_insert(0i64) += voters_reward as i64; - } - - if stakers_reward > 0 { - *validator_rewards.entry(*stake_pubkey).or_insert(0i64) += - stakers_reward as i64; - } - - stakers_reward + voters_reward - } else { - debug!( - "stake_state::redeem_rewards() failed for {}: {:?}", - stake_pubkey, rewards - ); - 0 - } - } + (Some(stake_account), Some(vote_account)) => stake_state::calculate_points( + &stake_account, + &vote_account, + Some(&stake_history), + ) + .unwrap_or(0), (_, _) => 0, } }) .sum(); + let point_value = self.check_point_value(validator_rewards as f64 / points as f64); + + let mut rewards = HashMap::new(); + + // pay according to point value + stake_delegations + .iter() + .for_each(|(stake_pubkey, delegation)| { + match ( + self.get_account(&stake_pubkey), + self.get_account(&delegation.voter_pubkey), + ) { + (Some(mut stake_account), Some(mut vote_account)) => { + let redeemed = stake_state::redeem_rewards( + &mut stake_account, + &mut vote_account, + point_value, + Some(&stake_history), + ); + if let Ok((stakers_reward, voters_reward)) = redeemed { + self.store_account(&stake_pubkey, &stake_account); + self.store_account(&delegation.voter_pubkey, &vote_account); + + if voters_reward > 0 { + *rewards.entry(delegation.voter_pubkey).or_insert(0i64) += + voters_reward as i64; + } + + if stakers_reward > 0 { + *rewards.entry(*stake_pubkey).or_insert(0i64) += + stakers_reward as i64; + } + } else { + debug!( + "stake_state::redeem_rewards() failed for {}: {:?}", + stake_pubkey, redeemed + ); + } + } + (_, _) => (), + } + }); + assert_eq!(self.rewards, None); - self.rewards = Some(validator_rewards.drain().collect()); - total_validator_rewards + self.rewards = Some(rewards.drain().collect()); + point_value } fn update_recent_blockhashes_locked(&self, locked_blockhash_queue: &BlockhashQueue) { diff --git a/runtime/src/stakes.rs b/runtime/src/stakes.rs index 5553b9d816..66d3d31826 100644 --- a/runtime/src/stakes.rs +++ b/runtime/src/stakes.rs @@ -15,9 +15,8 @@ pub struct Stakes { /// stake_delegations stake_delegations: HashMap, - /// unclaimed points. - // a point is a credit multiplied by the stake - points: u64, + /// unused + unused: u64, /// current epoch, used to calculate current stake epoch: Epoch, @@ -49,7 +48,7 @@ impl Stakes { Stakes { stake_delegations: self.stake_delegations.clone(), - points: self.points, + unused: self.unused, epoch, vote_accounts: self .vote_accounts @@ -106,15 +105,6 @@ impl Stakes { |v| v.0, ); - // count any increase in points, can only go forward - let old_credits = old - .and_then(|(_stake, old_account)| VoteState::credits_from(old_account)) - .unwrap_or(0); - - let credits = VoteState::credits_from(account).unwrap_or(old_credits); - - self.points += credits.saturating_sub(old_credits) * stake; - self.vote_accounts.insert(*pubkey, (stake, account.clone())); } } else if solana_stake_program::check_id(&account.owner) { @@ -176,18 +166,6 @@ impl Stakes { .and_then(|(_k, (_stake, account))| VoteState::from(account)) .map(|vote_state| vote_state.node_pubkey) } - - /// currently unclaimed points - pub fn points(&self) -> u64 { - self.points - } - - /// "claims" points, resets points to 0 - pub fn claim_points(&mut self) -> u64 { - let points = self.points; - self.points = 0; - points - } } #[cfg(test)] @@ -302,76 +280,6 @@ pub mod tests { assert_eq!(stakes.highest_staked_node(), Some(vote11_node_pubkey)) } - #[test] - fn test_stakes_points() { - let mut stakes = Stakes::default(); - stakes.epoch = 4; - - let stake = 42; - assert_eq!(stakes.points(), 0); - assert_eq!(stakes.claim_points(), 0); - assert_eq!(stakes.claim_points(), 0); - - let ((vote_pubkey, mut vote_account), (stake_pubkey, stake_account)) = - create_staked_node_accounts(stake); - - stakes.store(&vote_pubkey, &vote_account); - stakes.store(&stake_pubkey, &stake_account); - - assert_eq!(stakes.points(), 0); - assert_eq!(stakes.claim_points(), 0); - - let mut vote_state = Some(VoteState::from(&vote_account).unwrap()); - for i in 0..MAX_LOCKOUT_HISTORY + 42 { - if let Some(v) = vote_state.as_mut() { - v.process_slot_vote_unchecked(i as u64) - } - let versioned = VoteStateVersions::Current(Box::new(vote_state.take().unwrap())); - VoteState::to(&versioned, &mut vote_account).unwrap(); - match versioned { - VoteStateVersions::Current(v) => { - vote_state = Some(*v); - } - _ => panic!("Has to be of type Current"), - }; - stakes.store(&vote_pubkey, &vote_account); - assert_eq!( - stakes.points(), - vote_state.as_ref().unwrap().credits() * stake - ); - } - vote_account.lamports = 0; - stakes.store(&vote_pubkey, &vote_account); - assert_eq!( - stakes.points(), - vote_state.as_ref().unwrap().credits() * stake - ); - - assert_eq!( - stakes.claim_points(), - vote_state.as_ref().unwrap().credits() * stake - ); - assert_eq!(stakes.claim_points(), 0); - assert_eq!(stakes.claim_points(), 0); - - // points come out of nowhere, but don't care here ;) - vote_account.lamports = 1; - stakes.store(&vote_pubkey, &vote_account); - assert_eq!( - stakes.points(), - vote_state.as_ref().unwrap().credits() * stake - ); - - // test going backwards, should never go backwards - let old_vote_state = vote_state; - let vote_account = vote_state::create_account(&vote_pubkey, &Pubkey::new_rand(), 0, 1); - stakes.store(&vote_pubkey, &vote_account); - assert_eq!( - stakes.points(), - old_vote_state.as_ref().unwrap().credits() * stake - ); - } - #[test] fn test_stakes_vote_account_disappear_reappear() { let mut stakes = Stakes::default();