Implement finalizer so that all locked accounts are dropped (#3585)
* Implement finalizer so that all locked accounts are dropped when finalizer goes out of scope * Add test for tx error with lock conflict * Fix double unlock from destructor running after a call to unlock
This commit is contained in:
parent
92c66a411b
commit
d228b6467c
|
@ -17,6 +17,7 @@ use crate::sigverify_stage::VerifiedPackets;
|
|||
use bincode::deserialize;
|
||||
use solana_metrics::counter::Counter;
|
||||
use solana_runtime::bank::{self, Bank};
|
||||
use solana_runtime::locked_accounts_results::LockedAccountsResults;
|
||||
use solana_sdk::pubkey::Pubkey;
|
||||
use solana_sdk::timing::{self, duration_as_us, MAX_RECENT_BLOCKHASHES};
|
||||
use solana_sdk::transaction::{Transaction, TransactionError};
|
||||
|
@ -253,18 +254,15 @@ impl BankingStage {
|
|||
bank: &Bank,
|
||||
txs: &[Transaction],
|
||||
poh: &Arc<Mutex<PohRecorder>>,
|
||||
lock_results: &[bank::Result<()>],
|
||||
lock_results: &LockedAccountsResults,
|
||||
) -> Result<()> {
|
||||
let now = Instant::now();
|
||||
// Use a shorter maximum age when adding transactions into the pipeline. This will reduce
|
||||
// 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) = bank.load_and_execute_transactions(
|
||||
txs,
|
||||
lock_results.to_vec(),
|
||||
MAX_RECENT_BLOCKHASHES / 2,
|
||||
);
|
||||
let (loaded_accounts, results) =
|
||||
bank.load_and_execute_transactions(txs, lock_results, MAX_RECENT_BLOCKHASHES / 2);
|
||||
let load_execute_time = now.elapsed();
|
||||
|
||||
let record_time = {
|
||||
|
@ -306,7 +304,7 @@ impl BankingStage {
|
|||
|
||||
let now = Instant::now();
|
||||
// Once the accounts are new transactions can enter the pipeline to process them
|
||||
bank.unlock_accounts(&txs, &lock_results);
|
||||
drop(lock_results);
|
||||
let unlock_time = now.elapsed();
|
||||
|
||||
debug!(
|
||||
|
|
|
@ -5,6 +5,7 @@ use crate::leader_schedule_utils;
|
|||
use rayon::prelude::*;
|
||||
use solana_metrics::counter::Counter;
|
||||
use solana_runtime::bank::{Bank, Result};
|
||||
use solana_runtime::locked_accounts_results::LockedAccountsResults;
|
||||
use solana_sdk::genesis_block::GenesisBlock;
|
||||
use solana_sdk::timing::duration_as_ms;
|
||||
use solana_sdk::timing::MAX_RECENT_BLOCKHASHES;
|
||||
|
@ -19,17 +20,16 @@ fn first_err(results: &[Result<()>]) -> Result<()> {
|
|||
Ok(())
|
||||
}
|
||||
|
||||
fn par_execute_entries(bank: &Bank, entries: &[(&Entry, Vec<Result<()>>)]) -> Result<()> {
|
||||
fn par_execute_entries(bank: &Bank, entries: &[(&Entry, LockedAccountsResults)]) -> Result<()> {
|
||||
inc_new_counter_info!("bank-par_execute_entries-count", entries.len());
|
||||
let results: Vec<Result<()>> = entries
|
||||
.into_par_iter()
|
||||
.map(|(e, lock_results)| {
|
||||
.map(|(e, locked_accounts)| {
|
||||
let results = bank.load_execute_and_commit_transactions(
|
||||
&e.transactions,
|
||||
lock_results.to_vec(),
|
||||
locked_accounts,
|
||||
MAX_RECENT_BLOCKHASHES,
|
||||
);
|
||||
bank.unlock_accounts(&e.transactions, &results);
|
||||
first_err(&results)
|
||||
})
|
||||
.collect();
|
||||
|
@ -57,11 +57,12 @@ pub fn process_entries(bank: &Bank, entries: &[Entry]) -> Result<()> {
|
|||
let lock_results = bank.lock_accounts(&entry.transactions);
|
||||
// if any of the locks error out
|
||||
// execute the current group
|
||||
if first_err(&lock_results).is_err() {
|
||||
if first_err(lock_results.locked_accounts_results()).is_err() {
|
||||
par_execute_entries(bank, &mt_group)?;
|
||||
// Drop all the locks on accounts by clearing the LockedAccountsFinalizer's in the
|
||||
// mt_group
|
||||
mt_group = vec![];
|
||||
//reset the lock and push the entry
|
||||
bank.unlock_accounts(&entry.transactions, &lock_results);
|
||||
drop(lock_results);
|
||||
let lock_results = bank.lock_accounts(&entry.transactions);
|
||||
mt_group.push((entry, lock_results));
|
||||
} else {
|
||||
|
@ -616,6 +617,88 @@ mod tests {
|
|||
assert_eq!(bank.get_balance(&keypair3.pubkey()), 2);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_process_entries_2_txes_collision_and_error() {
|
||||
let (genesis_block, mint_keypair) = GenesisBlock::new(1000);
|
||||
let bank = Bank::new(&genesis_block);
|
||||
let keypair1 = Keypair::new();
|
||||
let keypair2 = Keypair::new();
|
||||
let keypair3 = Keypair::new();
|
||||
let keypair4 = Keypair::new();
|
||||
|
||||
// fund: put 4 in each of 1 and 2
|
||||
assert_matches!(bank.transfer(4, &mint_keypair, &keypair1.pubkey()), Ok(_));
|
||||
assert_matches!(bank.transfer(4, &mint_keypair, &keypair2.pubkey()), Ok(_));
|
||||
assert_matches!(bank.transfer(4, &mint_keypair, &keypair4.pubkey()), Ok(_));
|
||||
|
||||
// construct an Entry whose 2nd transaction would cause a lock conflict with previous entry
|
||||
let entry_1_to_mint = next_entry(
|
||||
&bank.last_blockhash(),
|
||||
1,
|
||||
vec![
|
||||
SystemTransaction::new_account(
|
||||
&keypair1,
|
||||
&mint_keypair.pubkey(),
|
||||
1,
|
||||
bank.last_blockhash(),
|
||||
0,
|
||||
),
|
||||
SystemTransaction::new_move(
|
||||
&keypair4,
|
||||
&keypair4.pubkey(),
|
||||
1,
|
||||
Hash::default(), // Should cause a transaction failure with BlockhashNotFound
|
||||
0,
|
||||
),
|
||||
],
|
||||
);
|
||||
|
||||
let entry_2_to_3_mint_to_1 = next_entry(
|
||||
&entry_1_to_mint.hash,
|
||||
1,
|
||||
vec![
|
||||
SystemTransaction::new_account(
|
||||
&keypair2,
|
||||
&keypair3.pubkey(),
|
||||
2,
|
||||
bank.last_blockhash(),
|
||||
0,
|
||||
), // should be fine
|
||||
SystemTransaction::new_account(
|
||||
&keypair1,
|
||||
&mint_keypair.pubkey(),
|
||||
2,
|
||||
bank.last_blockhash(),
|
||||
0,
|
||||
), // will collide
|
||||
],
|
||||
);
|
||||
|
||||
assert!(process_entries(
|
||||
&bank,
|
||||
&[entry_1_to_mint.clone(), entry_2_to_3_mint_to_1.clone()]
|
||||
)
|
||||
.is_err());
|
||||
|
||||
// First transaction in first entry succeeded, so keypair1 lost 1 lamport
|
||||
assert_eq!(bank.get_balance(&keypair1.pubkey()), 3);
|
||||
assert_eq!(bank.get_balance(&keypair2.pubkey()), 4);
|
||||
|
||||
// Check all accounts are unlocked
|
||||
let txs1 = &entry_1_to_mint.transactions[..];
|
||||
let txs2 = &entry_2_to_3_mint_to_1.transactions[..];
|
||||
let locked_accounts1 = bank.lock_accounts(txs1);
|
||||
for result in locked_accounts1.locked_accounts_results() {
|
||||
assert!(result.is_ok());
|
||||
}
|
||||
// txs1 and txs2 have accounts that conflict, so we must drop txs1 first
|
||||
drop(locked_accounts1);
|
||||
let locked_accounts2 = bank.lock_accounts(txs2);
|
||||
for result in locked_accounts2.locked_accounts_results() {
|
||||
assert!(result.is_ok());
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_process_entries_2_entries_par() {
|
||||
let (genesis_block, mint_keypair) = GenesisBlock::new(1000);
|
||||
|
|
|
@ -5,6 +5,7 @@
|
|||
|
||||
use crate::accounts::{Accounts, ErrorCounters, InstructionAccounts, InstructionLoaders};
|
||||
use crate::blockhash_queue::BlockhashQueue;
|
||||
use crate::locked_accounts_results::LockedAccountsResults;
|
||||
use crate::runtime::{ProcessInstruction, Runtime};
|
||||
use crate::status_cache::StatusCache;
|
||||
use bincode::serialize;
|
||||
|
@ -467,18 +468,28 @@ impl Bank {
|
|||
.map_or(Ok(()), |sig| self.get_signature_status(sig).unwrap())
|
||||
}
|
||||
|
||||
pub fn lock_accounts(&self, txs: &[Transaction]) -> Vec<Result<()>> {
|
||||
pub fn lock_accounts<'a, 'b>(
|
||||
&'a self,
|
||||
txs: &'b [Transaction],
|
||||
) -> LockedAccountsResults<'a, 'b> {
|
||||
if self.is_frozen() {
|
||||
warn!("=========== FIXME: lock_accounts() working on a frozen bank! ================");
|
||||
}
|
||||
// TODO: put this assert back in
|
||||
// assert!(!self.is_frozen());
|
||||
self.accounts.lock_accounts(self.accounts_id, txs)
|
||||
let results = self.accounts.lock_accounts(self.accounts_id, txs);
|
||||
LockedAccountsResults::new(results, &self, txs)
|
||||
}
|
||||
|
||||
pub fn unlock_accounts(&self, txs: &[Transaction], results: &[Result<()>]) {
|
||||
self.accounts
|
||||
.unlock_accounts(self.accounts_id, txs, results)
|
||||
pub fn unlock_accounts(&self, locked_accounts_results: &mut LockedAccountsResults) {
|
||||
if locked_accounts_results.needs_unlock {
|
||||
locked_accounts_results.needs_unlock = false;
|
||||
self.accounts.unlock_accounts(
|
||||
self.accounts_id,
|
||||
locked_accounts_results.transactions(),
|
||||
locked_accounts_results.locked_accounts_results(),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
fn load_accounts(
|
||||
|
@ -498,17 +509,17 @@ impl Bank {
|
|||
fn check_refs(
|
||||
&self,
|
||||
txs: &[Transaction],
|
||||
lock_results: Vec<Result<()>>,
|
||||
lock_results: &LockedAccountsResults,
|
||||
error_counters: &mut ErrorCounters,
|
||||
) -> Vec<Result<()>> {
|
||||
txs.iter()
|
||||
.zip(lock_results.into_iter())
|
||||
.zip(lock_results.locked_accounts_results())
|
||||
.map(|(tx, lock_res)| {
|
||||
if lock_res.is_ok() && !tx.verify_refs() {
|
||||
error_counters.invalid_account_index += 1;
|
||||
Err(TransactionError::InvalidAccountIndex)
|
||||
} else {
|
||||
lock_res
|
||||
lock_res.clone()
|
||||
}
|
||||
})
|
||||
.collect()
|
||||
|
@ -575,7 +586,7 @@ impl Bank {
|
|||
pub fn load_and_execute_transactions(
|
||||
&self,
|
||||
txs: &[Transaction],
|
||||
lock_results: Vec<Result<()>>,
|
||||
lock_results: &LockedAccountsResults,
|
||||
max_age: usize,
|
||||
) -> (
|
||||
Vec<Result<(InstructionAccounts, InstructionLoaders)>>,
|
||||
|
@ -741,7 +752,7 @@ impl Bank {
|
|||
pub fn load_execute_and_commit_transactions(
|
||||
&self,
|
||||
txs: &[Transaction],
|
||||
lock_results: Vec<Result<()>>,
|
||||
lock_results: &LockedAccountsResults,
|
||||
max_age: usize,
|
||||
) -> Vec<Result<()>> {
|
||||
let (loaded_accounts, executed) =
|
||||
|
@ -753,10 +764,7 @@ impl Bank {
|
|||
#[must_use]
|
||||
pub fn process_transactions(&self, txs: &[Transaction]) -> Vec<Result<()>> {
|
||||
let lock_results = self.lock_accounts(txs);
|
||||
let results =
|
||||
self.load_execute_and_commit_transactions(txs, lock_results, MAX_RECENT_BLOCKHASHES);
|
||||
self.unlock_accounts(txs, &results);
|
||||
results
|
||||
self.load_execute_and_commit_transactions(txs, &lock_results, MAX_RECENT_BLOCKHASHES)
|
||||
}
|
||||
|
||||
/// Create, sign, and process a Transaction from `keypair` to `to` of
|
||||
|
@ -1312,7 +1320,7 @@ mod tests {
|
|||
let lock_result = bank.lock_accounts(&pay_alice);
|
||||
let results_alice = bank.load_execute_and_commit_transactions(
|
||||
&pay_alice,
|
||||
lock_result,
|
||||
&lock_result,
|
||||
MAX_RECENT_BLOCKHASHES,
|
||||
);
|
||||
assert_eq!(results_alice[0], Ok(()));
|
||||
|
@ -1329,7 +1337,7 @@ mod tests {
|
|||
Err(TransactionError::AccountInUse)
|
||||
);
|
||||
|
||||
bank.unlock_accounts(&pay_alice, &results_alice);
|
||||
drop(lock_result);
|
||||
|
||||
assert!(bank.transfer(2, &mint_keypair, &bob.pubkey()).is_ok());
|
||||
}
|
||||
|
|
|
@ -5,6 +5,7 @@ pub mod bank_client;
|
|||
mod blockhash_queue;
|
||||
pub mod bloom;
|
||||
pub mod loader_utils;
|
||||
pub mod locked_accounts_results;
|
||||
mod native_loader;
|
||||
pub mod runtime;
|
||||
mod status_cache;
|
||||
|
|
|
@ -0,0 +1,42 @@
|
|||
use crate::bank::{Bank, Result};
|
||||
use solana_sdk::transaction::Transaction;
|
||||
|
||||
// Represents the results of trying to lock a set of accounts
|
||||
pub struct LockedAccountsResults<'a, 'b> {
|
||||
locked_accounts_results: Vec<Result<()>>,
|
||||
bank: &'a Bank,
|
||||
transactions: &'b [Transaction],
|
||||
pub(crate) needs_unlock: bool,
|
||||
}
|
||||
|
||||
impl<'a, 'b> LockedAccountsResults<'a, 'b> {
|
||||
pub fn new(
|
||||
locked_accounts_results: Vec<Result<()>>,
|
||||
bank: &'a Bank,
|
||||
transactions: &'b [Transaction],
|
||||
) -> Self {
|
||||
Self {
|
||||
locked_accounts_results,
|
||||
bank,
|
||||
transactions,
|
||||
needs_unlock: true,
|
||||
}
|
||||
}
|
||||
|
||||
pub fn locked_accounts_results(&self) -> &Vec<Result<()>> {
|
||||
&self.locked_accounts_results
|
||||
}
|
||||
|
||||
pub fn transactions(&self) -> &[Transaction] {
|
||||
self.transactions
|
||||
}
|
||||
}
|
||||
|
||||
// Unlock all locked accounts in destructor.
|
||||
impl<'a, 'b> Drop for LockedAccountsResults<'a, 'b> {
|
||||
fn drop(&mut self) {
|
||||
if self.needs_unlock {
|
||||
self.bank.unlock_accounts(self)
|
||||
}
|
||||
}
|
||||
}
|
Loading…
Reference in New Issue