Optimize account copies and use RefCell to handle duplicate accounts in BPF programs (#7958)

programs/bpf/c/src/dup_accounts/dup_accounts.c

@@ -0,0 +1,57 @@
+/**
+ * @brief Example C-based BPF program that exercises duplicate keyed ka
+ * passed to it
+ */
+#include <solana_sdk.h>
+
+/**
+ * Custom error for when input serialization fails
+ */
+
+extern uint32_t entrypoint(const uint8_t *input) {
+  #define FAILURE 1
+  #define INVALID_INPUT 2
+
+  SolKeyedAccount ka[4];
+  SolParameters params = (SolParameters) { .ka = ka };
+
+  if (!sol_deserialize(input, &params, SOL_ARRAY_SIZE(ka))) {
+    return INVALID_INPUT;
+  }
+
+  switch (params.data[0]) {
+      case(1):
+          sol_log("modify first account userdata");
+          ka[2].userdata[0] = 1;
+          break;
+      case(2):
+          sol_log("modify first account userdata");
+          ka[3].userdata[0] = 2;
+          break;
+      case(3):
+          sol_log("modify both account userdata");
+          ka[2].userdata[0] += 1;
+          ka[3].userdata[0] += 2;
+          break;
+      case(4):
+          sol_log("modify first account lamports");
+          *ka[1].lamports -= 1;
+          *ka[2].lamports += 1;
+          break;
+      case(5):
+          sol_log("modify first account lamports");
+          *ka[1].lamports -= 2;
+          *ka[3].lamports += 2;
+          break;
+      case(6):
+          sol_log("modify both account lamports");
+          *ka[1].lamports -= 3;
+          *ka[2].lamports += 1;
+          *ka[3].lamports += 2;
+          break;
+      default:
+          sol_log("Unrecognized command");
+          return FAILURE;
+  }
+  return SUCCESS;
+}

programs/bpf/rust/dup_accounts/src/lib.rs

@@ -12,32 +12,32 @@ fn process_instruction(_program_id: &Pubkey, accounts: &mut [AccountInfo], data:
     match data[0] {
         1 => {
             info!("modify first account data");
-            accounts[2].data[0] = 1;
+            accounts[2].m.borrow_mut().data[0] = 1;
         }
         2 => {
             info!("modify first account data");
-            accounts[3].data[0] = 2;
+            accounts[3].m.borrow_mut().data[0] = 2;
         }
         3 => {
-            info!("modify both account data, should fail");
-            accounts[2].data[0] = 1;
-            accounts[3].data[0] = 2;
+            info!("modify both account data");
+            accounts[2].m.borrow_mut().data[0] += 1;
+            accounts[3].m.borrow_mut().data[0] += 2;
         }
         4 => {
             info!("modify first account lamports");
-            *accounts[1].lamports -= 1;
-            *accounts[2].lamports += 1;
+            *accounts[1].m.borrow_mut().lamports -= 1;
+            *accounts[2].m.borrow_mut().lamports += 1;
         }
         5 => {
             info!("modify first account lamports");
-            *accounts[1].lamports -= 2;
-            *accounts[3].lamports += 2;
+            *accounts[1].m.borrow_mut().lamports -= 2;
+            *accounts[3].m.borrow_mut().lamports += 2;
         }
         6 => {
-            info!("modify both account lamports, should fail");
-            *accounts[1].lamports -= 1;
-            *accounts[2].lamports += 1;
-            *accounts[3].lamports += 2;
+            info!("modify both account lamports");
+            *accounts[1].m.borrow_mut().lamports -= 3;
+            *accounts[2].m.borrow_mut().lamports += 1;
+            *accounts[3].m.borrow_mut().lamports += 2;
         }
         _ => {
             info!("Unrecognized command");

programs/bpf/rust/external_spend/src/lib.rs

@@ -8,7 +8,7 @@ fn process_instruction(_program_id: &Pubkey, accounts: &mut [AccountInfo], _data
     // account 0 is the mint and not owned by this program, any debit of its lamports
     // should result in a failed program execution.  Test to ensure that this debit
     // is seen by the runtime and fails as expected
-    *accounts[0].lamports -= 1;
+    *accounts[0].m.borrow_mut().lamports -= 1;
 
     SUCCESS
 }

programs/bpf/tests/programs.rs

@@ -27,10 +27,14 @@ mod bpf {
     mod bpf_c {
         use super::*;
         use solana_runtime::loader_utils::create_invoke_instruction;
+        use solana_sdk::account::Account;
         use solana_sdk::bpf_loader;
         use solana_sdk::client::SyncClient;
+        use solana_sdk::instruction::{AccountMeta, Instruction};
         use solana_sdk::signature::KeypairUtil;
         use std::io::Read;
+        use std::sync::Arc;
+        use solana_sdk::pubkey::Pubkey;
 
         #[test]
         fn test_program_bpf_c() {
@@ -72,6 +76,81 @@ mod bpf {
                 }
             }
         }
+
+        #[test]
+        fn test_program_bpf_c_duplicate_accounts() {
+            solana_logger::setup();
+
+            let filename = create_bpf_path("dup_accounts");
+            let mut file = File::open(filename).unwrap();
+            let mut elf = Vec::new();
+            file.read_to_end(&mut elf).unwrap();
+
+            let GenesisConfigInfo {
+                genesis_config,
+                mint_keypair,
+                ..
+            } = create_genesis_config(50);
+
+            let bank = Arc::new(Bank::new(&genesis_config));
+            let bank_client = BankClient::new_shared(&bank);
+            let program_id = load_program(&bank_client, &mint_keypair, &bpf_loader::id(), elf);
+
+            let payee_account = Account::new(10, 1, &program_id);
+            let payee_pubkey = Pubkey::new_rand();
+            bank.store_account(&payee_pubkey, &payee_account);
+
+            let account = Account::new(10, 1, &program_id);
+            let pubkey = Pubkey::new_rand();
+            let account_metas = vec![
+                AccountMeta::new(mint_keypair.pubkey(), true),
+                AccountMeta::new(payee_pubkey, false),
+                AccountMeta::new(pubkey, false),
+                AccountMeta::new(pubkey, false),
+            ];
+
+            bank.store_account(&pubkey, &account);
+            let instruction = Instruction::new(program_id, &1u8, account_metas.clone());
+            let result = bank_client.send_instruction(&mint_keypair, instruction);
+            let data = bank_client.get_account_data(&pubkey).unwrap().unwrap();
+            assert!(result.is_ok());
+            assert_eq!(data[0], 1);
+
+            bank.store_account(&pubkey, &account);
+            let instruction = Instruction::new(program_id, &2u8, account_metas.clone());
+            let result = bank_client.send_instruction(&mint_keypair, instruction);
+            let data = bank_client.get_account_data(&pubkey).unwrap().unwrap();
+            assert!(result.is_ok());
+            assert_eq!(data[0], 2);
+
+            bank.store_account(&pubkey, &account);
+            let instruction = Instruction::new(program_id, &3u8, account_metas.clone());
+            let result = bank_client.send_instruction(&mint_keypair, instruction);
+            let data = bank_client.get_account_data(&pubkey).unwrap().unwrap();
+            assert!(result.is_ok());
+            assert_eq!(data[0], 3);
+
+            bank.store_account(&pubkey, &account);
+            let instruction = Instruction::new(program_id, &4u8, account_metas.clone());
+            let result = bank_client.send_instruction(&mint_keypair, instruction);
+            let lamports = bank_client.get_balance(&pubkey).unwrap();
+            assert!(result.is_ok());
+            assert_eq!(lamports, 11);
+
+            bank.store_account(&pubkey, &account);
+            let instruction = Instruction::new(program_id, &5u8, account_metas.clone());
+            let result = bank_client.send_instruction(&mint_keypair, instruction);
+            let lamports = bank_client.get_balance(&pubkey).unwrap();
+            assert!(result.is_ok());
+            assert_eq!(lamports, 12);
+
+            bank.store_account(&pubkey, &account);
+            let instruction = Instruction::new(program_id, &6u8, account_metas.clone());
+            let result = bank_client.send_instruction(&mint_keypair, instruction);
+            let lamports = bank_client.get_balance(&pubkey).unwrap();
+            assert!(result.is_ok());
+            assert_eq!(lamports, 13);
+        }
     }
 
     #[cfg(feature = "bpf_rust")]
@@ -193,7 +272,9 @@ mod bpf {
             bank.store_account(&pubkey, &account);
             let instruction = Instruction::new(program_id, &3u8, account_metas.clone());
             let result = bank_client.send_instruction(&mint_keypair, instruction);
-            assert!(!result.is_ok());
+            let data = bank_client.get_account_data(&pubkey).unwrap().unwrap();
+            assert!(result.is_ok());
+            assert_eq!(data[0], 3);
 
             bank.store_account(&pubkey, &account);
             let instruction = Instruction::new(program_id, &4u8, account_metas.clone());
@@ -212,7 +293,9 @@ mod bpf {
             bank.store_account(&pubkey, &account);
             let instruction = Instruction::new(program_id, &6u8, account_metas.clone());
             let result = bank_client.send_instruction(&mint_keypair, instruction);
-            assert!(!result.is_ok());
+            let lamports = bank_client.get_balance(&pubkey).unwrap();
+            assert!(result.is_ok());
+            assert_eq!(lamports, 13);
         }
     }
 }

programs/bpf_loader/src/lib.rs

@@ -8,7 +8,6 @@ use log::*;
 use solana_rbpf::{memory_region::MemoryRegion, EbpfVm};
 use solana_sdk::{
     account::KeyedAccount,
-    hash::{Hash, Hasher},
     instruction::InstructionError,
     instruction_processor_utils::{is_executable, limited_deserialize, next_keyed_account},
     loader_instruction::LoaderInstruction,
@@ -16,7 +15,6 @@ use solana_sdk::{
     sysvar::rent,
 };
 use std::{
-    collections::HashMap,
     convert::TryFrom,
     io::{prelude::*, Error},
     mem,
@@ -46,6 +44,16 @@ pub fn check_elf(prog: &[u8]) -> Result<(), Error> {
     Ok(())
 }
 
+/// Look for a duplicate account and return its position if found
+pub fn is_dup(accounts: &[KeyedAccount], keyed_account: &KeyedAccount) -> (bool, usize) {
+    for (i, account) in accounts.iter().enumerate() {
+        if account == keyed_account {
+            return (true, i);
+        }
+    }
+    (false, 0)
+}
+
 pub fn serialize_parameters(
     program_id: &Pubkey,
     keyed_accounts: &[KeyedAccount],
@@ -56,16 +64,22 @@ pub fn serialize_parameters(
     let mut v: Vec<u8> = Vec::new();
     v.write_u64::<LittleEndian>(keyed_accounts.len() as u64)
         .unwrap();
-    for keyed_account in keyed_accounts.iter() {
-        v.write_u64::<LittleEndian>(keyed_account.signer_key().is_some() as u64)
-            .unwrap();
-        v.write_all(keyed_account.unsigned_key().as_ref()).unwrap();
-        v.write_u64::<LittleEndian>(keyed_account.lamports()?)
-            .unwrap();
-        v.write_u64::<LittleEndian>(keyed_account.data_len()? as u64)
-            .unwrap();
-        v.write_all(&keyed_account.try_account_ref()?.data).unwrap();
-        v.write_all(keyed_account.owner()?.as_ref()).unwrap();
+    for (i, keyed_account) in keyed_accounts.iter().enumerate() {
+        let (is_dup, position) = is_dup(&keyed_accounts[..i], keyed_account);
+        if is_dup {
+            v.write_u8(position as u8).unwrap();
+        } else {
+            v.write_u8(0).unwrap();
+            v.write_u64::<LittleEndian>(keyed_account.signer_key().is_some() as u64)
+                .unwrap();
+            v.write_all(keyed_account.unsigned_key().as_ref()).unwrap();
+            v.write_u64::<LittleEndian>(keyed_account.lamports()?)
+                .unwrap();
+            v.write_u64::<LittleEndian>(keyed_account.data_len()? as u64)
+                .unwrap();
+            v.write_all(&keyed_account.try_account_ref()?.data).unwrap();
+            v.write_all(keyed_account.owner()?.as_ref()).unwrap();
+        }
     }
     v.write_u64::<LittleEndian>(data.len() as u64).unwrap();
     v.write_all(data).unwrap();
@@ -79,63 +93,25 @@ pub fn deserialize_parameters(
 ) -> Result<(), InstructionError> {
     assert_eq!(32, mem::size_of::<Pubkey>());
 
-    let calculate_hash = |lamports: u64, data: &[u8]| -> Hash {
-        let mut hasher = Hasher::default();
-        let mut buf = [0u8; 8];
-        LittleEndian::write_u64(&mut buf[..], lamports);
-        hasher.hash(&buf);
-        hasher.hash(data);
-        hasher.result()
-    };
-
-    // remember any duplicate accounts
-    let mut map: HashMap<Pubkey, (Hash, bool)> = HashMap::new();
-    for (i, keyed_account) in keyed_accounts.iter().enumerate() {
-        if keyed_accounts[i + 1..].contains(keyed_account)
-            && !map.contains_key(keyed_account.unsigned_key())
-        {
-            let hash = calculate_hash(
-                keyed_account.lamports()?,
-                &keyed_account.try_account_ref()?.data,
-            );
-            map.insert(*keyed_account.unsigned_key(), (hash, false));
-        }
-    }
-
-    let mut start = mem::size_of::<u64>();
+    let mut start = mem::size_of::<u64>(); // number of accounts
     for keyed_account in keyed_accounts.iter() {
-        start += mem::size_of::<u64>() // signer_key boolean
-            + mem::size_of::<Pubkey>(); // pubkey
-        let lamports = LittleEndian::read_u64(&buffer[start..]);
-        start += mem::size_of::<u64>() // lamports
-            + mem::size_of::<u64>(); // length tag
-        let end = start + keyed_account.data_len()?;
-        let data_start = start;
-        let data_end = end;
-
-        // if duplicate, modified, and dirty, then bail
-        let mut do_update = true;
-        if let Some((hash, is_dirty)) = map.get_mut(keyed_account.unsigned_key()) {
-            let new_hash = calculate_hash(lamports, &buffer[data_start..data_end]);
-            if *hash != new_hash {
-                if *is_dirty {
-                    return Err(InstructionError::DuplicateAccountOutOfSync);
-                }
-                *is_dirty = true; // fail if modified again
-            } else {
-                do_update = false; // no changes, don't need to update account
-            }
-        }
-        if do_update {
-            keyed_account.try_account_ref_mut()?.lamports = lamports;
+        let duplicate = buffer[start] != 0; // duplicate info
+        start += 1;
+        if !duplicate {
+            start += mem::size_of::<u64>(); // is_signer
+            start += mem::size_of::<Pubkey>(); // pubkey
+            keyed_account.try_account_ref_mut()?.lamports =
+                LittleEndian::read_u64(&buffer[start..]);
+            start += mem::size_of::<u64>() // lamports
+                + mem::size_of::<u64>(); // data length
+            let end = start + keyed_account.data_len()?;
             keyed_account
                 .try_account_ref_mut()?
                 .data
-                .clone_from_slice(&buffer[data_start..data_end]);
+                .clone_from_slice(&buffer[start..end]);
+            start += keyed_account.data_len()? // data
+                + mem::size_of::<Pubkey>(); // owner
         }
-
-        start += keyed_account.data_len()? // data
-            + mem::size_of::<Pubkey>(); // owner
     }
     Ok(())
 }