Fix bank new_from_field for epoch reward status

[HaoranYi]

Problem

We have a bug when reconstruct the reward_status on the bank from deserialized field.

Bank new_from_field is not using deserialized epoch reward status from field to reconstruct the bank.

This should have been caught by our tests. But unfortunately, there is a typo in the test, which didn't catch the bug.

Summary of Changes

• fix bank new_from_field to reconstruct epoch reward status.
• fix bank serde test assert for epoch reward status

Fixes #

[brooksprumo]

Lgtm, thanks for fixing. I think we'll want to backport this to v1.18 too.

[mergify]

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (52d3c02) 81.6% compared to head (923c8d1) 81.6%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #34992   +/-   ##
=======================================
  Coverage    81.6%    81.6%           
=======================================
  Files         830      830           
  Lines      224897   224897           
=======================================
+ Hits       183563   183576   +13     
+ Misses      41334    41321   -13     

[t-nelson]

isn't this tens of mb of duplicative data that could be trivially recovered upon snapshot load?

[HaoranYi]

isn't this tens of mb of duplicative data that could be trivially recovered upon snapshot load?

Yes, if the snapshot is taken during a rewarding period. All rewards are serialized to the snapshot and deserialized into the bank upon loading.

[t-nelson]

yes but what this is change is introducing, is totally duplicative of data that's already stored in a snapshot. we can just recover it at snapshot load. it's less than a slot's worth of work

[HaoranYi]

By recover do you mean recompute them?
AFAIK, current reward compute code doesn't support back computing the epoch reward at the epoch boundary. Vote states, which were used to compute the stake reward before at the epoch boundary, have already changed.

[jstarry]

I think we can discuss in solana-foundation/solana-improvement-documents#116 instead, this PR is just for a bugfix.

[t-nelson]

I think we can discuss in solana-foundation/solana-improvement-documents#116 instead, this PR is just for a bugfix.

this pr should be reverted before we forget and ship it to some public cluster

[HaoranYi]

I don't understand why we can't ship this PR to public cluster.
If we don't enable the partition reward feature, the reward status will just be empty, same as what we have now.
If we enable the partition reward feature, without this bugfix the cluster may diverge.
In both cases, we want to have this bug fix.

[jstarry]

This should definitely not be reverted. It's a bugfix regardless of whether we ship snapshot changes or not. We only include epoch rewards status in the snapshot if the partitioned rewards feature is activated.