From 6afeaac7a5f9ff18aa7f9d597599e35b5133dfcc Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 6 Jul 2021 23:04:19 +0000 Subject: [PATCH] Fix transaction logs and inner ixs for leader nodes (backport #18395) (#18448) * Fix transaction logs and inner ixs for leader nodes (#18395) * Fix transaction logs and inner ixs for leader nodes * Fix cpi log storage flag (cherry picked from commit 5dd399dafa4a7f5a71e467adb70628c5ac157e7a) # Conflicts: # runtime/src/bank.rs * resolve conflict Co-authored-by: Justin Starry --- ledger/src/blockstore_processor.rs | 4 +- programs/bpf/tests/programs.rs | 39 +++++------- rpc/src/transaction_status_service.rs | 14 ++--- runtime/src/bank.rs | 85 +++++++++++++-------------- 4 files changed, 65 insertions(+), 77 deletions(-) diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index 8a6a2cdbd81e36..4ceef36fa23b36 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -1199,7 +1199,7 @@ pub struct TransactionStatusBatch { pub balances: TransactionBalancesSet, pub token_balances: TransactionTokenBalancesSet, pub inner_instructions: Option>>, - pub transaction_logs: Option>, + pub transaction_logs: Option>>, pub rent_debits: Vec, } @@ -1218,7 +1218,7 @@ impl TransactionStatusSender { balances: TransactionBalancesSet, token_balances: TransactionTokenBalancesSet, inner_instructions: Vec>, - transaction_logs: Vec, + transaction_logs: Vec>, rent_debits: Vec, ) { let slot = bank.slot(); diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 2751b366982311..6f35c58b318a16 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> = - if let Some(transaction_logs) = transaction_logs { - Box::new(transaction_logs.into_iter()) - } else { - Box::new(std::iter::repeat_with(Vec::new)) - }; + let transaction_logs_iter: Box< + dyn Iterator>, + > = if let Some(transaction_logs) = transaction_logs { + Box::new(transaction_logs.into_iter()) + } else { + Box::new(std::iter::repeat_with(|| None)) + }; for ( transaction, (status, nonce_rollback), @@ -125,7 +126,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 a56289083371de..7fa318185bb43c 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2670,9 +2670,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_accounts .into_iter() .next() @@ -2995,17 +2993,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 @@ -3072,7 +3075,7 @@ impl Bank { Vec, Vec, Vec>, - Vec, + Vec>, Vec, u64, u64, @@ -3121,7 +3124,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); @@ -3130,7 +3134,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); @@ -3173,20 +3181,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, @@ -3272,7 +3271,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, @@ -3281,12 +3279,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, + }); + } } } @@ -4045,7 +4045,7 @@ impl Bank { TransactionResults, TransactionBalancesSet, Vec>, - Vec, + Vec>, ) { let pre_balances = if collect_balances { self.collect_balances(batch) @@ -10143,9 +10143,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); @@ -12934,7 +12936,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 @@ -12947,17 +12951,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