Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persist EpochRewards sysvar #572

Merged

Conversation

CriesofCarrots
Copy link

@CriesofCarrots CriesofCarrots commented Apr 4, 2024

Problem

As per SIMD-0118, the reworked EpochRewards sysvar must persist between rewards periods. This will provide an easy way for on-chain programs to determine whether the rewards period is active (checking the sysvar active field). Meanwhile, not adjusting the sysvar's lamport balance on distribution avoids existing gymnastics around ensuring rent-exemptness and that the account is totally empty at the right time.

Summary of Changes

  • Persist EpochRewards sysvar between reward intervals
  • Stop adjusting EpochRewards lamports balance based on rewards

Towards #426

@CriesofCarrots CriesofCarrots force-pushed the simd-118-persist-sysvar branch from 23a07e1 to 11d3d02 Compare April 4, 2024 03:54
@CriesofCarrots CriesofCarrots force-pushed the simd-118-persist-sysvar branch from 11d3d02 to 07ff5f2 Compare April 4, 2024 04:26
@CriesofCarrots
Copy link
Author

CriesofCarrots commented Apr 4, 2024

CI coverage failures not related to this PR, so far
(edit) made it on retry

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (0168e0a) to head (417756d).
Report is 23 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master     #572     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         847      849      +2     
  Lines      229180   229191     +11     
=========================================
- Hits       187600   187569     -31     
- Misses      41580    41622     +42     

joncinque
joncinque previously approved these changes Apr 4, 2024
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks good to me overall! Just some small things on the tests

runtime/src/bank/partitioned_epoch_rewards/mod.rs Outdated Show resolved Hide resolved
Comment on lines +440 to +445
if slot == num_slots_in_epoch {
// cap should increase because of new epoch rewards
assert!(post_cap > pre_cap);
} else {
assert_eq!(post_cap, pre_cap);
}

Choose a reason for hiding this comment

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

I'm a bit confused by this check. Shouldn't the cap increase at both epoch rollovers and not just the first? ie also when slot == 2 * num_slots_in_epoch?

When I ran this locally, I saw that post_cap is only 1 lamport bigger when slot == num_slots_in_epoch, which seems a bit strange. Is it just the rent exemption for the epoch rewards sysvar account?

Copy link
Author

Choose a reason for hiding this comment

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

Shouldn't the cap increase at both epoch rollovers and not just the first?

No. At the first block of epoch 1, the EpochRewards sysvar gets created for the first time, which increases the capitalization the amount of the sysvar's rent-exempt reserve (see below).
At the first block of epoch 2, the capitalization doesn't change: the sysvar has persisted throughout epoch 1, so no new rent-exempt reserve, and distribution starts the following block, so no new rewards.

Is it just the rent exemption for the epoch rewards sysvar account?

Correct. Sysvar lamport balances default to 1 lamport when the GenesisConfig sets Rent::free() (which it does in most of the genesis_utils fns, like create_genesis_config_with_vote_accounts()... that's a whole other issue which we can discuss separately 😅 ).

runtime/src/bank/partitioned_epoch_rewards/mod.rs Outdated Show resolved Hide resolved
@CriesofCarrots CriesofCarrots merged commit 9253c46 into anza-xyz:master Apr 5, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants