diff --git a/bench-exchange/src/bench.rs b/bench-exchange/src/bench.rs index 54326b024..2f5e81a9f 100644 --- a/bench-exchange/src/bench.rs +++ b/bench-exchange/src/bench.rs @@ -646,6 +646,20 @@ where false } +fn verify_funding_transfer( + client: &T, + tx: &Transaction, + amount: u64, +) -> bool { + for a in &tx.message().account_keys[1..] { + if client.get_balance(a).unwrap_or(0) >= amount { + return true; + } + } + + false +} + pub fn fund_keys(client: &Client, source: &Keypair, dests: &[Arc], lamports: u64) { let total = lamports * (dests.len() as u64 + 1); let mut funded: Vec<(&Keypair, u64)> = vec![(source, total)]; @@ -703,6 +717,7 @@ pub fn fund_keys(client: &Client, source: &Keypair, dests: &[Arc], lamp .collect(); let mut retries = 0; + let amount = chunk[0].1[0].1; while !to_fund_txs.is_empty() { let receivers = to_fund_txs .iter() @@ -731,7 +746,7 @@ pub fn fund_keys(client: &Client, source: &Keypair, dests: &[Arc], lamp let mut waits = 0; loop { sleep(Duration::from_millis(200)); - to_fund_txs.retain(|(_, tx)| !verify_transfer(client, &tx)); + to_fund_txs.retain(|(_, tx)| !verify_funding_transfer(client, &tx, amount)); if to_fund_txs.is_empty() { break; } diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index c953c6c40..9b58b6d40 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -435,7 +435,7 @@ impl BankingStage { // the likelihood of any single thread getting starved and processing old ids. // TODO: Banking stage threads should be prioritized to complete faster then this queue // expires. - let (loaded_accounts, results, mut retryable_txs) = + let (mut loaded_accounts, results, mut retryable_txs) = bank.load_and_execute_transactions(txs, lock_results, MAX_PROCESSING_AGE); let load_execute_time = now.elapsed(); @@ -454,7 +454,7 @@ impl BankingStage { let commit_time = { let now = Instant::now(); - bank.commit_transactions(txs, &loaded_accounts, &results); + bank.commit_transactions(txs, &mut loaded_accounts, &results); now.elapsed() }; @@ -1500,7 +1500,7 @@ mod tests { let bank = Arc::new(Bank::new(&genesis_block)); let pubkey = Pubkey::new_rand(); - let transactions = vec![system_transaction::transfer( + let transactions = vec![system_transaction::create_user_account( &mint_keypair, &pubkey, 1, diff --git a/core/src/replay_stage.rs b/core/src/replay_stage.rs index 4201eb053..b82c62f4e 100644 --- a/core/src/replay_stage.rs +++ b/core/src/replay_stage.rs @@ -608,6 +608,7 @@ impl ReplayStage { slot_full_sender: &Sender<(u64, Pubkey)>, ) { bank.freeze(); + bank.commit_credits(); info!("bank frozen {}", bank.slot()); if let Err(e) = slot_full_sender.send((bank.slot(), *bank.collector_id())) { trace!("{} slot_full alert failed: {:?}", my_pubkey, e); diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index e9121723f..8b3a86d7b 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -9,8 +9,9 @@ use crate::message_processor::has_duplicates; use bincode::serialize; use log::*; use solana_metrics::inc_new_counter_error; -use solana_sdk::account::{Account, LamportCredit}; +use solana_sdk::account::Account; use solana_sdk::hash::{Hash, Hasher}; +use solana_sdk::message::Message; use solana_sdk::native_loader; use solana_sdk::pubkey::Pubkey; use solana_sdk::signature::{Keypair, KeypairUtil}; @@ -24,21 +25,30 @@ use std::fs::remove_dir_all; use std::io::{BufReader, Read}; use std::ops::Neg; use std::path::Path; -use std::sync::atomic::{AtomicUsize, Ordering}; -use std::sync::{Arc, Mutex}; +use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; +use std::sync::{Arc, Mutex, RwLock}; const ACCOUNTSDB_DIR: &str = "accountsdb"; const NUM_ACCOUNT_DIRS: usize = 4; +#[derive(Default, Debug)] +struct CreditOnlyLock { + credits: AtomicU64, + lock_count: Mutex, +} + /// This structure handles synchronization for db #[derive(Default, Debug)] pub struct Accounts { /// Single global AccountsDB pub accounts_db: Arc, - /// set of accounts which are currently in the pipeline + /// set of credit-debit accounts which are currently in the pipeline account_locks: Mutex>, + /// Set of credit-only accounts which are currently in the pipeline, caching account balance and number of locks + credit_only_account_locks: Arc>>, + /// List of persistent stores pub paths: String, @@ -98,6 +108,7 @@ impl Accounts { Accounts { accounts_db, account_locks: Mutex::new(HashSet::new()), + credit_only_account_locks: Arc::new(RwLock::new(HashMap::new())), paths, own_paths, } @@ -107,6 +118,7 @@ impl Accounts { Accounts { accounts_db, account_locks: Mutex::new(HashSet::new()), + credit_only_account_locks: Arc::new(RwLock::new(HashMap::new())), paths: parent.paths.clone(), own_paths: parent.own_paths, } @@ -142,7 +154,7 @@ impl Accounts { // If a fee can pay for execution then the program will be scheduled let mut called_accounts: Vec = vec![]; let mut credits: InstructionCredits = vec![]; - for key in &message.account_keys { + for key in message.account_keys.iter() { if !message.program_ids().contains(&key) { called_accounts.push( AccountsDB::load(storage, ancestors, accounts_index, key) @@ -320,36 +332,85 @@ impl Accounts { /// Slow because lock is held for 1 operation instead of many pub fn store_slow(&self, fork: Fork, pubkey: &Pubkey, account: &Account) { let mut accounts = HashMap::new(); - accounts.insert(pubkey, (account, 0)); + accounts.insert(pubkey, account); self.accounts_db.store(fork, &accounts); } fn lock_account( locks: &mut HashSet, - keys: &[&Pubkey], + credit_only_locks: &Arc>>, + message: &Message, error_counters: &mut ErrorCounters, ) -> Result<()> { - // Copy all the accounts - for k in keys { + let (credit_debit_keys, credit_only_keys) = message.get_account_keys_by_lock_type(); + + for k in credit_debit_keys.iter() { + let credit_only_locks = credit_only_locks.read().unwrap(); + if locks.contains(k) + || credit_only_locks + .get(&k) + .map_or(false, |lock| *lock.lock_count.lock().unwrap() > 0) + { + error_counters.account_in_use += 1; + debug!("Account in use: {:?}", k); + return Err(TransactionError::AccountInUse); + } + } + for k in credit_only_keys.iter() { if locks.contains(k) { error_counters.account_in_use += 1; debug!("Account in use: {:?}", k); return Err(TransactionError::AccountInUse); } } - for k in keys { - locks.insert(**k); + + for k in credit_debit_keys { + locks.insert(*k); } + let mut credit_only_writes: Vec<&Pubkey> = vec![]; + for k in credit_only_keys { + let credit_only_locks = credit_only_locks.read().unwrap(); + if let Some(credit_only_lock) = credit_only_locks.get(&k) { + *credit_only_lock.lock_count.lock().unwrap() += 1; + } else { + credit_only_writes.push(k); + } + } + + for k in credit_only_writes.iter() { + let mut credit_only_locks = credit_only_locks.write().unwrap(); + assert!(credit_only_locks.get(&k).is_none()); + credit_only_locks.insert( + **k, + CreditOnlyLock { + credits: AtomicU64::new(0), + lock_count: Mutex::new(1), + }, + ); + } + Ok(()) } - fn unlock_account(tx: &Transaction, result: &Result<()>, locks: &mut HashSet) { + fn unlock_account( + tx: &Transaction, + result: &Result<()>, + locks: &mut HashSet, + credit_only_locks: &Arc>>, + ) { + let (credit_debit_keys, credit_only_keys) = &tx.message().get_account_keys_by_lock_type(); match result { Err(TransactionError::AccountInUse) => (), _ => { - for k in &tx.message().account_keys { + for k in credit_debit_keys { locks.remove(k); } + for k in credit_only_keys { + let locks = credit_only_locks.read().unwrap(); + if let Some(lock) = locks.get(&k) { + *lock.lock_count.lock().unwrap() -= 1; + } + } } } } @@ -401,7 +462,8 @@ impl Accounts { let message = &tx.message(); Self::lock_account( &mut self.account_locks.lock().unwrap(), - &message.get_account_keys_by_lock_type().0, + &self.credit_only_account_locks, + &message, &mut error_counters, ) }) @@ -420,10 +482,11 @@ impl Accounts { /// Once accounts are unlocked, new transactions that modify that state can enter the pipeline pub fn unlock_accounts(&self, txs: &[Transaction], results: &[Result<()>]) { let mut account_locks = self.account_locks.lock().unwrap(); + let credit_only_locks = self.credit_only_account_locks.clone(); debug!("bank unlock accounts"); - txs.iter() - .zip(results.iter()) - .for_each(|(tx, result)| Self::unlock_account(tx, result, &mut account_locks)); + txs.iter().zip(results.iter()).for_each(|(tx, result)| { + Self::unlock_account(tx, result, &mut account_locks, &credit_only_locks) + }); } pub fn has_accounts(&self, fork: Fork) -> bool { @@ -436,10 +499,18 @@ impl Accounts { fork: Fork, txs: &[Transaction], res: &[Result<()>], - loaded: &[Result<(InstructionAccounts, InstructionLoaders, InstructionCredits)>], + loaded: &mut [Result<(InstructionAccounts, InstructionLoaders, InstructionCredits)>], ) { - let accounts = collect_accounts(txs, res, loaded); - self.accounts_db.store(fork, &accounts); + let accounts = self.collect_accounts(txs, res, loaded); + // Only store credit-debit accounts immediately + let mut accounts_to_store: HashMap<&Pubkey, &Account> = HashMap::new(); + + for (pubkey, (account, is_debitable)) in accounts.iter() { + if *is_debitable { + accounts_to_store.insert(pubkey, account); + } + } + self.accounts_db.store(fork, &accounts_to_store); } /// Purge a fork if it is not a root @@ -451,39 +522,70 @@ impl Accounts { pub fn add_root(&self, fork: Fork) { self.accounts_db.add_root(fork) } -} -fn collect_accounts<'a>( - txs: &'a [Transaction], - res: &'a [Result<()>], - loaded: &'a [Result<(InstructionAccounts, InstructionLoaders, InstructionCredits)>], -) -> HashMap<&'a Pubkey, (&'a Account, LamportCredit)> { - let mut accounts: HashMap<&Pubkey, (&Account, LamportCredit)> = HashMap::new(); - for (i, raccs) in loaded.iter().enumerate() { - if res[i].is_err() || raccs.is_err() { - continue; + /// Commit remaining credit-only changes, regardless of reference count + pub fn commit_credits(&self, ancestors: &HashMap, fork: Fork) { + let credit_only_account_locks = self.credit_only_account_locks.read().unwrap(); + for (pubkey, credit_only_lock) in credit_only_account_locks.iter() { + let credit = credit_only_lock.credits.load(Ordering::Relaxed); + if credit > 0 { + let mut account = self + .load_slow(ancestors, pubkey) + .map(|(account, _)| account) + .unwrap_or_default(); + account.lamports += credit; + self.store_slow(fork, pubkey, &account); + } } + drop(credit_only_account_locks); + self.clear_credit_only_account_locks(); + } - let message = &txs[i].message(); - let acc = raccs.as_ref().unwrap(); - for ((key, account), credit) in message - .account_keys - .iter() - .zip(acc.0.iter()) - .zip(acc.2.iter()) - { - if !accounts.contains_key(key) { - accounts.insert(key, (account, 0)); - } else if *credit > 0 { - // Credit-only accounts may be referenced by multiple transactions - // Collect credits to update account lamport balance before store. - if let Some((_, c)) = accounts.get_mut(key) { - *c += credit; + pub fn clear_credit_only_account_locks(&self) { + let mut credit_only_account_locks = self.credit_only_account_locks.write().unwrap(); + credit_only_account_locks.clear(); + } + + fn collect_accounts<'a>( + &self, + txs: &'a [Transaction], + res: &'a [Result<()>], + loaded: &'a mut [Result<(InstructionAccounts, InstructionLoaders, InstructionCredits)>], + ) -> HashMap<&'a Pubkey, (&'a Account, bool)> { + let mut accounts: HashMap<&Pubkey, (&Account, bool)> = HashMap::new(); + for (i, raccs) in loaded.iter_mut().enumerate() { + if res[i].is_err() || raccs.is_err() { + continue; + } + + let message = &txs[i].message(); + let acc = raccs.as_mut().unwrap(); + for (((i, key), account), credit) in message + .account_keys + .iter() + .enumerate() + .zip(acc.0.iter()) + .zip(acc.2.iter()) + { + if !accounts.contains_key(key) { + accounts.insert(key, (account, message.is_debitable(i))); + } + if *credit > 0 { + // Increment credit-only account balance Atomic + if accounts.get_mut(key).is_some() { + self.credit_only_account_locks + .read() + .unwrap() + .get(key) + .unwrap() + .credits + .fetch_add(*credit, Ordering::Relaxed); + } } } } + accounts } - accounts } #[cfg(test)] @@ -501,6 +603,8 @@ mod tests { use solana_sdk::syscall; use solana_sdk::transaction::Transaction; use std::io::Cursor; + use std::sync::atomic::AtomicBool; + use std::{thread, time}; fn load_accounts_with_fee( tx: Transaction, @@ -637,7 +741,7 @@ mod tests { assert_eq!(error_counters.insufficient_funds, 1); assert_eq!(loaded_accounts.len(), 1); assert_eq!( - loaded_accounts[0], + loaded_accounts[0].clone(), Err(TransactionError::InsufficientFundsForFee) ); } @@ -930,7 +1034,6 @@ mod tests { assert_eq!(error_counters.account_loaded_twice, 1); assert_eq!(loaded_accounts.len(), 1); - loaded_accounts[0].clone().unwrap_err(); assert_eq!( loaded_accounts[0], Err(TransactionError::AccountLoadedTwice) @@ -1033,28 +1136,276 @@ mod tests { ); } + #[test] + fn test_accounts_locks() { + let keypair0 = Keypair::new(); + let keypair1 = Keypair::new(); + let keypair2 = Keypair::new(); + let keypair3 = Keypair::new(); + + let account0 = Account::new(1, 0, &Pubkey::default()); + let account1 = Account::new(2, 0, &Pubkey::default()); + let account2 = Account::new(3, 0, &Pubkey::default()); + let account3 = Account::new(4, 0, &Pubkey::default()); + + let accounts = Accounts::new(None); + accounts.store_slow(0, &keypair0.pubkey(), &account0); + accounts.store_slow(0, &keypair1.pubkey(), &account1); + accounts.store_slow(0, &keypair2.pubkey(), &account2); + accounts.store_slow(0, &keypair3.pubkey(), &account3); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let message = Message::new_with_compiled_instructions( + 1, + 0, + 2, + vec![keypair0.pubkey(), keypair1.pubkey(), native_loader::id()], + Hash::default(), + instructions, + ); + let tx = Transaction::new(&[&keypair0], message, Hash::default()); + let results0 = accounts.lock_accounts(&[tx.clone()]); + + assert!(results0[0].is_ok()); + assert_eq!( + *accounts + .credit_only_account_locks + .read() + .unwrap() + .get(&keypair1.pubkey()) + .unwrap() + .lock_count + .lock() + .unwrap(), + 1 + ); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let message = Message::new_with_compiled_instructions( + 1, + 0, + 2, + vec![keypair2.pubkey(), keypair1.pubkey(), native_loader::id()], + Hash::default(), + instructions, + ); + let tx0 = Transaction::new(&[&keypair2], message, Hash::default()); + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let message = Message::new_with_compiled_instructions( + 1, + 0, + 2, + vec![keypair1.pubkey(), keypair3.pubkey(), native_loader::id()], + Hash::default(), + instructions, + ); + let tx1 = Transaction::new(&[&keypair1], message, Hash::default()); + let txs = vec![tx0, tx1]; + let results1 = accounts.lock_accounts(&txs); + + assert!(results1[0].is_ok()); // Credit-only account (keypair1) can be referenced multiple times + assert!(results1[1].is_err()); // Credit-only account (keypair1) cannot also be locked as credit-debit + assert_eq!( + *accounts + .credit_only_account_locks + .read() + .unwrap() + .get(&keypair1.pubkey()) + .unwrap() + .lock_count + .lock() + .unwrap(), + 2 + ); + + accounts.unlock_accounts(&[tx], &results0); + accounts.unlock_accounts(&txs, &results1); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let message = Message::new_with_compiled_instructions( + 1, + 0, + 2, + vec![keypair1.pubkey(), keypair3.pubkey(), native_loader::id()], + Hash::default(), + instructions, + ); + let tx = Transaction::new(&[&keypair1], message, Hash::default()); + let results2 = accounts.lock_accounts(&[tx]); + + assert!(results2[0].is_ok()); // Now keypair1 account can be locked as credit-debit + + // Check that credit-only credits are still cached in accounts struct + let credit_only_account_locks = accounts.credit_only_account_locks.read().unwrap(); + let keypair1_lock = credit_only_account_locks.get(&keypair1.pubkey()); + assert!(keypair1_lock.is_some()); + assert_eq!(*keypair1_lock.unwrap().lock_count.lock().unwrap(), 0); + } + + #[test] + fn test_accounts_locks_multithreaded() { + let counter = Arc::new(AtomicU64::new(0)); + let exit = Arc::new(AtomicBool::new(false)); + + let keypair0 = Keypair::new(); + let keypair1 = Keypair::new(); + let keypair2 = Keypair::new(); + + let account0 = Account::new(1, 0, &Pubkey::default()); + let account1 = Account::new(2, 0, &Pubkey::default()); + let account2 = Account::new(3, 0, &Pubkey::default()); + + let accounts = Accounts::new(None); + accounts.store_slow(0, &keypair0.pubkey(), &account0); + accounts.store_slow(0, &keypair1.pubkey(), &account1); + accounts.store_slow(0, &keypair2.pubkey(), &account2); + + let accounts_arc = Arc::new(accounts); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let credit_only_message = Message::new_with_compiled_instructions( + 1, + 0, + 2, + vec![keypair0.pubkey(), keypair1.pubkey(), native_loader::id()], + Hash::default(), + instructions, + ); + let credit_only_tx = Transaction::new(&[&keypair0], credit_only_message, Hash::default()); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let credit_debit_message = Message::new_with_compiled_instructions( + 1, + 0, + 2, + vec![keypair1.pubkey(), keypair2.pubkey(), native_loader::id()], + Hash::default(), + instructions, + ); + let credit_debit_tx = Transaction::new(&[&keypair1], credit_debit_message, Hash::default()); + + let counter_clone = counter.clone(); + let accounts_clone = accounts_arc.clone(); + let exit_clone = exit.clone(); + thread::spawn(move || { + let counter_clone = counter_clone.clone(); + let exit_clone = exit_clone.clone(); + loop { + let txs = vec![credit_debit_tx.clone()]; + let results = accounts_clone.clone().lock_accounts(&txs); + for result in results.iter() { + if result.is_ok() { + counter_clone.clone().fetch_add(1, Ordering::SeqCst); + } + } + accounts_clone.unlock_accounts(&txs, &results); + if exit_clone.clone().load(Ordering::Relaxed) { + break; + } + } + }); + let counter_clone = counter.clone(); + for _ in 0..5 { + let txs = vec![credit_only_tx.clone()]; + let results = accounts_arc.clone().lock_accounts(&txs); + if results[0].is_ok() { + let counter_value = counter_clone.clone().load(Ordering::SeqCst); + thread::sleep(time::Duration::from_millis(50)); + assert_eq!(counter_value, counter_clone.clone().load(Ordering::SeqCst)); + } + accounts_arc.unlock_accounts(&txs, &results); + thread::sleep(time::Duration::from_millis(50)); + } + exit.store(true, Ordering::Relaxed); + } + + #[test] + fn test_commit_credits() { + let pubkey0 = Pubkey::new_rand(); + let pubkey1 = Pubkey::new_rand(); + let pubkey2 = Pubkey::new_rand(); + + let account0 = Account::new(1, 0, &Pubkey::default()); + let account1 = Account::new(2, 0, &Pubkey::default()); + + let accounts = Accounts::new(None); + accounts.store_slow(0, &pubkey0, &account0); + accounts.store_slow(0, &pubkey1, &account1); + + let mut credit_only_account_locks = accounts.credit_only_account_locks.write().unwrap(); + credit_only_account_locks.insert( + pubkey0, + CreditOnlyLock { + credits: AtomicU64::new(0), + lock_count: Mutex::new(1), + }, + ); + credit_only_account_locks.insert( + pubkey1, + CreditOnlyLock { + credits: AtomicU64::new(5), + lock_count: Mutex::new(1), + }, + ); + credit_only_account_locks.insert( + pubkey2, + CreditOnlyLock { + credits: AtomicU64::new(10), + lock_count: Mutex::new(1), + }, + ); + + drop(credit_only_account_locks); + + let ancestors = vec![(0, 0)].into_iter().collect(); + accounts.commit_credits(&ancestors, 0); + + // No change when CreditOnlyLock credits are 0 + assert_eq!( + accounts.load_slow(&ancestors, &pubkey0).unwrap().0.lamports, + 1 + ); + // New balance should equal previous balance plus CreditOnlyLock credits + assert_eq!( + accounts.load_slow(&ancestors, &pubkey1).unwrap().0.lamports, + 7 + ); + // New account should be created + assert_eq!( + accounts.load_slow(&ancestors, &pubkey2).unwrap().0.lamports, + 10 + ); + // Account locks should be cleared + assert_eq!(accounts.credit_only_account_locks.read().unwrap().len(), 0); + } + #[test] fn test_collect_accounts() { let keypair0 = Keypair::new(); let keypair1 = Keypair::new(); let pubkey = Pubkey::new_rand(); - let instructions = vec![CompiledInstruction::new(0, &(), vec![0, 1])]; - let tx0 = Transaction::new_with_compiled_instructions( - &[&keypair0], - &[pubkey], + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let message = Message::new_with_compiled_instructions( + 1, + 0, + 2, + vec![keypair0.pubkey(), pubkey, native_loader::id()], Hash::default(), - vec![native_loader::id()], instructions, ); - let instructions = vec![CompiledInstruction::new(0, &(), vec![0, 1])]; - let tx1 = Transaction::new_with_compiled_instructions( - &[&keypair1], - &[pubkey], + let tx0 = Transaction::new(&[&keypair0], message, Hash::default()); + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let message = Message::new_with_compiled_instructions( + 1, + 0, + 2, + vec![keypair1.pubkey(), pubkey, native_loader::id()], Hash::default(), - vec![native_loader::id()], instructions, ); + let tx1 = Transaction::new(&[&keypair1], message, Hash::default()); let txs = vec![tx0, tx1]; let loaders = vec![Ok(()), Ok(())]; @@ -1074,27 +1425,47 @@ mod tests { let instruction_accounts1 = vec![account1, account2.clone()]; let instruction_loaders1 = vec![]; - let instruction_credits1 = vec![2, 3]; + let instruction_credits1 = vec![0, 3]; let loaded1 = Ok(( instruction_accounts1, instruction_loaders1, instruction_credits1, )); - let loaded = vec![loaded0, loaded1]; + let mut loaded = vec![loaded0, loaded1]; - let accounts = collect_accounts(&txs, &loaders, &loaded); - assert_eq!(accounts.len(), 3); - assert!(accounts.contains_key(&keypair0.pubkey())); - assert!(accounts.contains_key(&keypair1.pubkey())); - assert!(accounts.contains_key(&pubkey)); + let accounts = Accounts::new(None); + let mut credit_only_locks = accounts.credit_only_account_locks.write().unwrap(); + credit_only_locks.insert( + pubkey, + CreditOnlyLock { + credits: AtomicU64::new(0), + lock_count: Mutex::new(1), + }, + ); + drop(credit_only_locks); + let collected_accounts = accounts.collect_accounts(&txs, &loaders, &mut loaded); + assert_eq!(collected_accounts.len(), 3); + assert!(collected_accounts.contains_key(&keypair0.pubkey())); + assert!(collected_accounts.contains_key(&keypair1.pubkey())); + assert!(collected_accounts.contains_key(&pubkey)); - // Accounts referenced once are assumed to have the correct lamport balance, even if a - // credit amount is reported (as in a credit-only account) - assert_eq!(accounts.get(&keypair0.pubkey()).unwrap().1, 0); - assert_eq!(accounts.get(&keypair1.pubkey()).unwrap().1, 0); - // Accounts referenced more than once need to pass the accumulated credits of additional - // references on to store - assert_eq!(accounts.get(&pubkey).unwrap().1, 3); + let credit_debit_account0 = collected_accounts.get(&keypair0.pubkey()).unwrap(); + assert_eq!(credit_debit_account0.1, true); + let credit_debit_account1 = collected_accounts.get(&keypair1.pubkey()).unwrap(); + assert_eq!(credit_debit_account1.1, true); + + let credit_only_account = collected_accounts.get(&pubkey).unwrap(); + assert_eq!(credit_only_account.1, false); + // Ensure credit_only_lock reflects credits from both accounts: 2 + 3 = 5 + let credit_only_locks = accounts.credit_only_account_locks.read().unwrap(); + assert_eq!( + credit_only_locks + .get(&pubkey) + .unwrap() + .credits + .load(Ordering::Relaxed), + 5 + ); } } diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index b479ba562..a540fafd2 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -480,11 +480,11 @@ impl AccountsDB { fn store_accounts( &self, fork_id: Fork, - accounts: &HashMap<&Pubkey, (&Account, LamportCredit)>, + accounts: &HashMap<&Pubkey, &Account>, ) -> Vec { - let with_meta: Vec<(StorageMeta, &Account, u64)> = accounts + let with_meta: Vec<(StorageMeta, &Account)> = accounts .iter() - .map(|(pubkey, (account, credit))| { + .map(|(pubkey, account)| { let write_version = self.write_version.fetch_add(1, Ordering::Relaxed) as u64; let data_len = if account.lamports == 0 { 0 @@ -497,14 +497,7 @@ impl AccountsDB { data_len, }; - let mut lamports: u64 = account.lamports; - if *credit > 0 { - // Only credit-only accounts with multiple transaction credits will have credit - // values greater than 0. Update lamports to collect all credits. - lamports += credit; - } - - (meta, *account, lamports) + (meta, *account) }) .collect(); let mut infos: Vec = vec![]; @@ -515,12 +508,12 @@ impl AccountsDB { storage.set_status(AccountStorageStatus::Full); continue; } - for (offset, (_, _, lamports)) in rvs.iter().zip(&with_meta[infos.len()..]) { + for (offset, (_, account)) in rvs.iter().zip(&with_meta[infos.len()..]) { storage.add_account(); infos.push(AccountInfo { id: storage.id, offset: *offset, - lamports: *lamports, + lamports: account.lamports, }); } // restore the state to available @@ -533,7 +526,7 @@ impl AccountsDB { &self, fork_id: Fork, infos: Vec, - accounts: &HashMap<&Pubkey, (&Account, LamportCredit)>, + accounts: &HashMap<&Pubkey, &Account>, ) -> (Vec<(Fork, AccountInfo)>, u64) { let mut reclaims: Vec<(Fork, AccountInfo)> = Vec::with_capacity(infos.len() * 2); let mut index = self.accounts_index.write().unwrap(); @@ -588,7 +581,7 @@ impl AccountsDB { } /// Store the account update. - pub fn store(&self, fork_id: Fork, accounts: &HashMap<&Pubkey, (&Account, LamportCredit)>) { + pub fn store(&self, fork_id: Fork, accounts: &HashMap<&Pubkey, &Account>) { let infos = self.store_accounts(fork_id, accounts); let (reclaims, last_root) = self.update_index(fork_id, infos, accounts); @@ -746,7 +739,7 @@ mod tests { let key = Pubkey::default(); let account0 = Account::new(1, 0, &key); - db.store(0, &hashmap!(&key => (&account0, 0))); + db.store(0, &hashmap!(&key => &account0)); db.add_root(0); let ancestors = vec![(1, 1)].into_iter().collect(); assert_eq!(db.load_slow(&ancestors, &key), Some((account0, 0))); @@ -760,10 +753,10 @@ mod tests { let key = Pubkey::default(); let account0 = Account::new(1, 0, &key); - db.store(0, &hashmap!(&key => (&account0, 0))); + db.store(0, &hashmap!(&key => &account0)); let account1 = Account::new(0, 0, &key); - db.store(1, &hashmap!(&key => (&account1, 0))); + db.store(1, &hashmap!(&key => &account1)); let ancestors = vec![(1, 1)].into_iter().collect(); assert_eq!(&db.load_slow(&ancestors, &key).unwrap().0, &account1); @@ -780,10 +773,10 @@ mod tests { let key = Pubkey::default(); let account0 = Account::new(1, 0, &key); - db.store(0, &hashmap!(&key => (&account0, 0))); + db.store(0, &hashmap!(&key => &account0)); let account1 = Account::new(0, 0, &key); - db.store(1, &hashmap!(&key => (&account1, 0))); + db.store(1, &hashmap!(&key => &account1)); db.add_root(0); let ancestors = vec![(1, 1)].into_iter().collect(); @@ -802,7 +795,7 @@ mod tests { let account0 = Account::new(1, 0, &key); // store value 1 in the "root", i.e. db zero - db.store(0, &hashmap!(&key => (&account0, 0))); + db.store(0, &hashmap!(&key => &account0)); // now we have: // @@ -815,7 +808,7 @@ mod tests { // store value 0 in one child let account1 = Account::new(0, 0, &key); - db.store(1, &hashmap!(&key => (&account1, 0))); + db.store(1, &hashmap!(&key => &account1)); // masking accounts is done at the Accounts level, at accountsDB we see // original account (but could also accept "None", which is implemented @@ -886,8 +879,8 @@ mod tests { let pubkey = Pubkey::new_rand(); let account = Account::new(1, ACCOUNT_DATA_FILE_SIZE as usize / 3, &pubkey); - db.store(1, &hashmap!(&pubkey => (&account, 0))); - db.store(1, &hashmap!(&pubkeys[0] => (&account, 0))); + db.store(1, &hashmap!(&pubkey => &account)); + db.store(1, &hashmap!(&pubkeys[0] => &account)); { let stores = db.storage.read().unwrap(); let fork_0_stores = &stores.0.get(&0).unwrap(); @@ -917,11 +910,11 @@ mod tests { let paths = get_tmp_accounts_path!(); let db0 = AccountsDB::new(&paths.paths); let account0 = Account::new(1, 0, &key); - db0.store(0, &hashmap!(&key => (&account0, 0))); + db0.store(0, &hashmap!(&key => &account0)); // 0 lamports in the child let account1 = Account::new(0, 0, &key); - db0.store(1, &hashmap!(&key => (&account1, 0))); + db0.store(1, &hashmap!(&key => &account1)); // masking accounts is done at the Accounts level, at accountsDB we see // original account @@ -931,23 +924,6 @@ mod tests { assert_eq!(db0.load_slow(&ancestors, &key), Some((account0, 0))); } - #[test] - fn test_credit_check() { - let paths = get_tmp_accounts_path!(); - let db = AccountsDB::new(&paths.paths); - let pubkey = Pubkey::default(); - let account = Account::new(1, 0, &pubkey); - db.store(0, &hashmap!(&pubkey => (&account, 0))); - - // Load account with lamport credit - let account_new = Account::new(5, 0, &pubkey); - db.store(0, &hashmap!(&pubkey => (&account_new, 4))); - - let ancestors = vec![(0, 0)].into_iter().collect(); - let (account_load, _) = db.load_slow(&ancestors, &pubkey).unwrap(); - assert_eq!(account_load.lamports, 9); - } - fn create_account( accounts: &AccountsDB, pubkeys: &mut Vec, @@ -962,7 +938,7 @@ mod tests { pubkeys.push(pubkey.clone()); let ancestors = vec![(fork, 0)].into_iter().collect(); assert!(accounts.load_slow(&ancestors, &pubkey).is_none()); - accounts.store(fork, &hashmap!(&pubkey => (&account, 0))); + accounts.store(fork, &hashmap!(&pubkey => &account)); } for t in 0..num_vote { let pubkey = Pubkey::new_rand(); @@ -970,7 +946,7 @@ mod tests { pubkeys.push(pubkey.clone()); let ancestors = vec![(fork, 0)].into_iter().collect(); assert!(accounts.load_slow(&ancestors, &pubkey).is_none()); - accounts.store(fork, &hashmap!(&pubkey => (&account, 0))); + accounts.store(fork, &hashmap!(&pubkey => &account)); } } @@ -980,7 +956,7 @@ mod tests { let ancestors = vec![(fork, 0)].into_iter().collect(); if let Some((mut account, _)) = accounts.load_slow(&ancestors, &pubkeys[idx]) { account.lamports = account.lamports + 1; - accounts.store(fork, &hashmap!(&pubkeys[idx] => (&account, 0))); + accounts.store(fork, &hashmap!(&pubkeys[idx] => &account)); if account.lamports == 0 { let ancestors = vec![(fork, 0)].into_iter().collect(); assert!(accounts.load_slow(&ancestors, &pubkeys[idx]).is_none()); @@ -1031,7 +1007,7 @@ mod tests { ) { for idx in 0..num { let account = Account::new((idx + count) as u64, 0, &Account::default().owner); - accounts.store(fork, &hashmap!(&pubkeys[idx] => (&account, 0))); + accounts.store(fork, &hashmap!(&pubkeys[idx] => &account)); } } @@ -1076,7 +1052,7 @@ mod tests { for i in 0..9 { let key = Pubkey::new_rand(); let account = Account::new(i + 1, size as usize / 4, &key); - accounts.store(0, &hashmap!(&key => (&account, 0))); + accounts.store(0, &hashmap!(&key => &account)); keys.push(key); } for (i, key) in keys.iter().enumerate() { @@ -1111,7 +1087,7 @@ mod tests { let status = [AccountStorageStatus::Available, AccountStorageStatus::Full]; let pubkey1 = Pubkey::new_rand(); let account1 = Account::new(1, ACCOUNT_DATA_FILE_SIZE as usize / 2, &pubkey1); - accounts.store(0, &hashmap!(&pubkey1 => (&account1, 0))); + accounts.store(0, &hashmap!(&pubkey1 => &account1)); { let stores = accounts.storage.read().unwrap(); assert_eq!(stores.0.len(), 1); @@ -1121,7 +1097,7 @@ mod tests { let pubkey2 = Pubkey::new_rand(); let account2 = Account::new(1, ACCOUNT_DATA_FILE_SIZE as usize / 2, &pubkey2); - accounts.store(0, &hashmap!(&pubkey2 => (&account2, 0))); + accounts.store(0, &hashmap!(&pubkey2 => &account2)); { let stores = accounts.storage.read().unwrap(); assert_eq!(stores.0.len(), 1); @@ -1144,7 +1120,7 @@ mod tests { // lots of stores, but 3 storages should be enough for everything for i in 0..25 { let index = i % 2; - accounts.store(0, &hashmap!(&pubkey1 => (&account1, 0))); + accounts.store(0, &hashmap!(&pubkey1 => &account1)); { let stores = accounts.storage.read().unwrap(); assert_eq!(stores.0.len(), 1); @@ -1202,7 +1178,7 @@ mod tests { let pubkey = Pubkey::new_rand(); let account = Account::new(1, 0, &Account::default().owner); //store an account - accounts.store(0, &hashmap!(&pubkey => (&account, 0))); + accounts.store(0, &hashmap!(&pubkey => &account)); let ancestors = vec![(0, 0)].into_iter().collect(); let info = accounts .accounts_index @@ -1222,7 +1198,7 @@ mod tests { .is_some()); //store causes cleanup - accounts.store(1, &hashmap!(&pubkey => (&account, 0))); + accounts.store(1, &hashmap!(&pubkey => &account)); //fork is gone assert!(accounts.storage.read().unwrap().0.get(&0).is_none()); @@ -1296,7 +1272,7 @@ mod tests { loop { let account_bal = thread_rng().gen_range(1, 99); account.lamports = account_bal; - db.store(fork_id, &hashmap!(&pubkey => (&account, 0))); + db.store(fork_id, &hashmap!(&pubkey => &account)); let (account, fork) = db.load_slow(&HashMap::new(), &pubkey).expect( &format!("Could not fetch stored account {}, iter {}", pubkey, i), ); diff --git a/runtime/src/append_vec.rs b/runtime/src/append_vec.rs index 65eb0d892..b5c393a74 100644 --- a/runtime/src/append_vec.rs +++ b/runtime/src/append_vec.rs @@ -226,13 +226,13 @@ impl AppendVec { } #[allow(clippy::mutex_atomic)] - pub fn append_accounts(&self, accounts: &[(StorageMeta, &Account, u64)]) -> Vec { + pub fn append_accounts(&self, accounts: &[(StorageMeta, &Account)]) -> Vec { let mut offset = self.append_offset.lock().unwrap(); let mut rv = vec![]; - for (storage_meta, account, lamports) in accounts { + for (storage_meta, account) in accounts { let meta_ptr = storage_meta as *const StorageMeta; let balance = AccountBalance { - lamports: *lamports, + lamports: account.lamports, owner: account.owner, executable: account.executable, }; @@ -254,7 +254,7 @@ impl AppendVec { } pub fn append_account(&self, storage_meta: StorageMeta, account: &Account) -> Option { - self.append_accounts(&[(storage_meta, account, account.lamports)]) + self.append_accounts(&[(storage_meta, account)]) .first() .cloned() } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 828cd947e..2683890a2 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -345,6 +345,7 @@ impl Bank { self.collector_id = *collector_id; self.rc.accounts = Arc::new(Accounts::new_from_parent(&parent.rc.accounts)); + self.clear_credit_only_account_locks(); self.epoch_stakes = { let mut epoch_stakes = parent.epoch_stakes.clone(); @@ -742,10 +743,12 @@ impl Bank { } } - /// Process a Transaction. This is used for unit tests and simply calls the vector Bank::process_transactions method. + /// Process a Transaction. This is used for unit tests and simply calls the vector + /// Bank::process_transactions method, and commits credit-only credits. pub fn process_transaction(&self, tx: &Transaction) -> Result<()> { let txs = vec![tx.clone()]; self.process_transactions(&txs)[0].clone()?; + self.commit_credits(); tx.signatures .get(0) .map_or(Ok(()), |sig| self.get_signature_status(sig).unwrap()) @@ -1063,7 +1066,11 @@ impl Bank { pub fn commit_transactions( &self, txs: &[Transaction], - loaded_accounts: &[Result<(InstructionAccounts, InstructionLoaders, InstructionCredits)>], + loaded_accounts: &mut [Result<( + InstructionAccounts, + InstructionLoaders, + InstructionCredits, + )>], executed: &[Result<()>], ) -> Vec> { if self.is_frozen() { @@ -1102,10 +1109,10 @@ impl Bank { lock_results: &LockedAccountsResults, max_age: usize, ) -> Vec> { - let (loaded_accounts, executed, _) = + let (mut loaded_accounts, executed, _) = self.load_and_execute_transactions(txs, lock_results, max_age); - self.commit_transactions(txs, &loaded_accounts, &executed) + self.commit_transactions(txs, &mut loaded_accounts, &executed) } #[must_use] @@ -1410,6 +1417,15 @@ impl Bank { dbank.rc.accounts.hash_internal_state(dbank.slot) ); } + + pub fn commit_credits(&self) { + self.rc + .accounts + .commit_credits(&self.ancestors, self.slot()); + } + fn clear_credit_only_account_locks(&self) { + self.rc.accounts.clear_credit_only_account_locks(); + } } impl Drop for Bank { @@ -1575,6 +1591,7 @@ mod tests { let t1 = system_transaction::transfer(&mint_keypair, &key1, 1, genesis_block.hash()); let t2 = system_transaction::transfer(&mint_keypair, &key2, 1, genesis_block.hash()); let res = bank.process_transactions(&vec![t1.clone(), t2.clone()]); + bank.commit_credits(); assert_eq!(res.len(), 2); assert_eq!(res[0], Ok(())); assert_eq!(res[1], Err(TransactionError::AccountInUse)); @@ -1943,33 +1960,51 @@ mod tests { } #[test] - fn test_need_credit_only_accounts() { - let (genesis_block, mint_keypair) = create_genesis_block(10); + fn test_credit_only_accounts() { + let (genesis_block, mint_keypair) = create_genesis_block(100); let bank = Bank::new(&genesis_block); let payer0 = Keypair::new(); let payer1 = Keypair::new(); - let recipient = Pubkey::new_rand(); + let recipient = Keypair::new(); // Fund additional payers bank.transfer(3, &mint_keypair, &payer0.pubkey()).unwrap(); bank.transfer(3, &mint_keypair, &payer1.pubkey()).unwrap(); - let tx0 = system_transaction::transfer(&mint_keypair, &recipient, 1, genesis_block.hash()); - let tx1 = system_transaction::transfer(&payer0, &recipient, 1, genesis_block.hash()); - let tx2 = system_transaction::transfer(&payer1, &recipient, 1, genesis_block.hash()); + let tx0 = system_transaction::transfer( + &mint_keypair, + &recipient.pubkey(), + 1, + genesis_block.hash(), + ); + let tx1 = + system_transaction::transfer(&payer0, &recipient.pubkey(), 1, genesis_block.hash()); + let tx2 = + system_transaction::transfer(&payer1, &recipient.pubkey(), 1, genesis_block.hash()); let txs = vec![tx0, tx1, tx2]; let results = bank.process_transactions(&txs); + bank.commit_credits(); - // If multiple transactions attempt to deposit into the same account, only the first will - // succeed, even though such atomic adds are safe. A System Transfer `To` account should be - // given credit-only handling + // If multiple transactions attempt to deposit into the same account, they should succeed, + // since System Transfer `To` accounts are given credit-only handling + assert_eq!(results[0], Ok(())); + assert_eq!(results[1], Ok(())); + assert_eq!(results[2], Ok(())); + assert_eq!(bank.get_balance(&recipient.pubkey()), 3); + let tx0 = system_transaction::transfer( + &mint_keypair, + &recipient.pubkey(), + 2, + genesis_block.hash(), + ); + let tx1 = + system_transaction::transfer(&recipient, &payer0.pubkey(), 1, genesis_block.hash()); + let txs = vec![tx0, tx1]; + let results = bank.process_transactions(&txs); + bank.commit_credits(); + + // However, an account may not be locked as credit-only and credit-debit at the same time. assert_eq!(results[0], Ok(())); assert_eq!(results[1], Err(TransactionError::AccountInUse)); - assert_eq!(results[2], Err(TransactionError::AccountInUse)); - - // After credit-only account handling is implemented, the following checks should pass instead: - // assert_eq!(results[0], Ok(())); - // assert_eq!(results[1], Ok(())); - // assert_eq!(results[2], Ok(())); } #[test] @@ -2016,11 +2051,12 @@ mod tests { fn test_credit_only_relaxed_locks() { use solana_sdk::message::{Message, MessageHeader}; - let (genesis_block, mint_keypair) = create_genesis_block(3); + let (genesis_block, _) = create_genesis_block(3); let bank = Bank::new(&genesis_block); let key0 = Keypair::new(); - let key1 = Pubkey::new_rand(); - let key2 = Pubkey::new_rand(); + let key1 = Keypair::new(); + let key2 = Keypair::new(); + let key3 = Pubkey::new_rand(); let message = Message { header: MessageHeader { @@ -2028,23 +2064,50 @@ mod tests { num_credit_only_signed_accounts: 0, num_credit_only_unsigned_accounts: 1, }, - account_keys: vec![key0.pubkey(), key1, key2], + account_keys: vec![key0.pubkey(), key3], recent_blockhash: Hash::default(), instructions: vec![], }; - let tx0 = Transaction::new(&[&key0], message, genesis_block.hash()); - let txs = vec![tx0]; + let tx = Transaction::new(&[&key0], message, genesis_block.hash()); + let txs = vec![tx]; - let lock_result = bank.lock_accounts(&txs); - assert_eq!(lock_result.locked_accounts_results(), &vec![Ok(())]); + let lock_result0 = bank.lock_accounts(&txs); + assert!(lock_result0.locked_accounts_results()[0].is_ok()); - // Try executing a tx loading a credit-only account from tx0 - assert!(bank.transfer(1, &mint_keypair, &key2).is_ok()); - // Try executing a tx loading a account locked as credit-debit in tx0 - assert_eq!( - bank.transfer(1, &mint_keypair, &key1), - Err(TransactionError::AccountInUse) - ); + // Try locking accounts, locking a previously credit-only account as credit-debit + // should fail + let message = Message { + header: MessageHeader { + num_required_signatures: 1, + num_credit_only_signed_accounts: 0, + num_credit_only_unsigned_accounts: 0, + }, + account_keys: vec![key1.pubkey(), key3], + recent_blockhash: Hash::default(), + instructions: vec![], + }; + let tx = Transaction::new(&[&key1], message, genesis_block.hash()); + let txs = vec![tx]; + + let lock_result1 = bank.lock_accounts(&txs); + assert!(lock_result1.locked_accounts_results()[0].is_err()); + + // Try locking a previously credit-only account a 2nd time; should succeed + let message = Message { + header: MessageHeader { + num_required_signatures: 1, + num_credit_only_signed_accounts: 0, + num_credit_only_unsigned_accounts: 1, + }, + account_keys: vec![key2.pubkey(), key3], + recent_blockhash: Hash::default(), + instructions: vec![], + }; + let tx = Transaction::new(&[&key2], message, genesis_block.hash()); + let txs = vec![tx]; + + let lock_result2 = bank.lock_accounts(&txs); + assert!(lock_result2.locked_accounts_results()[0].is_ok()); } #[test] diff --git a/runtime/src/bank_client.rs b/runtime/src/bank_client.rs index 08d0e93b9..a6ea0e7a2 100644 --- a/runtime/src/bank_client.rs +++ b/runtime/src/bank_client.rs @@ -197,6 +197,7 @@ impl BankClient { transactions.push(tx); } let _ = bank.process_transactions(&transactions); + bank.commit_credits(); } } diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 2a9540a49..815abcb4d 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -1,7 +1,9 @@ use crate::native_loader; use crate::system_instruction_processor; use serde::{Deserialize, Serialize}; -use solana_sdk::account::{create_keyed_accounts, Account, KeyedAccount, LamportCredit}; +use solana_sdk::account::{ + create_keyed_credit_only_accounts, Account, KeyedAccount, LamportCredit, +}; use solana_sdk::instruction::{CompiledInstruction, InstructionError}; use solana_sdk::instruction_processor_utils; use solana_sdk::message::Message; @@ -139,17 +141,28 @@ impl MessageProcessor { ) -> Result<(), InstructionError> { let program_id = instruction.program_id(&message.account_keys); - let mut keyed_accounts = create_keyed_accounts(executable_accounts); + let mut keyed_accounts = create_keyed_credit_only_accounts(executable_accounts); let mut keyed_accounts2: Vec<_> = instruction .accounts .iter() .map(|&index| { let index = index as usize; let key = &message.account_keys[index]; - (key, index < message.header.num_required_signatures as usize) + let is_debitable = message.is_debitable(index); + ( + key, + index < message.header.num_required_signatures as usize, + is_debitable, + ) }) .zip(program_accounts.iter_mut()) - .map(|((key, is_signer), account)| KeyedAccount::new(key, is_signer, account)) + .map(|((key, is_signer, is_debitable), account)| { + if is_debitable { + KeyedAccount::new(key, is_signer, account) + } else { + KeyedAccount::new_credit_only(key, is_signer, account) + } + }) .collect(); keyed_accounts.append(&mut keyed_accounts2); @@ -184,6 +197,7 @@ impl MessageProcessor { credits: &mut [&mut LamportCredit], ) -> Result<(), InstructionError> { let program_id = instruction.program_id(&message.account_keys); + assert_eq!(instruction.accounts.len(), program_accounts.len()); // TODO: the runtime should be checking read/write access to memory // we are trusting the hard-coded programs not to clobber or allocate let pre_total: u128 = program_accounts @@ -202,7 +216,13 @@ impl MessageProcessor { program_accounts .iter() .enumerate() - .map(|(i, program_account)| (i, program_account, message.is_debitable(i))), + .map(|(i, program_account)| { + ( + i, + program_account, + message.is_debitable(instruction.accounts[i] as usize), + ) + }), ) { verify_instruction( diff --git a/runtime/src/system_instruction_processor.rs b/runtime/src/system_instruction_processor.rs index 31bb84f82..c8b8140ff 100644 --- a/runtime/src/system_instruction_processor.rs +++ b/runtime/src/system_instruction_processor.rs @@ -311,7 +311,7 @@ mod tests { let mut to_account = Account::new(1, 0, &Pubkey::new(&[3; 32])); // account owner should not matter let mut keyed_accounts = [ KeyedAccount::new(&from, true, &mut from_account), - KeyedAccount::new(&to, false, &mut to_account), + KeyedAccount::new_credit_only(&to, false, &mut to_account), ]; transfer_lamports(&mut keyed_accounts, 50).unwrap(); let from_lamports = from_account.lamports; @@ -322,7 +322,7 @@ mod tests { // Attempt to move more lamports than remaining in from_account keyed_accounts = [ KeyedAccount::new(&from, true, &mut from_account), - KeyedAccount::new(&to, false, &mut to_account), + KeyedAccount::new_credit_only(&to, false, &mut to_account), ]; let result = transfer_lamports(&mut keyed_accounts, 100); assert_eq!(result, Err(SystemError::ResultWithNegativeLamports)); diff --git a/sdk/src/account.rs b/sdk/src/account.rs index 898c67089..9e4270a90 100644 --- a/sdk/src/account.rs +++ b/sdk/src/account.rs @@ -78,6 +78,7 @@ pub type LamportCredit = u64; #[derive(Debug)] pub struct KeyedAccount<'a> { is_signer: bool, // Transaction was signed by this account's key + is_debitable: bool, key: &'a Pubkey, pub account: &'a mut Account, } @@ -95,10 +96,28 @@ impl<'a> KeyedAccount<'a> { self.key } + pub fn is_debitable(&self) -> bool { + self.is_debitable + } + pub fn new(key: &'a Pubkey, is_signer: bool, account: &'a mut Account) -> KeyedAccount<'a> { KeyedAccount { - key, is_signer, + is_debitable: true, + key, + account, + } + } + + pub fn new_credit_only( + key: &'a Pubkey, + is_signer: bool, + account: &'a mut Account, + ) -> KeyedAccount<'a> { + KeyedAccount { + is_signer, + is_debitable: false, + key, account, } } @@ -108,6 +127,7 @@ impl<'a> From<(&'a Pubkey, &'a mut Account)> for KeyedAccount<'a> { fn from((key, account): (&'a Pubkey, &'a mut Account)) -> Self { KeyedAccount { is_signer: false, + is_debitable: true, key, account, } @@ -118,6 +138,7 @@ impl<'a> From<&'a mut (Pubkey, Account)> for KeyedAccount<'a> { fn from((key, account): &'a mut (Pubkey, Account)) -> Self { KeyedAccount { is_signer: false, + is_debitable: true, key, account, } @@ -127,3 +148,15 @@ impl<'a> From<&'a mut (Pubkey, Account)> for KeyedAccount<'a> { pub fn create_keyed_accounts(accounts: &mut [(Pubkey, Account)]) -> Vec { accounts.iter_mut().map(Into::into).collect() } + +pub fn create_keyed_credit_only_accounts(accounts: &mut [(Pubkey, Account)]) -> Vec { + accounts + .iter_mut() + .map(|(key, account)| KeyedAccount { + is_signer: false, + is_debitable: false, + key, + account, + }) + .collect() +} diff --git a/sdk/src/system_instruction.rs b/sdk/src/system_instruction.rs index edba60c9b..40cc3dd99 100644 --- a/sdk/src/system_instruction.rs +++ b/sdk/src/system_instruction.rs @@ -90,7 +90,7 @@ pub fn assign(from_pubkey: &Pubkey, program_id: &Pubkey) -> Instruction { pub fn transfer(from_pubkey: &Pubkey, to_pubkey: &Pubkey, lamports: u64) -> Instruction { let account_metas = vec![ AccountMeta::new(*from_pubkey, true), - AccountMeta::new(*to_pubkey, false), + AccountMeta::new_credit_only(*to_pubkey, false), ]; Instruction::new( system_program::id(),