diff --git a/core/src/replay_stage.rs b/core/src/replay_stage.rs index 6fda2e0ab4..dcb85ab6a0 100644 --- a/core/src/replay_stage.rs +++ b/core/src/replay_stage.rs @@ -5879,7 +5879,7 @@ pub mod tests { // Simulate landing a vote for slot 0 landing in slot 1 let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1)); - bank1.fill_bank_with_ticks(); + bank1.fill_bank_with_ticks_for_tests(); tower.record_bank_vote(&bank0, &my_vote_pubkey); ReplayStage::push_vote( &bank0, @@ -5919,7 +5919,7 @@ pub mod tests { // Trying to refresh the vote for bank 0 in bank 1 or bank 2 won't succeed because // the last vote has landed already let bank2 = Arc::new(Bank::new_from_parent(&bank1, &Pubkey::default(), 2)); - bank2.fill_bank_with_ticks(); + bank2.fill_bank_with_ticks_for_tests(); bank2.freeze(); for refresh_bank in &[&bank1, &bank2] { ReplayStage::refresh_last_vote( @@ -6001,13 +6001,19 @@ pub mod tests { assert_eq!(tower.last_voted_slot().unwrap(), 1); // Create a bank where the last vote transaction will have expired - let expired_bank = Arc::new(Bank::new_from_parent( - &bank2, - &Pubkey::default(), - bank2.slot() + MAX_PROCESSING_AGE as Slot, - )); - expired_bank.fill_bank_with_ticks(); - expired_bank.freeze(); + let expired_bank = { + let mut parent_bank = bank2.clone(); + for _ in 0..MAX_PROCESSING_AGE { + parent_bank = Arc::new(Bank::new_from_parent( + &parent_bank, + &Pubkey::default(), + parent_bank.slot() + 1, + )); + parent_bank.fill_bank_with_ticks_for_tests(); + parent_bank.freeze(); + } + parent_bank + }; // Now trying to refresh the vote for slot 1 will succeed because the recent blockhash // of the last vote transaction has expired @@ -6070,7 +6076,7 @@ pub mod tests { vote_account.vote_state().as_ref().unwrap().tower(), vec![0, 1] ); - expired_bank_child.fill_bank_with_ticks(); + expired_bank_child.fill_bank_with_ticks_for_tests(); expired_bank_child.freeze(); // Trying to refresh the vote on a sibling bank where: @@ -6082,7 +6088,7 @@ pub mod tests { &Pubkey::default(), expired_bank_child.slot() + 1, )); - expired_bank_sibling.fill_bank_with_ticks(); + expired_bank_sibling.fill_bank_with_ticks_for_tests(); expired_bank_sibling.freeze(); // Set the last refresh to now, shouldn't refresh because the last refresh just happened. last_vote_refresh_time.last_refresh_time = Instant::now(); diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index ad26f7b4ba..9cd67c5e6e 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -3951,7 +3951,7 @@ pub mod tests { } #[test] - fn test_confirm_slot_entries() { + fn test_confirm_slot_entries_without_fix() { const HASHES_PER_TICK: u64 = 10; const TICKS_PER_SLOT: u64 = 2; @@ -3966,7 +3966,9 @@ pub mod tests { genesis_config.ticks_per_slot = TICKS_PER_SLOT; let genesis_hash = genesis_config.hash(); - let slot_0_bank = Arc::new(Bank::new_for_tests(&genesis_config)); + let mut slot_0_bank = Bank::new_for_tests(&genesis_config); + slot_0_bank.deactivate_feature(&feature_set::fix_recent_blockhashes::id()); + let slot_0_bank = Arc::new(slot_0_bank); assert_eq!(slot_0_bank.slot(), 0); assert_eq!(slot_0_bank.tick_height(), 0); assert_eq!(slot_0_bank.max_tick_height(), 2); @@ -4033,4 +4035,124 @@ pub mod tests { assert_eq!(slot_2_bank.get_hash_age(&slot_1_hash), Some(1)); assert_eq!(slot_2_bank.get_hash_age(&slot_2_hash), Some(0)); } + + #[test] + fn test_confirm_slot_entries_with_fix() { + const HASHES_PER_TICK: u64 = 10; + const TICKS_PER_SLOT: u64 = 2; + + let collector_id = Pubkey::new_unique(); + + let GenesisConfigInfo { + mut genesis_config, + mint_keypair, + .. + } = create_genesis_config(10_000); + genesis_config.poh_config.hashes_per_tick = Some(HASHES_PER_TICK); + genesis_config.ticks_per_slot = TICKS_PER_SLOT; + let genesis_hash = genesis_config.hash(); + + let slot_0_bank = Arc::new(Bank::new_for_tests(&genesis_config)); + assert_eq!(slot_0_bank.slot(), 0); + assert_eq!(slot_0_bank.tick_height(), 0); + assert_eq!(slot_0_bank.max_tick_height(), 2); + assert_eq!(slot_0_bank.last_blockhash(), genesis_hash); + assert_eq!(slot_0_bank.get_hash_age(&genesis_hash), Some(0)); + + let slot_0_entries = entry::create_ticks(TICKS_PER_SLOT, HASHES_PER_TICK, genesis_hash); + let slot_0_hash = slot_0_entries.last().unwrap().hash; + confirm_slot_entries_for_tests(&slot_0_bank, slot_0_entries, true, genesis_hash).unwrap(); + assert_eq!(slot_0_bank.tick_height(), slot_0_bank.max_tick_height()); + assert_eq!(slot_0_bank.last_blockhash(), slot_0_hash); + assert_eq!(slot_0_bank.get_hash_age(&genesis_hash), Some(1)); + assert_eq!(slot_0_bank.get_hash_age(&slot_0_hash), Some(0)); + + let slot_2_bank = Arc::new(Bank::new_from_parent(&slot_0_bank, &collector_id, 2)); + assert_eq!(slot_2_bank.slot(), 2); + assert_eq!(slot_2_bank.tick_height(), 2); + assert_eq!(slot_2_bank.max_tick_height(), 6); + assert_eq!(slot_2_bank.last_blockhash(), slot_0_hash); + + let slot_1_entries = entry::create_ticks(TICKS_PER_SLOT, HASHES_PER_TICK, slot_0_hash); + let slot_1_hash = slot_1_entries.last().unwrap().hash; + confirm_slot_entries_for_tests(&slot_2_bank, slot_1_entries, false, slot_0_hash).unwrap(); + assert_eq!(slot_2_bank.tick_height(), 4); + assert_eq!(slot_2_bank.last_blockhash(), slot_0_hash); + assert_eq!(slot_2_bank.get_hash_age(&genesis_hash), Some(1)); + assert_eq!(slot_2_bank.get_hash_age(&slot_0_hash), Some(0)); + + struct TestCase { + recent_blockhash: Hash, + expected_result: result::Result<(), BlockstoreProcessorError>, + } + + let test_cases = [ + TestCase { + recent_blockhash: slot_1_hash, + expected_result: Err(BlockstoreProcessorError::InvalidTransaction( + TransactionError::BlockhashNotFound, + )), + }, + TestCase { + recent_blockhash: slot_0_hash, + expected_result: Ok(()), + }, + ]; + + // Check that slot 2 transactions can only use hashes for completed blocks. + for TestCase { + recent_blockhash, + expected_result, + } in test_cases + { + let slot_2_entries = { + let to_pubkey = Pubkey::new_unique(); + let mut prev_entry_hash = slot_1_hash; + let mut remaining_entry_hashes = HASHES_PER_TICK; + + let tx = + system_transaction::transfer(&mint_keypair, &to_pubkey, 1, recent_blockhash); + remaining_entry_hashes = remaining_entry_hashes.checked_sub(1).unwrap(); + let mut entries = vec![next_entry_mut(&mut prev_entry_hash, 1, vec![tx])]; + + entries.push(next_entry_mut( + &mut prev_entry_hash, + remaining_entry_hashes, + vec![], + )); + entries.push(next_entry_mut( + &mut prev_entry_hash, + HASHES_PER_TICK, + vec![], + )); + + entries + }; + + let slot_2_hash = slot_2_entries.last().unwrap().hash; + let result = + confirm_slot_entries_for_tests(&slot_2_bank, slot_2_entries, true, slot_1_hash); + match (result, expected_result) { + (Ok(()), Ok(())) => { + assert_eq!(slot_2_bank.tick_height(), slot_2_bank.max_tick_height()); + assert_eq!(slot_2_bank.last_blockhash(), slot_2_hash); + assert_eq!(slot_2_bank.get_hash_age(&genesis_hash), Some(2)); + assert_eq!(slot_2_bank.get_hash_age(&slot_0_hash), Some(1)); + assert_eq!(slot_2_bank.get_hash_age(&slot_2_hash), Some(0)); + } + ( + Err(BlockstoreProcessorError::InvalidTransaction(err)), + Err(BlockstoreProcessorError::InvalidTransaction(expected_err)), + ) => { + assert_eq!(err, expected_err); + } + (result, expected_result) => { + panic!( + "actual result {:?} != expected result {:?}", + result, expected_result + ); + } + } + } + } } diff --git a/poh/src/poh_recorder.rs b/poh/src/poh_recorder.rs index b54ee0a9e2..bc1acafea9 100644 --- a/poh/src/poh_recorder.rs +++ b/poh/src/poh_recorder.rs @@ -1127,7 +1127,7 @@ mod tests { Arc::new(AtomicBool::default()), ); - bank0.fill_bank_with_ticks(); + bank0.fill_bank_with_ticks_for_tests(); let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1)); // Set a working bank @@ -1239,7 +1239,7 @@ mod tests { Arc::new(AtomicBool::default()), ); - bank0.fill_bank_with_ticks(); + bank0.fill_bank_with_ticks_for_tests(); let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1)); poh_recorder.set_bank(&bank1); // Let poh_recorder tick up to bank1.tick_height() - 1 @@ -1324,7 +1324,7 @@ mod tests { Arc::new(AtomicBool::default()), ); - bank0.fill_bank_with_ticks(); + bank0.fill_bank_with_ticks_for_tests(); let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1)); poh_recorder.set_bank(&bank1); @@ -1420,7 +1420,7 @@ mod tests { Arc::new(AtomicBool::default()), ); - bank0.fill_bank_with_ticks(); + bank0.fill_bank_with_ticks_for_tests(); let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1)); poh_recorder.set_bank(&bank1); @@ -1962,12 +1962,13 @@ mod tests { ); //create a new bank let bank = Arc::new(Bank::new_from_parent(&bank, &Pubkey::default(), 2)); - //put 2 slots worth of virtual ticks into poh - for _ in 0..(bank.ticks_per_slot() * 2) { + // add virtual ticks into poh for slots 0, 1, and 2 + for _ in 0..(bank.ticks_per_slot() * 3) { poh_recorder.tick(); } poh_recorder.set_bank(&bank); - assert!(!bank.is_hash_valid_for_age(&genesis_hash, 1)); + assert!(!bank.is_hash_valid_for_age(&genesis_hash, 0)); + assert!(bank.is_hash_valid_for_age(&genesis_hash, 1)); } } diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs index ce1401b919..3cae577bb6 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -26,7 +26,7 @@ use { clock::Slot, entrypoint::{ProgramResult, SUCCESS}, feature_set::FEATURE_NAMES, - fee_calculator::{FeeCalculator, FeeRateGovernor}, + fee_calculator::{FeeCalculator, FeeRateGovernor, DEFAULT_TARGET_LAMPORTS_PER_SIGNATURE}, genesis_config::{ClusterType, GenesisConfig}, hash::Hash, instruction::{Instruction, InstructionError}, @@ -421,41 +421,6 @@ pub fn read_file>(path: P) -> Vec { file_data } -fn setup_fees(bank: Bank) -> Bank { - // Realistic fees part 1: Fake a single signature by calling - // `bank.commit_transactions()` so that the fee in the child bank will be - // initialized with a non-zero fee. - assert_eq!(bank.signature_count(), 0); - bank.commit_transactions( - &[], // transactions - &mut [], // loaded accounts - vec![], // transaction execution results - 0, // executed tx count - 0, // executed with failure output tx count - 1, // signature count - &mut ExecuteTimings::default(), - ); - assert_eq!(bank.signature_count(), 1); - - // Advance beyond slot 0 for a slightly more realistic test environment - let bank = Arc::new(bank); - let bank = Bank::new_from_parent(&bank, bank.collector_id(), bank.slot() + 1); - debug!("Bank slot: {}", bank.slot()); - - // Realistic fees part 2: Tick until a new blockhash is produced to pick up the - // non-zero fees - let last_blockhash = bank.last_blockhash(); - while last_blockhash == bank.last_blockhash() { - bank.register_tick(&Hash::new_unique()); - } - - // Make sure a fee is now required - let lamports_per_signature = bank.get_lamports_per_signature(); - assert_ne!(lamports_per_signature, 0); - - bank -} - pub struct ProgramTest { accounts: Vec<(Pubkey, AccountSharedData)>, builtins: Vec, @@ -748,7 +713,11 @@ impl ProgramTest { } let rent = Rent::default(); - let fee_rate_governor = FeeRateGovernor::default(); + let fee_rate_governor = FeeRateGovernor { + // Initialize with a non-zero fee + lamports_per_signature: DEFAULT_TARGET_LAMPORTS_PER_SIGNATURE / 2, + ..FeeRateGovernor::default() + }; let bootstrap_validator_pubkey = Pubkey::new_unique(); let bootstrap_validator_stake_lamports = rent.minimum_balance(VoteState::size_of()) + sol_to_lamports(1_000_000.0); @@ -837,7 +806,14 @@ impl ProgramTest { ..ComputeBudget::default() })); } - let bank = setup_fees(bank); + // Advance beyond slot 0 for a slightly more realistic test environment + let bank = { + let bank = Arc::new(bank); + bank.fill_bank_with_ticks_for_tests(); + let bank = Bank::new_from_parent(&bank, bank.collector_id(), bank.slot() + 1); + debug!("Bank slot: {}", bank.slot()); + bank + }; let slot = bank.slot(); let last_blockhash = bank.last_blockhash(); let bank_forks = Arc::new(RwLock::new(BankForks::new(bank))); @@ -861,6 +837,7 @@ impl ProgramTest { pub async fn start(self) -> (BanksClient, Keypair, Hash) { let (bank_forks, block_commitment_cache, last_blockhash, gci) = self.setup_bank(); let target_tick_duration = gci.genesis_config.poh_config.target_tick_duration; + let target_slot_duration = target_tick_duration * gci.genesis_config.ticks_per_slot as u32; let transport = start_local_server( bank_forks.clone(), block_commitment_cache.clone(), @@ -876,12 +853,12 @@ impl ProgramTest { // test tokio::spawn(async move { loop { + tokio::time::sleep(target_slot_duration).await; bank_forks .read() .unwrap() .working_bank() - .register_tick(&Hash::new_unique()); - tokio::time::sleep(target_tick_duration).await; + .register_recent_blockhash(&Hash::new_unique()); } }); @@ -1018,6 +995,8 @@ impl ProgramTestContext { .genesis_config .poh_config .target_tick_duration; + let target_slot_duration = + target_tick_duration * genesis_config_info.genesis_config.ticks_per_slot as u32; let exit = Arc::new(AtomicBool::new(false)); let bank_task = DroppableTask( exit.clone(), @@ -1026,12 +1005,12 @@ impl ProgramTestContext { if exit.load(Ordering::Relaxed) { break; } + tokio::time::sleep(target_slot_duration).await; running_bank_forks .read() .unwrap() .working_bank() - .register_tick(&Hash::new_unique()); - tokio::time::sleep(target_tick_duration).await; + .register_recent_blockhash(&Hash::new_unique()); } }), ); @@ -1102,12 +1081,9 @@ impl ProgramTestContext { let mut bank_forks = self.bank_forks.write().unwrap(); let bank = bank_forks.working_bank(); - // Force ticks until a new blockhash, otherwise retried transactions will have + // Fill ticks until a new blockhash is recorded, otherwise retried transactions will have // the same signature - let last_blockhash = bank.last_blockhash(); - while last_blockhash == bank.last_blockhash() { - bank.register_tick(&Hash::new_unique()); - } + bank.fill_bank_with_ticks_for_tests(); // Ensure that we are actually progressing forward let working_slot = bank.slot(); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index fc7b9c9dd6..db805dd888 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3463,6 +3463,18 @@ impl Bank { } } + /// Register a new recent blockhash in the bank's recent blockhash queue. Called when a bank + /// reaches its max tick height. Can be called by tests to get new blockhashes for transaction + /// processing without advancing to a new bank slot. + pub fn register_recent_blockhash(&self, blockhash: &Hash) { + // Only acquire the write lock for the blockhash queue on block boundaries because + // readers can starve this write lock acquisition and ticks would be slowed down too + // much if the write lock is acquired for each tick. + let mut w_blockhash_queue = self.blockhash_queue.write().unwrap(); + w_blockhash_queue.register_hash(blockhash, self.fee_rate_governor.lamports_per_signature); + self.update_recent_blockhashes_locked(&w_blockhash_queue); + } + /// Tell the bank which Entry IDs exist on the ledger. This function assumes subsequent calls /// correspond to later entries, and will boot the oldest ones once its internal cache is full. /// Once boot, the bank will reject transactions using that `hash`. @@ -3477,12 +3489,7 @@ impl Bank { inc_new_counter_debug!("bank-register_tick-registered", 1); if self.is_block_boundary(self.tick_height.load(Relaxed) + 1) { - // Only acquire the write lock for the blockhash queue on block boundaries because - // readers can starve this write lock acquisition and ticks would be slowed down too - // much if the write lock is acquired for each tick. - let mut w_blockhash_queue = self.blockhash_queue.write().unwrap(); - w_blockhash_queue.register_hash(hash, self.fee_rate_governor.lamports_per_signature); - self.update_recent_blockhashes_locked(&w_blockhash_queue); + self.register_recent_blockhash(hash); } // ReplayStage will start computing the accounts delta hash when it @@ -3498,7 +3505,14 @@ impl Bank { } pub fn is_block_boundary(&self, tick_height: u64) -> bool { - tick_height % self.ticks_per_slot == 0 + if self + .feature_set + .is_active(&feature_set::fix_recent_blockhashes::id()) + { + tick_height == self.max_tick_height + } else { + tick_height % self.ticks_per_slot == 0 + } } /// Prepare a transaction batch from a list of legacy transactions. Used for tests only. @@ -6567,17 +6581,14 @@ impl Bank { self.feature_set = Arc::new(feature_set); } - pub fn fill_bank_with_ticks(&self) { - let parent_distance = if self.slot() == 0 { - 1 - } else { - self.slot() - self.parent_slot() - }; - for _ in 0..parent_distance { + pub fn fill_bank_with_ticks_for_tests(&self) { + if self.tick_height.load(Relaxed) < self.max_tick_height { let last_blockhash = self.last_blockhash(); while self.last_blockhash() == last_blockhash { self.register_tick(&Hash::new_unique()) } + } else { + warn!("Bank already reached max tick height, cannot fill it with more ticks"); } } diff --git a/sdk/program/src/clock.rs b/sdk/program/src/clock.rs index e7181d0496..cc15900028 100644 --- a/sdk/program/src/clock.rs +++ b/sdk/program/src/clock.rs @@ -57,7 +57,7 @@ pub const MAX_HASH_AGE_IN_SECONDS: usize = 120; #[cfg(test)] static_assertions::const_assert_eq!(MAX_RECENT_BLOCKHASHES, 300); -// Number of maximum recent blockhashes (one blockhash per slot) +// Number of maximum recent blockhashes (one blockhash per non-skipped slot) pub const MAX_RECENT_BLOCKHASHES: usize = MAX_HASH_AGE_IN_SECONDS * DEFAULT_TICKS_PER_SECOND as usize / DEFAULT_TICKS_PER_SLOT as usize; diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 470aa3833e..e2d1e48871 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -355,6 +355,10 @@ pub mod executables_incur_cpi_data_cost { solana_sdk::declare_id!("7GUcYgq4tVtaqNCKT3dho9r4665Qp5TxCZ27Qgjx3829"); } +pub mod fix_recent_blockhashes { + solana_sdk::declare_id!("6iyggb5MTcsvdcugX7bEKbHV8c6jdLbpHwkncrgLMhfo"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -438,6 +442,7 @@ lazy_static! { (reject_callx_r10::id(), "Reject bpf callx r10 instructions"), (drop_redundant_turbine_path::id(), "drop redundant turbine path"), (executables_incur_cpi_data_cost::id(), "Executables incure CPI data costs"), + (fix_recent_blockhashes::id(), "stop adding hashes for skipped slots to recent blockhashes"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()