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.

Sign up for GitHub

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

Jump to bottom

fix fee resulted bank hash mismatch #35012

Closed
wants to merge 2 commits into from
Closed

fix fee resulted bank hash mismatch #35012

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d56fa3
fix fee resulted bank hash mismatch
tao-stones Jan 30, 2024
f6d7f19
rename
tao-stones Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6720,17 +6720,6 @@ impl Bank {
&self.runtime_config.compute_budget.unwrap_or_default(),
false, /* debugging_features */
));

// genesis_config loaded by accounts_db::open_genesis_config() from ledger
// has it's lamports_per_signature set to zero; bank sets its value correctly
// after the first block with a transaction in it. This is a hack to mimic
// the process.
let derived_fee_rate_governor =
FeeRateGovernor::new_derived(&genesis_config.fee_rate_governor, 0);
// new bank's fee_structure.lamports_per_signature should be inline with
// what's configured in GenesisConfig
self.fee_structure.lamports_per_signature =
derived_fee_rate_governor.lamports_per_signature;
}

pub fn set_inflation(&self, inflation: Inflation) {
Expand Down
6 changes: 5 additions & 1 deletion sdk/src/fee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,14 @@ impl FeeStructure {
pub fn calculate_fee(
&self,
message: &SanitizedMessage,
_unused: u64,
for_test_only: u64,
budget_limits: &FeeBudgetLimits,
include_loaded_account_data_size_in_fee: bool,
) -> u64 {
if for_test_only == 0 {
return 0;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the parameter's original function - to set entire fee to zero if it is zero

Copy link
Contributor

Choose a reason for hiding this comment

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

this looks extremely hacky. can we just revert and try again, only correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll revert the chain of PRs first. Then look for a better way to fix that weird parameter.

Are you planning to revert #34970 and #34865 or stick with this PR? I'm fine with either approach. If possible I'd like to release v1.18.1 today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#35016 reverts 3 PRs. imo, less better than this PR 😜

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer this PR too... the new arg name and early return improve readability.

self.calculate_fee_details(
message,
budget_limits,
Expand Down
Loading