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

Simd 118: always pass epoch reward status to snapshot as None #1253

Merged

Conversation

CriesofCarrots
Copy link

Problem

As of #1159, Bank::epoch_reward_status is always populated from recalculation during the rewards period; the snapshot data is never used. Therefore, there is no need to pass a populated epoch_reward_status to snapshot serialization.

Summary of Changes

Always pass epoch_reward_status to snapshot serialization as None. There are several other pieces of snapshot logic that can now be removed, but I wanted to take this slowly and carefully.

Side note: I was surprised that the frozen_abi hash needed to be updated on this change, since the Some variant is still available in the snapshot format. However, it was a useful opportunity to learn more about the frozen_abi process. Structs tagged with AbiExample are initialized with minimally populated values, not defaults. So a print statement for Bank::epoch_reward_status added to the test here shows EpochRewardStatus::Active(_) containing one reward (with default values), and this is what was being used in serialization to determine the frozen_abi hash. Since this PR always uses None in serialization, the frozen_abi hash changes.

On the plus side, I would expect the frozen_abi hash to not change again when the actual epoch_reward_status field is removed from the snapshot.

brooksprumo
brooksprumo previously approved these changes May 8, 2024
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

:shipit:

Looks good to me. Thanks for the info about why AbiExample's digest changed. That would've confused me.

I'm assuming subsequent PRs may remove the epoch rewards status field from the snapshot entirely?

@CriesofCarrots
Copy link
Author

I'm assuming subsequent PRs may remove the epoch rewards status field from the snapshot entirely?

Yes exactly, that's where I'm headed next! I have a question for you about that, but I'll go ask on discord.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.1%. Comparing base (2bc026d) to head (ba775c9).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #1253     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         893      893             
  Lines      236600   236594      -6     
=========================================
- Hits       194429   194424      -5     
+ Misses      42171    42170      -1     

@CriesofCarrots CriesofCarrots merged commit d375673 into anza-xyz:master May 9, 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