From 3967d615926cebddff61d00e7e28985d1fc74b51 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Thu, 26 Sep 2024 08:44:23 +0800 Subject: [PATCH 1/2] fix: set allocation size to 0 for transactions known to fail (#2966) (cherry picked from commit 443246dee0ec0cacea08d8bc63eed7d4e57089f7) # Conflicts: # cost-model/src/cost_model.rs --- cost-model/src/cost_model.rs | 220 ++++++++++++++++++++++++++++++----- 1 file changed, 191 insertions(+), 29 deletions(-) diff --git a/cost-model/src/cost_model.rs b/cost-model/src/cost_model.rs index 3205bfae990d3a..033899994602a5 100644 --- a/cost-model/src/cost_model.rs +++ b/cost-model/src/cost_model.rs @@ -23,7 +23,11 @@ use { instruction::CompiledInstruction, program_utils::limited_deserialize, pubkey::Pubkey, - system_instruction::SystemInstruction, + saturating_add_assign, + system_instruction::{ + SystemInstruction, MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION, + MAX_PERMITTED_DATA_LENGTH, + }, system_program, transaction::SanitizedTransaction, }, @@ -31,6 +35,13 @@ use { pub struct CostModel; +#[derive(Debug, PartialEq)] +enum SystemProgramAccountAllocation { + None, + Some(u64), + Failed, +} + impl CostModel { pub fn calculate_cost( transaction: &SanitizedTransaction, @@ -177,47 +188,40 @@ impl CostModel { fn calculate_account_data_size_on_deserialized_system_instruction( instruction: SystemInstruction, - ) -> u64 { + ) -> SystemProgramAccountAllocation { 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, + SystemInstruction::CreateAccount { space, .. } + | SystemInstruction::CreateAccountWithSeed { space, .. } + | SystemInstruction::Allocate { space } + | SystemInstruction::AllocateWithSeed { space, .. } => { + if space > MAX_PERMITTED_DATA_LENGTH { + SystemProgramAccountAllocation::Failed + } else { + SystemProgramAccountAllocation::Some(space) + } + } + _ => SystemProgramAccountAllocation::None, } } fn calculate_account_data_size_on_instruction( program_id: &Pubkey, instruction: &CompiledInstruction, - ) -> u64 { + ) -> SystemProgramAccountAllocation { 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, - ); + Self::calculate_account_data_size_on_deserialized_system_instruction(instruction) + } else { + SystemProgramAccountAllocation::Failed } + } else { + SystemProgramAccountAllocation::None } - 0 } /// eventually, potentially determine account data size of all writable accounts /// at the moment, calculate account data size of account creation +<<<<<<< HEAD fn calculate_account_data_size(transaction: &SanitizedTransaction) -> u64 { transaction .message() @@ -226,6 +230,40 @@ impl CostModel { Self::calculate_account_data_size_on_instruction(program_id, instruction) }) .sum() +======= + fn calculate_allocated_accounts_data_size(transaction: &SanitizedTransaction) -> u64 { + let mut tx_attempted_allocation_size: u64 = 0; + for (program_id, instruction) in transaction.message().program_instructions_iter() { + match Self::calculate_account_data_size_on_instruction(program_id, instruction) { + SystemProgramAccountAllocation::Failed => { + // If any system program instructions can be statically + // determined to fail, no allocations will actually be + // persisted by the transaction. So return 0 here so that no + // account allocation budget is used for this failed + // transaction. + return 0; + } + SystemProgramAccountAllocation::None => continue, + SystemProgramAccountAllocation::Some(ix_attempted_allocation_size) => { + saturating_add_assign!( + tx_attempted_allocation_size, + ix_attempted_allocation_size + ); + } + } + } + + // The runtime prevents transactions from allocating too much account + // data so clamp the attempted allocation size to the max amount. + // + // Note that if there are any custom bpf instructions in the transaction + // it's tricky to know whether a newly allocated account will be freed + // or not during an intermediate instruction in the transaction so we + // shouldn't assume that a large sum of allocations will necessarily + // lead to transaction failure. + (MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION as u64) + .min(tx_attempted_allocation_size) +>>>>>>> 443246dee0 (fix: set allocation size to 0 for transactions known to fail (#2966)) } } @@ -251,6 +289,130 @@ mod tests { (Keypair::new(), Hash::new_unique()) } + #[test] + fn test_calculate_allocated_accounts_data_size_no_allocation() { + let transaction = Transaction::new_unsigned(Message::new( + &[system_instruction::transfer( + &Pubkey::new_unique(), + &Pubkey::new_unique(), + 1, + )], + Some(&Pubkey::new_unique()), + )); + let sanitized_tx = SanitizedTransaction::from_transaction_for_tests(transaction); + + assert_eq!( + CostModel::calculate_allocated_accounts_data_size(&sanitized_tx), + 0 + ); + } + + #[test] + fn test_calculate_allocated_accounts_data_size_multiple_allocations() { + let space1 = 100; + let space2 = 200; + let transaction = Transaction::new_unsigned(Message::new( + &[ + system_instruction::create_account( + &Pubkey::new_unique(), + &Pubkey::new_unique(), + 1, + space1, + &Pubkey::new_unique(), + ), + system_instruction::allocate(&Pubkey::new_unique(), space2), + ], + Some(&Pubkey::new_unique()), + )); + let sanitized_tx = SanitizedTransaction::from_transaction_for_tests(transaction); + + assert_eq!( + CostModel::calculate_allocated_accounts_data_size(&sanitized_tx), + space1 + space2 + ); + } + + #[test] + fn test_calculate_allocated_accounts_data_size_max_limit() { + let spaces = [MAX_PERMITTED_DATA_LENGTH, MAX_PERMITTED_DATA_LENGTH, 100]; + assert!( + spaces.iter().copied().sum::() + > MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION as u64 + ); + let transaction = Transaction::new_unsigned(Message::new( + &[ + system_instruction::create_account( + &Pubkey::new_unique(), + &Pubkey::new_unique(), + 1, + spaces[0], + &Pubkey::new_unique(), + ), + system_instruction::create_account( + &Pubkey::new_unique(), + &Pubkey::new_unique(), + 1, + spaces[1], + &Pubkey::new_unique(), + ), + system_instruction::create_account( + &Pubkey::new_unique(), + &Pubkey::new_unique(), + 1, + spaces[2], + &Pubkey::new_unique(), + ), + ], + Some(&Pubkey::new_unique()), + )); + let sanitized_tx = SanitizedTransaction::from_transaction_for_tests(transaction); + + assert_eq!( + CostModel::calculate_allocated_accounts_data_size(&sanitized_tx), + MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION as u64, + ); + } + + #[test] + fn test_calculate_allocated_accounts_data_size_overflow() { + let transaction = Transaction::new_unsigned(Message::new( + &[ + system_instruction::create_account( + &Pubkey::new_unique(), + &Pubkey::new_unique(), + 1, + 100, + &Pubkey::new_unique(), + ), + system_instruction::allocate(&Pubkey::new_unique(), u64::MAX), + ], + Some(&Pubkey::new_unique()), + )); + let sanitized_tx = SanitizedTransaction::from_transaction_for_tests(transaction); + + assert_eq!( + 0, // SystemProgramAccountAllocation::Failed, + CostModel::calculate_allocated_accounts_data_size(&sanitized_tx), + ); + } + + #[test] + fn test_calculate_allocated_accounts_data_size_invalid_ix() { + let transaction = Transaction::new_unsigned(Message::new( + &[ + system_instruction::allocate(&Pubkey::new_unique(), 100), + Instruction::new_with_bincode(system_program::id(), &(), vec![]), + ], + Some(&Pubkey::new_unique()), + )); + let sanitized_tx = SanitizedTransaction::from_transaction_for_tests(transaction); + + assert_eq!( + 0, // SystemProgramAccountAllocation::Failed, + CostModel::calculate_allocated_accounts_data_size(&sanitized_tx), + ); + } + #[test] fn test_cost_model_data_len_cost() { let lamports = 0; @@ -280,14 +442,14 @@ mod tests { }, ] { assert_eq!( - space, + SystemProgramAccountAllocation::Some(space), CostModel::calculate_account_data_size_on_deserialized_system_instruction( instruction ) ); } assert_eq!( - 0, + SystemProgramAccountAllocation::None, CostModel::calculate_account_data_size_on_deserialized_system_instruction( SystemInstruction::TransferWithSeed { lamports, From d50a08914552319878dd619417e63aa36d4efdc8 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Thu, 26 Sep 2024 08:42:54 +0000 Subject: [PATCH 2/2] fix conflicts --- cost-model/src/cost_model.rs | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/cost-model/src/cost_model.rs b/cost-model/src/cost_model.rs index 033899994602a5..92a7914ca5973e 100644 --- a/cost-model/src/cost_model.rs +++ b/cost-model/src/cost_model.rs @@ -221,17 +221,7 @@ impl CostModel { /// eventually, potentially determine account data size of all writable accounts /// at the moment, calculate account data size of account creation -<<<<<<< HEAD fn calculate_account_data_size(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_allocated_accounts_data_size(transaction: &SanitizedTransaction) -> u64 { let mut tx_attempted_allocation_size: u64 = 0; for (program_id, instruction) in transaction.message().program_instructions_iter() { match Self::calculate_account_data_size_on_instruction(program_id, instruction) { @@ -263,7 +253,6 @@ impl CostModel { // lead to transaction failure. (MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION as u64) .min(tx_attempted_allocation_size) ->>>>>>> 443246dee0 (fix: set allocation size to 0 for transactions known to fail (#2966)) } } @@ -290,7 +279,7 @@ mod tests { } #[test] - fn test_calculate_allocated_accounts_data_size_no_allocation() { + fn test_calculate_account_data_size_no_allocation() { let transaction = Transaction::new_unsigned(Message::new( &[system_instruction::transfer( &Pubkey::new_unique(), @@ -301,14 +290,11 @@ mod tests { )); let sanitized_tx = SanitizedTransaction::from_transaction_for_tests(transaction); - assert_eq!( - CostModel::calculate_allocated_accounts_data_size(&sanitized_tx), - 0 - ); + assert_eq!(CostModel::calculate_account_data_size(&sanitized_tx), 0); } #[test] - fn test_calculate_allocated_accounts_data_size_multiple_allocations() { + fn test_calculate_account_data_size_multiple_allocations() { let space1 = 100; let space2 = 200; let transaction = Transaction::new_unsigned(Message::new( @@ -327,13 +313,13 @@ mod tests { let sanitized_tx = SanitizedTransaction::from_transaction_for_tests(transaction); assert_eq!( - CostModel::calculate_allocated_accounts_data_size(&sanitized_tx), + CostModel::calculate_account_data_size(&sanitized_tx), space1 + space2 ); } #[test] - fn test_calculate_allocated_accounts_data_size_max_limit() { + fn test_calculate_account_data_size_max_limit() { let spaces = [MAX_PERMITTED_DATA_LENGTH, MAX_PERMITTED_DATA_LENGTH, 100]; assert!( spaces.iter().copied().sum::() @@ -368,13 +354,13 @@ mod tests { let sanitized_tx = SanitizedTransaction::from_transaction_for_tests(transaction); assert_eq!( - CostModel::calculate_allocated_accounts_data_size(&sanitized_tx), + CostModel::calculate_account_data_size(&sanitized_tx), MAX_PERMITTED_ACCOUNTS_DATA_ALLOCATIONS_PER_TRANSACTION as u64, ); } #[test] - fn test_calculate_allocated_accounts_data_size_overflow() { + fn test_calculate_account_data_size_overflow() { let transaction = Transaction::new_unsigned(Message::new( &[ system_instruction::create_account( @@ -392,12 +378,12 @@ mod tests { assert_eq!( 0, // SystemProgramAccountAllocation::Failed, - CostModel::calculate_allocated_accounts_data_size(&sanitized_tx), + CostModel::calculate_account_data_size(&sanitized_tx), ); } #[test] - fn test_calculate_allocated_accounts_data_size_invalid_ix() { + fn test_calculate_account_data_size_invalid_ix() { let transaction = Transaction::new_unsigned(Message::new( &[ system_instruction::allocate(&Pubkey::new_unique(), 100), @@ -409,7 +395,7 @@ mod tests { assert_eq!( 0, // SystemProgramAccountAllocation::Failed, - CostModel::calculate_allocated_accounts_data_size(&sanitized_tx), + CostModel::calculate_account_data_size(&sanitized_tx), ); }