runtime checks for rent_epoch (#5629)

* runtime checks for rent_epoch

* add actual test

* bigger timeout

* backout 90 min timeout

* new noop

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",

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" }

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: &Account,
-    pre_lamports: u64,
+    post: &Account,
-    pre_data: &[u8],
-    pre_executable: bool,
-    account: &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
+    if *program_id != post.owner && !system_program::check_id(&program_id) && pre.data != post.data
-        && !system_program::check_id(&program_id)
-        && pre_data != &account.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
+            || pre.executable
-            || *program_id != account.owner && !system_program::check_id(&program_id))
+            || *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 (
+        for (pre_account, (i, post_account, is_debitable)) in
-            (pre_program_id, pre_lamports, pre_data, pre_executable),
+            pre_accounts
-            (i, post_account, is_debitable),
+                .iter()
-        ) in pre_data.iter().zip(program_accounts.iter().enumerate().map(
+                .zip(program_accounts.iter().enumerate().map(|(i, account)| {
-            |(i, program_account)| {
+                    (
-                (
+                        i,
-                    i,
+                        account,
-                    program_account,
+                        message.is_debitable(instruction.accounts[i] as usize),
-                    message.is_debitable(instruction.accounts[i] as usize),
+                    )
-                )
+                }))
-            },
+        {
-        )) {
+            verify_instruction(is_debitable, &program_id, pre_account, post_account)?;
-            verify_instruction(
-                is_debitable,
-                &program_id,
-                pre_program_id,
-                *pre_lamports,
-                pre_data,
-                *pre_executable,
-                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,
+                &Account::new(0, 0, pre),
-                0,
-                &[],
-                false,
                 &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);
+            let pre = Account {
-            account.executable = post_executable;
+                owner: alice_program_id,
-            verify_instruction(
+                executable: pre_executable,
-                is_debitable,
+                ..Account::default()
-                &program_id,
+            };
-                &alice_program_id,
+
-                0,
+            let post = Account {
-                &[],
+                owner: alice_program_id,
-                pre_executable,
+                executable: post_executable,
-                &account,
+                ..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);
+                let pre = Account::new(0, 0, &alice_program_id);
-                verify_instruction(
+                let post = Account::new_data(0, &[42], &alice_program_id).unwrap();
-                    is_debitable,
+                verify_instruction(is_debitable, &program_id, &pre, &post)
-                    &program_id,
-                    &alice_program_id,
-                    0,
-                    &[42],
-                    false,
-                    &account,
-                )
             };
 
         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(
+            verify_instruction(false, &system_program::id(), &pre, &post),
-                false,
-                &system_program::id(),
-                &alice_program_id,
-                42,
-                &[],
-                false,
-                &account
-            ),
             Err(InstructionError::ExternalAccountLamportSpend),
             "debit should fail, even if system program"
         );
         assert_eq!(
-            verify_instruction(
+            verify_instruction(false, &alice_program_id, &pre, &post,),
-                false,
-                &alice_program_id,
-                &alice_program_id,
-                42,
-                &[],
-                false,
-                &account
-            ),
             Err(InstructionError::CreditOnlyLamportSpend),
             "debit should fail, even if owning program"
         );

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.

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 {
-            //  if id().to_string() != $name {
+                panic!("id for `{}` should be `{:?}`", $name, $crate::pubkey::bs58::decode($name).into_vec().unwrap());
-            //      panic!("id for `{}` should be `{:?}`", $name, $crate::pubkey::bs58::decode($name).into_vec().unwrap());
+            }
-            //  }
-            assert_eq!(id().to_string(), $name)
         }
     )
 );