From 800472ddf57af0ef3c946829c4aa094a9184f391 Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Tue, 28 Dec 2021 05:14:48 -0600 Subject: [PATCH] Add AccountsDataMeter to InvokeContext (#21813) --- program-runtime/src/accounts_data_meter.rs | 170 ++++++++++++++++++ program-runtime/src/invoke_context.rs | 15 +- program-runtime/src/lib.rs | 1 + programs/bpf_loader/src/lib.rs | 17 +- runtime/src/bank.rs | 32 ++-- runtime/src/message_processor.rs | 17 +- sdk/program/src/instruction.rs | 4 + sdk/program/src/program_error.rs | 8 + sdk/src/feature_set.rs | 5 + storage-proto/proto/transaction_by_addr.proto | 1 + storage-proto/src/convert.rs | 4 + 11 files changed, 247 insertions(+), 27 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 0000000000..00f3e7843a --- /dev/null +++ b/program-runtime/src/accounts_data_meter.rs @@ -0,0 +1,170 @@ +//! 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; + +/// 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); + } + + #[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()); + } +} diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index afb677ccda..3bd73dec5e 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}, @@ -146,6 +146,7 @@ pub struct InvokeContext<'a> { compute_budget: ComputeBudget, current_compute_budget: ComputeBudget, compute_meter: Rc>, + accounts_data_meter: AccountsDataMeter, executors: Rc>, pub instruction_recorder: Option>>, pub feature_set: Arc, @@ -168,6 +169,7 @@ impl<'a> InvokeContext<'a> { feature_set: Arc, blockhash: Hash, lamports_per_signature: u64, + current_accounts_data_len: u64, ) -> Self { Self { transaction_context, @@ -181,6 +183,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(current_accounts_data_len), executors, instruction_recorder, feature_set, @@ -206,6 +209,7 @@ impl<'a> InvokeContext<'a> { Arc::new(FeatureSet::all_enabled()), Hash::default(), 0, + 0, ) } @@ -856,6 +860,11 @@ impl<'a> InvokeContext<'a> { self.compute_meter.clone() } + /// Get this invocation's AccountsDataMeter + 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 /// the work that can be re-used across executions pub fn add_executor(&self, pubkey: &Pubkey, executor: Arc) { diff --git a/program-runtime/src/lib.rs b/program-runtime/src/lib.rs index bca6d41321..c72e6d94c6 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/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index c41153ba1e..d301fc2e7a 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, @@ -1041,7 +1042,17 @@ impl Executor for BpfExecutor { } match result { Ok(status) if status != SUCCESS => { - let error: InstructionError = status.into(); + 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) } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index e61d1f9341..6a3125db76 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -146,7 +146,7 @@ use { sync::{ atomic::{ AtomicBool, AtomicU64, - Ordering::{AcqRel, Acquire, Relaxed, Release}, + Ordering::{Acquire, Relaxed, Release}, }, Arc, LockResult, RwLock, RwLockReadGuard, RwLockWriteGuard, }, @@ -238,7 +238,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. @@ -1192,8 +1192,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 } @@ -1441,7 +1440,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()); @@ -3591,11 +3590,10 @@ 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( - process_result.accounts_data_len_delta, - ) + self.store_accounts_data_len(process_result.accounts_data_len) }); } else { // TODO: support versioned messages @@ -3752,16 +3750,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 d3bb5f2975..4c9d4beb7a 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -38,8 +38,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 { @@ -64,6 +64,7 @@ impl MessageProcessor { sysvars: &[(Pubkey, Vec)], blockhash: Hash, lamports_per_signature: u64, + current_accounts_data_len: u64, ) -> Result { let mut invoke_context = InvokeContext::new( transaction_context, @@ -77,6 +78,7 @@ impl MessageProcessor { feature_set, blockhash, lamports_per_signature, + current_accounts_data_len, ); debug_assert_eq!(program_indices.len(), message.instructions.len()); @@ -141,7 +143,9 @@ impl MessageProcessor { ); timings.accumulate(&invoke_context.timings); } - Ok(ProcessedMessageInfo::default()) + Ok(ProcessedMessageInfo { + accounts_data_len: invoke_context.get_accounts_data_meter().current(), + }) } } @@ -265,6 +269,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert!(result.is_ok()); assert_eq!( @@ -305,6 +310,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert_eq!( result, @@ -337,6 +343,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert_eq!( result, @@ -480,6 +487,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert_eq!( result, @@ -513,6 +521,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert!(result.is_ok()); @@ -543,6 +552,7 @@ mod tests { &[], Hash::default(), 0, + 0, ); assert!(result.is_ok()); assert_eq!( @@ -615,6 +625,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 c8be38bef5..e1fbb1825a 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 cc0f18f186..c47638dddb 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/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 4c37b319fe..3b85a49fd8 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() diff --git a/storage-proto/proto/transaction_by_addr.proto b/storage-proto/proto/transaction_by_addr.proto index d1a40cfd7c..582c31991f 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 636dcc3f9d..4237321960 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) => {