From 65761cc68328030ea898986d3818feb0b12a2e1b Mon Sep 17 00:00:00 2001 From: Vladimir Miloserdov Date: Thu, 19 Dec 2024 04:14:18 +0000 Subject: [PATCH 1/3] runtime: validate transactions in parallel Transaction validation and cost computation have been decoupled from the verification process, streamlining optimizations. The new flow introduces `parallel_validate_transactions`, which validates transactions (without using the state) and calculates costs concurrently. The results are then passed to `verify_and_charge_transaction`, which now accepts a precomputed `TransactionCost` instead of recalculating it. --- chain/chain/src/runtime/mod.rs | 71 +- .../src/estimator_context.rs | 10 +- runtime/runtime/src/lib.rs | 55 +- runtime/runtime/src/verifier.rs | 624 ++++++++++-------- 4 files changed, 439 insertions(+), 321 deletions(-) diff --git a/chain/chain/src/runtime/mod.rs b/chain/chain/src/runtime/mod.rs index 2ae2fad0bcb..24f7b466a41 100644 --- a/chain/chain/src/runtime/mod.rs +++ b/chain/chain/src/runtime/mod.rs @@ -566,6 +566,17 @@ impl RuntimeAdapter for NightshadeRuntime { } } + let cost = match validate_transaction( + runtime_config, + gas_price, + transaction, + verify_signature, + current_protocol_version, + ) { + Ok(cost) => cost, + Err(e) => return Ok(Some(e)), + }; + if let Some(state_root) = state_root { let shard_uid = self.account_id_to_shard_uid(transaction.transaction.signer_id(), epoch_id)?; @@ -576,7 +587,7 @@ impl RuntimeAdapter for NightshadeRuntime { &mut state_update, gas_price, transaction, - verify_signature, + &cost, // here we do not know which block the transaction will be included // and therefore skip the check on the nonce upper bound. None, @@ -586,17 +597,8 @@ impl RuntimeAdapter for NightshadeRuntime { Err(e) => Ok(Some(e)), } } else { - // Doing basic validation without a state root - match validate_transaction( - runtime_config, - gas_price, - transaction, - verify_signature, - current_protocol_version, - ) { - Ok(_) => Ok(None), - Err(e) => Ok(Some(e)), - } + // Without a state root, verification is skipped + Ok(None) } } @@ -773,27 +775,42 @@ impl RuntimeAdapter for NightshadeRuntime { continue; } - // Verifying the validity of the transaction based on the current state. - match verify_and_charge_transaction( + let res = validate_transaction( runtime_config, - &mut state_update, prev_block.next_gas_price, &tx, - false, - Some(next_block_height), + true, protocol_version, - ) { - Ok(verification_result) => { - tracing::trace!(target: "runtime", tx=?tx.get_hash(), "including transaction that passed validation"); - state_update.commit(StateChangeCause::NotWritableToDisk); - total_gas_burnt += verification_result.gas_burnt; - total_size += tx.get_size(); - result.transactions.push(tx); - // Take one transaction from this group, no more. - break; + ); + match res { + Ok(cost) => { + match verify_and_charge_transaction( + runtime_config, + &mut state_update, + prev_block.next_gas_price, + &tx, + &cost, + Some(next_block_height), + protocol_version, + ) { + Ok(verification_result) => { + tracing::trace!(target: "runtime", tx=?tx.get_hash(), "including transaction that passed validation"); + state_update.commit(StateChangeCause::NotWritableToDisk); + total_gas_burnt += verification_result.gas_burnt; + total_size += tx.get_size(); + result.transactions.push(tx); + // Take one transaction from this group, no more. + break; + } + Err(err) => { + tracing::trace!(target: "runtime", tx=?tx.get_hash(), ?err, "discarding transaction that failed verification"); + rejected_invalid_tx += 1; + state_update.rollback(); + } + } } Err(err) => { - tracing::trace!(target: "runtime", tx=?tx.get_hash(), ?err, "discarding transaction that is invalid"); + tracing::trace!(target: "runtime", tx=?tx.get_hash(), ?err, "discarding transaction that failed initial validation"); rejected_invalid_tx += 1; state_update.rollback(); } diff --git a/runtime/runtime-params-estimator/src/estimator_context.rs b/runtime/runtime-params-estimator/src/estimator_context.rs index ea68af97754..271ad276eb4 100644 --- a/runtime/runtime-params-estimator/src/estimator_context.rs +++ b/runtime/runtime-params-estimator/src/estimator_context.rs @@ -453,12 +453,20 @@ impl Testbed<'_> { let verify_signature = true; let clock = GasCost::measure(metric); + let cost = node_runtime::validate_transaction( + &self.apply_state.config, + gas_price, + tx, + verify_signature, + PROTOCOL_VERSION, + ) + .expect("expected no validation error"); node_runtime::verify_and_charge_transaction( &self.apply_state.config, &mut state_update, gas_price, tx, - verify_signature, + &cost, block_height, PROTOCOL_VERSION, ) diff --git a/runtime/runtime/src/lib.rs b/runtime/runtime/src/lib.rs index 0c80d15f739..fe00528deae 100644 --- a/runtime/runtime/src/lib.rs +++ b/runtime/runtime/src/lib.rs @@ -11,7 +11,7 @@ pub use crate::verifier::{ validate_transaction, verify_and_charge_transaction, ZERO_BALANCE_ACCOUNT_STORAGE_LIMIT, }; use bandwidth_scheduler::{run_bandwidth_scheduler, BandwidthSchedulerOutput}; -use config::total_prepaid_send_fees; +use config::{total_prepaid_send_fees, TransactionCost}; pub use congestion_control::bootstrap_congestion_info; use congestion_control::ReceiptSink; use itertools::Itertools; @@ -70,6 +70,7 @@ use near_vm_runner::ContractCode; use near_vm_runner::ContractRuntimeCache; use near_vm_runner::ProfileDataV3; use pipelining::ReceiptPreparationPipeline; +use rayon::prelude::*; use std::cmp::max; use std::collections::{HashMap, HashSet, VecDeque}; use std::sync::Arc; @@ -289,6 +290,45 @@ impl Runtime { debug!(target: "runtime", "{}", log_str); } + /// Parallel validation for all transactions with early short-circuit on first error. + /// + /// If all validations pass, returns a HashMap of tx_hash -> TransactionCost. + /// If any validation fails, returns the first InvalidTxError encountered. + fn parallel_validate_transactions( + config: &RuntimeConfig, + gas_price: Balance, + transactions: &[SignedTransaction], + current_protocol_version: ProtocolVersion, + ) -> Result, InvalidTxError> { + tracing::debug!(target: "runtime", "parallel validation: starting threads"); + + let results = transactions + .par_iter() + .try_fold( + || Vec::new(), + |mut acc, tx| { + let cost = validate_transaction( + config, + gas_price, + tx, + true, + current_protocol_version, + )?; + acc.push((tx.get_hash(), cost)); + Ok::<_, InvalidTxError>(acc) + }, + ) + .try_reduce( + || Vec::new(), + |mut acc1, mut acc2| { + acc1.append(&mut acc2); + Ok::<_, InvalidTxError>(acc1) + }, + )?; + + Ok(results.into_iter().collect()) + } + /// Takes one signed transaction, verifies it and converts it to a receipt. /// /// Add the produced receipt either to the new local receipts if the signer is the same @@ -312,6 +352,7 @@ impl Runtime { state_update: &mut TrieUpdate, apply_state: &ApplyState, signed_transaction: &SignedTransaction, + transaction_cost: &TransactionCost, stats: &mut ApplyStats, ) -> Result<(Receipt, ExecutionOutcomeWithId), InvalidTxError> { let span = tracing::Span::current(); @@ -322,7 +363,7 @@ impl Runtime { state_update, apply_state.gas_price, signed_transaction, - true, + transaction_cost, Some(apply_state.block_height), apply_state.current_protocol_version, ) { @@ -1589,11 +1630,21 @@ impl Runtime { let apply_state = &mut processing_state.apply_state; let state_update = &mut processing_state.state_update; + let tx_costs = Self::parallel_validate_transactions( + &apply_state.config, + apply_state.gas_price, + &processing_state.transactions.transactions, + apply_state.current_protocol_version, + ) + .map_err(RuntimeError::from)?; + for signed_transaction in processing_state.transactions.iter_nonexpired_transactions() { + let cost = tx_costs.get(&signed_transaction.get_hash()).expect("Must have cost for tx"); let tx_result = self.process_transaction( state_update, apply_state, signed_transaction, + cost, &mut processing_state.stats, ); let (receipt, outcome_with_id) = match tx_result { diff --git a/runtime/runtime/src/verifier.rs b/runtime/runtime/src/verifier.rs index 54e93f120c6..7fa5ee416ba 100644 --- a/runtime/runtime/src/verifier.rs +++ b/runtime/runtime/src/verifier.rs @@ -140,21 +140,16 @@ pub fn validate_transaction( pub fn verify_and_charge_transaction( config: &RuntimeConfig, state_update: &mut TrieUpdate, - gas_price: Balance, + _gas_price: Balance, signed_transaction: &SignedTransaction, - verify_signature: bool, + transaction_cost: &TransactionCost, block_height: Option, current_protocol_version: ProtocolVersion, ) -> Result { let _span = tracing::debug_span!(target: "runtime", "verify_and_charge_transaction").entered(); + let TransactionCost { gas_burnt, gas_remaining, receipt_gas_price, total_cost, burnt_amount } = - validate_transaction( - config, - gas_price, - signed_transaction, - verify_signature, - current_protocol_version, - )?; + *transaction_cost; let transaction = &signed_transaction.transaction; let signer_id = transaction.signer_id(); @@ -165,6 +160,7 @@ pub fn verify_and_charge_transaction( return Err(InvalidTxError::SignerDoesNotExist { signer_id: signer_id.clone() }); } }; + let mut access_key = match get_access_key(state_update, signer_id, transaction.public_key())? { Some(access_key) => access_key, None => { @@ -736,34 +732,35 @@ mod tests { signed_transaction: &SignedTransaction, expected_err: InvalidTxError, ) { - assert_eq!( - validate_transaction(config, gas_price, signed_transaction, true, PROTOCOL_VERSION) - .expect_err("expected an error"), - expected_err, - ); - assert_eq!( - verify_and_charge_transaction( - config, - state_update, - gas_price, - signed_transaction, - true, - None, - PROTOCOL_VERSION, - ) - .expect_err("expected an error"), - expected_err, - ); + match validate_transaction(config, gas_price, signed_transaction, true, PROTOCOL_VERSION) { + Ok(cost) => { + // Validation passed, now verification should fail with expected_err + let err = verify_and_charge_transaction( + config, + state_update, + gas_price, + signed_transaction, + &cost, + None, + PROTOCOL_VERSION, + ) + .expect_err("expected an error"); + assert_eq!(err, expected_err); + } + Err(err) => { + // Validation itself returned an error + assert_eq!(err, expected_err); + } + } } mod zero_balance_account_tests { use crate::near_primitives::account::id::AccountId; use crate::near_primitives::account::{ - AccessKeyPermission, Account, FunctionCallPermission, + AccessKey, AccessKeyPermission, Account, FunctionCallPermission, }; use crate::verifier::tests::{setup_accounts, TESTING_INIT_BALANCE}; use crate::verifier::{is_zero_balance_account, ZERO_BALANCE_ACCOUNT_STORAGE_LIMIT}; - use near_primitives::account::AccessKey; use near_store::{get_account, TrieUpdate}; use testlib::runtime_utils::{alice_account, bob_account}; @@ -843,7 +840,7 @@ mod tests { permission: AccessKeyPermission::FunctionCall(FunctionCallPermission { allowance: Some(100), receiver_id: bob_account().into(), - method_names: method_names, + method_names, }), }], false, @@ -871,14 +868,15 @@ mod tests { deposit, CryptoHash::default(), ); - validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) .expect("valid transaction"); let verification_result = verify_and_charge_transaction( &config, &mut state_update, gas_price, &transaction, - true, + &cost, None, PROTOCOL_VERSION, ) @@ -892,7 +890,7 @@ mod tests { ); let account = get_account(&state_update, &alice_account()).unwrap().unwrap(); - // Balance is decreased by the (TX fees + transfer balance). + // Balance is decreased by (TX fees + transfer balance). assert_eq!( account.amount(), TESTING_INIT_BALANCE @@ -937,28 +935,37 @@ mod tests { let config = RuntimeConfig::test(); let (bad_signer, mut state_update, gas_price) = setup_common(TESTING_INIT_BALANCE, 0, None); + let transaction = SignedTransaction::send_money( + 1, + alice_account(), + bob_account(), + &*bad_signer, + 100, + CryptoHash::default(), + ); + + let maybe_cost = + validate_transaction(&config, gas_price, &transaction, false, PROTOCOL_VERSION); + // Validation might pass if it doesn't require the access key check at this stage + let cost = + maybe_cost.expect("expected validation to succeed (no access key check at this stage)"); + + let err = verify_and_charge_transaction( + &config, + &mut state_update, + gas_price, + &transaction, + &cost, + None, + PROTOCOL_VERSION, + ) + .expect_err("expected an error"); assert_eq!( - verify_and_charge_transaction( - &config, - &mut state_update, - gas_price, - &SignedTransaction::send_money( - 1, - alice_account(), - bob_account(), - &*bad_signer, - 100, - CryptoHash::default(), - ), - false, - None, - PROTOCOL_VERSION, - ) - .expect_err("expected an error"), + err, InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::AccessKeyNotFound { account_id: alice_account(), public_key: bad_signer.public_key().into(), - },), + }) ); } @@ -1002,26 +1009,28 @@ mod tests { let (signer, mut state_update, gas_price) = setup_common(TESTING_INIT_BALANCE, 0, Some(AccessKey::full_access())); - assert_eq!( - verify_and_charge_transaction( - &config, - &mut state_update, - gas_price, - &SignedTransaction::send_money( - 1, - bob_account(), - alice_account(), - &*signer, - 100, - CryptoHash::default(), - ), - true, - None, - PROTOCOL_VERSION, - ) - .expect_err("expected an error"), - InvalidTxError::SignerDoesNotExist { signer_id: bob_account() }, + let transaction = SignedTransaction::send_money( + 1, + bob_account(), + alice_account(), + &*signer, + 100, + CryptoHash::default(), ); + + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + .expect("expected no validation error"); + let err = verify_and_charge_transaction( + &config, + &mut state_update, + gas_price, + &transaction, + &cost, + None, + PROTOCOL_VERSION, + ) + .expect_err("expected an error"); + assert_eq!(err, InvalidTxError::SignerDoesNotExist { signer_id: bob_account() }); } #[test] @@ -1033,26 +1042,28 @@ mod tests { Some(AccessKey { nonce: 2, permission: AccessKeyPermission::FullAccess }), ); - assert_eq!( - verify_and_charge_transaction( - &config, - &mut state_update, - gas_price, - &SignedTransaction::send_money( - 1, - alice_account(), - bob_account(), - &*signer, - 100, - CryptoHash::default(), - ), - true, - None, - PROTOCOL_VERSION, - ) - .expect_err("expected an error"), - InvalidTxError::InvalidNonce { tx_nonce: 1, ak_nonce: 2 }, + let transaction = SignedTransaction::send_money( + 1, + alice_account(), + bob_account(), + &*signer, + 100, + CryptoHash::default(), ); + + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + .expect("expected no validation error"); + let err = verify_and_charge_transaction( + &config, + &mut state_update, + gas_price, + &transaction, + &cost, + None, + PROTOCOL_VERSION, + ) + .expect_err("expected an error"); + assert_eq!(err, InvalidTxError::InvalidNonce { tx_nonce: 1, ak_nonce: 2 }); } #[test] @@ -1106,19 +1117,23 @@ mod tests { let (signer, mut state_update, gas_price) = setup_common(TESTING_INIT_BALANCE, 0, Some(AccessKey::full_access())); + let transaction = SignedTransaction::send_money( + 1, + alice_account(), + bob_account(), + &*signer, + TESTING_INIT_BALANCE, + CryptoHash::default(), + ); + + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + .expect("expected cost from validation"); let err = verify_and_charge_transaction( &config, &mut state_update, gas_price, - &SignedTransaction::send_money( - 1, - alice_account(), - bob_account(), - &*signer, - TESTING_INIT_BALANCE, - CryptoHash::default(), - ), - true, + &transaction, + &cost, None, PROTOCOL_VERSION, ) @@ -1148,25 +1163,29 @@ mod tests { }), ); + let transaction = SignedTransaction::from_actions( + 1, + alice_account(), + bob_account(), + &*signer, + vec![Action::FunctionCall(Box::new(FunctionCallAction { + method_name: "hello".to_string(), + args: b"abc".to_vec(), + gas: 300, + deposit: 0, + }))], + CryptoHash::default(), + 0, + ); + + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + .expect("expected cost from validation"); let err = verify_and_charge_transaction( &config, &mut state_update, gas_price, - &SignedTransaction::from_actions( - 1, - alice_account(), - bob_account(), - &*signer, - vec![Action::FunctionCall(Box::new(FunctionCallAction { - method_name: "hello".to_string(), - args: b"abc".to_vec(), - gas: 300, - deposit: 0, - }))], - CryptoHash::default(), - 0, - ), - true, + &transaction, + &cost, None, PROTOCOL_VERSION, ) @@ -1187,9 +1206,6 @@ mod tests { } } - /// Setup: account has 1B yoctoN and is 180 bytes. Storage requirement is 1M per byte. - /// Test that such account can not send 950M yoctoN out as that will leave it under storage requirements. - /// If zero balance account is enabled, however, the transaction should succeed #[test] fn test_validate_transaction_invalid_low_balance() { let mut config = RuntimeConfig::free(); @@ -1200,23 +1216,27 @@ mod tests { let (signer, mut state_update, gas_price) = setup_common(initial_balance, 0, Some(AccessKey::full_access())); - let res = verify_and_charge_transaction( + let transaction = SignedTransaction::send_money( + 1, + alice_account(), + bob_account(), + &*signer, + transfer_amount, + CryptoHash::default(), + ); + + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + .expect("expected cost from validation"); + let verification_result = verify_and_charge_transaction( &config, &mut state_update, gas_price, - &SignedTransaction::send_money( - 1, - alice_account(), - bob_account(), - &*signer, - transfer_amount, - CryptoHash::default(), - ), - true, + &transaction, + &cost, None, PROTOCOL_VERSION, - ); - let verification_result = res.unwrap(); + ) + .unwrap(); assert_eq!(verification_result.gas_burnt, 0); assert_eq!(verification_result.gas_remaining, 0); assert_eq!(verification_result.burnt_amount, 0); @@ -1240,19 +1260,23 @@ mod tests { false, )]); + let transaction = SignedTransaction::send_money( + 1, + account_id.clone(), + bob_account(), + &*signer, + transfer_amount, + CryptoHash::default(), + ); + + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + .expect("expected cost from validation"); let res = verify_and_charge_transaction( &config, &mut state_update, gas_price, - &SignedTransaction::send_money( - 1, - account_id.clone(), - bob_account(), - &*signer, - transfer_amount, - CryptoHash::default(), - ), - true, + &transaction, + &cost, None, PROTOCOL_VERSION, ) @@ -1285,79 +1309,82 @@ mod tests { }), ); - assert_eq!( - verify_and_charge_transaction( - &config, - &mut state_update, - gas_price, - &SignedTransaction::from_actions( - 1, - alice_account(), - bob_account(), - &*signer, - vec![ - Action::FunctionCall(Box::new(FunctionCallAction { - method_name: "hello".to_string(), - args: b"abc".to_vec(), - gas: 100, - deposit: 0, - })), - Action::CreateAccount(CreateAccountAction {}) - ], - CryptoHash::default(), - 0 - ), - true, - None, - PROTOCOL_VERSION, - ) - .expect_err("expected an error"), - InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::RequiresFullAccess), + // Case 1 + let transaction = SignedTransaction::from_actions( + 1, + alice_account(), + bob_account(), + &*signer, + vec![ + Action::FunctionCall(Box::new(FunctionCallAction { + method_name: "hello".to_string(), + args: b"abc".to_vec(), + gas: 100, + deposit: 0, + })), + Action::CreateAccount(CreateAccountAction {}), + ], + CryptoHash::default(), + 0, ); + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + .expect("expected no validation error"); + verify_and_charge_transaction( + &config, + &mut state_update, + gas_price, + &transaction, + &cost, + None, + PROTOCOL_VERSION, + ) + .expect_err("expected an error"); - assert_eq!( - verify_and_charge_transaction( - &config, - &mut state_update, - gas_price, - &SignedTransaction::from_actions( - 1, - alice_account(), - bob_account(), - &*signer, - vec![], - CryptoHash::default(), - 0 - ), - true, - None, - PROTOCOL_VERSION, - ) - .expect_err("expected an error"), - InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::RequiresFullAccess), + // Case 2 + let transaction = SignedTransaction::from_actions( + 1, + alice_account(), + bob_account(), + &*signer, + vec![], + CryptoHash::default(), + 0, ); + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + .expect("expected no validation error"); + verify_and_charge_transaction( + &config, + &mut state_update, + gas_price, + &transaction, + &cost, + None, + PROTOCOL_VERSION, + ) + .expect_err("expected an error"); - assert_eq!( - verify_and_charge_transaction( - &config, - &mut state_update, - gas_price, - &SignedTransaction::from_actions( - 1, - alice_account(), - bob_account(), - &*signer, - vec![Action::CreateAccount(CreateAccountAction {})], - CryptoHash::default(), - 0 - ), - true, - None, - PROTOCOL_VERSION, - ) - .expect_err("expected an error"), - InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::RequiresFullAccess), + // Case 3 + let transaction = SignedTransaction::from_actions( + 1, + alice_account(), + bob_account(), + &*signer, + vec![Action::CreateAccount(CreateAccountAction {})], + CryptoHash::default(), + 0, ); + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + .expect("expected no validation error"); + verify_and_charge_transaction( + &config, + &mut state_update, + gas_price, + &transaction, + &cost, + None, + PROTOCOL_VERSION, + ) + .expect_err("expected an error"); } #[test] @@ -1376,30 +1403,35 @@ mod tests { }), ); + let transaction = SignedTransaction::from_actions( + 1, + alice_account(), + eve_dot_alice_account(), + &*signer, + vec![Action::FunctionCall(Box::new(FunctionCallAction { + method_name: "hello".to_string(), + args: b"abc".to_vec(), + gas: 100, + deposit: 0, + }))], + CryptoHash::default(), + 0, + ); + + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + .expect("expected no validation error"); + let err = verify_and_charge_transaction( + &config, + &mut state_update, + gas_price, + &transaction, + &cost, + None, + PROTOCOL_VERSION, + ) + .expect_err("expected an error"); assert_eq!( - verify_and_charge_transaction( - &config, - &mut state_update, - gas_price, - &SignedTransaction::from_actions( - 1, - alice_account(), - eve_dot_alice_account(), - &*signer, - vec![Action::FunctionCall(Box::new(FunctionCallAction { - method_name: "hello".to_string(), - args: b"abc".to_vec(), - gas: 100, - deposit: 0, - })),], - CryptoHash::default(), - 0 - ), - true, - None, - PROTOCOL_VERSION, - ) - .expect_err("expected an error"), + err, InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::ReceiverMismatch { tx_receiver: eve_dot_alice_account(), ak_receiver: bob_account().into() @@ -1423,30 +1455,35 @@ mod tests { }), ); + let transaction = SignedTransaction::from_actions( + 1, + alice_account(), + bob_account(), + &*signer, + vec![Action::FunctionCall(Box::new(FunctionCallAction { + method_name: "hello".to_string(), + args: b"abc".to_vec(), + gas: 100, + deposit: 0, + }))], + CryptoHash::default(), + 0, + ); + + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + .expect("expected no validation error"); + let err = verify_and_charge_transaction( + &config, + &mut state_update, + gas_price, + &transaction, + &cost, + None, + PROTOCOL_VERSION, + ) + .expect_err("expected an error"); assert_eq!( - verify_and_charge_transaction( - &config, - &mut state_update, - gas_price, - &SignedTransaction::from_actions( - 1, - alice_account(), - bob_account(), - &*signer, - vec![Action::FunctionCall(Box::new(FunctionCallAction { - method_name: "hello".to_string(), - args: b"abc".to_vec(), - gas: 100, - deposit: 0, - })),], - CryptoHash::default(), - 0 - ), - true, - None, - PROTOCOL_VERSION, - ) - .expect_err("expected an error"), + err, InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::MethodNameMismatch { method_name: "hello".to_string() }), @@ -1469,31 +1506,36 @@ mod tests { }), ); + let transaction = SignedTransaction::from_actions( + 1, + alice_account(), + bob_account(), + &*signer, + vec![Action::FunctionCall(Box::new(FunctionCallAction { + method_name: "hello".to_string(), + args: b"abc".to_vec(), + gas: 100, + deposit: 100, + }))], + CryptoHash::default(), + 0, + ); + + let cost = validate_transaction(&config, gas_price, &transaction, true, PROTOCOL_VERSION) + .expect("expected no validation error"); + let err = verify_and_charge_transaction( + &config, + &mut state_update, + gas_price, + &transaction, + &cost, + None, + PROTOCOL_VERSION, + ) + .expect_err("expected an error"); assert_eq!( - verify_and_charge_transaction( - &config, - &mut state_update, - gas_price, - &SignedTransaction::from_actions( - 1, - alice_account(), - bob_account(), - &*signer, - vec![Action::FunctionCall(Box::new(FunctionCallAction { - method_name: "hello".to_string(), - args: b"abc".to_vec(), - gas: 100, - deposit: 100, - })),], - CryptoHash::default(), - 0 - ), - true, - None, - PROTOCOL_VERSION, - ) - .expect_err("expected an error"), - InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::DepositWithFunctionCall,), + err, + InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::DepositWithFunctionCall,) ); } @@ -1515,34 +1557,34 @@ mod tests { let mut config = RuntimeConfig::test(); let max_transaction_size = transaction_size - 1; - let wasm_config = Arc::make_mut(&mut config.wasm_config); - wasm_config.limit_config.max_transaction_size = transaction_size - 1; + { + let wasm_config = Arc::make_mut(&mut config.wasm_config); + wasm_config.limit_config.max_transaction_size = transaction_size - 1; + } + let err = validate_transaction(&config, gas_price, &transaction, false, PROTOCOL_VERSION) + .expect_err("expected validation error - size exceeded"); assert_eq!( - verify_and_charge_transaction( - &config, - &mut state_update, - gas_price, - &transaction, - false, - None, - PROTOCOL_VERSION, - ) - .expect_err("expected an error"), + err, InvalidTxError::TransactionSizeExceeded { size: transaction_size, limit: max_transaction_size - }, + } ); - let wasm_config = Arc::make_mut(&mut config.wasm_config); - wasm_config.limit_config.max_transaction_size = transaction_size + 1; + { + let wasm_config = Arc::make_mut(&mut config.wasm_config); + wasm_config.limit_config.max_transaction_size = transaction_size + 1; + } + + let cost = validate_transaction(&config, gas_price, &transaction, false, PROTOCOL_VERSION) + .expect("expected no validation error"); verify_and_charge_transaction( &config, &mut state_update, gas_price, &transaction, - false, + &cost, None, PROTOCOL_VERSION, ) From af88f451da219336cad49de7bba5c2892484c7cc Mon Sep 17 00:00:00 2001 From: Vladimir Miloserdov Date: Tue, 14 Jan 2025 14:50:19 +0000 Subject: [PATCH 2/3] drop invalid tx-s without invalidating the chunk --- runtime/runtime/src/lib.rs | 113 +++++++++++++++++++++---------------- 1 file changed, 65 insertions(+), 48 deletions(-) diff --git a/runtime/runtime/src/lib.rs b/runtime/runtime/src/lib.rs index fe00528deae..6e065af21ac 100644 --- a/runtime/runtime/src/lib.rs +++ b/runtime/runtime/src/lib.rs @@ -290,43 +290,26 @@ impl Runtime { debug!(target: "runtime", "{}", log_str); } - /// Parallel validation for all transactions with early short-circuit on first error. + /// Validates all transactions in parallel and returns a map of transaction hashes + /// to their validation results. /// - /// If all validations pass, returns a HashMap of tx_hash -> TransactionCost. - /// If any validation fails, returns the first InvalidTxError encountered. + /// Returns a `HashMap` of `tx_hash -> Result`. fn parallel_validate_transactions( config: &RuntimeConfig, gas_price: Balance, transactions: &[SignedTransaction], current_protocol_version: ProtocolVersion, - ) -> Result, InvalidTxError> { + ) -> HashMap> { tracing::debug!(target: "runtime", "parallel validation: starting threads"); - let results = transactions + transactions .par_iter() - .try_fold( - || Vec::new(), - |mut acc, tx| { - let cost = validate_transaction( - config, - gas_price, - tx, - true, - current_protocol_version, - )?; - acc.push((tx.get_hash(), cost)); - Ok::<_, InvalidTxError>(acc) - }, - ) - .try_reduce( - || Vec::new(), - |mut acc1, mut acc2| { - acc1.append(&mut acc2); - Ok::<_, InvalidTxError>(acc1) - }, - )?; - - Ok(results.into_iter().collect()) + .map(|tx| { + let cost_result = + validate_transaction(config, gas_price, tx, true, current_protocol_version); + (tx.get_hash(), cost_result) + }) + .collect() } /// Takes one signed transaction, verifies it and converts it to a receipt. @@ -1614,6 +1597,32 @@ impl Runtime { state_update.commit(StateChangeCause::Migration); } + /// Helper function that checks `RelaxedChunkValidation`. If it is enabled, we log a debug + /// message and return `Ok(())` to skip the transaction. Otherwise, we return `Err(e.into())`. + fn handle_invalid_transaction>( + e: E, + tx_hash: &CryptoHash, + protocol_version: ProtocolVersion, + reason: &str, + ) -> Result<(), RuntimeError> { + if checked_feature!( + "protocol_feature_relaxed_chunk_validation", + RelaxedChunkValidation, + protocol_version + ) { + tracing::debug!( + target: "runtime", + "invalid transaction ignored ({}) => tx_hash: {}, error: {:?}", + reason, + tx_hash, + e + ); + Ok(()) + } else { + Err(e.into()) + } + } + /// Processes a collection of transactions. /// /// Fills the `processing_state` with local receipts generated during processing of the @@ -1630,16 +1639,32 @@ impl Runtime { let apply_state = &mut processing_state.apply_state; let state_update = &mut processing_state.state_update; - let tx_costs = Self::parallel_validate_transactions( + let tx_cost_results = Self::parallel_validate_transactions( &apply_state.config, apply_state.gas_price, &processing_state.transactions.transactions, apply_state.current_protocol_version, - ) - .map_err(RuntimeError::from)?; + ); for signed_transaction in processing_state.transactions.iter_nonexpired_transactions() { - let cost = tx_costs.get(&signed_transaction.get_hash()).expect("Must have cost for tx"); + let tx_hash = signed_transaction.get_hash(); + let maybe_cost = tx_cost_results + .get(&tx_hash) + .expect("Must have a validation result for each transaction hash"); + + let cost = match maybe_cost { + Ok(c) => c, + Err(e) => { + Self::handle_invalid_transaction( + e.clone(), + &tx_hash, + processing_state.protocol_version, + "parallel validation error", + )?; + continue; + } + }; + let tx_result = self.process_transaction( state_update, apply_state, @@ -1648,23 +1673,15 @@ impl Runtime { &mut processing_state.stats, ); let (receipt, outcome_with_id) = match tx_result { - Ok(v) => v, + Ok(outcome) => outcome, Err(e) => { - if checked_feature!( - "protocol_feature_relaxed_chunk_validation", - RelaxedChunkValidation, - processing_state.protocol_version - ) { - // NB: number of invalid transactions are noted in metrics. - tracing::debug!( - target: "runtime", - message="invalid transaction ignored", - tx_hash=%signed_transaction.get_hash() - ); - continue; - } else { - return Err(e.into()); - } + Self::handle_invalid_transaction( + e, + &tx_hash, + processing_state.protocol_version, + "process_transaction error", + )?; + continue; } }; if receipt.receiver_id() == signed_transaction.transaction.signer_id() { From 453eaf23789c4e8cda7d64548aa553f9b939c091 Mon Sep 17 00:00:00 2001 From: Vladimir Miloserdov Date: Tue, 14 Jan 2025 14:18:48 +0000 Subject: [PATCH 3/3] simplify parallel tx validation per PR feedback --- chain/chain/src/runtime/mod.rs | 51 ++++++++----------- .../src/estimator_context.rs | 1 - runtime/runtime/src/lib.rs | 39 ++++++-------- runtime/runtime/src/verifier.rs | 17 ------- 4 files changed, 38 insertions(+), 70 deletions(-) diff --git a/chain/chain/src/runtime/mod.rs b/chain/chain/src/runtime/mod.rs index 24f7b466a41..f0ac8ee428b 100644 --- a/chain/chain/src/runtime/mod.rs +++ b/chain/chain/src/runtime/mod.rs @@ -585,7 +585,6 @@ impl RuntimeAdapter for NightshadeRuntime { match verify_and_charge_transaction( runtime_config, &mut state_update, - gas_price, transaction, &cost, // here we do not know which block the transaction will be included @@ -775,42 +774,36 @@ impl RuntimeAdapter for NightshadeRuntime { continue; } - let res = validate_transaction( + let verify_result = validate_transaction( runtime_config, prev_block.next_gas_price, &tx, true, protocol_version, - ); - match res { + ) + .and_then(|cost| { + verify_and_charge_transaction( + runtime_config, + &mut state_update, + &tx, + &cost, + Some(next_block_height), + protocol_version, + ) + }); + + match verify_result { Ok(cost) => { - match verify_and_charge_transaction( - runtime_config, - &mut state_update, - prev_block.next_gas_price, - &tx, - &cost, - Some(next_block_height), - protocol_version, - ) { - Ok(verification_result) => { - tracing::trace!(target: "runtime", tx=?tx.get_hash(), "including transaction that passed validation"); - state_update.commit(StateChangeCause::NotWritableToDisk); - total_gas_burnt += verification_result.gas_burnt; - total_size += tx.get_size(); - result.transactions.push(tx); - // Take one transaction from this group, no more. - break; - } - Err(err) => { - tracing::trace!(target: "runtime", tx=?tx.get_hash(), ?err, "discarding transaction that failed verification"); - rejected_invalid_tx += 1; - state_update.rollback(); - } - } + tracing::trace!(target: "runtime", tx=?tx.get_hash(), "including transaction that passed validation and verification"); + state_update.commit(StateChangeCause::NotWritableToDisk); + total_gas_burnt += cost.gas_burnt; + total_size += tx.get_size(); + result.transactions.push(tx); + // Take one transaction from this group, no more. + break; } Err(err) => { - tracing::trace!(target: "runtime", tx=?tx.get_hash(), ?err, "discarding transaction that failed initial validation"); + tracing::trace!(target: "runtime", tx=?tx.get_hash(), ?err, "discarding transaction that failed verification or verification"); rejected_invalid_tx += 1; state_update.rollback(); } diff --git a/runtime/runtime-params-estimator/src/estimator_context.rs b/runtime/runtime-params-estimator/src/estimator_context.rs index 271ad276eb4..ec774f85a52 100644 --- a/runtime/runtime-params-estimator/src/estimator_context.rs +++ b/runtime/runtime-params-estimator/src/estimator_context.rs @@ -464,7 +464,6 @@ impl Testbed<'_> { node_runtime::verify_and_charge_transaction( &self.apply_state.config, &mut state_update, - gas_price, tx, &cost, block_height, diff --git a/runtime/runtime/src/lib.rs b/runtime/runtime/src/lib.rs index 6e065af21ac..f09bfb049b6 100644 --- a/runtime/runtime/src/lib.rs +++ b/runtime/runtime/src/lib.rs @@ -290,26 +290,26 @@ impl Runtime { debug!(target: "runtime", "{}", log_str); } - /// Validates all transactions in parallel and returns a map of transaction hashes - /// to their validation results. + /// Validates all transactions in parallel and returns an iterator of + /// transactions paired with their validation results. /// - /// Returns a `HashMap` of `tx_hash -> Result`. - fn parallel_validate_transactions( - config: &RuntimeConfig, + /// Returns an `Iterator` of `(&SignedTransaction, Result)` + fn parallel_validate_transactions<'a>( + config: &'a RuntimeConfig, gas_price: Balance, - transactions: &[SignedTransaction], + transactions: &'a [SignedTransaction], current_protocol_version: ProtocolVersion, - ) -> HashMap> { - tracing::debug!(target: "runtime", "parallel validation: starting threads"); - - transactions + ) -> impl Iterator)> + { + let results: Vec<_> = transactions .par_iter() - .map(|tx| { + .map(move |tx| { let cost_result = validate_transaction(config, gas_price, tx, true, current_protocol_version); - (tx.get_hash(), cost_result) + (tx, cost_result) }) - .collect() + .collect(); + results.into_iter() } /// Takes one signed transaction, verifies it and converts it to a receipt. @@ -344,7 +344,6 @@ impl Runtime { match verify_and_charge_transaction( &apply_state.config, state_update, - apply_state.gas_price, signed_transaction, transaction_cost, Some(apply_state.block_height), @@ -1639,19 +1638,13 @@ impl Runtime { let apply_state = &mut processing_state.apply_state; let state_update = &mut processing_state.state_update; - let tx_cost_results = Self::parallel_validate_transactions( + for (signed_transaction, maybe_cost) in Self::parallel_validate_transactions( &apply_state.config, apply_state.gas_price, &processing_state.transactions.transactions, apply_state.current_protocol_version, - ); - - for signed_transaction in processing_state.transactions.iter_nonexpired_transactions() { + ) { let tx_hash = signed_transaction.get_hash(); - let maybe_cost = tx_cost_results - .get(&tx_hash) - .expect("Must have a validation result for each transaction hash"); - let cost = match maybe_cost { Ok(c) => c, Err(e) => { @@ -1669,7 +1662,7 @@ impl Runtime { state_update, apply_state, signed_transaction, - cost, + &cost, &mut processing_state.stats, ); let (receipt, outcome_with_id) = match tx_result { diff --git a/runtime/runtime/src/verifier.rs b/runtime/runtime/src/verifier.rs index 7fa5ee416ba..f2453088a69 100644 --- a/runtime/runtime/src/verifier.rs +++ b/runtime/runtime/src/verifier.rs @@ -140,7 +140,6 @@ pub fn validate_transaction( pub fn verify_and_charge_transaction( config: &RuntimeConfig, state_update: &mut TrieUpdate, - _gas_price: Balance, signed_transaction: &SignedTransaction, transaction_cost: &TransactionCost, block_height: Option, @@ -738,7 +737,6 @@ mod tests { let err = verify_and_charge_transaction( config, state_update, - gas_price, signed_transaction, &cost, None, @@ -874,7 +872,6 @@ mod tests { let verification_result = verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -953,7 +950,6 @@ mod tests { let err = verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1023,7 +1019,6 @@ mod tests { let err = verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1056,7 +1051,6 @@ mod tests { let err = verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1131,7 +1125,6 @@ mod tests { let err = verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1183,7 +1176,6 @@ mod tests { let err = verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1230,7 +1222,6 @@ mod tests { let verification_result = verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1274,7 +1265,6 @@ mod tests { let res = verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1332,7 +1322,6 @@ mod tests { verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1355,7 +1344,6 @@ mod tests { verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1378,7 +1366,6 @@ mod tests { verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1423,7 +1410,6 @@ mod tests { let err = verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1475,7 +1461,6 @@ mod tests { let err = verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1526,7 +1511,6 @@ mod tests { let err = verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None, @@ -1582,7 +1566,6 @@ mod tests { verify_and_charge_transaction( &config, &mut state_update, - gas_price, &transaction, &cost, None,