From 807c69d97cc2949961b2a4214251dc4dd8104406 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Mon, 10 Jun 2019 20:50:02 -0600 Subject: [PATCH] Slimmer implementation of credit-only accounts (#4592) * Add credit-only debit/data check to verify_instruction * Store credits and pass to accounts_db * Add InstructionErrors and tests * Relax account locks for credit-only accounts * Collect credit-only account credits before passing to accounts_db to store properly * Convert System Transfer accounts to credit-only, and fixup test * Functionalize collect_accounts to unit test * Review comments * Rebase --- Cargo.lock | 9 +- runtime/Cargo.toml | 1 + runtime/src/accounts.rs | 188 ++++++++++++++++++++++-------- runtime/src/accounts_db.rs | 99 ++++++++++------ runtime/src/append_vec.rs | 11 +- runtime/src/bank.rs | 71 +++++++---- runtime/src/message_processor.rs | 194 +++++++++++++++++++++++++++++-- sdk/src/account.rs | 2 + sdk/src/instruction.rs | 6 + sdk/src/message.rs | 18 +-- sdk/src/system_instruction.rs | 2 +- 11 files changed, 474 insertions(+), 127 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3f08cc5c3e..a435eb5e68 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1262,6 +1262,11 @@ dependencies = [ "cfg-if 0.1.7 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "maplit" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" + [[package]] name = "matches" version = "0.1.8" @@ -1580,7 +1585,7 @@ dependencies = [ "redox_syscall 0.1.51 (registry+https://github.com/rust-lang/crates.io-index)", "rustc_version 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", "smallvec 0.6.9 (registry+https://github.com/rust-lang/crates.io-index)", - "winapi 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi 0.3.7 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -2635,6 +2640,7 @@ dependencies = [ "libc 0.2.58 (registry+https://github.com/rust-lang/crates.io-index)", "libloading 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", + "maplit 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)", "memmap 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)", "rand 0.6.5 (registry+https://github.com/rust-lang/crates.io-index)", "rayon 1.0.3 (registry+https://github.com/rust-lang/crates.io-index)", @@ -3601,6 +3607,7 @@ dependencies = [ "checksum lock_api 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "ed946d4529956a20f2d63ebe1b69996d5a2137c91913fe3ebbeff957f5bca7ff" "checksum log 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)" = "e19e8d5c34a3e0e2223db8e060f9e8264aeeb5c5fc64a4ee9965c062211c024b" "checksum log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)" = "c84ec4b527950aa83a329754b01dbe3f58361d1c5efacd1f6d68c494d08a17c6" +"checksum maplit 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "08cbb6b4fef96b6d77bfc40ec491b1690c779e77b05cd9f07f787ed376fd4c43" "checksum matches 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)" = "7ffc5c5338469d4d3ea17d269fa8ea3512ad247247c30bd2df69e68309ed0a08" "checksum memchr 2.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "2efc7bc57c883d4a4d6e3246905283d8dae951bb3bd32f49d6ef297f546e1c39" "checksum memmap 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)" = "e2ffa2c986de11a9df78620c01eeaaf27d94d3ff02bf81bfcca953102dd0c6ff" diff --git a/runtime/Cargo.toml b/runtime/Cargo.toml index 70097ffdc8..2bcae704c0 100644 --- a/runtime/Cargo.toml +++ b/runtime/Cargo.toml @@ -17,6 +17,7 @@ hashbrown = "0.2.0" libc = "0.2.58" libloading = "0.5.0" log = "0.4.2" +maplit = "1.0.1" memmap = "0.6.2" rand = "0.6.5" rayon = "1.0.0" diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 3712d47202..ee50250a26 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -1,6 +1,6 @@ use crate::accounts_db::{ get_paths_vec, AccountInfo, AccountStorage, AccountsDB, AppendVecId, ErrorCounters, - InstructionAccounts, InstructionLoaders, + InstructionAccounts, InstructionCredits, InstructionLoaders, }; use crate::accounts_index::{AccountsIndex, Fork}; use crate::append_vec::StoredAccount; @@ -9,7 +9,7 @@ use bincode::serialize; use log::*; use serde::{Deserialize, Serialize}; use solana_metrics::inc_new_counter_error; -use solana_sdk::account::Account; +use solana_sdk::account::{Account, LamportCredit}; use solana_sdk::fee_calculator::FeeCalculator; use solana_sdk::hash::{Hash, Hasher}; use solana_sdk::native_loader; @@ -170,7 +170,7 @@ impl Accounts { tx: &Transaction, fee: u64, error_counters: &mut ErrorCounters, - ) -> Result> { + ) -> Result<(Vec, InstructionCredits)> { // Copy all the accounts let message = tx.message(); if tx.signatures.is_empty() && fee != 0 { @@ -185,6 +185,7 @@ impl Accounts { // There is no way to predict what program will execute without an error // 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 { if !message.program_ids().contains(&key) { called_accounts.push( @@ -192,6 +193,7 @@ impl Accounts { .map(|(account, _)| account) .unwrap_or_default(), ); + credits.push(0); } } if called_accounts.is_empty() || called_accounts[0].lamports == 0 { @@ -205,7 +207,7 @@ impl Accounts { Err(TransactionError::InsufficientFundsForFee) } else { called_accounts[0].lamports -= fee; - Ok(called_accounts) + Ok((called_accounts, credits)) } } } @@ -289,7 +291,7 @@ impl Accounts { lock_results: Vec>, fee_calculator: &FeeCalculator, error_counters: &mut ErrorCounters, - ) -> Vec> { + ) -> Vec> { //PERF: hold the lock to scan for the references, but not to clone the accounts //TODO: two locks usually leads to deadlocks, should this be one structure? let accounts_index = self.accounts_db.accounts_index.read().unwrap(); @@ -299,7 +301,7 @@ impl Accounts { .map(|etx| match etx { (tx, Ok(())) => { let fee = fee_calculator.calculate_fee(tx.message()); - let accounts = Self::load_tx_accounts( + let (accounts, credits) = Self::load_tx_accounts( &storage, ancestors, &accounts_index, @@ -314,7 +316,7 @@ impl Accounts { tx, error_counters, )?; - Ok((accounts, loaders)) + Ok((accounts, loaders, credits)) } (_, Err(e)) => Err(e), }) @@ -357,12 +359,14 @@ impl Accounts { /// Slow because lock is held for 1 operation instead of many pub fn store_slow(&self, fork: Fork, pubkey: &Pubkey, account: &Account) { - self.accounts_db.store(fork, &[(pubkey, account)]); + let mut accounts = HashMap::new(); + accounts.insert(pubkey, (account, 0)); + self.accounts_db.store(fork, &accounts); } fn lock_account( (fork_locks, parent_locks): (&mut HashSet, &mut Vec>), - keys: &[Pubkey], + keys: &[&Pubkey], error_counters: &mut ErrorCounters, ) -> Result<()> { // Copy all the accounts @@ -402,19 +406,19 @@ impl Accounts { } } for k in keys { - fork_locks.insert(*k); + fork_locks.insert(**k); } Ok(()) } - fn lock_record_account(record_locks: &AccountLocks, keys: &[Pubkey]) { + fn lock_record_account(record_locks: &AccountLocks, keys: &[&Pubkey]) { let mut fork_locks = record_locks.lock().unwrap(); for k in keys { // The fork locks should always be a subset of the account locks, so // the account locks should prevent record locks from ever touching the // same accounts assert!(!fork_locks.contains(k)); - fork_locks.insert(*k); + fork_locks.insert(**k); } } @@ -489,8 +493,7 @@ impl Accounts { let message = tx.borrow().message(); Self::lock_account( (&mut self.account_locks.lock().unwrap(), parent_record_locks), - &message.account_keys[..(message.account_keys.len() - - message.header.num_credit_only_unsigned_accounts as usize)], + &message.get_account_keys_by_lock_type().0, &mut error_counters, ) }) @@ -513,11 +516,7 @@ impl Accounts { let record_locks = self.record_locks.lock().unwrap(); for tx in txs { let message = tx.borrow().message(); - Self::lock_record_account( - &record_locks.0, - &message.account_keys[..(message.account_keys.len() - - message.header.num_credit_only_unsigned_accounts as usize)], - ); + Self::lock_record_account(&record_locks.0, &message.get_account_keys_by_lock_type().0); } } @@ -553,20 +552,9 @@ impl Accounts { fork: Fork, txs: &[Transaction], res: &[Result<()>], - loaded: &[Result<(InstructionAccounts, InstructionLoaders)>], + loaded: &[Result<(InstructionAccounts, InstructionLoaders, InstructionCredits)>], ) { - let mut accounts: Vec<(&Pubkey, &Account)> = vec![]; - for (i, raccs) in loaded.iter().enumerate() { - if res[i].is_err() || raccs.is_err() { - continue; - } - - let message = &txs[i].message(); - let acc = raccs.as_ref().unwrap(); - for (key, account) in message.account_keys.iter().zip(acc.0.iter()) { - accounts.push((key, account)); - } - } + let accounts = collect_accounts(txs, res, loaded); self.accounts_db.store(fork, &accounts); } @@ -581,6 +569,39 @@ impl Accounts { } } +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; + } + + 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; + } + } + } + } + accounts +} + #[cfg(test)] mod tests { // TODO: all the bank tests are bank specific, issue: 2194 @@ -603,7 +624,7 @@ mod tests { ka: &Vec<(Pubkey, Account)>, fee_calculator: &FeeCalculator, error_counters: &mut ErrorCounters, - ) -> Vec> { + ) -> Vec> { let accounts = Accounts::new(None); for ka in ka.iter() { accounts.store_slow(0, &ka.0, &ka.1); @@ -624,7 +645,7 @@ mod tests { tx: Transaction, ka: &Vec<(Pubkey, Account)>, error_counters: &mut ErrorCounters, - ) -> Vec> { + ) -> Vec> { let fee_calculator = FeeCalculator::default(); load_accounts_with_fee(tx, ka, &fee_calculator, error_counters) } @@ -800,11 +821,13 @@ mod tests { assert_eq!(error_counters.account_not_found, 0); assert_eq!(loaded_accounts.len(), 1); match &loaded_accounts[0] { - Ok((a, l)) => { - assert_eq!(a.len(), 2); - assert_eq!(a[0], accounts[0].1); - assert_eq!(l.len(), 1); - assert_eq!(l[0].len(), 0); + Ok((instruction_accounts, instruction_loaders, instruction_credits)) => { + assert_eq!(instruction_accounts.len(), 2); + assert_eq!(instruction_accounts[0], accounts[0].1); + assert_eq!(instruction_loaders.len(), 1); + assert_eq!(instruction_loaders[0].len(), 0); + assert_eq!(instruction_credits.len(), 2); + assert_eq!(instruction_credits, &vec![0, 0]); } Err(e) => Err(e).unwrap(), } @@ -984,16 +1007,18 @@ mod tests { assert_eq!(error_counters.account_not_found, 0); assert_eq!(loaded_accounts.len(), 1); match &loaded_accounts[0] { - Ok((a, l)) => { - assert_eq!(a.len(), 1); - assert_eq!(a[0], accounts[0].1); - assert_eq!(l.len(), 2); - assert_eq!(l[0].len(), 1); - assert_eq!(l[1].len(), 2); - for instruction_loaders in l.iter() { - for (i, a) in instruction_loaders.iter().enumerate() { + Ok((instruction_accounts, instruction_loaders, instruction_credits)) => { + assert_eq!(instruction_accounts.len(), 1); + assert_eq!(instruction_accounts[0], accounts[0].1); + assert_eq!(instruction_loaders.len(), 2); + assert_eq!(instruction_loaders[0].len(), 1); + assert_eq!(instruction_loaders[1].len(), 2); + assert_eq!(instruction_credits.len(), 1); + assert_eq!(instruction_credits, &vec![0]); + for loaders in instruction_loaders.iter() { + for (i, accounts_subset) in loaders.iter().enumerate() { // +1 to skip first not loader account - assert_eq![a.1, accounts[i + 1].1]; + assert_eq![accounts_subset.1, accounts[i + 1].1]; } } } @@ -1121,7 +1146,7 @@ mod tests { &mut child.account_locks.lock().unwrap(), &mut child.record_locks.lock().unwrap().1 ), - &vec![locked_pubkey], + &vec![&locked_pubkey], &mut ErrorCounters::default() ), Ok(()) @@ -1209,4 +1234,69 @@ mod tests { daccounts.hash_internal_state(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], + 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], + Hash::default(), + vec![native_loader::id()], + instructions, + ); + let txs = vec![tx0, tx1]; + + let loaders = vec![Ok(()), Ok(())]; + + 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 instruction_accounts0 = vec![account0, account2.clone()]; + let instruction_loaders0 = vec![]; + let instruction_credits0 = vec![0, 2]; + let loaded0 = Ok(( + instruction_accounts0, + instruction_loaders0, + instruction_credits0, + )); + + let instruction_accounts1 = vec![account1, account2.clone()]; + let instruction_loaders1 = vec![]; + let instruction_credits1 = vec![2, 3]; + let loaded1 = Ok(( + instruction_accounts1, + instruction_loaders1, + instruction_credits1, + )); + + let 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)); + + // 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); + } } diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index cf4cce0655..25f96d966f 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -28,7 +28,7 @@ use rayon::ThreadPool; use serde::de::{MapAccess, Visitor}; use serde::ser::{SerializeMap, Serializer}; use serde::{Deserialize, Serialize}; -use solana_sdk::account::Account; +use solana_sdk::account::{Account, LamportCredit}; use solana_sdk::pubkey::Pubkey; use std::collections::{HashMap, HashSet}; use std::fmt; @@ -74,6 +74,7 @@ pub struct AccountInfo { /// An offset into the AccountsDB::storage vector pub type AppendVecId = usize; pub type InstructionAccounts = Vec; +pub type InstructionCredits = Vec; pub type InstructionLoaders = Vec>; #[derive(Default, Debug)] @@ -429,10 +430,14 @@ impl AccountsDB { } } - fn store_accounts(&self, fork_id: Fork, accounts: &[(&Pubkey, &Account)]) -> Vec { - let with_meta: Vec<(StorageMeta, &Account)> = accounts + fn store_accounts( + &self, + fork_id: Fork, + accounts: &HashMap<&Pubkey, (&Account, LamportCredit)>, + ) -> Vec { + let with_meta: Vec<(StorageMeta, &Pubkey, &Account, u64)> = accounts .iter() - .map(|(pubkey, account)| { + .map(|(pubkey, (account, credit))| { let write_version = self.write_version.fetch_add(1, Ordering::Relaxed) as u64; let data_len = if account.lamports == 0 { 0 @@ -444,7 +449,15 @@ impl AccountsDB { pubkey: **pubkey, data_len, }; - (meta, *account) + + 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, *pubkey, *account, lamports) }) .collect(); let mut infos: Vec = vec![]; @@ -455,12 +468,12 @@ impl AccountsDB { storage.set_status(AccountStorageStatus::Full); continue; } - for (offset, (_, account)) in rvs.iter().zip(&with_meta[infos.len()..]) { + for (offset, (_, _, _, lamports)) in rvs.iter().zip(&with_meta[infos.len()..]) { storage.add_account(); infos.push(AccountInfo { id: storage.id, offset: *offset, - lamports: account.lamports, + lamports: *lamports, }); } // restore the state to available @@ -473,12 +486,12 @@ impl AccountsDB { &self, fork_id: Fork, infos: Vec, - accounts: &[(&Pubkey, &Account)], + accounts: &HashMap<&Pubkey, (&Account, LamportCredit)>, ) -> (Vec<(Fork, AccountInfo)>, u64) { - let mut reclaims = Vec::with_capacity(infos.len() * 2); + let mut reclaims: Vec<(Fork, AccountInfo)> = Vec::with_capacity(infos.len() * 2); let mut index = self.accounts_index.write().unwrap(); - for (i, info) in infos.into_iter().enumerate() { - let key = &accounts[i].0; + for (info, account) in infos.into_iter().zip(accounts.iter()) { + let key = &account.0; index.insert(fork_id, key, info, &mut reclaims); } (reclaims, index.last_root) @@ -528,7 +541,7 @@ impl AccountsDB { } /// Store the account update. - pub fn store(&self, fork_id: Fork, accounts: &[(&Pubkey, &Account)]) { + pub fn store(&self, fork_id: Fork, accounts: &HashMap<&Pubkey, (&Account, LamportCredit)>) { let infos = self.store_accounts(fork_id, accounts); let (reclaims, last_root) = self.update_index(fork_id, infos, accounts); @@ -689,6 +702,7 @@ mod tests { // TODO: all the bank tests are bank specific, issue: 2194 use super::*; use bincode::{deserialize_from, serialize_into, serialized_size}; + use maplit::hashmap; use rand::{thread_rng, Rng}; use solana_sdk::account::Account; @@ -743,7 +757,7 @@ mod tests { let key = Pubkey::default(); let account0 = Account::new(1, 0, &key); - db.store(0, &[(&key, &account0)]); + db.store(0, &hashmap!(&key => (&account0, 0))); db.add_root(0); let ancestors = vec![(1, 1)].into_iter().collect(); assert_eq!(db.load_slow(&ancestors, &key), Some((account0, 0))); @@ -757,10 +771,10 @@ mod tests { let key = Pubkey::default(); let account0 = Account::new(1, 0, &key); - db.store(0, &[(&key, &account0)]); + db.store(0, &hashmap!(&key => (&account0, 0))); let account1 = Account::new(0, 0, &key); - db.store(1, &[(&key, &account1)]); + db.store(1, &hashmap!(&key => (&account1, 0))); let ancestors = vec![(1, 1)].into_iter().collect(); assert_eq!(&db.load_slow(&ancestors, &key).unwrap().0, &account1); @@ -777,10 +791,10 @@ mod tests { let key = Pubkey::default(); let account0 = Account::new(1, 0, &key); - db.store(0, &[(&key, &account0)]); + db.store(0, &hashmap!(&key => (&account0, 0))); let account1 = Account::new(0, 0, &key); - db.store(1, &[(&key, &account1)]); + db.store(1, &hashmap!(&key => (&account1, 0))); db.add_root(0); let ancestors = vec![(1, 1)].into_iter().collect(); @@ -799,7 +813,7 @@ mod tests { let account0 = Account::new(1, 0, &key); // store value 1 in the "root", i.e. db zero - db.store(0, &[(&key, &account0)]); + db.store(0, &hashmap!(&key => (&account0, 0))); // now we have: // @@ -812,7 +826,7 @@ mod tests { // store value 0 in one child let account1 = Account::new(0, 0, &key); - db.store(1, &[(&key, &account1)]); + db.store(1, &hashmap!(&key => (&account1, 0))); // masking accounts is done at the Accounts level, at accountsDB we see // original account (but could also accept "None", which is implemented @@ -883,8 +897,8 @@ mod tests { let pubkey = Pubkey::new_rand(); let account = Account::new(1, ACCOUNT_DATA_FILE_SIZE as usize / 3, &pubkey); - db.store(1, &[(&pubkey, &account)]); - db.store(1, &[(&pubkeys[0], &account)]); + db.store(1, &hashmap!(&pubkey => (&account, 0))); + db.store(1, &hashmap!(&pubkeys[0] => (&account, 0))); { let stores = db.storage.read().unwrap(); let fork_0_stores = &stores.0.get(&0).unwrap(); @@ -914,11 +928,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, &[(&key, &account0)]); + db0.store(0, &hashmap!(&key => (&account0, 0))); // 0 lamports in the child let account1 = Account::new(0, 0, &key); - db0.store(1, &[(&key, &account1)]); + db0.store(1, &hashmap!(&key => (&account1, 0))); // masking accounts is done at the Accounts level, at accountsDB we see // original account @@ -928,6 +942,23 @@ 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, @@ -942,7 +973,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, &[(&pubkey, &account)]); + accounts.store(fork, &hashmap!(&pubkey => (&account, 0))); } for t in 0..num_vote { let pubkey = Pubkey::new_rand(); @@ -950,7 +981,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, &[(&pubkey, &account)]); + accounts.store(fork, &hashmap!(&pubkey => (&account, 0))); } } @@ -960,7 +991,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, &[(&pubkeys[idx], &account)]); + accounts.store(fork, &hashmap!(&pubkeys[idx] => (&account, 0))); if account.lamports == 0 { let ancestors = vec![(fork, 0)].into_iter().collect(); assert!(accounts.load_slow(&ancestors, &pubkeys[idx]).is_none()); @@ -1005,7 +1036,7 @@ mod tests { ) { for idx in 0..num { let account = Account::new((idx + count) as u64, 0, &Account::default().owner); - accounts.store(fork, &[(&pubkeys[idx], &account)]); + accounts.store(fork, &hashmap!(&pubkeys[idx] => (&account, 0))); } } @@ -1050,7 +1081,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, &[(&key, &account)]); + accounts.store(0, &hashmap!(&key => (&account, 0))); keys.push(key); } for (i, key) in keys.iter().enumerate() { @@ -1085,7 +1116,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, &[(&pubkey1, &account1)]); + accounts.store(0, &hashmap!(&pubkey1 => (&account1, 0))); { let stores = accounts.storage.read().unwrap(); assert_eq!(stores.0.len(), 1); @@ -1095,7 +1126,7 @@ mod tests { let pubkey2 = Pubkey::new_rand(); let account2 = Account::new(1, ACCOUNT_DATA_FILE_SIZE as usize / 2, &pubkey2); - accounts.store(0, &[(&pubkey2, &account2)]); + accounts.store(0, &hashmap!(&pubkey2 => (&account2, 0))); { let stores = accounts.storage.read().unwrap(); assert_eq!(stores.0.len(), 1); @@ -1118,7 +1149,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, &[(&pubkey1, &account1)]); + accounts.store(0, &hashmap!(&pubkey1 => (&account1, 0))); { let stores = accounts.storage.read().unwrap(); assert_eq!(stores.0.len(), 1); @@ -1176,7 +1207,7 @@ mod tests { let pubkey = Pubkey::new_rand(); let account = Account::new(1, 0, &Account::default().owner); //store an account - accounts.store(0, &[(&pubkey, &account)]); + accounts.store(0, &hashmap!(&pubkey => (&account, 0))); let ancestors = vec![(0, 0)].into_iter().collect(); let info = accounts .accounts_index @@ -1203,7 +1234,7 @@ mod tests { .is_some()); //store causes cleanup - accounts.store(1, &[(&pubkey, &account)]); + accounts.store(1, &hashmap!(&pubkey => (&account, 0))); //fork is gone assert!(accounts.storage.read().unwrap().0.get(&0).is_none()); @@ -1268,7 +1299,7 @@ mod tests { loop { let account_bal = thread_rng().gen_range(1, 99); account.lamports = account_bal; - db.store(fork_id, &[(&pubkey, &account)]); + db.store(fork_id, &hashmap!(&pubkey => (&account, 0))); 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 063b347424..3719886b5b 100644 --- a/runtime/src/append_vec.rs +++ b/runtime/src/append_vec.rs @@ -220,13 +220,16 @@ impl AppendVec { } #[allow(clippy::mutex_atomic)] - pub fn append_accounts(&self, accounts: &[(StorageMeta, &Account)]) -> Vec { + pub fn append_accounts( + &self, + accounts: &[(StorageMeta, &Pubkey, &Account, u64)], + ) -> Vec { let mut offset = self.append_offset.lock().unwrap(); let mut rv = vec![]; - for (storage_meta, account) in accounts { + for (storage_meta, _, account, lamports) in accounts { let meta_ptr = storage_meta as *const StorageMeta; let balance = AccountBalance { - lamports: account.lamports, + lamports: *lamports, owner: account.owner, executable: account.executable, }; @@ -248,7 +251,7 @@ impl AppendVec { } pub fn append_account(&self, storage_meta: StorageMeta, account: &Account) -> Option { - self.append_accounts(&[(storage_meta, account)]) + self.append_accounts(&[(storage_meta, &Pubkey::default(), account, account.lamports)]) .first() .cloned() } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index d8ec0ea84a..fe36f188de 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3,7 +3,9 @@ //! on behalf of the caller, and a low-level API for when they have //! already been signed and verified. use crate::accounts::{AccountLockType, Accounts}; -use crate::accounts_db::{AccountsDB, ErrorCounters, InstructionAccounts, InstructionLoaders}; +use crate::accounts_db::{ + AccountsDB, ErrorCounters, InstructionAccounts, InstructionCredits, InstructionLoaders, +}; use crate::accounts_index::Fork; use crate::blockhash_queue::BlockhashQueue; use crate::epoch_schedule::EpochSchedule; @@ -679,7 +681,7 @@ impl Bank { txs: &[Transaction], results: Vec>, error_counters: &mut ErrorCounters, - ) -> Vec> { + ) -> Vec> { self.rc.accounts.load_accounts( &self.ancestors, txs, @@ -837,7 +839,7 @@ impl Bank { lock_results: &LockedAccountsResults, max_age: usize, ) -> ( - Vec>, + Vec>, Vec>, ) { debug!("processing transactions: {}", txs.len()); @@ -858,10 +860,9 @@ impl Bank { .zip(txs.iter()) .map(|(accs, tx)| match accs { Err(e) => Err(e.clone()), - Ok((ref mut accounts, ref mut loaders)) => { - self.message_processor - .process_message(tx.message(), loaders, accounts) - } + Ok((ref mut accounts, ref mut loaders, ref mut credits)) => self + .message_processor + .process_message(tx.message(), loaders, accounts, credits), }) .collect(); @@ -941,7 +942,7 @@ impl Bank { pub fn commit_transactions( &self, txs: &[Transaction], - loaded_accounts: &[Result<(InstructionAccounts, InstructionLoaders)>], + loaded_accounts: &[Result<(InstructionAccounts, InstructionLoaders, InstructionCredits)>], executed: &[Result<()>], ) -> Vec> { if self.is_frozen() { @@ -1161,7 +1162,7 @@ impl Bank { &self, txs: &[Transaction], res: &[Result<()>], - loaded: &[Result<(InstructionAccounts, InstructionLoaders)>], + loaded: &[Result<(InstructionAccounts, InstructionLoaders, InstructionCredits)>], ) { for (i, raccs) in loaded.iter().enumerate() { if res[i].is_err() || raccs.is_err() { @@ -1631,7 +1632,7 @@ mod tests { } #[test] - fn test_need_credit_only_accounts() { + fn test_credit_only_accounts() { let (genesis_block, mint_keypair) = create_genesis_block(10); let bank = Bank::new(&genesis_block); let payer0 = Keypair::new(); @@ -1646,18 +1647,13 @@ mod tests { let txs = vec![tx0, tx1, tx2]; let results = bank.process_transactions(&txs); - // 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], 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(())); + assert_eq!(results[1], Ok(())); + assert_eq!(results[2], Ok(())); + assert_eq!(bank.get_balance(&recipient), 3); } #[test] @@ -1700,6 +1696,41 @@ mod tests { assert!(bank.transfer(2, &mint_keypair, &bob.pubkey()).is_ok()); } + #[test] + fn test_credit_only_relaxed_locks() { + use solana_sdk::message::{Message, MessageHeader}; + + let (genesis_block, mint_keypair) = 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 message = Message { + header: MessageHeader { + num_required_signatures: 1, + num_credit_only_signed_accounts: 0, + num_credit_only_unsigned_accounts: 1, + }, + account_keys: vec![key0.pubkey(), key1, key2], + recent_blockhash: Hash::default(), + instructions: vec![], + }; + let tx0 = Transaction::new(&[&key0], message, genesis_block.hash()); + let txs = vec![tx0]; + + let lock_result = bank.lock_accounts(&txs); + assert_eq!(lock_result.locked_accounts_results(), &vec![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) + ); + } + #[test] fn test_bank_invalid_account_index() { let (genesis_block, mint_keypair) = create_genesis_block(1); diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index e19f0a194b..b7f841d999 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -1,7 +1,7 @@ use crate::native_loader; use crate::system_instruction_processor; use serde::{Deserialize, Serialize}; -use solana_sdk::account::{create_keyed_accounts, Account, KeyedAccount}; +use solana_sdk::account::{create_keyed_accounts, Account, KeyedAccount, LamportCredit}; use solana_sdk::instruction::{CompiledInstruction, InstructionError}; use solana_sdk::instruction_processor_utils; use solana_sdk::message::Message; @@ -55,6 +55,7 @@ fn get_subset_unchecked_mut<'a, T>( } fn verify_instruction( + is_debitable: bool, program_id: &Pubkey, pre_program_id: &Pubkey, pre_lamports: u64, @@ -71,6 +72,10 @@ fn verify_instruction( if *program_id != account.owner && pre_lamports > account.lamports { return Err(InstructionError::ExternalAccountLamportSpend); } + // The balance of credit-only accounts may only increase + if !is_debitable && pre_lamports > account.lamports { + return Err(InstructionError::CreditOnlyLamportSpend); + } // For accounts unassigned to the program, the data may not change. if *program_id != account.owner && !system_program::check_id(&program_id) @@ -78,6 +83,10 @@ fn verify_instruction( { return Err(InstructionError::ExternalAccountDataModified); } + // Credit-only account data may not change. + if !is_debitable && pre_data != &account.data[..] { + return Err(InstructionError::CreditOnlyDataModified); + } Ok(()) } @@ -172,6 +181,7 @@ impl MessageProcessor { instruction: &CompiledInstruction, executable_accounts: &mut [(Pubkey, Account)], program_accounts: &mut [&mut Account], + credits: &mut [&mut LamportCredit], ) -> Result<(), InstructionError> { let program_id = instruction.program_id(&message.account_keys); // TODO: the runtime should be checking read/write access to memory @@ -183,18 +193,26 @@ impl MessageProcessor { .collect(); self.process_instruction(message, instruction, executable_accounts, program_accounts)?; - // Verify the instruction - for ((pre_program_id, pre_lamports, pre_data), post_account) in - pre_data.iter().zip(program_accounts.iter()) + for ((pre_program_id, pre_lamports, pre_data), (i, post_account, is_debitable)) in + pre_data.iter().zip( + program_accounts + .iter() + .enumerate() + .map(|(i, program_account)| (i, program_account, message.is_debitable(i))), + ) { verify_instruction( + is_debitable, &program_id, pre_program_id, *pre_lamports, pre_data, post_account, )?; + if !is_debitable { + *credits[i] += post_account.lamports - *pre_lamports; + } } // The total sum of all the lamports in all the accounts cannot change. let post_total: u64 = program_accounts.iter().map(|a| a.lamports).sum(); @@ -212,6 +230,7 @@ impl MessageProcessor { message: &Message, loaders: &mut [Vec<(Pubkey, Account)>], accounts: &mut [Account], + credits: &mut [LamportCredit], ) -> Result<(), TransactionError> { for (instruction_index, instruction) in message.instructions.iter().enumerate() { let executable_index = message @@ -223,11 +242,14 @@ impl MessageProcessor { // TODO: `get_subset_unchecked_mut` panics on an index out of bounds if an executable // account is also included as a regular account for an instruction, because the // executable account is not passed in as part of the accounts slice + let mut instruction_credits = get_subset_unchecked_mut(credits, &instruction.accounts) + .map_err(|err| TransactionError::InstructionError(instruction_index as u8, err))?; self.execute_instruction( message, instruction, executable_accounts, &mut program_accounts, + &mut instruction_credits, ) .map_err(|err| TransactionError::InstructionError(instruction_index as u8, err))?; } @@ -238,6 +260,9 @@ impl MessageProcessor { #[cfg(test)] mod tests { use super::*; + use solana_sdk::instruction::{AccountMeta, Instruction, InstructionError}; + use solana_sdk::message::Message; + use solana_sdk::native_loader::{create_loadable_account, id}; #[test] fn test_has_duplicates() { @@ -280,7 +305,7 @@ mod tests { pre: &Pubkey, post: &Pubkey, ) -> Result<(), InstructionError> { - verify_instruction(&ix, &pre, 0, &[], &Account::new(0, 0, post)) + verify_instruction(true, &ix, &pre, 0, &[], &Account::new(0, 0, post)) } let system_program_id = system_program::id(); @@ -301,24 +326,175 @@ mod tests { #[test] fn test_verify_instruction_change_data() { - fn change_data(program_id: &Pubkey) -> Result<(), InstructionError> { + fn change_data(program_id: &Pubkey, is_debitable: bool) -> Result<(), InstructionError> { let alice_program_id = Pubkey::new_rand(); let account = Account::new(0, 0, &alice_program_id); - verify_instruction(&program_id, &alice_program_id, 0, &[42], &account) + verify_instruction( + is_debitable, + &program_id, + &alice_program_id, + 0, + &[42], + &account, + ) } let system_program_id = system_program::id(); let mallory_program_id = Pubkey::new_rand(); assert_eq!( - change_data(&system_program_id), + change_data(&system_program_id, true), Ok(()), "system program should be able to change the data" ); assert_eq!( - change_data(&mallory_program_id), + change_data(&mallory_program_id, true), Err(InstructionError::ExternalAccountDataModified), "malicious Mallory should not be able to change the account data" ); + + assert_eq!( + change_data(&system_program_id, false), + Err(InstructionError::CreditOnlyDataModified), + "system program should not be able to change the data if credit-only" + ); + } + + #[test] + fn test_verify_instruction_credit_only() { + let alice_program_id = Pubkey::new_rand(); + let account = Account::new(0, 0, &alice_program_id); + assert_eq!( + verify_instruction( + false, + &system_program::id(), + &alice_program_id, + 42, + &[], + &account + ), + Err(InstructionError::ExternalAccountLamportSpend), + "debit should fail, even if system program" + ); + assert_eq!( + verify_instruction( + false, + &alice_program_id, + &alice_program_id, + 42, + &[], + &account + ), + Err(InstructionError::CreditOnlyLamportSpend), + "debit should fail, even if owning program" + ); + } + + #[test] + fn test_process_message_credit_only_handling() { + #[derive(Serialize, Deserialize)] + enum MockSystemInstruction { + Correct { lamports: u64 }, + AttemptDebit { lamports: u64 }, + Misbehave { lamports: u64 }, + } + + fn mock_system_process_instruction( + _program_id: &Pubkey, + keyed_accounts: &mut [KeyedAccount], + data: &[u8], + ) -> Result<(), InstructionError> { + if let Ok(instruction) = bincode::deserialize(data) { + match instruction { + MockSystemInstruction::Correct { lamports } => { + keyed_accounts[0].account.lamports -= lamports; + keyed_accounts[1].account.lamports += lamports; + Ok(()) + } + MockSystemInstruction::AttemptDebit { lamports } => { + keyed_accounts[0].account.lamports += lamports; + keyed_accounts[1].account.lamports -= lamports; + Ok(()) + } + // Credit a credit-only account for more lamports than debited + MockSystemInstruction::Misbehave { lamports } => { + keyed_accounts[0].account.lamports -= lamports; + keyed_accounts[1].account.lamports = 2 * lamports; + Ok(()) + } + } + } else { + Err(InstructionError::InvalidInstructionData) + } + } + + let mock_system_program_id = Pubkey::new(&[2u8; 32]); + let mut message_processor = MessageProcessor::default(); + message_processor + .add_instruction_processor(mock_system_program_id, mock_system_process_instruction); + + let mut accounts: Vec = Vec::new(); + let account = Account::new(100, 1, &mock_system_program_id); + accounts.push(account); + let account = Account::new(0, 1, &mock_system_program_id); + accounts.push(account); + + let mut loaders: Vec> = Vec::new(); + let account = create_loadable_account("mock_system_program"); + loaders.push(vec![(id(), account)]); + + let from_pubkey = Pubkey::new_rand(); + let to_pubkey = Pubkey::new_rand(); + let account_metas = vec![ + AccountMeta::new(from_pubkey, true), + AccountMeta::new_credit_only(to_pubkey, false), + ]; + let message = Message::new(vec![Instruction::new( + mock_system_program_id, + &MockSystemInstruction::Correct { lamports: 50 }, + account_metas.clone(), + )]); + let mut deltas = vec![0, 0]; + + let result = + message_processor.process_message(&message, &mut loaders, &mut accounts, &mut deltas); + assert_eq!(result, Ok(())); + assert_eq!(accounts[0].lamports, 50); + assert_eq!(accounts[1].lamports, 50); + assert_eq!(deltas, vec![0, 50]); + + let message = Message::new(vec![Instruction::new( + mock_system_program_id, + &MockSystemInstruction::AttemptDebit { lamports: 50 }, + account_metas.clone(), + )]); + let mut deltas = vec![0, 0]; + + let result = + message_processor.process_message(&message, &mut loaders, &mut accounts, &mut deltas); + assert_eq!( + result, + Err(TransactionError::InstructionError( + 0, + InstructionError::CreditOnlyLamportSpend + )) + ); + + let message = Message::new(vec![Instruction::new( + mock_system_program_id, + &MockSystemInstruction::Misbehave { lamports: 50 }, + account_metas, + )]); + let mut deltas = vec![0, 0]; + + let result = + message_processor.process_message(&message, &mut loaders, &mut accounts, &mut deltas); + assert_eq!( + result, + Err(TransactionError::InstructionError( + 0, + InstructionError::UnbalancedInstruction + )) + ); } } diff --git a/sdk/src/account.rs b/sdk/src/account.rs index 627d8092fc..4601e70190 100644 --- a/sdk/src/account.rs +++ b/sdk/src/account.rs @@ -55,6 +55,8 @@ impl Account { } } +pub type LamportCredit = u64; + #[repr(C)] #[derive(Debug)] pub struct KeyedAccount<'a> { diff --git a/sdk/src/instruction.rs b/sdk/src/instruction.rs index 5dcd23225b..6781ab154d 100644 --- a/sdk/src/instruction.rs +++ b/sdk/src/instruction.rs @@ -52,6 +52,12 @@ pub enum InstructionError { /// Program modified the data of an account that doesn't belong to it ExternalAccountDataModified, + /// Credit-only account spent lamports + CreditOnlyLamportSpend, + + /// Credit-only account modified data + CreditOnlyDataModified, + /// An account was referenced more than once in a single instruction DuplicateAccountIndex, diff --git a/sdk/src/message.rs b/sdk/src/message.rs index c38cb08812..aa46e1fd34 100644 --- a/sdk/src/message.rs +++ b/sdk/src/message.rs @@ -216,7 +216,7 @@ impl Message { .position(|&&pubkey| pubkey == self.account_keys[index]) } - fn is_credit_debit(&self, i: usize) -> bool { + pub fn is_debitable(&self, i: usize) -> bool { i < (self.header.num_required_signatures - self.header.num_credit_only_signed_accounts) as usize || (i >= self.header.num_required_signatures as usize @@ -228,7 +228,7 @@ impl Message { let mut credit_debit_keys = vec![]; let mut credit_only_keys = vec![]; for (i, key) in self.account_keys.iter().enumerate() { - if self.is_credit_debit(i) { + if self.is_debitable(i) { credit_debit_keys.push(key); } else { credit_only_keys.push(key); @@ -516,7 +516,7 @@ mod tests { } #[test] - fn test_is_credit_debit() { + fn test_is_debitable() { let key0 = Pubkey::new_rand(); let key1 = Pubkey::new_rand(); let key2 = Pubkey::new_rand(); @@ -534,12 +534,12 @@ mod tests { recent_blockhash: Hash::default(), instructions: vec![], }; - assert_eq!(message.is_credit_debit(0), true); - assert_eq!(message.is_credit_debit(1), false); - assert_eq!(message.is_credit_debit(2), false); - assert_eq!(message.is_credit_debit(3), true); - assert_eq!(message.is_credit_debit(4), true); - assert_eq!(message.is_credit_debit(5), false); + assert_eq!(message.is_debitable(0), true); + assert_eq!(message.is_debitable(1), false); + assert_eq!(message.is_debitable(2), false); + assert_eq!(message.is_debitable(3), true); + assert_eq!(message.is_debitable(4), true); + assert_eq!(message.is_debitable(5), false); } #[test] diff --git a/sdk/src/system_instruction.rs b/sdk/src/system_instruction.rs index edba60c9b6..40cc3dd991 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(),