From 5850af53169741e71cf548ada179b6476dd8891d Mon Sep 17 00:00:00 2001 From: Tao Zhu <82401714+taozhu-chicago@users.noreply.github.com> Date: Thu, 1 Dec 2022 18:25:09 -0600 Subject: [PATCH] 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 --- .../src/forward_packet_batches_by_accounts.rs | 273 ++++++++---------- core/src/latest_unprocessed_votes.rs | 1 + core/src/unprocessed_transaction_storage.rs | 13 +- runtime/src/cost_tracker.rs | 12 - 4 files changed, 133 insertions(+), 166 deletions(-) diff --git a/core/src/forward_packet_batches_by_accounts.rs b/core/src/forward_packet_batches_by_accounts.rs index 4273445db73cbf..5b6b35e514d662 100644 --- a/core/src/forward_packet_batches_by_accounts.rs +++ b/core/src/forward_packet_batches_by_accounts.rs @@ -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, + feature_set: &FeatureSet, ) -> Result { - 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, + 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, + 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()); diff --git a/core/src/latest_unprocessed_votes.rs b/core/src/latest_unprocessed_votes.rs index 4fe4096c3882b4..cd0c3522b846d5 100644 --- a/core/src/latest_unprocessed_votes.rs +++ b/core/src/latest_unprocessed_votes.rs @@ -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 { diff --git a/core/src/unprocessed_transaction_storage.rs b/core/src/unprocessed_transaction_storage.rs index e4d15af0ab40fa..5d2e0fe5ee25b2 100644 --- a/core/src/unprocessed_transaction_storage.rs +++ b/core/src/unprocessed_transaction_storage.rs @@ -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 { 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); diff --git a/runtime/src/cost_tracker.rs b/runtime/src/cost_tracker.rs index 2398bce6890d32..7734d87d73457c 100644 --- a/runtime/src/cost_tracker.rs +++ b/runtime/src/cost_tracker.rs @@ -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 { - 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,