From c9d91a6b039b05750bde8d0667469543f4a838e3 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Fri, 2 Jul 2021 14:28:05 -0500 Subject: [PATCH] Fix transaction logs and inner ixs for leader nodes --- ledger/src/blockstore_processor.rs | 11 +--- programs/bpf/tests/programs.rs | 39 +++++------- rpc/src/transaction_status_service.rs | 22 +------ runtime/src/bank.rs | 85 +++++++++++++-------------- 4 files changed, 62 insertions(+), 95 deletions(-) diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index 5aeb98c3a8a232..05ddebd432b30d 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -1198,8 +1198,8 @@ pub struct TransactionStatusBatch { pub statuses: Vec, pub balances: TransactionBalancesSet, pub token_balances: TransactionTokenBalancesSet, - pub inner_instructions: Option>>, - pub transaction_logs: Option>, + pub inner_instructions: Vec>, + pub transaction_logs: Vec>, pub rent_debits: Vec, } @@ -1218,15 +1218,10 @@ impl TransactionStatusSender { balances: TransactionBalancesSet, token_balances: TransactionTokenBalancesSet, inner_instructions: Vec>, - transaction_logs: Vec, + transaction_logs: Vec>, rent_debits: Vec, ) { let slot = bank.slot(); - let (inner_instructions, transaction_logs) = if !self.enable_cpi_and_log_storage { - (None, None) - } else { - (Some(inner_instructions), Some(transaction_logs)) - }; if let Err(e) = self .sender .send(TransactionStatusMessage::Batch(TransactionStatusBatch { diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index bca2c69255e418..6e7909d077a9b0 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -292,26 +292,24 @@ fn process_transaction_and_record_inner( let signature = tx.signatures.get(0).unwrap().clone(); let txs = vec![tx]; let tx_batch = bank.prepare_batch(txs.iter()); - let (mut results, _, mut inner, _transaction_logs) = bank.load_execute_and_commit_transactions( - &tx_batch, - MAX_PROCESSING_AGE, - false, - true, - false, - &mut ExecuteTimings::default(), - ); - let inner_instructions = if inner.is_empty() { - Some(vec![vec![]]) - } else { - inner.swap_remove(0) - }; + let (mut results, _, mut inner_instructions, _transaction_logs) = bank + .load_execute_and_commit_transactions( + &tx_batch, + MAX_PROCESSING_AGE, + false, + true, + false, + &mut ExecuteTimings::default(), + ); let result = results .fee_collection_results .swap_remove(0) .and_then(|_| bank.get_signature_status(&signature).unwrap()); ( result, - inner_instructions.expect("cpi recording should be enabled"), + inner_instructions + .swap_remove(0) + .expect("cpi recording should be enabled"), ) } @@ -329,8 +327,8 @@ fn execute_transactions(bank: &Bank, txs: &[Transaction]) -> Vec Vec Vec { let slot = bank.slot(); - let inner_instructions_iter: Box< - dyn Iterator>, - > = if let Some(inner_instructions) = inner_instructions { - Box::new(inner_instructions.into_iter()) - } else { - Box::new(std::iter::repeat_with(|| None)) - }; - let transaction_logs_iter: Box> = - if let Some(transaction_logs) = transaction_logs { - Box::new(transaction_logs.into_iter()) - } else { - Box::new(std::iter::repeat_with(Vec::new)) - }; for ( transaction, (status, nonce_rollback), @@ -97,8 +82,8 @@ impl TransactionStatusService { balances.post_balances, token_balances.pre_token_balances, token_balances.post_token_balances, - inner_instructions_iter, - transaction_logs_iter, + inner_instructions.into_iter(), + transaction_logs.into_iter(), rent_debits.into_iter(), ) { if Bank::can_commit(&status) && !transaction.signatures.is_empty() { @@ -125,7 +110,6 @@ impl TransactionStatusService { .collect() }); - let log_messages = Some(log_messages); let pre_token_balances = Some(pre_token_balances); let post_token_balances = Some(post_token_balances); let rewards = Some( diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index dc7f5e1813eb28..7db90c7c6c3e5b 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2675,9 +2675,7 @@ impl Bank { ); let transaction_result = executed[0].0.clone().map(|_| ()); - let log_messages = log_messages - .get(0) - .map_or(vec![], |messages| messages.to_vec()); + let log_messages = log_messages.get(0).cloned().flatten().unwrap_or_default(); let post_transaction_accounts = loaded_txs .into_iter() .next() @@ -2992,17 +2990,22 @@ impl Bank { Ok(()) } + fn collect_log_messages( + log_collector: Option>, + ) -> Option { + log_collector.and_then(|log_collector| Rc::try_unwrap(log_collector).map(Into::into).ok()) + } + fn compile_recorded_instructions( - inner_instructions: &mut Vec>, instruction_recorders: Option>, message: &Message, - ) { - inner_instructions.push(instruction_recorders.map(|instruction_recorders| { + ) -> Option { + instruction_recorders.map(|instruction_recorders| { instruction_recorders .into_iter() .map(|r| r.compile_instructions(message)) .collect() - })); + }) } /// Get any cached executors needed by the transaction @@ -3069,7 +3072,7 @@ impl Bank { Vec, Vec, Vec>, - Vec, + Vec>, Vec, u64, u64, @@ -3118,7 +3121,8 @@ impl Bank { let mut signature_count: u64 = 0; let mut inner_instructions: Vec> = Vec::with_capacity(hashed_txs.len()); - let mut transaction_log_messages = Vec::with_capacity(hashed_txs.len()); + let mut transaction_log_messages: Vec>> = + Vec::with_capacity(hashed_txs.len()); let bpf_compute_budget = self .bpf_compute_budget .unwrap_or_else(BpfComputeBudget::new); @@ -3127,7 +3131,11 @@ impl Bank { .iter_mut() .zip(hashed_txs.as_transactions_iter()) .map(|(accs, tx)| match accs { - (Err(e), _nonce_rollback) => (Err(e.clone()), None), + (Err(e), _nonce_rollback) => { + inner_instructions.push(None); + transaction_log_messages.push(None); + (Err(e.clone()), None) + } (Ok(loaded_transaction), nonce_rollback) => { signature_count += u64::from(tx.message().header.num_required_signatures); let executors = self.get_executors(&tx.message, &loaded_transaction.loaders); @@ -3167,20 +3175,11 @@ impl Bank { &self.ancestors, ); - if enable_log_recording { - let log_messages: TransactionLogMessages = - Rc::try_unwrap(log_collector.unwrap_or_default()) - .unwrap_or_default() - .into(); - - transaction_log_messages.push(log_messages); - } - - Self::compile_recorded_instructions( - &mut inner_instructions, + transaction_log_messages.push(Self::collect_log_messages(log_collector)); + inner_instructions.push(Self::compile_recorded_instructions( instruction_recorders, &tx.message, - ); + )); if let Err(e) = Self::refcells_to_accounts( &mut loaded_transaction.accounts, @@ -3266,7 +3265,6 @@ impl Bank { } let is_vote = is_simple_vote_transaction(tx); - let store = match transaction_log_collector_config.filter { TransactionLogCollectorFilter::All => !is_vote || mentioned_address, TransactionLogCollectorFilter::AllWithVotes => true, @@ -3275,12 +3273,14 @@ impl Bank { }; if store { - transaction_log_collector.logs.push(TransactionLogInfo { - signature: tx.signatures[0], - result: r.clone(), - is_vote, - log_messages: transaction_log_messages.get(i).cloned().unwrap_or_default(), - }); + if let Some(log_messages) = transaction_log_messages.get(i).cloned().flatten() { + transaction_log_collector.logs.push(TransactionLogInfo { + signature: tx.signatures[0], + result: r.clone(), + is_vote, + log_messages, + }); + } } } @@ -4036,7 +4036,7 @@ impl Bank { TransactionResults, TransactionBalancesSet, Vec>, - Vec, + Vec>, ) { let pre_balances = if collect_balances { self.collect_balances(batch) @@ -10135,9 +10135,11 @@ pub(crate) mod tests { &mut ExecuteTimings::default(), ); - assert!(inner_instructions[0].iter().all(|ix| ix.is_empty())); - assert_eq!(transaction_logs.len(), 0); + assert!(inner_instructions.iter().all(Option::is_none)); + assert!(transaction_logs.iter().all(Option::is_none)); + assert_eq!(inner_instructions.len(), 3); + assert_eq!(transaction_logs.len(), 3); assert_eq!(transaction_balances_set.pre_balances.len(), 3); assert_eq!(transaction_balances_set.post_balances.len(), 3); @@ -12926,7 +12928,9 @@ pub(crate) mod tests { let success_sig = tx0.signatures[0]; let tx1 = system_transaction::transfer(&sender1, &recipient1, 110, blockhash); // Should produce insufficient funds log let failure_sig = tx1.signatures[0]; - let txs = vec![tx1, tx0]; + let mut invalid_tx = system_transaction::transfer(&sender1, &recipient1, 10, blockhash); + invalid_tx.message.header.num_required_signatures = 0; // this tx won't be processed because it has no signers + let txs = vec![invalid_tx, tx1, tx0]; let batch = bank.prepare_batch(txs.iter()); let log_results = bank @@ -12939,17 +12943,10 @@ pub(crate) mod tests { &mut ExecuteTimings::default(), ) .3; - assert_eq!(log_results.len(), 2); - assert!(log_results[0] - .clone() - .pop() - .unwrap() - .contains(&"failed".to_string())); - assert!(log_results[1] - .clone() - .pop() - .unwrap() - .contains(&"success".to_string())); + assert_eq!(log_results.len(), 3); + assert!(log_results[0].as_ref().is_none()); + assert!(log_results[1].as_ref().unwrap()[2].contains(&"failed".to_string())); + assert!(log_results[2].as_ref().unwrap()[1].contains(&"success".to_string())); let stored_logs = &bank.transaction_log_collector.read().unwrap().logs; let success_log_info = stored_logs