From f75e26d992dada5e3167995c53170942461c8cb9 Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Wed, 15 Dec 2021 17:02:45 -0600 Subject: [PATCH 1/8] Add AccountsDataMeter to InvokeContext --- program-runtime/src/invoke_context.rs | 57 +++++++++++++++++++ runtime/src/bank.rs | 3 +- runtime/src/message_processor.rs | 13 ++++- sdk/program/src/instruction.rs | 4 ++ sdk/program/src/program_error.rs | 8 +++ storage-proto/proto/transaction_by_addr.proto | 1 + storage-proto/src/convert.rs | 4 ++ 7 files changed, 88 insertions(+), 2 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index dc40eb3254c867..22351237743a91 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -23,6 +23,8 @@ use { }, std::{cell::RefCell, collections::HashMap, fmt::Debug, rc::Rc, sync::Arc}, }; +/// The maximum allowed size, in bytes, of the accounts data +pub const MAX_ACCOUNTS_DATA_LEN: u64 = 128_000_000_000; // 128 GB pub type TransactionAccountRefCell = (Pubkey, RefCell); pub type TransactionAccountRefCells = Vec; @@ -117,6 +119,46 @@ impl ComputeMeter { } } +/// Meter and track the amount of space available in the accounts data +#[derive(Debug, Default, Clone, Copy, Eq, PartialEq)] +pub struct AccountsDataMeter { + /// The amount of available space in the accounts data (i.e. max - current) + capacity: u64, + /// The amount of available space *consumed* from the accounts data. This value is used to + /// update the Bank after transactions are successfully processed. This value may be negative, + /// which indicates that the accounts data has shrunk. + consumed: i64, +} +impl AccountsDataMeter { + pub fn new_ref(capacity: u64) -> Rc> { + Rc::new(RefCell::new(Self { + capacity, + consumed: 0, + })) + } + pub fn capacity(&self) -> u64 { + self.capacity + } + pub fn consumed(&self) -> i64 { + self.consumed + } + pub fn remaining(&self) -> i64 { + let capacity = self.capacity() as i64; + let consumed = self.consumed(); + capacity.saturating_sub(consumed) + } + pub fn consume(&mut self, amount: i64) -> Result<(), InstructionError> { + if amount.is_positive() && amount > self.remaining() { + return Err(InstructionError::AccountsDataBudgetExceeded); + } + // SAFETY: We know that the consumed amount can never exceed MAX_ACCOUNTS_DATA_LEN (which + // is far below i64::MAX), and therefore we can never free/deallocate more than + // MAX_ACCOUNTS_DATA_LEN either, so we can be sure wrapping will not happen at either end. + self.consumed = self.consumed.wrapping_add(amount); + Ok(()) + } +} + pub struct StackFrame<'a> { pub number_of_program_accounts: usize, pub keyed_accounts: Vec>, @@ -154,6 +196,7 @@ pub struct InvokeContext<'a> { compute_budget: ComputeBudget, current_compute_budget: ComputeBudget, compute_meter: Rc>, + accounts_data_meter: Rc>, executors: Rc>, pub instruction_recorder: Option>>, pub feature_set: Arc, @@ -177,7 +220,14 @@ impl<'a> InvokeContext<'a> { feature_set: Arc, blockhash: Hash, lamports_per_signature: u64, + current_accounts_data_len: u64, ) -> Self { + let accounts_data_meter = { + debug_assert!(current_accounts_data_len <= MAX_ACCOUNTS_DATA_LEN); + AccountsDataMeter::new_ref( + MAX_ACCOUNTS_DATA_LEN.saturating_sub(current_accounts_data_len), + ) + }; Self { invoke_stack: Vec::with_capacity(compute_budget.max_invoke_depth), rent, @@ -189,6 +239,7 @@ impl<'a> InvokeContext<'a> { current_compute_budget: compute_budget, compute_budget, compute_meter: ComputeMeter::new_ref(compute_budget.max_units), + accounts_data_meter, executors, instruction_recorder, feature_set, @@ -215,6 +266,7 @@ impl<'a> InvokeContext<'a> { Arc::new(FeatureSet::all_enabled()), Hash::default(), 0, + 0, ) } @@ -841,6 +893,11 @@ impl<'a> InvokeContext<'a> { self.compute_meter.clone() } + /// Get this invocation's AccountsDataMeter + pub fn get_accounts_data_meter(&self) -> Rc> { + Rc::clone(&self.accounts_data_meter) + } + /// Loaders may need to do work in order to execute a program. Cache /// the work that can be re-used across executions pub fn add_executor(&self, pubkey: &Pubkey, executor: Arc) { diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 4d5a059e834fcb..316572e74c348e 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 = "2pPboTQ9ixNuR1hvRt7McJriam5EHfd3vpBWfxnVbmF3")] pub type BankSlotDelta = SlotDelta>; // Eager rent collection repeats in cyclic manner. @@ -3609,6 +3609,7 @@ impl Bank { &*self.sysvar_cache.read().unwrap(), blockhash, lamports_per_signature, + self.accounts_data_len.load(Acquire), ) .map(|process_result| { self.update_accounts_data_len( diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 35c97923b78a24..e1e6b90578b8aa 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -65,6 +65,7 @@ impl MessageProcessor { sysvars: &[(Pubkey, Vec)], blockhash: Hash, lamports_per_signature: u64, + current_accounts_data_len: u64, ) -> Result { let mut invoke_context = InvokeContext::new( rent, @@ -78,6 +79,7 @@ impl MessageProcessor { feature_set, blockhash, lamports_per_signature, + current_accounts_data_len, ); debug_assert_eq!(program_indices.len(), message.instructions.len()); @@ -139,7 +141,9 @@ impl MessageProcessor { ); timings.accumulate(&invoke_context.timings); } - Ok(ProcessedMessageInfo::default()) + Ok(ProcessedMessageInfo { + accounts_data_len_delta: invoke_context.get_accounts_data_meter().take().consumed(), + }) } } @@ -264,6 +268,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert!(result.is_ok()); assert_eq!(accounts[0].1.borrow().lamports(), 100); @@ -293,6 +298,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert_eq!( result, @@ -326,6 +332,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert_eq!( result, @@ -470,6 +477,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert_eq!( result, @@ -503,6 +511,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert!(result.is_ok()); @@ -533,6 +542,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert!(result.is_ok()); assert_eq!(accounts[0].1.borrow().lamports(), 80); @@ -590,6 +600,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert_eq!( result, diff --git a/sdk/program/src/instruction.rs b/sdk/program/src/instruction.rs index c8be38bef511b3..e1fbb1825a845a 100644 --- a/sdk/program/src/instruction.rs +++ b/sdk/program/src/instruction.rs @@ -248,6 +248,10 @@ pub enum InstructionError { /// Illegal account owner #[error("Provided owner is not allowed")] IllegalOwner, + + /// Accounts data budget exceeded + #[error("Requested account data allocation exceeded the accounts data budget")] + AccountsDataBudgetExceeded, // Note: For any new error added here an equivalent ProgramError and its // conversions must also be added } diff --git a/sdk/program/src/program_error.rs b/sdk/program/src/program_error.rs index cc0f18f18667ef..c47638dddb29b9 100644 --- a/sdk/program/src/program_error.rs +++ b/sdk/program/src/program_error.rs @@ -49,6 +49,8 @@ pub enum ProgramError { UnsupportedSysvar, #[error("Provided owner is not allowed")] IllegalOwner, + #[error("Requested account data allocation exceeded the accounts data budget")] + AccountsDataBudgetExceeded, } pub trait PrintProgramError { @@ -87,6 +89,7 @@ impl PrintProgramError for ProgramError { Self::AccountNotRentExempt => msg!("Error: AccountNotRentExempt"), Self::UnsupportedSysvar => msg!("Error: UnsupportedSysvar"), Self::IllegalOwner => msg!("Error: IllegalOwner"), + Self::AccountsDataBudgetExceeded => msg!("Error: AccountsDataBudgetExceeded"), } } } @@ -117,6 +120,7 @@ pub const BORSH_IO_ERROR: u64 = to_builtin!(15); pub const ACCOUNT_NOT_RENT_EXEMPT: u64 = to_builtin!(16); pub const UNSUPPORTED_SYSVAR: u64 = to_builtin!(17); pub const ILLEGAL_OWNER: u64 = to_builtin!(18); +pub const ACCOUNTS_DATA_BUDGET_EXCEEDED: u64 = to_builtin!(19); // Warning: Any new program errors added here must also be: // - Added to the below conversions // - Added as an equivilent to InstructionError @@ -143,6 +147,7 @@ impl From for u64 { ProgramError::AccountNotRentExempt => ACCOUNT_NOT_RENT_EXEMPT, ProgramError::UnsupportedSysvar => UNSUPPORTED_SYSVAR, ProgramError::IllegalOwner => ILLEGAL_OWNER, + ProgramError::AccountsDataBudgetExceeded => ACCOUNTS_DATA_BUDGET_EXCEEDED, ProgramError::Custom(error) => { if error == 0 { CUSTOM_ZERO @@ -175,6 +180,7 @@ impl From for ProgramError { ACCOUNT_NOT_RENT_EXEMPT => Self::AccountNotRentExempt, UNSUPPORTED_SYSVAR => Self::UnsupportedSysvar, ILLEGAL_OWNER => Self::IllegalOwner, + ACCOUNTS_DATA_BUDGET_EXCEEDED => Self::AccountsDataBudgetExceeded, _ => Self::Custom(error as u32), } } @@ -203,6 +209,7 @@ impl TryFrom for ProgramError { Self::Error::AccountNotRentExempt => Ok(Self::AccountNotRentExempt), Self::Error::UnsupportedSysvar => Ok(Self::UnsupportedSysvar), Self::Error::IllegalOwner => Ok(Self::IllegalOwner), + Self::Error::AccountsDataBudgetExceeded => Ok(Self::AccountsDataBudgetExceeded), _ => Err(error), } } @@ -233,6 +240,7 @@ where ACCOUNT_NOT_RENT_EXEMPT => Self::AccountNotRentExempt, UNSUPPORTED_SYSVAR => Self::UnsupportedSysvar, ILLEGAL_OWNER => Self::IllegalOwner, + ACCOUNTS_DATA_BUDGET_EXCEEDED => Self::AccountsDataBudgetExceeded, _ => { // A valid custom error has no bits set in the upper 32 if error >> BUILTIN_BIT_SHIFT == 0 { diff --git a/storage-proto/proto/transaction_by_addr.proto b/storage-proto/proto/transaction_by_addr.proto index d1a40cfd7cc5f6..582c31991f3b4a 100644 --- a/storage-proto/proto/transaction_by_addr.proto +++ b/storage-proto/proto/transaction_by_addr.proto @@ -105,6 +105,7 @@ enum InstructionErrorType { ARITHMETIC_OVERFLOW = 47; UNSUPPORTED_SYSVAR = 48; ILLEGAL_OWNER = 49; + ACCOUNTS_DATA_BUDGET_EXCEEDED = 50; } message UnixTimestamp { diff --git a/storage-proto/src/convert.rs b/storage-proto/src/convert.rs index 636dcc3f9dea44..423732196010a5 100644 --- a/storage-proto/src/convert.rs +++ b/storage-proto/src/convert.rs @@ -536,6 +536,7 @@ impl TryFrom for TransactionError { 47 => InstructionError::ArithmeticOverflow, 48 => InstructionError::UnsupportedSysvar, 49 => InstructionError::IllegalOwner, + 50 => InstructionError::AccountsDataBudgetExceeded, _ => return Err("Invalid InstructionError"), }; @@ -795,6 +796,9 @@ impl From for tx_by_addr::TransactionError { InstructionError::IllegalOwner => { tx_by_addr::InstructionErrorType::IllegalOwner } + InstructionError::AccountsDataBudgetExceeded => { + tx_by_addr::InstructionErrorType::AccountsDataBudgetExceeded + } } as i32, custom: match instruction_error { InstructionError::Custom(custom) => { From b98418cb5e95e0f87bb655ad393344ec6c133e03 Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Mon, 20 Dec 2021 15:44:20 -0600 Subject: [PATCH 2/8] Tweak the API of the AccountsDataMeter to hold max/cur --- program-runtime/src/invoke_context.rs | 71 ++++++++++++++------------- runtime/src/bank.rs | 29 +++++------ runtime/src/message_processor.rs | 6 +-- 3 files changed, 52 insertions(+), 54 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index 22351237743a91..a00a1b484b62bd 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -119,42 +119,51 @@ impl ComputeMeter { } } -/// Meter and track the amount of space available in the accounts data +/// Meter and track the amount of available accounts data space #[derive(Debug, Default, Clone, Copy, Eq, PartialEq)] pub struct AccountsDataMeter { - /// The amount of available space in the accounts data (i.e. max - current) - capacity: u64, - /// The amount of available space *consumed* from the accounts data. This value is used to - /// update the Bank after transactions are successfully processed. This value may be negative, - /// which indicates that the accounts data has shrunk. - consumed: i64, + /// The maximum amount of accounts data space that can be used (in bytes) + maximum: u64, + /// The current amount of accounts data space used (in bytes) + current: u64, } impl AccountsDataMeter { - pub fn new_ref(capacity: u64) -> Rc> { - Rc::new(RefCell::new(Self { - capacity, - consumed: 0, - })) - } - pub fn capacity(&self) -> u64 { - self.capacity + /// Make a new AccountsDataMeter, wrapped in an Rc & RefCell + pub fn new_ref(current_accounts_data_len: u64) -> Rc> { + let accounts_data_meter = Self { + maximum: MAX_ACCOUNTS_DATA_LEN, + current: current_accounts_data_len, + }; + debug_assert!(accounts_data_meter.current <= accounts_data_meter.maximum); + Rc::new(RefCell::new(accounts_data_meter)) } - pub fn consumed(&self) -> i64 { - self.consumed + /// Consume the AccountsDataMeter and return the final (i.e. current) accounts data len + pub fn finalize(self) -> u64 { + self.current } - pub fn remaining(&self) -> i64 { - let capacity = self.capacity() as i64; - let consumed = self.consumed(); - capacity.saturating_sub(consumed) + /// Get the remaining amount of accounts data space + pub fn remaining(&self) -> u64 { + self.maximum.saturating_sub(self.current) } + /// Consume accounts data space. If `amount` is positive, we are *increasing* the amount of + /// accounts data space used. If `amount` is negative, we are *decreasing* the amount of + /// accounts data space used. pub fn consume(&mut self, amount: i64) -> Result<(), InstructionError> { - if amount.is_positive() && amount > self.remaining() { - return Err(InstructionError::AccountsDataBudgetExceeded); + if amount == 0 { + // nothing to do here; lets us skip doing unnecessary work in the 'else' case + return Ok(()); + } + + if amount.is_positive() { + let amount = amount as u64; + if amount > self.remaining() { + return Err(InstructionError::AccountsDataBudgetExceeded); + } + self.current = self.current.saturating_add(amount); + } else { + let amount = amount.abs() as u64; + self.current = self.current.saturating_sub(amount); } - // SAFETY: We know that the consumed amount can never exceed MAX_ACCOUNTS_DATA_LEN (which - // is far below i64::MAX), and therefore we can never free/deallocate more than - // MAX_ACCOUNTS_DATA_LEN either, so we can be sure wrapping will not happen at either end. - self.consumed = self.consumed.wrapping_add(amount); Ok(()) } } @@ -222,12 +231,6 @@ impl<'a> InvokeContext<'a> { lamports_per_signature: u64, current_accounts_data_len: u64, ) -> Self { - let accounts_data_meter = { - debug_assert!(current_accounts_data_len <= MAX_ACCOUNTS_DATA_LEN); - AccountsDataMeter::new_ref( - MAX_ACCOUNTS_DATA_LEN.saturating_sub(current_accounts_data_len), - ) - }; Self { invoke_stack: Vec::with_capacity(compute_budget.max_invoke_depth), rent, @@ -239,7 +242,7 @@ impl<'a> InvokeContext<'a> { current_compute_budget: compute_budget, compute_budget, compute_meter: ComputeMeter::new_ref(compute_budget.max_units), - accounts_data_meter, + accounts_data_meter: AccountsDataMeter::new_ref(current_accounts_data_len), executors, instruction_recorder, feature_set, diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 316572e74c348e..15ef06e9b4c5f9 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -148,7 +148,7 @@ use { sync::{ atomic::{ AtomicBool, AtomicU64, - Ordering::{AcqRel, Acquire, Relaxed, Release}, + Ordering::{Acquire, Relaxed, Release}, }, Arc, LockResult, RwLock, RwLockReadGuard, RwLockWriteGuard, }, @@ -1194,8 +1194,7 @@ impl Bank { }; let total_accounts_stats = bank.get_total_accounts_stats().unwrap(); - bank.accounts_data_len - .store(total_accounts_stats.data_len as u64, Release); + bank.store_accounts_data_len(total_accounts_stats.data_len as u64); bank } @@ -1443,7 +1442,7 @@ impl Bank { freeze_started: AtomicBool::new(false), cost_tracker: RwLock::new(CostTracker::default()), sysvar_cache: RwLock::new(Vec::new()), - accounts_data_len: AtomicU64::new(parent.accounts_data_len.load(Acquire)), + accounts_data_len: AtomicU64::new(parent.load_accounts_data_len()), }; let mut ancestors = Vec::with_capacity(1 + new.parents().len()); @@ -3612,9 +3611,7 @@ impl Bank { self.accounts_data_len.load(Acquire), ) .map(|process_result| { - self.update_accounts_data_len( - process_result.accounts_data_len_delta, - ) + self.store_accounts_data_len(process_result.accounts_data_len) }); } else { // TODO: support versioned messages @@ -3774,16 +3771,14 @@ impl Bank { ) } - /// Update the bank's accounts_data_len field based on the `delta`. - fn update_accounts_data_len(&self, delta: i64) { - if delta == 0 { - return; - } - if delta > 0 { - self.accounts_data_len.fetch_add(delta as u64, AcqRel); - } else { - self.accounts_data_len.fetch_sub(delta.abs() as u64, AcqRel); - } + /// Load the accounts data len + fn load_accounts_data_len(&self) -> u64 { + self.accounts_data_len.load(Acquire) + } + + /// Store a new value to the accounts data len + fn store_accounts_data_len(&self, accounts_data_len: u64) { + self.accounts_data_len.store(accounts_data_len, Release) } /// Calculate fee for `SanitizedMessage` diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index e1e6b90578b8aa..8ba2e4ce0e250b 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -39,8 +39,8 @@ impl ::solana_frozen_abi::abi_example::AbiExample for MessageProcessor { /// Resultant information gathered from calling process_message() #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] pub struct ProcessedMessageInfo { - /// The amount that the accounts data len has changed - pub accounts_data_len_delta: i64, + /// The new accounts data len + pub accounts_data_len: u64, } impl MessageProcessor { @@ -142,7 +142,7 @@ impl MessageProcessor { timings.accumulate(&invoke_context.timings); } Ok(ProcessedMessageInfo { - accounts_data_len_delta: invoke_context.get_accounts_data_meter().take().consumed(), + accounts_data_len: invoke_context.get_accounts_data_meter().take().finalize(), }) } } From cd82a700229aa0e541c5ce7fcd64a8ca58954f41 Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Wed, 15 Dec 2021 17:06:24 -0600 Subject: [PATCH 3/8] Add feature gate for capping accounts data len --- sdk/src/feature_set.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 4c37b319fea508..3b85a49fd84766 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -283,6 +283,10 @@ pub mod reject_all_elf_rw { solana_sdk::declare_id!("DeMpxgMq51j3rZfNK2hQKZyXknQvqevPSFPJFNTbXxsS"); } +pub mod cap_accounts_data_len { + solana_sdk::declare_id!("capRxUrBjNkkCpjrJxPGfPaWijB7q3JoDfsWXAnt46r"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -348,6 +352,7 @@ lazy_static! { (evict_invalid_stakes_cache_entries::id(), "evict invalid stakes cache entries on epoch boundaries"), (allow_votes_to_directly_update_vote_state::id(), "enable direct vote state update"), (reject_all_elf_rw::id(), "reject all read-write data in program elfs"), + (cap_accounts_data_len::id(), "cap the accounts data len"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() From 1326aabb2c220c6b56187a2d8fa87ce8f845b3e7 Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Wed, 22 Dec 2021 13:11:09 -0600 Subject: [PATCH 4/8] pr: map invalid error --- programs/bpf_loader/src/lib.rs | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index c41153ba1e83a2..e00d5674d88b63 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -38,8 +38,8 @@ use { clock::Clock, entrypoint::{HEAP_LENGTH, SUCCESS}, feature_set::{ - do_support_realloc, reduce_required_deploy_balance, reject_all_elf_rw, - reject_deployment_of_unresolved_syscalls, + cap_accounts_data_len, do_support_realloc, reduce_required_deploy_balance, + reject_all_elf_rw, reject_deployment_of_unresolved_syscalls, reject_section_virtual_address_file_offset_mismatch, requestable_heap_size, start_verify_shift32_imm, stop_verify_mul64_imm_nonzero, }, @@ -47,6 +47,7 @@ use { keyed_account::{from_keyed_account, keyed_account_at_index, KeyedAccount}, loader_instruction::LoaderInstruction, loader_upgradeable_instruction::UpgradeableLoaderInstruction, + program_error::ACCOUNTS_DATA_BUDGET_EXCEEDED, program_utils::limited_deserialize, pubkey::Pubkey, rent::Rent, @@ -1040,10 +1041,24 @@ impl Executor for BpfExecutor { stable_log::program_return(&log_collector, &program_id, return_data); } match result { - Ok(status) if status != SUCCESS => { - let error: InstructionError = status.into(); - stable_log::program_failure(&log_collector, &program_id, &error); - Err(error) + Ok(status) => { + if status == SUCCESS { + Ok(()) + } else { + let error: InstructionError = if status == ACCOUNTS_DATA_BUDGET_EXCEEDED + && !invoke_context + .feature_set + .is_active(&cap_accounts_data_len::id()) + { + // Until the cap_accounts_data_len feature is enabled, map the + // ACCOUNTS_DATA_BUDGET_EXCEEDED error to InvalidError + InstructionError::InvalidError + } else { + status.into() + }; + stable_log::program_failure(&log_collector, &program_id, &error); + Err(error) + } } Err(error) => { let error = match error { @@ -1058,7 +1073,6 @@ impl Executor for BpfExecutor { stable_log::program_failure(&log_collector, &program_id, &error); Err(error) } - _ => Ok(()), } }; execute_time.stop(); From 1c4231a1d26a5a7af00e67c9dbc3dec577d882f3 Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Wed, 22 Dec 2021 15:16:04 -0600 Subject: [PATCH 5/8] pr: fix match --- programs/bpf_loader/src/lib.rs | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index e00d5674d88b63..d301fc2e7ab612 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -1041,24 +1041,20 @@ impl Executor for BpfExecutor { stable_log::program_return(&log_collector, &program_id, return_data); } match result { - Ok(status) => { - if status == SUCCESS { - Ok(()) + Ok(status) if status != SUCCESS => { + let error: InstructionError = if status == ACCOUNTS_DATA_BUDGET_EXCEEDED + && !invoke_context + .feature_set + .is_active(&cap_accounts_data_len::id()) + { + // Until the cap_accounts_data_len feature is enabled, map the + // ACCOUNTS_DATA_BUDGET_EXCEEDED error to InvalidError + InstructionError::InvalidError } else { - let error: InstructionError = if status == ACCOUNTS_DATA_BUDGET_EXCEEDED - && !invoke_context - .feature_set - .is_active(&cap_accounts_data_len::id()) - { - // Until the cap_accounts_data_len feature is enabled, map the - // ACCOUNTS_DATA_BUDGET_EXCEEDED error to InvalidError - InstructionError::InvalidError - } else { - status.into() - }; - stable_log::program_failure(&log_collector, &program_id, &error); - Err(error) - } + status.into() + }; + stable_log::program_failure(&log_collector, &program_id, &error); + Err(error) } Err(error) => { let error = match error { @@ -1073,6 +1069,7 @@ impl Executor for BpfExecutor { stable_log::program_failure(&log_collector, &program_id, &error); Err(error) } + _ => Ok(()), } }; execute_time.stop(); From c543da6c990069d4cac6dd2af645cd24ba186cdc Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Mon, 27 Dec 2021 11:02:38 -0600 Subject: [PATCH 6/8] pr: move AccountsDataMeter to own file, and remove Rc> --- program-runtime/src/accounts_data_meter.rs | 145 +++++++++++++++++++++ program-runtime/src/invoke_context.rs | 63 +-------- program-runtime/src/lib.rs | 1 + runtime/src/message_processor.rs | 2 +- 4 files changed, 154 insertions(+), 57 deletions(-) create mode 100644 program-runtime/src/accounts_data_meter.rs diff --git a/program-runtime/src/accounts_data_meter.rs b/program-runtime/src/accounts_data_meter.rs new file mode 100644 index 00000000000000..26cdc67562da0f --- /dev/null +++ b/program-runtime/src/accounts_data_meter.rs @@ -0,0 +1,145 @@ +//! 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 +/// [Validator Requirements](https://docs.solana.com/running-validator/validator-reqs), and +/// validators often put the ledger on a RAM disk (i.e. tmpfs). +pub const MAX_ACCOUNTS_DATA_LEN: u64 = 128_000_000_000; // 128 GB + +/// 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 current amount of accounts data space used (in bytes) + current: u64, +} + +impl AccountsDataMeter { + /// Make a new AccountsDataMeter + pub fn new(current_accounts_data_len: u64) -> Self { + let accounts_data_meter = Self { + maximum: MAX_ACCOUNTS_DATA_LEN, + current: current_accounts_data_len, + }; + debug_assert!(accounts_data_meter.current <= 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 current amount of accounts data space used (in bytes) + pub fn current(&self) -> u64 { + self.current + } + + /// Get the remaining amount of accounts data space (in bytes) + pub fn remaining(&self) -> u64 { + self.maximum.saturating_sub(self.current) + } + + /// Consume accounts data space, in bytes. If `amount` is positive, we are *increasing* the + /// amount of accounts data space used. If `amount` is negative, we are *decreasing* the + /// amount of accounts data space used. + pub fn consume(&mut self, amount: i64) -> Result<(), InstructionError> { + if amount == 0 { + // nothing to do here; lets us skip doing unnecessary work in the 'else' case + return Ok(()); + } + + if amount.is_positive() { + let amount = amount as u64; + if amount > self.remaining() { + return Err(InstructionError::AccountsDataBudgetExceeded); + } + self.current = self.current.saturating_add(amount); + } else { + let amount = amount.abs() as u64; + self.current = self.current.saturating_sub(amount); + } + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_new() { + let current = 1234; + let accounts_data_meter = AccountsDataMeter::new(current); + assert_eq!(accounts_data_meter.maximum, MAX_ACCOUNTS_DATA_LEN); + assert_eq!(accounts_data_meter.current, current); + } + + #[test] + fn test_new_can_use_max_len() { + let _ = AccountsDataMeter::new(MAX_ACCOUNTS_DATA_LEN); + } + + #[test] + #[should_panic] + fn test_new_panics_if_current_len_too_big() { + let _ = AccountsDataMeter::new(MAX_ACCOUNTS_DATA_LEN + 1); + } + + #[test] + fn test_remaining() { + let current_accounts_data_len = 0; + let accounts_data_meter = AccountsDataMeter::new(current_accounts_data_len); + assert_eq!(accounts_data_meter.remaining(), MAX_ACCOUNTS_DATA_LEN); + } + + #[test] + fn test_remaining_saturates() { + let current_accounts_data_len = 0; + let mut accounts_data_meter = AccountsDataMeter::new(current_accounts_data_len); + // To test that remaining() saturates, need to break the invariant that current <= maximum + accounts_data_meter.current = MAX_ACCOUNTS_DATA_LEN + 1; + assert_eq!(accounts_data_meter.remaining(), 0); + } + + #[test] + fn test_consume() { + let current_accounts_data_len = 0; + let mut accounts_data_meter = AccountsDataMeter::new(current_accounts_data_len); + + // Test: simple, positive numbers + let result = accounts_data_meter.consume(0); + assert!(result.is_ok()); + let result = accounts_data_meter.consume(1); + assert!(result.is_ok()); + let result = accounts_data_meter.consume(4); + assert!(result.is_ok()); + let result = accounts_data_meter.consume(9); + assert!(result.is_ok()); + + // Test: can consume the remaining amount + let remaining = accounts_data_meter.remaining() as i64; + let result = accounts_data_meter.consume(remaining); + assert!(result.is_ok()); + assert_eq!(accounts_data_meter.remaining(), 0); + } + + #[test] + fn test_consume_deallocate() { + let current_accounts_data_len = 10_000; + let mut accounts_data_meter = AccountsDataMeter::new(current_accounts_data_len); + let remaining_before = accounts_data_meter.remaining(); + + let amount = (current_accounts_data_len / 2) as i64; + let amount = -amount; + let result = accounts_data_meter.consume(amount); + assert!(result.is_ok()); + let remaining_after = accounts_data_meter.remaining(); + assert_eq!(remaining_after, remaining_before + amount.abs() as u64); + } +} diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index a00a1b484b62bd..7ebff7743c5eb8 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -1,8 +1,8 @@ use { crate::{ - ic_logger_msg, ic_msg, instruction_recorder::InstructionRecorder, - log_collector::LogCollector, native_loader::NativeLoader, pre_account::PreAccount, - timings::ExecuteDetailsTimings, + accounts_data_meter::AccountsDataMeter, ic_logger_msg, ic_msg, + instruction_recorder::InstructionRecorder, log_collector::LogCollector, + native_loader::NativeLoader, pre_account::PreAccount, timings::ExecuteDetailsTimings, }, solana_sdk::{ account::{AccountSharedData, ReadableAccount}, @@ -119,55 +119,6 @@ impl ComputeMeter { } } -/// 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 current amount of accounts data space used (in bytes) - current: u64, -} -impl AccountsDataMeter { - /// Make a new AccountsDataMeter, wrapped in an Rc & RefCell - pub fn new_ref(current_accounts_data_len: u64) -> Rc> { - let accounts_data_meter = Self { - maximum: MAX_ACCOUNTS_DATA_LEN, - current: current_accounts_data_len, - }; - debug_assert!(accounts_data_meter.current <= accounts_data_meter.maximum); - Rc::new(RefCell::new(accounts_data_meter)) - } - /// Consume the AccountsDataMeter and return the final (i.e. current) accounts data len - pub fn finalize(self) -> u64 { - self.current - } - /// Get the remaining amount of accounts data space - pub fn remaining(&self) -> u64 { - self.maximum.saturating_sub(self.current) - } - /// Consume accounts data space. If `amount` is positive, we are *increasing* the amount of - /// accounts data space used. If `amount` is negative, we are *decreasing* the amount of - /// accounts data space used. - pub fn consume(&mut self, amount: i64) -> Result<(), InstructionError> { - if amount == 0 { - // nothing to do here; lets us skip doing unnecessary work in the 'else' case - return Ok(()); - } - - if amount.is_positive() { - let amount = amount as u64; - if amount > self.remaining() { - return Err(InstructionError::AccountsDataBudgetExceeded); - } - self.current = self.current.saturating_add(amount); - } else { - let amount = amount.abs() as u64; - self.current = self.current.saturating_sub(amount); - } - Ok(()) - } -} - pub struct StackFrame<'a> { pub number_of_program_accounts: usize, pub keyed_accounts: Vec>, @@ -205,7 +156,7 @@ pub struct InvokeContext<'a> { compute_budget: ComputeBudget, current_compute_budget: ComputeBudget, compute_meter: Rc>, - accounts_data_meter: Rc>, + accounts_data_meter: AccountsDataMeter, executors: Rc>, pub instruction_recorder: Option>>, pub feature_set: Arc, @@ -242,7 +193,7 @@ impl<'a> InvokeContext<'a> { current_compute_budget: compute_budget, compute_budget, compute_meter: ComputeMeter::new_ref(compute_budget.max_units), - accounts_data_meter: AccountsDataMeter::new_ref(current_accounts_data_len), + accounts_data_meter: AccountsDataMeter::new(current_accounts_data_len), executors, instruction_recorder, feature_set, @@ -897,8 +848,8 @@ impl<'a> InvokeContext<'a> { } /// Get this invocation's AccountsDataMeter - pub fn get_accounts_data_meter(&self) -> Rc> { - Rc::clone(&self.accounts_data_meter) + pub fn get_accounts_data_meter(&self) -> &AccountsDataMeter { + &self.accounts_data_meter } /// Loaders may need to do work in order to execute a program. Cache diff --git a/program-runtime/src/lib.rs b/program-runtime/src/lib.rs index bca6d413211bbb..c72e6d94c659d0 100644 --- a/program-runtime/src/lib.rs +++ b/program-runtime/src/lib.rs @@ -1,5 +1,6 @@ #![cfg_attr(RUSTC_WITH_SPECIALIZATION, feature(min_specialization))] +pub mod accounts_data_meter; pub mod instruction_recorder; pub mod invoke_context; pub mod log_collector; diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 8ba2e4ce0e250b..2756cd166bbb61 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -142,7 +142,7 @@ impl MessageProcessor { timings.accumulate(&invoke_context.timings); } Ok(ProcessedMessageInfo { - accounts_data_len: invoke_context.get_accounts_data_meter().take().finalize(), + accounts_data_len: invoke_context.get_accounts_data_meter().current(), }) } } From 60dc5fd38e2bdcc748f760c9a84650b720140b68 Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Mon, 27 Dec 2021 11:18:44 -0600 Subject: [PATCH 7/8] fixup: remove dup const --- program-runtime/src/invoke_context.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index 7ebff7743c5eb8..f1575a8451e28d 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -23,8 +23,6 @@ use { }, std::{cell::RefCell, collections::HashMap, fmt::Debug, rc::Rc, sync::Arc}, }; -/// The maximum allowed size, in bytes, of the accounts data -pub const MAX_ACCOUNTS_DATA_LEN: u64 = 128_000_000_000; // 128 GB pub type TransactionAccountRefCell = (Pubkey, RefCell); pub type TransactionAccountRefCells = Vec; From f6723281d567505941ebdae8a417927f5e6dca33 Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Mon, 27 Dec 2021 11:43:33 -0600 Subject: [PATCH 8/8] add more tests --- program-runtime/src/accounts_data_meter.rs | 27 +++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/program-runtime/src/accounts_data_meter.rs b/program-runtime/src/accounts_data_meter.rs index 26cdc67562da0f..00f3e7843ad730 100644 --- a/program-runtime/src/accounts_data_meter.rs +++ b/program-runtime/src/accounts_data_meter.rs @@ -7,7 +7,7 @@ use solana_sdk::instruction::InstructionError; /// 128 GB was chosen because it is the RAM amount listed under Hardware Recommendations on /// [Validator Requirements](https://docs.solana.com/running-validator/validator-reqs), and /// validators often put the ledger on a RAM disk (i.e. tmpfs). -pub const MAX_ACCOUNTS_DATA_LEN: u64 = 128_000_000_000; // 128 GB +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)] @@ -142,4 +142,29 @@ mod tests { let remaining_after = accounts_data_meter.remaining(); assert_eq!(remaining_after, remaining_before + amount.abs() as u64); } + + #[test] + fn test_consume_too_much() { + let current_accounts_data_len = 0; + let mut accounts_data_meter = AccountsDataMeter::new(current_accounts_data_len); + + // Test: consuming more than what's available (1) returns an error, (2) does not consume + let remaining = accounts_data_meter.remaining(); + let result = accounts_data_meter.consume(remaining as i64 + 1); + assert!(result.is_err()); + assert_eq!(accounts_data_meter.remaining(), remaining); + } + + #[test] + fn test_consume_zero() { + // Pre-condition: set up the accounts data meter such that there is no remaining space + let current_accounts_data_len = 1234; + let mut accounts_data_meter = AccountsDataMeter::new(current_accounts_data_len); + accounts_data_meter.maximum = current_accounts_data_len; + assert_eq!(accounts_data_meter.remaining(), 0); + + // Test: can always consume zero, even if there is no remaining space + let result = accounts_data_meter.consume(0); + assert!(result.is_ok()); + } }