runtime: Implement rent-adjusted stake delegations for amended SIMD-0392 (formerly SIMD-0488)#11332
runtime: Implement rent-adjusted stake delegations for amended SIMD-0392 (formerly SIMD-0488)#11332joncinque wants to merge 9 commits intoanza-xyz:masterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11332 +/- ##
========================================
Coverage 83.1% 83.2%
========================================
Files 847 847
Lines 320546 321076 +530
========================================
+ Hits 266680 267261 +581
+ Misses 53866 53815 -51 🚀 New features to boost your workflow:
|
|
Although the SIMD isn't in yet, for the sake of moving things forward quickly, please take a look when you have a moment. The actual code changes are pretty small, so this should be an easy backport if we make that decision. |
|
|
||
| #[derive(Default)] | ||
| struct RewardsAccumulator { | ||
| /// Accounts receiving voting commissions at the epoch boundary |
There was a problem hiding this comment.
please avoid unrelated changes. this should be a separate pr
There was a problem hiding this comment.
The other comments "/// Number of stake rewards, used to figure out size of rewards vector" and "/// Accumulated reward lamports, only used for sanity checks" are very important, since it helps show what's actually important in all of these structs.
Adding comments to two out of three fields in a struct felt silly, so I added this one too. The other ones are important to this change though.
| stake.delegation.stake = new_delegation; | ||
| // Deactivate stake if needed | ||
| if new_delegation == 0 { | ||
| stake.delegation.deactivation_epoch = rewarded_epoch; |
There was a problem hiding this comment.
since phone guys don't know what encapsulation is, can we have a comment here about this deactivation being immediate, unlike an authority-requested deactivation which is cast to the next epoch boundary
| } | ||
| } | ||
|
|
||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
i think it'd be worthwhile to do a cleanup preliminary pr where we just pass the whole stake account down
There was a problem hiding this comment.
I don't think it would really help to be honest -- it's either we pass the whole account and rent sysvar, or we pass the current and minimum lamport amount. I found the lamport amounts to be much simpler
There was a problem hiding this comment.
ok but you literally added the "fuck you guys i'm lazy" attribute
There was a problem hiding this comment.
I was answering the suggestion -- if you really want to refactor this function, I can do it
runtime/src/inflation_rewards/mod.rs
Outdated
| }) | ||
| )) | ||
| } else { | ||
| // No rewards to pay out, but still might need to modify delegation |
There was a problem hiding this comment.
seems like it'd be more maintainable to combine these blocks. for the sake of this operation, we don't care whether the account has earned rewards
There was a problem hiding this comment.
I implemented your suggestion, but it makes the code much harder to understand (and thus, maintain) in my opinion. I've pushed it up, let me know what you think
|
Noting that this will be rebased on #9380 to use the feature gate from there |
| let new_delegation = std::cmp::min( | ||
| stake.delegation.stake.saturating_add(staker_rewards), | ||
| current_lamports | ||
| .saturating_add(staker_rewards) | ||
| .saturating_sub(minimum_lamports), | ||
| ); |
#### Problem As mentioned during this [SIMD-0392 amendment](solana-foundation/solana-improvement-documents#488 (comment)), block metadata should carry the information of when a stake account is deactivting. #### Summary of changes Add a new `DeactivatingStake` variant of `RewardType`, to be used in anza-xyz/agave#11332.
* reward-info: Add variant for deactivating stake #### Problem As mentioned during this [SIMD-0392 amendment](solana-foundation/solana-improvement-documents#488 (comment)), block metadata should carry the information of when a stake account is deactivting. #### Summary of changes Add a new `DeactivatingStake` variant of `RewardType`, to be used in anza-xyz/agave#11332. * Change wording
|
If this PR represents a change to the public RPC API:
Thank you for keeping the RPC clients in sync with the server API @joncinque. |
|
This now contains the new reward type of |
| } | ||
| } | ||
|
|
||
| pub fn new_with_pre_stake_account( |
There was a problem hiding this comment.
| pub fn new_with_pre_stake_account( | |
| #[cfg(feature = "dev-context-only-utils")] | |
| pub fn new_with_pre_stake_account( |
'cause it calls Pubkey::unique()
There was a problem hiding this comment.
This is already gated with dev-context-only-utils on line 97
| let vote_state = | ||
| vote_state::VoteStateV4::deserialize(vote_account.data(), voter_pubkey).unwrap(); | ||
| let credits_observed = vote_state.credits(); | ||
| let last_epoch = vote_state.epoch_credits.last().map(|c| c.0).unwrap_or(0); |
There was a problem hiding this comment.
is the _or(0) the right thing to do here? seems like current_epoch - 1 is more correct, but this doesn't not work? just unclear whether it will encourage us to write more misleading tests
There was a problem hiding this comment.
Sure why not, it can't hurt to pass in the epoch too. I was going with the simplest way to just create a deactivated stake account
| && (reward_type == RewardType::Staking | ||
| || reward_type == RewardType::DeactivatedStake)) |
There was a problem hiding this comment.
in retrospect, we should have added a helper on RewardType
There was a problem hiding this comment.
The thought did cross my mind 🙃
| )) | ||
| .map_err(|_| DistributionError::UnableToSetState)?; | ||
|
|
||
| let reward_type = if partitioned_stake_reward.stake.delegation.stake( |
There was a problem hiding this comment.
thanks for reminding me how worthlessly ambiguous the stake interface is 🫠
Problem
As described in SIMD-0488, we need a mechanism for adjusting stake delegations during epoch rewards payout.
Summary of Changes
Implement the new calculation logic. The commits can be read in order:
Note: leaving this in draft until the SIMD goes through, there's still one more open question about respecting the minimum SOL delegation amount