From 29d12d9ff144b82e84e07134d4e71402f025a4a5 Mon Sep 17 00:00:00 2001 From: Rob Walker Date: Fri, 1 Mar 2019 16:39:23 -0800 Subject: [PATCH] remove new_bank_from_parent_with_id() (#3039) --- runtime/src/bank.rs | 54 ++++++++++++++++++-------------------- src/bank_forks.rs | 3 ++- src/blocktree_processor.rs | 4 +-- src/replay_stage.rs | 13 +++++---- src/rpc_pubsub.rs | 7 ++++- src/staking_utils.rs | 9 +++++-- 6 files changed, 51 insertions(+), 39 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index b7df6f34ff..2964c09157 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -28,7 +28,6 @@ use solana_sdk::token_program; use solana_sdk::transaction::Transaction; use solana_sdk::vote_program::{self, VoteState}; use std::result; -use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, RwLock}; use std::time::Instant; @@ -139,7 +138,7 @@ impl Bank { } /// Create a new bank that points to an immutable checkpoint of another bank. - pub fn new_from_parent_and_id(parent: &Arc, collector_id: Pubkey, id: u64) -> Self { + pub fn new_from_parent(parent: &Arc, collector_id: Pubkey, id: u64) -> Self { parent.freeze(); let mut bank = Self::default(); @@ -171,18 +170,6 @@ impl Bank { bank } - /// Create a new bank that points to an immutable checkpoint of another bank. - /// TODO: remove me in favor of _and_id(), id should not be an assumed value - pub fn new_from_parent(parent: &Arc) -> Self { - static BANK_ID: AtomicUsize = AtomicUsize::new(1); - let collector_id = parent.collector_id; - Self::new_from_parent_and_id( - parent, - collector_id, - BANK_ID.fetch_add(1, Ordering::Relaxed) as u64, - ) - } - pub fn slot(&self) -> u64 { self.slot } @@ -233,7 +220,7 @@ impl Bank { assert!(genesis_block.tokens >= genesis_block.bootstrap_leader_tokens); assert!(genesis_block.bootstrap_leader_tokens >= 2); - // Bootstrap leader collects fees until `new_from_parent_and_id` is called. + // Bootstrap leader collects fees until `new_from_parent` is called. self.collector_id = genesis_block.bootstrap_leader_id; let mint_tokens = genesis_block.tokens - genesis_block.bootstrap_leader_tokens; @@ -1312,13 +1299,17 @@ mod tests { res[0].clone().unwrap_err(); } + fn new_from_parent(parent: &Arc) -> Bank { + Bank::new_from_parent(parent, Pubkey::default(), parent.slot() + 1) + } + /// Verify that the parent's vector is computed correctly #[test] fn test_bank_parents() { let (genesis_block, _) = GenesisBlock::new(1); let parent = Arc::new(Bank::new(&genesis_block)); - let bank = Bank::new_from_parent(&parent); + let bank = new_from_parent(&parent); assert!(Arc::ptr_eq(&bank.parents()[0], &parent)); } @@ -1332,7 +1323,7 @@ mod tests { let tx = SystemTransaction::new_move(&mint_keypair, key1.pubkey(), 1, genesis_block.hash(), 0); assert_eq!(parent.process_transaction(&tx), Ok(())); - let bank = Bank::new_from_parent(&parent); + let bank = new_from_parent(&parent); assert_eq!( bank.process_transaction(&tx), Err(BankError::DuplicateSignature) @@ -1350,7 +1341,7 @@ mod tests { let tx = SystemTransaction::new_move(&mint_keypair, key1.pubkey(), 1, genesis_block.hash(), 0); assert_eq!(parent.process_transaction(&tx), Ok(())); - let bank = Bank::new_from_parent(&parent); + let bank = new_from_parent(&parent); let tx = SystemTransaction::new_move(&key1, key2.pubkey(), 1, genesis_block.hash(), 0); assert_eq!(bank.process_transaction(&tx), Ok(())); assert_eq!(parent.get_signature_status(&tx.signatures[0]), None); @@ -1375,7 +1366,7 @@ mod tests { assert_eq!(bank0.hash_internal_state(), bank1.hash_internal_state()); // Checkpointing should not change its state - let bank2 = Bank::new_from_parent(&Arc::new(bank1)); + let bank2 = new_from_parent(&Arc::new(bank1)); assert_eq!(bank0.hash_internal_state(), bank2.hash_internal_state()); } @@ -1390,7 +1381,7 @@ mod tests { fn test_bank_hash_internal_state_squash() { let collector_id = Pubkey::default(); let bank0 = Arc::new(Bank::new(&GenesisBlock::new(10).0)); - let bank1 = Bank::new_from_parent_and_id(&bank0, collector_id, 1); + let bank1 = Bank::new_from_parent(&bank0, collector_id, 1); // no delta in bank1, hashes match assert_eq!(bank0.hash_internal_state(), bank1.hash_internal_state()); @@ -1416,7 +1407,7 @@ mod tests { assert_eq!(parent.process_transaction(&tx_move_mint_to_1), Ok(())); assert_eq!(parent.transaction_count(), 1); - let bank = Bank::new_from_parent(&parent); + let bank = new_from_parent(&parent); assert_eq!(bank.transaction_count(), 0); let tx_move_1_to_2 = SystemTransaction::new_move(&key1, key2.pubkey(), 1, genesis_block.hash(), 0); @@ -1460,7 +1451,7 @@ mod tests { .transfer(1, &mint_keypair, key1.pubkey(), genesis_block.hash()) .unwrap(); assert_eq!(parent.get_balance(&key1.pubkey()), 1); - let bank = Bank::new_from_parent(&parent); + let bank = new_from_parent(&parent); bank.squash(); assert_eq!(parent.get_balance(&key1.pubkey()), 1); } @@ -1496,24 +1487,31 @@ mod tests { Some((*key, None)) } - assert!(parent.epoch_vote_states(1, all).is_some()); - assert!(parent.epoch_vote_states(2, all).is_some()); + let mut i = 1; + loop { + if i > STAKERS_SLOT_OFFSET / SLOTS_PER_EPOCH { + break; + } + assert!(parent.epoch_vote_states(i, all).is_some()); + i += 1; + } // child crosses epoch boundary and is the first slot in the epoch - let child = Bank::new_from_parent_and_id( + let child = Bank::new_from_parent( &parent, leader_id, SLOTS_PER_EPOCH - (STAKERS_SLOT_OFFSET % SLOTS_PER_EPOCH), ); - assert!(child.epoch_vote_states(3, all).is_some()); + + assert!(child.epoch_vote_states(i, all).is_some()); // child crosses epoch boundary but isn't the first slot in the epoch - let child = Bank::new_from_parent_and_id( + let child = Bank::new_from_parent( &parent, leader_id, SLOTS_PER_EPOCH - (STAKERS_SLOT_OFFSET % SLOTS_PER_EPOCH) + 1, ); - assert!(child.epoch_vote_states(3, all).is_some()); + assert!(child.epoch_vote_states(i, all).is_some()); } } diff --git a/src/bank_forks.rs b/src/bank_forks.rs index 6a943562b4..a40cdc6b65 100644 --- a/src/bank_forks.rs +++ b/src/bank_forks.rs @@ -69,13 +69,14 @@ mod tests { use super::*; use solana_sdk::genesis_block::GenesisBlock; use solana_sdk::hash::Hash; + use solana_sdk::pubkey::Pubkey; #[test] fn test_bank_forks() { let (genesis_block, _) = GenesisBlock::new(10_000); let bank = Bank::new(&genesis_block); let mut bank_forks = BankForks::new(0, bank); - let child_bank = Bank::new_from_parent(&bank_forks[0u64]); + let child_bank = Bank::new_from_parent(&bank_forks[0u64], Pubkey::default(), 1); child_bank.register_tick(&Hash::default()); bank_forks.insert(1, child_bank); assert_eq!(bank_forks[1u64].tick_height(), 1); diff --git a/src/blocktree_processor.rs b/src/blocktree_processor.rs index 1050beae75..be45c9cbb4 100644 --- a/src/blocktree_processor.rs +++ b/src/blocktree_processor.rs @@ -124,7 +124,7 @@ pub fn process_blocktree( let (slot, starting_bank, starting_entry_height, mut last_entry_hash) = pending_slots.pop().unwrap(); - let bank = Arc::new(Bank::new_from_parent_and_id( + let bank = Arc::new(Bank::new_from_parent( &starting_bank, leader_schedule_utils::slot_leader_at(slot, &starting_bank), starting_bank.slot(), @@ -213,7 +213,7 @@ pub fn process_blocktree( // This is a fork point, create a new child bank for each fork pending_slots.extend(meta.next_slots.iter().map(|next_slot| { - let child_bank = Arc::new(Bank::new_from_parent_and_id( + let child_bank = Arc::new(Bank::new_from_parent( &bank, leader_schedule_utils::slot_leader_at(*next_slot, &bank), *next_slot, diff --git a/src/replay_stage.rs b/src/replay_stage.rs index 9c8635642c..397f62a0de 100644 --- a/src/replay_stage.rs +++ b/src/replay_stage.rs @@ -200,7 +200,7 @@ impl ReplayStage { // If the next slot is going to be a new slot and we're the leader for that slot, // make a new working bank, set it as the working bank. if tick_height + 1 == first_tick_in_slot && leader_id == my_id { - bank = Self::create_and_set_working_bank(slot, &bank_forks, &old_bank); + bank = Self::create_and_set_working_bank(&old_bank, leader_id, slot, &bank_forks); } // Send a rotation notification back to Fullnode to initialize the TPU to the right @@ -254,9 +254,10 @@ impl ReplayStage { trace!("{} replay_stage: new_slot found: {:?}", my_id, new_slot); // Reset the state bank = Self::create_and_set_working_bank( + &bank, + current_leader_id, new_slot.unwrap(), &bank_forks, - &bank, ); current_slot = new_slot; Self::reset_state( @@ -346,9 +347,10 @@ impl ReplayStage { if my_id == leader_id { // Create new bank for next slot if we are the leader for that slot bank = Self::create_and_set_working_bank( + &old_bank, + leader_id, next_slot, &bank_forks, - &old_bank, ); current_slot = Some(next_slot); Self::reset_state( @@ -406,11 +408,12 @@ impl ReplayStage { } fn create_and_set_working_bank( + parent: &Arc, + leader_id: Pubkey, slot: u64, bank_forks: &Arc>, - parent: &Arc, ) -> Arc { - let new_bank = Bank::new_from_parent(&parent); + let new_bank = Bank::new_from_parent(&parent, leader_id, slot); new_bank.squash(); let mut bank_forks = bank_forks.write().unwrap(); bank_forks.insert(slot, new_bank); diff --git a/src/rpc_pubsub.rs b/src/rpc_pubsub.rs index 6bf2c1616c..ac4d615035 100644 --- a/src/rpc_pubsub.rs +++ b/src/rpc_pubsub.rs @@ -168,6 +168,7 @@ mod tests { use solana_sdk::budget_program; use solana_sdk::budget_transaction::BudgetTransaction; use solana_sdk::genesis_block::GenesisBlock; + use solana_sdk::pubkey::Pubkey; use solana_sdk::signature::{Keypair, KeypairUtil}; use solana_sdk::system_transaction::SystemTransaction; use solana_sdk::transaction::Transaction; @@ -184,7 +185,11 @@ mod tests { subscriptions.notify_subscribers(&bank); // Simulate a block boundary - Ok(Arc::new(Bank::new_from_parent(&bank))) + Ok(Arc::new(Bank::new_from_parent( + &bank, + Pubkey::default(), + bank.slot() + 1, + ))) } fn create_session() -> Arc { diff --git a/src/staking_utils.rs b/src/staking_utils.rs index fe76a86709..cb62dad011 100644 --- a/src/staking_utils.rs +++ b/src/staking_utils.rs @@ -120,6 +120,7 @@ mod tests { use hashbrown::HashSet; use solana_sdk::genesis_block::GenesisBlock; use solana_sdk::hash::Hash; + use solana_sdk::pubkey::Pubkey; use solana_sdk::signature::{Keypair, KeypairUtil}; use std::iter::FromIterator; use std::sync::Arc; @@ -131,20 +132,24 @@ mod tests { (bank.tick_index(), bank.slot_index(), bank.epoch_height()) } + fn new_from_parent(parent: &Arc) -> Bank { + Bank::new_from_parent(parent, Pubkey::default(), parent.slot() + 1) + } + #[test] fn test_bank_staked_nodes_at_epoch() { let pubkey = Keypair::new().pubkey(); let bootstrap_tokens = 2; let (genesis_block, _) = GenesisBlock::new_with_leader(2, pubkey, bootstrap_tokens); let bank = Bank::new(&genesis_block); - let bank = Bank::new_from_parent(&Arc::new(bank)); + let bank = new_from_parent(&Arc::new(bank)); let ticks_per_offset = bank.stakers_slot_offset() * bank.ticks_per_slot(); register_ticks(&bank, ticks_per_offset); assert_eq!(bank.slot_height(), bank.stakers_slot_offset()); let mut expected = HashMap::new(); expected.insert(pubkey, bootstrap_tokens - 1); - let bank = Bank::new_from_parent(&Arc::new(bank)); + let bank = new_from_parent(&Arc::new(bank)); assert_eq!( node_stakes_at_slot_extractor(&bank, bank.slot_height(), |s, _| s), expected