diff --git a/Cargo.lock b/Cargo.lock index d947b63e9f..8bc8778796 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3702,6 +3702,7 @@ dependencies = [ "solana-logger 0.19.0-pre0", "solana-measure 0.19.0-pre0", "solana-metrics 0.19.0-pre0", + "solana-noop-program 0.19.0-pre0", "solana-sdk 0.19.0-pre0", "solana-stake-api 0.19.0-pre0", "solana-stake-program 0.19.0-pre0", diff --git a/runtime/Cargo.toml b/runtime/Cargo.toml index d507acca7f..2a99f92e9a 100644 --- a/runtime/Cargo.toml +++ b/runtime/Cargo.toml @@ -41,3 +41,6 @@ tempfile = "3.1.0" [lib] crate-type = ["lib"] name = "solana_runtime" + +[dev-dependencies] +solana-noop-program = { path = "../programs/noop_program", version = "0.19.0-pre0" } diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index a3fea72cd1..958d3cf66f 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -61,50 +61,49 @@ fn get_subset_unchecked_mut<'a, T>( fn verify_instruction( is_debitable: bool, program_id: &Pubkey, - pre_program_id: &Pubkey, - pre_lamports: u64, - pre_data: &[u8], - pre_executable: bool, - account: &Account, + pre: &Account, + post: &Account, ) -> Result<(), InstructionError> { // Verify the transaction // Make sure that program_id is still the same or this was just assigned by the system program, // but even the system program can't touch a credit-only account - if *pre_program_id != account.owner && (!is_debitable || !system_program::check_id(&program_id)) - { + if pre.owner != post.owner && (!is_debitable || !system_program::check_id(&program_id)) { return Err(InstructionError::ModifiedProgramId); } // For accounts unassigned to the program, the individual balance of each accounts cannot decrease. - if *program_id != account.owner && pre_lamports > account.lamports { + if *program_id != post.owner && pre.lamports > post.lamports { return Err(InstructionError::ExternalAccountLamportSpend); } // The balance of credit-only accounts may only increase - if !is_debitable && pre_lamports > account.lamports { + if !is_debitable && pre.lamports > post.lamports { return Err(InstructionError::CreditOnlyLamportSpend); } // For accounts unassigned to the program, the data may not change. - if *program_id != account.owner - && !system_program::check_id(&program_id) - && pre_data != &account.data[..] + if *program_id != post.owner && !system_program::check_id(&program_id) && pre.data != post.data { return Err(InstructionError::ExternalAccountDataModified); } // Credit-only account data may not change. - if !is_debitable && pre_data != &account.data[..] { + if !is_debitable && pre.data != post.data { return Err(InstructionError::CreditOnlyDataModified); } // executable is one-way (false->true) and // only system or the account owner may modify - if pre_executable != account.executable + if pre.executable != post.executable && (!is_debitable - || pre_executable - || *program_id != account.owner && !system_program::check_id(&program_id)) + || pre.executable + || *program_id != post.owner && !system_program::check_id(&program_id)) { return Err(InstructionError::ExecutableModified); } + // no one modifies rent_epoch (yet) + if pre.rent_epoch != post.rent_epoch { + return Err(InstructionError::RentEpochModified); + } + Ok(()) } @@ -249,36 +248,28 @@ impl MessageProcessor { .iter() .map(|a| u128::from(a.lamports)) .sum(); - let pre_data: Vec<_> = program_accounts + #[allow(clippy::map_clone)] + let pre_accounts: Vec<_> = program_accounts .iter_mut() - .map(|a| (a.owner, a.lamports, a.data.clone(), a.executable)) + .map(|account| account.clone()) // cloned() doesn't work on & & .collect(); self.process_instruction(message, instruction, executable_accounts, program_accounts)?; // Verify the instruction - for ( - (pre_program_id, pre_lamports, pre_data, pre_executable), - (i, post_account, is_debitable), - ) in pre_data.iter().zip(program_accounts.iter().enumerate().map( - |(i, program_account)| { - ( - i, - program_account, - message.is_debitable(instruction.accounts[i] as usize), - ) - }, - )) { - verify_instruction( - is_debitable, - &program_id, - pre_program_id, - *pre_lamports, - pre_data, - *pre_executable, - post_account, - )?; + for (pre_account, (i, post_account, is_debitable)) in + pre_accounts + .iter() + .zip(program_accounts.iter().enumerate().map(|(i, account)| { + ( + i, + account, + message.is_debitable(instruction.accounts[i] as usize), + ) + })) + { + verify_instruction(is_debitable, &program_id, pre_account, post_account)?; if !is_debitable { - *credits[i] += post_account.lamports - *pre_lamports; + *credits[i] += post_account.lamports - pre_account.lamports; } } // The total sum of all the lamports in all the accounts cannot change. @@ -286,6 +277,7 @@ impl MessageProcessor { .iter() .map(|a| u128::from(a.lamports)) .sum(); + if pre_total != post_total { return Err(InstructionError::UnbalancedInstruction); } @@ -379,10 +371,7 @@ mod tests { verify_instruction( is_debitable, &ix, - &pre, - 0, - &[], - false, + &Account::new(0, 0, pre), &Account::new(0, 0, post), ) } @@ -427,17 +416,18 @@ mod tests { pre_executable: bool, post_executable: bool| -> Result<(), InstructionError> { - let mut account = Account::new(0, 0, &alice_program_id); - account.executable = post_executable; - verify_instruction( - is_debitable, - &program_id, - &alice_program_id, - 0, - &[], - pre_executable, - &account, - ) + let pre = Account { + owner: alice_program_id, + executable: pre_executable, + ..Account::default() + }; + + let post = Account { + owner: alice_program_id, + executable: post_executable, + ..Account::default() + }; + verify_instruction(is_debitable, &program_id, &pre, &post) }; let mallory_program_id = Pubkey::new_rand(); @@ -476,16 +466,9 @@ mod tests { let change_data = |program_id: &Pubkey, is_debitable: bool| -> Result<(), InstructionError> { - let account = Account::new(0, 0, &alice_program_id); - verify_instruction( - is_debitable, - &program_id, - &alice_program_id, - 0, - &[42], - false, - &account, - ) + let pre = Account::new(0, 0, &alice_program_id); + let post = Account::new_data(0, &[42], &alice_program_id).unwrap(); + verify_instruction(is_debitable, &program_id, &pre, &post) }; let system_program_id = system_program::id(); @@ -514,33 +497,38 @@ mod tests { ); } + #[test] + fn test_verify_instruction_rent_epoch() { + let alice_program_id = Pubkey::new_rand(); + let pre = Account::new(0, 0, &alice_program_id); + let mut post = Account::new(0, 0, &alice_program_id); + + assert_eq!( + verify_instruction(false, &system_program::id(), &pre, &post), + Ok(()), + "nothing changed!" + ); + + post.rent_epoch += 1; + assert_eq!( + verify_instruction(false, &system_program::id(), &pre, &post), + Err(InstructionError::RentEpochModified), + "no one touches rent_epoch" + ); + } + #[test] fn test_verify_instruction_credit_only() { let alice_program_id = Pubkey::new_rand(); - let account = Account::new(0, 0, &alice_program_id); + let pre = Account::new(42, 0, &alice_program_id); + let post = Account::new(0, 0, &alice_program_id); assert_eq!( - verify_instruction( - false, - &system_program::id(), - &alice_program_id, - 42, - &[], - false, - &account - ), + verify_instruction(false, &system_program::id(), &pre, &post), Err(InstructionError::ExternalAccountLamportSpend), "debit should fail, even if system program" ); assert_eq!( - verify_instruction( - false, - &alice_program_id, - &alice_program_id, - 42, - &[], - false, - &account - ), + verify_instruction(false, &alice_program_id, &pre, &post,), Err(InstructionError::CreditOnlyLamportSpend), "debit should fail, even if owning program" ); diff --git a/sdk/src/instruction.rs b/sdk/src/instruction.rs index 2b3ccb8c16..7e7f9fa79b 100644 --- a/sdk/src/instruction.rs +++ b/sdk/src/instruction.rs @@ -64,6 +64,9 @@ pub enum InstructionError { /// Executable bit on account changed, but shouldn't have ExecutableModified, + /// Rent_epoch account changed, but shouldn't have + RentEpochModified, + /// CustomError allows on-chain programs to implement program-specific error types and see /// them returned by the Solana runtime. A CustomError may be any type that is represented /// as or serialized to a u32 integer. diff --git a/sdk/src/pubkey.rs b/sdk/src/pubkey.rs index 7dc036fb50..5de78e3d6d 100644 --- a/sdk/src/pubkey.rs +++ b/sdk/src/pubkey.rs @@ -122,11 +122,9 @@ macro_rules! solana_name_id( #[cfg(test)] #[test] fn test_name_id() { - // un-comment me to see what the id should look like, given a name - // if id().to_string() != $name { - // panic!("id for `{}` should be `{:?}`", $name, $crate::pubkey::bs58::decode($name).into_vec().unwrap()); - // } - assert_eq!(id().to_string(), $name) + if id().to_string() != $name { + panic!("id for `{}` should be `{:?}`", $name, $crate::pubkey::bs58::decode($name).into_vec().unwrap()); + } } ) );