From 7fea2981f36a0b4e1459a8f2be81f360b367ab4f Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Wed, 13 Dec 2023 14:06:11 -0600 Subject: [PATCH 1/8] Move function to the top --- runtime/src/bank_forks.rs | 104 +++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 52 deletions(-) diff --git a/runtime/src/bank_forks.rs b/runtime/src/bank_forks.rs index dabd90e4c2c835..40d708981fb97d 100644 --- a/runtime/src/bank_forks.rs +++ b/runtime/src/bank_forks.rs @@ -90,6 +90,58 @@ impl BankForks { Self::new_from_banks(&[Arc::new(bank)], root) } + pub fn new_from_banks(initial_forks: &[Arc], root: Slot) -> Arc> { + let mut banks = HashMap::new(); + + // Iterate through the heads of all the different forks + for bank in initial_forks { + banks.insert( + bank.slot(), + BankWithScheduler::new_without_scheduler(bank.clone()), + ); + let parents = bank.parents(); + for parent in parents { + if banks + .insert( + parent.slot(), + BankWithScheduler::new_without_scheduler(parent.clone()), + ) + .is_some() + { + // All ancestors have already been inserted by another fork + break; + } + } + } + let mut descendants = HashMap::<_, HashSet<_>>::new(); + for (slot, bank) in &banks { + descendants.entry(*slot).or_default(); + for parent in bank.proper_ancestors() { + descendants.entry(parent).or_default().insert(*slot); + } + } + let bank_forks = Arc::new(RwLock::new(Self { + root: Arc::new(AtomicSlot::new(root)), + banks, + descendants, + snapshot_config: None, + accounts_hash_interval_slots: std::u64::MAX, + last_accounts_hash_slot: root, + in_vote_only_mode: Arc::new(AtomicBool::new(false)), + highest_slot_at_startup: 0, + scheduler_pool: None, + })); + + for bank in bank_forks.read().unwrap().banks.values() { + bank.loaded_programs_cache + .write() + .unwrap() + .set_fork_graph(bank_forks.clone()); + } + + bank_forks + } + pub fn banks(&self) -> &HashMap { &self.banks } @@ -167,58 +219,6 @@ impl BankForks { self[self.root()].clone() } - pub fn new_from_banks(initial_forks: &[Arc], root: Slot) -> Arc> { - let mut banks = HashMap::new(); - - // Iterate through the heads of all the different forks - for bank in initial_forks { - banks.insert( - bank.slot(), - BankWithScheduler::new_without_scheduler(bank.clone()), - ); - let parents = bank.parents(); - for parent in parents { - if banks - .insert( - parent.slot(), - BankWithScheduler::new_without_scheduler(parent.clone()), - ) - .is_some() - { - // All ancestors have already been inserted by another fork - break; - } - } - } - let mut descendants = HashMap::<_, HashSet<_>>::new(); - for (slot, bank) in &banks { - descendants.entry(*slot).or_default(); - for parent in bank.proper_ancestors() { - descendants.entry(parent).or_default().insert(*slot); - } - } - let bank_forks = Arc::new(RwLock::new(Self { - root: Arc::new(AtomicSlot::new(root)), - banks, - descendants, - snapshot_config: None, - accounts_hash_interval_slots: std::u64::MAX, - last_accounts_hash_slot: root, - in_vote_only_mode: Arc::new(AtomicBool::new(false)), - highest_slot_at_startup: 0, - scheduler_pool: None, - })); - - for bank in bank_forks.read().unwrap().banks.values() { - bank.loaded_programs_cache - .write() - .unwrap() - .set_fork_graph(bank_forks.clone()); - } - - bank_forks - } - pub fn install_scheduler_pool(&mut self, pool: InstalledSchedulerPoolArc) { info!("Installed new scheduler_pool into bank_forks: {:?}", pool); assert!( From dc71397d99a5bf15219276c4c6b2b527ed01c32b Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Wed, 13 Dec 2023 14:20:49 -0600 Subject: [PATCH 2/8] Remove option to construct BankForks with multiple banks Remove the unit test for that behavior as well --- runtime/src/bank_forks.rs | 54 ++++++++++++--------------------------- 1 file changed, 17 insertions(+), 37 deletions(-) diff --git a/runtime/src/bank_forks.rs b/runtime/src/bank_forks.rs index 40d708981fb97d..a4e8fe43a77edb 100644 --- a/runtime/src/bank_forks.rs +++ b/runtime/src/bank_forks.rs @@ -86,33 +86,28 @@ impl Index for BankForks { impl BankForks { pub fn new_rw_arc(bank: Bank) -> Arc> { + let bank = Arc::new(bank); let root = bank.slot(); - Self::new_from_banks(&[Arc::new(bank)], root) - } - pub fn new_from_banks(initial_forks: &[Arc], root: Slot) -> Arc> { let mut banks = HashMap::new(); - - // Iterate through the heads of all the different forks - for bank in initial_forks { - banks.insert( - bank.slot(), - BankWithScheduler::new_without_scheduler(bank.clone()), - ); - let parents = bank.parents(); - for parent in parents { - if banks - .insert( - parent.slot(), - BankWithScheduler::new_without_scheduler(parent.clone()), - ) - .is_some() - { - // All ancestors have already been inserted by another fork - break; - } + banks.insert( + bank.slot(), + BankWithScheduler::new_without_scheduler(bank.clone()), + ); + let parents = bank.parents(); + for parent in parents { + if banks + .insert( + parent.slot(), + BankWithScheduler::new_without_scheduler(parent.clone()), + ) + .is_some() + { + // All ancestors have already been inserted by another fork + break; } } + let mut descendants = HashMap::<_, HashSet<_>>::new(); for (slot, bank) in &banks { descendants.entry(*slot).or_default(); @@ -761,21 +756,6 @@ mod tests { assert_eq!(bank_forks.working_bank().tick_height(), 1); } - #[test] - fn test_bank_forks_new_from_banks() { - let GenesisConfigInfo { genesis_config, .. } = create_genesis_config(10_000); - let bank = Arc::new(Bank::new_for_tests(&genesis_config)); - let child_bank = Arc::new(Bank::new_from_parent(bank.clone(), &Pubkey::default(), 1)); - - let bank_forks = BankForks::new_from_banks(&[bank.clone(), child_bank.clone()], 0); - assert_eq!(bank_forks.read().unwrap().root(), 0); - assert_eq!(bank_forks.read().unwrap().working_bank().slot(), 1); - - let bank_forks = BankForks::new_from_banks(&[child_bank, bank], 0); - assert_eq!(bank_forks.read().unwrap().root(), 0); - assert_eq!(bank_forks.read().unwrap().working_bank().slot(), 1); - } - #[test] fn test_bank_forks_descendants() { let GenesisConfigInfo { genesis_config, .. } = create_genesis_config(10_000); From e03592c093a98b48c816d45df13e24163e27d329 Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Wed, 13 Dec 2023 15:46:21 -0600 Subject: [PATCH 3/8] Update unit tests in rpc --- rpc/src/rpc.rs | 48 ++++++++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/rpc/src/rpc.rs b/rpc/src/rpc.rs index 0a92a4d031e9ef..98051b0d557795 100644 --- a/rpc/src/rpc.rs +++ b/rpc/src/rpc.rs @@ -342,12 +342,13 @@ impl JsonRpcRequestProcessor { // Useful for unit testing pub fn new_from_bank( - bank: Arc, + bank: Bank, socket_addr_space: SocketAddrSpace, connection_cache: Arc, ) -> Self { let genesis_hash = bank.hash(); - let bank_forks = BankForks::new_from_banks(&[bank.clone()], bank.slot()); + let bank_forks = BankForks::new_rw_arc(bank); + let bank = bank_forks.read().unwrap().root_bank(); let blockstore = Arc::new(Blockstore::open(&get_tmp_ledger_path!()).unwrap()); let exit = Arc::new(AtomicBool::new(false)); let cluster_info = Arc::new({ @@ -5057,18 +5058,20 @@ pub mod tests { fn test_rpc_request_processor_new() { let bob_pubkey = solana_sdk::pubkey::new_rand(); let genesis = create_genesis_config(100); - let bank = Bank::new_with_bank_forks_for_tests(&genesis.genesis_config).0; - bank.transfer(20, &genesis.mint_keypair, &bob_pubkey) - .unwrap(); + let bank = Bank::new_for_tests(&genesis.genesis_config); let connection_cache = Arc::new(ConnectionCache::new("connection_cache_test")); - let request_processor = JsonRpcRequestProcessor::new_from_bank( + let meta = JsonRpcRequestProcessor::new_from_bank( bank, SocketAddrSpace::Unspecified, connection_cache, ); + + let bank = meta.bank_forks.read().unwrap().root_bank(); + bank.transfer(20, &genesis.mint_keypair, &bob_pubkey) + .unwrap(); + assert_eq!( - request_processor - .get_transaction_count(RpcContextConfig::default()) + meta.get_transaction_count(RpcContextConfig::default()) .unwrap(), 1 ); @@ -5078,7 +5081,7 @@ pub mod tests { fn test_rpc_get_balance() { let genesis = create_genesis_config(20); let mint_pubkey = genesis.mint_keypair.pubkey(); - let bank = Arc::new(Bank::new_for_tests(&genesis.genesis_config)); + let bank = Bank::new_for_tests(&genesis.genesis_config); let connection_cache = Arc::new(ConnectionCache::new("connection_cache_test")); let meta = JsonRpcRequestProcessor::new_from_bank( bank, @@ -5110,7 +5113,7 @@ pub mod tests { fn test_rpc_get_balance_via_client() { let genesis = create_genesis_config(20); let mint_pubkey = genesis.mint_keypair.pubkey(); - let bank = Arc::new(Bank::new_for_tests(&genesis.genesis_config)); + let bank = Bank::new_for_tests(&genesis.genesis_config); let connection_cache = Arc::new(ConnectionCache::new("connection_cache_test")); let meta = JsonRpcRequestProcessor::new_from_bank( bank, @@ -5227,17 +5230,7 @@ pub mod tests { fn test_rpc_get_tx_count() { let bob_pubkey = solana_sdk::pubkey::new_rand(); let genesis = create_genesis_config(10); - let bank = Bank::new_with_bank_forks_for_tests(&genesis.genesis_config).0; - // Add 4 transactions - bank.transfer(1, &genesis.mint_keypair, &bob_pubkey) - .unwrap(); - bank.transfer(2, &genesis.mint_keypair, &bob_pubkey) - .unwrap(); - bank.transfer(3, &genesis.mint_keypair, &bob_pubkey) - .unwrap(); - bank.transfer(4, &genesis.mint_keypair, &bob_pubkey) - .unwrap(); - + let bank = Bank::new_for_tests(&genesis.genesis_config); let connection_cache = Arc::new(ConnectionCache::new("connection_cache_test")); let meta = JsonRpcRequestProcessor::new_from_bank( bank, @@ -5248,6 +5241,17 @@ pub mod tests { let mut io = MetaIoHandler::default(); io.extend_with(rpc_minimal::MinimalImpl.to_delegate()); + // Add 4 transactions + let bank = meta.bank_forks.read().unwrap().root_bank(); + bank.transfer(1, &genesis.mint_keypair, &bob_pubkey) + .unwrap(); + bank.transfer(2, &genesis.mint_keypair, &bob_pubkey) + .unwrap(); + bank.transfer(3, &genesis.mint_keypair, &bob_pubkey) + .unwrap(); + bank.transfer(4, &genesis.mint_keypair, &bob_pubkey) + .unwrap(); + let req = r#"{"jsonrpc":"2.0","id":1,"method":"getTransactionCount"}"#; let res = io.handle_request_sync(req, meta); let expected = r#"{"jsonrpc":"2.0","result":4,"id":1}"#; @@ -6383,7 +6387,7 @@ pub mod tests { #[test] fn test_rpc_send_bad_tx() { let genesis = create_genesis_config(100); - let bank = Arc::new(Bank::new_for_tests(&genesis.genesis_config)); + let bank = Bank::new_for_tests(&genesis.genesis_config); let connection_cache = Arc::new(ConnectionCache::new("connection_cache_test")); let meta = JsonRpcRequestProcessor::new_from_bank( bank, From f810cb333e513a23c707c8077d1bedee6390ad36 Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Wed, 13 Dec 2023 15:49:13 -0600 Subject: [PATCH 4/8] Update unit test in ReplayStage --- core/src/replay_stage.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/replay_stage.rs b/core/src/replay_stage.rs index 8d0a43813c1e30..1b404c71ca5007 100644 --- a/core/src/replay_stage.rs +++ b/core/src/replay_stage.rs @@ -4914,8 +4914,8 @@ pub(crate) mod tests { bank0.register_default_tick_for_test(); } bank0.freeze(); - let arc_bank0 = Arc::new(bank0); - let bank_forks = BankForks::new_from_banks(&[arc_bank0], 0); + //let slot = bank0.slot(); + let bank_forks = BankForks::new_rw_arc(bank0); let exit = Arc::new(AtomicBool::new(false)); let block_commitment_cache = Arc::new(RwLock::new(BlockCommitmentCache::default())); From c62fa5f75e363d0aaab0a713e8c01afa733246c8 Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Wed, 13 Dec 2023 21:06:33 -0600 Subject: [PATCH 5/8] Review feedback - unnecessary comment --- core/src/replay_stage.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/replay_stage.rs b/core/src/replay_stage.rs index 1b404c71ca5007..a175fffa29914d 100644 --- a/core/src/replay_stage.rs +++ b/core/src/replay_stage.rs @@ -4914,7 +4914,6 @@ pub(crate) mod tests { bank0.register_default_tick_for_test(); } bank0.freeze(); - //let slot = bank0.slot(); let bank_forks = BankForks::new_rw_arc(bank0); let exit = Arc::new(AtomicBool::new(false)); From 2a1cf43924f6e1fc5cc2940350a10ae7587bffee Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Wed, 13 Dec 2023 21:32:00 -0600 Subject: [PATCH 6/8] Use root in variable names to be more clear about status of start bank --- runtime/src/bank_forks.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/runtime/src/bank_forks.rs b/runtime/src/bank_forks.rs index a4e8fe43a77edb..80e981912a9629 100644 --- a/runtime/src/bank_forks.rs +++ b/runtime/src/bank_forks.rs @@ -85,16 +85,17 @@ impl Index for BankForks { } impl BankForks { - pub fn new_rw_arc(bank: Bank) -> Arc> { - let bank = Arc::new(bank); - let root = bank.slot(); + pub fn new_rw_arc(root_bank: Bank) -> Arc> { + let root_bank = Arc::new(root_bank); + let root_slot = root_bank.slot(); let mut banks = HashMap::new(); banks.insert( - bank.slot(), - BankWithScheduler::new_without_scheduler(bank.clone()), + root_slot, + BankWithScheduler::new_without_scheduler(root_bank.clone()), ); - let parents = bank.parents(); + + let parents = root_bank.parents(); for parent in parents { if banks .insert( @@ -116,12 +117,12 @@ impl BankForks { } } let bank_forks = Arc::new(RwLock::new(Self { - root: Arc::new(AtomicSlot::new(root)), + root: Arc::new(AtomicSlot::new(root_slot)), banks, descendants, snapshot_config: None, accounts_hash_interval_slots: std::u64::MAX, - last_accounts_hash_slot: root, + last_accounts_hash_slot: root_slot, in_vote_only_mode: Arc::new(AtomicBool::new(false)), highest_slot_at_startup: 0, scheduler_pool: None, From 6a40a811b986468fca08d4c608aa0b6ed8c46ffc Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Wed, 13 Dec 2023 21:33:17 -0600 Subject: [PATCH 7/8] Only call set_fork_graph() on root_bank We know BankForks has exactly one root at this point, so looping is confusing / slightly wasteful --- runtime/src/bank_forks.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/runtime/src/bank_forks.rs b/runtime/src/bank_forks.rs index 80e981912a9629..43807e5b2ebeb9 100644 --- a/runtime/src/bank_forks.rs +++ b/runtime/src/bank_forks.rs @@ -128,12 +128,11 @@ impl BankForks { scheduler_pool: None, })); - for bank in bank_forks.read().unwrap().banks.values() { - bank.loaded_programs_cache - .write() - .unwrap() - .set_fork_graph(bank_forks.clone()); - } + root_bank + .loaded_programs_cache + .write() + .unwrap() + .set_fork_graph(bank_forks.clone()); bank_forks } From 9cac54c8c703d040dca3e524e4c46d346bcc3a66 Mon Sep 17 00:00:00 2001 From: Steven Czabaniuk Date: Wed, 13 Dec 2023 21:48:59 -0600 Subject: [PATCH 8/8] Avoid looping over banks again, we know there is only root_bank --- runtime/src/bank_forks.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/runtime/src/bank_forks.rs b/runtime/src/bank_forks.rs index 43807e5b2ebeb9..622c760d52520e 100644 --- a/runtime/src/bank_forks.rs +++ b/runtime/src/bank_forks.rs @@ -110,12 +110,11 @@ impl BankForks { } let mut descendants = HashMap::<_, HashSet<_>>::new(); - for (slot, bank) in &banks { - descendants.entry(*slot).or_default(); - for parent in bank.proper_ancestors() { - descendants.entry(parent).or_default().insert(*slot); - } + descendants.entry(root_slot).or_default(); + for parent in root_bank.proper_ancestors() { + descendants.entry(parent).or_default().insert(root_slot); } + let bank_forks = Arc::new(RwLock::new(Self { root: Arc::new(AtomicSlot::new(root_slot)), banks,