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.

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

earlier fee payer validation #34666

Merged
merged 7 commits into from
Jan 12, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
76 changes: 69 additions & 7 deletions core/src/banking_stage/consumer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,19 @@ use {
BankStart, PohRecorderError, RecordTransactionsSummary, RecordTransactionsTimings,
TransactionRecorder,
},
solana_program_runtime::timings::ExecuteTimings,
solana_program_runtime::{
compute_budget_processor::process_compute_budget_instructions, timings::ExecuteTimings,
},
solana_runtime::{
accounts::validate_fee_payer,
bank::{Bank, LoadAndExecuteTransactionsOutput},
transaction_batch::TransactionBatch,
},
solana_sdk::{
clock::{Slot, FORWARD_TRANSACTIONS_TO_LEADER_AT_SLOT_OFFSET, MAX_PROCESSING_AGE},
feature_set, saturating_add_assign,
feature_set,
message::SanitizedMessage,
saturating_add_assign,
timing::timestamp,
transaction::{self, AddressLoader, SanitizedTransaction, TransactionError},
},
Expand Down Expand Up @@ -394,9 +399,29 @@ impl Consumer {
txs: &[SanitizedTransaction],
chunk_offset: usize,
) -> ProcessTransactionBatchOutput {
// No filtering before QoS - transactions should have been sanitized immediately prior to this call
let pre_results = std::iter::repeat(Ok(()));
self.process_and_record_transactions_with_pre_results(bank, txs, chunk_offset, pre_results)
let mut error_counters = TransactionErrorMetrics::default();
let pre_results = vec![Ok(()); txs.len()];
let check_results =
bank.check_transactions(txs, &pre_results, MAX_PROCESSING_AGE, &mut error_counters);
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd potentially submit more transactions to bank for checking, which read-locks status_cache. Might be useful to add elapse metrics for this call, perhaps in separate PR.


let check_results = check_results
.into_iter()
.zip(txs)
Copy link
Contributor

Choose a reason for hiding this comment

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

is zip(txs) needed?

.map(|((result, _nonce), _tx)| result);

let mut output = self.process_and_record_transactions_with_pre_results(
bank,
txs,
chunk_offset,
check_results,
);

// Accumulate error counters from the initial checks into final results
output
.execute_and_commit_transactions_output
.error_counters
.accumulate(&error_counters);
output
}

pub fn process_and_record_aged_transactions(
Expand Down Expand Up @@ -684,6 +709,39 @@ impl Consumer {
}
}

pub fn check_fee_payer_unlocked(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

put this into a separate fn because I actually will be doing same operation in the new scheduler (#34699)

Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of lifting in this function, the saving is remove bad transactions upfront, and warms up good transactions' payer accounts, the extra time spent here is still a concern, especially if a lot of transactions going through this check. Can we add another metrics to monitor it?

bank: &Bank,
message: &SanitizedMessage,
error_counters: &mut TransactionErrorMetrics,
) -> Result<(), TransactionError> {
let fee_payer = message.fee_payer();
let budget_limits =
process_compute_budget_instructions(message.program_instructions_iter())?.into();
let fee = bank.fee_structure.calculate_fee(
message,
bank.get_lamports_per_signature(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I noted early it should fetch lamports_per_signature for durable transactions from nonce account. Then @t-nelson just clarified that durable transaction's fee is calculated based on fee rate at execution. So this should be just fine.

&budget_limits,
bank.feature_set.is_active(
&feature_set::include_loaded_accounts_data_size_in_fee_calculation::id(),
),
);
let (mut fee_payer_account, _slot) = bank
.rc
.accounts
.accounts_db
.load_with_fixed_root(&bank.ancestors, fee_payer)
.ok_or(TransactionError::AccountNotFound)?;

validate_fee_payer(
fee_payer,
&mut fee_payer_account,
0,
error_counters,
bank.rent_collector(),
fee,
)
}

fn accumulate_execute_units_and_time(execute_timings: &ExecuteTimings) -> (u64, u64) {
execute_timings.details.per_program_timings.values().fold(
(0, 0),
Expand Down Expand Up @@ -2291,8 +2349,12 @@ mod tests {
let keypair_c = Keypair::new();
let keypair_d = Keypair::new();
for keypair in &[&keypair_a, &keypair_b, &keypair_c, &keypair_d] {
bank.transfer(5_000, &genesis_config_info.mint_keypair, &keypair.pubkey())
.unwrap();
bank.transfer(
100_000,
Copy link
Contributor

Choose a reason for hiding this comment

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

why change from 5_000 to 100_000?

&genesis_config_info.mint_keypair,
&keypair.pubkey(),
)
.unwrap();
}

let make_prioritized_transfer =
Expand Down
25 changes: 25 additions & 0 deletions core/src/banking_stage/unprocessed_transaction_storage.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use {
super::{
consumer::Consumer,
forward_packet_batches_by_accounts::ForwardPacketBatchesByAccounts,
immutable_deserialized_packet::ImmutableDeserializedPacket,
latest_unprocessed_votes::{
Expand All @@ -16,6 +17,7 @@ use {
},
itertools::Itertools,
min_max_heap::MinMaxHeap,
solana_accounts_db::transaction_error_metrics::TransactionErrorMetrics,
solana_measure::{measure, measure_us},
solana_runtime::bank::Bank,
solana_sdk::{
Expand Down Expand Up @@ -136,6 +138,7 @@ pub struct ConsumeScannerPayload<'a> {
pub sanitized_transactions: Vec<SanitizedTransaction>,
pub slot_metrics_tracker: &'a mut LeaderSlotMetricsTracker,
pub message_hash_to_transaction: &'a mut HashMap<Hash, DeserializedPacket>,
pub error_counters: TransactionErrorMetrics,
}

fn consume_scan_should_process_packet(
Expand Down Expand Up @@ -177,6 +180,27 @@ fn consume_scan_should_process_packet(
return ProcessingDecision::Never;
}

// Only check fee-payer if we can actually take locks
Copy link
Contributor Author

Choose a reason for hiding this comment

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

renewed hatred for all the edge-cases of MI 😭

// We do not immediately discard on check lock failures here,
// because the priority guard requires that we always take locks
// except in the cases of discarding transactions (i.e. `Never`).
if payload.account_locks.check_locks(message)
&& Consumer::check_fee_payer_unlocked(bank, message, &mut payload.error_counters)
.is_err()
{
payload
.message_hash_to_transaction
.remove(packet.message_hash());
return ProcessingDecision::Never;
}

// NOTE:
// This must be the last operation before adding the transaction to the
// sanitized_transactions vector. Otherwise, a transaction could
// be blocked by a transaction that did not take batch locks. This
// will lead to some transactions never being processed, and a
// mismatch in the priorty-queue and hash map sizes.
t-nelson marked this conversation as resolved.
Show resolved Hide resolved
//
// Always take locks during batch creation.
// This prevents lower-priority transactions from taking locks
// needed by higher-priority txs that were skipped by this check.
Expand Down Expand Up @@ -213,6 +237,7 @@ where
sanitized_transactions: Vec::with_capacity(UNPROCESSED_BUFFER_STEP_SIZE),
slot_metrics_tracker,
message_hash_to_transaction,
error_counters: TransactionErrorMetrics::default(),
};
MultiIteratorScanner::new(
packets,
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/accounts/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ fn accumulate_and_check_loaded_account_data_size(
}
}

fn validate_fee_payer(
pub fn validate_fee_payer(
payer_address: &Pubkey,
payer_account: &mut AccountSharedData,
payer_index: IndexOfAccount,
Expand Down