From dc9a9238d54a01ed9f487249800cc267a41cffbf Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Mon, 8 Aug 2022 11:05:25 -0400 Subject: [PATCH] Do not check accounts data size in InvokeContext (#26773) --- program-runtime/src/accounts_data_meter.rs | 131 +-------------------- program-runtime/src/invoke_context.rs | 55 +++------ 2 files changed, 20 insertions(+), 166 deletions(-) diff --git a/program-runtime/src/accounts_data_meter.rs b/program-runtime/src/accounts_data_meter.rs index 244ce9b11..3e0553d1e 100644 --- a/program-runtime/src/accounts_data_meter.rs +++ b/program-runtime/src/accounts_data_meter.rs @@ -1,7 +1,6 @@ //! The accounts data space has a maximum size it is permitted to grow to. This module contains //! the constants and types for tracking and metering the accounts data space during program //! runtime. -use solana_sdk::instruction::InstructionError; /// The maximum allowed size, in bytes, of the accounts data /// 128 GB was chosen because it is the RAM amount listed under Hardware Recommendations on @@ -12,9 +11,6 @@ pub const MAX_ACCOUNTS_DATA_LEN: u64 = 128_000_000_000; /// Meter and track the amount of available accounts data space #[derive(Debug, Default, Clone, Copy, Eq, PartialEq)] pub struct AccountsDataMeter { - /// The maximum amount of accounts data space that can be used (in bytes) - maximum: u64, - /// The initial amount of accounts data space used (in bytes) initial: u64, @@ -26,18 +22,10 @@ impl AccountsDataMeter { /// Make a new AccountsDataMeter #[must_use] pub fn new(initial_accounts_data_len: u64) -> Self { - let accounts_data_meter = Self { - maximum: MAX_ACCOUNTS_DATA_LEN, + Self { initial: initial_accounts_data_len, delta: 0, - }; - debug_assert!(accounts_data_meter.initial <= accounts_data_meter.maximum); - accounts_data_meter - } - - /// Return the maximum amount of accounts data space that can be used (in bytes) - pub fn maximum(&self) -> u64 { - self.maximum + } } /// Return the initial amount of accounts data space used (in bytes) @@ -68,41 +56,12 @@ impl AccountsDataMeter { saturating_add_signed(self.initial, self.delta) } - /// Get the remaining amount of accounts data space (in bytes) - pub fn remaining(&self) -> u64 { - self.maximum.saturating_sub(self.current()) - } - /// Adjust the space used by accounts data by `amount` (in bytes). - /// - /// If `amount` is *positive*, we *increase* the space used by accounts data. If `amount` is - /// *negative*, we *decrease* the space used by accounts data. If `amount` is greater than - /// the remaining space, return an error and *do not* adjust accounts data space. - pub fn adjust_delta(&mut self, amount: i64) -> Result<(), InstructionError> { - if amount > self.remaining() as i64 { - return Err(InstructionError::MaxAccountsDataSizeExceeded); - } - self.adjust_delta_unchecked(amount); - Ok(()) - } - - /// Unconditionally adjust accounts data space. Refer to `adjust_delta()` for more - /// documentation. pub fn adjust_delta_unchecked(&mut self, amount: i64) { self.delta = self.delta.saturating_add(amount); } } -#[cfg(test)] -impl AccountsDataMeter { - pub fn set_maximum(&mut self, maximum: u64) { - self.maximum = maximum; - } - pub fn set_initial(&mut self, initial: u64) { - self.initial = initial; - } -} - #[cfg(test)] mod tests { use super::*; @@ -111,7 +70,6 @@ mod tests { fn test_new() { let initial = 1234; let accounts_data_meter = AccountsDataMeter::new(initial); - assert_eq!(accounts_data_meter.maximum, MAX_ACCOUNTS_DATA_LEN); assert_eq!(accounts_data_meter.initial, initial); } @@ -119,89 +77,4 @@ mod tests { fn test_new_can_use_max_len() { let _ = AccountsDataMeter::new(MAX_ACCOUNTS_DATA_LEN); } - - #[test] - #[should_panic] - fn test_new_panics_if_initial_len_too_big() { - let _ = AccountsDataMeter::new(MAX_ACCOUNTS_DATA_LEN + 1); - } - - #[test] - fn test_remaining() { - let initial_accounts_data_len = 0; - let accounts_data_meter = AccountsDataMeter::new(initial_accounts_data_len); - assert_eq!(accounts_data_meter.remaining(), MAX_ACCOUNTS_DATA_LEN); - } - - #[test] - fn test_remaining_saturates() { - let initial_accounts_data_len = 0; - let mut accounts_data_meter = AccountsDataMeter::new(initial_accounts_data_len); - // To test that remaining() saturates, need to break the invariant that initial <= maximum - accounts_data_meter.initial = MAX_ACCOUNTS_DATA_LEN + 1; - assert_eq!(accounts_data_meter.remaining(), 0); - } - - #[test] - fn test_adjust_delta() { - let initial_accounts_data_len = 0; - let mut accounts_data_meter = AccountsDataMeter::new(initial_accounts_data_len); - - // Test: simple, positive numbers - let result = accounts_data_meter.adjust_delta(0); - assert!(result.is_ok()); - let result = accounts_data_meter.adjust_delta(1); - assert!(result.is_ok()); - let result = accounts_data_meter.adjust_delta(4); - assert!(result.is_ok()); - let result = accounts_data_meter.adjust_delta(9); - assert!(result.is_ok()); - - // Test: adjust_delta can use up the remaining amount - let remaining = accounts_data_meter.remaining() as i64; - let result = accounts_data_meter.adjust_delta(remaining); - assert!(result.is_ok()); - assert_eq!(accounts_data_meter.remaining(), 0); - } - - #[test] - fn test_adjust_delta_deallocate() { - let initial_accounts_data_len = 10_000; - let mut accounts_data_meter = AccountsDataMeter::new(initial_accounts_data_len); - let remaining_before = accounts_data_meter.remaining(); - - let amount = (initial_accounts_data_len / 2) as i64; - let amount = -amount; - let result = accounts_data_meter.adjust_delta(amount); - assert!(result.is_ok()); - let remaining_after = accounts_data_meter.remaining(); - assert_eq!(remaining_after, remaining_before + amount.unsigned_abs()); - } - - #[test] - fn test_adjust_delta_exceeding() { - let initial_accounts_data_len = 0; - let mut accounts_data_meter = AccountsDataMeter::new(initial_accounts_data_len); - - // Test: adjusting delta by more than what's available - // (1) returns an error, - // (2) does not adjust delta - let remaining = accounts_data_meter.remaining(); - let result = accounts_data_meter.adjust_delta(remaining as i64 + 1); - assert!(result.is_err()); - assert_eq!(accounts_data_meter.remaining(), remaining); - } - - #[test] - fn test_adjust_delta_zero() { - // Pre-condition: set up the accounts data meter such that there is no remaining space - let initial_accounts_data_len = 1234; - let mut accounts_data_meter = AccountsDataMeter::new(initial_accounts_data_len); - accounts_data_meter.maximum = initial_accounts_data_len; - assert_eq!(accounts_data_meter.remaining(), 0); - - // Test: can always adjust delta by zero, even if there is no remaining space - let result = accounts_data_meter.adjust_delta(0); - assert!(result.is_ok()); - } } diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index 52c860bb6..a827cc51d 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -15,9 +15,7 @@ use { solana_sdk::{ account::{AccountSharedData, ReadableAccount}, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, - feature_set::{ - cap_accounts_data_len, enable_early_verification_of_account_modifications, FeatureSet, - }, + feature_set::{enable_early_verification_of_account_modifications, FeatureSet}, hash::Hash, instruction::{AccountMeta, Instruction, InstructionError}, native_loader, @@ -448,7 +446,6 @@ impl<'a> InvokeContext<'a> { instruction_accounts: &[InstructionAccount], program_indices: &[usize], ) -> Result<(), InstructionError> { - let cap_accounts_data_len = self.feature_set.is_active(&cap_accounts_data_len::id()); let instruction_context = self .transaction_context .get_current_instruction_context() @@ -519,12 +516,8 @@ impl<'a> InvokeContext<'a> { let pre_data_len = pre_account.data().len() as i64; let post_data_len = account.data().len() as i64; let data_len_delta = post_data_len.saturating_sub(pre_data_len); - if cap_accounts_data_len { - self.accounts_data_meter.adjust_delta(data_len_delta)?; - } else { - self.accounts_data_meter - .adjust_delta_unchecked(data_len_delta); - } + self.accounts_data_meter + .adjust_delta_unchecked(data_len_delta); } // Verify that the total sum of all the lamports did not change @@ -543,7 +536,6 @@ impl<'a> InvokeContext<'a> { instruction_accounts: &[InstructionAccount], before_instruction_context_push: bool, ) -> Result<(), InstructionError> { - let cap_accounts_data_len = self.feature_set.is_active(&cap_accounts_data_len::id()); let transaction_context = &self.transaction_context; let instruction_context = transaction_context.get_current_instruction_context()?; let program_id = instruction_context @@ -612,12 +604,8 @@ impl<'a> InvokeContext<'a> { let pre_data_len = pre_account.data().len() as i64; let post_data_len = account.data().len() as i64; let data_len_delta = post_data_len.saturating_sub(pre_data_len); - if cap_accounts_data_len { - self.accounts_data_meter.adjust_delta(data_len_delta)?; - } else { - self.accounts_data_meter - .adjust_delta_unchecked(data_len_delta); - } + self.accounts_data_meter + .adjust_delta_unchecked(data_len_delta); break; } @@ -1519,9 +1507,7 @@ mod tests { } #[test] - fn test_process_instruction_accounts_data_meter() { - solana_logger::setup(); - + fn test_process_instruction_accounts_resize_delta() { let program_key = Pubkey::new_unique(); let user_account_data_len = 123u64; let user_account = @@ -1545,14 +1531,6 @@ mod tests { let mut invoke_context = InvokeContext::new_mock(&mut transaction_context, &builtin_programs); - invoke_context - .accounts_data_meter - .set_initial(user_account_data_len as u64); - invoke_context - .accounts_data_meter - .set_maximum(user_account_data_len as u64 * 3); - let remaining_account_data_len = invoke_context.accounts_data_meter.remaining(); - let instruction_accounts = [ InstructionAccount { index_in_transaction: 0, @@ -1570,9 +1548,10 @@ mod tests { }, ]; - // Test 1: Resize the account to use up all the space; this must succeed + // Test: Resize the account to *the same size*, so not consuming any additional size; this must succeed { - let new_len = user_account_data_len + remaining_account_data_len; + let resize_delta: i64 = 0; + let new_len = (user_account_data_len as i64 + resize_delta) as u64; let instruction_data = bincode::serialize(&MockInstruction::Resize { new_len }).unwrap(); @@ -1590,13 +1569,14 @@ mod tests { .transaction_context .accounts_resize_delta() .unwrap(), - user_account_data_len as i64 * 2 + resize_delta ); } - // Test 2: Resize the account to *the same size*, so not consuming any additional size; this must succeed + // Test: Resize the account larger; this must succeed { - let new_len = user_account_data_len + remaining_account_data_len; + let resize_delta: i64 = 1; + let new_len = (user_account_data_len as i64 + resize_delta) as u64; let instruction_data = bincode::serialize(&MockInstruction::Resize { new_len }).unwrap(); @@ -1614,13 +1594,14 @@ mod tests { .transaction_context .accounts_resize_delta() .unwrap(), - user_account_data_len as i64 * 2 + resize_delta ); } - // Test 3: Resize the account to exceed the budget; this must succeed + // Test: Resize the account smaller; this must succeed { - let new_len = user_account_data_len + remaining_account_data_len + 1; + let resize_delta: i64 = -1; + let new_len = (user_account_data_len as i64 + resize_delta) as u64; let instruction_data = bincode::serialize(&MockInstruction::Resize { new_len }).unwrap(); @@ -1638,7 +1619,7 @@ mod tests { .transaction_context .accounts_resize_delta() .unwrap(), - user_account_data_len as i64 * 2 + 1 + resize_delta ); } }