fix fee resulted bank hash mismatch #35012
Conversation
|
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. |
| if for_test_only == 0 { | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
This was the parameter's original function - to set entire fee to zero if it is zero
There was a problem hiding this comment.
this looks extremely hacky. can we just revert and try again, only correctly?
There was a problem hiding this comment.
This was exactly what it did originally - read lamports_per_signature from blockhash_queue or nonce, if it is zero, the multiple 0 to calculated fee (for test); otherwise multiple 1 to calculated fee.
I was trying to get ride of this "for test only" parameter by initializing fee_structure from fee_rate_governor, which seems to not as reliable approach.
I'll revert the chain of PRs first. Then look for a better way to fix that weird parameter.
There was a problem hiding this comment.
If revert, also need to revert #34757, but I really want/need to keep its function. I am thinking either keep this PR (cause it's essentially what the reverted code does), or I can combine reverting #34865, #34757 and #34970, then an additional PR to do what #34757 on reverted codebase. @t-nelson are you have strong opinions on not to use this PR (even changing the var name to test_fee_rate, which is really what it is in current code base)?
There was a problem hiding this comment.
#35016 reverts 3 PRs. imo, less better than this PR 😜
There was a problem hiding this comment.
I prefer this PR too... the new arg name and early return improve readability.
|
confirmed bankhash matches with testnet ledger. Going to confirm it does the same with mainnet ledger. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #35012 +/- ##
=======================================
Coverage 81.6% 81.6%
=======================================
Files 830 830
Lines 224836 224835 -1
=======================================
+ Hits 183512 183533 +21
+ Misses 41324 41302 -22 |
|
close, in fav of #35018 |
Problem
New
banknot created from parent has itsfee_structure.lamports_per_signatureinitialized by genesis_config.fee_rate_governor, which should not be the case if bank is created from snapshot where fee_rate_governor is deserialized.Summary of Changes
For a backport-able fix:
Fixes #35008