From dc0e1e94c6d66b4bd6978da9ffb82bb9ba8c4db5 Mon Sep 17 00:00:00 2001 From: Simonas Kazlauskas Date: Tue, 12 Nov 2024 15:12:00 +0200 Subject: [PATCH] state_witness: do not include new transactions with each state witness (#12270) First and foremost this PR introduces an integration test that attempts to push through a chunk that has a few invalid transactions. Previously this would fail and make the entire chunk invalid. A new protocol feature is introduced that adjusts the code in few strategic places to remove the requirement for all transactions within a chunk to be valid, instead delegating the responsibility of checking all aspects of transaction validity to the moment when transactions get converted to receipts. Any transactions that are found to be invalid are simply ignored. At this point all validators should agree equally as to which transactions should be converted and which ones should be discarded. --- chain/chain/Cargo.toml | 5 + chain/chain/src/chain.rs | 13 +- chain/chain/src/runtime/mod.rs | 9 +- .../stateless_validation/chunk_validation.rs | 75 +++-- chain/client/Cargo.toml | 6 + .../state_witness_producer.rs | 43 +-- core/primitives-core/Cargo.toml | 2 + core/primitives-core/src/version.rs | 8 + core/primitives/Cargo.toml | 4 + core/primitives/src/errors.rs | 3 +- integration-tests/Cargo.toml | 4 + integration-tests/src/test_loop/tests/mod.rs | 32 +- .../access_key_nonce_for_implicit_accounts.rs | 19 +- .../client/features/adversarial_behaviors.rs | 1 + .../client/features/stateless_validation.rs | 157 +--------- .../src/tests/client/invalid_txs.rs | 291 ++++++++++++++++++ integration-tests/src/tests/client/mod.rs | 1 + .../src/tests/standard_cases/mod.rs | 66 ++-- integration-tests/src/tests/test_errors.rs | 27 +- integration-tests/src/user/mod.rs | 49 ++- integration-tests/src/user/rpc_user.rs | 13 +- integration-tests/src/user/runtime_user.rs | 69 +++-- runtime/runtime/Cargo.toml | 5 + runtime/runtime/src/lib.rs | 55 +++- 24 files changed, 638 insertions(+), 319 deletions(-) create mode 100644 integration-tests/src/tests/client/invalid_txs.rs diff --git a/chain/chain/Cargo.toml b/chain/chain/Cargo.toml index 510b35d0af3..f460c02f7ab 100644 --- a/chain/chain/Cargo.toml +++ b/chain/chain/Cargo.toml @@ -81,6 +81,10 @@ no_cache = ["near-store/no_cache"] protocol_feature_reject_blocks_with_outdated_protocol_version = [ "near-primitives/protocol_feature_reject_blocks_with_outdated_protocol_version", ] +protocol_feature_relaxed_chunk_validation = [ + "near-primitives/protocol_feature_relaxed_chunk_validation", + "node-runtime/protocol_feature_relaxed_chunk_validation", +] nightly = [ "near-async/nightly", @@ -98,6 +102,7 @@ nightly = [ "nightly_protocol", "node-runtime/nightly", "protocol_feature_reject_blocks_with_outdated_protocol_version", + "protocol_feature_relaxed_chunk_validation", ] nightly_protocol = [ "near-async/nightly_protocol", diff --git a/chain/chain/src/chain.rs b/chain/chain/src/chain.rs index b59fcc90310..f8b462b7202 100644 --- a/chain/chain/src/chain.rs +++ b/chain/chain/src/chain.rs @@ -3198,6 +3198,17 @@ impl Chain { prev_block_header: &BlockHeader, chunk: &ShardChunk, ) -> Result<(), Error> { + let protocol_version = + self.epoch_manager.get_epoch_protocol_version(block.header().epoch_id())?; + + if checked_feature!( + "protocol_feature_relaxed_chunk_validation", + RelaxedChunkValidation, + protocol_version + ) { + return Ok(()); + } + if !validate_transactions_order(chunk.transactions()) { let merkle_paths = Block::compute_chunk_headers_root(block.chunks().iter_deprecated()).1; @@ -3214,8 +3225,6 @@ impl Chain { return Err(Error::InvalidChunkProofs(Box::new(chunk_proof))); } - let protocol_version = - self.epoch_manager.get_epoch_protocol_version(block.header().epoch_id())?; if checked_feature!("stable", AccessKeyNonceRange, protocol_version) { let transaction_validity_period = self.transaction_validity_period; for transaction in chunk.transactions() { diff --git a/chain/chain/src/runtime/mod.rs b/chain/chain/src/runtime/mod.rs index 2e375167af0..e829f5907be 100644 --- a/chain/chain/src/runtime/mod.rs +++ b/chain/chain/src/runtime/mod.rs @@ -1400,11 +1400,18 @@ fn chunk_tx_gas_limit( fn calculate_transactions_size_limit( protocol_version: ProtocolVersion, runtime_config: &RuntimeConfig, - last_chunk_transactions_size: usize, + mut last_chunk_transactions_size: usize, transactions_gas_limit: Gas, ) -> u64 { // Checking feature WitnessTransactionLimits if ProtocolFeature::StatelessValidation.enabled(protocol_version) { + if near_primitives::checked_feature!( + "protocol_feature_relaxed_chunk_validation", + RelaxedChunkValidation, + protocol_version + ) { + last_chunk_transactions_size = 0; + } // Sum of transactions in the previous and current chunks should not exceed the limit. // Witness keeps transactions from both previous and current chunk, so we have to limit the sum of both. runtime_config diff --git a/chain/chain/src/stateless_validation/chunk_validation.rs b/chain/chain/src/stateless_validation/chunk_validation.rs index 64d77fce36a..d269cc78729 100644 --- a/chain/chain/src/stateless_validation/chunk_validation.rs +++ b/chain/chain/src/stateless_validation/chunk_validation.rs @@ -18,6 +18,7 @@ use near_epoch_manager::EpochManagerAdapter; use near_pool::TransactionGroupIteratorWrapper; use near_primitives::apply::ApplyChunkReason; use near_primitives::block::Block; +use near_primitives::checked_feature; use near_primitives::hash::{hash, CryptoHash}; use near_primitives::merkle::merklize; use near_primitives::receipt::Receipt; @@ -245,46 +246,54 @@ pub fn pre_validate_chunk_state_witness( ))); } - // Verify that all proposed transactions are valid. - let new_transactions = &state_witness.new_transactions; - if !new_transactions.is_empty() { - let transactions_validation_storage_config = RuntimeStorageConfig { - state_root: state_witness.chunk_header.prev_state_root(), - use_flat_storage: true, - source: StorageDataSource::Recorded(PartialStorage { - nodes: state_witness.new_transactions_validation_state.clone(), - }), - state_patch: Default::default(), - }; + let epoch_id = last_chunk_block.header().epoch_id(); + let protocol_version = epoch_manager.get_epoch_protocol_version(&epoch_id)?; + if !checked_feature!( + "protocol_feature_relaxed_chunk_validation", + RelaxedChunkValidation, + protocol_version + ) { + // Verify that all proposed transactions are valid. + let new_transactions = &state_witness.new_transactions; + if !new_transactions.is_empty() { + let transactions_validation_storage_config = RuntimeStorageConfig { + state_root: state_witness.chunk_header.prev_state_root(), + use_flat_storage: true, + source: StorageDataSource::Recorded(PartialStorage { + nodes: state_witness.new_transactions_validation_state.clone(), + }), + state_patch: Default::default(), + }; - match validate_prepared_transactions( - chain, - runtime_adapter, - &state_witness.chunk_header, - transactions_validation_storage_config, - &new_transactions, - &state_witness.transactions, - ) { - Ok(result) => { - if result.transactions.len() != new_transactions.len() { + match validate_prepared_transactions( + chain, + runtime_adapter, + &state_witness.chunk_header, + transactions_validation_storage_config, + &new_transactions, + &state_witness.transactions, + ) { + Ok(result) => { + if result.transactions.len() != new_transactions.len() { + return Err(Error::InvalidChunkStateWitness(format!( + "New transactions validation failed. \ + {} transactions out of {} proposed transactions were valid.", + result.transactions.len(), + new_transactions.len(), + ))); + } + } + Err(error) => { return Err(Error::InvalidChunkStateWitness(format!( - "New transactions validation failed. {} transactions out of {} proposed transactions were valid.", - result.transactions.len(), - new_transactions.len(), + "New transactions validation failed: {}", + error, ))); } - } - Err(error) => { - return Err(Error::InvalidChunkStateWitness(format!( - "New transactions validation failed: {}", - error, - ))); - } - }; + }; + } } let main_transition_params = if last_chunk_block.header().is_genesis() { - let epoch_id = last_chunk_block.header().epoch_id(); let shard_layout = epoch_manager.get_shard_layout(&epoch_id)?; let congestion_info = last_chunk_block .block_congestion_info() diff --git a/chain/client/Cargo.toml b/chain/client/Cargo.toml index 0c098578637..7a35a758379 100644 --- a/chain/client/Cargo.toml +++ b/chain/client/Cargo.toml @@ -73,6 +73,11 @@ near-primitives = { workspace = true, features = ["clock", "solomon", "rand"] } near-actix-test-utils.workspace = true [features] +protocol_feature_relaxed_chunk_validation = [ + "near-chain/protocol_feature_relaxed_chunk_validation", + "near-primitives/protocol_feature_relaxed_chunk_validation", +] + # if enabled, we assert in most situations that are impossible unless some byzantine behavior is observed. byzantine_asserts = ["near-chain/byzantine_asserts"] shadow_chunk_validation = ["near-chain/shadow_chunk_validation"] @@ -120,6 +125,7 @@ nightly = [ "near-telemetry/nightly", "near-vm-runner/nightly", "nightly_protocol", + "protocol_feature_relaxed_chunk_validation", ] sandbox = [ "near-client-primitives/sandbox", diff --git a/chain/client/src/stateless_validation/state_witness_producer.rs b/chain/client/src/stateless_validation/state_witness_producer.rs index 51b55b2bf22..1f3e1a5a5bc 100644 --- a/chain/client/src/stateless_validation/state_witness_producer.rs +++ b/chain/client/src/stateless_validation/state_witness_producer.rs @@ -1,6 +1,6 @@ -use std::collections::HashMap; -use std::sync::Arc; - +use super::partial_witness::partial_witness_actor::DistributeStateWitnessRequest; +use crate::stateless_validation::chunk_validator::send_chunk_endorsement_to_block_producers; +use crate::Client; use near_async::messaging::{CanSend, IntoSender}; use near_chain::{BlockHeader, Chain, ChainStoreAccess}; use near_chain_primitives::Error; @@ -20,11 +20,8 @@ use near_primitives::stateless_validation::stored_chunk_state_transition_data::{ use near_primitives::types::{AccountId, EpochId, ShardId}; use near_primitives::validator_signer::ValidatorSigner; use near_primitives::version::ProtocolFeature; - -use crate::stateless_validation::chunk_validator::send_chunk_endorsement_to_block_producers; -use crate::Client; - -use super::partial_witness::partial_witness_actor::DistributeStateWitnessRequest; +use std::collections::HashMap; +use std::sync::Arc; /// Result of collecting state transition data from the database to generate a state witness. /// Keep this private to this file. @@ -126,6 +123,7 @@ impl Client { let chunk_header = chunk.cloned_header(); let epoch_id = self.epoch_manager.get_epoch_id_from_prev_block(chunk_header.prev_block_hash())?; + let protocol_version = self.epoch_manager.get_epoch_protocol_version(&epoch_id)?; let prev_chunk = self.chain.get_chunk(&prev_chunk_header.chunk_hash())?; let StateTransitionData { main_transition, @@ -135,17 +133,26 @@ impl Client { contract_updates, } = self.collect_state_transition_data(&chunk_header, prev_chunk_header)?; - let new_transactions = chunk.transactions().to_vec(); - let new_transactions_validation_state = if new_transactions.is_empty() { - PartialState::default() + let (new_transactions, new_transactions_validation_state) = if checked_feature!( + "protocol_feature_relaxed_chunk_validation", + RelaxedChunkValidation, + protocol_version + ) { + (Vec::new(), PartialState::default()) } else { - // With stateless validation chunk producer uses recording reads when validating transactions. - // The storage proof must be available here. - transactions_storage_proof.ok_or_else(|| { - let message = "Missing storage proof for transactions validation"; - log_assert_fail!("{message}"); - Error::Other(message.to_owned()) - })? + let new_transactions = chunk.transactions().to_vec(); + let new_transactions_validation_state = if new_transactions.is_empty() { + PartialState::default() + } else { + // With stateless validation chunk producer uses recording reads when validating + // transactions. The storage proof must be available here. + transactions_storage_proof.ok_or_else(|| { + let message = "Missing storage proof for transactions validation"; + log_assert_fail!("{message}"); + Error::Other(message.to_owned()) + })? + }; + (new_transactions, new_transactions_validation_state) }; let source_receipt_proofs = diff --git a/core/primitives-core/Cargo.toml b/core/primitives-core/Cargo.toml index e4efb9a0896..40fd9e1fefa 100644 --- a/core/primitives-core/Cargo.toml +++ b/core/primitives-core/Cargo.toml @@ -38,6 +38,7 @@ protocol_feature_fix_staking_threshold = [] protocol_feature_fix_contract_loading_cost = [] protocol_feature_reject_blocks_with_outdated_protocol_version = [] protocol_feature_nonrefundable_transfer_nep491 = [] +protocol_feature_relaxed_chunk_validation = [] nightly = [ "nightly_protocol", @@ -45,6 +46,7 @@ nightly = [ "protocol_feature_fix_staking_threshold", "protocol_feature_nonrefundable_transfer_nep491", "protocol_feature_reject_blocks_with_outdated_protocol_version", + "protocol_feature_relaxed_chunk_validation", ] nightly_protocol = [ diff --git a/core/primitives-core/src/version.rs b/core/primitives-core/src/version.rs index 97856e07b8c..3fb1f51ea77 100644 --- a/core/primitives-core/src/version.rs +++ b/core/primitives-core/src/version.rs @@ -183,6 +183,12 @@ pub enum ProtocolFeature { /// to sync the current epoch's state. This is not strictly a protocol feature, but is included /// here to coordinate among nodes CurrentEpochStateSync, + /// Relaxed validation of transactions included in a chunk. + /// + /// Chunks no longer become entirely invalid in case invalid transactions are included in the + /// chunk. Instead the transactions are discarded during their conversion to receipts. + #[cfg(feature = "protocol_feature_relaxed_chunk_validation")] + RelaxedChunkValidation, } impl ProtocolFeature { @@ -262,6 +268,8 @@ impl ProtocolFeature { ProtocolFeature::ShuffleShardAssignments => 143, ProtocolFeature::CurrentEpochStateSync => 144, ProtocolFeature::SimpleNightshadeV4 => 145, + #[cfg(feature = "protocol_feature_relaxed_chunk_validation")] + ProtocolFeature::RelaxedChunkValidation => 146, ProtocolFeature::BandwidthScheduler => 147, // Place features that are not yet in Nightly below this line. } diff --git a/core/primitives/Cargo.toml b/core/primitives/Cargo.toml index 975fd8e35ce..047638bb33a 100644 --- a/core/primitives/Cargo.toml +++ b/core/primitives/Cargo.toml @@ -67,6 +67,9 @@ protocol_feature_reject_blocks_with_outdated_protocol_version = [ protocol_feature_nonrefundable_transfer_nep491 = [ "near-primitives-core/protocol_feature_nonrefundable_transfer_nep491", ] +protocol_feature_relaxed_chunk_validation = [ + "near-primitives-core/protocol_feature_relaxed_chunk_validation", +] nightly = [ "near-fmt/nightly", @@ -78,6 +81,7 @@ nightly = [ "protocol_feature_fix_staking_threshold", "protocol_feature_nonrefundable_transfer_nep491", "protocol_feature_reject_blocks_with_outdated_protocol_version", + "protocol_feature_relaxed_chunk_validation", ] nightly_protocol = [ diff --git a/core/primitives/src/errors.rs b/core/primitives/src/errors.rs index 51c38db549f..2178db63775 100644 --- a/core/primitives/src/errors.rs +++ b/core/primitives/src/errors.rs @@ -56,8 +56,7 @@ impl From for TxExecutionError { pub enum RuntimeError { /// An unexpected integer overflow occurred. The likely issue is an invalid state or the transition. UnexpectedIntegerOverflow(String), - /// An error happened during TX verification and account charging. It's likely the chunk is invalid. - /// and should be challenged. + /// An error happened during TX verification and account charging. InvalidTxError(InvalidTxError), /// Unexpected error which is typically related to the node storage corruption. /// It's possible the input state is invalid or malicious. diff --git a/integration-tests/Cargo.toml b/integration-tests/Cargo.toml index 2cc5fd93c04..1da15cc4c8b 100644 --- a/integration-tests/Cargo.toml +++ b/integration-tests/Cargo.toml @@ -101,6 +101,9 @@ protocol_feature_reject_blocks_with_outdated_protocol_version = [ protocol_feature_nonrefundable_transfer_nep491 = [ "near-primitives/protocol_feature_nonrefundable_transfer_nep491", ] +protocol_feature_relaxed_chunk_validation = [ + "near-primitives-core/protocol_feature_relaxed_chunk_validation", +] nightly = [ "near-actix-test-utils/nightly", @@ -131,6 +134,7 @@ nightly = [ "protocol_feature_fix_contract_loading_cost", "protocol_feature_nonrefundable_transfer_nep491", "protocol_feature_reject_blocks_with_outdated_protocol_version", + "protocol_feature_relaxed_chunk_validation", "testlib/nightly", ] nightly_protocol = [ diff --git a/integration-tests/src/test_loop/tests/mod.rs b/integration-tests/src/test_loop/tests/mod.rs index 026776f9450..7be430d582e 100644 --- a/integration-tests/src/test_loop/tests/mod.rs +++ b/integration-tests/src/test_loop/tests/mod.rs @@ -1,19 +1,19 @@ -pub mod bandwidth_scheduler_protocol_upgrade; +mod bandwidth_scheduler_protocol_upgrade; mod chunk_validator_kickout; -pub mod congestion_control; -pub mod congestion_control_genesis_bootstrap; -pub mod contract_distribution_cross_shard; -pub mod contract_distribution_simple; +mod congestion_control; +mod congestion_control_genesis_bootstrap; +mod contract_distribution_cross_shard; +mod contract_distribution_simple; mod create_delete_account; -pub mod epoch_sync; -pub mod fix_min_stake_ratio; -pub mod in_memory_tries; -pub mod max_receipt_size; -pub mod multinode_stateless_validators; -pub mod multinode_test_loop_example; -pub mod protocol_upgrade; +mod epoch_sync; +mod fix_min_stake_ratio; +mod in_memory_tries; +mod max_receipt_size; +mod multinode_stateless_validators; +mod multinode_test_loop_example; +mod protocol_upgrade; mod resharding_v3; -pub mod simple_test_loop_example; -pub mod state_sync; -pub mod syncing; -pub mod view_requests_to_archival_node; +mod simple_test_loop_example; +mod state_sync; +mod syncing; +mod view_requests_to_archival_node; diff --git a/integration-tests/src/tests/client/features/access_key_nonce_for_implicit_accounts.rs b/integration-tests/src/tests/client/features/access_key_nonce_for_implicit_accounts.rs index 7bb3bb42e3d..051e732fbf5 100644 --- a/integration-tests/src/tests/client/features/access_key_nonce_for_implicit_accounts.rs +++ b/integration-tests/src/tests/client/features/access_key_nonce_for_implicit_accounts.rs @@ -2,7 +2,7 @@ use crate::tests::client::process_blocks::produce_blocks_from_height; use assert_matches::assert_matches; use near_async::messaging::CanSend; use near_chain::orphan::NUM_ORPHAN_ANCESTORS_CHECK; -use near_chain::{Error, Provenance}; +use near_chain::{ChainStoreAccess as _, Error, Provenance}; use near_chain_configs::{Genesis, NEAR_BASE}; use near_chunks::metrics::PARTIAL_ENCODED_CHUNK_FORWARD_CACHED_WITHOUT_HEADER; use near_client::test_utils::{create_chunk_with_transactions, TestEnv}; @@ -260,7 +260,22 @@ fn test_chunk_transaction_validity() { .persist_and_distribute_encoded_chunk(chunk, merkle_paths, receipts, validator_id) .unwrap(); let res = env.clients[0].process_block_test(block.into(), Provenance::NONE); - assert_matches!(res.unwrap_err(), Error::InvalidTransactions); + match res.as_deref() { + Ok([h]) => { + let Ok(block) = env.clients[0].chain.get_block(&h) else { + panic!("did not find block from result!"); + }; + for chunk_hdr in block.chunks().iter_raw() { + let hash = chunk_hdr.chunk_hash(); + let Ok(chunk) = env.clients[0].chain.mut_chain_store().get_chunk(&hash) else { + continue; + }; + assert_eq!(chunk.prev_outgoing_receipts().len(), 0); + } + } + Err(Error::InvalidTransactions) => (), + e => panic!("unexpected result {e:?}"), + } } #[test] diff --git a/integration-tests/src/tests/client/features/adversarial_behaviors.rs b/integration-tests/src/tests/client/features/adversarial_behaviors.rs index 490d5238196..104c8e77eea 100644 --- a/integration-tests/src/tests/client/features/adversarial_behaviors.rs +++ b/integration-tests/src/tests/client/features/adversarial_behaviors.rs @@ -379,6 +379,7 @@ fn test_banning_chunk_producer_when_seeing_invalid_chunk() { #[test] #[cfg(feature = "test_features")] +#[cfg_attr(feature = "protocol_feature_relaxed_chunk_validation", ignore)] fn test_banning_chunk_producer_when_seeing_invalid_tx_in_chunk() { init_test_logger(); let mut test = AdversarialBehaviorTestData::new(); diff --git a/integration-tests/src/tests/client/features/stateless_validation.rs b/integration-tests/src/tests/client/features/stateless_validation.rs index 7b67f79bcdd..9f46489ed91 100644 --- a/integration-tests/src/tests/client/features/stateless_validation.rs +++ b/integration-tests/src/tests/client/features/stateless_validation.rs @@ -2,37 +2,36 @@ use crate::tests::client::features::wallet_contract::{ create_rlp_execute_tx, view_balance, NearSigner, }; use assert_matches::assert_matches; -use near_client::{ProcessTxResponse, ProduceChunkResult}; +use near_chain::Provenance; +use near_chain_configs::{Genesis, GenesisConfig, GenesisRecords}; +use near_client::test_utils::TestEnv; +use near_client::ProcessTxResponse; +use near_crypto::{InMemorySigner, KeyType, SecretKey}; use near_epoch_manager::{EpochManager, EpochManagerAdapter}; +use near_o11y::testonly::init_integration_logger; use near_primitives::account::id::AccountIdRef; use near_primitives::account::{AccessKeyPermission, FunctionCallPermission}; use near_primitives::action::{Action, AddKeyAction, TransferAction}; -use near_primitives::stateless_validation::state_witness::ChunkStateWitness; -use near_primitives::version::ProtocolFeature; -use near_store::test_utils::create_test_store; -use rand::rngs::StdRng; -use rand::{Rng, SeedableRng}; -use std::collections::HashSet; - -use near_chain::{Chain, Provenance}; -use near_chain_configs::{Genesis, GenesisConfig, GenesisRecords}; -use near_client::test_utils::{create_chunk_with_transactions, TestEnv}; -use near_crypto::{InMemorySigner, KeyType, SecretKey}; -use near_o11y::testonly::init_integration_logger; use near_primitives::epoch_manager::AllEpochConfigTestOverrides; use near_primitives::num_rational::Rational32; use near_primitives::shard_layout::ShardLayout; use near_primitives::state_record::StateRecord; +use near_primitives::stateless_validation::state_witness::ChunkStateWitness; use near_primitives::test_utils::{create_test_signer, create_user_test_signer}; use near_primitives::transaction::SignedTransaction; use near_primitives::types::{AccountInfo, EpochId, ShardId}; use near_primitives::utils::derive_eth_implicit_account_id; +use near_primitives::version::ProtocolFeature; use near_primitives::version::{ProtocolVersion, PROTOCOL_VERSION}; use near_primitives::views::FinalExecutionStatus; use near_primitives_core::account::{AccessKey, Account}; use near_primitives_core::hash::CryptoHash; use near_primitives_core::types::{AccountId, NumSeats}; +use near_store::test_utils::create_test_store; use nearcore::test_utils::TestEnvNightshadeSetupExt; +use rand::rngs::StdRng; +use rand::{Rng, SeedableRng}; +use std::collections::HashSet; const ONE_NEAR: u128 = 1_000_000_000_000_000_000_000_000; @@ -392,138 +391,6 @@ fn test_chunk_state_witness_bad_shard_id() { assert_matches!(error, near_chain::Error::InvalidShardId(_)); } -/// Test that processing chunks with invalid transactions does not lead to panics -#[test] -fn test_invalid_transactions() { - let accounts = - vec!["test0".parse().unwrap(), "test1".parse().unwrap(), "test2".parse().unwrap()]; - let signers: Vec<_> = accounts - .iter() - .map(|account_id: &AccountId| { - create_user_test_signer(AccountIdRef::new(account_id.as_str()).unwrap()) - }) - .collect(); - let genesis = Genesis::test(accounts.clone(), 2); - let mut env = TestEnv::builder(&genesis.config) - .validators(accounts.clone()) - .clients(accounts.clone()) - .nightshade_runtimes(&genesis) - .build(); - let new_signer = create_user_test_signer(AccountIdRef::new("test3").unwrap()); - - let tip = env.clients[0].chain.head().unwrap(); - let sender_account = accounts[0].clone(); - let receiver_account = accounts[1].clone(); - let invalid_transactions = vec![ - // transaction with invalid balance - SignedTransaction::send_money( - 1, - sender_account.clone(), - receiver_account.clone(), - &signers[0].clone().into(), - u128::MAX, - tip.last_block_hash, - ), - // transaction with invalid nonce - SignedTransaction::send_money( - 0, - sender_account.clone(), - receiver_account.clone(), - &signers[0].clone().into(), - ONE_NEAR, - tip.last_block_hash, - ), - // transaction with invalid sender account - SignedTransaction::send_money( - 2, - "test3".parse().unwrap(), - receiver_account.clone(), - &new_signer.into(), - ONE_NEAR, - tip.last_block_hash, - ), - ]; - // Need to create a valid transaction with the same accounts touched in order to have some state witness generated - let valid_tx = SignedTransaction::send_money( - 1, - sender_account, - receiver_account, - &signers[0].clone().into(), - ONE_NEAR, - tip.last_block_hash, - ); - let mut start_height = 1; - for tx in invalid_transactions { - for height in start_height..start_height + 3 { - let tip = env.clients[0].chain.head().unwrap(); - let chunk_producer = env.get_chunk_producer_at_offset(&tip, 1, ShardId::new(0)); - let block_producer = env.get_block_producer_at_offset(&tip, 1); - - let client = env.client(&chunk_producer); - let transactions = if height == start_height { vec![tx.clone()] } else { vec![] }; - if height == start_height { - let res = client.process_tx(valid_tx.clone(), false, false); - assert!(matches!(res, ProcessTxResponse::ValidTx)) - } - - let ( - ProduceChunkResult { - chunk, - encoded_chunk_parts_paths, - receipts, - transactions_storage_proof, - }, - _, - ) = create_chunk_with_transactions(client, transactions); - - let shard_chunk = client - .persist_and_distribute_encoded_chunk( - chunk, - encoded_chunk_parts_paths, - receipts, - client.validator_signer.get().unwrap().validator_id().clone(), - ) - .unwrap(); - let prev_block = client.chain.get_block(shard_chunk.prev_block()).unwrap(); - let prev_chunk_header = Chain::get_prev_chunk_header( - client.epoch_manager.as_ref(), - &prev_block, - shard_chunk.shard_id(), - ) - .unwrap(); - client - .send_chunk_state_witness_to_chunk_validators( - &client - .epoch_manager - .get_epoch_id_from_prev_block(shard_chunk.prev_block()) - .unwrap(), - prev_block.header(), - &prev_chunk_header, - &shard_chunk, - transactions_storage_proof, - &client.validator_signer.get(), - ) - .unwrap(); - - env.process_partial_encoded_chunks(); - for i in 0..env.clients.len() { - env.process_shards_manager_responses(i); - } - env.propagate_chunk_state_witnesses_and_endorsements(true); - let block = env.client(&block_producer).produce_block(height).unwrap().unwrap(); - for client in env.clients.iter_mut() { - client - .process_block_test_no_produce_chunk_allow_errors( - block.clone().into(), - Provenance::NONE, - ) - .unwrap(); - } - } - start_height += 3; - } -} - /// Tests that eth-implicit accounts still work with stateless validation. #[test] fn test_eth_implicit_accounts() { diff --git a/integration-tests/src/tests/client/invalid_txs.rs b/integration-tests/src/tests/client/invalid_txs.rs new file mode 100644 index 00000000000..8193b58adc9 --- /dev/null +++ b/integration-tests/src/tests/client/invalid_txs.rs @@ -0,0 +1,291 @@ +use near_chain::{Chain, Provenance}; +use near_chain_configs::Genesis; +use near_client::test_utils::{create_chunk_with_transactions, TestEnv}; +use near_client::{ProcessTxResponse, ProduceChunkResult}; +use near_primitives::account::id::AccountIdRef; +use near_primitives::test_utils::create_user_test_signer; +use near_primitives::transaction::SignedTransaction; +use near_primitives::types::{AccountId, ShardId}; +use nearcore::test_utils::TestEnvNightshadeSetupExt; + +const ONE_NEAR: u128 = 1_000_000_000_000_000_000_000_000; + +/// Test that processing chunks with invalid transactions does not lead to panics +#[test] +fn test_invalid_transactions_no_panic() { + let accounts = + vec!["test0".parse().unwrap(), "test1".parse().unwrap(), "test2".parse().unwrap()]; + let signers: Vec<_> = accounts + .iter() + .map(|account_id: &AccountId| { + create_user_test_signer(AccountIdRef::new(account_id.as_str()).unwrap()) + }) + .collect(); + let genesis = Genesis::test(accounts.clone(), 2); + let mut env = TestEnv::builder(&genesis.config) + .validators(accounts.clone()) + .clients(accounts.clone()) + .nightshade_runtimes(&genesis) + .build(); + let new_signer = create_user_test_signer(AccountIdRef::new("test3").unwrap()); + + let tip = env.clients[0].chain.head().unwrap(); + let sender_account = accounts[0].clone(); + let receiver_account = accounts[1].clone(); + let invalid_transactions = vec![ + // transaction with invalid balance + SignedTransaction::send_money( + 1, + sender_account.clone(), + receiver_account.clone(), + &signers[0].clone().into(), + u128::MAX, + tip.last_block_hash, + ), + // transaction with invalid nonce + SignedTransaction::send_money( + 0, + sender_account.clone(), + receiver_account.clone(), + &signers[0].clone().into(), + ONE_NEAR, + tip.last_block_hash, + ), + // transaction with invalid sender account + SignedTransaction::send_money( + 2, + "test3".parse().unwrap(), + receiver_account.clone(), + &new_signer.into(), + ONE_NEAR, + tip.last_block_hash, + ), + ]; + // Need to create a valid transaction with the same accounts touched in order to have some state witness generated + let valid_tx = SignedTransaction::send_money( + 1, + sender_account, + receiver_account, + &signers[0].clone().into(), + ONE_NEAR, + tip.last_block_hash, + ); + let mut start_height = 1; + for tx in invalid_transactions { + for height in start_height..start_height + 3 { + let tip = env.clients[0].chain.head().unwrap(); + let chunk_producer = env.get_chunk_producer_at_offset(&tip, 1, ShardId::new(0)); + let block_producer = env.get_block_producer_at_offset(&tip, 1); + + let client = env.client(&chunk_producer); + let transactions = if height == start_height { vec![tx.clone()] } else { vec![] }; + if height == start_height { + let res = client.process_tx(valid_tx.clone(), false, false); + assert!(matches!(res, ProcessTxResponse::ValidTx)) + } + + let ( + ProduceChunkResult { + chunk, + encoded_chunk_parts_paths, + receipts, + transactions_storage_proof, + }, + _, + ) = create_chunk_with_transactions(client, transactions); + + let shard_chunk = client + .persist_and_distribute_encoded_chunk( + chunk, + encoded_chunk_parts_paths, + receipts, + client.validator_signer.get().unwrap().validator_id().clone(), + ) + .unwrap(); + let prev_block = client.chain.get_block(shard_chunk.prev_block()).unwrap(); + let prev_chunk_header = Chain::get_prev_chunk_header( + client.epoch_manager.as_ref(), + &prev_block, + shard_chunk.shard_id(), + ) + .unwrap(); + client + .send_chunk_state_witness_to_chunk_validators( + &client + .epoch_manager + .get_epoch_id_from_prev_block(shard_chunk.prev_block()) + .unwrap(), + prev_block.header(), + &prev_chunk_header, + &shard_chunk, + transactions_storage_proof, + &client.validator_signer.get(), + ) + .unwrap(); + + env.process_partial_encoded_chunks(); + for i in 0..env.clients.len() { + env.process_shards_manager_responses(i); + } + env.propagate_chunk_state_witnesses_and_endorsements(true); + let block = env.client(&block_producer).produce_block(height).unwrap().unwrap(); + for client in env.clients.iter_mut() { + client + .process_block_test_no_produce_chunk_allow_errors( + block.clone().into(), + Provenance::NONE, + ) + .unwrap(); + } + } + start_height += 3; + } +} + +/// Test that processing a chunk with invalid transactions within it does not invalidate the entire +/// chunk and discards just the invalid transactions within. +/// +/// Tests the `RelaxedChunkValidation` feature. +#[test] +#[cfg(feature = "nightly")] +fn test_invalid_transactions_dont_invalidate_chunk() { + use near_chain::ChainStoreAccess as _; + near_o11y::testonly::init_test_logger(); + let accounts = + vec!["test0".parse().unwrap(), "test1".parse().unwrap(), "test2".parse().unwrap()]; + let signers: Vec<_> = accounts + .iter() + .map(|account_id: &AccountId| { + create_user_test_signer(AccountIdRef::new(account_id.as_str()).unwrap()) + }) + .collect(); + let genesis = Genesis::test(accounts.clone(), 2); + let mut env = TestEnv::builder(&genesis.config) + .validators(accounts.clone()) + .clients(accounts.clone()) + .nightshade_runtimes(&genesis) + .build(); + let new_signer = create_user_test_signer(AccountIdRef::new("test3").unwrap()); + + let tip = env.clients[0].chain.head().unwrap(); + let sender_account = accounts[0].clone(); + let receiver_account = accounts[1].clone(); + let chunk_transactions = vec![ + SignedTransaction::send_money( + 1, + sender_account.clone(), + receiver_account.clone(), + &signers[0].clone().into(), + ONE_NEAR, + tip.last_block_hash, + ), + // transaction with invalid balance + SignedTransaction::send_money( + 2, + sender_account.clone(), + receiver_account.clone(), + &signers[0].clone().into(), + u128::MAX, + tip.last_block_hash, + ), + // transaction with invalid nonce + SignedTransaction::send_money( + 0, + sender_account, + receiver_account.clone(), + &signers[0].clone().into(), + ONE_NEAR, + tip.last_block_hash, + ), + // transaction with invalid sender account + SignedTransaction::send_money( + 3, + "test3".parse().unwrap(), + receiver_account, + &new_signer.into(), + ONE_NEAR, + tip.last_block_hash, + ), + ]; + + let tip = env.clients[0].chain.head().unwrap(); + let chunk_producer = env.get_chunk_producer_at_offset(&tip, 1, ShardId::new(0)); + let block_producer = env.get_block_producer_at_offset(&tip, 1); + let client = env.client(&chunk_producer); + let ( + ProduceChunkResult { + chunk, + encoded_chunk_parts_paths, + receipts, + transactions_storage_proof, + }, + _, + ) = create_chunk_with_transactions(client, chunk_transactions); + + let shard_chunk = client + .persist_and_distribute_encoded_chunk( + chunk, + encoded_chunk_parts_paths, + receipts, + client.validator_signer.get().unwrap().validator_id().clone(), + ) + .unwrap(); + + let prev_block = client.chain.get_block(shard_chunk.prev_block()).unwrap(); + let prev_chunk_header = Chain::get_prev_chunk_header( + client.epoch_manager.as_ref(), + &prev_block, + shard_chunk.shard_id(), + ) + .unwrap(); + client + .send_chunk_state_witness_to_chunk_validators( + &client.epoch_manager.get_epoch_id_from_prev_block(shard_chunk.prev_block()).unwrap(), + prev_block.header(), + &prev_chunk_header, + &shard_chunk, + transactions_storage_proof, + &client.validator_signer.get(), + ) + .unwrap(); + + env.process_partial_encoded_chunks(); + for i in 0..env.clients.len() { + env.process_shards_manager_responses(i); + } + env.propagate_chunk_state_witnesses_and_endorsements(true); + let block = env.client(&block_producer).produce_block(1).unwrap().unwrap(); + for client in env.clients.iter_mut() { + let signer = client.validator_signer.get(); + client.start_process_block(block.clone().into(), Provenance::NONE, None, &signer).unwrap(); + near_chain::test_utils::wait_for_all_blocks_in_processing(&mut client.chain); + let (accepted_blocks, _errors) = client.postprocess_ready_blocks(None, true, &signer); + assert_eq!(accepted_blocks.len(), 1); + } + + env.process_partial_encoded_chunks(); + for i in 0..env.clients.len() { + env.process_shards_manager_responses(i); + } + env.propagate_chunk_state_witnesses_and_endorsements(true); + let block = env.client(&block_producer).produce_block(2).unwrap().unwrap(); + for client in env.clients.iter_mut() { + let signer = client.validator_signer.get(); + client.start_process_block(block.clone().into(), Provenance::NONE, None, &signer).unwrap(); + near_chain::test_utils::wait_for_all_blocks_in_processing(&mut client.chain); + let (accepted_blocks, _errors) = client.postprocess_ready_blocks(None, true, &signer); + assert_eq!(accepted_blocks.len(), 1); + } + env.propagate_chunk_state_witnesses_and_endorsements(true); + + let mut receipts = std::collections::BTreeSet::::new(); + for client in env.clients.iter_mut() { + let head = client.chain.get_head_block().unwrap(); + let chunk_hash = head.chunks().iter_raw().next().unwrap().chunk_hash(); + let Ok(chunk) = client.chain.mut_chain_store().get_chunk(&chunk_hash) else { + continue; + }; + receipts.extend(chunk.prev_outgoing_receipts().into_iter().map(|r| *r.receipt_id())); + } + assert_eq!(receipts.len(), 1, "only one receipt for the only valid transaction is expected"); +} diff --git a/integration-tests/src/tests/client/mod.rs b/integration-tests/src/tests/client/mod.rs index 9889f8afde7..10cf7816e78 100644 --- a/integration-tests/src/tests/client/mod.rs +++ b/integration-tests/src/tests/client/mod.rs @@ -5,6 +5,7 @@ mod chunks_management; mod cold_storage; mod features; mod flat_storage; +mod invalid_txs; mod process_blocks; mod resharding_v2; mod runtimes; diff --git a/integration-tests/src/tests/standard_cases/mod.rs b/integration-tests/src/tests/standard_cases/mod.rs index 7cc6e18b705..9615e7817d8 100644 --- a/integration-tests/src/tests/standard_cases/mod.rs +++ b/integration-tests/src/tests/standard_cases/mod.rs @@ -25,7 +25,7 @@ use near_store::trie::TrieNodesCount; use std::sync::Arc; use crate::node::Node; -use crate::user::User; +use crate::user::{CommitError, User}; use near_parameters::{RuntimeConfig, RuntimeConfigStore}; use near_primitives::receipt::{ActionReceipt, Receipt, ReceiptEnum, ReceiptV0}; use near_primitives::test_utils; @@ -841,13 +841,13 @@ pub fn test_delete_key_last(node: impl Node) { // forget the nonce when we delete a key! assert_eq!( err, - ServerError::TxExecutionError(TxExecutionError::InvalidTxError( - InvalidTxError::InvalidAccessKeyError( + CommitError::Server(ServerError::TxExecutionError( + TxExecutionError::InvalidTxError(InvalidTxError::InvalidAccessKeyError( InvalidAccessKeyError::AccessKeyNotFound { account_id: account_id.clone(), public_key: node.signer().public_key().into(), }, - ) + )) )) ) } @@ -1058,14 +1058,18 @@ pub fn test_access_key_smart_contract_reject_method_name(node: impl Node) { let transaction_result = node_user .function_call(account_id.clone(), bob_account(), "run_test", vec![], 10u64.pow(14), 0) .unwrap_err(); - assert_eq!( - transaction_result, - ServerError::TxExecutionError(TxExecutionError::InvalidTxError( - InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::MethodNameMismatch { - method_name: "run_test".to_string() - }) - )) - ); + if cfg!(feature = "protocol_feature_relaxed_chunk_validation") { + assert_eq!(transaction_result, CommitError::OutcomeNotFound); + } else { + assert_eq!( + transaction_result, + CommitError::Server(ServerError::TxExecutionError(TxExecutionError::InvalidTxError( + InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::MethodNameMismatch { + method_name: "run_test".to_string() + }) + ))) + ); + } } pub fn test_access_key_smart_contract_reject_contract_id(node: impl Node) { @@ -1093,15 +1097,19 @@ pub fn test_access_key_smart_contract_reject_contract_id(node: impl Node) { 0, ) .unwrap_err(); - assert_eq!( - transaction_result, - ServerError::TxExecutionError(TxExecutionError::InvalidTxError( - InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::ReceiverMismatch { - tx_receiver: eve_dot_alice_account(), - ak_receiver: bob_account().into() - }) - )) - ); + if cfg!(feature = "protocol_feature_relaxed_chunk_validation") { + assert_eq!(transaction_result, CommitError::OutcomeNotFound); + } else { + assert_eq!( + transaction_result, + CommitError::Server(ServerError::TxExecutionError(TxExecutionError::InvalidTxError( + InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::ReceiverMismatch { + tx_receiver: eve_dot_alice_account(), + ak_receiver: bob_account().into() + }) + ))) + ); + } } pub fn test_access_key_reject_non_function_call(node: impl Node) { @@ -1121,12 +1129,16 @@ pub fn test_access_key_reject_non_function_call(node: impl Node) { let transaction_result = node_user.delete_key(account_id.clone(), node.signer().public_key()).unwrap_err(); - assert_eq!( - transaction_result, - ServerError::TxExecutionError(TxExecutionError::InvalidTxError( - InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::RequiresFullAccess) - )) - ); + if cfg!(feature = "protocol_feature_relaxed_chunk_validation") { + assert_eq!(transaction_result, CommitError::OutcomeNotFound); + } else { + assert_eq!( + transaction_result, + CommitError::Server(ServerError::TxExecutionError(TxExecutionError::InvalidTxError( + InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::RequiresFullAccess) + ))) + ); + } } pub fn test_increase_stake(node: impl Node) { diff --git a/integration-tests/src/tests/test_errors.rs b/integration-tests/src/tests/test_errors.rs index c3dfda51e63..443392ecb4d 100644 --- a/integration-tests/src/tests/test_errors.rs +++ b/integration-tests/src/tests/test_errors.rs @@ -1,6 +1,7 @@ use std::sync::Arc; use crate::node::{Node, ThreadNode}; +use crate::user::CommitError; use near_chain_configs::test_utils::{TESTING_INIT_BALANCE, TESTING_INIT_STAKE}; use near_chain_configs::Genesis; use near_crypto::{InMemorySigner, KeyType}; @@ -55,11 +56,13 @@ fn test_check_tx_error_log() { let tx_result = node.user().commit_transaction(tx).unwrap_err(); assert_eq!( tx_result, - InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::AccessKeyNotFound { - account_id: bob_account(), - public_key: Box::new(signer.public_key()), - }) - .rpc_into() + CommitError::Server( + InvalidTxError::InvalidAccessKeyError(InvalidAccessKeyError::AccessKeyNotFound { + account_id: bob_account(), + public_key: Box::new(signer.public_key()), + }) + .rpc_into() + ) ); } @@ -96,11 +99,13 @@ fn test_deliver_tx_error_log() { let tx_result = node.user().commit_transaction(tx).unwrap_err(); assert_eq!( tx_result, - InvalidTxError::NotEnoughBalance { - signer_id: alice_account(), - balance: TESTING_INIT_BALANCE - TESTING_INIT_STAKE, - cost: TESTING_INIT_BALANCE + 1 + cost - } - .rpc_into() + CommitError::Server( + InvalidTxError::NotEnoughBalance { + signer_id: alice_account(), + balance: TESTING_INIT_BALANCE - TESTING_INIT_STAKE, + cost: TESTING_INIT_BALANCE + 1 + cost + } + .rpc_into() + ) ); } diff --git a/integration-tests/src/user/mod.rs b/integration-tests/src/user/mod.rs index e0b65ca549d..fd5f75fec36 100644 --- a/integration-tests/src/user/mod.rs +++ b/integration-tests/src/user/mod.rs @@ -27,6 +27,27 @@ pub mod runtime_user; const POISONED_LOCK_ERR: &str = "The lock was poisoned."; +#[derive(Debug, PartialEq, Eq)] +pub enum CommitError { + Server(ServerError), + OutcomeNotFound, +} + +impl std::error::Error for CommitError {} +impl std::fmt::Display for CommitError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + CommitError::Server(s) => f.write_fmt(format_args!( + "server error occurred while committing a transaction: {}", + s + )), + CommitError::OutcomeNotFound => { + f.write_str("transaction outcome not found while committing it (tx invalid...)") + } + } + } +} + pub trait User { fn view_account(&self, account_id: &AccountId) -> Result; @@ -53,7 +74,7 @@ pub trait User { fn commit_transaction( &self, signed_transaction: SignedTransaction, - ) -> Result; + ) -> Result; fn add_receipts( &self, @@ -78,7 +99,7 @@ pub trait User { fn get_transaction_result(&self, hash: &CryptoHash) -> Option; - fn get_transaction_final_result(&self, hash: &CryptoHash) -> FinalExecutionOutcomeView; + fn get_transaction_final_result(&self, hash: &CryptoHash) -> Option; fn get_state_root(&self) -> CryptoHash; @@ -97,7 +118,7 @@ pub trait User { signer_id: AccountId, receiver_id: AccountId, actions: Vec, - ) -> Result { + ) -> Result { let block_hash = self.get_best_block_hash().unwrap_or_default(); let signed_transaction = SignedTransaction::from_actions( self.get_access_key_nonce_for_signer(&signer_id).unwrap_or_default() + 1, @@ -116,7 +137,7 @@ pub trait User { signer_id: AccountId, receiver_id: AccountId, amount: Balance, - ) -> Result { + ) -> Result { self.sign_and_commit_actions( signer_id, receiver_id, @@ -128,7 +149,7 @@ pub trait User { &self, signer_id: AccountId, code: Vec, - ) -> Result { + ) -> Result { self.sign_and_commit_actions( signer_id.clone(), signer_id, @@ -144,7 +165,7 @@ pub trait User { args: Vec, gas: Gas, deposit: Balance, - ) -> Result { + ) -> Result { self.sign_and_commit_actions( signer_id, contract_id, @@ -163,7 +184,7 @@ pub trait User { new_account_id: AccountId, public_key: PublicKey, amount: Balance, - ) -> Result { + ) -> Result { self.sign_and_commit_actions( signer_id, new_account_id, @@ -183,7 +204,7 @@ pub trait User { signer_id: AccountId, public_key: PublicKey, access_key: AccessKey, - ) -> Result { + ) -> Result { self.sign_and_commit_actions( signer_id.clone(), signer_id, @@ -195,7 +216,7 @@ pub trait User { &self, signer_id: AccountId, public_key: PublicKey, - ) -> Result { + ) -> Result { self.sign_and_commit_actions( signer_id.clone(), signer_id, @@ -209,7 +230,7 @@ pub trait User { old_public_key: PublicKey, new_public_key: PublicKey, access_key: AccessKey, - ) -> Result { + ) -> Result { self.sign_and_commit_actions( signer_id.clone(), signer_id, @@ -225,7 +246,7 @@ pub trait User { signer_id: AccountId, receiver_id: AccountId, beneficiary_id: AccountId, - ) -> Result { + ) -> Result { self.sign_and_commit_actions( signer_id, receiver_id, @@ -237,7 +258,7 @@ pub trait User { &self, signer_id: AccountId, receiver_id: AccountId, - ) -> Result { + ) -> Result { self.delete_account_with_beneficiary_set(signer_id.clone(), receiver_id, signer_id) } @@ -246,7 +267,7 @@ pub trait User { signer_id: AccountId, public_key: PublicKey, stake: Balance, - ) -> Result { + ) -> Result { self.sign_and_commit_actions( signer_id.clone(), signer_id, @@ -264,7 +285,7 @@ pub trait User { receiver_id: AccountId, relayer_id: AccountId, actions: Vec, - ) -> Result { + ) -> Result { let inner_signer = create_user_test_signer(&signer_id); let user_nonce = self .get_access_key(&signer_id, &inner_signer.public_key) diff --git a/integration-tests/src/user/rpc_user.rs b/integration-tests/src/user/rpc_user.rs index 6b0a445976c..58f5f90dfaa 100644 --- a/integration-tests/src/user/rpc_user.rs +++ b/integration-tests/src/user/rpc_user.rs @@ -26,6 +26,8 @@ use near_primitives::views::{ use crate::user::User; +use super::CommitError; + pub struct RpcUser { account_id: AccountId, signer: Arc, @@ -136,7 +138,7 @@ impl User for RpcUser { fn commit_transaction( &self, transaction: SignedTransaction, - ) -> Result { + ) -> Result { let bytes = borsh::to_vec(&transaction).unwrap(); let result = self.actix(move |client| client.broadcast_tx_commit(to_base64(&bytes))); // Wait for one more block, to make sure all nodes actually apply the state transition. @@ -146,7 +148,9 @@ impl User for RpcUser { } match result { Ok(outcome) => Ok(outcome.final_execution_outcome.unwrap().into_outcome()), - Err(err) => Err(serde_json::from_value::(err.data.unwrap()).unwrap()), + Err(err) => Err(CommitError::Server( + serde_json::from_value::(err.data.unwrap()).unwrap(), + )), } } @@ -188,7 +192,7 @@ impl User for RpcUser { unimplemented!() } - fn get_transaction_final_result(&self, hash: &CryptoHash) -> FinalExecutionOutcomeView { + fn get_transaction_final_result(&self, hash: &CryptoHash) -> Option { let request = RpcTransactionStatusRequest { transaction_info: TransactionInfo::TransactionId { tx_hash: *hash, @@ -199,8 +203,7 @@ impl User for RpcUser { self.actix(move |client| client.tx(request)) .unwrap() .final_execution_outcome - .unwrap() - .into_outcome() + .map(|o| o.into_outcome()) } fn get_state_root(&self) -> CryptoHash { diff --git a/integration-tests/src/user/runtime_user.rs b/integration-tests/src/user/runtime_user.rs index 97c86fba0c6..a7c65c07d36 100644 --- a/integration-tests/src/user/runtime_user.rs +++ b/integration-tests/src/user/runtime_user.rs @@ -215,43 +215,48 @@ impl RuntimeUser { } // TODO(#10942) get rid of copy pasted code, it's outdated comparing to the original - fn get_final_transaction_result(&self, hash: &CryptoHash) -> FinalExecutionOutcomeView { + fn get_final_transaction_result(&self, hash: &CryptoHash) -> Option { let mut outcomes = self.get_recursive_transaction_results(hash); let mut looking_for_id = *hash; let num_outcomes = outcomes.len(); - let status = outcomes - .iter() - .find_map(|outcome_with_id| { - if outcome_with_id.id == looking_for_id { - match &outcome_with_id.outcome.status { - ExecutionStatusView::Unknown if num_outcomes == 1 => { - Some(FinalExecutionStatus::NotStarted) - } - ExecutionStatusView::Unknown => Some(FinalExecutionStatus::Started), - ExecutionStatusView::Failure(e) => { - Some(FinalExecutionStatus::Failure(e.clone())) - } - ExecutionStatusView::SuccessValue(v) => { - Some(FinalExecutionStatus::SuccessValue(v.clone())) - } - ExecutionStatusView::SuccessReceiptId(id) => { - looking_for_id = *id; - None - } + let status = outcomes.iter().find_map(|outcome_with_id| { + if outcome_with_id.id == looking_for_id { + match &outcome_with_id.outcome.status { + ExecutionStatusView::Unknown if num_outcomes == 1 => { + Some(FinalExecutionStatus::NotStarted) + } + ExecutionStatusView::Unknown => Some(FinalExecutionStatus::Started), + ExecutionStatusView::Failure(e) => { + Some(FinalExecutionStatus::Failure(e.clone())) + } + ExecutionStatusView::SuccessValue(v) => { + Some(FinalExecutionStatus::SuccessValue(v.clone())) + } + ExecutionStatusView::SuccessReceiptId(id) => { + looking_for_id = *id; + None } - } else { - None } - }) - .expect("results should resolve to a final outcome"); + } else { + None + } + }); + let status = if cfg!(feature = "protocol_feature_relaxed_chunk_validation") { + // If we don't find the transaction at all, it must have been ignored due to having + // been an invalid transaction (but due to relaxed validation we do not invalidate the + // entire chunk.) + status? + } else { + status.expect("results should resolve to a final outcome") + }; let receipts = outcomes.split_off(1); let transaction = self.transactions.borrow().get(hash).unwrap().clone().into(); - FinalExecutionOutcomeView { + Some(FinalExecutionOutcomeView { status, transaction, transaction_outcome: outcomes.pop().unwrap(), receipts_outcome: receipts, - } + }) } } @@ -307,7 +312,7 @@ impl User for RuntimeUser { block_height: apply_state.block_height, prev_block_hash: apply_state.prev_block_hash, block_hash: apply_state.block_hash, - shard_id: shard_id, + shard_id, epoch_id: apply_state.epoch_id, epoch_height: apply_state.epoch_height, block_timestamp: apply_state.block_timestamp, @@ -337,9 +342,11 @@ impl User for RuntimeUser { fn commit_transaction( &self, transaction: SignedTransaction, - ) -> Result { - self.apply_all(self.apply_state(), vec![], vec![transaction.clone()], true)?; - Ok(self.get_transaction_final_result(&transaction.get_hash())) + ) -> Result { + self.apply_all(self.apply_state(), vec![], vec![transaction.clone()], true) + .map_err(super::CommitError::Server)?; + self.get_transaction_final_result(&transaction.get_hash()) + .ok_or(super::CommitError::OutcomeNotFound) } fn add_receipts( @@ -376,7 +383,7 @@ impl User for RuntimeUser { self.transaction_results.borrow().get(hash).cloned() } - fn get_transaction_final_result(&self, hash: &CryptoHash) -> FinalExecutionOutcomeView { + fn get_transaction_final_result(&self, hash: &CryptoHash) -> Option { self.get_final_transaction_result(hash) } diff --git a/runtime/runtime/Cargo.toml b/runtime/runtime/Cargo.toml index 528c5641af0..4f1f5bc7e28 100644 --- a/runtime/runtime/Cargo.toml +++ b/runtime/runtime/Cargo.toml @@ -44,6 +44,7 @@ nightly = [ "near-wallet-contract/nightly", "nightly_protocol", "protocol_feature_nonrefundable_transfer_nep491", + "protocol_feature_relaxed_chunk_validation", "testlib/nightly", ] nightly_protocol = [ @@ -58,6 +59,10 @@ nightly_protocol = [ "testlib/nightly_protocol", ] protocol_feature_nonrefundable_transfer_nep491 = [] +protocol_feature_relaxed_chunk_validation = [ + "near-primitives/protocol_feature_relaxed_chunk_validation", + "near-primitives-core/protocol_feature_relaxed_chunk_validation", +] no_cpu_compatibility_checks = ["near-vm-runner/no_cpu_compatibility_checks"] no_cache = [ diff --git a/runtime/runtime/src/lib.rs b/runtime/runtime/src/lib.rs index 21219bd26ba..348eb0a05e6 100644 --- a/runtime/runtime/src/lib.rs +++ b/runtime/runtime/src/lib.rs @@ -289,13 +289,17 @@ impl Runtime { debug!(target: "runtime", "{}", log_str); } - /// Takes one signed transaction, verifies it and converts it to a receipt. Add this receipt - /// either to the new local receipts if the signer is the same as receiver or to the new - /// outgoing receipts. + /// Takes one signed transaction, verifies it and converts it to a receipt. + /// + /// Add the produced receipt receipt either to the new local receipts if the signer is the same + /// as receiver or to the new outgoing receipts. + /// /// When transaction is converted to a receipt, the account is charged for the full value of /// the generated receipt. - /// In case of successful verification (expected for valid chunks), returns the receipt and - /// `ExecutionOutcomeWithId` for the transaction. + /// + /// In case of successful verification, returns the receipt and `ExecutionOutcomeWithId` for + /// the transaction. + /// /// In case of an error, returns either `InvalidTxError` if the transaction verification failed /// or a `StorageError` wrapped into `RuntimeError`. #[instrument(target = "runtime", level = "debug", "process_transaction", skip_all, fields( @@ -1388,13 +1392,17 @@ impl Runtime { /// Applies new signed transactions and incoming receipts for some chunk/shard on top of /// given trie and the given state root. + /// /// If the validator accounts update is provided, updates validators accounts. - /// All new signed transactions should be valid and already verified by the chunk producer. - /// If any transaction is invalid, it would return an `InvalidTxError`. + /// /// Returns an `ApplyResult` that contains the new state root, trie changes, - /// new outgoing receipts, execution outcomes for - /// all transactions, local action receipts (generated from transactions with signer == - /// receivers) and incoming action receipts. + /// new outgoing receipts, execution outcomes for all transactions, local action receipts + /// (generated from transactions with signer == receivers) and incoming action receipts. + /// + /// Invalid transactions should have been filtered out by the chunk producer, but if a chunk + /// containing invalid transactions does make it to here, these transactions are skipped. This + /// does pollute the chain with junk data, but it also allows the protocol to make progress, as + /// the only alternative way to handle these transactions is to make the entire chunk invalid. #[instrument(target = "runtime", level = "debug", "apply", skip_all, fields( protocol_version = apply_state.current_protocol_version, num_transactions = transactions.len(), @@ -1556,6 +1564,9 @@ impl Runtime { /// /// Fills the `processing_state` with local receipts generated during processing of the /// transactions. + /// + /// Any transactions that fail to validate (e.g. invalid nonces, unknown signing keys, + /// insufficient NEAR balance, etc.) will be skipped, producing no receipts. fn process_transactions<'a>( &self, processing_state: &mut ApplyProcessingReceiptState<'a>, @@ -1565,12 +1576,32 @@ impl Runtime { let apply_state = &mut processing_state.apply_state; let state_update = &mut processing_state.state_update; for signed_transaction in processing_state.transactions { - let (receipt, outcome_with_id) = self.process_transaction( + let tx_result = self.process_transaction( state_update, apply_state, signed_transaction, &mut processing_state.stats, - )?; + ); + let (receipt, outcome_with_id) = match tx_result { + Ok(v) => v, + 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()); + } + } + }; if receipt.receiver_id() == signed_transaction.transaction.signer_id() { processing_state.local_receipts.push_back(receipt); } else {