Refactor to remove requested_cu from cost_trarcker (#29015)
* refactor cost tracker by removing requested_cu from it, call sites to use cost_model forr consistency * review fix
This commit is contained in:
Tao Zhu
GitHub
parent
20aeff3632
commit
5850af5316
|
|
@ -3,9 +3,10 @@ use {
|
|||
solana_perf::packet::Packet,
|
||||
solana_runtime::{
|
||||
block_cost_limits,
|
||||
cost_model::CostModel,
|
||||
cost_tracker::{CostTracker, CostTrackerError},
|
||||
},
|
||||
solana_sdk::{pubkey::Pubkey, transaction::SanitizedTransaction},
|
||||
solana_sdk::{feature_set::FeatureSet, transaction::SanitizedTransaction},
|
||||
std::sync::Arc,
|
||||
};
|
||||
|
||||
|
|
@ -57,16 +58,12 @@ impl ForwardBatch {
|
|||
|
||||
fn try_add(
|
||||
&mut self,
|
||||
write_lock_accounts: &[Pubkey],
|
||||
compute_units: u64,
|
||||
sanitized_transaction: &SanitizedTransaction,
|
||||
immutable_packet: Arc<ImmutableDeserializedPacket>,
|
||||
feature_set: &FeatureSet,
|
||||
) -> Result<u64, CostTrackerError> {
|
||||
let res = self.cost_tracker.try_add_requested_cus(
|
||||
write_lock_accounts,
|
||||
compute_units,
|
||||
immutable_packet.is_simple_vote(),
|
||||
);
|
||||
|
||||
let tx_cost = CostModel::calculate_cost(sanitized_transaction, feature_set);
|
||||
let res = self.cost_tracker.try_add(&tx_cost);
|
||||
if res.is_ok() {
|
||||
self.forwardable_packets.push(immutable_packet);
|
||||
}
|
||||
|
|
@ -126,29 +123,12 @@ impl ForwardPacketBatchesByAccounts {
|
|||
&mut self,
|
||||
sanitized_transaction: &SanitizedTransaction,
|
||||
packet: Arc<ImmutableDeserializedPacket>,
|
||||
feature_set: &FeatureSet,
|
||||
) -> bool {
|
||||
if self.accepting_packets {
|
||||
// get write_lock_accounts
|
||||
let message = sanitized_transaction.message();
|
||||
let write_lock_accounts: Vec<_> = message
|
||||
.account_keys()
|
||||
.iter()
|
||||
.enumerate()
|
||||
.filter_map(|(i, account_key)| {
|
||||
if message.is_writable(i) {
|
||||
Some(*account_key)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
})
|
||||
.collect();
|
||||
|
||||
// get requested CUs
|
||||
let requested_cu = packet.compute_unit_limit();
|
||||
|
||||
// try to fill into forward batches
|
||||
self.accepting_packets =
|
||||
self.add_packet_to_batches(&write_lock_accounts, requested_cu, packet);
|
||||
self.add_packet_to_batches(sanitized_transaction, packet, feature_set);
|
||||
}
|
||||
self.accepting_packets
|
||||
}
|
||||
|
|
@ -162,13 +142,13 @@ impl ForwardPacketBatchesByAccounts {
|
|||
/// next batch until either being added to one of 'bucket' or not being forwarded.
|
||||
fn add_packet_to_batches(
|
||||
&mut self,
|
||||
write_lock_accounts: &[Pubkey],
|
||||
compute_units: u64,
|
||||
sanitized_transaction: &SanitizedTransaction,
|
||||
immutable_packet: Arc<ImmutableDeserializedPacket>,
|
||||
feature_set: &FeatureSet,
|
||||
) -> bool {
|
||||
for forward_batch in self.forward_batches.iter_mut() {
|
||||
if forward_batch
|
||||
.try_add(write_lock_accounts, compute_units, immutable_packet.clone())
|
||||
.try_add(sanitized_transaction, immutable_packet.clone(), feature_set)
|
||||
.is_ok()
|
||||
{
|
||||
return true;
|
||||
|
|
@ -183,59 +163,81 @@ mod tests {
|
|||
use {
|
||||
super::*,
|
||||
crate::unprocessed_packet_batches::DeserializedPacket,
|
||||
solana_runtime::transaction_priority_details::TransactionPriorityDetails,
|
||||
solana_runtime::{
|
||||
bank::Bank,
|
||||
genesis_utils::{create_genesis_config, GenesisConfigInfo},
|
||||
transaction_priority_details::TransactionPriorityDetails,
|
||||
},
|
||||
solana_sdk::{
|
||||
feature_set::FeatureSet, hash::Hash, signature::Keypair, system_transaction,
|
||||
transaction::SimpleAddressLoader,
|
||||
feature_set::FeatureSet, hash::Hash, pubkey::Pubkey, signature::Keypair,
|
||||
system_transaction,
|
||||
},
|
||||
std::sync::Arc,
|
||||
};
|
||||
|
||||
fn build_deserialized_packet_for_test(
|
||||
fn test_setup() -> (Keypair, Hash) {
|
||||
solana_logger::setup();
|
||||
let GenesisConfigInfo {
|
||||
genesis_config,
|
||||
mint_keypair,
|
||||
..
|
||||
} = create_genesis_config(10);
|
||||
let bank = Arc::new(Bank::new_no_wallclock_throttle_for_tests(&genesis_config));
|
||||
let start_hash = bank.last_blockhash();
|
||||
(mint_keypair, start_hash)
|
||||
}
|
||||
|
||||
/// build test transaction, return corresponding sanitized_transaction and deserialized_packet,
|
||||
/// and the batch limit_ratio that would only allow one transaction per bucket.
|
||||
fn build_test_transaction_and_packet(
|
||||
priority: u64,
|
||||
write_to_account: &Pubkey,
|
||||
compute_unit_limit: u64,
|
||||
) -> DeserializedPacket {
|
||||
let tx =
|
||||
system_transaction::transfer(&Keypair::new(), write_to_account, 1, Hash::new_unique());
|
||||
let packet = Packet::from_data(None, tx).unwrap();
|
||||
DeserializedPacket::new_with_priority_details(
|
||||
packet,
|
||||
) -> (SanitizedTransaction, DeserializedPacket, u32) {
|
||||
let (mint_keypair, start_hash) = test_setup();
|
||||
let transaction =
|
||||
system_transaction::transfer(&mint_keypair, write_to_account, 2, start_hash);
|
||||
let sanitized_transaction =
|
||||
SanitizedTransaction::from_transaction_for_tests(transaction.clone());
|
||||
let tx_cost = CostModel::calculate_cost(&sanitized_transaction, &FeatureSet::all_enabled());
|
||||
let cost = tx_cost.sum();
|
||||
let deserialized_packet = DeserializedPacket::new_with_priority_details(
|
||||
Packet::from_data(None, transaction).unwrap(),
|
||||
TransactionPriorityDetails {
|
||||
priority,
|
||||
compute_unit_limit,
|
||||
compute_unit_limit: cost,
|
||||
},
|
||||
)
|
||||
.unwrap()
|
||||
.unwrap();
|
||||
|
||||
// set limit ratio so each batch can only have one test transaction
|
||||
let limit_ratio: u32 =
|
||||
((block_cost_limits::MAX_WRITABLE_ACCOUNT_UNITS - cost + 1) / cost) as u32;
|
||||
(sanitized_transaction, deserialized_packet, limit_ratio)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_try_add_to_forward_batch() {
|
||||
// set test batch limit to be 1 millionth of regular block limit
|
||||
let limit_ratio = 1_000_000u32;
|
||||
// set requested_cu to be half of batch account limit
|
||||
let requested_cu =
|
||||
block_cost_limits::MAX_WRITABLE_ACCOUNT_UNITS.saturating_div(limit_ratio as u64);
|
||||
|
||||
let (tx, packet, limit_ratio) =
|
||||
build_test_transaction_and_packet(0u64, &Pubkey::new_unique());
|
||||
let mut forward_batch = ForwardBatch::new(limit_ratio);
|
||||
|
||||
let write_lock_accounts = vec![Pubkey::new_unique(), Pubkey::new_unique()];
|
||||
let packet = build_deserialized_packet_for_test(10, &write_lock_accounts[1], requested_cu);
|
||||
// first packet will be successful
|
||||
// Assert first packet will be added to forwarding buffer
|
||||
assert!(forward_batch
|
||||
.try_add(
|
||||
&write_lock_accounts,
|
||||
requested_cu,
|
||||
packet.immutable_section().clone()
|
||||
&tx,
|
||||
packet.immutable_section().clone(),
|
||||
&FeatureSet::all_enabled(),
|
||||
)
|
||||
.is_ok());
|
||||
assert_eq!(1, forward_batch.forwardable_packets.len());
|
||||
// second packet will hit account limit, therefore not added
|
||||
|
||||
// Assert second copy of same packet will hit write account limit, therefore
|
||||
// not be added to forwarding buffer
|
||||
assert!(forward_batch
|
||||
.try_add(
|
||||
&write_lock_accounts,
|
||||
requested_cu,
|
||||
packet.immutable_section().clone()
|
||||
&tx,
|
||||
packet.immutable_section().clone(),
|
||||
&FeatureSet::all_enabled(),
|
||||
)
|
||||
.is_err());
|
||||
assert_eq!(1, forward_batch.forwardable_packets.len());
|
||||
|
|
@ -243,18 +245,21 @@ mod tests {
|
|||
|
||||
#[test]
|
||||
fn test_try_add_packet_to_batches() {
|
||||
solana_logger::setup();
|
||||
// set test batch limit to be 1 millionth of regular block limit
|
||||
let limit_ratio = 1_000_000u32;
|
||||
let number_of_batches = 2;
|
||||
// set requested_cu to be half of batch account limit
|
||||
let requested_cu =
|
||||
block_cost_limits::MAX_WRITABLE_ACCOUNT_UNITS.saturating_div(limit_ratio as u64);
|
||||
// setup two transactions, one has high priority that writes to hot account, the
|
||||
// other write to non-contentious account with no priority
|
||||
let hot_account = solana_sdk::pubkey::new_rand();
|
||||
let other_account = solana_sdk::pubkey::new_rand();
|
||||
let (tx_high_priority, packet_high_priority, limit_ratio) =
|
||||
build_test_transaction_and_packet(10, &hot_account);
|
||||
let (tx_low_priority, packet_low_priority, _) =
|
||||
build_test_transaction_and_packet(0, &other_account);
|
||||
|
||||
// setup forwarding with 2 buckets, each only allow one transaction
|
||||
let number_of_batches = 2;
|
||||
let mut forward_packet_batches_by_accounts =
|
||||
ForwardPacketBatchesByAccounts::new(limit_ratio, number_of_batches);
|
||||
|
||||
// initially both batches are empty
|
||||
// Assert initially both batches are empty
|
||||
{
|
||||
let mut batches = forward_packet_batches_by_accounts.iter_batches();
|
||||
assert_eq!(0, batches.next().unwrap().len());
|
||||
|
|
@ -262,61 +267,54 @@ mod tests {
|
|||
assert!(batches.next().is_none());
|
||||
}
|
||||
|
||||
let hot_account = solana_sdk::pubkey::new_rand();
|
||||
let other_account = solana_sdk::pubkey::new_rand();
|
||||
let packet_high_priority =
|
||||
build_deserialized_packet_for_test(10, &hot_account, requested_cu);
|
||||
let packet_low_priority =
|
||||
build_deserialized_packet_for_test(0, &other_account, requested_cu);
|
||||
// with 4 packets, first 3 write to same hot_account with higher priority,
|
||||
// the 4th write to other_account with lower priority;
|
||||
// assert the 1st and 4th fit in first batch, the 2nd in 2nd batch and 3rd will be dropped.
|
||||
|
||||
// 1st high-priority packet added to 1st batch
|
||||
// Assert one high-priority packet will be added to 1st bucket successfully
|
||||
{
|
||||
forward_packet_batches_by_accounts.add_packet_to_batches(
|
||||
&[hot_account],
|
||||
requested_cu,
|
||||
assert!(forward_packet_batches_by_accounts.add_packet_to_batches(
|
||||
&tx_high_priority,
|
||||
packet_high_priority.immutable_section().clone(),
|
||||
);
|
||||
&FeatureSet::all_enabled(),
|
||||
));
|
||||
let mut batches = forward_packet_batches_by_accounts.iter_batches();
|
||||
assert_eq!(1, batches.next().unwrap().len());
|
||||
assert_eq!(0, batches.next().unwrap().len());
|
||||
assert!(batches.next().is_none());
|
||||
}
|
||||
|
||||
// 2nd high-priority packet added to 2nd packet
|
||||
// Assert second high-priority packet will not fit in first bucket, but will
|
||||
// be added to 2nd packet
|
||||
{
|
||||
forward_packet_batches_by_accounts.add_packet_to_batches(
|
||||
&[hot_account],
|
||||
requested_cu,
|
||||
assert!(forward_packet_batches_by_accounts.add_packet_to_batches(
|
||||
&tx_high_priority,
|
||||
packet_high_priority.immutable_section().clone(),
|
||||
);
|
||||
&FeatureSet::all_enabled(),
|
||||
));
|
||||
let mut batches = forward_packet_batches_by_accounts.iter_batches();
|
||||
assert_eq!(1, batches.next().unwrap().len());
|
||||
assert_eq!(1, batches.next().unwrap().len());
|
||||
}
|
||||
|
||||
// 3rd high-priority packet not included in forwarding
|
||||
// Assert 3rd high-priority packet can not added since both bucket would
|
||||
// exceed hot-account limit
|
||||
{
|
||||
forward_packet_batches_by_accounts.add_packet_to_batches(
|
||||
&[hot_account],
|
||||
requested_cu,
|
||||
assert!(!forward_packet_batches_by_accounts.add_packet_to_batches(
|
||||
&tx_high_priority,
|
||||
packet_high_priority.immutable_section().clone(),
|
||||
);
|
||||
&FeatureSet::all_enabled(),
|
||||
));
|
||||
let mut batches = forward_packet_batches_by_accounts.iter_batches();
|
||||
assert_eq!(1, batches.next().unwrap().len());
|
||||
assert_eq!(1, batches.next().unwrap().len());
|
||||
assert!(batches.next().is_none());
|
||||
}
|
||||
|
||||
// 4rd lower priority packet added to 1st bucket on non-content account
|
||||
// Assert lower priority packet will be successfully added to first bucket
|
||||
// since non-contentious account is still free
|
||||
{
|
||||
forward_packet_batches_by_accounts.add_packet_to_batches(
|
||||
&[other_account],
|
||||
requested_cu,
|
||||
assert!(forward_packet_batches_by_accounts.add_packet_to_batches(
|
||||
&tx_low_priority,
|
||||
packet_low_priority.immutable_section().clone(),
|
||||
);
|
||||
&FeatureSet::all_enabled(),
|
||||
));
|
||||
let mut batches = forward_packet_batches_by_accounts.iter_batches();
|
||||
assert_eq!(2, batches.next().unwrap().len());
|
||||
assert_eq!(1, batches.next().unwrap().len());
|
||||
|
|
@ -326,21 +324,13 @@ mod tests {
|
|||
|
||||
#[test]
|
||||
fn test_try_add_packet() {
|
||||
solana_logger::setup();
|
||||
// set test batch limit to be 1 millionth of regular block limit
|
||||
let limit_ratio = 1_000_000u32;
|
||||
let (tx, packet, limit_ratio) =
|
||||
build_test_transaction_and_packet(10, &solana_sdk::pubkey::new_rand());
|
||||
let number_of_batches = 1;
|
||||
// set requested_cu to be batch account limit
|
||||
let requested_cu =
|
||||
block_cost_limits::MAX_WRITABLE_ACCOUNT_UNITS.saturating_div(limit_ratio as u64);
|
||||
|
||||
let mut forward_packet_batches_by_accounts =
|
||||
ForwardPacketBatchesByAccounts::new(limit_ratio, number_of_batches);
|
||||
|
||||
let hot_account = solana_sdk::pubkey::new_rand();
|
||||
let other_account = solana_sdk::pubkey::new_rand();
|
||||
|
||||
// assert initially batch is empty, and accepting new packets
|
||||
// Assert initially batch is empty, and accepting new packets
|
||||
{
|
||||
let mut batches = forward_packet_batches_by_accounts.iter_batches();
|
||||
assert_eq!(0, batches.next().unwrap().len());
|
||||
|
|
@ -348,62 +338,45 @@ mod tests {
|
|||
assert!(forward_packet_batches_by_accounts.accepting_packets);
|
||||
}
|
||||
|
||||
// build a packet that would take up hot account limit, add it to batches
|
||||
// assert it is added, and buffer still accepts more packets
|
||||
// Assert can successfully add first packet to forwarding buffer
|
||||
{
|
||||
let packet = build_deserialized_packet_for_test(10, &hot_account, requested_cu);
|
||||
let tx = packet
|
||||
.immutable_section()
|
||||
.build_sanitized_transaction(
|
||||
&Arc::new(FeatureSet::default()),
|
||||
false, //votes_only,
|
||||
SimpleAddressLoader::Disabled,
|
||||
)
|
||||
.unwrap();
|
||||
assert!(forward_packet_batches_by_accounts
|
||||
.try_add_packet(&tx, packet.immutable_section().clone()));
|
||||
assert!(forward_packet_batches_by_accounts.try_add_packet(
|
||||
&tx,
|
||||
packet.immutable_section().clone(),
|
||||
&FeatureSet::all_enabled()
|
||||
));
|
||||
|
||||
let mut batches = forward_packet_batches_by_accounts.iter_batches();
|
||||
assert_eq!(1, batches.next().unwrap().len());
|
||||
assert!(forward_packet_batches_by_accounts.accepting_packets);
|
||||
}
|
||||
|
||||
// build a small packet that writes to hot account, try add it to batches
|
||||
// assert it is not added, and buffer is no longer accept packets
|
||||
// Assert cannot add same packet to forwarding buffer again, due to reached account limit;
|
||||
// Doing so would setting accepting_packets flag to false
|
||||
{
|
||||
let packet =
|
||||
build_deserialized_packet_for_test(100, &hot_account, 1 /*requested_cu*/);
|
||||
let tx = packet
|
||||
.immutable_section()
|
||||
.build_sanitized_transaction(
|
||||
&Arc::new(FeatureSet::default()),
|
||||
false, //votes_only,
|
||||
SimpleAddressLoader::Disabled,
|
||||
)
|
||||
.unwrap();
|
||||
assert!(!forward_packet_batches_by_accounts
|
||||
.try_add_packet(&tx, packet.immutable_section().clone()));
|
||||
assert!(!forward_packet_batches_by_accounts.try_add_packet(
|
||||
&tx,
|
||||
packet.immutable_section().clone(),
|
||||
&FeatureSet::all_enabled()
|
||||
));
|
||||
|
||||
let mut batches = forward_packet_batches_by_accounts.iter_batches();
|
||||
assert_eq!(1, batches.next().unwrap().len());
|
||||
assert!(!forward_packet_batches_by_accounts.accepting_packets);
|
||||
}
|
||||
|
||||
// build a small packet that writes to other account, try add it to batches
|
||||
// assert it is not added due to buffer is no longer accept any packet
|
||||
// Assert no other packets can be added to forwarding buffer since accepting_packets is
|
||||
// now false
|
||||
{
|
||||
let packet =
|
||||
build_deserialized_packet_for_test(100, &other_account, 1 /*requested_cu*/);
|
||||
let tx = packet
|
||||
.immutable_section()
|
||||
.build_sanitized_transaction(
|
||||
&Arc::new(FeatureSet::default()),
|
||||
false, //votes_only,
|
||||
SimpleAddressLoader::Disabled,
|
||||
)
|
||||
.unwrap();
|
||||
assert!(!forward_packet_batches_by_accounts
|
||||
.try_add_packet(&tx, packet.immutable_section().clone()));
|
||||
// build a small packet to a non-contentious account with high priority
|
||||
let (tx2, packet2, _) =
|
||||
build_test_transaction_and_packet(100, &solana_sdk::pubkey::new_rand());
|
||||
|
||||
assert!(!forward_packet_batches_by_accounts.try_add_packet(
|
||||
&tx2,
|
||||
packet2.immutable_section().clone(),
|
||||
&FeatureSet::all_enabled()
|
||||
));
|
||||
|
||||
let mut batches = forward_packet_batches_by_accounts.iter_batches();
|
||||
assert_eq!(1, batches.next().unwrap().len());
|
||||
|
|
|
|||
|
|
@ -259,6 +259,7 @@ impl LatestUnprocessedVotes {
|
|||
if forward_packet_batches_by_accounts.try_add_packet(
|
||||
&sanitized_vote_transaction,
|
||||
deserialized_vote_packet,
|
||||
&bank.feature_set,
|
||||
) {
|
||||
vote.forwarded = true;
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -19,8 +19,8 @@ use {
|
|||
solana_measure::measure,
|
||||
solana_runtime::bank::Bank,
|
||||
solana_sdk::{
|
||||
clock::FORWARD_TRANSACTIONS_TO_LEADER_AT_SLOT_OFFSET, hash::Hash, saturating_add_assign,
|
||||
transaction::SanitizedTransaction,
|
||||
clock::FORWARD_TRANSACTIONS_TO_LEADER_AT_SLOT_OFFSET, feature_set::FeatureSet, hash::Hash,
|
||||
saturating_add_assign, transaction::SanitizedTransaction,
|
||||
},
|
||||
std::{
|
||||
collections::HashMap,
|
||||
|
|
@ -636,6 +636,7 @@ impl ThreadLocalUnprocessedPackets {
|
|||
&transaction_to_packet_indexes,
|
||||
&forwardable_transaction_indexes,
|
||||
&mut dropped_tx_before_forwarding_count,
|
||||
&bank.feature_set,
|
||||
);
|
||||
accepting_packets = accepted_packet_indexes.len()
|
||||
== forwardable_transaction_indexes.len();
|
||||
|
|
@ -798,6 +799,7 @@ impl ThreadLocalUnprocessedPackets {
|
|||
transaction_to_packet_indexes: &[usize],
|
||||
forwardable_transaction_indexes: &[usize],
|
||||
dropped_tx_before_forwarding_count: &mut usize,
|
||||
feature_set: &FeatureSet,
|
||||
) -> Vec<usize> {
|
||||
let mut added_packets_count: usize = 0;
|
||||
let mut accepted_packet_indexes = Vec::with_capacity(transaction_to_packet_indexes.len());
|
||||
|
|
@ -807,8 +809,11 @@ impl ThreadLocalUnprocessedPackets {
|
|||
transaction_to_packet_indexes[*forwardable_transaction_index];
|
||||
let immutable_deserialized_packet =
|
||||
packets_to_process[forwardable_packet_index].clone();
|
||||
if !forward_buffer.try_add_packet(sanitized_transaction, immutable_deserialized_packet)
|
||||
{
|
||||
if !forward_buffer.try_add_packet(
|
||||
sanitized_transaction,
|
||||
immutable_deserialized_packet,
|
||||
feature_set,
|
||||
) {
|
||||
break;
|
||||
}
|
||||
accepted_packet_indexes.push(forwardable_packet_index);
|
||||
|
|
|
|||
|
|
@ -95,18 +95,6 @@ impl CostTracker {
|
|||
Ok(self.block_cost)
|
||||
}
|
||||
|
||||
/// Using user requested compute-units to track cost.
|
||||
pub fn try_add_requested_cus(
|
||||
&mut self,
|
||||
write_lock_accounts: &[Pubkey],
|
||||
requested_cus: u64,
|
||||
is_vote: bool,
|
||||
) -> Result<u64, CostTrackerError> {
|
||||
self.would_fit_internal(write_lock_accounts.iter(), requested_cus, is_vote, 0)?;
|
||||
self.add_transaction_cost_internal(write_lock_accounts.iter(), requested_cus, is_vote, 0);
|
||||
Ok(self.block_cost)
|
||||
}
|
||||
|
||||
pub fn update_execution_cost(
|
||||
&mut self,
|
||||
estimated_tx_cost: &TransactionCost,
|
||||
|
|
|
|||
Loading…
Reference in New Issue