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

feat: support committing fee-only transactions #2425

Merged
merged 7 commits into from
Aug 22, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
26 changes: 25 additions & 1 deletion ledger/src/blockstore_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3803,13 +3803,14 @@ pub mod tests {

#[test]
fn test_update_transaction_statuses() {
// Make sure instruction errors still update the signature cache
let GenesisConfigInfo {
genesis_config,
mint_keypair,
..
} = create_genesis_config(11_000);
let (bank, _bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config);

// Make sure instruction errors still update the signature cache
let pubkey = solana_sdk::pubkey::new_rand();
bank.transfer(1_000, &mint_keypair, &pubkey).unwrap();
assert_eq!(bank.transaction_count(), 1);
Expand All @@ -3826,6 +3827,29 @@ pub mod tests {
Err(TransactionError::AlreadyProcessed)
);

// Make sure fees-only transactions still update the signature cache
apfitzge marked this conversation as resolved.
Show resolved Hide resolved
let missing_program_id = Pubkey::new_unique();
let tx = Transaction::new_signed_with_payer(
&[Instruction::new_with_bincode(
missing_program_id,
&10,
Vec::new(),
)],
Some(&mint_keypair.pubkey()),
&[&mint_keypair],
bank.last_blockhash(),
);
// First process attempt will fail but still update status cache
assert_eq!(
bank.process_transaction(&tx),
Err(TransactionError::ProgramAccountNotFound)
);
// Second attempt will be rejected since tx was already in status cache
assert_eq!(
bank.process_transaction(&tx),
Err(TransactionError::AlreadyProcessed)
);

// Make sure other errors don't update the signature cache
let tx = system_transaction::transfer(&mint_keypair, &pubkey, 1000, Hash::default());
let signature = tx.signatures[0];
Expand Down
32 changes: 19 additions & 13 deletions programs/sbf/tests/programs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ use {
transaction::{SanitizedTransaction, Transaction, TransactionError, VersionedTransaction},
},
solana_svm::{
transaction_commit_result::CommittedTransaction,
transaction_commit_result::{CommittedTransaction, TransactionCommitResult},
transaction_execution_result::InnerInstruction,
transaction_processor::ExecutionRecordingConfig,
},
Expand Down Expand Up @@ -91,6 +91,20 @@ fn process_transaction_and_record_inner(
Vec<Vec<InnerInstruction>>,
Vec<String>,
) {
let commit_result = load_execute_and_commit_transaction(bank, tx);
let CommittedTransaction {
inner_instructions,
log_messages,
status,
..
} = commit_result.unwrap();
let inner_instructions = inner_instructions.expect("cpi recording should be enabled");
let log_messages = log_messages.expect("log recording should be enabled");
(status, inner_instructions, log_messages)
}

#[cfg(feature = "sbf_rust")]
fn load_execute_and_commit_transaction(bank: &Bank, tx: Transaction) -> TransactionCommitResult {
let txs = vec![tx];
let tx_batch = bank.prepare_batch_for_tests(txs);
let mut commit_results = bank
Expand All @@ -107,15 +121,7 @@ fn process_transaction_and_record_inner(
None,
)
.0;
let CommittedTransaction {
inner_instructions,
log_messages,
status,
..
} = commit_results.swap_remove(0).unwrap();
let inner_instructions = inner_instructions.expect("cpi recording should be enabled");
let log_messages = log_messages.expect("log recording should be enabled");
(status, inner_instructions, log_messages)
commit_results.pop().unwrap()
}

#[cfg(feature = "sbf_rust")]
Expand Down Expand Up @@ -1879,10 +1885,10 @@ fn test_program_sbf_invoke_in_same_tx_as_deployment() {
bank.last_blockhash(),
);
if index == 0 {
let results = execute_transactions(&bank, vec![tx]);
let result = load_execute_and_commit_transaction(&bank, tx);
assert_eq!(
results[0].as_ref().unwrap_err(),
&TransactionError::ProgramAccountNotFound,
result.unwrap().status,
Err(TransactionError::ProgramAccountNotFound),
);
} else {
let (result, _, _) = process_transaction_and_record_inner(&bank, tx);
Expand Down
143 changes: 86 additions & 57 deletions runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,8 @@ use {
},
transaction_processing_callback::TransactionProcessingCallback,
transaction_processing_result::{
TransactionProcessingResult, TransactionProcessingResultExtensions,
ProcessedTransaction, TransactionProcessingResult,
TransactionProcessingResultExtensions,
},
transaction_processor::{
ExecutionRecordingConfig, TransactionBatchProcessor, TransactionLogMessages,
Expand Down Expand Up @@ -326,6 +327,7 @@ pub struct LoadAndExecuteTransactionsOutput {
pub processed_counts: ProcessedTransactionCounts,
}

#[derive(Debug, PartialEq)]
pub struct TransactionSimulationResult {
pub result: Result<()>,
apfitzge marked this conversation as resolved.
Show resolved Hide resolved
pub logs: TransactionLogMessages,
Expand Down Expand Up @@ -3091,14 +3093,13 @@ impl Bank {
assert_eq!(sanitized_txs.len(), processing_results.len());
for (tx, processing_result) in sanitized_txs.iter().zip(processing_results) {
if let Ok(processed_tx) = &processing_result {
let details = &processed_tx.execution_details;
// Add the message hash to the status cache to ensure that this message
// won't be processed again with a different signature.
status_cache.insert(
tx.message().recent_blockhash(),
tx.message_hash(),
self.slot(),
details.status.clone(),
processed_tx.status(),
);
// Add the transaction signature to the status cache so that transaction status
// can be queried by transaction signature over RPC. In the future, this should
Expand All @@ -3107,7 +3108,7 @@ impl Bank {
tx.message().recent_blockhash(),
tx.signature(),
self.slot(),
details.status.clone(),
processed_tx.status(),
);
}
}
Expand Down Expand Up @@ -3359,30 +3360,35 @@ impl Bank {
let processing_result = processing_results
.pop()
.unwrap_or(Err(TransactionError::InvalidProgramForExecution));
let flattened_result = processing_result.flattened_result();
let (post_simulation_accounts, logs, return_data, inner_instructions) =
let (post_simulation_accounts, result, logs, return_data, inner_instructions) =
match processing_result {
Ok(processed_tx) => {
let details = processed_tx.execution_details;
let post_simulation_accounts = processed_tx
.loaded_transaction
.accounts
.into_iter()
.take(number_of_accounts)
.collect::<Vec<_>>();
(
post_simulation_accounts,
details.log_messages,
details.return_data,
details.inner_instructions,
)
}
Err(_) => (vec![], None, None, None),
Ok(processed_tx) => match processed_tx {
ProcessedTransaction::Executed(executed_tx) => {
let details = executed_tx.execution_details;
let post_simulation_accounts = executed_tx
.loaded_transaction
.accounts
.into_iter()
.take(number_of_accounts)
.collect::<Vec<_>>();
(
post_simulation_accounts,
details.status,
details.log_messages,
details.return_data,
details.inner_instructions,
)
}
ProcessedTransaction::FeesOnly(fees_only_tx) => {
(vec![], Err(fees_only_tx.load_error), None, None, None)
}
},
Err(error) => (vec![], Err(error), None, None, None),
};
let logs = logs.unwrap_or_default();

TransactionSimulationResult {
result: flattened_result,
result,
logs,
post_simulation_accounts,
units_consumed,
Expand Down Expand Up @@ -3562,7 +3568,8 @@ impl Bank {
.filter_map(|(processing_result, transaction)| {
// Skip log collection for unprocessed transactions
let processed_tx = processing_result.processed_transaction()?;
let execution_details = &processed_tx.execution_details;
// Skip log collection for unexecuted transactions
let execution_details = processed_tx.execution_details()?;
Self::collect_transaction_logs(
&transaction_log_collector_config,
transaction,
Expand Down Expand Up @@ -3712,7 +3719,7 @@ impl Bank {
.iter()
.for_each(|processing_result| match processing_result {
Ok(processed_tx) => {
fees += processed_tx.loaded_transaction.fee_details.total_fee();
fees += processed_tx.fee_details().total_fee();
}
Err(_) => {}
});
Expand All @@ -3731,8 +3738,7 @@ impl Bank {
.iter()
.for_each(|processing_result| match processing_result {
Ok(processed_tx) => {
accumulated_fee_details
.accumulate(&processed_tx.loaded_transaction.fee_details);
accumulated_fee_details.accumulate(&processed_tx.fee_details());
}
Err(_) => {}
});
Expand Down Expand Up @@ -3805,7 +3811,9 @@ impl Bank {
let ((), update_executors_us) = measure_us!({
let mut cache = None;
for processing_result in &processing_results {
if let Some(executed_tx) = processing_result.processed_transaction() {
if let Some(ProcessedTransaction::Executed(executed_tx)) =
processing_result.processed_transaction()
{
let programs_modified_by_tx = &executed_tx.programs_modified_by_tx;
if executed_tx.was_successful() && !programs_modified_by_tx.is_empty() {
cache
Expand All @@ -3821,7 +3829,7 @@ impl Bank {
let accounts_data_len_delta = processing_results
.iter()
.filter_map(|processing_result| processing_result.processed_transaction())
.map(|processed_tx| &processed_tx.execution_details)
.filter_map(|processed_tx| processed_tx.execution_details())
.filter_map(|details| {
details
.status
Expand Down Expand Up @@ -3859,37 +3867,52 @@ impl Bank {
) -> Vec<TransactionCommitResult> {
processing_results
.into_iter()
.map(|processing_result| {
let processed_tx = processing_result?;
let execution_details = processed_tx.execution_details;
let LoadedTransaction {
rent_debits,
accounts: loaded_accounts,
loaded_accounts_data_size,
fee_details,
..
} = processed_tx.loaded_transaction;

// Rent is only collected for successfully executed transactions
let rent_debits = if execution_details.was_successful() {
rent_debits
} else {
RentDebits::default()
};
.map(|processing_result| match processing_result? {
ProcessedTransaction::Executed(executed_tx) => {
let execution_details = executed_tx.execution_details;
let LoadedTransaction {
rent_debits,
accounts: loaded_accounts,
loaded_accounts_data_size,
fee_details,
..
} = executed_tx.loaded_transaction;

// Rent is only collected for successfully executed transactions
let rent_debits = if execution_details.was_successful() {
rent_debits
} else {
RentDebits::default()
};

Ok(CommittedTransaction {
status: execution_details.status,
log_messages: execution_details.log_messages,
inner_instructions: execution_details.inner_instructions,
return_data: execution_details.return_data,
executed_units: execution_details.executed_units,
fee_details,
rent_debits,
Ok(CommittedTransaction {
status: execution_details.status,
log_messages: execution_details.log_messages,
inner_instructions: execution_details.inner_instructions,
return_data: execution_details.return_data,
executed_units: execution_details.executed_units,
fee_details,
rent_debits,
loaded_account_stats: TransactionLoadedAccountsStats {
loaded_accounts_count: loaded_accounts.len(),
loaded_accounts_data_size,
},
})
}
ProcessedTransaction::FeesOnly(fees_only_tx) => Ok(CommittedTransaction {
status: Err(fees_only_tx.load_error),
log_messages: None,
inner_instructions: None,
return_data: None,
executed_units: 0,
apfitzge marked this conversation as resolved.
Show resolved Hide resolved
rent_debits: RentDebits::default(),
fee_details: fees_only_tx.fee_details,
loaded_account_stats: TransactionLoadedAccountsStats {
loaded_accounts_count: loaded_accounts.len(),
loaded_accounts_data_size,
loaded_accounts_count: fees_only_tx.rollback_accounts.count(),
loaded_accounts_data_size: fees_only_tx.rollback_accounts.data_size()
as u32,
},
})
}),
})
.collect()
}
Expand All @@ -3898,6 +3921,7 @@ impl Bank {
let collected_rent = processing_results
.iter()
.filter_map(|processing_result| processing_result.processed_transaction())
.filter_map(|processed_tx| processed_tx.executed_transaction())
apfitzge marked this conversation as resolved.
Show resolved Hide resolved
.filter(|executed_tx| executed_tx.was_successful())
.map(|executed_tx| executed_tx.loaded_transaction.rent)
.sum();
Expand Down Expand Up @@ -5845,6 +5869,11 @@ impl Bank {
.processed_transaction()
.map(|processed_tx| (tx, processed_tx))
})
.filter_map(|(tx, processed_tx)| {
processed_tx
.executed_transaction()
.map(|executed_tx| (tx, executed_tx))
})
.filter(|(_, executed_tx)| executed_tx.was_successful())
.flat_map(|(tx, executed_tx)| {
let num_account_keys = tx.message().account_keys().len();
Expand Down
Loading