From 721496b9008791e1ef61e6306636a7ffe9bb926f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Tue, 13 Dec 2022 22:54:24 +0100 Subject: [PATCH] Feature - Program redeployment cooldown (#29136) * Registers the feature enable_program_redeployment_cooldown. * Adds redeployment slot constraint. * Adds test to assert that closed programs can not be reopened. * Ensure that program close truncates the account data. * Adds set_sysvar_for_tests() to SBF program tests. --- programs/bpf_loader/src/lib.rs | 124 +++++++++++++++++++--- programs/sbf/tests/programs.rs | 12 +++ runtime/src/bank_client.rs | 5 + sdk/program/src/bpf_loader_upgradeable.rs | 12 +++ sdk/src/feature_set.rs | 5 + 5 files changed, 146 insertions(+), 12 deletions(-) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 53a205f5a..a1eccd5d5 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -43,8 +43,8 @@ use { cap_accounts_data_allocations_per_transaction, cap_bpf_program_instruction_accounts, check_slice_translation_size, disable_deploy_of_alloc_free_syscall, disable_deprecated_loader, enable_bpf_loader_extend_program_ix, - enable_bpf_loader_set_authority_checked_ix, limit_max_instruction_trace_length, - FeatureSet, + enable_bpf_loader_set_authority_checked_ix, enable_program_redeployment_cooldown, + limit_max_instruction_trace_length, FeatureSet, }, instruction::{AccountMeta, InstructionError}, loader_instruction::LoaderInstruction, @@ -601,7 +601,6 @@ fn process_loader_upgradeable_instruction( } // Create ProgramData account - let (derived_address, bump_seed) = Pubkey::find_program_address(&[new_program_id.as_ref()], program_id); if derived_address != programdata_key { @@ -801,10 +800,18 @@ fn process_loader_upgradeable_instruction( return Err(InstructionError::InsufficientFunds); } if let UpgradeableLoaderState::ProgramData { - slot: _, + slot, upgrade_authority_address, } = programdata.get_state()? { + if invoke_context + .feature_set + .is_active(&enable_program_redeployment_cooldown::id()) + && clock.slot == slot + { + ic_logger_msg!(log_collector, "Program was deployed in this block already"); + return Err(InstructionError::InvalidArgument); + } if upgrade_authority_address.is_none() { ic_logger_msg!(log_collector, "Program not upgradeable"); return Err(InstructionError::Immutable); @@ -1048,7 +1055,14 @@ fn process_loader_upgradeable_instruction( let mut close_account = instruction_context.try_borrow_instruction_account(transaction_context, 0)?; let close_key = *close_account.get_key(); - match close_account.get_state()? { + let close_account_state = close_account.get_state()?; + if invoke_context + .feature_set + .is_active(&enable_program_redeployment_cooldown::id()) + { + close_account.set_data_length(UpgradeableLoaderState::size_of_uninitialized())?; + } + match close_account_state { UpgradeableLoaderState::Uninitialized => { let mut recipient_account = instruction_context .try_borrow_instruction_account(transaction_context, 1)?; @@ -1518,7 +1532,7 @@ mod tests { instruction::{AccountMeta, InstructionError}, pubkey::Pubkey, rent::Rent, - sysvar, + system_program, sysvar, }, std::{fs::File, io::Read, ops::Range}, }; @@ -2299,7 +2313,7 @@ mod tests { let spill_account = AccountSharedData::new(0, 0, &Pubkey::new_unique()); let rent_account = create_account_for_test(&rent); let clock_account = create_account_for_test(&Clock { - slot: SLOT, + slot: SLOT.saturating_add(1), ..Clock::default() }); let upgrade_authority_account = AccountSharedData::new(1, 0, &Pubkey::new_unique()); @@ -2394,7 +2408,7 @@ mod tests { assert_eq!( state, UpgradeableLoaderState::ProgramData { - slot: SLOT, + slot: SLOT.saturating_add(1), upgrade_authority_address: Some(upgrade_authority_address) } ); @@ -3529,7 +3543,7 @@ mod tests { let recipient_account = AccountSharedData::new(1, 0, &Pubkey::new_unique()); let buffer_address = Pubkey::new_unique(); let mut buffer_account = - AccountSharedData::new(1, UpgradeableLoaderState::size_of_buffer(0), &loader_id); + AccountSharedData::new(1, UpgradeableLoaderState::size_of_buffer(128), &loader_id); buffer_account .set_state(&UpgradeableLoaderState::Buffer { authority_address: Some(authority_address), @@ -3547,7 +3561,7 @@ mod tests { let programdata_address = Pubkey::new_unique(); let mut programdata_account = AccountSharedData::new( 1, - UpgradeableLoaderState::size_of_programdata(0), + UpgradeableLoaderState::size_of_programdata(128), &loader_id, ); programdata_account @@ -3603,6 +3617,10 @@ mod tests { assert_eq!(2, accounts.get(1).unwrap().lamports()); let state: UpgradeableLoaderState = accounts.first().unwrap().state().unwrap(); assert_eq!(state, UpgradeableLoaderState::Uninitialized); + assert_eq!( + UpgradeableLoaderState::size_of_uninitialized(), + accounts.first().unwrap().data().len() + ); // Case: close with wrong authority process_instruction( @@ -3651,6 +3669,10 @@ mod tests { assert_eq!(2, accounts.get(1).unwrap().lamports()); let state: UpgradeableLoaderState = accounts.first().unwrap().state().unwrap(); assert_eq!(state, UpgradeableLoaderState::Uninitialized); + assert_eq!( + UpgradeableLoaderState::size_of_uninitialized(), + accounts.first().unwrap().data().len() + ); // Case: close a program account let accounts = process_instruction( @@ -3659,8 +3681,8 @@ mod tests { &instruction, vec![ (programdata_address, programdata_account.clone()), - (recipient_address, recipient_account), - (authority_address, authority_account), + (recipient_address, recipient_account.clone()), + (authority_address, authority_account.clone()), (program_address, program_account.clone()), ], vec![ @@ -3683,8 +3705,14 @@ mod tests { assert_eq!(2, accounts.get(1).unwrap().lamports()); let state: UpgradeableLoaderState = accounts.first().unwrap().state().unwrap(); assert_eq!(state, UpgradeableLoaderState::Uninitialized); + assert_eq!( + UpgradeableLoaderState::size_of_uninitialized(), + accounts.first().unwrap().data().len() + ); // Try to invoke closed account + programdata_account = accounts.first().unwrap().clone(); + program_account = accounts.get(3).unwrap().clone(); process_instruction( &program_address, &[0, 1], @@ -3696,6 +3724,78 @@ mod tests { Vec::new(), Err(InstructionError::InvalidAccountData), ); + + // Case: Reopen should fail + process_instruction( + &loader_id, + &[], + &bincode::serialize(&UpgradeableLoaderInstruction::DeployWithMaxDataLen { + max_data_len: 0, + }) + .unwrap(), + vec![ + (recipient_address, recipient_account), + (programdata_address, programdata_account), + (program_address, program_account), + (buffer_address, buffer_account), + ( + sysvar::rent::id(), + create_account_for_test(&Rent::default()), + ), + ( + sysvar::clock::id(), + create_account_for_test(&Clock::default()), + ), + ( + system_program::id(), + AccountSharedData::new(0, 0, &system_program::id()), + ), + (authority_address, authority_account), + ], + vec![ + AccountMeta { + pubkey: recipient_address, + is_signer: true, + is_writable: true, + }, + AccountMeta { + pubkey: programdata_address, + is_signer: false, + is_writable: true, + }, + AccountMeta { + pubkey: program_address, + is_signer: false, + is_writable: true, + }, + AccountMeta { + pubkey: buffer_address, + is_signer: false, + is_writable: false, + }, + AccountMeta { + pubkey: sysvar::rent::id(), + is_signer: false, + is_writable: false, + }, + AccountMeta { + pubkey: sysvar::clock::id(), + is_signer: false, + is_writable: false, + }, + AccountMeta { + pubkey: system_program::id(), + is_signer: false, + is_writable: false, + }, + AccountMeta { + pubkey: authority_address, + is_signer: false, + is_writable: false, + }, + ], + Err(InstructionError::AccountAlreadyInitialized), + ); } /// fuzzing utility function diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 839a26d1c..cd5e12f5d 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -163,6 +163,10 @@ fn load_upgradeable_sbf_program( authority_keypair, elf, ); + bank_client.set_sysvar_for_tests(&clock::Clock { + slot: 1, + ..clock::Clock::default() + }); } #[cfg(feature = "sbf_rust")] @@ -2002,6 +2006,10 @@ fn test_program_sbf_upgrade() { &authority_keypair, "solana_sbf_rust_upgraded", ); + bank_client.set_sysvar_for_tests(&clock::Clock { + slot: 2, + ..clock::Clock::default() + }); // Call upgraded program instruction.data[0] += 1; @@ -2179,6 +2187,10 @@ fn test_program_sbf_invoke_upgradeable_via_cpi() { &authority_keypair, "solana_sbf_rust_upgraded", ); + bank_client.set_sysvar_for_tests(&clock::Clock { + slot: 2, + ..clock::Clock::default() + }); // Call the upgraded program instruction.data[0] += 1; diff --git a/runtime/src/bank_client.rs b/runtime/src/bank_client.rs index c1cedf620..c553ee86a 100644 --- a/runtime/src/bank_client.rs +++ b/runtime/src/bank_client.rs @@ -14,6 +14,7 @@ use { signature::{Keypair, Signature, Signer}, signers::Signers, system_instruction, + sysvar::{Sysvar, SysvarId}, transaction::{self, Transaction, VersionedTransaction}, transport::{Result, TransportError}, }, @@ -324,6 +325,10 @@ impl BankClient { pub fn new(bank: Bank) -> Self { Self::new_shared(&Arc::new(bank)) } + + pub fn set_sysvar_for_tests(&self, sysvar: &T) { + self.bank.set_sysvar_for_tests(sysvar); + } } #[cfg(test)] diff --git a/sdk/program/src/bpf_loader_upgradeable.rs b/sdk/program/src/bpf_loader_upgradeable.rs index 95e84af0c..f68131334 100644 --- a/sdk/program/src/bpf_loader_upgradeable.rs +++ b/sdk/program/src/bpf_loader_upgradeable.rs @@ -52,6 +52,11 @@ pub enum UpgradeableLoaderState { }, } impl UpgradeableLoaderState { + /// Size of a serialized program account. + pub const fn size_of_uninitialized() -> usize { + 4 // see test_state_size_of_uninitialized + } + /// Size of a buffer account's serialized metadata. pub const fn size_of_buffer_metadata() -> usize { 37 // see test_state_size_of_buffer_metadata @@ -374,6 +379,13 @@ pub fn extend_program( mod tests { use {super::*, bincode::serialized_size}; + #[test] + fn test_state_size_of_uninitialized() { + let buffer_state = UpgradeableLoaderState::Uninitialized; + let size = serialized_size(&buffer_state).unwrap(); + assert_eq!(UpgradeableLoaderState::size_of_uninitialized() as u64, size); + } + #[test] fn test_state_size_of_buffer_metadata() { let buffer_state = UpgradeableLoaderState::Buffer { diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index ca2cec38d..019b178e8 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -550,6 +550,10 @@ pub mod enable_alt_bn128_syscall { solana_sdk::declare_id!("A16q37opZdQMCbe5qJ6xpBB9usykfv8jZaMkxvZQi4GJ"); } +pub mod enable_program_redeployment_cooldown { + solana_sdk::declare_id!("J4HFT8usBxpcF63y46t1upYobJgChmKyZPm5uTBRg25Z"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -682,6 +686,7 @@ lazy_static! { (enable_bpf_loader_set_authority_checked_ix::id(), "enable bpf upgradeable loader SetAuthorityChecked instruction #28424"), (cap_transaction_accounts_data_size::id(), "cap transaction accounts data size up to its compute unit limits #27839"), (enable_alt_bn128_syscall::id(), "add alt_bn128 syscalls #27961"), + (enable_program_redeployment_cooldown::id(), "enable program redeployment cooldown #29135"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()