From 87e577db4cb0dbaafc150e3b72a5930a9b52f647 Mon Sep 17 00:00:00 2001 From: "Jeff Washington (jwash)" Date: Sun, 12 Dec 2021 15:00:35 -0600 Subject: [PATCH] Revert "use cost model to limit new account creation (#21369)" This reverts commit 90f41fd9b743ac4c78f032ac9cb38259ebf152d8. --- .../postgres_client_transaction.rs | 4 - core/src/banking_stage.rs | 4 +- core/src/qos_service.rs | 11 -- runtime/src/accounts.rs | 11 +- runtime/src/bank.rs | 5 +- runtime/src/block_cost_limits.rs | 3 - runtime/src/cost_model.rs | 108 +----------------- runtime/src/cost_tracker.rs | 101 ++++------------ sdk/src/transaction/error.rs | 4 - storage-proto/proto/transaction_by_addr.proto | 1 - storage-proto/src/convert.rs | 4 - 11 files changed, 33 insertions(+), 223 deletions(-) diff --git a/accountsdb-plugin-postgres/src/postgres_client/postgres_client_transaction.rs b/accountsdb-plugin-postgres/src/postgres_client/postgres_client_transaction.rs index f48b2456cf..5aa7e27851 100644 --- a/accountsdb-plugin-postgres/src/postgres_client/postgres_client_transaction.rs +++ b/accountsdb-plugin-postgres/src/postgres_client/postgres_client_transaction.rs @@ -330,7 +330,6 @@ pub enum DbTransactionErrorCode { WouldExceedMaxBlockCostLimit, UnsupportedVersion, InvalidWritableAccount, - WouldExceedMaxAccountDataCostLimit, } impl From<&TransactionError> for DbTransactionErrorCode { @@ -359,9 +358,6 @@ impl From<&TransactionError> for DbTransactionErrorCode { TransactionError::WouldExceedMaxBlockCostLimit => Self::WouldExceedMaxBlockCostLimit, TransactionError::UnsupportedVersion => Self::UnsupportedVersion, TransactionError::InvalidWritableAccount => Self::InvalidWritableAccount, - TransactionError::WouldExceedMaxAccountDataCostLimit => { - Self::WouldExceedMaxAccountDataCostLimit - } } } } diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index 15898d4ff6..3328c5c140 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -949,8 +949,8 @@ impl BankingStage { bank.prepare_sanitized_batch_with_results(txs, transactions_qos_results.into_iter()); lock_time.stop(); - // retryable_txs includes AccountInUse, WouldExceedMaxBlockCostLimit - // WouldExceedMaxAccountCostLimit, and WouldExceedMaxAccountDataCostLimit + // retryable_txs includes AccountInUse, WouldExceedMaxBlockCostLimit and + // WouldExceedMaxAccountCostLimit let (result, mut retryable_txs) = Self::process_and_record_transactions_locked( bank, poh, diff --git a/core/src/qos_service.rs b/core/src/qos_service.rs index de50031ce4..435ed56441 100644 --- a/core/src/qos_service.rs +++ b/core/src/qos_service.rs @@ -133,10 +133,6 @@ impl QosService { self.metrics.retried_txs_per_account_limit_count.fetch_add(1, Ordering::Relaxed); Err(TransactionError::WouldExceedMaxAccountCostLimit) } - CostTrackerError::WouldExceedAccountDataMaxLimit => { - self.metrics.retried_txs_per_account_data_limit_count.fetch_add(1, Ordering::Relaxed); - Err(TransactionError::WouldExceedMaxAccountDataCostLimit) - } } } }) @@ -169,7 +165,6 @@ struct QosServiceMetrics { selected_txs_count: AtomicU64, retried_txs_per_block_limit_count: AtomicU64, retried_txs_per_account_limit_count: AtomicU64, - retried_txs_per_account_data_limit_count: AtomicU64, } impl QosServiceMetrics { @@ -209,12 +204,6 @@ impl QosServiceMetrics { .swap(0, Ordering::Relaxed) as i64, i64 ), - ( - "retried_txs_per_account_data_limit_count", - self.retried_txs_per_account_data_limit_count - .swap(0, Ordering::Relaxed) as i64, - i64 - ), ); } } diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 1370d415e0..514a97fefc 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -1032,12 +1032,11 @@ impl Accounts { let keys: Vec<_> = txs .zip(results) .filter_map(|(tx, res)| match res { - Err(TransactionError::AccountInUse) - | Err(TransactionError::SanitizeFailure) - | Err(TransactionError::AccountLoadedTwice) - | Err(TransactionError::WouldExceedMaxBlockCostLimit) - | Err(TransactionError::WouldExceedMaxAccountCostLimit) - | Err(TransactionError::WouldExceedMaxAccountDataCostLimit) => None, + Err(TransactionError::AccountInUse) => None, + Err(TransactionError::SanitizeFailure) => None, + Err(TransactionError::AccountLoadedTwice) => None, + Err(TransactionError::WouldExceedMaxBlockCostLimit) => None, + Err(TransactionError::WouldExceedMaxAccountCostLimit) => None, _ => Some(tx.get_account_locks(demote_program_write_locks)), }) .collect(); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 5beef12b9a..36886dcf32 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -240,7 +240,7 @@ impl ExecuteTimings { } type BankStatusCache = StatusCache>; -#[frozen_abi(digest = "GcfJc94Hb3s7gzF7Uh4YxLSiQf1MvUtMmtU45BvinkVT")] +#[frozen_abi(digest = "32EjVUc6shHHVPpsnBAVfyBziMgyFzH8qxisLwmwwdS1")] pub type BankSlotDelta = SlotDelta>; // Eager rent collection repeats in cyclic manner. @@ -3525,8 +3525,7 @@ impl Bank { Some(index) } Err(TransactionError::WouldExceedMaxBlockCostLimit) - | Err(TransactionError::WouldExceedMaxAccountCostLimit) - | Err(TransactionError::WouldExceedMaxAccountDataCostLimit) => Some(index), + | Err(TransactionError::WouldExceedMaxAccountCostLimit) => Some(index), Err(_) => None, Ok(_) => None, }) diff --git a/runtime/src/block_cost_limits.rs b/runtime/src/block_cost_limits.rs index 4c6846d636..97be8a062d 100644 --- a/runtime/src/block_cost_limits.rs +++ b/runtime/src/block_cost_limits.rs @@ -58,6 +58,3 @@ pub const MAX_BLOCK_UNITS: u64 = /// limit is to prevent too many transactions write to same account, threrefore /// reduce block's paralellism. pub const MAX_WRITABLE_ACCOUNT_UNITS: u64 = MAX_BLOCK_REPLAY_TIME_US * COMPUTE_UNIT_TO_US_RATIO; - -/// max len of account data in a slot (bytes) -pub const MAX_ACCOUNT_DATA_LEN: u64 = 100_000_000; diff --git a/runtime/src/cost_model.rs b/runtime/src/cost_model.rs index ab74f9de58..a69b4c9f41 100644 --- a/runtime/src/cost_model.rs +++ b/runtime/src/cost_model.rs @@ -10,10 +10,7 @@ use { execute_cost_table::ExecuteCostTable, }, log::*, - solana_sdk::{ - instruction::CompiledInstruction, program_utils::limited_deserialize, pubkey::Pubkey, - system_instruction::SystemInstruction, system_program, transaction::SanitizedTransaction, - }, + solana_sdk::{pubkey::Pubkey, transaction::SanitizedTransaction}, std::collections::HashMap, }; @@ -30,7 +27,6 @@ pub struct TransactionCost { // `cost_weight` is a multiplier could be applied to transaction cost, // if set to zero allows the transaction to bypass cost limit check. pub cost_weight: u32, - pub account_data_size: u64, } impl Default for TransactionCost { @@ -42,7 +38,6 @@ impl Default for TransactionCost { data_bytes_cost: 0u64, execution_cost: 0u64, cost_weight: 1u32, - account_data_size: 0u64, } } } @@ -123,7 +118,6 @@ impl CostModel { tx_cost.data_bytes_cost = self.get_data_bytes_cost(transaction); tx_cost.execution_cost = self.get_transaction_cost(transaction); tx_cost.cost_weight = self.calculate_cost_weight(transaction); - tx_cost.account_data_size = self.calculate_account_data_size(transaction); debug!("transaction {:?} has cost {:?}", transaction, tx_cost); tx_cost @@ -207,59 +201,6 @@ impl CostModel { } } - fn calculate_account_data_size_on_deserialized_system_instruction( - instruction: SystemInstruction, - ) -> u64 { - match instruction { - SystemInstruction::CreateAccount { - lamports: _lamports, - space, - owner: _owner, - } => space, - SystemInstruction::CreateAccountWithSeed { - base: _base, - seed: _seed, - lamports: _lamports, - space, - owner: _owner, - } => space, - SystemInstruction::Allocate { space } => space, - SystemInstruction::AllocateWithSeed { - base: _base, - seed: _seed, - space, - owner: _owner, - } => space, - _ => 0, - } - } - - fn calculate_account_data_size_on_instruction( - program_id: &Pubkey, - instruction: &CompiledInstruction, - ) -> u64 { - if program_id == &system_program::id() { - if let Ok(instruction) = limited_deserialize(&instruction.data) { - return Self::calculate_account_data_size_on_deserialized_system_instruction( - instruction, - ); - } - } - 0 - } - - /// eventually, potentially determine account data size of all writable accounts - /// at the moment, calculate account data size of account creation - fn calculate_account_data_size(&self, transaction: &SanitizedTransaction) -> u64 { - transaction - .message() - .program_instructions_iter() - .map(|(program_id, instruction)| { - Self::calculate_account_data_size_on_instruction(program_id, instruction) - }) - .sum() - } - fn calculate_cost_weight(&self, transaction: &SanitizedTransaction) -> u32 { if is_simple_vote_transaction(transaction) { // vote has zero cost weight, so it bypasses block cost limit checking @@ -331,53 +272,6 @@ mod tests { ); } - #[test] - fn test_cost_model_data_len_cost() { - let lamports = 0; - let owner = Pubkey::default(); - let seed = String::default(); - let space = 100; - let base = Pubkey::default(); - for instruction in [ - SystemInstruction::CreateAccount { - lamports, - space, - owner, - }, - SystemInstruction::CreateAccountWithSeed { - base, - seed: seed.clone(), - lamports, - space, - owner, - }, - SystemInstruction::Allocate { space }, - SystemInstruction::AllocateWithSeed { - base, - seed, - space, - owner, - }, - ] { - assert_eq!( - space, - CostModel::calculate_account_data_size_on_deserialized_system_instruction( - instruction - ) - ); - } - assert_eq!( - 0, - CostModel::calculate_account_data_size_on_deserialized_system_instruction( - SystemInstruction::TransferWithSeed { - lamports, - from_seed: String::default(), - from_owner: Pubkey::default(), - } - ) - ); - } - #[test] fn test_cost_model_simple_transaction() { let (mint_keypair, start_hash) = test_setup(); diff --git a/runtime/src/cost_tracker.rs b/runtime/src/cost_tracker.rs index d682b4119d..4ad4525fe3 100644 --- a/runtime/src/cost_tracker.rs +++ b/runtime/src/cost_tracker.rs @@ -18,8 +18,6 @@ pub enum CostTrackerError { /// would exceed account max limit WouldExceedAccountMaxLimit, - - WouldExceedAccountDataMaxLimit, } #[derive(AbiExample, Debug)] @@ -29,7 +27,6 @@ pub struct CostTracker { cost_by_writable_accounts: HashMap, block_cost: u64, transaction_count: u64, - account_data_size: u64, } impl Default for CostTracker { @@ -47,7 +44,6 @@ impl CostTracker { cost_by_writable_accounts: HashMap::with_capacity(WRITABLE_ACCOUNTS_PER_BLOCK), block_cost: 0, transaction_count: 0, - account_data_size: 0, } } @@ -62,11 +58,7 @@ impl CostTracker { _transaction: &SanitizedTransaction, tx_cost: &TransactionCost, ) -> Result<(), CostTrackerError> { - self.would_fit( - &tx_cost.writable_accounts, - tx_cost.sum(), - tx_cost.account_data_size, - ) + self.would_fit(&tx_cost.writable_accounts, &tx_cost.sum()) } pub fn add_transaction_cost( @@ -74,11 +66,7 @@ impl CostTracker { _transaction: &SanitizedTransaction, tx_cost: &TransactionCost, ) { - self.add_transaction( - &tx_cost.writable_accounts, - tx_cost.sum(), - tx_cost.account_data_size, - ); + self.add_transaction(&tx_cost.writable_accounts, &tx_cost.sum()); } pub fn try_add( @@ -87,8 +75,8 @@ impl CostTracker { tx_cost: &TransactionCost, ) -> Result { let cost = tx_cost.sum() * tx_cost.cost_weight as u64; - self.would_fit(&tx_cost.writable_accounts, cost, tx_cost.account_data_size)?; - self.add_transaction(&tx_cost.writable_accounts, cost, tx_cost.account_data_size); + self.would_fit(&tx_cost.writable_accounts, &cost)?; + self.add_transaction(&tx_cost.writable_accounts, &cost); Ok(self.block_cost) } @@ -112,7 +100,6 @@ impl CostTracker { ), ("costliest_account", costliest_account.to_string(), String), ("costliest_account_cost", costliest_account_cost as i64, i64), - ("account_data_size", self.account_data_size, i64), ); } @@ -129,26 +116,17 @@ impl CostTracker { (costliest_account, costliest_account_cost) } - fn would_fit( - &self, - keys: &[Pubkey], - cost: u64, - account_data_len: u64, - ) -> Result<(), CostTrackerError> { + fn would_fit(&self, keys: &[Pubkey], cost: &u64) -> Result<(), CostTrackerError> { // check against the total package cost if self.block_cost + cost > self.block_cost_limit { return Err(CostTrackerError::WouldExceedBlockMaxLimit); } // check if the transaction itself is more costly than the account_cost_limit - if cost > self.account_cost_limit { + if *cost > self.account_cost_limit { return Err(CostTrackerError::WouldExceedAccountMaxLimit); } - if self.account_data_size.saturating_add(account_data_len) > MAX_ACCOUNT_DATA_LEN { - return Err(CostTrackerError::WouldExceedAccountDataMaxLimit); - } - // check each account against account_cost_limit, for account_key in keys.iter() { match self.cost_by_writable_accounts.get(account_key) { @@ -166,7 +144,7 @@ impl CostTracker { Ok(()) } - fn add_transaction(&mut self, keys: &[Pubkey], cost: u64, account_data_size: u64) { + fn add_transaction(&mut self, keys: &[Pubkey], cost: &u64) { for account_key in keys.iter() { *self .cost_by_writable_accounts @@ -175,7 +153,6 @@ impl CostTracker { } self.block_cost += cost; self.transaction_count += 1; - self.account_data_size = self.account_data_size.saturating_add(account_data_size); } } @@ -235,24 +212,11 @@ mod tests { // build testee to have capacity for one simple transaction let mut testee = CostTracker::new(cost, cost); - assert!(testee.would_fit(&keys, cost, 0).is_ok()); - testee.add_transaction(&keys, cost, 0); + assert!(testee.would_fit(&keys, &cost).is_ok()); + testee.add_transaction(&keys, &cost); assert_eq!(cost, testee.block_cost); } - #[test] - fn test_cost_tracker_add_data() { - let (mint_keypair, start_hash) = test_setup(); - let (_tx, keys, cost) = build_simple_transaction(&mint_keypair, &start_hash); - - // build testee to have capacity for one simple transaction - let mut testee = CostTracker::new(cost, cost); - assert!(testee.would_fit(&keys, cost, 0).is_ok()); - let old = testee.account_data_size; - testee.add_transaction(&keys, cost, 1); - assert_eq!(old + 1, testee.account_data_size); - } - #[test] fn test_cost_tracker_ok_add_two_same_accounts() { let (mint_keypair, start_hash) = test_setup(); @@ -263,12 +227,12 @@ mod tests { // build testee to have capacity for two simple transactions, with same accounts let mut testee = CostTracker::new(cost1 + cost2, cost1 + cost2); { - assert!(testee.would_fit(&keys1, cost1, 0).is_ok()); - testee.add_transaction(&keys1, cost1, 0); + assert!(testee.would_fit(&keys1, &cost1).is_ok()); + testee.add_transaction(&keys1, &cost1); } { - assert!(testee.would_fit(&keys2, cost2, 0).is_ok()); - testee.add_transaction(&keys2, cost2, 0); + assert!(testee.would_fit(&keys2, &cost2).is_ok()); + testee.add_transaction(&keys2, &cost2); } assert_eq!(cost1 + cost2, testee.block_cost); assert_eq!(1, testee.cost_by_writable_accounts.len()); @@ -285,12 +249,12 @@ mod tests { // build testee to have capacity for two simple transactions, with same accounts let mut testee = CostTracker::new(cmp::max(cost1, cost2), cost1 + cost2); { - assert!(testee.would_fit(&keys1, cost1, 0).is_ok()); - testee.add_transaction(&keys1, cost1, 0); + assert!(testee.would_fit(&keys1, &cost1).is_ok()); + testee.add_transaction(&keys1, &cost1); } { - assert!(testee.would_fit(&keys2, cost2, 0).is_ok()); - testee.add_transaction(&keys2, cost2, 0); + assert!(testee.would_fit(&keys2, &cost2).is_ok()); + testee.add_transaction(&keys2, &cost2); } assert_eq!(cost1 + cost2, testee.block_cost); assert_eq!(2, testee.cost_by_writable_accounts.len()); @@ -307,12 +271,12 @@ mod tests { let mut testee = CostTracker::new(cmp::min(cost1, cost2), cost1 + cost2); // should have room for first transaction { - assert!(testee.would_fit(&keys1, cost1, 0).is_ok()); - testee.add_transaction(&keys1, cost1, 0); + assert!(testee.would_fit(&keys1, &cost1).is_ok()); + testee.add_transaction(&keys1, &cost1); } // but no more sapce on the same chain (same signer account) { - assert!(testee.would_fit(&keys2, cost2, 0).is_err()); + assert!(testee.would_fit(&keys2, &cost2).is_err()); } } @@ -328,34 +292,15 @@ mod tests { let mut testee = CostTracker::new(cmp::max(cost1, cost2), cost1 + cost2 - 1); // should have room for first transaction { - assert!(testee.would_fit(&keys1, cost1, 0).is_ok()); - testee.add_transaction(&keys1, cost1, 0); + assert!(testee.would_fit(&keys1, &cost1).is_ok()); + testee.add_transaction(&keys1, &cost1); } // but no more room for package as whole { - assert!(testee.would_fit(&keys2, cost2, 0).is_err()); + assert!(testee.would_fit(&keys2, &cost2).is_err()); } } - #[test] - fn test_cost_tracker_reach_data_limit() { - let (mint_keypair, start_hash) = test_setup(); - // build two transactions with diff accounts - let (_tx1, _keys1, cost1) = build_simple_transaction(&mint_keypair, &start_hash); - let second_account = Keypair::new(); - let (_tx2, keys2, cost2) = build_simple_transaction(&second_account, &start_hash); - - // build testee that passes - let testee = CostTracker::new(cmp::max(cost1, cost2), cost1 + cost2 - 1); - assert!(testee - .would_fit(&keys2, cost2, MAX_ACCOUNT_DATA_LEN) - .is_ok()); - // data is too big - assert!(testee - .would_fit(&keys2, cost2, MAX_ACCOUNT_DATA_LEN + 1) - .is_err()); - } - #[test] fn test_cost_tracker_try_add_is_atomic() { let (mint_keypair, start_hash) = test_setup(); diff --git a/sdk/src/transaction/error.rs b/sdk/src/transaction/error.rs index 60ed8c39f5..acf064b9f4 100644 --- a/sdk/src/transaction/error.rs +++ b/sdk/src/transaction/error.rs @@ -101,10 +101,6 @@ pub enum TransactionError { /// Transaction would exceed max account limit within the block #[error("Transaction would exceed max account limit within the block")] WouldExceedMaxAccountCostLimit, - - /// Transaction would exceed max account data limit within the block - #[error("Transaction would exceed max account data limit within the block")] - WouldExceedMaxAccountDataCostLimit, } impl From for TransactionError { diff --git a/storage-proto/proto/transaction_by_addr.proto b/storage-proto/proto/transaction_by_addr.proto index d1a40cfd7c..36c17832ee 100644 --- a/storage-proto/proto/transaction_by_addr.proto +++ b/storage-proto/proto/transaction_by_addr.proto @@ -45,7 +45,6 @@ enum TransactionErrorType { UNSUPPORTED_VERSION = 18; INVALID_WRITABLE_ACCOUNT = 19; WOULD_EXCEED_MAX_ACCOUNT_COST_LIMIT = 20; - WOULD_EXCEED_MAX_ACCOUNT_DATA_COST_LIMIT = 21; } message InstructionError { diff --git a/storage-proto/src/convert.rs b/storage-proto/src/convert.rs index 636dcc3f9d..e8511867c6 100644 --- a/storage-proto/src/convert.rs +++ b/storage-proto/src/convert.rs @@ -567,7 +567,6 @@ impl TryFrom for TransactionError { 18 => TransactionError::UnsupportedVersion, 19 => TransactionError::InvalidWritableAccount, 20 => TransactionError::WouldExceedMaxAccountCostLimit, - 21 => TransactionError::WouldExceedMaxAccountDataCostLimit, _ => return Err("Invalid TransactionError"), }) } @@ -638,9 +637,6 @@ impl From for tx_by_addr::TransactionError { TransactionError::WouldExceedMaxAccountCostLimit => { tx_by_addr::TransactionErrorType::WouldExceedMaxAccountCostLimit } - TransactionError::WouldExceedMaxAccountDataCostLimit => { - tx_by_addr::TransactionErrorType::WouldExceedMaxAccountDataCostLimit - } } as i32, instruction_error: match transaction_error { TransactionError::InstructionError(index, ref instruction_error) => {