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

…port #19633) (#19637) * 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]> (cherry picked from commit fcda5d4) # Conflicts: # cli-output/src/display.rs # core/src/transaction_status_service.rs # program-test/src/lib.rs # programs/bpf_loader/src/syscalls.rs # runtime/src/accounts.rs # runtime/src/bank.rs # runtime/src/message_processor.rs # sdk/benches/serialize_instructions.rs # sdk/program/src/message.rs # sdk/src/feature_set.rs # transaction-status/src/parse_accounts.rs * Replace feature and fix conflicts Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Tyera Eulberg <[email protected]>
solana-labs · Sep 4, 2021 · 53f8e58 · 53f8e58
1 parent b4fdce9
commit 53f8e58
Show file tree

Hide file tree

Showing 12 changed files with 198 additions and 241 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, /*demote_sysvar_write_locks=*/ true) {
+        if message.is_writable(index, /*demote_program_write_locks=*/ true) {
             "w" // comment for consistent rust fmt (no joking; lol)
         } else {
             "-"

diff --git a/core/src/transaction_status_service.rs b/core/src/transaction_status_service.rs
@@ -111,7 +111,7 @@ impl TransactionStatusService {
                         let fee = fee_calculator.calculate_fee(transaction.message());
                         let (writable_keys, readonly_keys) = transaction
                             .message
-                            .get_account_keys_by_lock_type(bank.demote_sysvar_write_locks());
+                            .get_account_keys_by_lock_type(bank.demote_program_write_locks());
 
                         let inner_instructions = inner_instructions.map(|inner_instructions| {
                             inner_instructions

diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs
@@ -19,7 +19,7 @@ use {
         clock::{Clock, Slot},
         entrypoint::{ProgramResult, SUCCESS},
         epoch_schedule::EpochSchedule,
-        feature_set::demote_sysvar_write_locks,
+        feature_set::demote_program_write_locks,
         fee_calculator::{FeeCalculator, FeeRateGovernor},
         genesis_config::{ClusterType, GenesisConfig},
         hash::Hash,
@@ -256,14 +256,14 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
             }
             panic!("Program id {} wasn't found in account_infos", program_id);
         };
-        let demote_sysvar_write_locks =
-            invoke_context.is_feature_active(&demote_sysvar_write_locks::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, demote_sysvar_write_locks))
+            .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 +334,7 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
 
         // Copy writeable account modifications back into the caller's AccountInfos
         for (i, account_pubkey) in message.account_keys.iter().enumerate() {
-            if !message.is_writable(i, true) {
+            if !message.is_writable(i, demote_program_write_locks) {
                 continue;
             }
 

diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs
@@ -1773,7 +1773,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(),
@@ -1792,10 +1792,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)
     );
 }
 
@@ -2076,97 +2078,6 @@ fn test_program_bpf_upgrade_via_cpi() {
     );
 }
 
-#[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,7 +19,7 @@ use solana_sdk::{
     entrypoint::{MAX_PERMITTED_DATA_INCREASE, SUCCESS},
     epoch_schedule::EpochSchedule,
     feature_set::{
-        cpi_data_cost, cpi_share_ro_and_exec_accounts, demote_sysvar_write_locks,
+        cpi_data_cost, cpi_share_ro_and_exec_accounts, demote_program_write_locks,
         enforce_aligned_host_addrs, keccak256_syscall_enabled, memory_ops_syscalls,
         set_upgrade_authority_via_cpi_enabled, sysvar_via_syscall, update_data_on_realloc,
     },
@@ -2146,7 +2146,7 @@ fn call<'a>(
         accounts,
         account_refs,
         caller_write_privileges,
-        demote_sysvar_write_locks,
+        demote_program_write_locks,
     ) = {
         let invoke_context = syscall.get_context()?;
 
@@ -2237,7 +2237,7 @@ fn call<'a>(
             accounts,
             account_refs,
             caller_write_privileges,
-            invoke_context.is_feature_active(&demote_sysvar_write_locks::id()),
+            invoke_context.is_feature_active(&demote_program_write_locks::id()),
         )
     };
 
@@ -2263,9 +2263,9 @@ fn call<'a>(
         for (i, (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, demote_sysvar_write_locks) && !account.executable {
-                    *account_ref.lamports = account.lamports;
-                    *account_ref.owner = account.owner;
+                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() {
                         if !account_ref.data.is_empty() {
                             // Only support for `CreateAccount` at this time.