Skip to content

Commit

Permalink
feat: support committing fee-only transactions (#2425)
Browse files Browse the repository at this point in the history 
* feat: support committing fee-only transactions

* update and add new tests

* fix sbf test

* feedback

* fix new test

* test cleanup
  • Loading branch information
@jstarry
jstarry authored Aug 22, 2024
1 parent 8ae52fb commit 530e9c3
Show file tree
Hide file tree
Showing 16 changed files with 559 additions and 194 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 25 additions & 1 deletion ledger/src/blockstore_processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3801,13 +3801,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 @@ -3824,6 +3825,29 @@ pub mod tests {
Err(TransactionError::AlreadyProcessed)
);

// Make sure fees-only transactions still update the signature cache
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
View file
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 @@ -92,6 +92,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 @@ -108,15 +122,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 @@ -1880,10 +1886,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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,8 @@ use {
},
transaction_processing_callback::TransactionProcessingCallback,
transaction_processing_result::{
TransactionProcessingResult, TransactionProcessingResultExtensions,
ProcessedTransaction, TransactionProcessingResult,
TransactionProcessingResultExtensions,
},
transaction_processor::{
ExecutionRecordingConfig, TransactionBatchProcessor, TransactionLogMessages,
Expand Down Expand Up @@ -328,6 +329,7 @@ pub struct LoadAndExecuteTransactionsOutput {
pub processed_counts: ProcessedTransactionCounts,
}

#[derive(Debug, PartialEq)]
pub struct TransactionSimulationResult {
pub result: Result<()>,
pub logs: TransactionLogMessages,
Expand Down Expand Up @@ -3098,14 +3100,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 @@ -3114,7 +3115,7 @@ impl Bank {
tx.message().recent_blockhash(),
tx.signature(),
self.slot(),
details.status.clone(),
processed_tx.status(),
);
}
}
Expand Down Expand Up @@ -3364,30 +3365,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 @@ -3567,7 +3573,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 @@ -3717,7 +3724,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 @@ -3736,8 +3743,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 @@ -3810,7 +3816,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 @@ -3826,7 +3834,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 @@ -3864,37 +3872,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,
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 @@ -3903,6 +3926,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())
.filter(|executed_tx| executed_tx.was_successful())
.map(|executed_tx| executed_tx.loaded_transaction.rent)
.sum();
Expand Down Expand Up @@ -5853,6 +5877,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

0 comments on commit 530e9c3

Please sign in to comment.