From c5e4290bf06523ca04867d05afafc66cdaa5a468 Mon Sep 17 00:00:00 2001 From: Longarithm Date: Wed, 10 Mar 2021 13:38:06 +0300 Subject: [PATCH 01/13] Implement first version of validating summary tx size --- core/primitives-core/src/config.rs | 9 +++++++-- core/primitives/src/errors.rs | 7 +++++++ core/primitives/src/transaction.rs | 7 +++++++ runtime/runtime/src/config.rs | 9 +++++++++ runtime/runtime/src/verifier.rs | 11 ++++++++++- 5 files changed, 40 insertions(+), 3 deletions(-) diff --git a/core/primitives-core/src/config.rs b/core/primitives-core/src/config.rs index 2624a9dda2e..08ee3ea04db 100644 --- a/core/primitives-core/src/config.rs +++ b/core/primitives-core/src/config.rs @@ -68,6 +68,8 @@ pub struct VMLimitConfig { pub max_length_returned_data: u64, /// Max contract size pub max_contract_size: u64, + /// Max contract size + pub max_transaction_size: u64, /// Max storage key size pub max_length_storage_key: u64, /// Max storage value size @@ -146,9 +148,12 @@ impl Default for VMLimitConfig { // Should be low enough to deserialize an access key without paying. max_number_bytes_method_names: 2000, max_length_method_name: 256, // basic safety limit - max_arguments_length: 4 * 2u64.pow(20), // 4 Mib + // max_arguments_length: 4 * 2u64.pow(20), // 4 Mib + max_arguments_length: 100 * 2u64.pow(10), // 100 Kib max_length_returned_data: 4 * 2u64.pow(20), // 4 Mib - max_contract_size: 4 * 2u64.pow(20), // 4 Mib, + // max_contract_size: 4 * 2u64.pow(20), // 4 Mib, + max_contract_size: 300 * 2u64.pow(10), // 300 Kib + max_transaction_size: 300 * 2u64.pow(10), // 300 Kib max_length_storage_key: 4 * 2u64.pow(20), // 4 Mib max_length_storage_value: 4 * 2u64.pow(20), // 4 Mib diff --git a/core/primitives/src/errors.rs b/core/primitives/src/errors.rs index 77b15c2f704..bb02b714d0f 100644 --- a/core/primitives/src/errors.rs +++ b/core/primitives/src/errors.rs @@ -197,6 +197,8 @@ pub enum ActionsValidationError { UnsuitableStakingKey { public_key: PublicKey }, /// The attached amount of gas in a FunctionCall action has to be a positive number. FunctionCallZeroAttachedGas, + /// The size of all actions exceeded the limit. + TransactionSizeExceeded { total_size: u64, limit: u64 }, } /// Describes the error for validating a receipt. @@ -311,6 +313,11 @@ impl Display for ActionsValidationError { f, "The attached amount of gas in a FunctionCall action has to be a positive number", ), + ActionsValidationError::TransactionSizeExceeded { total_size, limit } => write!( + f, + "Total transaction size {} exceeded the limit {}", + total_size, limit + ), } } } diff --git a/core/primitives/src/transaction.rs b/core/primitives/src/transaction.rs index 5efca94904c..52e0c1681ff 100644 --- a/core/primitives/src/transaction.rs +++ b/core/primitives/src/transaction.rs @@ -72,6 +72,13 @@ impl Action { _ => 0, } } + pub fn get_size(&self) -> usize { + match self { + Action::FunctionCall(a) => a.args.len(), + Action::DeployContract(a) => a.code.len(), + _ => 0, + } + } } /// Create account action diff --git a/runtime/runtime/src/config.rs b/runtime/runtime/src/config.rs index d338ace603e..e7e55623ae5 100644 --- a/runtime/runtime/src/config.rs +++ b/runtime/runtime/src/config.rs @@ -60,6 +60,10 @@ pub fn safe_add_balance(a: Balance, b: Balance) -> Result Result { + a.checked_add(b).ok_or_else(|| IntegerOverflowError {}) +} + #[macro_export] macro_rules! safe_add_balance_apply { ($x: expr) => {$x}; @@ -264,6 +268,11 @@ pub fn total_deposit(actions: &[Action]) -> Result Result { + actions.iter().try_fold(0, |acc, action| safe_add_usize(acc, action.get_size())) +} + /// Get the total sum of prepaid gas for given actions. pub fn total_prepaid_gas(actions: &[Action]) -> Result { actions.iter().try_fold(0, |acc, action| safe_add_gas(acc, action.get_prepaid_gas())) diff --git a/runtime/runtime/src/verifier.rs b/runtime/runtime/src/verifier.rs index fe786c4b177..458c446768b 100644 --- a/runtime/runtime/src/verifier.rs +++ b/runtime/runtime/src/verifier.rs @@ -20,7 +20,7 @@ use near_store::{ get_access_key, get_account, set_access_key, set_account, StorageError, TrieUpdate, }; -use crate::config::{total_prepaid_gas, tx_cost, TransactionCost}; +use crate::config::{total_prepaid_gas, tx_cost, TransactionCost, total_size}; use crate::VerificationResult; use near_primitives::runtime::config::RuntimeConfig; @@ -305,6 +305,15 @@ pub(crate) fn validate_actions( }); } + let total_size = + total_size(actions).map_err(|_| ActionsValidationError::IntegerOverflow)? as u64; + if total_size > limit_config.max_transaction_size { + return Err(ActionsValidationError::TransactionSizeExceeded { + total_size, + limit: limit_config.max_transaction_size, + }); + } + Ok(()) } From caf9c776ee93a31f95bcc8e811a7750d1a14e6d0 Mon Sep 17 00:00:00 2001 From: Longarithm Date: Sat, 13 Mar 2021 03:46:12 +0300 Subject: [PATCH 02/13] Add feature flag for limiting tx size --- Cargo.toml | 3 ++- chain/chain/src/validate.rs | 2 +- core/primitives-core/Cargo.toml | 1 + core/primitives-core/src/config.rs | 12 ++++++------ core/primitives/Cargo.toml | 3 ++- core/primitives/src/errors.rs | 10 ++++++---- core/primitives/src/transaction.rs | 1 + core/primitives/src/version.rs | 6 +++++- neard/Cargo.toml | 3 ++- runtime/runtime/Cargo.toml | 1 + runtime/runtime/src/config.rs | 12 +++++++----- runtime/runtime/src/verifier.rs | 20 ++++++++++++-------- test-utils/testlib/Cargo.toml | 1 + 13 files changed, 47 insertions(+), 28 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 1dd38eb47a7..8dc8e169d34 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -108,11 +108,12 @@ metric_recorder = ["neard/metric_recorder"] delay_detector = ["neard/delay_detector"] rosetta_rpc = ["neard/rosetta_rpc"] nightly_protocol = ["near-primitives/nightly_protocol", "near-jsonrpc/nightly_protocol"] -nightly_protocol_features = ["nightly_protocol", "neard/nightly_protocol_features", "protocol_feature_evm", "protocol_feature_block_header_v3", "protocol_feature_alt_bn128"] +nightly_protocol_features = ["nightly_protocol", "neard/nightly_protocol_features", "protocol_feature_evm", "protocol_feature_block_header_v3", "protocol_feature_alt_bn128", "protocol_feature_tx_size_limit"] protocol_feature_forward_chunk_parts = ["neard/protocol_feature_forward_chunk_parts"] protocol_feature_evm = ["neard/protocol_feature_evm", "testlib/protocol_feature_evm", "runtime-params-estimator/protocol_feature_evm"] protocol_feature_alt_bn128 = ["neard/protocol_feature_alt_bn128", "testlib/protocol_feature_alt_bn128", "runtime-params-estimator/protocol_feature_alt_bn128"] protocol_feature_block_header_v3 = ["near-primitives/protocol_feature_block_header_v3", "near-chain/protocol_feature_block_header_v3", "neard/protocol_feature_block_header_v3"] +protocol_feature_tx_size_limit = ["near-primitives/protocol_feature_tx_size_limit", "node-runtime/protocol_feature_tx_size_limit", "neard/protocol_feature_tx_size_limit"] # enable this to build neard with wasmer 1.0 runner # now if none of wasmer0_default, wasmer1_default or wasmtime_default is enabled, wasmer0 would be default diff --git a/chain/chain/src/validate.rs b/chain/chain/src/validate.rs index 827c8f0f10d..c40a758712f 100644 --- a/chain/chain/src/validate.rs +++ b/chain/chain/src/validate.rs @@ -83,7 +83,7 @@ pub fn validate_chunk_proofs(chunk: &ShardChunk, runtime_adapter: &dyn RuntimeAd } /// Validates that the given transactions are in proper valid order. -/// See https://nomicon.io/BlockchainLayer/Transactions.html#transaction-ordering +/// See https://nomicon.io/ChainSpec/Transactions.html#transaction-ordering pub fn validate_transactions_order(transactions: &[SignedTransaction]) -> bool { let mut nonces: HashMap<(&AccountId, &PublicKey), Nonce> = HashMap::new(); let mut batches: HashMap<(&AccountId, &PublicKey), usize> = HashMap::new(); diff --git a/core/primitives-core/Cargo.toml b/core/primitives-core/Cargo.toml index 2abac400b77..59e4e224a36 100644 --- a/core/primitives-core/Cargo.toml +++ b/core/primitives-core/Cargo.toml @@ -27,3 +27,4 @@ default = [] costs_counting = [] protocol_feature_evm = [] protocol_feature_alt_bn128 = [] +protocol_feature_tx_size_limit = [] diff --git a/core/primitives-core/src/config.rs b/core/primitives-core/src/config.rs index 763c86f88fe..fe5550f5926 100644 --- a/core/primitives-core/src/config.rs +++ b/core/primitives-core/src/config.rs @@ -68,7 +68,8 @@ pub struct VMLimitConfig { pub max_length_returned_data: u64, /// Max contract size pub max_contract_size: u64, - /// Max contract size + /// Max transaction size + #[cfg(feature = "protocol_feature_tx_size_limit")] pub max_transaction_size: u64, /// Max storage key size pub max_length_storage_key: u64, @@ -148,12 +149,11 @@ impl Default for VMLimitConfig { // Should be low enough to deserialize an access key without paying. max_number_bytes_method_names: 2000, max_length_method_name: 256, // basic safety limit - // max_arguments_length: 4 * 2u64.pow(20), // 4 Mib - max_arguments_length: 100 * 2u64.pow(10), // 100 Kib + max_arguments_length: 4 * 2u64.pow(20), // 4 Mib max_length_returned_data: 4 * 2u64.pow(20), // 4 Mib - // max_contract_size: 4 * 2u64.pow(20), // 4 Mib, - max_contract_size: 300 * 2u64.pow(10), // 300 Kib - max_transaction_size: 300 * 2u64.pow(10), // 300 Kib + max_contract_size: 4 * 2u64.pow(20), // 4 Mib, + #[cfg(feature = "protocol_feature_tx_size_limit")] + max_transaction_size: 4 * 2u64.pow(20), // 4 Mib max_length_storage_key: 4 * 2u64.pow(20), // 4 Mib max_length_storage_value: 4 * 2u64.pow(20), // 4 Mib diff --git a/core/primitives/Cargo.toml b/core/primitives/Cargo.toml index 68910a7af9d..2d18f66f890 100644 --- a/core/primitives/Cargo.toml +++ b/core/primitives/Cargo.toml @@ -48,7 +48,8 @@ protocol_feature_rectify_inflation = [] protocol_feature_evm = ["near-primitives-core/protocol_feature_evm"] protocol_feature_block_header_v3 = [] protocol_feature_alt_bn128 = ["near-primitives-core/protocol_feature_alt_bn128", "near-vm-errors/protocol_feature_alt_bn128"] -nightly_protocol_features = ["nightly_protocol", "protocol_feature_forward_chunk_parts", "protocol_feature_rectify_inflation", "protocol_feature_evm", "protocol_feature_block_header_v3", "protocol_feature_alt_bn128"] +protocol_feature_tx_size_limit = ["near-primitives-core/protocol_feature_tx_size_limit"] +nightly_protocol_features = ["nightly_protocol", "protocol_feature_forward_chunk_parts", "protocol_feature_rectify_inflation", "protocol_feature_evm", "protocol_feature_block_header_v3", "protocol_feature_alt_bn128", "protocol_feature_tx_size_limit"] nightly_protocol = [] diff --git a/core/primitives/src/errors.rs b/core/primitives/src/errors.rs index 2e63e73c131..4bcdeb675af 100644 --- a/core/primitives/src/errors.rs +++ b/core/primitives/src/errors.rs @@ -197,8 +197,9 @@ pub enum ActionsValidationError { UnsuitableStakingKey { public_key: PublicKey }, /// The attached amount of gas in a FunctionCall action has to be a positive number. FunctionCallZeroAttachedGas, - /// The size of all actions exceeded the limit. - TransactionSizeExceeded { total_size: u64, limit: u64 }, + /// Transaction size exceeded the limit. + #[cfg(feature = "protocol_feature_tx_size_limit")] + TransactionSizeExceeded { size: u64, limit: u64 }, } /// Describes the error for validating a receipt. @@ -313,10 +314,11 @@ impl Display for ActionsValidationError { f, "The attached amount of gas in a FunctionCall action has to be a positive number", ), - ActionsValidationError::TransactionSizeExceeded { total_size, limit } => write!( + #[cfg(feature = "protocol_feature_tx_size_limit")] + ActionsValidationError::TransactionSizeExceeded { size, limit } => write!( f, "Total transaction size {} exceeded the limit {}", - total_size, limit + size, limit ), } } diff --git a/core/primitives/src/transaction.rs b/core/primitives/src/transaction.rs index 52e0c1681ff..5d5e47b5312 100644 --- a/core/primitives/src/transaction.rs +++ b/core/primitives/src/transaction.rs @@ -72,6 +72,7 @@ impl Action { _ => 0, } } + #[cfg(feature = "protocol_feature_tx_size_limit")] pub fn get_size(&self) -> usize { match self { Action::FunctionCall(a) => a.args.len(), diff --git a/core/primitives/src/version.rs b/core/primitives/src/version.rs index c6a439ab913..c5c6fe55328 100644 --- a/core/primitives/src/version.rs +++ b/core/primitives/src/version.rs @@ -92,6 +92,8 @@ pub enum ProtocolFeature { LowerStorageCost, #[cfg(feature = "protocol_feature_alt_bn128")] AltBn128, + #[cfg(feature = "protocol_feature_tx_size_limit")] + TransactionSizeLimit, } /// Current latest stable version of the protocol. @@ -100,7 +102,7 @@ pub const PROTOCOL_VERSION: ProtocolVersion = 42; /// Current latest nightly version of the protocol. #[cfg(feature = "nightly_protocol")] -pub const PROTOCOL_VERSION: ProtocolVersion = 105; +pub const PROTOCOL_VERSION: ProtocolVersion = 106; lazy_static! { static ref STABLE_PROTOCOL_FEATURES_TO_VERSION_MAPPING: HashMap = @@ -138,6 +140,8 @@ lazy_static! { (ProtocolFeature::BlockHeaderV3, 104), #[cfg(feature = "protocol_feature_alt_bn128")] (ProtocolFeature::AltBn128, 105), + #[cfg(feature = "protocol_feature_tx_size_limit")] + (ProtocolFeature::TransactionSizeLimit, 106), ] .into_iter() .collect(); diff --git a/neard/Cargo.toml b/neard/Cargo.toml index 69e394664fa..59970e8ffb9 100644 --- a/neard/Cargo.toml +++ b/neard/Cargo.toml @@ -72,7 +72,8 @@ protocol_feature_rectify_inflation = ["near-epoch-manager/protocol_feature_recti protocol_feature_evm = ["near-primitives/protocol_feature_evm", "node-runtime/protocol_feature_evm", "near-chain-configs/protocol_feature_evm", "near-chain/protocol_feature_evm"] protocol_feature_alt_bn128 = ["near-primitives/protocol_feature_alt_bn128", "node-runtime/protocol_feature_alt_bn128"] protocol_feature_block_header_v3 = ["near-epoch-manager/protocol_feature_block_header_v3", "near-primitives/protocol_feature_block_header_v3", "near-chain/protocol_feature_block_header_v3", "near-client/protocol_feature_block_header_v3"] -nightly_protocol_features = ["nightly_protocol", "near-primitives/nightly_protocol_features", "near-client/nightly_protocol_features", "near-epoch-manager/nightly_protocol_features", "near-store/nightly_protocol_features", "protocol_feature_forward_chunk_parts", "protocol_feature_rectify_inflation", "protocol_feature_evm", "protocol_feature_block_header_v3", "protocol_feature_alt_bn128"] +protocol_feature_tx_size_limit = ["near-primitives/protocol_feature_tx_size_limit", "node-runtime/protocol_feature_tx_size_limit"] +nightly_protocol_features = ["nightly_protocol", "near-primitives/nightly_protocol_features", "near-client/nightly_protocol_features", "near-epoch-manager/nightly_protocol_features", "near-store/nightly_protocol_features", "protocol_feature_forward_chunk_parts", "protocol_feature_rectify_inflation", "protocol_feature_evm", "protocol_feature_block_header_v3", "protocol_feature_alt_bn128", "protocol_feature_tx_size_limit"] nightly_protocol = ["near-primitives/nightly_protocol", "near-jsonrpc/nightly_protocol"] [[bin]] diff --git a/runtime/runtime/Cargo.toml b/runtime/runtime/Cargo.toml index 8b9bc4a54bf..092ce1aa626 100644 --- a/runtime/runtime/Cargo.toml +++ b/runtime/runtime/Cargo.toml @@ -51,6 +51,7 @@ protocol_feature_alt_bn128 = [ "near-vm-runner/protocol_feature_alt_bn128", "near-vm-errors/protocol_feature_alt_bn128", ] +protocol_feature_tx_size_limit = [] [dev-dependencies] tempfile = "3" diff --git a/runtime/runtime/src/config.rs b/runtime/runtime/src/config.rs index e7e55623ae5..a9a08d60c08 100644 --- a/runtime/runtime/src/config.rs +++ b/runtime/runtime/src/config.rs @@ -60,6 +60,7 @@ pub fn safe_add_balance(a: Balance, b: Balance) -> Result Result { a.checked_add(b).ok_or_else(|| IntegerOverflowError {}) } @@ -268,16 +269,17 @@ pub fn total_deposit(actions: &[Action]) -> Result Result { - actions.iter().try_fold(0, |acc, action| safe_add_usize(acc, action.get_size())) -} - /// Get the total sum of prepaid gas for given actions. pub fn total_prepaid_gas(actions: &[Action]) -> Result { actions.iter().try_fold(0, |acc, action| safe_add_gas(acc, action.get_prepaid_gas())) } +/// Get the total size of given actions. +#[cfg(feature = "protocol_feature_tx_size_limit")] +pub fn total_size(actions: &[Action]) -> Result { + actions.iter().try_fold(0, |acc, action| safe_add_usize(acc, action.get_size())) +} + #[cfg(test)] mod tests { use super::*; diff --git a/runtime/runtime/src/verifier.rs b/runtime/runtime/src/verifier.rs index 458c446768b..7d833f34215 100644 --- a/runtime/runtime/src/verifier.rs +++ b/runtime/runtime/src/verifier.rs @@ -20,7 +20,9 @@ use near_store::{ get_access_key, get_account, set_access_key, set_account, StorageError, TrieUpdate, }; -use crate::config::{total_prepaid_gas, tx_cost, TransactionCost, total_size}; +use crate::config::{total_prepaid_gas, tx_cost, TransactionCost}; +#[cfg(feature = "protocol_feature_tx_size_limit")] +use crate::config::total_size; use crate::VerificationResult; use near_primitives::runtime::config::RuntimeConfig; @@ -305,13 +307,15 @@ pub(crate) fn validate_actions( }); } - let total_size = - total_size(actions).map_err(|_| ActionsValidationError::IntegerOverflow)? as u64; - if total_size > limit_config.max_transaction_size { - return Err(ActionsValidationError::TransactionSizeExceeded { - total_size, - limit: limit_config.max_transaction_size, - }); + #[cfg(feature = "protocol_feature_tx_size_limit")] { + let total_size = + total_size(actions).map_err(|_| ActionsValidationError::IntegerOverflow)? as u64; + if total_size > limit_config.max_transaction_size { + return Err(ActionsValidationError::TransactionSizeExceeded { + size: total_size, + limit: limit_config.max_transaction_size, + }); + } } Ok(()) diff --git a/test-utils/testlib/Cargo.toml b/test-utils/testlib/Cargo.toml index 60e41d09631..fd48f8779be 100644 --- a/test-utils/testlib/Cargo.toml +++ b/test-utils/testlib/Cargo.toml @@ -54,3 +54,4 @@ protocol_feature_alt_bn128 = [ "near-vm-errors/protocol_feature_alt_bn128", ] protocol_feature_evm = ["near-evm-runner/protocol_feature_evm", "near-primitives/protocol_feature_evm", "neard/protocol_feature_evm", "node-runtime/protocol_feature_evm", "near-chain-configs/protocol_feature_evm", "near-chain/protocol_feature_evm"] +protocol_feature_tx_size_limit = ["near-primitives/protocol_feature_tx_size_limit", "node-runtime/protocol_feature_tx_size_limit"] From 5bb8eea800cdda76ff2787dd7fdb282ae5818ecf Mon Sep 17 00:00:00 2001 From: Longarithm Date: Mon, 15 Mar 2021 15:44:54 +0300 Subject: [PATCH 03/13] Fix formatting --- core/primitives-core/src/config.rs | 2 +- runtime/runtime/src/verifier.rs | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/core/primitives-core/src/config.rs b/core/primitives-core/src/config.rs index fe5550f5926..54889408776 100644 --- a/core/primitives-core/src/config.rs +++ b/core/primitives-core/src/config.rs @@ -153,7 +153,7 @@ impl Default for VMLimitConfig { max_length_returned_data: 4 * 2u64.pow(20), // 4 Mib max_contract_size: 4 * 2u64.pow(20), // 4 Mib, #[cfg(feature = "protocol_feature_tx_size_limit")] - max_transaction_size: 4 * 2u64.pow(20), // 4 Mib + max_transaction_size: 4 * 2u64.pow(20), // 4 Mib max_length_storage_key: 4 * 2u64.pow(20), // 4 Mib max_length_storage_value: 4 * 2u64.pow(20), // 4 Mib diff --git a/runtime/runtime/src/verifier.rs b/runtime/runtime/src/verifier.rs index 7d833f34215..6d42b201d07 100644 --- a/runtime/runtime/src/verifier.rs +++ b/runtime/runtime/src/verifier.rs @@ -20,9 +20,9 @@ use near_store::{ get_access_key, get_account, set_access_key, set_account, StorageError, TrieUpdate, }; -use crate::config::{total_prepaid_gas, tx_cost, TransactionCost}; #[cfg(feature = "protocol_feature_tx_size_limit")] use crate::config::total_size; +use crate::config::{total_prepaid_gas, tx_cost, TransactionCost}; use crate::VerificationResult; use near_primitives::runtime::config::RuntimeConfig; @@ -307,7 +307,8 @@ pub(crate) fn validate_actions( }); } - #[cfg(feature = "protocol_feature_tx_size_limit")] { + #[cfg(feature = "protocol_feature_tx_size_limit")] + { let total_size = total_size(actions).map_err(|_| ActionsValidationError::IntegerOverflow)? as u64; if total_size > limit_config.max_transaction_size { From 29a1ee766287e55e5123ba02ab3c908e2582f1d7 Mon Sep 17 00:00:00 2001 From: Longarithm Date: Mon, 15 Mar 2021 16:58:02 +0300 Subject: [PATCH 04/13] Add test checking exceeding tx size limit --- runtime/runtime/src/verifier.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/runtime/runtime/src/verifier.rs b/runtime/runtime/src/verifier.rs index 6d42b201d07..3ba06f7364d 100644 --- a/runtime/runtime/src/verifier.rs +++ b/runtime/runtime/src/verifier.rs @@ -1424,6 +1424,27 @@ mod tests { ); } + #[test] + #[cfg(feature = "protocol_feature_tx_size_limit")] + fn test_validate_actions_exceeding_tx_size_limit() { + let mut limit_config = VMLimitConfig::default(); + limit_config.max_transaction_size = 3; + let contract_size = 5; + assert_eq!( + validate_actions( + &limit_config, + &vec![Action::DeployContract(DeployContractAction { + code: vec![1; contract_size as usize] + }),] + ) + .expect_err("Expected an error"), + ActionsValidationError::TransactionSizeExceeded { + size: contract_size, + limit: limit_config.max_transaction_size + }, + ); + } + // Individual actions #[test] From 83e8cc9d2e6165541490af9be848d8dfbe2d9909 Mon Sep 17 00:00:00 2001 From: Longarithm Date: Mon, 15 Mar 2021 18:40:24 +0300 Subject: [PATCH 05/13] Add new test --- runtime/runtime/src/verifier.rs | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/runtime/runtime/src/verifier.rs b/runtime/runtime/src/verifier.rs index 3ba06f7364d..8b7d3de29d6 100644 --- a/runtime/runtime/src/verifier.rs +++ b/runtime/runtime/src/verifier.rs @@ -1426,10 +1426,12 @@ mod tests { #[test] #[cfg(feature = "protocol_feature_tx_size_limit")] - fn test_validate_actions_exceeding_tx_size_limit() { + fn test_validate_actions_checking_tx_size_limit() { let mut limit_config = VMLimitConfig::default(); - limit_config.max_transaction_size = 3; let contract_size = 5; + let args_size = 3; + + limit_config.max_transaction_size = 3; assert_eq!( validate_actions( &limit_config, @@ -1443,6 +1445,25 @@ mod tests { limit: limit_config.max_transaction_size }, ); + + limit_config.max_transaction_size = 10; + assert_eq!( + validate_actions( + &limit_config, + &vec![ + Action::DeployContract(DeployContractAction { + code: vec![1; contract_size as usize] + }), + Action::FunctionCall(FunctionCallAction { + method_name: "test-method".to_string(), + args: vec![0; args_size as usize], + gas: 100, + deposit: 0 + }) + ] + ), + Ok(()), + ); } // Individual actions From e8e8e7ff47816a9fb7ca37bfc26769898b52101c Mon Sep 17 00:00:00 2001 From: Bowen Wang Date: Tue, 16 Mar 2021 16:31:45 -0700 Subject: [PATCH 06/13] fix --- core/primitives-core/src/config.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/core/primitives-core/src/config.rs b/core/primitives-core/src/config.rs index 54889408776..9dd8276a3a5 100644 --- a/core/primitives-core/src/config.rs +++ b/core/primitives-core/src/config.rs @@ -21,6 +21,7 @@ pub struct VMConfig { /// Describes limits for VM and Runtime. #[derive(Debug, Serialize, Deserialize, Clone, Hash, PartialEq, Eq)] +#[serde(default)] pub struct VMLimitConfig { /// Max amount of gas that can be used, excluding gas attached to promises. pub max_gas_burnt: Gas, From 98b49f49abee62b943274da93a262758cff5028a Mon Sep 17 00:00:00 2001 From: Longarithm Date: Thu, 18 Mar 2021 15:39:56 +0300 Subject: [PATCH 07/13] Improve computation of transaction size --- core/primitives/src/errors.rs | 18 ++--- core/primitives/src/transaction.rs | 8 -- runtime/runtime/src/config.rs | 6 -- runtime/runtime/src/verifier.rs | 121 ++++++++++++++++------------- 4 files changed, 74 insertions(+), 79 deletions(-) diff --git a/core/primitives/src/errors.rs b/core/primitives/src/errors.rs index 584df64ca98..fa504059595 100644 --- a/core/primitives/src/errors.rs +++ b/core/primitives/src/errors.rs @@ -143,6 +143,9 @@ pub enum InvalidTxError { Expired, /// An error occurred while validating actions of a Transaction. ActionsValidation(ActionsValidationError), + /// The size of serialized transaction exceeded the limit. + #[cfg(feature = "protocol_feature_tx_size_limit")] + TransactionSizeExceeded { size: u64, limit: u64 }, } #[derive( @@ -199,9 +202,6 @@ pub enum ActionsValidationError { UnsuitableStakingKey { public_key: PublicKey }, /// The attached amount of gas in a FunctionCall action has to be a positive number. FunctionCallZeroAttachedGas, - /// Transaction size exceeded the limit. - #[cfg(feature = "protocol_feature_tx_size_limit")] - TransactionSizeExceeded { size: u64, limit: u64 }, } /// Describes the error for validating a receipt. @@ -316,12 +316,6 @@ impl Display for ActionsValidationError { f, "The attached amount of gas in a FunctionCall action has to be a positive number", ), - #[cfg(feature = "protocol_feature_tx_size_limit")] - ActionsValidationError::TransactionSizeExceeded { size, limit } => write!( - f, - "Total transaction size {} exceeded the limit {}", - size, limit - ), } } } @@ -448,6 +442,12 @@ impl Display for InvalidTxError { write!(f, "Transaction actions validation error: {}", error) } InvalidTxError::NonceTooLarge { tx_nonce, upper_bound } => { write!(f, "Transaction nonce {} must be smaller than the access key nonce upper bound {}", tx_nonce, upper_bound) } + #[cfg(feature = "protocol_feature_tx_size_limit")] + InvalidTxError::TransactionSizeExceeded { size, limit } => write!( + f, + "Size of serialized transaction {} exceeded the limit {}", + size, limit + ), } } } diff --git a/core/primitives/src/transaction.rs b/core/primitives/src/transaction.rs index 5d5e47b5312..5efca94904c 100644 --- a/core/primitives/src/transaction.rs +++ b/core/primitives/src/transaction.rs @@ -72,14 +72,6 @@ impl Action { _ => 0, } } - #[cfg(feature = "protocol_feature_tx_size_limit")] - pub fn get_size(&self) -> usize { - match self { - Action::FunctionCall(a) => a.args.len(), - Action::DeployContract(a) => a.code.len(), - _ => 0, - } - } } /// Create account action diff --git a/runtime/runtime/src/config.rs b/runtime/runtime/src/config.rs index a9a08d60c08..c52fa8e03d2 100644 --- a/runtime/runtime/src/config.rs +++ b/runtime/runtime/src/config.rs @@ -274,12 +274,6 @@ pub fn total_prepaid_gas(actions: &[Action]) -> Result Result { - actions.iter().try_fold(0, |acc, action| safe_add_usize(acc, action.get_size())) -} - #[cfg(test)] mod tests { use super::*; diff --git a/runtime/runtime/src/verifier.rs b/runtime/runtime/src/verifier.rs index f010ee3dfc2..41687977447 100644 --- a/runtime/runtime/src/verifier.rs +++ b/runtime/runtime/src/verifier.rs @@ -20,13 +20,13 @@ use near_store::{ get_access_key, get_account, set_access_key, set_account, StorageError, TrieUpdate, }; -#[cfg(feature = "protocol_feature_tx_size_limit")] -use crate::config::total_size; use crate::config::{total_prepaid_gas, tx_cost, TransactionCost}; use crate::VerificationResult; use near_primitives::checked_feature; use near_primitives::runtime::config::RuntimeConfig; use near_primitives::types::BlockHeight; +#[cfg(feature = "protocol_feature_tx_size_limit")] +use borsh::ser::BorshSerialize; /// Validates the transaction without using the state. It allows any node to validate a /// transaction before forwarding it to the node that tracks the `signer_id` account. @@ -57,6 +57,19 @@ pub fn validate_transaction( return Err(InvalidTxError::InvalidSignature.into()); } + #[cfg(feature = "protocol_feature_tx_size_limit")] + { + let transaction_size = signed_transaction.try_to_vec() + .expect("Borsh serializer is not expected to ever fail").len() as u64; + let max_transaction_size = config.wasm_config.limit_config.max_transaction_size; + if transaction_size > max_transaction_size { + return Err(InvalidTxError::TransactionSizeExceeded { + size: transaction_size, + limit: max_transaction_size, + }.into()); + } + } + validate_actions(&config.wasm_config.limit_config, &transaction.actions) .map_err(|e| InvalidTxError::ActionsValidation(e))?; @@ -328,18 +341,6 @@ pub(crate) fn validate_actions( }); } - #[cfg(feature = "protocol_feature_tx_size_limit")] - { - let total_size = - total_size(actions).map_err(|_| ActionsValidationError::IntegerOverflow)? as u64; - if total_size > limit_config.max_transaction_size { - return Err(ActionsValidationError::TransactionSizeExceeded { - size: total_size, - limit: limit_config.max_transaction_size, - }); - } - } - Ok(()) } @@ -1190,6 +1191,56 @@ mod tests { ); } + #[test] + #[cfg(feature = "protocol_feature_tx_size_limit")] + fn test_validate_transaction_exceeding_tx_size_limit() { + let (signer, mut state_update, gas_price) = setup_common( + TESTING_INIT_BALANCE, + 0, + Some(AccessKey { + nonce: 0, + permission: AccessKeyPermission::FunctionCall(FunctionCallPermission { + allowance: None, + receiver_id: bob_account(), + method_names: vec!["not_hello".to_string(), "world".to_string()], + }), + }), + ); + + let transaction = SignedTransaction::from_actions( + 1, + alice_account(), + bob_account(), + &*signer, + vec![Action::DeployContract(DeployContractAction { + code: vec![1; 5] + }),], + CryptoHash::default()); + let transaction_size = transaction.try_to_vec() + .expect("Borsh serializer is not expected to ever fail").len() as u64; + + let mut config = RuntimeConfig::default(); + let max_transaction_size = transaction_size - 1; + config.wasm_config.limit_config.max_transaction_size = transaction_size - 1; + + assert_eq!( + verify_and_charge_transaction( + &config, + &mut state_update, + gas_price, + &transaction, + true, + None, + PROTOCOL_VERSION, + ) + .expect_err("expected an error"), + RuntimeError::InvalidTxError(InvalidTxError::TransactionSizeExceeded { + size: transaction_size, + limit: max_transaction_size + }), + ); + } + // Receipts #[test] @@ -1459,48 +1510,6 @@ mod tests { ); } - #[test] - #[cfg(feature = "protocol_feature_tx_size_limit")] - fn test_validate_actions_checking_tx_size_limit() { - let mut limit_config = VMLimitConfig::default(); - let contract_size = 5; - let args_size = 3; - - limit_config.max_transaction_size = 3; - assert_eq!( - validate_actions( - &limit_config, - &vec![Action::DeployContract(DeployContractAction { - code: vec![1; contract_size as usize] - }),] - ) - .expect_err("Expected an error"), - ActionsValidationError::TransactionSizeExceeded { - size: contract_size, - limit: limit_config.max_transaction_size - }, - ); - - limit_config.max_transaction_size = 10; - assert_eq!( - validate_actions( - &limit_config, - &vec![ - Action::DeployContract(DeployContractAction { - code: vec![1; contract_size as usize] - }), - Action::FunctionCall(FunctionCallAction { - method_name: "test-method".to_string(), - args: vec![0; args_size as usize], - gas: 100, - deposit: 0 - }) - ] - ), - Ok(()), - ); - } - // Individual actions #[test] From 4956d18db6a72278d13a0188c3b1f9a51531fc7d Mon Sep 17 00:00:00 2001 From: Longarithm Date: Thu, 18 Mar 2021 15:50:40 +0300 Subject: [PATCH 08/13] Add check for tx with valid size --- runtime/runtime/src/verifier.rs | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/runtime/runtime/src/verifier.rs b/runtime/runtime/src/verifier.rs index 41687977447..1a4588bda33 100644 --- a/runtime/runtime/src/verifier.rs +++ b/runtime/runtime/src/verifier.rs @@ -1194,18 +1194,8 @@ mod tests { #[test] #[cfg(feature = "protocol_feature_tx_size_limit")] fn test_validate_transaction_exceeding_tx_size_limit() { - let (signer, mut state_update, gas_price) = setup_common( - TESTING_INIT_BALANCE, - 0, - Some(AccessKey { - nonce: 0, - permission: AccessKeyPermission::FunctionCall(FunctionCallPermission { - allowance: None, - receiver_id: bob_account(), - method_names: vec!["not_hello".to_string(), "world".to_string()], - }), - }), - ); + let (signer, mut state_update, gas_price) = + setup_common(TESTING_INIT_BALANCE, 0, Some(AccessKey::full_access())); let transaction = SignedTransaction::from_actions( 1, @@ -1229,7 +1219,7 @@ mod tests { &mut state_update, gas_price, &transaction, - true, + false, None, PROTOCOL_VERSION, ) @@ -1239,6 +1229,17 @@ mod tests { limit: max_transaction_size }), ); + + config.wasm_config.limit_config.max_transaction_size = transaction_size + 1; + verify_and_charge_transaction( + &config, + &mut state_update, + gas_price, + &transaction, + false, + None, + PROTOCOL_VERSION, + ).expect("valid transaction"); } // Receipts From b87a56ac58710bd63fc921a708df06ab7249e5dc Mon Sep 17 00:00:00 2001 From: Longarithm Date: Thu, 18 Mar 2021 16:19:41 +0300 Subject: [PATCH 09/13] Run cargo fmt --- runtime/runtime/src/verifier.rs | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/runtime/runtime/src/verifier.rs b/runtime/runtime/src/verifier.rs index 1a4588bda33..07f3a0c50df 100644 --- a/runtime/runtime/src/verifier.rs +++ b/runtime/runtime/src/verifier.rs @@ -22,11 +22,11 @@ use near_store::{ use crate::config::{total_prepaid_gas, tx_cost, TransactionCost}; use crate::VerificationResult; +#[cfg(feature = "protocol_feature_tx_size_limit")] +use borsh::ser::BorshSerialize; use near_primitives::checked_feature; use near_primitives::runtime::config::RuntimeConfig; use near_primitives::types::BlockHeight; -#[cfg(feature = "protocol_feature_tx_size_limit")] -use borsh::ser::BorshSerialize; /// Validates the transaction without using the state. It allows any node to validate a /// transaction before forwarding it to the node that tracks the `signer_id` account. @@ -59,14 +59,17 @@ pub fn validate_transaction( #[cfg(feature = "protocol_feature_tx_size_limit")] { - let transaction_size = signed_transaction.try_to_vec() - .expect("Borsh serializer is not expected to ever fail").len() as u64; + let transaction_size = signed_transaction + .try_to_vec() + .expect("Borsh serializer is not expected to ever fail") + .len() as u64; let max_transaction_size = config.wasm_config.limit_config.max_transaction_size; if transaction_size > max_transaction_size { return Err(InvalidTxError::TransactionSizeExceeded { size: transaction_size, limit: max_transaction_size, - }.into()); + } + .into()); } } @@ -1202,12 +1205,12 @@ mod tests { alice_account(), bob_account(), &*signer, - vec![Action::DeployContract(DeployContractAction { - code: vec![1; 5] - }),], - CryptoHash::default()); - let transaction_size = transaction.try_to_vec() - .expect("Borsh serializer is not expected to ever fail").len() as u64; + vec![Action::DeployContract(DeployContractAction { code: vec![1; 5] })], + CryptoHash::default(), + ); + let transaction_size = + transaction.try_to_vec().expect("Borsh serializer is not expected to ever fail").len() + as u64; let mut config = RuntimeConfig::default(); let max_transaction_size = transaction_size - 1; @@ -1223,7 +1226,7 @@ mod tests { None, PROTOCOL_VERSION, ) - .expect_err("expected an error"), + .expect_err("expected an error"), RuntimeError::InvalidTxError(InvalidTxError::TransactionSizeExceeded { size: transaction_size, limit: max_transaction_size @@ -1239,7 +1242,8 @@ mod tests { false, None, PROTOCOL_VERSION, - ).expect("valid transaction"); + ) + .expect("valid transaction"); } // Receipts From f379ff776e21c688dc94c8fa1c30eec865a2f9f3 Mon Sep 17 00:00:00 2001 From: Longarithm Date: Thu, 18 Mar 2021 22:06:02 +0300 Subject: [PATCH 10/13] Add TODO --- core/primitives-core/src/config.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/core/primitives-core/src/config.rs b/core/primitives-core/src/config.rs index 9dd8276a3a5..8d85cf38051 100644 --- a/core/primitives-core/src/config.rs +++ b/core/primitives-core/src/config.rs @@ -20,6 +20,7 @@ pub struct VMConfig { } /// Describes limits for VM and Runtime. +/// TODO #4139: consider switching to strongly-typed wrappers instead of raw quantities #[derive(Debug, Serialize, Deserialize, Clone, Hash, PartialEq, Eq)] #[serde(default)] pub struct VMLimitConfig { From 030c85a5d7d5a757af3a73b5f9ec26cca42dc59a Mon Sep 17 00:00:00 2001 From: Longarithm Date: Fri, 19 Mar 2021 23:53:21 +0300 Subject: [PATCH 11/13] Introduce size field for SignedTransaction --- chain/rosetta-rpc/src/lib.rs | 2 +- core/primitives/src/test_utils.rs | 2 +- core/primitives/src/transaction.rs | 12 ++++++++---- runtime/runtime/src/verifier.rs | 6 +----- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/chain/rosetta-rpc/src/lib.rs b/chain/rosetta-rpc/src/lib.rs index 561181203a4..51b2f32d359 100644 --- a/chain/rosetta-rpc/src/lib.rs +++ b/chain/rosetta-rpc/src/lib.rs @@ -655,7 +655,7 @@ async fn construction_payloads( actions, }; - let transaction_hash = unsigned_transaction.get_hash().clone(); + let (transaction_hash, _) = unsigned_transaction.get_hash_and_size().clone(); Ok(Json(models::ConstructionPayloadsResponse { unsigned_transaction: unsigned_transaction.into(), diff --git a/core/primitives/src/test_utils.rs b/core/primitives/src/test_utils.rs index 5b3eead5583..e650bfb11b4 100644 --- a/core/primitives/src/test_utils.rs +++ b/core/primitives/src/test_utils.rs @@ -42,7 +42,7 @@ impl Transaction { } pub fn sign(self, signer: &dyn Signer) -> SignedTransaction { - let signature = signer.sign(self.get_hash().as_ref()); + let signature = signer.sign(self.get_hash_and_size().0.as_ref()); SignedTransaction::new(signature, self) } diff --git a/core/primitives/src/transaction.rs b/core/primitives/src/transaction.rs index 5efca94904c..8e14b702284 100644 --- a/core/primitives/src/transaction.rs +++ b/core/primitives/src/transaction.rs @@ -37,9 +37,9 @@ pub struct Transaction { impl Transaction { /// Computes a hash of the transaction for signing - pub fn get_hash(&self) -> CryptoHash { + pub fn get_hash_and_size(&self) -> (CryptoHash, u64) { let bytes = self.try_to_vec().expect("Failed to deserialize"); - hash(&bytes) + (hash(&bytes), bytes.len() as u64) } } @@ -205,22 +205,26 @@ pub struct SignedTransaction { pub signature: Signature, #[borsh_skip] hash: CryptoHash, + #[borsh_skip] + size: u64, } impl SignedTransaction { pub fn new(signature: Signature, transaction: Transaction) -> Self { - let mut signed_tx = Self { signature, transaction, hash: CryptoHash::default() }; + let mut signed_tx = Self { signature, transaction, hash: CryptoHash::default(), size: u64::default() }; signed_tx.init(); signed_tx } pub fn init(&mut self) { - self.hash = self.transaction.get_hash(); + (self.hash, self.size) = self.transaction.get_hash_and_size(); } pub fn get_hash(&self) -> CryptoHash { self.hash } + + pub fn get_size(&self) -> u64 { self.size } } impl Hash for SignedTransaction { diff --git a/runtime/runtime/src/verifier.rs b/runtime/runtime/src/verifier.rs index 83d440e961f..870614bb404 100644 --- a/runtime/runtime/src/verifier.rs +++ b/runtime/runtime/src/verifier.rs @@ -59,12 +59,8 @@ pub fn validate_transaction( #[cfg(feature = "protocol_feature_tx_size_limit")] { - let transaction_size = signed_transaction - .try_to_vec() - .expect("Borsh serializer is not expected to ever fail") - .len() as u64; let max_transaction_size = config.wasm_config.limit_config.max_transaction_size; - if transaction_size > max_transaction_size { + if signed_transaction.get_size() > max_transaction_size { return Err(InvalidTxError::TransactionSizeExceeded { size: transaction_size, limit: max_transaction_size, From 69ead0e42c00ed089815ff8fa7b77f1ac9616010 Mon Sep 17 00:00:00 2001 From: Longarithm Date: Sat, 20 Mar 2021 00:08:52 +0300 Subject: [PATCH 12/13] Fix error in test --- core/primitives/src/transaction.rs | 13 +++++++++---- runtime/runtime/src/verifier.rs | 9 +++------ 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/core/primitives/src/transaction.rs b/core/primitives/src/transaction.rs index 8e14b702284..baf4e43e28c 100644 --- a/core/primitives/src/transaction.rs +++ b/core/primitives/src/transaction.rs @@ -36,7 +36,7 @@ pub struct Transaction { } impl Transaction { - /// Computes a hash of the transaction for signing + /// Computes a hash of the transaction for signing and size of serialized transaction pub fn get_hash_and_size(&self) -> (CryptoHash, u64) { let bytes = self.try_to_vec().expect("Failed to deserialize"); (hash(&bytes), bytes.len() as u64) @@ -211,20 +211,25 @@ pub struct SignedTransaction { impl SignedTransaction { pub fn new(signature: Signature, transaction: Transaction) -> Self { - let mut signed_tx = Self { signature, transaction, hash: CryptoHash::default(), size: u64::default() }; + let mut signed_tx = + Self { signature, transaction, hash: CryptoHash::default(), size: u64::default() }; signed_tx.init(); signed_tx } pub fn init(&mut self) { - (self.hash, self.size) = self.transaction.get_hash_and_size(); + let hash_and_size = self.transaction.get_hash_and_size(); + self.hash = hash_and_size.0; + self.size = hash_and_size.1; } pub fn get_hash(&self) -> CryptoHash { self.hash } - pub fn get_size(&self) -> u64 { self.size } + pub fn get_size(&self) -> u64 { + self.size + } } impl Hash for SignedTransaction { diff --git a/runtime/runtime/src/verifier.rs b/runtime/runtime/src/verifier.rs index 870614bb404..5813d17329b 100644 --- a/runtime/runtime/src/verifier.rs +++ b/runtime/runtime/src/verifier.rs @@ -22,8 +22,6 @@ use near_store::{ use crate::config::{total_prepaid_gas, tx_cost, TransactionCost}; use crate::VerificationResult; -#[cfg(feature = "protocol_feature_tx_size_limit")] -use borsh::ser::BorshSerialize; use near_primitives::checked_feature; use near_primitives::runtime::config::RuntimeConfig; use near_primitives::types::BlockHeight; @@ -59,8 +57,9 @@ pub fn validate_transaction( #[cfg(feature = "protocol_feature_tx_size_limit")] { + let transaction_size = signed_transaction.get_size(); let max_transaction_size = config.wasm_config.limit_config.max_transaction_size; - if signed_transaction.get_size() > max_transaction_size { + if transaction_size > max_transaction_size { return Err(InvalidTxError::TransactionSizeExceeded { size: transaction_size, limit: max_transaction_size, @@ -1205,9 +1204,7 @@ mod tests { vec![Action::DeployContract(DeployContractAction { code: vec![1; 5] })], CryptoHash::default(), ); - let transaction_size = - transaction.try_to_vec().expect("Borsh serializer is not expected to ever fail").len() - as u64; + let transaction_size = transaction.get_size(); let mut config = RuntimeConfig::default(); let max_transaction_size = transaction_size - 1; From 7eee61143ba089cb27205e0060d92dc0caaa518f Mon Sep 17 00:00:00 2001 From: Longarithm Date: Wed, 24 Mar 2021 03:04:50 +0300 Subject: [PATCH 13/13] Minor fix --- core/primitives/src/transaction.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/primitives/src/transaction.rs b/core/primitives/src/transaction.rs index baf4e43e28c..724a76aaeeb 100644 --- a/core/primitives/src/transaction.rs +++ b/core/primitives/src/transaction.rs @@ -218,9 +218,9 @@ impl SignedTransaction { } pub fn init(&mut self) { - let hash_and_size = self.transaction.get_hash_and_size(); - self.hash = hash_and_size.0; - self.size = hash_and_size.1; + let (hash, size) = self.transaction.get_hash_and_size(); + self.hash = hash; + self.size = size; } pub fn get_hash(&self) -> CryptoHash {