Demote write locks on transaction program ids (backport #19593) (#19633)

* Demote write locks on transaction program ids (#19593) * Add feature * Demote write lock on program ids * Fixup bpf tests * Update MappedMessage::is_writable * Comma nit * Review comments (cherry picked from commit decec3c) # Conflicts: # core/src/banking_stage.rs # core/src/cost_model.rs # core/src/cost_tracker.rs # ledger-tool/src/main.rs # program-runtime/src/instruction_processor.rs # programs/bpf/tests/programs.rs # programs/bpf_loader/src/syscalls.rs # rpc/src/transaction_status_service.rs # runtime/src/accounts.rs # runtime/src/bank.rs # runtime/src/message_processor.rs # sdk/benches/serialize_instructions.rs # sdk/program/src/message/mapped.rs # sdk/program/src/message/sanitized.rs # sdk/src/transaction/sanitized.rs * Fix conflicts Co-authored-by: Tyera Eulberg <[email protected]> Co-authored-by: Tyera Eulberg <[email protected]>
solana-labs · Sep 4, 2021 · fcda5d4 · fcda5d4
1 parent 2e4a2c1
commit fcda5d4
Show file tree

Hide file tree

Showing 12 changed files with 227 additions and 160 deletions.
diff --git a/cli-output/src/display.rs b/cli-output/src/display.rs
@@ -140,7 +140,7 @@ fn format_account_mode(message: &Message, index: usize) -> String {
         } else {
             "-"
         },
-        if message.is_writable(index) {
+        if message.is_writable(index, /*demote_program_write_locks=*/ true) {
             "w" // comment for consistent rust fmt (no joking; lol)
         } else {
             "-"

diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs
@@ -19,6 +19,7 @@ use {
         clock::{Clock, Slot},
         entrypoint::{ProgramResult, SUCCESS},
         epoch_schedule::EpochSchedule,
+        feature_set::demote_program_write_locks,
         fee_calculator::{FeeCalculator, FeeRateGovernor},
         genesis_config::{ClusterType, GenesisConfig},
         hash::Hash,
@@ -258,12 +259,14 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
             }
             panic!("Program id {} wasn't found in account_infos", program_id);
         };
+        let demote_program_write_locks =
+            invoke_context.is_feature_active(&demote_program_write_locks::id());
         // TODO don't have the caller's keyed_accounts so can't validate writer or signer escalation or deescalation yet
         let caller_privileges = message
             .account_keys
             .iter()
             .enumerate()
-            .map(|(i, _)| message.is_writable(i))
+            .map(|(i, _)| message.is_writable(i, demote_program_write_locks))
             .collect::<Vec<bool>>();
 
         stable_log::program_invoke(&logger, &program_id, invoke_context.invoke_depth());
@@ -334,7 +337,7 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
 
         // Copy writeable account modifications back into the caller's AccountInfos
         for (i, (pubkey, account)) in accounts.iter().enumerate().take(message.account_keys.len()) {
-            if !message.is_writable(i) {
+            if !message.is_writable(i, demote_program_write_locks) {
                 continue;
             }
             for account_info in account_infos {

diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs
@@ -1782,7 +1782,7 @@ fn test_program_bpf_upgrade_and_invoke_in_same_tx() {
         "solana_bpf_rust_panic",
     );
 
-    // Invoke, then upgrade the program, and then invoke again in same tx
+    // Attempt to invoke, then upgrade the program in same tx
     let message = Message::new(
         &[
             invoke_instruction.clone(),
@@ -1801,10 +1801,12 @@ fn test_program_bpf_upgrade_and_invoke_in_same_tx() {
         message.clone(),
         bank.last_blockhash(),
     );
+    // program_id is automatically demoted to readonly, preventing the upgrade, which requires
+    // writeability
     let (result, _) = process_transaction_and_record_inner(&bank, tx);
     assert_eq!(
         result.unwrap_err(),
-        TransactionError::InstructionError(2, InstructionError::ProgramFailedToComplete)
+        TransactionError::InstructionError(1, InstructionError::InvalidArgument)
     );
 }
 
@@ -2103,97 +2105,6 @@ fn test_program_bpf_upgrade_via_cpi() {
     assert_ne!(programdata, original_programdata);
 }
 
-#[cfg(feature = "bpf_rust")]
-#[test]
-fn test_program_bpf_upgrade_self_via_cpi() {
-    solana_logger::setup();
-
-    let GenesisConfigInfo {
-        genesis_config,
-        mint_keypair,
-        ..
-    } = create_genesis_config(50);
-    let mut bank = Bank::new(&genesis_config);
-    let (name, id, entrypoint) = solana_bpf_loader_program!();
-    bank.add_builtin(&name, id, entrypoint);
-    let (name, id, entrypoint) = solana_bpf_loader_upgradeable_program!();
-    bank.add_builtin(&name, id, entrypoint);
-    let bank = Arc::new(bank);
-    let bank_client = BankClient::new_shared(&bank);
-    let noop_program_id = load_bpf_program(
-        &bank_client,
-        &bpf_loader::id(),
-        &mint_keypair,
-        "solana_bpf_rust_noop",
-    );
-
-    // Deploy upgradeable program
-    let buffer_keypair = Keypair::new();
-    let program_keypair = Keypair::new();
-    let program_id = program_keypair.pubkey();
-    let authority_keypair = Keypair::new();
-    load_upgradeable_bpf_program(
-        &bank_client,
-        &mint_keypair,
-        &buffer_keypair,
-        &program_keypair,
-        &authority_keypair,
-        "solana_bpf_rust_invoke_and_return",
-    );
-
-    let mut invoke_instruction = Instruction::new_with_bytes(
-        program_id,
-        &[0],
-        vec![
-            AccountMeta::new(noop_program_id, false),
-            AccountMeta::new(noop_program_id, false),
-            AccountMeta::new(clock::id(), false),
-            AccountMeta::new(fees::id(), false),
-        ],
-    );
-
-    // Call the upgraded program
-    invoke_instruction.data[0] += 1;
-    let result =
-        bank_client.send_and_confirm_instruction(&mint_keypair, invoke_instruction.clone());
-    assert!(result.is_ok());
-
-    // Prepare for upgrade
-    let buffer_keypair = Keypair::new();
-    load_upgradeable_buffer(
-        &bank_client,
-        &mint_keypair,
-        &buffer_keypair,
-        &authority_keypair,
-        "solana_bpf_rust_panic",
-    );
-
-    // Invoke, then upgrade the program, and then invoke again in same tx
-    let message = Message::new(
-        &[
-            invoke_instruction.clone(),
-            bpf_loader_upgradeable::upgrade(
-                &program_id,
-                &buffer_keypair.pubkey(),
-                &authority_keypair.pubkey(),
-                &mint_keypair.pubkey(),
-            ),
-            invoke_instruction,
-        ],
-        Some(&mint_keypair.pubkey()),
-    );
-    let tx = Transaction::new(
-        &[&mint_keypair, &authority_keypair],
-        message.clone(),
-        bank.last_blockhash(),
-    );
-    let (result, _) = process_transaction_and_record_inner(&bank, tx);
-    assert_eq!(
-        result.unwrap_err(),
-        TransactionError::InstructionError(2, InstructionError::ProgramFailedToComplete)
-    );
-}
-
 #[cfg(feature = "bpf_rust")]
 #[test]
 fn test_program_bpf_set_upgrade_authority_via_cpi() {

diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs
@@ -19,9 +19,9 @@ use solana_sdk::{
     entrypoint::{MAX_PERMITTED_DATA_INCREASE, SUCCESS},
     epoch_schedule::EpochSchedule,
     feature_set::{
-        close_upgradeable_program_accounts, cpi_data_cost, enforce_aligned_host_addrs,
-        keccak256_syscall_enabled, libsecp256k1_0_5_upgrade_enabled, mem_overlap_fix,
-        memory_ops_syscalls, secp256k1_recover_syscall_enabled,
+        close_upgradeable_program_accounts, cpi_data_cost, demote_program_write_locks,
+        enforce_aligned_host_addrs, keccak256_syscall_enabled, libsecp256k1_0_5_upgrade_enabled,
+        mem_overlap_fix, memory_ops_syscalls, secp256k1_recover_syscall_enabled,
         set_upgrade_authority_via_cpi_enabled, sysvar_via_syscall, update_data_on_realloc,
     },
     hash::{Hasher, HASH_BYTES},
@@ -2244,7 +2244,14 @@ fn call<'a>(
     signers_seeds_len: u64,
     memory_mapping: &MemoryMapping,
 ) -> Result<u64, EbpfError<BpfError>> {
-    let (message, executables, accounts, account_refs, caller_write_privileges) = {
+    let (
+        message,
+        executables,
+        accounts,
+        account_refs,
+        caller_write_privileges,
+        demote_program_write_locks,
+    ) = {
         let invoke_context = syscall.get_context()?;
 
         invoke_context
@@ -2335,6 +2342,7 @@ fn call<'a>(
             accounts,
             account_refs,
             caller_write_privileges,
+            invoke_context.is_feature_active(&demote_program_write_locks::id()),
         )
     };
 
@@ -2360,7 +2368,7 @@ fn call<'a>(
         for (i, ((_key, account), account_ref)) in accounts.iter().zip(account_refs).enumerate() {
             let account = account.borrow();
             if let Some(mut account_ref) = account_ref {
-                if message.is_writable(i) && !account.executable() {
+                if message.is_writable(i, demote_program_write_locks) && !account.executable() {
                     *account_ref.lamports = account.lamports();
                     *account_ref.owner = *account.owner();
                     if account_ref.data.len() != account.data().len() {

diff --git a/rpc/src/transaction_status_service.rs b/rpc/src/transaction_status_service.rs
@@ -112,8 +112,9 @@ impl TransactionStatusService {
                             })
                             .expect("FeeCalculator must exist");
                         let fee = fee_calculator.calculate_fee(transaction.message());
-                        let (writable_keys, readonly_keys) =
-                            transaction.message.get_account_keys_by_lock_type();
+                        let (writable_keys, readonly_keys) = transaction
+                            .message
+                            .get_account_keys_by_lock_type(bank.demote_program_write_locks());
 
                         let inner_instructions = inner_instructions.map(|inner_instructions| {
                             inner_instructions