Deserialize Collector Fee Details

[tao-stones]

Problem

bank needs to collect transaction signature fees separately from priority fee, so it can be rewarded differently (per SIMD-0096).

This PR only does deserialize the new field, so it can be backport to beta to allow beta to load snapshot generated from edge. It is part of end-to-end poc implementation of simd-0096.

Summary of Changes

• comment to deprecate collector_fees from deserialization
• define CollectorFeeDetails
• add code to support deserialize, of eof, new field

Fixes #

[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

Attention: Patch coverage is 87.50000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 81.7%. Comparing base (bf2e8ee) to head (c54e9a3).

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #35326   +/-   ##
=======================================
  Coverage    81.7%    81.7%           
=======================================
  Files         834      834           
  Lines      224232   224241    +9     
=======================================
+ Hits       183382   183406   +24     
+ Misses      40850    40835   -15     

[jeffwashington]

@tao-stones, can you please add a link to the simd?
Does this have a master pr already? If so, please link to that. I have seen these difficult to get right the first time if the final code isn't working yet. It is important to get the older version to be able to read snapshots from the newer version.

[tao-stones]

@tao-stones, can you please add a link to the simd? Does this have a master pr already? If so, please link to that. I have seen these difficult to get right the first time if the final code isn't working yet. It is important to get the older version to be able to read snapshots from the newer version.

Updated PR Description with links to simd and poc impl pr. This PR is slimmed to be backported to v1.18 to allow beta to read snapshot generated from edge. I haven't committed changes that could break snapshot, so thanks in advance for being thorough on reviewing.

One thing to point out: I commented to deprecate the original collector_fees: u64, as it is being replaced by new collector_fee_details: CollectorFeeDetails. I meant to ask what is the best practice to deprecate a existing field?

[jeffwashington]

what is the best practice to deprecate a existing field?

Is this serialized in the bank? It should probably at some point be written as default, but it cannot be removed in bincode, afaik. @brooksprumo

[jeffwashington]

does this have a known purpose? Will it be enough? Will it ever be used?

[tao-stones]

it is reserved for potential write-lock fee (SIMD-0110), which would be 100% burnt. It is not yet approved, but there is a chance it will, want to be greedy to have space allocated for it for now 😸

[jeffwashington]

When do we need this backport present in 1.18? Should we wait until things like reserved are stable and known?

[tao-stones]

what is the best practice to deprecate a existing field?

Is this serialized in the bank? It should probably at some point be written as default, but it cannot be removed in bincode, afaik. @brooksprumo

yea it is serialized in bank currently, perhaps repurpose it down the road.

[tao-stones]

When do we need this backport present in 1.18? Should we wait until things like reserved are stable and known?

if that reserved: u64 is OK to be merged into edge, then we probably don't need to wait for backporting.

[sakridge]

Whether this needs to be backported or not is a different issue than if we should merge into master. I think any review of backport worthy-ness should be on the backport PR.

[sakridge]

The jump team also brought up using accounts for these types of things would be cleaner and not require special logic to ingest the snapshot since the values will just be contained in a serialized account. I'm not sure we are completely aligned on that, but since this is adding something new in that area it might be a good idea to evaluate how feasible that is.

[tao-stones]

Stuff associated with account, such as write-lock fee, are good candidate to be stored with account, especially with new account layout. But that perhaps belongs to separate topic.

This PR is to replace bank.collector_fees, which holds fees current bank collected. We need something more granular than u64 to store collected fees, in order to burn/reward different fees differently (eg. 100% reward prio fee, 50% burn transaction fee, etc)

Historically, bank.collector_fees is (de)desrialized; Now think again, I wonder if it is necessary to serialize collected fees to snapshot after all? Current working bank accumulates/collects transaction fees, then to distribute them when bank is frozen. After that collector_fees has no obvious use to me. If that's the case, then it doesn't need to be serialized to snapshot. Perhaps I miss something there. @sakridge wdyt?

[willhickey]

This repository is no longer in use. Please re-open this pull request in the agave repo: https://github.com/anza-xyz/agave