Cleanup after simplify_writable_program_account_check feature activation (#34840)

runtime/src/accounts/mod.rs

@@ -28,12 +28,7 @@ use {
             create_executable_meta, is_builtin, is_executable, Account, AccountSharedData,
             ReadableAccount, WritableAccount,
         },
-        account_utils::StateMut,
+        feature_set::{include_loaded_accounts_data_size_in_fee_calculation, FeatureSet},
-        bpf_loader_upgradeable::{self, UpgradeableLoaderState},
-        feature_set::{
-            include_loaded_accounts_data_size_in_fee_calculation,
-            simplify_writable_program_account_check, FeatureSet,
-        },
         fee::FeeStructure,
         message::SanitizedMessage,
         native_loader,
@@ -195,12 +190,9 @@ fn load_transaction_accounts(
                     account_overrides.and_then(|overrides| overrides.get(key))
                 {
                     (account_override.data().len(), account_override.clone(), 0)
-                } else if let Some(program) = (feature_set
+                } else if let Some(program) = (!instruction_account && !message.is_writable(i))
-                    .is_active(&simplify_writable_program_account_check::id())
+                    .then_some(())
-                    && !instruction_account
+                    .and_then(|_| loaded_programs.find(key))
-                    && !message.is_writable(i))
-                .then_some(())
-                .and_then(|_| loaded_programs.find(key))
                 {
                     // Optimization to skip loading of accounts which are only used as
                     // programs in top-level instructions and not passed as instruction accounts.
@@ -275,41 +267,6 @@ fn load_transaction_accounts(
                     validated_fee_payer = true;
                 }
 
-                if !feature_set.is_active(&simplify_writable_program_account_check::id()) {
-                    if bpf_loader_upgradeable::check_id(account.owner()) {
-                        if message.is_writable(i) && !message.is_upgradeable_loader_present() {
-                            error_counters.invalid_writable_account += 1;
-                            return Err(TransactionError::InvalidWritableAccount);
-                        }
-
-                        if is_builtin(&account) || is_executable(&account, feature_set) {
-                            // The upgradeable loader requires the derived ProgramData account
-                            if let Ok(UpgradeableLoaderState::Program {
-                                programdata_address,
-                            }) = account.state()
-                            {
-                                if accounts_db
-                                    .load_with_fixed_root(ancestors, &programdata_address)
-                                    .is_none()
-                                {
-                                    error_counters.account_not_found += 1;
-                                    return Err(TransactionError::ProgramAccountNotFound);
-                                }
-                            } else {
-                                error_counters.invalid_program_for_execution += 1;
-                                return Err(TransactionError::InvalidProgramForExecution);
-                            }
-                        }
-                    } else {
-                        if (is_builtin(&account) || is_executable(&account, feature_set))
-                            && message.is_writable(i)
-                        {
-                            error_counters.invalid_writable_account += 1;
-                            return Err(TransactionError::InvalidWritableAccount);
-                        }
-                    }
-                }
-
                 if in_reward_interval
                     && message.is_writable(i)
                     && solana_stake_program::check_id(account.owner())
@@ -548,6 +505,7 @@ mod tests {
         },
         solana_sdk::{
             account::{AccountSharedData, WritableAccount},
+            bpf_loader_upgradeable,
             compute_budget::ComputeBudgetInstruction,
             epoch_schedule::EpochSchedule,
             hash::Hash,
@@ -1045,309 +1003,6 @@ mod tests {
         }
     }
 
-    #[test]
-    fn test_load_accounts_executable_with_write_lock() {
-        let mut accounts: Vec<TransactionAccount> = Vec::new();
-        let mut error_counters = TransactionErrorMetrics::default();
-
-        let keypair = Keypair::new();
-        let key0 = keypair.pubkey();
-        let key1 = Pubkey::from([5u8; 32]);
-        let key2 = Pubkey::from([6u8; 32]);
-
-        let mut account = AccountSharedData::new(1, 0, &Pubkey::default());
-        account.set_rent_epoch(1);
-        accounts.push((key0, account));
-
-        let mut account = AccountSharedData::new(40, 1, &native_loader::id());
-        account.set_executable(true);
-        account.set_rent_epoch(1);
-        accounts.push((key1, account));
-
-        let mut account = AccountSharedData::new(40, 1, &native_loader::id());
-        account.set_executable(true);
-        account.set_rent_epoch(1);
-        accounts.push((key2, account));
-
-        let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])];
-        let mut message = Message::new_with_compiled_instructions(
-            1,
-            0,
-            1, // only one executable marked as readonly
-            vec![key0, key1, key2],
-            Hash::default(),
-            instructions,
-        );
-        let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
-        let loaded_accounts = load_accounts_with_excluded_features(
-            tx,
-            &accounts,
-            &mut error_counters,
-            Some(&[simplify_writable_program_account_check::id()]),
-        );
-
-        assert_eq!(error_counters.invalid_writable_account, 1);
-        assert_eq!(loaded_accounts.len(), 1);
-        assert_eq!(
-            loaded_accounts[0],
-            (Err(TransactionError::InvalidWritableAccount), None)
-        );
-
-        // Mark executables as readonly
-        message.account_keys = vec![key0, key1, key2]; // revert key change
-        message.header.num_readonly_unsigned_accounts = 2; // mark both executables as readonly
-        let tx = Transaction::new(&[&keypair], message, Hash::default());
-        let loaded_accounts = load_accounts_with_excluded_features(
-            tx,
-            &accounts,
-            &mut error_counters,
-            Some(&[simplify_writable_program_account_check::id()]),
-        );
-
-        assert_eq!(error_counters.invalid_writable_account, 1);
-        assert_eq!(loaded_accounts.len(), 1);
-        let result = loaded_accounts[0].0.as_ref().unwrap();
-        assert_eq!(result.accounts[..2], accounts[..2]);
-        assert_eq!(
-            result.accounts[result.program_indices[0][0] as usize],
-            accounts[2]
-        );
-    }
-
-    #[test]
-    fn test_load_accounts_upgradeable_with_write_lock() {
-        let mut accounts: Vec<TransactionAccount> = Vec::new();
-        let mut error_counters = TransactionErrorMetrics::default();
-
-        let keypair = Keypair::new();
-        let key0 = keypair.pubkey();
-        let key1 = Pubkey::from([5u8; 32]);
-        let key2 = Pubkey::from([6u8; 32]);
-        let programdata_key1 = Pubkey::from([7u8; 32]);
-        let programdata_key2 = Pubkey::from([8u8; 32]);
-
-        let mut account = AccountSharedData::new(1, 0, &Pubkey::default());
-        account.set_rent_epoch(1);
-        accounts.push((key0, account));
-
-        let program_data = UpgradeableLoaderState::ProgramData {
-            slot: 42,
-            upgrade_authority_address: None,
-        };
-
-        let program = UpgradeableLoaderState::Program {
-            programdata_address: programdata_key1,
-        };
-        let mut account =
-            AccountSharedData::new_data(40, &program, &bpf_loader_upgradeable::id()).unwrap();
-        account.set_executable(true);
-        account.set_rent_epoch(1);
-        accounts.push((key1, account));
-        let mut account =
-            AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap();
-        account.set_rent_epoch(1);
-        accounts.push((programdata_key1, account));
-
-        let program = UpgradeableLoaderState::Program {
-            programdata_address: programdata_key2,
-        };
-        let mut account =
-            AccountSharedData::new_data(40, &program, &bpf_loader_upgradeable::id()).unwrap();
-        account.set_executable(true);
-        account.set_rent_epoch(1);
-        accounts.push((key2, account));
-        let mut account =
-            AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap();
-        account.set_rent_epoch(1);
-        accounts.push((programdata_key2, account));
-
-        let mut account = AccountSharedData::new(40, 1, &native_loader::id()); // create mock bpf_loader_upgradeable
-        account.set_executable(true);
-        account.set_rent_epoch(1);
-        accounts.push((bpf_loader_upgradeable::id(), account));
-
-        let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])];
-        let mut message = Message::new_with_compiled_instructions(
-            1,
-            0,
-            1, // only one executable marked as readonly
-            vec![key0, key1, key2],
-            Hash::default(),
-            instructions,
-        );
-        let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
-        let loaded_accounts = load_accounts_with_excluded_features(
-            tx.clone(),
-            &accounts,
-            &mut error_counters,
-            Some(&[simplify_writable_program_account_check::id()]),
-        );
-
-        assert_eq!(error_counters.invalid_writable_account, 1);
-        assert_eq!(loaded_accounts.len(), 1);
-        assert_eq!(
-            loaded_accounts[0],
-            (Err(TransactionError::InvalidWritableAccount), None)
-        );
-
-        // Solution 0: Include feature simplify_writable_program_account_check
-        let loaded_accounts =
-            load_accounts_with_excluded_features(tx, &accounts, &mut error_counters, None);
-
-        assert_eq!(error_counters.invalid_writable_account, 1);
-        assert_eq!(loaded_accounts.len(), 1);
-
-        // Solution 1: include bpf_loader_upgradeable account
-        message.account_keys = vec![key0, key1, bpf_loader_upgradeable::id()];
-        let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
-        let loaded_accounts = load_accounts_with_excluded_features(
-            tx,
-            &accounts,
-            &mut error_counters,
-            Some(&[simplify_writable_program_account_check::id()]),
-        );
-
-        assert_eq!(error_counters.invalid_writable_account, 1);
-        assert_eq!(loaded_accounts.len(), 1);
-        let result = loaded_accounts[0].0.as_ref().unwrap();
-        assert_eq!(result.accounts[..2], accounts[..2]);
-        assert_eq!(
-            result.accounts[result.program_indices[0][0] as usize],
-            accounts[5]
-        );
-
-        // Solution 2: mark programdata as readonly
-        message.account_keys = vec![key0, key1, key2]; // revert key change
-        message.header.num_readonly_unsigned_accounts = 2; // mark both executables as readonly
-        let tx = Transaction::new(&[&keypair], message, Hash::default());
-        let loaded_accounts = load_accounts_with_excluded_features(
-            tx,
-            &accounts,
-            &mut error_counters,
-            Some(&[simplify_writable_program_account_check::id()]),
-        );
-
-        assert_eq!(error_counters.invalid_writable_account, 1);
-        assert_eq!(loaded_accounts.len(), 1);
-        let result = loaded_accounts[0].0.as_ref().unwrap();
-        assert_eq!(result.accounts[..2], accounts[..2]);
-        assert_eq!(
-            result.accounts[result.program_indices[0][0] as usize],
-            accounts[5]
-        );
-        assert_eq!(
-            result.accounts[result.program_indices[0][1] as usize],
-            accounts[3]
-        );
-    }
-
-    #[test]
-    fn test_load_accounts_programdata_with_write_lock() {
-        let mut accounts: Vec<TransactionAccount> = Vec::new();
-        let mut error_counters = TransactionErrorMetrics::default();
-
-        let keypair = Keypair::new();
-        let key0 = keypair.pubkey();
-        let key1 = Pubkey::from([5u8; 32]);
-        let key2 = Pubkey::from([6u8; 32]);
-
-        let mut account = AccountSharedData::new(1, 0, &Pubkey::default());
-        account.set_rent_epoch(1);
-        accounts.push((key0, account));
-
-        let program_data = UpgradeableLoaderState::ProgramData {
-            slot: 42,
-            upgrade_authority_address: None,
-        };
-        let mut account =
-            AccountSharedData::new_data(40, &program_data, &bpf_loader_upgradeable::id()).unwrap();
-        account.set_rent_epoch(1);
-        accounts.push((key1, account));
-
-        let mut account = AccountSharedData::new(40, 1, &native_loader::id());
-        account.set_executable(true);
-        account.set_rent_epoch(1);
-        accounts.push((key2, account));
-
-        let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])];
-        let mut message = Message::new_with_compiled_instructions(
-            1,
-            0,
-            1, // only the program marked as readonly
-            vec![key0, key1, key2],
-            Hash::default(),
-            instructions,
-        );
-        let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
-        let loaded_accounts = load_accounts_with_excluded_features(
-            tx.clone(),
-            &accounts,
-            &mut error_counters,
-            Some(&[simplify_writable_program_account_check::id()]),
-        );
-
-        assert_eq!(error_counters.invalid_writable_account, 1);
-        assert_eq!(loaded_accounts.len(), 1);
-        assert_eq!(
-            loaded_accounts[0],
-            (Err(TransactionError::InvalidWritableAccount), None)
-        );
-
-        // Solution 0: Include feature simplify_writable_program_account_check
-        let loaded_accounts =
-            load_accounts_with_excluded_features(tx, &accounts, &mut error_counters, None);
-
-        assert_eq!(error_counters.invalid_writable_account, 1);
-        assert_eq!(loaded_accounts.len(), 1);
-
-        // Solution 1: include bpf_loader_upgradeable account
-        let mut account = AccountSharedData::new(40, 1, &native_loader::id()); // create mock bpf_loader_upgradeable
-        account.set_executable(true);
-        account.set_rent_epoch(1);
-        let accounts_with_upgradeable_loader = vec![
-            accounts[0].clone(),
-            accounts[1].clone(),
-            (bpf_loader_upgradeable::id(), account),
-        ];
-        message.account_keys = vec![key0, key1, bpf_loader_upgradeable::id()];
-        let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
-        let loaded_accounts = load_accounts_with_excluded_features(
-            tx,
-            &accounts_with_upgradeable_loader,
-            &mut error_counters,
-            Some(&[simplify_writable_program_account_check::id()]),
-        );
-
-        assert_eq!(error_counters.invalid_writable_account, 1);
-        assert_eq!(loaded_accounts.len(), 1);
-        let result = loaded_accounts[0].0.as_ref().unwrap();
-        assert_eq!(result.accounts[..2], accounts_with_upgradeable_loader[..2]);
-        assert_eq!(
-            result.accounts[result.program_indices[0][0] as usize],
-            accounts_with_upgradeable_loader[2]
-        );
-
-        // Solution 2: mark programdata as readonly
-        message.account_keys = vec![key0, key1, key2]; // revert key change
-        message.header.num_readonly_unsigned_accounts = 2; // extend readonly set to include programdata
-        let tx = Transaction::new(&[&keypair], message, Hash::default());
-        let loaded_accounts = load_accounts_with_excluded_features(
-            tx,
-            &accounts,
-            &mut error_counters,
-            Some(&[simplify_writable_program_account_check::id()]),
-        );
-
-        assert_eq!(error_counters.invalid_writable_account, 1);
-        assert_eq!(loaded_accounts.len(), 1);
-        let result = loaded_accounts[0].0.as_ref().unwrap();
-        assert_eq!(result.accounts[..2], accounts[..2]);
-        assert_eq!(
-            result.accounts[result.program_indices[0][0] as usize],
-            accounts[2]
-        );
-    }
-
     fn load_accounts_no_store(
         accounts: &Accounts,
         tx: Transaction,

runtime/src/bank/tests.rs

@@ -1036,10 +1036,9 @@ fn test_rent_exempt_executable_account() {
         transfer_lamports,
         genesis_config.hash(),
     );
-
+    assert_matches!(
-    assert_eq!(
         bank.process_transaction(&tx),
-        Err(TransactionError::InvalidWritableAccount)
+        Err(TransactionError::InstructionError(0, _))
     );
     assert_eq!(bank.get_balance(&account_pubkey), account_balance);
 }