Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Return FeeCalculator with TransactionResults to use in transaction_status_service#10967

Closed
CriesofCarrots wants to merge 2 commits intosolana-labs:masterfrom
CriesofCarrots:fix-tss
Closed

Return FeeCalculator with TransactionResults to use in transaction_status_service#10967
CriesofCarrots wants to merge 2 commits intosolana-labs:masterfrom
CriesofCarrots:fix-tss

Conversation

@CriesofCarrots
Copy link
Copy Markdown
Contributor

Problem

This .expect() is reachable by a mainnet-beta validator:

let fee_calculator = match hash_age_kind {
Some(HashAgeKind::DurableNonce(_, account)) => {
nonce_utils::fee_calculator_of(&account)
}
_ => bank.get_fee_calculator(&transaction.message().recent_blockhash),
}
.expect("FeeCalculator must exist");

Summary of Changes

Return FeeCalculators used in processing transactions with TransactionResults, ensuring that if the transaction assessed fees successfully, the FeeCalculator is available for the TransactionStatusService.

Caveat: So far I haven't been able to write a test that fails on that expect.
A recent_blockhash transaction doesn't hit this exception, even if the blockhash is relatively old, because the bank uses the stricter MAX_PROCESSING_AGE to pass/fail transactions, so the recent_blockhash can be at worst in the middle of the bank's blockhash_queue.

Nonce transactions seem more suspicious, but I didn't come up with a failure case. I tried to remove a nonce account immediately after using it, before pulling the fee calculator from that bank, and the removal was caught by this error:

return Err(NonceError::NotExpired.into());
I didn't think of any other legitimate way to make the nonce account fee calculator unavailable, short of data corruption; advancing the nonce account doesn't hit the exception, since fee_calculator_of actually doesn't care if it's returning the right fee calculator (I think this could lead to incorrect nonced-tx fees being cached in Blockstore).

I still think this pr makes this area more robust, and if nothing else it resolves the potential issue with nonce transactions caching incorrect fee data in TSS.

Fixes #10958

@CriesofCarrots
Copy link
Copy Markdown
Contributor Author

@mvines , thoughts?
@t-nelson , am I right about the nonced-tx fee calculator issue? (I will add a test)

@t-nelson
Copy link
Copy Markdown
Contributor

t-nelson commented Jul 9, 2020

@t-nelson , am I right about the nonced-tx fee calculator issue? (I will add a test)

Sounds right. fee_calculator_of() was written under the assumption that it be used in the context of a validated nonce transaction referencing the given account. If the bank the account is queried from isn't frozen, that assumption won't hold

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 9, 2020

Codecov Report

Merging #10967 into master will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #10967     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         318      318             
  Lines       72747    72741      -6     
=========================================
- Hits        59777    59770      -7     
- Misses      12970    12971      +1     

@CriesofCarrots
Copy link
Copy Markdown
Contributor Author

CriesofCarrots commented Jul 10, 2020

Nonce issue described is not actually an issue, as the fee calculator is pulled from the actual account in HashAgeKind::DurableNonce.

@stale
Copy link
Copy Markdown

stale Bot commented Jul 17, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jul 17, 2020
@CriesofCarrots CriesofCarrots deleted the fix-tss branch July 24, 2020 22:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

stale [bot only] Added to stale content; results in auto-close after a week.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v1.1: panicked at ‘FeeCalculator must exist’, core/src/transaction_status_service.rs:65:38

2 participants