Remove redundant check during loading upgradeable program for writing (…

…#30561) * Remove redundant check during loading upgradeable program * Apply clippy suggestion
solana-labs · Mar 28, 2023 · 035c974 · 035c974
1 parent aebc191
commit 035c974
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 13 deletions.
diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs
@@ -41,7 +41,8 @@ use {
             delay_visibility_of_program_deployment, enable_request_heap_frame_ix,
             include_loaded_accounts_data_size_in_fee_calculation,
             remove_congestion_multiplier_from_fee_calculation, remove_deprecated_request_unit_ix,
-            use_default_units_in_fee_calculation, FeatureSet,
+            simplify_writable_program_account_check, use_default_units_in_fee_calculation,
+            FeatureSet,
         },
         fee::FeeStructure,
         genesis_config::ClusterType,
@@ -458,7 +459,10 @@ impl Accounts {
                     }
 
                     if bpf_loader_upgradeable::check_id(account.owner()) {
-                        if message.is_writable(i) && !message.is_upgradeable_loader_present() {
+                        if !feature_set.is_active(&simplify_writable_program_account_check::id())
+                            && message.is_writable(i)
+                            && !message.is_upgradeable_loader_present()
+                        {
                             error_counters.invalid_writable_account += 1;
                             return Err(TransactionError::InvalidWritableAccount);
                         }
@@ -2537,8 +2541,12 @@ mod tests {
             instructions,
         );
         let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
-        let loaded_accounts =
-            load_accounts_with_excluded_features(tx, &accounts, &mut error_counters, None);
+        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);
@@ -2547,11 +2555,22 @@ mod tests {
             (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, None);
+        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);
@@ -2566,8 +2585,12 @@ mod tests {
         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, None);
+        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);
@@ -2621,8 +2644,12 @@ mod tests {
             instructions,
         );
         let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
-        let loaded_accounts =
-            load_accounts_with_excluded_features(tx, &accounts, &mut error_counters, None);
+        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);
@@ -2631,6 +2658,13 @@ mod tests {
             (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);
@@ -2646,7 +2680,7 @@ mod tests {
             tx,
             &accounts_with_upgradeable_loader,
             &mut error_counters,
-            None,
+            Some(&[simplify_writable_program_account_check::id()]),
         );
 
         assert_eq!(error_counters.invalid_writable_account, 1);
@@ -2662,8 +2696,12 @@ mod tests {
         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, None);
+        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);

diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs
@@ -641,6 +641,9 @@ pub mod include_loaded_accounts_data_size_in_fee_calculation {
 pub mod native_programs_consume_cu {
     solana_sdk::declare_id!("8pgXCMNXC8qyEFypuwpXyRxLXZdpM4Qo72gJ6k87A6wL");
 }
+pub mod simplify_writable_program_account_check {
+    solana_sdk::declare_id!("5ZCcFAzJ1zsFKe1KSZa9K92jhx7gkcKj97ci2DBo1vwj");
+}
 
 lazy_static! {
     /// Map of feature identifiers to user-visible description
@@ -797,6 +800,7 @@ lazy_static! {
         (remove_bpf_loader_incorrect_program_id::id(), "stop incorrectly throwing IncorrectProgramId in bpf_loader #30747"),
         (include_loaded_accounts_data_size_in_fee_calculation::id(), "include transaction loaded accounts data size in base fee calculation #30657"),
         (native_programs_consume_cu::id(), "Native program should consume compute units #30620"),
+        (simplify_writable_program_account_check::id(), "Simplify checks performed for writable upgradeable program accounts #30559"),
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()