Address review comments
This commit is contained in:
parent
2ed29771f2
commit
a058f348a2
|
@ -1351,34 +1351,22 @@ impl BankingStage {
|
|||
gossip_vote_sender: &ReplayVoteSender,
|
||||
qos_service: &QosService,
|
||||
) -> ProcessTransactionBatchOutput {
|
||||
let (
|
||||
(transactions_qos_results, cost_model_throttled_transactions_count, transaction_costs),
|
||||
cost_model_time,
|
||||
) = Measure::this(
|
||||
|_| {
|
||||
let tx_costs = qos_service.compute_transaction_costs(txs.iter());
|
||||
let mut cost_model_time = Measure::start("cost_model");
|
||||
|
||||
let transaction_costs = qos_service.compute_transaction_costs(txs.iter());
|
||||
|
||||
let (transactions_qos_results, num_included) =
|
||||
qos_service.select_transactions_per_cost(txs.iter(), tx_costs.iter(), bank);
|
||||
qos_service.select_transactions_per_cost(txs.iter(), transaction_costs.iter(), bank);
|
||||
|
||||
let cost_model_throttled_transactions_count =
|
||||
txs.len().saturating_sub(num_included);
|
||||
let cost_model_throttled_transactions_count = txs.len().saturating_sub(num_included);
|
||||
|
||||
qos_service.accumulate_estimated_transaction_costs(
|
||||
&Self::accumulate_batched_transaction_costs(
|
||||
tx_costs.iter(),
|
||||
transaction_costs.iter(),
|
||||
transactions_qos_results.iter(),
|
||||
),
|
||||
);
|
||||
(
|
||||
transactions_qos_results,
|
||||
cost_model_throttled_transactions_count,
|
||||
tx_costs,
|
||||
)
|
||||
},
|
||||
(),
|
||||
"cost_model",
|
||||
);
|
||||
cost_model_time.stop();
|
||||
|
||||
// Only lock accounts for those transactions are selected for the block;
|
||||
// Once accounts are locked, other threads cannot encode transactions that will modify the
|
||||
|
@ -1410,6 +1398,9 @@ impl BankingStage {
|
|||
..
|
||||
} = execute_and_commit_transactions_output;
|
||||
|
||||
// TODO: This does not revert the cost tracker changes from all unexecuted transactions
|
||||
// yet: For example tx that are too old will not be included in the block, but are not
|
||||
// retryable.
|
||||
QosService::update_or_remove_transaction_costs(
|
||||
transaction_costs.iter(),
|
||||
transactions_qos_results.iter(),
|
||||
|
|
|
@ -133,7 +133,7 @@ impl QosService {
|
|||
let mut num_included = 0;
|
||||
let select_results = transactions
|
||||
.zip(transactions_costs)
|
||||
.map(|(tx, cost)| match cost_tracker.try_add(tx, cost) {
|
||||
.map(|(tx, cost)| match cost_tracker.try_add(cost) {
|
||||
Ok(current_block_cost) => {
|
||||
debug!("slot {:?}, transaction {:?}, cost {:?}, fit into current block, current block cost {}", bank.slot(), tx, cost, current_block_cost);
|
||||
self.metrics.stats.selected_txs_count.fetch_add(1, Ordering::Relaxed);
|
||||
|
@ -184,8 +184,10 @@ impl QosService {
|
|||
transaction_costs
|
||||
.zip(transaction_qos_results)
|
||||
.enumerate()
|
||||
.for_each(|(index, (tx_cost, qos_result))| {
|
||||
if qos_result.is_ok() && retryable_transaction_indexes.contains(&index) {
|
||||
.for_each(|(index, (tx_cost, qos_inclusion_result))| {
|
||||
// Only transactions that the qos service incuded have been added to the
|
||||
// cost tracker.
|
||||
if qos_inclusion_result.is_ok() && retryable_transaction_indexes.contains(&index) {
|
||||
cost_tracker.remove(tx_cost);
|
||||
} else {
|
||||
// TODO: Update the cost tracker with the actual execution compute units.
|
||||
|
|
|
@ -822,7 +822,7 @@ fn compute_slot_cost(blockstore: &Blockstore, slot: Slot) -> Result<(), String>
|
|||
num_programs += transaction.message().instructions().len();
|
||||
|
||||
let tx_cost = cost_model.calculate_cost(&transaction);
|
||||
let result = cost_tracker.try_add(&transaction, &tx_cost);
|
||||
let result = cost_tracker.try_add(&tx_cost);
|
||||
if result.is_err() {
|
||||
println!(
|
||||
"Slot: {}, CostModel rejected transaction {:?}, reason {:?}",
|
||||
|
|
|
@ -16,7 +16,7 @@ use {
|
|||
const MAX_WRITABLE_ACCOUNTS: usize = 256;
|
||||
|
||||
// costs are stored in number of 'compute unit's
|
||||
#[derive(Debug, Clone)]
|
||||
#[derive(Debug)]
|
||||
pub struct TransactionCost {
|
||||
pub writable_accounts: Vec<Pubkey>,
|
||||
pub signature_cost: u64,
|
||||
|
|
|
@ -1,11 +1,11 @@
|
|||
//! `cost_tracker` keeps tracking transaction cost per chained accounts as well as for entire block
|
||||
//! The main functions are:
|
||||
//! - would_fit(&tx_cost), immutable function to test if tx with tx_cost would fit into current block
|
||||
//! - add_transaction(&tx_cost), mutable function to accumulate tx_cost to tracker.
|
||||
//! - add_transaction_cost(&tx_cost), mutable function to accumulate tx_cost to tracker.
|
||||
//!
|
||||
use {
|
||||
crate::{block_cost_limits::*, cost_model::TransactionCost},
|
||||
solana_sdk::{clock::Slot, pubkey::Pubkey, transaction::SanitizedTransaction},
|
||||
solana_sdk::{clock::Slot, pubkey::Pubkey},
|
||||
std::collections::HashMap,
|
||||
};
|
||||
|
||||
|
@ -89,11 +89,7 @@ impl CostTracker {
|
|||
self.vote_cost_limit = vote_cost_limit;
|
||||
}
|
||||
|
||||
pub fn try_add(
|
||||
&mut self,
|
||||
_transaction: &SanitizedTransaction,
|
||||
tx_cost: &TransactionCost,
|
||||
) -> Result<u64, CostTrackerError> {
|
||||
pub fn try_add(&mut self, tx_cost: &TransactionCost) -> Result<u64, CostTrackerError> {
|
||||
self.would_fit(tx_cost)?;
|
||||
self.add_transaction_cost(tx_cost);
|
||||
Ok(self.block_cost)
|
||||
|
@ -104,7 +100,7 @@ impl CostTracker {
|
|||
_estimated_tx_cost: &TransactionCost,
|
||||
_actual_execution_cost: u64,
|
||||
) {
|
||||
// adjust block_cost / vote_cost / account_cost by (actual_execution_cost - execution_cost)
|
||||
// TODO: adjust block_cost / vote_cost / account_cost by (actual_execution_cost - execution_cost)
|
||||
}
|
||||
|
||||
pub fn remove(&mut self, tx_cost: &TransactionCost) {
|
||||
|
@ -260,7 +256,9 @@ mod tests {
|
|||
hash::Hash,
|
||||
signature::{Keypair, Signer},
|
||||
system_transaction,
|
||||
transaction::{MessageHash, SimpleAddressLoader, VersionedTransaction},
|
||||
transaction::{
|
||||
MessageHash, SanitizedTransaction, SimpleAddressLoader, VersionedTransaction,
|
||||
},
|
||||
},
|
||||
solana_vote_program::vote_transaction,
|
||||
std::{cmp, sync::Arc},
|
||||
|
@ -360,7 +358,7 @@ mod tests {
|
|||
// build testee to have capacity for one simple transaction
|
||||
let mut testee = CostTracker::new(cost, cost, cost, None);
|
||||
assert!(testee.would_fit(&tx_cost).is_ok());
|
||||
testee.add_transaction(&tx_cost);
|
||||
testee.add_transaction_cost(&tx_cost);
|
||||
assert_eq!(cost, testee.block_cost);
|
||||
assert_eq!(0, testee.vote_cost);
|
||||
let (_costliest_account, costliest_account_cost) = testee.find_costliest_account();
|
||||
|
@ -376,7 +374,7 @@ mod tests {
|
|||
// build testee to have capacity for one simple transaction
|
||||
let mut testee = CostTracker::new(cost, cost, cost, None);
|
||||
assert!(testee.would_fit(&tx_cost).is_ok());
|
||||
testee.add_transaction(&tx_cost);
|
||||
testee.add_transaction_cost(&tx_cost);
|
||||
assert_eq!(cost, testee.block_cost);
|
||||
assert_eq!(cost, testee.vote_cost);
|
||||
let (_costliest_account, costliest_account_cost) = testee.find_costliest_account();
|
||||
|
@ -394,7 +392,7 @@ mod tests {
|
|||
let mut testee = CostTracker::new(cost, cost, cost, None);
|
||||
assert!(testee.would_fit(&tx_cost).is_ok());
|
||||
let old = testee.account_data_size;
|
||||
testee.add_transaction(&tx_cost);
|
||||
testee.add_transaction_cost(&tx_cost);
|
||||
assert_eq!(old + 1, testee.account_data_size);
|
||||
}
|
||||
|
||||
|
@ -411,11 +409,11 @@ mod tests {
|
|||
let mut testee = CostTracker::new(cost1 + cost2, cost1 + cost2, cost1 + cost2, None);
|
||||
{
|
||||
assert!(testee.would_fit(&tx_cost1).is_ok());
|
||||
testee.add_transaction(&tx_cost1);
|
||||
testee.add_transaction_cost(&tx_cost1);
|
||||
}
|
||||
{
|
||||
assert!(testee.would_fit(&tx_cost2).is_ok());
|
||||
testee.add_transaction(&tx_cost2);
|
||||
testee.add_transaction_cost(&tx_cost2);
|
||||
}
|
||||
assert_eq!(cost1 + cost2, testee.block_cost);
|
||||
assert_eq!(1, testee.cost_by_writable_accounts.len());
|
||||
|
@ -438,11 +436,11 @@ mod tests {
|
|||
CostTracker::new(cmp::max(cost1, cost2), cost1 + cost2, cost1 + cost2, None);
|
||||
{
|
||||
assert!(testee.would_fit(&tx_cost1).is_ok());
|
||||
testee.add_transaction(&tx_cost1);
|
||||
testee.add_transaction_cost(&tx_cost1);
|
||||
}
|
||||
{
|
||||
assert!(testee.would_fit(&tx_cost2).is_ok());
|
||||
testee.add_transaction(&tx_cost2);
|
||||
testee.add_transaction_cost(&tx_cost2);
|
||||
}
|
||||
assert_eq!(cost1 + cost2, testee.block_cost);
|
||||
assert_eq!(2, testee.cost_by_writable_accounts.len());
|
||||
|
@ -465,7 +463,7 @@ mod tests {
|
|||
// should have room for first transaction
|
||||
{
|
||||
assert!(testee.would_fit(&tx_cost1).is_ok());
|
||||
testee.add_transaction(&tx_cost1);
|
||||
testee.add_transaction_cost(&tx_cost1);
|
||||
}
|
||||
// but no more sapce on the same chain (same signer account)
|
||||
{
|
||||
|
@ -493,7 +491,7 @@ mod tests {
|
|||
// should have room for first transaction
|
||||
{
|
||||
assert!(testee.would_fit(&tx_cost1).is_ok());
|
||||
testee.add_transaction(&tx_cost1);
|
||||
testee.add_transaction_cost(&tx_cost1);
|
||||
}
|
||||
// but no more room for package as whole
|
||||
{
|
||||
|
@ -521,7 +519,7 @@ mod tests {
|
|||
// should have room for first vote
|
||||
{
|
||||
assert!(testee.would_fit(&tx_cost1).is_ok());
|
||||
testee.add_transaction(&tx_cost1);
|
||||
testee.add_transaction_cost(&tx_cost1);
|
||||
}
|
||||
// but no more room for package as whole
|
||||
{
|
||||
|
@ -591,45 +589,38 @@ mod tests {
|
|||
}
|
||||
|
||||
#[test]
|
||||
fn test_cost_tracker_commit_and_cancel() {
|
||||
fn test_cost_tracker_remove() {
|
||||
let (mint_keypair, start_hash) = test_setup();
|
||||
// build two transactions with diff accounts
|
||||
let second_account = Keypair::new();
|
||||
let (tx1, tx_cost1) = build_simple_transaction(&mint_keypair, &start_hash);
|
||||
let (tx2, tx_cost2) = build_simple_transaction(&second_account, &start_hash);
|
||||
let (_tx1, tx_cost1) = build_simple_transaction(&mint_keypair, &start_hash);
|
||||
let (_tx2, tx_cost2) = build_simple_transaction(&second_account, &start_hash);
|
||||
let cost1 = tx_cost1.sum();
|
||||
let cost2 = tx_cost2.sum();
|
||||
// build testee
|
||||
let mut testee = CostTracker::new(cost1 + cost2, cost1 + cost2, cost1 + cost2, None);
|
||||
|
||||
assert!(testee.try_add(&tx1, &tx_cost1).is_ok());
|
||||
assert!(testee.try_add(&tx2, &tx_cost2).is_ok());
|
||||
|
||||
// assert the block cost is still zero
|
||||
assert_eq!(0, testee.block_cost);
|
||||
|
||||
// assert tx1 cost applied to tracker if committed
|
||||
testee.commit_transaction(&tx1, None);
|
||||
assert_eq!(cost1, testee.block_cost);
|
||||
|
||||
// assert tx2 cost will not be applied to tracker if cancelled
|
||||
testee.cancel_transaction(&tx2);
|
||||
assert_eq!(cost1, testee.block_cost);
|
||||
|
||||
// still can add tx2
|
||||
assert!(testee.try_add(&tx2, &tx_cost2).is_ok());
|
||||
// cannot add tx1 while tx2 is pending
|
||||
assert!(testee.try_add(&tx1, &tx_cost1).is_err());
|
||||
// after commit tx2, the block will have both tx1 and tx2
|
||||
testee.commit_transaction(&tx2, None);
|
||||
assert!(testee.try_add(&tx_cost1).is_ok());
|
||||
assert!(testee.try_add(&tx_cost2).is_ok());
|
||||
assert_eq!(cost1 + cost2, testee.block_cost);
|
||||
|
||||
// removing a tx_cost affects block_cost
|
||||
testee.remove(&tx_cost1);
|
||||
assert_eq!(cost2, testee.block_cost);
|
||||
|
||||
// add back tx1
|
||||
assert!(testee.try_add(&tx_cost1).is_ok());
|
||||
assert_eq!(cost1 + cost2, testee.block_cost);
|
||||
|
||||
// cannot add tx1 again, cost limit would be exceeded
|
||||
assert!(testee.try_add(&tx_cost1).is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_cost_tracker_try_add_is_atomic() {
|
||||
let (mint_keypair, start_hash) = test_setup();
|
||||
// build two mocking vote transactions with diff accounts
|
||||
let (tx1, _tx_cost1) = build_simple_vote_transaction(&mint_keypair, &start_hash);
|
||||
let (_tx1, _tx_cost1) = build_simple_vote_transaction(&mint_keypair, &start_hash);
|
||||
|
||||
let acct1 = Pubkey::new_unique();
|
||||
let acct2 = Pubkey::new_unique();
|
||||
|
@ -651,8 +642,7 @@ mod tests {
|
|||
execution_cost: cost,
|
||||
..TransactionCost::default()
|
||||
};
|
||||
assert!(testee.try_add(&tx1, &tx_cost).is_ok());
|
||||
testee.commit_transaction(&tx1, None);
|
||||
assert!(testee.try_add(&tx_cost).is_ok());
|
||||
let (_costliest_account, costliest_account_cost) = testee.find_costliest_account();
|
||||
assert_eq!(cost, testee.block_cost);
|
||||
assert_eq!(3, testee.cost_by_writable_accounts.len());
|
||||
|
@ -670,8 +660,7 @@ mod tests {
|
|||
execution_cost: cost,
|
||||
..TransactionCost::default()
|
||||
};
|
||||
assert!(testee.try_add(&tx1, &tx_cost).is_ok());
|
||||
testee.commit_transaction(&tx1, None);
|
||||
assert!(testee.try_add(&tx_cost).is_ok());
|
||||
let (costliest_account, costliest_account_cost) = testee.find_costliest_account();
|
||||
assert_eq!(cost * 2, testee.block_cost);
|
||||
assert_eq!(3, testee.cost_by_writable_accounts.len());
|
||||
|
@ -691,8 +680,7 @@ mod tests {
|
|||
execution_cost: cost,
|
||||
..TransactionCost::default()
|
||||
};
|
||||
assert!(testee.try_add(&tx1, &tx_cost).is_err());
|
||||
testee.commit_transaction(&tx1, None);
|
||||
assert!(testee.try_add(&tx_cost).is_err());
|
||||
let (costliest_account, costliest_account_cost) = testee.find_costliest_account();
|
||||
assert_eq!(cost * 2, testee.block_cost);
|
||||
assert_eq!(3, testee.cost_by_writable_accounts.len());
|
||||
|
|
Loading…
Reference in New Issue