From 63b18a1c008c879e35399d76b3d9b71b8f47f545 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Fri, 12 Sep 2025 06:14:46 -0700 Subject: [PATCH 01/20] Make the following blocking operations async: - add_header, which may block on disk I/O. - to_socket_addrs, which is may block on DNS lookup. --- rs/bitcoin/adapter/bin/ic-btc-adapter/main.rs | 9 +- rs/bitcoin/adapter/src/addressbook.rs | 78 ++-- rs/bitcoin/adapter/src/blockchainmanager.rs | 233 +++++------ rs/bitcoin/adapter/src/blockchainstate.rs | 366 ++++++++++-------- rs/bitcoin/adapter/src/connectionmanager.rs | 20 +- .../adapter/src/get_successors_handler.rs | 101 +++-- rs/bitcoin/adapter/src/header_cache.rs | 2 +- rs/bitcoin/adapter/src/lib.rs | 77 ++-- rs/bitcoin/adapter/src/router.rs | 17 +- rs/bitcoin/adapter/src/rpc_server.rs | 16 +- rs/bitcoin/adapter/tests/integration.rs | 67 +++- 11 files changed, 563 insertions(+), 423 deletions(-) diff --git a/rs/bitcoin/adapter/bin/ic-btc-adapter/main.rs b/rs/bitcoin/adapter/bin/ic-btc-adapter/main.rs index cae3bb341e5e..ea0231370225 100644 --- a/rs/bitcoin/adapter/bin/ic-btc-adapter/main.rs +++ b/rs/bitcoin/adapter/bin/ic-btc-adapter/main.rs @@ -44,11 +44,6 @@ pub async fn main() { start_metrics_grpc(metrics_registry.clone(), log.clone(), stream); } - start_server( - &log, - &metrics_registry, - &tokio::runtime::Handle::current(), - config, - ); - shutdown_signal(log.clone()).await; + start_server(log.clone(), metrics_registry, config).await; + shutdown_signal(log).await; } diff --git a/rs/bitcoin/adapter/src/addressbook.rs b/rs/bitcoin/adapter/src/addressbook.rs index 04317cdd3d04..098737b0d651 100644 --- a/rs/bitcoin/adapter/src/addressbook.rs +++ b/rs/bitcoin/adapter/src/addressbook.rs @@ -42,6 +42,9 @@ pub enum AddressBookError { /// This field contains the max amount of addresses that can be received. max_amount: usize, }, + /// async error thrown by tokio runtime + #[error("JoinError")] + JoinError(#[from] tokio::task::JoinError), } /// A convenience wrapper type for addressing results from the AddressBook. @@ -120,21 +123,28 @@ impl AddressBook { /// This function takes the DNS seeds and creates a new queue of socket addresses to connect to /// for the address discovery process. - fn build_seed_queue(&mut self) { + async fn build_seed_queue(&mut self) -> Result<(), tokio::task::JoinError> { let mut rng = StdRng::from_entropy(); let dns_seeds = self .dns_seeds .iter() - .map(|seed| format_addr(seed, self.port)); - let mut addresses = dns_seeds - .flat_map(|seed| { - seed.to_socket_addrs().map_or(vec![], |v| { - v.filter(|addr| !self.ipv6_only || addr.is_ipv6()).collect() + .map(|seed| format_addr(seed, self.port)) + .collect::>(); + let ipv6_only = self.ipv6_only; + let mut addresses = tokio::task::spawn_blocking(move || { + dns_seeds + .into_iter() + .flat_map(|seed| { + seed.to_socket_addrs().map_or(vec![], |v| { + v.filter(|addr| !ipv6_only || addr.is_ipv6()).collect() + }) }) - }) - .collect::>(); + .collect::>() + }) + .await?; addresses.shuffle(&mut rng); self.seed_queue = addresses.into_iter().collect(); + Ok(()) } /// This function is used when the adapter idles. It clears out the seed queue and active addresses. @@ -243,9 +253,9 @@ impl AddressBook { /// This function retrieves the next seed address from the seed queue. /// If no seeds are found, the seed queue is rebuilt from the DNS seeds. - pub fn pop_seed(&mut self) -> AddressBookResult { + pub async fn pop_seed(&mut self) -> AddressBookResult { if self.seed_queue.is_empty() { - self.build_seed_queue(); + self.build_seed_queue().await?; } let address = self @@ -366,14 +376,14 @@ mod test { /// This function tests the `AddressManager::add_many(...)` function to ensure /// addresses that are not valid are skipped while adding the valid addresses. - #[test] - fn test_address_manager_add_many() { + #[tokio::test] + async fn test_address_manager_add_many() { let config = ConfigBuilder::default_with(Network::Bitcoin) .with_dns_seeds(vec![String::from("127.0.0.1")]) .build(); let mut book = AddressBook::new(&config, no_op_logger()); - let seed = book.pop_seed().expect("there should be 1 seed"); + let seed = book.pop_seed().await.expect("there should be 1 seed"); let socket_1 = SocketAddr::from_str("127.0.0.1:8444").expect("bad address format"); let address_1 = Address::new(&socket_1, ServiceFlags::NETWORK); @@ -390,15 +400,15 @@ mod test { /// This function tests the `AddressBook::add_many(...)` function to ensure /// IPv4 addresses are skipped when in IPv6 only mode. - #[test] - fn test_address_book_add_many_ipv6_only() { + #[tokio::test] + async fn test_address_book_add_many_ipv6_only() { let config = ConfigBuilder::default_with(Network::Bitcoin) .with_dns_seeds(vec![String::from("127.0.0.1"), String::from("::1")]) .with_ipv6_only(true) .build(); let mut book = AddressBook::new(&config, no_op_logger()); - let seed = book.pop_seed().expect("there should be 1 seed"); + let seed = book.pop_seed().await.expect("there should be 1 seed"); let socket_1 = SocketAddr::from_str("127.0.0.1:8444").expect("bad address format"); let address_1 = Address::new(&socket_1, ServiceFlags::NETWORK); @@ -416,13 +426,13 @@ mod test { /// This function tests the `AddressBook::discard(...)` function to ensure /// the addresses are removed from the pool. - #[test] - fn test_discard_address() { + #[tokio::test] + async fn test_discard_address() { let config = ConfigBuilder::default_with(Network::Bitcoin) .with_dns_seeds(vec![String::from("127.0.0.1"), String::from("192.168.1.1")]) .build(); let mut book = AddressBook::new(&config, no_op_logger()); - let seed = book.pop_seed().expect("there should be 1 seed"); + let seed = book.pop_seed().await.expect("there should be 1 seed"); let socket = SocketAddr::from_str("127.0.0.1:8444").expect("bad address format"); let address = Address::new(&socket, ServiceFlags::NETWORK); @@ -467,35 +477,37 @@ mod test { /// This function ensures that the [AddressBook::pop_seed](AddressBook::pop_seed) method /// gives the next address in queue but also pushes it to the back of the queue. - #[test] - fn test_pop_seed() { + #[tokio::test] + async fn test_pop_seed() { let config = ConfigBuilder::default_with(Network::Signet) .with_dns_seeds(vec![String::from("127.0.0.1"), String::from("192.168.1.1")]) .build(); let mut book = AddressBook::new(&config, no_op_logger()); - book.pop_seed().expect("there should be 1 seed"); + book.pop_seed().await.expect("there should be 1 seed"); assert_eq!(book.seed_queue.len(), 1); - book.pop_seed().expect("there should be 1 seed"); + book.pop_seed().await.expect("there should be 1 seed"); assert_eq!(book.seed_queue.len(), 0); - book.pop_seed().expect("pop_seed should rebuild seed queue"); + book.pop_seed() + .await + .expect("pop_seed should rebuild seed queue"); assert_eq!(book.seed_queue.len(), 1); // Remove the remaining seed address and then empty the dns_seeds. - book.pop_seed().expect("there should be 1 seed"); + book.pop_seed().await.expect("there should be 1 seed"); book.dns_seeds.clear(); // `pop_seed` should now cause the AddressBookError::NoSeedAddressesFound error. assert!(matches!( - book.pop_seed(), + book.pop_seed().await, Err(AddressBookError::NoSeedAddressesFound) )); } /// This function tests to ensure that when the seed queue is built and IPv6 only is enabled, /// IPv4 seeds are filtered out. - #[test] - fn test_seeds_ipv6_only() { + #[tokio::test] + async fn test_seeds_ipv6_only() { let config = ConfigBuilder::default_with(Network::Signet) .with_dns_seeds(vec![ String::from("127.0.0.1"), @@ -505,22 +517,22 @@ mod test { .build(); let mut book = AddressBook::new(&config, no_op_logger()); assert_eq!(book.seed_queue.len(), 0); - book.build_seed_queue(); + book.build_seed_queue().await.unwrap(); assert_eq!(book.seed_queue.len(), 1); - let addr_entry = book.pop_seed().expect("there should be 1 seed"); + let addr_entry = book.pop_seed().await.expect("there should be 1 seed"); assert!(addr_entry.addr().is_ipv6()); } /// This test exercises `AddressBook::clear(...)` by checking the following: /// 1. If there are DNS seeds, the known, active addresses, and seed queue should be emptied. /// 2. If there are no DNS seeds, the seed queue and active addresses is emptied. - #[test] - fn test_clear() { + #[tokio::test] + async fn test_clear() { let config = ConfigBuilder::default_with(Network::Bitcoin) .with_dns_seeds(vec![String::from("127.0.0.1"), String::from("192.168.1.1")]) .build(); let mut book = AddressBook::new(&config, no_op_logger()); - let seed = book.pop_seed().expect("there should be 1 seed"); + let seed = book.pop_seed().await.expect("there should be 1 seed"); let socket = SocketAddr::from_str("127.0.0.1:8444").expect("bad address format"); let address = Address::new( &socket, diff --git a/rs/bitcoin/adapter/src/blockchainmanager.rs b/rs/bitcoin/adapter/src/blockchainmanager.rs index 0a7fbf29aee1..8420deb784ff 100644 --- a/rs/bitcoin/adapter/src/blockchainmanager.rs +++ b/rs/bitcoin/adapter/src/blockchainmanager.rs @@ -22,7 +22,7 @@ use ic_logger::{ReplicaLogger, debug, error, info, trace, warn}; use std::{ collections::{HashMap, HashSet}, net::SocketAddr, - sync::{Arc, Mutex}, + sync::Arc, time::{Duration, Instant}, }; use thiserror::Error; @@ -138,7 +138,7 @@ struct GetDataRequestInfo { pub struct BlockchainManager { /// This field contains the BlockchainState, which stores and manages /// all the information related to the headers and blocks. - blockchain: Arc>>, + blockchain: Arc>, /// This field stores the map of which bitcoin nodes sent which "inv" messages. peer_info: HashMap, @@ -180,7 +180,7 @@ impl BlockchainManager { /// in order to get its client so the manager can send messages to the /// BTC network. pub fn new( - blockchain: Arc>>, + blockchain: Arc>, logger: ReplicaLogger, metrics: RouterMetrics, ) -> Self { @@ -209,7 +209,7 @@ impl BlockchainManager { self.block_sync_queue.clear(); self.getdata_request_info.clear(); self.peer_info.clear(); - self.blockchain.lock().unwrap().clear_blocks(); + self.blockchain.clear_blocks(); } /// This method sends `getheaders` command to the adapter. @@ -272,17 +272,16 @@ impl BlockchainManager { let mut last_block = None; let maybe_locators = { - let blockchain_state = self.blockchain.lock().unwrap(); for inv in inventory { if let Inventory::Block(hash) = inv { peer.tip = *hash; - if blockchain_state.get_cached_header(hash).is_none() { + if self.blockchain.get_cached_header(hash).is_none() { last_block = Some(hash); } } } - last_block.map(|stop_hash| (blockchain_state.locator_hashes(), *stop_hash)) + last_block.map(|stop_hash| (self.blockchain.locator_hashes(), *stop_hash)) }; if let Some(locators) = maybe_locators { @@ -299,7 +298,7 @@ impl BlockchainManager { Ok(()) } - fn received_headers_message( + async fn received_headers_message( &mut self, channel: &mut impl Channel, addr: &SocketAddr, @@ -337,11 +336,10 @@ impl BlockchainManager { }; let maybe_locators = { - let mut blockchain_state = self.blockchain.lock().unwrap(); - let prev_tip_height = blockchain_state.get_active_chain_tip().height; - - let (block_hashes_of_added_headers, maybe_err) = blockchain_state.add_headers(headers); - let active_tip = blockchain_state.get_active_chain_tip(); + let prev_tip_height = self.blockchain.get_active_chain_tip().height; + let (block_hashes_of_added_headers, maybe_err) = + self.blockchain.add_headers(headers).await; + let active_tip = self.blockchain.get_active_chain_tip(); if prev_tip_height < active_tip.height { info!( self.logger, @@ -353,8 +351,8 @@ impl BlockchainManager { // Update the peer's tip and height to the last let maybe_last_header = match block_hashes_of_added_headers.last() { - Some(last) => blockchain_state.get_cached_header(last), - None => blockchain_state.get_cached_header(&last_block_hash), + Some(last) => self.blockchain.get_cached_header(last), + None => self.blockchain.get_cached_header(&last_block_hash), }; if let Some(last) = &maybe_last_header @@ -376,7 +374,7 @@ impl BlockchainManager { )); } Some(AddHeaderError::PrevHeaderNotCached(stop_hash)) => { - Some((blockchain_state.locator_hashes(), stop_hash)) + Some((self.blockchain.locator_hashes(), stop_hash)) } Some(AddHeaderError::Internal(_)) => { // Error writing the header cache, stop getting more headers @@ -408,7 +406,7 @@ impl BlockchainManager { } /// This function processes "block" messages received from Bitcoin nodes - fn received_block_message( + async fn received_block_message( &mut self, addr: &SocketAddr, block: &Network::Block, @@ -435,7 +433,7 @@ impl BlockchainManager { block_hash ); - match self.blockchain.lock().unwrap().add_block(block.clone()) { + match self.blockchain.add_block(block.clone()).await { Ok(()) => Ok(()), Err(err) => { warn!( @@ -459,10 +457,9 @@ impl BlockchainManager { } let (initial_hash, locator_hashes) = { - let blockchain = self.blockchain.lock().unwrap(); ( - blockchain.genesis().block_hash(), - blockchain.locator_hashes(), + self.blockchain.genesis().block_hash(), + self.blockchain.locator_hashes(), ) }; @@ -525,6 +522,7 @@ impl BlockchainManager { fn sync_blocks(&mut self, channel: &mut impl Channel) { // Timeout requests so they may be retried again. + trace!(self.logger, "in sync_blocks"); let mut retry_queue: LinkedHashSet = LinkedHashSet::new(); for (block_hash, request) in self.getdata_request_info.iter_mut() { match request.sent_at { @@ -547,10 +545,11 @@ impl BlockchainManager { // If nothing to be synced, then there is nothing to do at this point. if retry_queue.is_empty() && self.block_sync_queue.is_empty() { + trace!(self.logger, "sync_blocks returns early 1"); return; } - let is_cache_full = self.blockchain.lock().unwrap().is_block_cache_full(); + let is_cache_full = self.blockchain.is_block_cache_full(); if is_cache_full { debug!(self.logger, "Cache full"); @@ -569,6 +568,7 @@ impl BlockchainManager { let mut peers: Vec<_> = self.peer_info.values().collect(); let len = peers.len(); if len == 0 { + trace!(self.logger, "sync_blocks returns early 2"); return; } peers.rotate_left(self.round_robin_offset % len); @@ -588,11 +588,17 @@ impl BlockchainManager { Some(hash) => { selected_inventory.push(hash); } - None => break, + None => { + break; + } } } if selected_inventory.is_empty() { + trace!( + self.logger, + "sync_blocks get_next_block_hash_to_sync returns None" + ); continue; } @@ -630,7 +636,7 @@ impl BlockchainManager { /// This function is called by the adapter when a new event takes place. /// The event could be receiving "getheaders", "getdata", "inv" messages from bitcoin peers. /// The event could be change in connection status with a bitcoin peer. - pub fn process_bitcoin_network_message( + pub async fn process_bitcoin_network_message( &mut self, channel: &mut impl Channel, addr: SocketAddr, @@ -647,7 +653,7 @@ impl BlockchainManager { } } NetworkMessage::Headers(headers) => { - if let Err(err) = self.received_headers_message(channel, &addr, headers) { + if let Err(err) = self.received_headers_message(channel, &addr, headers).await { warn!( self.logger, "Received an invalid headers message form {}: {}", addr, err @@ -656,7 +662,7 @@ impl BlockchainManager { } } NetworkMessage::Block(block) => { - if let Err(err) = self.received_block_message(&addr, block) { + if let Err(err) = self.received_block_message(&addr, block).await { warn!(self.logger, "Received an invalid block {}: {}", addr, err); return Err(ProcessNetworkMessageError::InvalidMessage); } @@ -690,7 +696,7 @@ impl BlockchainManager { // request to fetch the newest information from a peer. if !self.getheaders_requests.contains_key(&addr) && self.catchup_headers.contains(&addr) { - let locators = self.blockchain.lock().unwrap().locator_hashes(); + let locators = self.blockchain.locator_hashes(); self.send_getheaders(channel, &addr, (locators, BlockHash::all_zeros())); self.catchup_headers.remove(&addr); } @@ -703,10 +709,9 @@ impl BlockchainManager { /// Add block hashes to the sync queue that are not already being synced, planned to be synced, /// or in the block cache. pub fn enqueue_new_blocks_to_download(&mut self, next_headers: Vec) { - let state = self.blockchain.lock().unwrap(); for header in next_headers { let hash = header.block_hash(); - if state.get_block(&hash).is_none() + if self.blockchain.get_block(&hash).is_none() && !self.block_sync_queue.contains(&hash) && !self.getdata_request_info.contains_key(&hash) { @@ -719,23 +724,29 @@ impl BlockchainManager { /// needed. pub fn prune_blocks(&mut self, anchor: BlockHash, processed_block_hashes: Vec) { { - let mut blockchain = self.blockchain.lock().unwrap(); - let anchor_height = blockchain + let anchor_height = self + .blockchain .get_cached_header(&anchor) .map_or(0, |c| c.data.height); let filter_height = anchor_height .checked_add(1) .expect("prune by block height: overflow occurred"); - blockchain.prune_blocks(&processed_block_hashes); - blockchain.prune_blocks_below_height(filter_height); + self.blockchain.prune_blocks(&processed_block_hashes); + self.blockchain.prune_blocks_below_height(filter_height); self.getdata_request_info.retain(|b, _| { - blockchain.get_cached_header(b).map_or(0, |c| c.data.height) >= filter_height + self.blockchain + .get_cached_header(b) + .map_or(0, |c| c.data.height) + >= filter_height }); self.block_sync_queue.retain(|b| { - blockchain.get_cached_header(b).map_or(0, |c| c.data.height) >= filter_height + self.blockchain + .get_cached_header(b) + .map_or(0, |c| c.data.height) + >= filter_height }); }; @@ -747,11 +758,7 @@ impl BlockchainManager { /// Retrieves the height of the active tip. pub fn get_height(&self) -> BlockHeight { - self.blockchain - .lock() - .unwrap() - .get_active_chain_tip() - .height + self.blockchain.get_active_chain_tip().height } } @@ -791,7 +798,7 @@ pub mod test { ( blockchain_state.genesis(), BlockchainManager::new( - Arc::new(Mutex::new(blockchain_state)), + Arc::new(blockchain_state), no_op_logger(), RouterMetrics::new(&MetricsRegistry::default()), ), @@ -800,8 +807,8 @@ pub mod test { /// Tests `BlockchainManager::send_getheaders(...)` to ensure the manager's outgoing command /// queue - #[test] - fn test_manager_can_send_getheaders_messages() { + #[tokio::test] + async fn test_manager_can_send_getheaders_messages() { let addr = SocketAddr::from_str("127.0.0.1:8333").expect("bad address format"); let mut channel = TestChannel::new(vec![addr]); let (genesis, mut blockchain_manager) = create_blockchain_manager(Network::Bitcoin); @@ -850,8 +857,8 @@ pub mod test { /// to the peer. When first starting, the adapter should send only the genesis hash. After headers /// are received, the locator hashes sent should follow the algorithm defined in /// BlockchainState::locator_hashes. - #[test] - fn test_init_sync() { + #[tokio::test] + async fn test_init_sync() { let addr1 = SocketAddr::from_str("127.0.0.1:8333").expect("bad address format"); let addr2 = SocketAddr::from_str("127.0.0.1:8444").expect("bad address format"); let sockets = vec![addr1, addr2]; @@ -908,6 +915,7 @@ pub mod test { assert!( blockchain_manager .process_bitcoin_network_message(&mut channel, addr1, &message) + .await .is_ok() ); @@ -943,11 +951,11 @@ pub mod test { ); } - #[test] + #[tokio::test] /// This unit test verifies if the incoming inv messages are processed correctly. /// This test first creates a BlockChainManager, adds a peer, and let the initial sync happen. /// The test then sends an inv message for a fork chain, and verifies if the BlockChainManager responds correctly. - fn test_received_inv() { + async fn test_received_inv() { let sockets = vec![SocketAddr::from_str("127.0.0.1:8333").expect("bad address format")]; let mut channel = TestChannel::new(sockets.clone()); let (genesis, mut blockchain_manager) = create_blockchain_manager(Network::Regtest); @@ -968,16 +976,12 @@ pub mod test { sockets[0], &NetworkMessage::Headers(chain.clone()) ) + .await .is_ok() ); assert_eq!( - blockchain_manager - .blockchain - .lock() - .unwrap() - .get_active_chain_tip() - .height, + blockchain_manager.blockchain.get_active_chain_tip().height, 16, "Height of the blockchain is not matching after adding the headers" ); @@ -998,6 +1002,7 @@ pub mod test { assert!( blockchain_manager .process_bitcoin_network_message(&mut channel, sockets[0], &message) + .await .is_ok() ); if let Some(command) = channel.pop_front() { @@ -1037,8 +1042,8 @@ pub mod test { /// This test performs a surface level check to make ensure the `sync_blocks` and `received_block_message` /// adds to and removes from `BlockchainManager.getdata_request_info` correctly. - #[test] - fn test_simple_sync_blocks_and_received_block_message_lifecycle() { + #[tokio::test] + async fn test_simple_sync_blocks_and_received_block_message_lifecycle() { let peer_addr = SocketAddr::from_str("127.0.0.1:8333").expect("bad address format"); let sockets = vec![peer_addr]; let mut channel = TestChannel::new(sockets.clone()); @@ -1055,11 +1060,8 @@ pub mod test { let headers = vec![block_1.header, block_2.header]; // Initialize the blockchain manager state { - let (added_headers, maybe_err) = blockchain_manager - .blockchain - .lock() - .unwrap() - .add_headers(&headers); + let (added_headers, maybe_err) = + blockchain_manager.blockchain.add_headers(&headers).await; assert_eq!(added_headers.len(), headers.len()); assert!(maybe_err.is_none()); blockchain_manager @@ -1093,7 +1095,9 @@ pub mod test { } // Ensure there is now 1 request. - let result = blockchain_manager.received_block_message(&peer_addr, &block_1); + let result = blockchain_manager + .received_block_message(&peer_addr, &block_1) + .await; assert!(result.is_ok()); { let available_requests_for_peer = blockchain_manager @@ -1104,7 +1108,9 @@ pub mod test { assert_eq!(available_requests_for_peer, 1); } - let result = blockchain_manager.received_block_message(&peer_addr, &block_2); + let result = blockchain_manager + .received_block_message(&peer_addr, &block_2) + .await; assert!(result.is_ok()); blockchain_manager.sync_blocks(&mut channel); // Ensure there is now zero requests. @@ -1120,8 +1126,8 @@ pub mod test { /// This function tests to ensure that the BlockchainManager does not send out `getdata` /// requests when the block cache has reached the size threshold. - #[test] - fn test_sync_blocks_size_limit() { + #[tokio::test] + async fn test_sync_blocks_size_limit() { let addr = SocketAddr::from_str("127.0.0.1:8333").expect("bad address format"); let sockets = vec![addr]; let mut channel = TestChannel::new(sockets.clone()); @@ -1135,13 +1141,13 @@ pub mod test { { blockchain_manager.add_peer(&mut channel, &addr); - let mut blockchain = blockchain_manager.blockchain.lock().unwrap(); - let (added_headers, _) = blockchain.add_headers(&headers); + let blockchain = &blockchain_manager.blockchain; + let (added_headers, _) = blockchain.add_headers(&headers).await; assert_eq!(added_headers.len(), 5); // Add the 5 large blocks. for block in large_blocks { - blockchain.add_block(block).unwrap(); + blockchain.add_block(block).await.unwrap(); } }; @@ -1156,8 +1162,8 @@ pub mod test { /// This function tests to ensure that the BlockchainManager retries timed out `getdata` requests /// when calling `sync_blocks`. - #[test] - fn test_ensure_sync_blocks_retries_timed_out_getdata_requests() { + #[tokio::test] + async fn test_ensure_sync_blocks_retries_timed_out_getdata_requests() { let addr = SocketAddr::from_str("127.0.0.1:8333").expect("bad address format"); let sockets = vec![addr]; let mut channel = TestChannel::new(sockets.clone()); @@ -1205,8 +1211,8 @@ pub mod test { /// This function tests to ensure that the BlockchainManager retries `getdata` requests /// that were sent to peers that have disconnected when calling `sync_blocks`. - #[test] - fn test_manager_retries_getdata_requests_where_the_peer_has_disconnected() { + #[tokio::test] + async fn test_manager_retries_getdata_requests_where_the_peer_has_disconnected() { let addr = SocketAddr::from_str("127.0.0.1:8333").expect("bad address format"); let addr2 = SocketAddr::from_str("127.0.0.1:3338").expect("bad address format"); let sockets = vec![addr]; @@ -1257,8 +1263,8 @@ pub mod test { ); } - #[test] - fn test_ensure_getdata_requests_are_not_retried_with_a_full_cache() { + #[tokio::test] + async fn test_ensure_getdata_requests_are_not_retried_with_a_full_cache() { let addr = SocketAddr::from_str("127.0.0.1:8333").expect("bad address format"); let sockets = vec![addr]; let mut channel = TestChannel::new(sockets.clone()); @@ -1276,13 +1282,17 @@ pub mod test { large_blockchain.drain(..1); { - let mut blockchain = blockchain_manager.blockchain.lock().unwrap(); - let (added_headers, maybe_err) = blockchain.add_headers(&large_blockchain_headers); + let blockchain = &blockchain_manager.blockchain; + let (added_headers, maybe_err) = + blockchain.add_headers(&large_blockchain_headers).await; assert_eq!(added_headers.len(), large_blockchain_headers.len()); assert!(maybe_err.is_none()); for block in large_blockchain { - blockchain.add_block(block).expect("failed to add block"); + blockchain + .add_block(block) + .await + .expect("failed to add block"); } assert!(blockchain.is_block_cache_full()); @@ -1318,8 +1328,8 @@ pub mod test { /// Tests the `BlockchainManager::idle(...)` function to ensure it clears the state from the /// BlockchainManager. - #[test] - fn test_make_idle() { + #[tokio::test] + async fn test_make_idle() { let peer_addr = SocketAddr::from_str("127.0.0.1:8333").expect("bad address format"); let sockets = vec![peer_addr]; let mut channel = TestChannel::new(sockets.clone()); @@ -1338,12 +1348,12 @@ pub mod test { // Initialize the blockchain manager state { blockchain_manager.add_peer(&mut channel, &peer_addr); - let mut blockchain = blockchain_manager.blockchain.lock().unwrap(); - let (added_headers, maybe_err) = blockchain.add_headers(&headers); + let blockchain = &blockchain_manager.blockchain; + let (added_headers, maybe_err) = blockchain.add_headers(&headers).await; assert_eq!(added_headers.len(), headers.len()); assert!(maybe_err.is_none()); - blockchain.add_block(block_2).expect("invalid block"); + blockchain.add_block(block_2).await.expect("invalid block"); }; blockchain_manager .block_sync_queue @@ -1353,8 +1363,6 @@ pub mod test { assert!( blockchain_manager .blockchain - .lock() - .unwrap() .get_block(&block_2_hash) .is_some() ); @@ -1366,16 +1374,14 @@ pub mod test { assert!( blockchain_manager .blockchain - .lock() - .unwrap() .get_block(&block_2_hash) .is_none() ); assert_eq!(blockchain_manager.peer_info.len(), 0); } - #[test] - fn test_enqueue_new_blocks_to_download() { + #[tokio::test] + async fn test_enqueue_new_blocks_to_download() { let (genesis, mut blockchain_manager) = create_blockchain_manager(Network::Regtest); let next_headers = generate_headers(genesis.block_hash(), genesis.time, 5, &[]); @@ -1398,8 +1404,8 @@ pub mod test { ); } - #[test] - fn test_enqueue_new_blocks_to_download_no_duplicates() { + #[tokio::test] + async fn test_enqueue_new_blocks_to_download_no_duplicates() { let (genesis, mut blockchain_manager) = create_blockchain_manager(Network::Regtest); let next_headers = generate_headers(genesis.block_hash(), genesis.time, 5, &[]); @@ -1424,10 +1430,13 @@ pub mod test { txdata: vec![], }; { - let mut blockchain = blockchain_manager.blockchain.lock().unwrap(); - let (headers_added, maybe_err) = blockchain.add_headers(&next_headers); - assert_eq!(headers_added.len(), next_headers.len(), "{maybe_err:#?}"); - blockchain.add_block(block).expect("unable to add block"); + let blockchain = &blockchain_manager.blockchain; + let (headers_added, maybe_err) = blockchain.add_headers(&next_headers).await; + assert_eq!(headers_added.len(), next_headers.len(), "{:#?}", maybe_err); + blockchain + .add_block(block) + .await + .expect("unable to add block"); } blockchain_manager.enqueue_new_blocks_to_download(next_headers); assert_eq!(blockchain_manager.block_sync_queue.len(), 3); @@ -1443,8 +1452,8 @@ pub mod test { ); } - #[test] - fn test_pruning_blocks_based_on_the_anchor_hash_and_processed_hashes() { + #[tokio::test] + async fn test_pruning_blocks_based_on_the_anchor_hash_and_processed_hashes() { let (genesis, mut blockchain_manager) = create_blockchain_manager(Network::Regtest); let addr = SocketAddr::from_str("127.0.0.1:8333").expect("invalid address"); let mut channel = TestChannel::new(vec![addr]); @@ -1461,10 +1470,13 @@ pub mod test { }; { - let mut blockchain = blockchain_manager.blockchain.lock().unwrap(); - let (headers_added, maybe_err) = blockchain.add_headers(&next_headers); - assert_eq!(headers_added.len(), next_headers.len(), "{maybe_err:#?}"); - blockchain.add_block(block_3).expect("unable to add block"); + let blockchain = &blockchain_manager.blockchain; + let (headers_added, maybe_err) = blockchain.add_headers(&next_headers).await; + assert_eq!(headers_added.len(), next_headers.len(), "{:#?}", maybe_err); + blockchain + .add_block(block_3) + .await + .expect("unable to add block"); } blockchain_manager.add_peer(&mut channel, &addr); @@ -1494,15 +1506,13 @@ pub mod test { assert!( blockchain_manager .blockchain - .lock() - .unwrap() .get_block(&next_hashes[2]) .is_none() ); } - #[test] - fn test_pruning_blocks_to_ensure_it_does_not_prune_anchor_adjacent_blocks() { + #[tokio::test] + async fn test_pruning_blocks_to_ensure_it_does_not_prune_anchor_adjacent_blocks() { let (genesis, mut blockchain_manager) = create_blockchain_manager(Network::Regtest); let genesis_hash = genesis.block_hash(); let addr = SocketAddr::from_str("127.0.0.1:8333").expect("invalid address"); @@ -1520,10 +1530,13 @@ pub mod test { }; { - let mut blockchain = blockchain_manager.blockchain.lock().unwrap(); - let (headers_added, maybe_err) = blockchain.add_headers(&next_headers); - assert_eq!(headers_added.len(), next_headers.len(), "{maybe_err:#?}"); - blockchain.add_block(block_5).expect("unable to add block"); + let blockchain = &blockchain_manager.blockchain; + let (headers_added, maybe_err) = blockchain.add_headers(&next_headers).await; + assert_eq!(headers_added.len(), next_headers.len(), "{:#?}", maybe_err); + blockchain + .add_block(block_5) + .await + .expect("unable to add block"); } blockchain_manager.add_peer(&mut channel, &addr); @@ -1555,8 +1568,6 @@ pub mod test { assert!( blockchain_manager .blockchain - .lock() - .unwrap() .get_block(&next_hashes[4]) .is_some() ); @@ -1621,8 +1632,8 @@ pub mod test { /// Tests that the `handle_getheaders_timeouts(...)` method removes timed out `getheaders` requests /// and triggers the discard of the connection. - #[test] - fn test_handle_getheaders_timeouts() { + #[tokio::test] + async fn test_handle_getheaders_timeouts() { let addr = SocketAddr::from_str("127.0.0.1:8333").expect("bad address format"); let addr2 = SocketAddr::from_str("127.0.0.1:8444").expect("bad address format"); let mut channel = TestChannel::new(vec![addr, addr2]); diff --git a/rs/bitcoin/adapter/src/blockchainstate.rs b/rs/bitcoin/adapter/src/blockchainstate.rs index db44c0be49f1..eef389a67427 100644 --- a/rs/bitcoin/adapter/src/blockchainstate.rs +++ b/rs/bitcoin/adapter/src/blockchainstate.rs @@ -12,7 +12,11 @@ use bitcoin::{BlockHash, block::Header as PureHeader, consensus::Encodable}; use ic_btc_validation::HeaderStore; use ic_logger::ReplicaLogger; use ic_metrics::MetricsRegistry; -use std::{collections::HashMap, path::PathBuf, sync::Arc}; +use std::{ + collections::HashMap, + path::PathBuf, + sync::{Arc, RwLock}, +}; use thiserror::Error; /// The limit at which we should stop making additional requests for new blocks as the block cache @@ -40,11 +44,11 @@ pub type SerializedBlock = Vec; /// The BlockChainState caches all the Bitcoin headers, some of the Bitcoin blocks. /// The BlockChainState also maintains the child relationhips between the headers. pub struct BlockchainState { - /// This field stores all the Bitcoin headers using a HashMap containing BlockHash and the corresponding header. - header_cache: Box + Send>, + /// This field stores all the Bitcoin headers using a HashMap containining BlockHash and the corresponding header. + header_cache: Arc + Send>, /// This field stores a hashmap containing BlockHash and the corresponding SerializedBlock. - block_cache: HashMap>, + block_cache: RwLock>>, /// Used to determine how validation should be handled with `validate_header`. network: Network, @@ -58,8 +62,8 @@ where /// Create a new BlockChainState object with in-memory cache. pub fn new(network: Network, metrics_registry: &MetricsRegistry) -> Self { let genesis_block_header = network.genesis_block_header(); - let header_cache = Box::new(InMemoryHeaderCache::new(genesis_block_header)); - let block_cache = HashMap::new(); + let header_cache = Arc::new(InMemoryHeaderCache::new(genesis_block_header)); + let block_cache = RwLock::new(HashMap::new()); BlockchainState { header_cache, block_cache, @@ -76,12 +80,12 @@ where logger: ReplicaLogger, ) -> Self { let genesis_block_header = network.genesis_block_header(); - let header_cache = Box::new(LMDBHeaderCache::new( + let header_cache = Arc::new(LMDBHeaderCache::new( genesis_block_header, cache_dir, logger, )); - let block_cache = HashMap::new(); + let block_cache = RwLock::new(HashMap::new()); BlockchainState { header_cache, block_cache, @@ -102,7 +106,7 @@ where /// Returns the hashes of all cached blocks. pub(crate) fn get_cached_blocks(&self) -> Vec { - self.block_cache.keys().copied().collect() + self.block_cache.read().unwrap().keys().copied().collect() } /// Processes the `headers` message received from Bitcoin nodes by adding them to the state. @@ -110,23 +114,25 @@ where /// with a [AddHeaderError::PrevHeaderNotCached](AddHeaderError::PrevHeaderNotCached) error. /// If the header has been added to the cache, it will be returned in a vector alongside /// a possible error that may have occurred while adding the headers. - pub fn add_headers( - &mut self, + pub async fn add_headers( + &self, headers: &[Network::Header], ) -> (Vec, Option) { let mut block_hashes_of_added_headers = vec![]; + let mut err = None; - let err = headers - .iter() - .try_for_each(|header| match self.add_header(header.clone()) { + for header in headers.iter() { + match self.add_header(header.clone()).await { Ok(AddHeaderResult::HeaderAdded(block_hash)) => { block_hashes_of_added_headers.push(block_hash); - Ok(()) } - Ok(AddHeaderResult::HeaderAlreadyExists) => Ok(()), - Err(err) => Err(err), - }) - .err(); + Ok(AddHeaderResult::HeaderAlreadyExists) => (), + Err(error) => { + err = Some(error); + break; + } + } + } let num_tips = self.header_cache.get_num_tips(); self.metrics.tips.set(num_tips as i64); @@ -138,7 +144,7 @@ where } /// This method adds the input header to the `header_cache`. - fn add_header(&mut self, header: Network::Header) -> Result { + async fn add_header(&self, header: Network::Header) -> Result { let block_hash = header.block_hash(); // If the header already exists in the cache, @@ -151,15 +157,19 @@ where .validate_header(self, &header) .map_err(|err| AddHeaderError::InvalidHeader(block_hash, err))?; - self.header_cache - .add_header(block_hash, header) - .inspect(|_| { - self.metrics.header_cache_size.inc(); + let header_cache = self.header_cache.clone(); + let metrics = self.metrics.clone(); + tokio::task::spawn_blocking(move || { + header_cache.add_header(block_hash, header).inspect(|_| { + metrics.header_cache_size.inc(); }) + }) + .await + .map_err(|err| AddHeaderError::Internal(format!("{}", err)))? } /// This method adds a new block to the `block_cache` - pub fn add_block(&mut self, block: Network::Block) -> Result<(), AddBlockError> { + pub async fn add_block(&self, block: Network::Block) -> Result<(), AddBlockError> { let block_hash = block.block_hash(); if block.compute_merkle_root().is_some() && !block.check_merkle_root() { @@ -169,6 +179,7 @@ where // If the block's header is not added before, then add the header into the `header_cache` first. let _ = self .add_header(block.header().clone()) + .await .map_err(AddBlockError::Header)?; let mut serialized_block = vec![]; @@ -177,6 +188,8 @@ where .map_err(|e| AddBlockError::CouldNotSerialize(block_hash, e.to_string()))?; self.block_cache + .write() + .unwrap() .insert(block_hash, Arc::new(serialized_block)); self.metrics @@ -184,7 +197,7 @@ where .set(self.get_block_cache_size() as i64); self.metrics .block_cache_elements - .set(self.block_cache.len() as i64); + .set(self.block_cache.read().unwrap().len() as i64); Ok(()) } @@ -195,16 +208,18 @@ where /// This method is used to remove blocks in the `header_cache` that are found in the given /// block hashes. - pub fn prune_blocks(&mut self, block_hashes: &[BlockHash]) { + pub fn prune_blocks(&self, block_hashes: &[BlockHash]) { for block_hash in block_hashes { - self.block_cache.remove(block_hash); + self.block_cache.write().unwrap().remove(block_hash); } } /// Removes blocks that are below a given height from the block cache. - pub fn prune_blocks_below_height(&mut self, height: BlockHeight) { + pub fn prune_blocks_below_height(&self, height: BlockHeight) { let hashes_below_height = self .block_cache + .write() + .unwrap() .keys() .filter(|b| height > self.get_cached_header(b).map_or(0, |c| c.data.height)) .copied() @@ -254,12 +269,12 @@ where /// This method takes a block hash /// If the corresponding block is stored in the `block_cache`, the cached block is returned. pub fn get_block(&self, block_hash: &BlockHash) -> Option> { - self.block_cache.get(block_hash).cloned() + self.block_cache.read().unwrap().get(block_hash).cloned() } /// Used when the adapter is shutdown and no longer requires holding on to blocks. - pub fn clear_blocks(&mut self) { - self.block_cache = HashMap::new(); + pub fn clear_blocks(&self) { + *self.block_cache.write().unwrap() = HashMap::new(); } pub(crate) fn is_block_cache_full(&self) -> bool { @@ -268,7 +283,12 @@ where /// Returns the current size of the block cache. pub fn get_block_cache_size(&self) -> usize { - self.block_cache.values().map(|block| block.len()).sum() + self.block_cache + .read() + .unwrap() + .values() + .map(|block| block.len()) + .sum() } } @@ -303,11 +323,14 @@ mod test { use ic_btc_validation::ValidateHeaderError; use std::collections::HashSet; - fn run_in_memory(network: Network, test_fn: impl Fn(BlockchainState)) { + fn run_in_memory(network: Network, test_fn: impl Fn(BlockchainState) -> R) -> R { test_fn(BlockchainState::new(network, &MetricsRegistry::default())) } - fn run_with_cache_dir(network: Network, test_fn: impl Fn(BlockchainState)) { + fn run_with_cache_dir( + network: Network, + test_fn: impl Fn(BlockchainState) -> R, + ) -> R { let dir = tempdir().unwrap(); test_fn(BlockchainState::new_with_cache_dir( network, @@ -317,11 +340,12 @@ mod test { )) } - fn test_get_block(mut state: BlockchainState) { + async fn test_get_block(state: BlockchainState) { let test_state = TestState::setup(); state .add_block(test_state.block_1.clone()) + .await .expect("should be able to add block 1"); let block_1_hash = test_state.block_1.block_hash(); let block_2_hash = test_state.block_2.block_hash(); @@ -341,25 +365,25 @@ mod test { assert_eq!(block.block_hash(), block_1_hash); } - #[test] - fn test_get_block_in_memory() { - run_in_memory(Network::Bitcoin, test_get_block) + #[tokio::test] + async fn test_get_block_in_memory() { + run_in_memory(Network::Bitcoin, test_get_block).await } - #[test] - fn test_get_block_on_disk() { - run_with_cache_dir(Network::Bitcoin, test_get_block) + #[tokio::test] + async fn test_get_block_on_disk() { + run_with_cache_dir(Network::Bitcoin, test_get_block).await } /// Tests whether or not the `BlockchainState::add_headers(...)` function can add headers to the cache /// successfully. - fn test_adding_headers_successfully(mut state: BlockchainState) { + async fn test_adding_headers_successfully(state: BlockchainState) { let initial_header = state.genesis(); let chain = generate_headers(initial_header.block_hash(), initial_header.time, 16, &[]); let chain_hashes: Vec = chain.iter().map(|header| header.block_hash()).collect(); let last_hash = *chain_hashes.last().unwrap(); - let (added_headers, maybe_err) = state.add_headers(&chain); + let (added_headers, maybe_err) = state.add_headers(&chain).await; assert!(maybe_err.is_none()); let last_block_hashes = added_headers.last().unwrap(); @@ -369,25 +393,25 @@ mod test { assert_eq!(tip.header.block_hash(), last_hash); } - #[test] - fn test_adding_headers_successfully_in_memory() { - run_in_memory(Network::Regtest, test_adding_headers_successfully) + #[tokio::test] + async fn test_adding_headers_successfully_in_memory() { + run_in_memory(Network::Regtest, test_adding_headers_successfully).await } - #[test] - fn test_adding_headers_successfully_on_disk() { - run_with_cache_dir(Network::Regtest, test_adding_headers_successfully) + #[tokio::test] + async fn test_adding_headers_successfully_on_disk() { + run_with_cache_dir(Network::Regtest, test_adding_headers_successfully).await } /// Tests whether or not the `BlockchainState::add_headers(...)` function can add 2500 mainnet headers to the cache /// successfully. After ~2000 headers there is difficulty adjustment so 2500 headers make sure that we test /// at least one header validation with difficulty adjustment. /// This is a regression test for incident at btc height 799_498. - fn test_adding_mainnet_headers_successfully(mut state: BlockchainState) { + async fn test_adding_mainnet_headers_successfully(state: BlockchainState) { let headers_json = include_str!("../test_data/first_2500_mainnet_headers.json"); let headers: Vec<_> = serde_json::from_str(headers_json).unwrap(); - let (added_headers, maybe_err) = state.add_headers(&headers); + let (added_headers, maybe_err) = state.add_headers(&headers).await; assert!( maybe_err.is_none(), "Error when adding valid mainnet headers." @@ -399,24 +423,24 @@ mod test { assert_eq!(tip.height, 2499); } - #[test] - fn test_adding_mainnet_headers_successfully_in_memory() { - run_in_memory(Network::Bitcoin, test_adding_mainnet_headers_successfully) + #[tokio::test] + async fn test_adding_mainnet_headers_successfully_in_memory() { + run_in_memory(Network::Bitcoin, test_adding_mainnet_headers_successfully).await } - #[test] - fn test_adding_mainnet_headers_successfully_on_disk() { - run_with_cache_dir(Network::Bitcoin, test_adding_mainnet_headers_successfully) + #[tokio::test] + async fn test_adding_mainnet_headers_successfully_on_disk() { + run_with_cache_dir(Network::Bitcoin, test_adding_mainnet_headers_successfully).await } /// Tests whether or not the `BlockchainState::add_headers(...)` function can add 2500 testnet headers to the cache /// successfully. After ~2000 headers there is difficulty adjustment so 2500 headers make sure that we test /// at least one header validation with difficulty adjustment. - fn test_adding_testnet_headers_successfully(mut state: BlockchainState) { + async fn test_adding_testnet_headers_successfully(state: BlockchainState) { let headers_json = include_str!("../test_data/first_2500_testnet_headers.json"); let headers: Vec<_> = serde_json::from_str(headers_json).unwrap(); - let (added_headers, maybe_err) = state.add_headers(&headers); + let (added_headers, maybe_err) = state.add_headers(&headers).await; assert!( maybe_err.is_none(), "Error when adding valid testnet headers." @@ -428,19 +452,19 @@ mod test { assert_eq!(tip.height, 2499); } - #[test] - fn test_adding_testnet_headers_successfully_in_memory() { - run_in_memory(Network::Testnet, test_adding_testnet_headers_successfully) + #[tokio::test] + async fn test_adding_testnet_headers_successfully_in_memory() { + run_in_memory(Network::Testnet, test_adding_testnet_headers_successfully).await } - #[test] - fn test_adding_testnet_headers_successfully_on_disk() { - run_with_cache_dir(Network::Testnet, test_adding_testnet_headers_successfully) + #[tokio::test] + async fn test_adding_testnet_headers_successfully_on_disk() { + run_with_cache_dir(Network::Testnet, test_adding_testnet_headers_successfully).await } /// Tests whether or not the `BlockchainState::add_headers(...)` function can add headers that /// cause 2 forks in the chain. The state should be able to determine what is the active tip. - fn test_forks_when_adding_headers(mut state: BlockchainState) { + async fn test_forks_when_adding_headers(state: BlockchainState) { let initial_header = state.genesis(); // Create an arbitrary chain and adding to the BlockchainState @@ -448,7 +472,7 @@ mod test { let chain_hashes: Vec = chain.iter().map(|header| header.block_hash()).collect(); let last_chain_hash = chain_hashes.last().expect("missing last hash"); - let (_, maybe_err) = state.add_headers(&chain); + let (_, maybe_err) = state.add_headers(&chain).await; assert!( maybe_err.is_none(), "unsuccessfully added first chain: {maybe_err:?}" @@ -462,7 +486,7 @@ mod test { .collect(); let last_fork_hash = fork_hashes.last().expect("missing last hash"); - let (_, maybe_err) = state.add_headers(&fork_chain); + let (_, maybe_err) = state.add_headers(&fork_chain).await; assert!( maybe_err.is_none(), "unsuccessfully added fork chain: {maybe_err:?}" @@ -476,67 +500,67 @@ mod test { assert_eq!(state.get_active_chain_tip().height, 27); } - #[test] - fn test_forks_when_adding_headers_in_memory() { - run_in_memory(Network::Regtest, test_forks_when_adding_headers) + #[tokio::test] + async fn test_forks_when_adding_headers_in_memory() { + run_in_memory(Network::Regtest, test_forks_when_adding_headers).await } - #[test] - fn test_forks_when_adding_headers_on_disk() { - run_with_cache_dir(Network::Regtest, test_forks_when_adding_headers) + #[tokio::test] + async fn test_forks_when_adding_headers_on_disk() { + run_with_cache_dir(Network::Regtest, test_forks_when_adding_headers).await } /// Tests `BlockchainState::add_headers(...)` with an empty set of headers. - fn test_adding_an_empty_headers_vector(mut state: BlockchainState) { + async fn test_adding_an_empty_headers_vector(state: BlockchainState) { let chain = vec![]; - let (added_headers, maybe_err) = state.add_headers(&chain); + let (added_headers, maybe_err) = state.add_headers(&chain).await; assert!(maybe_err.is_none()); assert!(added_headers.is_empty()); assert_eq!(state.get_active_chain_tip().height, 0); } - #[test] - fn test_adding_an_empty_headers_vector_in_memory() { - run_in_memory(Network::Bitcoin, test_adding_an_empty_headers_vector) + #[tokio::test] + async fn test_adding_an_empty_headers_vector_in_memory() { + run_in_memory(Network::Bitcoin, test_adding_an_empty_headers_vector).await } - #[test] - fn test_adding_an_empty_headers_vector_on_disk() { - run_with_cache_dir(Network::Bitcoin, test_adding_an_empty_headers_vector) + #[tokio::test] + async fn test_adding_an_empty_headers_vector_on_disk() { + run_with_cache_dir(Network::Bitcoin, test_adding_an_empty_headers_vector).await } /// Tests whether or not the `BlockchainState::add_headers(...)` function can handle adding already known /// headers in the state. - fn test_adding_headers_that_already_exist(mut state: BlockchainState) { + async fn test_adding_headers_that_already_exist(state: BlockchainState) { let initial_header = state.genesis(); let chain = generate_headers(initial_header.block_hash(), initial_header.time, 16, &[]); let chain_hashes: Vec = chain.iter().map(|header| header.block_hash()).collect(); let last_hash = *chain_hashes.last().unwrap(); - let (last_block_hashes, maybe_err) = state.add_headers(&chain); + let (last_block_hashes, maybe_err) = state.add_headers(&chain).await; assert!(maybe_err.is_none()); assert_eq!(last_block_hashes.len(), 16); let last_block_hash = last_block_hashes.last().unwrap(); assert_eq!(*last_block_hash, last_hash); - let (added_headers, maybe_err) = state.add_headers(&chain); + let (added_headers, maybe_err) = state.add_headers(&chain).await; assert!(maybe_err.is_none()); assert!(added_headers.is_empty()); } - #[test] - fn test_adding_headers_that_already_exist_in_memory() { - run_in_memory(Network::Regtest, test_adding_headers_that_already_exist) + #[tokio::test] + async fn test_adding_headers_that_already_exist_in_memory() { + run_in_memory(Network::Regtest, test_adding_headers_that_already_exist).await } - #[test] - fn test_adding_headers_that_already_exist_on_disk() { - run_with_cache_dir(Network::Regtest, test_adding_headers_that_already_exist) + #[tokio::test] + async fn test_adding_headers_that_already_exist_on_disk() { + run_with_cache_dir(Network::Regtest, test_adding_headers_that_already_exist).await } /// Tests whether or not the `BlockchainState::add_headers(...)` function can add headers while avoiding /// adding a header that is invalid. - fn test_adding_headers_with_an_invalid_header(mut state: BlockchainState) { + async fn test_adding_headers_with_an_invalid_header(state: BlockchainState) { let initial_header = state.genesis(); let mut chain = generate_headers(initial_header.block_hash(), initial_header.time, 16, &[]); let last_header = chain.get_mut(10).unwrap(); @@ -545,7 +569,7 @@ mod test { let chain_hashes: Vec = chain.iter().map(|header| header.block_hash()).collect(); let last_hash = chain_hashes[10]; - let (added_headers, maybe_err) = state.add_headers(&chain); + let (added_headers, maybe_err) = state.add_headers(&chain).await; assert_eq!(added_headers.len(), 10); assert!( @@ -556,30 +580,30 @@ mod test { assert_eq!(tip.height, 10); } - #[test] - fn test_adding_headers_with_an_invalid_header_in_memory() { - run_in_memory(Network::Regtest, test_adding_headers_with_an_invalid_header) + #[tokio::test] + async fn test_adding_headers_with_an_invalid_header_in_memory() { + run_in_memory(Network::Regtest, test_adding_headers_with_an_invalid_header).await } - #[test] - fn test_adding_headers_with_an_invalid_header_on_disk() { - run_with_cache_dir(Network::Regtest, test_adding_headers_with_an_invalid_header) + #[tokio::test] + async fn test_adding_headers_with_an_invalid_header_on_disk() { + run_with_cache_dir(Network::Regtest, test_adding_headers_with_an_invalid_header).await } /// Tests the functionality of `BlockchainState::add_block(...)` to push it through the add_header /// validation and adding the block to the cache. - fn test_adding_blocks_to_the_cache(mut state: BlockchainState) { + async fn test_adding_blocks_to_the_cache(state: BlockchainState) { let block_1 = block_1(); let mut block_2 = block_2(); // Attempt to add block 2 to the cache before block 1's header has been added. let block_2_hash = block_2.header.block_hash(); - let result = state.add_block(block_2.clone()); + let result = state.add_block(block_2.clone()).await; assert!( matches!(result, Err(AddBlockError::Header(AddHeaderError::InvalidHeader(stop_hash, err))) if stop_hash == block_2_hash && matches!(err, ValidateHeaderError::PrevHeaderNotFound)), ); - let result = state.add_block(block_1); + let result = state.add_block(block_1).await; assert!(matches!(result, Ok(()))); // Make a block 2's merkle root invalid and try to add the block to the cache. @@ -587,88 +611,114 @@ mod test { TxMerkleNode::from_raw_hash(bitcoin::hashes::Hash::all_zeros()); // Block 2's hash will now be changed because of the merkle root change. let block_2_hash = block_2.block_hash(); - let result = state.add_block(block_2); + let result = state.add_block(block_2).await; assert!( matches!(result, Err(AddBlockError::InvalidMerkleRoot(stop_hash)) if stop_hash == block_2_hash), ); } - #[test] - fn test_adding_blocks_to_the_cache_in_memory() { - run_in_memory(Network::Bitcoin, test_adding_blocks_to_the_cache) + #[tokio::test] + async fn test_adding_blocks_to_the_cache_in_memory() { + run_in_memory(Network::Bitcoin, test_adding_blocks_to_the_cache).await } - #[test] - fn test_adding_blocks_to_the_cache_on_disk() { - run_with_cache_dir(Network::Bitcoin, test_adding_blocks_to_the_cache) + #[tokio::test] + async fn test_adding_blocks_to_the_cache_on_disk() { + run_with_cache_dir(Network::Bitcoin, test_adding_blocks_to_the_cache).await } /// Tests the functionality of `BlockchainState::prune_blocks(...)` to ensure /// blocks are removed from the cache. - fn test_pruning_blocks_from_the_cache(mut state: BlockchainState) { + async fn test_pruning_blocks_from_the_cache(state: BlockchainState) { let test_state = TestState::setup(); let block_1_hash = test_state.block_1.block_hash(); let block_2_hash = test_state.block_2.block_hash(); - state.add_block(test_state.block_1).unwrap(); - state.add_block(test_state.block_2).unwrap(); + state.add_block(test_state.block_1).await.unwrap(); + state.add_block(test_state.block_2).await.unwrap(); state.prune_blocks(&[block_2_hash]); - assert!(state.block_cache.contains_key(&block_1_hash)); - assert!(!state.block_cache.contains_key(&block_2_hash)); + assert!( + state + .block_cache + .read() + .unwrap() + .contains_key(&block_1_hash) + ); + assert!( + !state + .block_cache + .read() + .unwrap() + .contains_key(&block_2_hash) + ); } - #[test] - fn test_pruning_blocks_from_the_cache_in_memory() { - run_in_memory(Network::Bitcoin, test_pruning_blocks_from_the_cache) + #[tokio::test] + async fn test_pruning_blocks_from_the_cache_in_memory() { + run_in_memory(Network::Bitcoin, test_pruning_blocks_from_the_cache).await } - #[test] - fn test_pruning_blocks_from_the_cache_on_disk() { - run_with_cache_dir(Network::Bitcoin, test_pruning_blocks_from_the_cache) + #[tokio::test] + async fn test_pruning_blocks_from_the_cache_on_disk() { + run_with_cache_dir(Network::Bitcoin, test_pruning_blocks_from_the_cache).await } /// Tests the functionality of `BlockchainState::prune_blocks_below_height(...)` to ensure /// blocks are removed from the cache that are below a given height. - fn test_pruning_blocks_below_a_given_height_from_the_cache( - mut state: BlockchainState, + async fn test_pruning_blocks_below_a_given_height_from_the_cache( + state: BlockchainState, ) { let test_state = TestState::setup(); let block_1_hash = test_state.block_1.block_hash(); let block_2_hash = test_state.block_2.block_hash(); - state.add_block(test_state.block_1).unwrap(); - state.add_block(test_state.block_2).unwrap(); + state.add_block(test_state.block_1).await.unwrap(); + state.add_block(test_state.block_2).await.unwrap(); state.prune_blocks_below_height(2); - assert!(!state.block_cache.contains_key(&block_1_hash)); - assert!(state.block_cache.contains_key(&block_2_hash)); + assert!( + !state + .block_cache + .read() + .unwrap() + .contains_key(&block_1_hash) + ); + assert!( + state + .block_cache + .read() + .unwrap() + .contains_key(&block_2_hash) + ); } - #[test] - fn test_pruning_blocks_below_a_given_height_from_the_cache_in_memory() { + #[tokio::test] + async fn test_pruning_blocks_below_a_given_height_from_the_cache_in_memory() { run_in_memory( Network::Bitcoin, test_pruning_blocks_below_a_given_height_from_the_cache, ) + .await } - #[test] - fn test_pruning_blocks_below_a_given_height_from_the_cache_on_disk() { + #[tokio::test] + async fn test_pruning_blocks_below_a_given_height_from_the_cache_on_disk() { run_with_cache_dir( Network::Bitcoin, test_pruning_blocks_below_a_given_height_from_the_cache, ) + .await } /// Simple test to verify that `BlockchainState::block_cache_size()` returns the total /// number of bytes in the block cache. - fn test_block_cache_size(mut state: BlockchainState) { + async fn test_block_cache_size(state: BlockchainState) { let test_state = TestState::setup(); let block_cache_size = state.get_block_cache_size(); assert_eq!(block_cache_size, 0); - state.add_block(test_state.block_1.clone()).unwrap(); - state.add_block(test_state.block_2.clone()).unwrap(); + state.add_block(test_state.block_1.clone()).await.unwrap(); + state.add_block(test_state.block_2.clone()).await.unwrap(); let expected_cache_size = test_state.block_1.total_size() + test_state.block_2.total_size(); let block_cache_size = state.get_block_cache_size(); @@ -676,23 +726,23 @@ mod test { assert_eq!(expected_cache_size, block_cache_size); } - #[test] - fn test_block_cache_size_in_memory() { - run_in_memory(Network::Bitcoin, test_block_cache_size) + #[tokio::test] + async fn test_block_cache_size_in_memory() { + run_in_memory(Network::Bitcoin, test_block_cache_size).await } - #[test] - fn test_block_cache_size_on_disk() { - run_with_cache_dir(Network::Bitcoin, test_block_cache_size) + #[tokio::test] + async fn test_block_cache_size_on_disk() { + run_with_cache_dir(Network::Bitcoin, test_block_cache_size).await } /// Test that verifies that the tip is always correctly sorted in case of forks and blocks /// with unknown headers. - fn test_sorted_tip(mut state: BlockchainState) { + async fn test_sorted_tip(state: BlockchainState) { let h1 = state.genesis(); // h1 - h2 let h2 = generate_header(h1.block_hash(), h1.time, 0); - state.add_headers(&[h2]); + state.add_headers(&[h2]).await; assert_eq!(state.get_active_chain_tip().header, h2); // Create a fork with 3 headers where the last one is invalid and check that h3f is the tip. @@ -704,7 +754,7 @@ mod test { let h3f = generate_header(h2f.block_hash(), h2f.time, 0); // Set time to zero to make header invalid let h4f_invalid = generate_header(h2f.block_hash(), 0, 0); - state.add_headers(&[h2f, h3f, h4f_invalid]); + state.add_headers(&[h2f, h3f, h4f_invalid]).await; assert_eq!(state.get_active_chain_tip().header, h3f); // Extend non fork chain with blocks (with unknown headers) and make sure h4 is tip. @@ -718,32 +768,34 @@ mod test { header: h3, txdata: Vec::new(), }) + .await .unwrap(); state .add_block(Block { header: h4, txdata: Vec::new(), }) + .await .unwrap(); assert_eq!(state.get_active_chain_tip().header, h4); } - #[test] - fn test_sorted_tip_in_memory() { - run_in_memory(Network::Regtest, test_sorted_tip) + #[tokio::test] + async fn test_sorted_tip_in_memory() { + run_in_memory(Network::Regtest, test_sorted_tip).await } - #[test] - fn test_sorted_tip_on_disk() { - run_with_cache_dir(Network::Regtest, test_sorted_tip) + #[tokio::test] + async fn test_sorted_tip_on_disk() { + run_with_cache_dir(Network::Regtest, test_sorted_tip).await } /// Test header store `get_header` function. - fn test_headerstore_get_cached_header(mut state: BlockchainState) { + async fn test_headerstore_get_cached_header(state: BlockchainState) { let initial_header = state.genesis(); let chain = generate_headers(initial_header.block_hash(), initial_header.time, 2500, &[]); - let (added_headers, maybe_err) = state.add_headers(&chain); + let (added_headers, maybe_err) = state.add_headers(&chain).await; assert_eq!(added_headers.len(), 2500); assert!(maybe_err.is_none()); @@ -760,13 +812,13 @@ mod test { } } - #[test] - fn test_headerstore_get_cached_header_in_memory() { - run_in_memory(Network::Regtest, test_headerstore_get_cached_header) + #[tokio::test] + async fn test_headerstore_get_cached_header_in_memory() { + run_in_memory(Network::Regtest, test_headerstore_get_cached_header).await } - #[test] - fn test_headerstore_get_cached_header_on_disk() { - run_with_cache_dir(Network::Regtest, test_headerstore_get_cached_header) + #[tokio::test] + async fn test_headerstore_get_cached_header_on_disk() { + run_with_cache_dir(Network::Regtest, test_headerstore_get_cached_header).await } } diff --git a/rs/bitcoin/adapter/src/connectionmanager.rs b/rs/bitcoin/adapter/src/connectionmanager.rs index 132322798509..bcb99be5246c 100644 --- a/rs/bitcoin/adapter/src/connectionmanager.rs +++ b/rs/bitcoin/adapter/src/connectionmanager.rs @@ -71,6 +71,9 @@ pub enum ConnectionManagerError { /// This can happen from recycling the DNS seed queue addresses. #[error("Address {0} is already connected")] AlreadyConnected(SocketAddr), + /// async error thrown by tokio runtime + #[error("JoinError")] + JoinError(#[from] tokio::task::JoinError), } /// This type is a simple wrapper for results created by a connection manager. @@ -165,14 +168,15 @@ impl ConnectionManager { /// This function contains the actions the must occur every time the connection /// manager must execute actions. - pub fn tick( + pub async fn tick( &mut self, current_height: BlockHeight, handle: fn(StreamConfigOf) -> JoinHandle<()>, ) { self.current_height = current_height; - if let Err(ConnectionManagerError::AddressBook(err)) = self.manage_connections(handle) { + if let Err(ConnectionManagerError::AddressBook(err)) = self.manage_connections(handle).await + { error!(self.logger, "{}", err); } } @@ -188,7 +192,7 @@ impl ConnectionManager { } /// This function will remove disconnects and establish new connections. - fn manage_connections( + async fn manage_connections( &mut self, handle: fn(StreamConfigOf) -> JoinHandle<()>, ) -> ConnectionManagerResult<()> { @@ -203,7 +207,7 @@ impl ConnectionManager { .known_peer_addresses .set(self.address_book.size() as i64); while self.connections.len() < self.get_max_number_of_connections() { - self.make_connection(handle)?; + self.make_connection(handle).await?; } Ok(()) } @@ -294,13 +298,13 @@ impl ConnectionManager { } /// This function creates a new connection with a stream to a BTC node. - fn make_connection( + async fn make_connection( &mut self, handle: fn(StreamConfigOf) -> JoinHandle<()>, ) -> ConnectionManagerResult<()> { self.metrics.connections.inc(); let address_entry_result = if !self.address_book.has_enough_addresses() { - self.address_book.pop_seed() + self.address_book.pop_seed().await } else { self.address_book.pop() }; @@ -912,7 +916,7 @@ mod test { manager.get_max_number_of_connections(), MAX_CONNECTIONS_DURING_ADDRESS_DISCOVERY ); - manager.tick(BLOCK_HEIGHT_FOR_TESTS, simple_handle); + manager.tick(BLOCK_HEIGHT_FOR_TESTS, simple_handle).await; for i in 0..4u8 { tokio::select! { event = manager @@ -932,7 +936,7 @@ mod test { // has been disconnected. continue; } - manager.tick(BLOCK_HEIGHT_FOR_TESTS, simple_handle); + manager.tick(BLOCK_HEIGHT_FOR_TESTS, simple_handle).await; } assert_eq!(manager.current_height, 1); assert_eq!( diff --git a/rs/bitcoin/adapter/src/get_successors_handler.rs b/rs/bitcoin/adapter/src/get_successors_handler.rs index 17fb44de4bee..350158b4bb3c 100644 --- a/rs/bitcoin/adapter/src/get_successors_handler.rs +++ b/rs/bitcoin/adapter/src/get_successors_handler.rs @@ -1,6 +1,6 @@ use std::{ collections::{HashSet, VecDeque}, - sync::{Arc, Mutex}, + sync::Arc, }; use bitcoin::{BlockHash, consensus::Encodable}; @@ -86,7 +86,7 @@ pub struct GetSuccessorsResponse
{ /// Contains the functionality to respond to GetSuccessorsRequests via the RPC /// server. pub struct GetSuccessorsHandler { - state: Arc>>, + state: Arc>, blockchain_manager_tx: Sender, network: Network, metrics: GetSuccessorMetrics, @@ -97,7 +97,7 @@ impl GetSuccessorsHandler { /// inside of the adapter when a `GetSuccessorsRequest` is received. pub fn new( network: Network, - state: Arc>>, + state: Arc>, blockchain_manager_tx: Sender, metrics_registry: &MetricsRegistry, ) -> Self { @@ -124,21 +124,21 @@ impl GetSuccessorsHandler { .observe(request.processed_block_hashes.len() as f64); let (blocks, next, obsolete_blocks) = { - let state = self.state.lock().unwrap(); - let anchor_height = state + let anchor_height = self + .state .get_cached_header(&request.anchor) .map_or(0, |cached| cached.data.height); let allow_multiple_blocks = self.network.are_multiple_blocks_allowed(anchor_height); let blocks = get_successor_blocks( - &state, + &self.state, &request.anchor, &request.processed_block_hashes, allow_multiple_blocks, &self.network, ); let next = get_next_headers( - &state, + &self.state, &request.anchor, &request.processed_block_hashes, &blocks, @@ -151,8 +151,8 @@ impl GetSuccessorsHandler { // There is also a chance that they are reachable from the anchor, just not through the cache. // Meaning that we still need to download some other blocks first. (hence we need to free the cache). let mut obsolete_blocks = request.processed_block_hashes; - if blocks.is_empty() && state.is_block_cache_full() { - obsolete_blocks.extend(state.get_cached_blocks()) + if blocks.is_empty() && self.state.is_block_cache_full() { + obsolete_blocks.extend(self.state.get_cached_blocks()) } (blocks, next, obsolete_blocks) }; @@ -311,8 +311,6 @@ fn get_next_headers( mod test { use super::*; - use std::sync::{Arc, Mutex}; - use bitcoin::{Block, consensus::Decodable}; use ic_metrics::MetricsRegistry; use tokio::sync::mpsc::channel; @@ -336,7 +334,7 @@ mod test { let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); let handler = GetSuccessorsHandler::new( network, - Arc::new(Mutex::new(blockchain_state)), + Arc::new(blockchain_state), blockchain_manager_tx, &MetricsRegistry::default(), ); @@ -379,16 +377,20 @@ mod test { }; { - let mut blockchain = handler.state.lock().unwrap(); - blockchain.add_headers(&main_chain); - blockchain.add_headers(&side_chain); - blockchain.add_headers(&side_chain_2); + let blockchain = &handler.state; + blockchain.add_headers(&main_chain).await; + blockchain.add_headers(&side_chain).await; + blockchain.add_headers(&side_chain_2).await; // Add main block 2 - blockchain.add_block(main_block_2).expect("invalid block"); + blockchain + .add_block(main_block_2) + .await + .expect("invalid block"); // Add side block 1 blockchain .add_block(side_block_1.clone()) + .await .expect("invalid block"); } @@ -444,7 +446,7 @@ mod test { let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); let handler = GetSuccessorsHandler::new( network, - Arc::new(Mutex::new(blockchain_state)), + Arc::new(blockchain_state), blockchain_manager_tx, &MetricsRegistry::default(), ); @@ -461,13 +463,15 @@ mod test { txdata: vec![], }; { - let mut blockchain = handler.state.lock().unwrap(); - blockchain.add_headers(&main_chain); + let blockchain = &handler.state; + blockchain.add_headers(&main_chain).await; blockchain .add_block(main_block_1.clone()) + .await .expect("invalid block"); blockchain .add_block(main_block_2.clone()) + .await .expect("invalid block"); } let request = GetSuccessorsRequest { @@ -492,7 +496,7 @@ mod test { let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); let handler = GetSuccessorsHandler::new( network, - Arc::new(Mutex::new(blockchain_state)), + Arc::new(blockchain_state), blockchain_manager_tx, &MetricsRegistry::default(), ); @@ -521,17 +525,20 @@ mod test { txdata: vec![], }; { - let mut blockchain = handler.state.lock().unwrap(); - blockchain.add_headers(&main_chain); - blockchain.add_headers(&side_chain); + let blockchain = &handler.state; + blockchain.add_headers(&main_chain).await; + blockchain.add_headers(&side_chain).await; blockchain .add_block(main_block_1.clone()) + .await .expect("invalid block"); blockchain .add_block(main_block_2.clone()) + .await .expect("invalid block"); blockchain .add_block(side_block_1.clone()) + .await .expect("invalid block"); } // |-> 1' @@ -563,20 +570,20 @@ mod test { let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); let handler = GetSuccessorsHandler::new( network, - Arc::new(Mutex::new(blockchain_state)), + Arc::new(blockchain_state), blockchain_manager_tx, &MetricsRegistry::default(), ); let main_chain = generate_headers(genesis_hash, genesis.time, 120, &[]); { - let mut blockchain = handler.state.lock().unwrap(); - blockchain.add_headers(&main_chain); + let blockchain = &handler.state; + blockchain.add_headers(&main_chain).await; for header in main_chain { let block = Block { header, txdata: vec![], }; - blockchain.add_block(block).expect("invalid block"); + blockchain.add_block(block).await.expect("invalid block"); } } @@ -599,7 +606,7 @@ mod test { let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); let handler = GetSuccessorsHandler::new( network, - Arc::new(Mutex::new(blockchain_state)), + Arc::new(blockchain_state), blockchain_manager_tx, &MetricsRegistry::default(), ); @@ -624,22 +631,26 @@ mod test { txdata: vec![], }; { - let mut blockchain = handler.state.lock().unwrap(); - let (_, maybe_err) = blockchain.add_headers(&main_chain); + let blockchain = &handler.state; + let (_, maybe_err) = blockchain.add_headers(&main_chain).await; assert!( maybe_err.is_none(), "Error was found in main chain: {maybe_err:#?}" ); - let (_, maybe_err) = blockchain.add_headers(&side_chain); + let (_, maybe_err) = blockchain.add_headers(&side_chain).await; assert!( maybe_err.is_none(), "Error was found in side chain: {maybe_err:#?}" ); - blockchain.add_block(main_block_2).expect("invalid block"); + blockchain + .add_block(main_block_2) + .await + .expect("invalid block"); blockchain .add_block(side_block_1.clone()) + .await .expect("invalid block"); } @@ -689,7 +700,7 @@ mod test { let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); let handler = GetSuccessorsHandler::new( network, - Arc::new(Mutex::new(blockchain_state)), + Arc::new(blockchain_state), blockchain_manager_tx, &MetricsRegistry::default(), ); @@ -712,16 +723,20 @@ mod test { }; { - let mut blockchain = handler.state.lock().unwrap(); - let (added_headers, _) = blockchain.add_headers(&headers); + let blockchain = &handler.state; + let (added_headers, _) = blockchain.add_headers(&headers).await; assert_eq!(added_headers.len(), 1); - let (added_headers, _) = blockchain.add_headers(&additional_headers); + let (added_headers, _) = blockchain.add_headers(&additional_headers).await; assert_eq!(added_headers.len(), 1); blockchain .add_block(large_block.clone()) + .await + .expect("invalid block"); + blockchain + .add_block(small_block) + .await .expect("invalid block"); - blockchain.add_block(small_block).expect("invalid block"); }; let request = GetSuccessorsRequest { @@ -751,7 +766,7 @@ mod test { let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); let handler = GetSuccessorsHandler::new( network, - Arc::new(Mutex::new(blockchain_state)), + Arc::new(blockchain_state), blockchain_manager_tx, &MetricsRegistry::default(), ); @@ -761,8 +776,8 @@ mod test { generate_large_block_blockchain(main_chain[4].block_hash(), main_chain[4].time, 1); { - let mut blockchain = handler.state.lock().unwrap(); - let (added_headers, _) = blockchain.add_headers(&main_chain); + let blockchain = &handler.state; + let (added_headers, _) = blockchain.add_headers(&main_chain).await; assert_eq!(added_headers.len(), 5); let main_blocks = main_chain .iter() @@ -772,11 +787,11 @@ mod test { }) .collect::>(); for block in main_blocks { - blockchain.add_block(block).unwrap(); + blockchain.add_block(block).await.unwrap(); } for block in &large_blocks { - blockchain.add_block(block.clone()).unwrap(); + blockchain.add_block(block.clone()).await.unwrap(); } }; diff --git a/rs/bitcoin/adapter/src/header_cache.rs b/rs/bitcoin/adapter/src/header_cache.rs index 5056b5aaa8dd..8c91a2a89d94 100644 --- a/rs/bitcoin/adapter/src/header_cache.rs +++ b/rs/bitcoin/adapter/src/header_cache.rs @@ -234,7 +234,7 @@ impl HeaderCache for RwLock Environment { let mut builder = Environment::new(); - let builder_flags = EnvironmentFlags::NO_TLS; + let builder_flags = EnvironmentFlags::NO_TLS | EnvironmentFlags::NO_META_SYNC; let permission = 0o644; builder.set_flags(builder_flags); // 3 databases: 1 for headers, 1 for tips, 1 for parent-child relation. diff --git a/rs/bitcoin/adapter/src/lib.rs b/rs/bitcoin/adapter/src/lib.rs index c4b67ef06fc2..08cd571d9c53 100644 --- a/rs/bitcoin/adapter/src/lib.rs +++ b/rs/bitcoin/adapter/src/lib.rs @@ -8,11 +8,7 @@ use bitcoin::p2p::message::NetworkMessage; use bitcoin::{BlockHash, block::Header as PureHeader}; use ic_logger::ReplicaLogger; use ic_metrics::MetricsRegistry; -use std::{ - net::SocketAddr, - sync::{Arc, Mutex}, - time::Instant, -}; +use std::{net::SocketAddr, sync::Arc, time::Instant}; use tokio::sync::{mpsc::channel, watch}; /// This module contains the AddressManager struct. The struct stores addresses /// that will be used to create new connections. It also tracks addresses that @@ -198,68 +194,71 @@ impl AdapterState { } } -fn start_server_helper( - log: &ReplicaLogger, - metrics_registry: &MetricsRegistry, - rt_handle: &tokio::runtime::Handle, +async fn start_server_helper( + log: ReplicaLogger, + metrics_registry: MetricsRegistry, config: config::Config, ) where Network: BlockchainNetwork + Sync + Send + 'static, Network::Header: Send, - Network::Block: Send, + Network::Block: Send + Sync, { - let _enter = rt_handle.enter(); let (adapter_state, tx) = AdapterState::new(config.idle_seconds); let (blockchain_manager_tx, blockchain_manager_rx) = channel(100); let blockchain_state = if let Some(cache_dir) = &config.cache_dir { BlockchainState::new_with_cache_dir( config.network, cache_dir.clone(), - metrics_registry, + &metrics_registry, log.clone(), ) } else { - BlockchainState::new(config.network, metrics_registry) + BlockchainState::new(config.network, &metrics_registry) }; - let blockchain_state = Arc::new(Mutex::new(blockchain_state)); + let blockchain_state = Arc::new(blockchain_state); let (transaction_manager_tx, transaction_manager_rx) = channel(100); - start_grpc_server( - config.network, - config.incoming_source.clone(), - log.clone(), - tx, - blockchain_state.clone(), - blockchain_manager_tx, - transaction_manager_tx, - metrics_registry, - ); - start_main_event_loop( - &config, - log.clone(), - blockchain_state, - transaction_manager_rx, - adapter_state, - blockchain_manager_rx, - metrics_registry, - ) + let handles = [ + start_grpc_server( + config.network, + config.incoming_source.clone(), + log.clone(), + tx, + blockchain_state.clone(), + blockchain_manager_tx, + transaction_manager_tx, + &metrics_registry, + ), + start_main_event_loop( + &config, + log.clone(), + blockchain_state, + transaction_manager_rx, + adapter_state, + blockchain_manager_rx, + &metrics_registry, + ), + ]; + + for handle in handles { + let _ = handle.await; // Waits for each task to complete + } } /// Starts the gRPC server and the router for handling incoming requests. -pub fn start_server( - log: &ReplicaLogger, - metrics_registry: &MetricsRegistry, - rt_handle: &tokio::runtime::Handle, +pub async fn start_server( + log: ReplicaLogger, + metrics_registry: MetricsRegistry, config: config::Config, ) { match config.network { AdapterNetwork::Bitcoin(network) => { let btc_config = config.with_network(network); - start_server_helper(log, metrics_registry, rt_handle, btc_config) + start_server_helper(log, metrics_registry, btc_config).await } AdapterNetwork::Dogecoin(network) => { let doge_config = config.with_network(network); - start_server_helper(log, metrics_registry, rt_handle, doge_config) + start_server_helper(log, metrics_registry, doge_config).await } } } diff --git a/rs/bitcoin/adapter/src/router.rs b/rs/bitcoin/adapter/src/router.rs index dfbd86dd66c7..4a65af6bb63a 100644 --- a/rs/bitcoin/adapter/src/router.rs +++ b/rs/bitcoin/adapter/src/router.rs @@ -15,7 +15,7 @@ use bitcoin::p2p::message::NetworkMessage; use ic_logger::ReplicaLogger; use ic_metrics::MetricsRegistry; use std::net::SocketAddr; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use std::time::Duration; use tokio::{ sync::mpsc::{Receiver, channel}, @@ -30,15 +30,16 @@ use tokio::{ pub fn start_main_event_loop( config: &Config, logger: ReplicaLogger, - blockchain_state: Arc>>, + blockchain_state: Arc>, mut transaction_manager_rx: Receiver, mut adapter_state: AdapterState, mut blockchain_manager_rx: Receiver, metrics_registry: &MetricsRegistry, -) where - Network: BlockchainNetwork + Send + 'static, +) -> tokio::task::JoinHandle<()> +where + Network: BlockchainNetwork + Send + Sync + 'static, Network::Header: Send, - Network::Block: Send, + Network::Block: Send + Sync, { let (network_message_sender, mut network_message_receiver) = channel::<( SocketAddr, @@ -88,7 +89,7 @@ pub fn start_main_event_loop( connection_manager.discard(&address); } - if let Err(ProcessNetworkMessageError::InvalidMessage) = blockchain_manager.process_bitcoin_network_message(&mut connection_manager, address, &message) { + if let Err(ProcessNetworkMessageError::InvalidMessage) = blockchain_manager.process_bitcoin_network_message(&mut connection_manager, address, &message).await { connection_manager.discard(&address); } if let Err(ProcessNetworkMessageError::InvalidMessage) = transaction_manager.process_bitcoin_network_message(&mut connection_manager, address, &message) { @@ -114,11 +115,11 @@ pub fn start_main_event_loop( _ = tick_interval.tick() => { // After an event is dispatched, the managers `tick` method is called to process possible // outgoing messages. - connection_manager.tick(blockchain_manager.get_height(), handle_stream); + connection_manager.tick(blockchain_manager.get_height(), handle_stream).await; blockchain_manager.tick(&mut connection_manager); transaction_manager.advertise_txids(&mut connection_manager); } }; } - }); + }) } diff --git a/rs/bitcoin/adapter/src/rpc_server.rs b/rs/bitcoin/adapter/src/rpc_server.rs index 8277b84025fc..024cbce25d5f 100644 --- a/rs/bitcoin/adapter/src/rpc_server.rs +++ b/rs/bitcoin/adapter/src/rpc_server.rs @@ -15,10 +15,9 @@ use ic_http_endpoints_async_utils::{incoming_from_nth_systemd_socket, incoming_f use ic_logger::{ReplicaLogger, debug}; use ic_metrics::MetricsRegistry; use std::convert::{TryFrom, TryInto}; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use std::time::Instant; -use tokio::sync::mpsc; -use tokio::sync::watch; +use tokio::sync::{mpsc, watch}; use tonic::{Request, Response, Status, transport::Server}; struct BtcServiceImpl { @@ -133,11 +132,12 @@ pub fn start_grpc_server( incoming_source: IncomingSource, logger: ReplicaLogger, last_received_tx: watch::Sender>, - blockchain_state: Arc>>, + blockchain_state: Arc>, blockchain_manager_tx: mpsc::Sender, transaction_manager_tx: mpsc::Sender, metrics_registry: &MetricsRegistry, -) where +) -> tokio::task::JoinHandle<()> +where Network: BlockchainNetwork + Sync + Send + 'static, Network::Header: Send, { @@ -166,7 +166,7 @@ pub fn start_grpc_server( .serve_with_incoming(incoming); tokio::spawn(async move { server_fut.await.expect("gRPC server crashed"); - }); + }) } IncomingSource::Systemd => { let incoming = unsafe { incoming_from_nth_systemd_socket(1) }; @@ -179,7 +179,7 @@ pub fn start_grpc_server( .serve_with_incoming(incoming); tokio::spawn(async move { server_fut.await.expect("gRPC server crashed"); - }); + }) } - }; + } } diff --git a/rs/bitcoin/adapter/tests/integration.rs b/rs/bitcoin/adapter/tests/integration.rs index 59248555d6e9..ab1a98183c53 100644 --- a/rs/bitcoin/adapter/tests/integration.rs +++ b/rs/bitcoin/adapter/tests/integration.rs @@ -13,6 +13,7 @@ use ic_btc_replica_types::{ Network, SendTransactionRequest, }; use ic_config::bitcoin_payload_builder_config::Config as BitcoinPayloadBuilderConfig; +use ic_config::logger::{Config as LoggerConfig, Level as LoggerLevel}; use ic_interfaces_adapter_client::{Options, RpcAdapterClient, RpcError}; use ic_logger::{ReplicaLogger, replica_logger::no_op_logger}; use ic_metrics::MetricsRegistry; @@ -91,8 +92,8 @@ fn make_send_tx_request( } fn start_adapter>( - logger: &ReplicaLogger, - metrics_registry: &MetricsRegistry, + logger: ReplicaLogger, + metrics_registry: MetricsRegistry, rt_handle: &tokio::runtime::Handle, nodes: Vec, uds_path: &Path, @@ -104,10 +105,14 @@ fn start_adapter>( ipv6_only: true, address_limits: (1, 1), idle_seconds: 6, // it can take at most 5 seconds for tcp connections etc to be established. + logger: LoggerConfig { + level: LoggerLevel::Trace, + ..LoggerConfig::default() + }, ..Config::default_with(network.into()) }; - - start_server(logger, metrics_registry, rt_handle, config); + let _enter = rt_handle.enter(); + rt_handle.spawn(start_server(logger, metrics_registry, config)); } fn start_bitcoind(network: T) -> Daemon { @@ -168,8 +173,8 @@ fn start_adapter_and_client>( let res = Builder::new() .make(|uds_path| { start_adapter( - &logger, - &metrics_registry, + logger.clone(), + metrics_registry.clone(), rt.handle(), urls.clone(), uds_path, @@ -190,7 +195,14 @@ fn start_adapter_and_client>( .unwrap(); if let AdapterState::Active = adapter_state { // We send this request to make sure the adapter is not idle. - let _ = make_get_successors_request(&res.0, anchor[..].to_vec(), vec![]); + for _ in 0..10 { + let res = make_get_successors_request(&res.0, anchor[..].to_vec(), vec![]); + if res.is_err() { + std::thread::sleep(std::time::Duration::from_secs(1)); + } else { + break; + } + } } res @@ -236,6 +248,31 @@ fn wait_for_connection(client: &RpcClient, connection_count } } +/* +fn wait_for_connection_( + client: &RpcClient, + connection_count: usize, + rt_handle: &tokio::runtime::Handle, +) { + let mut tries = 0; + while client.get_connection_count().unwrap() != connection_count { + std::thread::sleep(std::time::Duration::from_secs(1)); + tries += 1; + if tries > 5 { + rt_handle.block_on(async { + let dump = rt_handle.dump().await; + for (i, task) in dump.tasks().iter().enumerate() { + let trace = task.trace(); + println!("TASK {i}:"); + println!("{trace}\n"); + } + }); + panic!("Timeout in wait_for_connection"); + } + } +} +*/ + // This is an expensive operation. Should only be used when checking for an upper bound on the number of connections. fn exact_connections(client: &RpcClient, connection_count: usize) { // It always takes less than 5 seconds for the client to connect to the adapter. @@ -343,6 +380,13 @@ fn sync_blocks( Err(err) => panic!("{err:?}"), } tries += 1; + eprintln!( + "synced blocks = {} <= {}, tries {} <= {}", + blocks.len(), + max_num_blocks, + tries, + max_tries + ); std::thread::sleep(std::time::Duration::from_secs(1)); } @@ -805,7 +849,8 @@ fn doge_test_send_tx() { /// Checks that the client (replica) receives blocks from both created forks. fn test_receives_blocks_from_forks>() { - let logger = no_op_logger(); + use ic_logger::new_replica_logger_from_config; + let network = T::REGTEST; let bitcoind1 = start_bitcoind(network); let client1 = &bitcoind1.rpc_client; @@ -817,6 +862,12 @@ fn test_receives_blocks_from_forks>() { let url2 = bitcoind2.p2p_socket().unwrap(); let rt = tokio::runtime::Runtime::new().unwrap(); + + let logger_config = LoggerConfig { + level: LoggerLevel::Trace, + ..LoggerConfig::default() + }; + let (logger, _async_log_guard) = new_replica_logger_from_config(&logger_config); let (adapter_client, _path) = start_active_adapter_and_client( &rt, vec![SocketAddr::V4(url1), SocketAddr::V4(url2)], From e99beb332dc9a3c503167e88291af506a5b59c55 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Tue, 16 Sep 2025 00:18:01 -0700 Subject: [PATCH 02/20] Update bench --- rs/bitcoin/adapter/benches/e2e.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/rs/bitcoin/adapter/benches/e2e.rs b/rs/bitcoin/adapter/benches/e2e.rs index 3c57fb9cf9a0..bc8a3de1ad19 100644 --- a/rs/bitcoin/adapter/benches/e2e.rs +++ b/rs/bitcoin/adapter/benches/e2e.rs @@ -80,13 +80,7 @@ fn e2e(criterion: &mut Criterion) { .make(|uds_path| { Ok(rt.block_on(async { config.incoming_source = IncomingSource::Path(uds_path.to_path_buf()); - - start_server( - &no_op_logger(), - &MetricsRegistry::default(), - rt.handle(), - config.clone(), - ); + start_server(no_op_logger(), MetricsRegistry::default(), config.clone()).await; start_client(uds_path).await })) }) @@ -193,12 +187,14 @@ fn add_800k_block_headers(criterion: &mut Criterion) { group.sample_size(10); group.bench_function("add_headers", |bench| { + let rt = tokio::runtime::Runtime::new().unwrap(); bench.iter(|| { - let mut blockchain_state = + let blockchain_state = BlockchainState::new(Network::Bitcoin, &MetricsRegistry::default()); // Headers are processed in chunks of at most MAX_HEADERS_SIZE entries for chunk in bitcoin_headers_to_add.chunks(MAX_HEADERS_SIZE) { - let (added_headers, error) = blockchain_state.add_headers(chunk); + let (added_headers, error) = + rt.block_on(async { blockchain_state.add_headers(chunk).await }); assert!(error.is_none()); assert_eq!(added_headers.len(), chunk.len()) } From 1d234ff9d382dcf7ad79cc6ac00e4ff9841537b7 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Tue, 16 Sep 2025 00:25:31 -0700 Subject: [PATCH 03/20] Cleanup --- rs/bitcoin/adapter/tests/integration.rs | 36 +++---------------------- 1 file changed, 3 insertions(+), 33 deletions(-) diff --git a/rs/bitcoin/adapter/tests/integration.rs b/rs/bitcoin/adapter/tests/integration.rs index ab1a98183c53..7e81689f9b59 100644 --- a/rs/bitcoin/adapter/tests/integration.rs +++ b/rs/bitcoin/adapter/tests/integration.rs @@ -194,7 +194,9 @@ fn start_adapter_and_client>( .parse() .unwrap(); if let AdapterState::Active = adapter_state { - // We send this request to make sure the adapter is not idle. + // Send this request to make sure the adapter is not idle. + // Retry until the request goes through, because the adapter may not be fully + // started yet. for _ in 0..10 { let res = make_get_successors_request(&res.0, anchor[..].to_vec(), vec![]); if res.is_err() { @@ -248,31 +250,6 @@ fn wait_for_connection(client: &RpcClient, connection_count } } -/* -fn wait_for_connection_( - client: &RpcClient, - connection_count: usize, - rt_handle: &tokio::runtime::Handle, -) { - let mut tries = 0; - while client.get_connection_count().unwrap() != connection_count { - std::thread::sleep(std::time::Duration::from_secs(1)); - tries += 1; - if tries > 5 { - rt_handle.block_on(async { - let dump = rt_handle.dump().await; - for (i, task) in dump.tasks().iter().enumerate() { - let trace = task.trace(); - println!("TASK {i}:"); - println!("{trace}\n"); - } - }); - panic!("Timeout in wait_for_connection"); - } - } -} -*/ - // This is an expensive operation. Should only be used when checking for an upper bound on the number of connections. fn exact_connections(client: &RpcClient, connection_count: usize) { // It always takes less than 5 seconds for the client to connect to the adapter. @@ -380,13 +357,6 @@ fn sync_blocks( Err(err) => panic!("{err:?}"), } tries += 1; - eprintln!( - "synced blocks = {} <= {}, tries {} <= {}", - blocks.len(), - max_num_blocks, - tries, - max_tries - ); std::thread::sleep(std::time::Duration::from_secs(1)); } From 0528f7c7922cb032153b1aae5c26c5c7f0847161 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Tue, 16 Sep 2025 00:41:54 -0700 Subject: [PATCH 04/20] Cleanup --- rs/bitcoin/adapter/src/addressbook.rs | 3 ++- rs/bitcoin/adapter/src/header_cache.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/rs/bitcoin/adapter/src/addressbook.rs b/rs/bitcoin/adapter/src/addressbook.rs index 098737b0d651..09cb2799ed40 100644 --- a/rs/bitcoin/adapter/src/addressbook.rs +++ b/rs/bitcoin/adapter/src/addressbook.rs @@ -122,7 +122,8 @@ impl AddressBook { } /// This function takes the DNS seeds and creates a new queue of socket addresses to connect to - /// for the address discovery process. + /// for the address discovery process. It is made async because `to_socket_addrs` is a blocking + /// operation that may involve DNS lookup. async fn build_seed_queue(&mut self) -> Result<(), tokio::task::JoinError> { let mut rng = StdRng::from_entropy(); let dns_seeds = self diff --git a/rs/bitcoin/adapter/src/header_cache.rs b/rs/bitcoin/adapter/src/header_cache.rs index 8c91a2a89d94..5056b5aaa8dd 100644 --- a/rs/bitcoin/adapter/src/header_cache.rs +++ b/rs/bitcoin/adapter/src/header_cache.rs @@ -234,7 +234,7 @@ impl HeaderCache for RwLock Environment { let mut builder = Environment::new(); - let builder_flags = EnvironmentFlags::NO_TLS | EnvironmentFlags::NO_META_SYNC; + let builder_flags = EnvironmentFlags::NO_TLS; let permission = 0o644; builder.set_flags(builder_flags); // 3 databases: 1 for headers, 1 for tips, 1 for parent-child relation. From 5b62806309cc8723250b6340338d77feca871d33 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Tue, 16 Sep 2025 00:51:11 -0700 Subject: [PATCH 05/20] Fix --- rs/pocket_ic_server/src/pocket_ic.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/rs/pocket_ic_server/src/pocket_ic.rs b/rs/pocket_ic_server/src/pocket_ic.rs index 68832c849508..34a26ab88745 100644 --- a/rs/pocket_ic_server/src/pocket_ic.rs +++ b/rs/pocket_ic_server/src/pocket_ic.rs @@ -309,7 +309,6 @@ impl BitcoinAdapterParts { log_level: Option, replica_logger: ReplicaLogger, metrics_registry: MetricsRegistry, - runtime: Arc, ) -> Self { let bitcoin_adapter_config = BitcoinAdapterConfig { nodes: bitcoind_addr, @@ -321,12 +320,7 @@ impl BitcoinAdapterParts { ..BitcoinAdapterConfig::default_with(Network::Regtest.into()) }; let adapter = tokio::spawn(async move { - start_btc_server( - &replica_logger, - &metrics_registry, - runtime.handle(), - bitcoin_adapter_config, - ) + start_btc_server(replica_logger, metrics_registry, bitcoin_adapter_config).await }); let start = std::time::Instant::now(); loop { @@ -862,7 +856,6 @@ impl PocketIcSubnets { self.log_level, sm.replica_logger.clone(), sm.metrics_registry.clone(), - self.runtime.clone(), )); } From 9b558143ddc03ff69de151d5fc188897c6ba38e2 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Tue, 16 Sep 2025 00:58:28 -0700 Subject: [PATCH 06/20] Cleanup --- rs/bitcoin/adapter/tests/integration.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/rs/bitcoin/adapter/tests/integration.rs b/rs/bitcoin/adapter/tests/integration.rs index 7e81689f9b59..d99a1887b19e 100644 --- a/rs/bitcoin/adapter/tests/integration.rs +++ b/rs/bitcoin/adapter/tests/integration.rs @@ -105,10 +105,6 @@ fn start_adapter>( ipv6_only: true, address_limits: (1, 1), idle_seconds: 6, // it can take at most 5 seconds for tcp connections etc to be established. - logger: LoggerConfig { - level: LoggerLevel::Trace, - ..LoggerConfig::default() - }, ..Config::default_with(network.into()) }; let _enter = rt_handle.enter(); From ec9ffcbaa6e9270f0f2548287a39fe3b2261b406 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Tue, 16 Sep 2025 03:45:56 -0700 Subject: [PATCH 07/20] Remove extra trace --- rs/bitcoin/adapter/src/blockchainmanager.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/rs/bitcoin/adapter/src/blockchainmanager.rs b/rs/bitcoin/adapter/src/blockchainmanager.rs index 8420deb784ff..75e912e2cb2d 100644 --- a/rs/bitcoin/adapter/src/blockchainmanager.rs +++ b/rs/bitcoin/adapter/src/blockchainmanager.rs @@ -522,7 +522,6 @@ impl BlockchainManager { fn sync_blocks(&mut self, channel: &mut impl Channel) { // Timeout requests so they may be retried again. - trace!(self.logger, "in sync_blocks"); let mut retry_queue: LinkedHashSet = LinkedHashSet::new(); for (block_hash, request) in self.getdata_request_info.iter_mut() { match request.sent_at { @@ -545,7 +544,6 @@ impl BlockchainManager { // If nothing to be synced, then there is nothing to do at this point. if retry_queue.is_empty() && self.block_sync_queue.is_empty() { - trace!(self.logger, "sync_blocks returns early 1"); return; } @@ -568,7 +566,6 @@ impl BlockchainManager { let mut peers: Vec<_> = self.peer_info.values().collect(); let len = peers.len(); if len == 0 { - trace!(self.logger, "sync_blocks returns early 2"); return; } peers.rotate_left(self.round_robin_offset % len); @@ -595,10 +592,6 @@ impl BlockchainManager { } if selected_inventory.is_empty() { - trace!( - self.logger, - "sync_blocks get_next_block_hash_to_sync returns None" - ); continue; } From 2c649918fca916d34f3a73edd3c4110f60f4ce7c Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Tue, 16 Sep 2025 04:13:52 -0700 Subject: [PATCH 08/20] Parallel futures --- rs/bitcoin/adapter/src/addressbook.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/rs/bitcoin/adapter/src/addressbook.rs b/rs/bitcoin/adapter/src/addressbook.rs index 09cb2799ed40..c5582b5aafba 100644 --- a/rs/bitcoin/adapter/src/addressbook.rs +++ b/rs/bitcoin/adapter/src/addressbook.rs @@ -132,17 +132,18 @@ impl AddressBook { .map(|seed| format_addr(seed, self.port)) .collect::>(); let ipv6_only = self.ipv6_only; - let mut addresses = tokio::task::spawn_blocking(move || { - dns_seeds - .into_iter() - .flat_map(|seed| { - seed.to_socket_addrs().map_or(vec![], |v| { - v.filter(|addr| !ipv6_only || addr.is_ipv6()).collect() - }) + let tasks = dns_seeds + .into_iter() + .map(|seed| tokio::task::spawn_blocking(move || seed.to_socket_addrs())); + let mut addresses = futures::future::try_join_all(tasks) + .await? + .into_iter() + .flat_map(|addr| { + addr.map_or(vec![], |v| { + v.filter(|addr| !ipv6_only || addr.is_ipv6()).collect() }) - .collect::>() - }) - .await?; + }) + .collect::>(); addresses.shuffle(&mut rng); self.seed_queue = addresses.into_iter().collect(); Ok(()) From 0743fc4c4c460aae989865390bab4061efcc6612 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Tue, 16 Sep 2025 09:06:07 -0700 Subject: [PATCH 09/20] Address reviewer comments --- rs/bitcoin/adapter/src/blockchainstate.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rs/bitcoin/adapter/src/blockchainstate.rs b/rs/bitcoin/adapter/src/blockchainstate.rs index eef389a67427..362e0af72582 100644 --- a/rs/bitcoin/adapter/src/blockchainstate.rs +++ b/rs/bitcoin/adapter/src/blockchainstate.rs @@ -44,7 +44,7 @@ pub type SerializedBlock = Vec; /// The BlockChainState caches all the Bitcoin headers, some of the Bitcoin blocks. /// The BlockChainState also maintains the child relationhips between the headers. pub struct BlockchainState { - /// This field stores all the Bitcoin headers using a HashMap containining BlockHash and the corresponding header. + /// This field stores all the Bitcoin headers using a HashMap containing BlockHash and the corresponding header. header_cache: Arc + Send>, /// This field stores a hashmap containing BlockHash and the corresponding SerializedBlock. @@ -165,7 +165,7 @@ where }) }) .await - .map_err(|err| AddHeaderError::Internal(format!("{}", err)))? + .map_err(|err: tokio::task::JoinError| AddHeaderError::Internal(format!("{}", err)))? } /// This method adds a new block to the `block_cache` From 67a18b258085e10ce959e1999385a02d72954183 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Tue, 16 Sep 2025 17:02:48 -0700 Subject: [PATCH 10/20] Refactor --- rs/bitcoin/adapter/src/addressbook.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/rs/bitcoin/adapter/src/addressbook.rs b/rs/bitcoin/adapter/src/addressbook.rs index c5582b5aafba..57c5cfeb27eb 100644 --- a/rs/bitcoin/adapter/src/addressbook.rs +++ b/rs/bitcoin/adapter/src/addressbook.rs @@ -126,15 +126,11 @@ impl AddressBook { /// operation that may involve DNS lookup. async fn build_seed_queue(&mut self) -> Result<(), tokio::task::JoinError> { let mut rng = StdRng::from_entropy(); - let dns_seeds = self - .dns_seeds - .iter() - .map(|seed| format_addr(seed, self.port)) - .collect::>(); let ipv6_only = self.ipv6_only; - let tasks = dns_seeds - .into_iter() - .map(|seed| tokio::task::spawn_blocking(move || seed.to_socket_addrs())); + let tasks = self.dns_seeds.iter().map(|seed| { + let addr = format_addr(seed, self.port); + tokio::task::spawn_blocking(move || addr.to_socket_addrs()) + }); let mut addresses = futures::future::try_join_all(tasks) .await? .into_iter() From 5d6f1a302a96df6dc4e3199f51c1fc373a50e589 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Wed, 17 Sep 2025 06:38:39 -0700 Subject: [PATCH 11/20] feat(btc-adapter): implement a hybrid header cache --- rs/bitcoin/adapter/src/blockchainmanager.rs | 3 +- rs/bitcoin/adapter/src/blockchainstate.rs | 41 +- .../adapter/src/get_successors_handler.rs | 27 +- rs/bitcoin/adapter/src/header_cache.rs | 513 ++++++++++-------- rs/bitcoin/adapter/src/lib.rs | 16 +- 5 files changed, 317 insertions(+), 283 deletions(-) diff --git a/rs/bitcoin/adapter/src/blockchainmanager.rs b/rs/bitcoin/adapter/src/blockchainmanager.rs index 75e912e2cb2d..4f39cf4e471c 100644 --- a/rs/bitcoin/adapter/src/blockchainmanager.rs +++ b/rs/bitcoin/adapter/src/blockchainmanager.rs @@ -787,7 +787,8 @@ pub mod test { fn create_blockchain_manager( network: Network, ) -> (BlockHeader, BlockchainManager) { - let blockchain_state = BlockchainState::new(network, &MetricsRegistry::default()); + let blockchain_state = + BlockchainState::new(network, None, &MetricsRegistry::default(), no_op_logger()); ( blockchain_state.genesis(), BlockchainManager::new( diff --git a/rs/bitcoin/adapter/src/blockchainstate.rs b/rs/bitcoin/adapter/src/blockchainstate.rs index 362e0af72582..d9f6524c2191 100644 --- a/rs/bitcoin/adapter/src/blockchainstate.rs +++ b/rs/bitcoin/adapter/src/blockchainstate.rs @@ -2,10 +2,7 @@ //! use crate::{ common::{BlockHeight, BlockchainBlock, BlockchainHeader, BlockchainNetwork}, - header_cache::{ - AddHeaderError, AddHeaderResult, HeaderCache, HeaderNode, InMemoryHeaderCache, - LMDBHeaderCache, Tip, - }, + header_cache::{AddHeaderError, AddHeaderResult, HeaderNode, HybridHeaderCache, Tip}, metrics::BlockchainStateMetrics, }; use bitcoin::{BlockHash, block::Header as PureHeader, consensus::Encodable}; @@ -45,7 +42,7 @@ pub type SerializedBlock = Vec; /// The BlockChainState also maintains the child relationhips between the headers. pub struct BlockchainState { /// This field stores all the Bitcoin headers using a HashMap containing BlockHash and the corresponding header. - header_cache: Arc + Send>, + pub(crate) header_cache: Arc>, /// This field stores a hashmap containing BlockHash and the corresponding SerializedBlock. block_cache: RwLock>>, @@ -59,28 +56,15 @@ impl BlockchainState where Network::Header: Send + Sync, { - /// Create a new BlockChainState object with in-memory cache. - pub fn new(network: Network, metrics_registry: &MetricsRegistry) -> Self { - let genesis_block_header = network.genesis_block_header(); - let header_cache = Arc::new(InMemoryHeaderCache::new(genesis_block_header)); - let block_cache = RwLock::new(HashMap::new()); - BlockchainState { - header_cache, - block_cache, - network, - metrics: BlockchainStateMetrics::new(metrics_registry), - } - } - - /// Create a new BlockChainState with on-disk cache. - pub fn new_with_cache_dir( + /// Create a new BlockChainState with an optional on-disk cache if cache_dir is specified. + pub fn new( network: Network, - cache_dir: PathBuf, + cache_dir: Option, metrics_registry: &MetricsRegistry, logger: ReplicaLogger, ) -> Self { let genesis_block_header = network.genesis_block_header(); - let header_cache = Arc::new(LMDBHeaderCache::new( + let header_cache = Arc::new(HybridHeaderCache::new( genesis_block_header, cache_dir, logger, @@ -324,7 +308,12 @@ mod test { use std::collections::HashSet; fn run_in_memory(network: Network, test_fn: impl Fn(BlockchainState) -> R) -> R { - test_fn(BlockchainState::new(network, &MetricsRegistry::default())) + test_fn(BlockchainState::new( + network, + None, + &MetricsRegistry::default(), + no_op_logger(), + )) } fn run_with_cache_dir( @@ -332,9 +321,9 @@ mod test { test_fn: impl Fn(BlockchainState) -> R, ) -> R { let dir = tempdir().unwrap(); - test_fn(BlockchainState::new_with_cache_dir( + test_fn(BlockchainState::new( network, - dir.path().to_path_buf(), + Some(dir.path().to_path_buf()), &MetricsRegistry::default(), no_op_logger(), )) @@ -492,7 +481,7 @@ mod test { "unsuccessfully added fork chain: {maybe_err:?}" ); - let mut tips = state.header_cache.get_tips(); + let mut tips = crate::header_cache::test::get_tips(&state.header_cache); tips.sort_by(|x, y| y.work.cmp(&x.work)); assert_eq!(tips.len(), 2); assert_eq!(tips[0].header.block_hash(), *last_fork_hash); diff --git a/rs/bitcoin/adapter/src/get_successors_handler.rs b/rs/bitcoin/adapter/src/get_successors_handler.rs index 350158b4bb3c..20aa810151e6 100644 --- a/rs/bitcoin/adapter/src/get_successors_handler.rs +++ b/rs/bitcoin/adapter/src/get_successors_handler.rs @@ -92,7 +92,7 @@ pub struct GetSuccessorsHandler { metrics: GetSuccessorMetrics, } -impl GetSuccessorsHandler { +impl GetSuccessorsHandler { /// Creates a GetSuccessorsHandler to be used to access the blockchain state /// inside of the adapter when a `GetSuccessorsRequest` is received. pub fn new( @@ -123,6 +123,10 @@ impl GetSuccessorsHandler { .processed_block_hashes .observe(request.processed_block_hashes.len() as f64); + // Spawn persist-to-disk task without waiting for it to finish + let cache = self.state.header_cache.clone(); + tokio::task::spawn_blocking(move || cache.persist_headers_below_anchor(request.anchor)); + let (blocks, next, obsolete_blocks) = { let anchor_height = self .state @@ -312,6 +316,7 @@ mod test { use super::*; use bitcoin::{Block, consensus::Decodable}; + use ic_logger::no_op_logger; use ic_metrics::MetricsRegistry; use tokio::sync::mpsc::channel; @@ -323,12 +328,18 @@ mod test { Block::consensus_decode(&mut (*serialized_block).as_slice()).unwrap() } + fn new_blockchain_state( + network: Network, + ) -> BlockchainState { + BlockchainState::new(network, None, &MetricsRegistry::default(), no_op_logger()) + } + /// This tests ensures that `BlockchainManager::get_successors(...)` will return relevant blocks /// with the next headers of many forks and enqueue missing block hashes. #[tokio::test] async fn test_get_successors() { let network = bitcoin::Network::Regtest; - let blockchain_state = BlockchainState::new(network, &MetricsRegistry::default()); + let blockchain_state = new_blockchain_state(network); let genesis = blockchain_state.genesis(); let genesis_hash = genesis.block_hash(); let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); @@ -440,7 +451,7 @@ mod test { #[tokio::test] async fn test_get_successors_wait_header_sync_regtest() { let network = bitcoin::Network::Regtest; - let blockchain_state = BlockchainState::new(network, &MetricsRegistry::default()); + let blockchain_state = new_blockchain_state(network); let genesis = blockchain_state.genesis(); let genesis_hash = genesis.block_hash(); let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); @@ -490,7 +501,7 @@ mod test { #[tokio::test] async fn test_get_successors_multiple_blocks() { let network = bitcoin::Network::Regtest; - let blockchain_state = BlockchainState::new(network, &MetricsRegistry::default()); + let blockchain_state = new_blockchain_state(network); let genesis = blockchain_state.genesis(); let genesis_hash = genesis.block_hash(); let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); @@ -564,7 +575,7 @@ mod test { #[tokio::test] async fn test_get_successors_max_num_blocks() { let network = bitcoin::Network::Regtest; - let blockchain_state = BlockchainState::new(network, &MetricsRegistry::default()); + let blockchain_state = new_blockchain_state(network); let genesis = blockchain_state.genesis(); let genesis_hash = genesis.block_hash(); let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); @@ -600,7 +611,7 @@ mod test { #[tokio::test] async fn test_get_successors_multiple_blocks_out_of_order() { let network = bitcoin::Network::Regtest; - let blockchain_state = BlockchainState::new(network, &MetricsRegistry::default()); + let blockchain_state = new_blockchain_state(network); let genesis = blockchain_state.genesis(); let genesis_hash = genesis.block_hash(); let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); @@ -694,7 +705,7 @@ mod test { #[tokio::test] async fn test_get_successors_large_block() { let network = bitcoin::Network::Regtest; - let blockchain_state = BlockchainState::new(network, &MetricsRegistry::default()); + let blockchain_state = new_blockchain_state(network); let genesis = blockchain_state.genesis(); let genesis_hash = genesis.block_hash(); let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); @@ -760,7 +771,7 @@ mod test { #[tokio::test] async fn test_get_successors_many_blocks_until_size_cap_is_met() { let network = bitcoin::Network::Regtest; - let blockchain_state = BlockchainState::new(network, &MetricsRegistry::default()); + let blockchain_state = new_blockchain_state(network); let genesis = blockchain_state.genesis(); let genesis_hash = genesis.block_hash(); let (blockchain_manager_tx, _blockchain_manager_rx) = channel(10); diff --git a/rs/bitcoin/adapter/src/header_cache.rs b/rs/bitcoin/adapter/src/header_cache.rs index 5056b5aaa8dd..25b908a2d3c6 100644 --- a/rs/bitcoin/adapter/src/header_cache.rs +++ b/rs/bitcoin/adapter/src/header_cache.rs @@ -10,12 +10,12 @@ use bitcoin::{ use ic_btc_validation::ValidateHeaderError; use ic_logger::{ReplicaLogger, error}; use lmdb::{ - Cursor, Database, DatabaseFlags, Environment, EnvironmentFlags, RoTransaction, RwTransaction, + Database, DatabaseFlags, Environment, EnvironmentFlags, RoTransaction, RwTransaction, Transaction, WriteFlags, }; use std::collections::HashMap; use std::path::{Path, PathBuf}; -use std::sync::RwLock; +use std::sync::{Arc, RwLock}; use thiserror::Error; /// The max size (in bytes) of a LMDB cache, also know as the LMDB map @@ -78,6 +78,22 @@ impl Decodable for HeaderData
{ } } +impl Encodable for HeaderNode
{ + fn consensus_encode(&self, writer: &mut W) -> Result { + let data_size = self.data.consensus_encode(writer)?; + let children_size = self.children.consensus_encode(writer)?; + Ok(data_size + children_size) + } +} + +impl Decodable for HeaderNode
{ + fn consensus_decode(reader: &mut R) -> Result { + let data = >::consensus_decode(reader)?; + let children = >::consensus_decode(reader)?; + Ok(Self { data, children }) + } +} + /// The result when `add_header(...)` is called. #[derive(Debug)] pub enum AddHeaderResult { @@ -101,9 +117,6 @@ pub enum AddHeaderError { } pub struct InMemoryHeaderCache
{ - /// The starting point of the blockchain. - genesis: PureHeader, - /// Maps block hash to [HeaderNode]. cache: HashMap>, @@ -114,9 +127,6 @@ pub struct InMemoryHeaderCache
{ pub trait HeaderCache: Send + Sync { type Header; - /// Return the genesis header. - fn get_genesis(&self) -> PureHeader; - /// Return the header for the given block hash. fn get_header(&self, hash: BlockHash) -> Option>; @@ -133,41 +143,27 @@ pub trait HeaderCache: Send + Sync { /// Return the number of tips. fn get_num_tips(&self) -> usize; - /// Return all tips. - fn get_tips(&self) -> Vec>; + /// Return all ancestor headers of the given anchor from the cache, including the anchor itself. + /// The return result is ordered by child first. + fn get_ancestors(&self, anchor: BlockHash) -> Vec<(BlockHash, HeaderNode)>; + + /// Prune headers below the anchor_height. + fn prune_headers_below_height(&self, anchor_height: BlockHeight); } impl InMemoryHeaderCache
{ /// Creates a new cache with a genesis header - pub fn new(genesis: Header) -> RwLock { - let tips = vec![Tip { - header: genesis.clone(), - height: 0, - work: genesis.work(), - }]; - let data = HeaderData { - header: genesis.clone(), - height: 0, - work: genesis.work(), - }; + pub fn new_with_anchor(anchor: Tip
) -> Self { + let tips = vec![anchor.clone()]; let mut cache = HashMap::new(); - cache.insert(genesis.block_hash(), data.into()); - - RwLock::new(InMemoryHeaderCache { - genesis: genesis.into_pure_header(), - cache, - tips, - }) + cache.insert(anchor.header.block_hash(), anchor.into()); + InMemoryHeaderCache { cache, tips } } } impl HeaderCache for RwLock> { type Header = Header; - fn get_genesis(&self) -> PureHeader { - self.read().unwrap().genesis - } - fn get_header(&self, hash: BlockHash) -> Option> { self.read().unwrap().cache.get(&hash).cloned() } @@ -189,6 +185,7 @@ impl HeaderCache for RwLock HeaderCache for RwLock Vec> { - self.read().unwrap().tips.clone() + fn get_ancestors(&self, anchor: BlockHash) -> Vec<(BlockHash, HeaderNode
)> { + let mut hash = anchor; + let mut to_persist = Vec::new(); + let mut next_hash = None; + while let Some(mut node) = self.get_header(hash) { + // Make sure the returned anchor node has no child, and others have a single child + node.children = next_hash.iter().copied().collect::>(); + let prev_hash = node.data.header.prev_block_hash(); + to_persist.push((hash, node)); + next_hash = Some(hash); + hash = prev_hash; + } + to_persist + } + + fn prune_headers_below_height(&self, anchor_height: BlockHeight) { + println!("prune below height {anchor_height}"); + let mut this = self.write().unwrap(); + this.cache + .retain(|_, node| node.data.height >= anchor_height); + this.tips.retain(|tip| tip.height >= anchor_height); } } @@ -237,8 +253,7 @@ fn create_db_env(path: &Path) -> Environment { let builder_flags = EnvironmentFlags::NO_TLS; let permission = 0o644; builder.set_flags(builder_flags); - // 3 databases: 1 for headers, 1 for tips, 1 for parent-child relation. - builder.set_max_dbs(3); + builder.set_max_dbs(1); builder.set_map_size(MAX_LMDB_CACHE_SIZE); builder .open_with_permissions(path, permission) @@ -258,6 +273,8 @@ pub enum LMDBCacheError { Decode(#[from] encode::Error), #[error("Encoding error {0}")] Encode(#[from] io::Error), + #[error("JoinError")] + TaskJoin(#[from] tokio::task::JoinError), } impl From for LMDBCacheError { @@ -266,16 +283,6 @@ impl From for LMDBCacheError { } } -/// Macro that logs the error when result is not Ok. -macro_rules! log_err { - ($r:expr, $log:expr, $reason:expr) => { - $r.map_err(|err| { - error!($log, "Error in DB operation {}: {:?}", $reason, err); - err - }) - }; -} - /// Like log_err, but won't log the error if it matches the given error code. macro_rules! log_err_except { ($r:expr, $log:expr, $code:pat, $reason:expr) => { @@ -287,21 +294,16 @@ macro_rules! log_err_except { }; } -pub struct LMDBHeaderCache
{ - genesis: Header, +pub struct LMDBHeaderCache { + log: ReplicaLogger, db_env: Environment, - // Map BlockHash to HeaderData. + // Map BlockHash to HeaderData headers: Database, - // Map BlockHash to Tip. - tips: Database, - // Map parent's BlockHash to child's BlockHash, duplicated key (parent's hash) allowed. - children: Database, - log: ReplicaLogger, } -impl LMDBHeaderCache
{ +impl LMDBHeaderCache { /// Load the cache with a genesis header and cache directory. - pub fn new(genesis: Header, mut cache_dir: PathBuf, log: ReplicaLogger) -> Self { + pub fn new(mut cache_dir: PathBuf, log: ReplicaLogger) -> Self { cache_dir.push("headers"); let path = cache_dir.as_path(); std::fs::create_dir_all(path).unwrap_or_else(|err| { @@ -311,134 +313,64 @@ impl LMDBHeaderCache
{ let headers = db_env .create_db(Some("HEADERS"), DatabaseFlags::empty()) .unwrap_or_else(|err| panic!("Error creating db for metadata: {:?}", err)); - let tips = db_env - .create_db(Some("TIPS"), DatabaseFlags::empty()) - .unwrap_or_else(|err| panic!("Error creating db for metadata: {:?}", err)); - let children = db_env - .create_db(Some("CHILDREN"), DatabaseFlags::DUP_SORT) - .unwrap_or_else(|err| panic!("Error creating db for metadata: {:?}", err)); - - let cache = LMDBHeaderCache { - genesis: genesis.clone(), + LMDBHeaderCache { + log, db_env, headers, - tips, - children, - log, - }; - - cache - .run_rw_txn(|tx| cache.tx_add_header(tx, None, genesis.block_hash(), genesis)) - .unwrap(); - - cache - } - - fn tx_is_tip( - &self, - tx: &mut Tx, - block_hash: BlockHash, - ) -> Result { - match tx.get(self.tips, &block_hash) { - Ok(_) => Ok(true), - Err(lmdb::Error::NotFound) => Ok(false), - Err(err) => Err(err.into()), } } - fn tx_get_tips( - &self, - tx: &mut Tx, - ) -> Result>, LMDBCacheError> { - let mut cursor = tx.open_ro_cursor(self.tips)?; - cursor - .iter_start() - .map(|row| { - let (_, mut value) = row?; - let tip = >::consensus_decode(&mut value)?; - Ok(tip) - }) - .collect::>() - } - - fn tx_get_num_tips(&self, tx: &mut Tx) -> Result { - let mut cursor = tx.open_ro_cursor(self.tips)?; - Ok(cursor.iter_start().count()) - } - - fn tx_get_children( - &self, - tx: &mut Tx, - hash: BlockHash, - ) -> Result, LMDBCacheError> { - let mut cursor = tx.open_ro_cursor(self.children)?; - let children = cursor - .iter_dup_of(hash) - .map(|row| { - let (_key, mut value) = row?; - let child_hash = BlockHash::consensus_decode(&mut value)?; - Ok(child_hash) - }) - .collect::, LMDBCacheError>>()?; - Ok(children) - } - - fn tx_get_header( + fn tx_get_header( &self, - tx: &mut Tx, + tx: &Tx, hash: BlockHash, ) -> Result, LMDBCacheError> { let mut bytes = tx.get(self.headers, &hash)?; - let data = >::consensus_decode(&mut bytes)?; - let children = self.tx_get_children(tx, hash)?; - Ok(HeaderNode { data, children }) + let node = >::consensus_decode(&mut bytes)?; + Ok(node) } - fn tx_add_child( + fn tx_add_header( &self, tx: &mut RwTransaction, - prev_hash: BlockHash, block_hash: BlockHash, + node: HeaderNode
, ) -> Result<(), LMDBCacheError> { - Ok(tx.put(self.children, &prev_hash, &block_hash, WriteFlags::empty())?) + let mut bytes = Vec::new(); + node.consensus_encode(&mut bytes)?; + tx.put(self.headers, &block_hash, &bytes, WriteFlags::empty())?; + Ok(()) + } + + fn tx_get_tip_hash(&self, tx: &Tx) -> Result { + let mut bytes = tx.get(self.headers, b"TIP")?; + let hash = ::consensus_decode(&mut bytes)?; + Ok(hash) + } + + fn tx_get_tip( + &self, + tx: &Tx, + ) -> Result, LMDBCacheError> { + let hash = self.tx_get_tip_hash(tx)?; + self.tx_get_header(tx, hash).map(|node| node.data) } - fn tx_add_header( + fn tx_update_tip( &self, tx: &mut RwTransaction, - prev_node: Option<&HeaderNode
>, - block_hash: BlockHash, - header: Header, - ) -> Result { - let tip = Tip { - header: header.clone(), - height: prev_node.map(|p| p.data.height + 1).unwrap_or_default(), - work: prev_node - .map(|p| p.data.work + header.work()) - .unwrap_or(header.work()), - }; - let mut bytes = Vec::new(); - tip.consensus_encode(&mut bytes)?; - tx.put(self.headers, &block_hash, &bytes, WriteFlags::empty())?; - tx.put(self.tips, &block_hash, &bytes, WriteFlags::empty())?; - - if let Some(prev_node) = prev_node { - let prev_hash = prev_node.data.header.block_hash(); - self.tx_add_child(tx, prev_hash, block_hash)?; - // If the previous header already exists in `tips`, then remove it - if self.tx_is_tip(tx, prev_hash)? { - tx.del(self.tips, &prev_hash, None)?; - } - } - Ok(AddHeaderResult::HeaderAdded(block_hash)) + tip_hash: BlockHash, + ) -> Result<(), LMDBCacheError> { + tx.put(self.headers, b"TIP", &tip_hash, WriteFlags::empty())?; + Ok(()) } fn run_ro_txn<'a, R, F>(&'a self, f: F) -> Result where - F: FnOnce(&mut RoTransaction<'a>) -> Result, + F: FnOnce(&RoTransaction<'a>) -> Result, { - let mut tx = self.db_env.begin_ro_txn()?; - let result = f(&mut tx)?; + let tx = self.db_env.begin_ro_txn()?; + let result = f(&tx)?; tx.commit()?; Ok(result) } @@ -454,83 +386,145 @@ impl LMDBHeaderCache
{ } } -impl HeaderCache for LMDBHeaderCache
{ - type Header = Header; +pub struct HybridHeaderCache
{ + in_memory: RwLock>, + on_disk: Option>, + genesis_hash: BlockHash, +} - fn get_genesis(&self) -> PureHeader { - self.genesis.clone().into_pure_header() +impl HybridHeaderCache
{ + pub fn new(genesis: Header, cache_dir: Option, log: ReplicaLogger) -> Self { + let genesis_hash = genesis.block_hash(); + let on_disk = cache_dir.map(|dir| Arc::new(LMDBHeaderCache::new(dir, log))); + // Try reading the anchor (tip of the chain) from disk. + // If it doesn't exist, use genesis header. + let anchor = on_disk + .as_ref() + .and_then(|cache| { + log_err_except!( + cache.run_ro_txn(|tx| cache.tx_get_tip(tx)), + cache.log, + LMDBCacheError::Lmdb(lmdb::Error::NotFound), + "tx_get_tip()" + ) + }) + .unwrap_or_else(|| HeaderData { + header: genesis.clone(), + height: 0, + work: genesis.work(), + }); + let in_memory = RwLock::new(InMemoryHeaderCache::new_with_anchor(anchor)); + Self { + in_memory, + on_disk, + genesis_hash, + } } +} - fn get_header(&self, hash: BlockHash) -> Option> { - log_err_except!( - self.run_ro_txn(|tx| self.tx_get_header(tx, hash)), - self.log, - LMDBCacheError::Lmdb(lmdb::Error::NotFound), - format!("tx_get_header({hash})") - ) +impl HybridHeaderCache
{ + /// Get the genesis header. + pub fn get_genesis(&self) -> PureHeader { + // The unwrap below is safe because: + // - if the on-disk cache is configured and has persisted data, it should + // contain the genesis header; + // - otherwise the in-memory cache hasn't been pruned, and should contain + // the genesis header. + self.get_header(self.genesis_hash) + .unwrap() + .data + .header + .into_pure_header() } - fn add_header( + fn get_header_on_disk(&self, hash: BlockHash) -> Option> { + self.on_disk.as_ref().and_then(|cache| { + log_err_except!( + cache.run_ro_txn(|tx| cache.tx_get_header(tx, hash)), + cache.log, + LMDBCacheError::Lmdb(lmdb::Error::NotFound), + format!("tx_get_header({hash})") + ) + }) + } + + /// Get a header by hash. + pub fn get_header(&self, hash: BlockHash) -> Option> { + self.in_memory + .get_header(hash) + .or_else(|| self.get_header_on_disk(hash)) + } + + /// Add a new header. + pub fn add_header( &self, block_hash: BlockHash, header: Header, ) -> Result { - let prev_hash = header.prev_block_hash(); - let prev_node = self - .get_header(prev_hash) - .ok_or(AddHeaderError::PrevHeaderNotCached(prev_hash))?; + self.in_memory.add_header(block_hash, header) + } - log_err!( - self.run_rw_txn(move |tx| self.tx_add_header(tx, Some(&prev_node), block_hash, header)), - self.log, - format!("tx_add_header({block_hash})") - ) - .map_err(|err| AddHeaderError::Internal(format!("{err:?}"))) + /// Returns the tip header with the highest cumulative work. + pub fn get_active_chain_tip(&self) -> Tip
{ + self.in_memory.get_active_chain_tip() } - /// This method returns the tip header with the highest cumulative work. - fn get_active_chain_tip(&self) -> Tip
{ - self.get_tips() - .into_iter() - .max_by(|x, y| x.work.cmp(&y.work)) - .unwrap_or_else(|| panic!("Impossible: failed to find active_chain_tip")) + /// Returns the number of tip headers. + pub fn get_num_tips(&self) -> usize { + self.in_memory.get_num_tips() } - fn get_num_tips(&self) -> usize { - log_err!( - self.run_ro_txn(|tx| self.tx_get_num_tips(tx)), - self.log, - "tx_num_tips" - ) - .unwrap_or_else(|err| panic!("Failed to get_num_tips {:?}", err)) - } - - fn get_tips(&self) -> Vec> { - log_err!( - self.run_ro_txn(|tx| self.tx_get_tips(tx)), - self.log, - "tx_get_tips" - ) - .unwrap_or_else(|err| panic!("Failed to get tips {:?}", err)) + /// Persist headers below the anchor (as headers) and the anchor (as tip) on to disk, and + /// Prune headers below the anchor from the in-memory cache. + /// It is a no-op if the on-disk cache is not configured. + pub fn persist_headers_below_anchor(&self, anchor: BlockHash) -> Result<(), LMDBCacheError> { + if let Some(on_disk) = &self.on_disk { + let to_persist = self.in_memory.get_ancestors(anchor); + if let Some((hash, first)) = to_persist.first() { + let anchor_hash = *hash; + let anchor_height = first.data.height; + on_disk.run_rw_txn(|tx| { + for (hash, node) in to_persist { + on_disk.tx_add_header(tx, hash, node)?; + } + on_disk.tx_update_tip(tx, anchor_hash)?; + Ok(()) + })?; + self.in_memory.prune_headers_below_height(anchor_height); + } + } + Ok(()) } } #[cfg(test)] -mod test { +pub(crate) mod test { use super::*; use crate::BlockchainNetwork; use ic_btc_adapter_test_utils::generate_headers; use ic_test_utilities_logger::with_test_replica_logger; - use std::collections::BTreeSet; + use std::collections::{BTreeMap, BTreeSet}; use tempfile::tempdir; + /// Creates an empty new cache. + pub fn new_in_memory_cache_with_genesis( + genesis: Header, + ) -> InMemoryHeaderCache
{ + let data = HeaderData { + header: genesis.clone(), + height: 0, + work: genesis.work(), + }; + InMemoryHeaderCache::new_with_anchor(data) + } + #[test] fn test_in_memory_header_cache() { let network = bitcoin::Network::Bitcoin; let genesis_block_header = network.genesis_block_header(); let genesis_block_hash = genesis_block_header.block_hash(); { - let cache = >::new(genesis_block_header); + let cache = RwLock::new(new_in_memory_cache_with_genesis(genesis_block_header)); assert!(cache.get_header(genesis_block_hash).is_some()); let node = cache.get_header(genesis_block_hash).unwrap(); assert_eq!(node.data.height, 0); @@ -561,32 +555,40 @@ mod test { } } - fn get_tips_of( - cache: &impl HeaderCache
, + // get all tips whose ancestors include given block hash. + fn get_tips_of( + cache: &HybridHeaderCache
, block_hash: BlockHash, - ) -> BTreeSet> { - let mut set = BTreeSet::new(); + ) -> Vec> { + let mut tips = Vec::new(); let node = cache.get_header(block_hash).unwrap(); if node.children.is_empty() { - set.insert(node); + tips.push(node.data); } else { for hash in node.children { - set.append(&mut get_tips_of(cache, hash)) + tips.append(&mut get_tips_of(cache, hash)) } } - set + tips + } + + // get all tips that extends the node of the given block hash. + pub(crate) fn get_tips( + cache: &HybridHeaderCache
, + ) -> Vec> { + get_tips_of(cache, cache.get_genesis().block_hash()) } #[test] - fn test_lmdb_header_cache() { + fn test_hybrid_header_cache() { let dir = tempdir().unwrap(); let network = bitcoin::Network::Bitcoin; let genesis_block_header = network.genesis_block_header(); let genesis_block_hash = genesis_block_header.block_hash(); with_test_replica_logger(|logger| { - let cache = >::new( + let cache = >::new( genesis_block_header, - dir.path().to_path_buf(), + Some(dir.path().to_path_buf()), logger, ); assert!(cache.get_header(genesis_block_hash).is_some()); @@ -595,8 +597,8 @@ mod test { assert_eq!(node.data.header, genesis_block_header); assert_eq!(cache.get_active_chain_tip().header, genesis_block_header); - // Make a few new header - let mut next_headers = BTreeSet::new(); + // Make a few new headers + let mut next_headers = BTreeMap::new(); for i in 1..4 { for header in generate_headers(genesis_block_hash, genesis_block_header.time, i, &[]) @@ -604,41 +606,76 @@ mod test { cache.add_header(header.block_hash(), header).unwrap(); let next_node = cache.get_header(header.block_hash()).unwrap(); assert_eq!(next_node.data.header, header); - next_headers.insert(header); + next_headers.insert(header.block_hash(), header); + } + } + // Add more headers + let intermediate = cache.get_active_chain_tip(); + let intermediate_hash = intermediate.header.block_hash(); + for i in 1..4 { + for header in generate_headers(intermediate_hash, intermediate.header.time, i, &[]) + { + cache.add_header(header.block_hash(), header).unwrap(); + let next_node = cache.get_header(header.block_hash()).unwrap(); + assert_eq!(next_node.data.header, header); + next_headers.insert(header.block_hash(), header); } } let tip = cache.get_active_chain_tip(); - assert!(next_headers.contains(&tip.header)); + assert!(next_headers.contains_key(&tip.header.block_hash())); assert_eq!( next_headers - .iter() + .values() .map(|x| cache.get_header(x.block_hash()).unwrap().data.work) .max(), Some(tip.work) ); + assert!(cache.get_header(genesis_block_hash).is_some()); + let mut hash = tip.header.block_hash(); + while hash != genesis_block_hash { + let header = *next_headers.get(&hash).unwrap(); + assert_eq!(Some(header), cache.get_header(hash).map(|x| x.data.header)); + hash = header.prev_block_hash(); + } + let tips = get_tips(&cache); + assert_eq!(tips.len(), 5); + cache + .persist_headers_below_anchor(intermediate_hash) + .unwrap(); + // Check if the chain from genesis to tip can still be found + assert!(cache.get_header(genesis_block_hash).is_some()); + let mut hash = tip.header.block_hash(); + while hash != genesis_block_hash { + let header = *next_headers.get(&hash).unwrap(); + assert_eq!(Some(header), cache.get_header(hash).map(|x| x.data.header)); + hash = header.prev_block_hash(); + } + // Check if all tip ancestors can be found. + let tips = get_tips(&cache); + assert_eq!(tips.len(), 3); + for mut tip in tips { + while tip.header.block_hash() != genesis_block_hash { + let prev_hash = tip.header.prev_block_hash(); + let node = cache.get_header(prev_hash).unwrap(); + assert_eq!(node.data.height + 1, tip.height); + assert!(node.children.contains(&tip.header.block_hash())); + tip = node.data; + } + } }); - // Re-open the cache and check to see if data still exists + // Re-open the cache and check to see if headers were persisted with_test_replica_logger(|logger| { - let cache = >::new( + let cache = >::new( genesis_block_header, - dir.path().to_path_buf(), + Some(dir.path().to_path_buf()), logger, ); assert!(cache.get_header(genesis_block_hash).is_some()); - let node = cache.get_header(genesis_block_hash).unwrap(); - assert_eq!(node.data.height, 0); - assert_eq!(node.data.header, genesis_block_header); - assert_eq!(node.children.len(), 3); - let tips = get_tips_of(&cache, genesis_block_hash) - .iter() - .map(|node| node.data.header.block_hash()) - .collect::>(); - let tip = cache.get_active_chain_tip(); - assert!(tips.contains(&tip.header.block_hash())); - for hash in tips.iter() { - assert!(cache.get_header(*hash).is_some()); - } + let tips = get_tips_of(&cache, genesis_block_hash); + assert_eq!(tips.len(), 1); + assert_eq!(tips[0], cache.get_active_chain_tip()); + assert_eq!(tips[0].height, 3); }); } } diff --git a/rs/bitcoin/adapter/src/lib.rs b/rs/bitcoin/adapter/src/lib.rs index 08cd571d9c53..53757f8e40a3 100644 --- a/rs/bitcoin/adapter/src/lib.rs +++ b/rs/bitcoin/adapter/src/lib.rs @@ -205,16 +205,12 @@ async fn start_server_helper( { let (adapter_state, tx) = AdapterState::new(config.idle_seconds); let (blockchain_manager_tx, blockchain_manager_rx) = channel(100); - let blockchain_state = if let Some(cache_dir) = &config.cache_dir { - BlockchainState::new_with_cache_dir( - config.network, - cache_dir.clone(), - &metrics_registry, - log.clone(), - ) - } else { - BlockchainState::new(config.network, &metrics_registry) - }; + let blockchain_state = BlockchainState::new( + config.network, + config.cache_dir.clone(), + &metrics_registry, + log.clone(), + ); let blockchain_state = Arc::new(blockchain_state); let (transaction_manager_tx, transaction_manager_rx) = channel(100); From af97c61fdd549fc3d9420f1ac2bcc1c8846ee9c8 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Wed, 17 Sep 2025 07:25:59 -0700 Subject: [PATCH 12/20] Minor --- rs/bitcoin/adapter/src/get_successors_handler.rs | 4 +++- rs/bitcoin/adapter/src/header_cache.rs | 13 ++++++++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/rs/bitcoin/adapter/src/get_successors_handler.rs b/rs/bitcoin/adapter/src/get_successors_handler.rs index 20aa810151e6..1cc93be5b271 100644 --- a/rs/bitcoin/adapter/src/get_successors_handler.rs +++ b/rs/bitcoin/adapter/src/get_successors_handler.rs @@ -125,7 +125,9 @@ impl GetSuccessorsHandler { // Spawn persist-to-disk task without waiting for it to finish let cache = self.state.header_cache.clone(); - tokio::task::spawn_blocking(move || cache.persist_headers_below_anchor(request.anchor)); + tokio::task::spawn_blocking(move || { + cache.persist_and_prune_headers_below_anchor(request.anchor) + }); let (blocks, next, obsolete_blocks) = { let anchor_height = self diff --git a/rs/bitcoin/adapter/src/header_cache.rs b/rs/bitcoin/adapter/src/header_cache.rs index 2c44aa4303c7..1dad813aaa3c 100644 --- a/rs/bitcoin/adapter/src/header_cache.rs +++ b/rs/bitcoin/adapter/src/header_cache.rs @@ -14,7 +14,7 @@ use lmdb::{ }; use std::collections::HashMap; use std::path::{Path, PathBuf}; -use std::sync::{Arc, RwLock}; +use std::sync::RwLock; use thiserror::Error; /// The max size (in bytes) of a LMDB cache, also know as the LMDB map @@ -384,14 +384,14 @@ impl LMDBHeaderCache { pub struct HybridHeaderCache
{ in_memory: RwLock>, - on_disk: Option>, + on_disk: Option, genesis_hash: BlockHash, } impl HybridHeaderCache
{ pub fn new(genesis: Header, cache_dir: Option, log: ReplicaLogger) -> Self { let genesis_hash = genesis.block_hash(); - let on_disk = cache_dir.map(|dir| Arc::new(LMDBHeaderCache::new(dir, log))); + let on_disk = cache_dir.map(|dir| LMDBHeaderCache::new(dir, log)); // Try reading the anchor (tip of the chain) from disk. // If it doesn't exist, use genesis header. let anchor = on_disk @@ -473,7 +473,10 @@ impl HybridHeaderCache
/// Persist headers below the anchor (as headers) and the anchor (as tip) on to disk, and /// Prune headers below the anchor from the in-memory cache. /// It is a no-op if the on-disk cache is not configured. - pub fn persist_headers_below_anchor(&self, anchor: BlockHash) -> Result<(), LMDBCacheError> { + pub fn persist_and_prune_headers_below_anchor( + &self, + anchor: BlockHash, + ) -> Result<(), LMDBCacheError> { if let Some(on_disk) = &self.on_disk { let to_persist = self.in_memory.get_ancestors(anchor); if let Some((hash, first)) = to_persist.first() { @@ -636,7 +639,7 @@ pub(crate) mod test { let tips = get_tips(&cache); assert_eq!(tips.len(), 5); cache - .persist_headers_below_anchor(intermediate_hash) + .persist_and_prune_headers_below_anchor(intermediate_hash) .unwrap(); // Check if the chain from genesis to tip can still be found assert!(cache.get_header(genesis_block_hash).is_some()); From 080219aeedf866fa7986b08cd03fd303bedb4d17 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Wed, 17 Sep 2025 07:32:56 -0700 Subject: [PATCH 13/20] Fix benches --- rs/bitcoin/adapter/benches/e2e.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/rs/bitcoin/adapter/benches/e2e.rs b/rs/bitcoin/adapter/benches/e2e.rs index 370d8b9960e0..596b450a6d7d 100644 --- a/rs/bitcoin/adapter/benches/e2e.rs +++ b/rs/bitcoin/adapter/benches/e2e.rs @@ -189,8 +189,12 @@ fn add_800k_block_headers(criterion: &mut Criterion) { group.bench_function("add_headers", |bench| { let rt = tokio::runtime::Runtime::new().unwrap(); bench.iter(|| { - let blockchain_state = - BlockchainState::new(Network::Bitcoin, &MetricsRegistry::default()); + let blockchain_state = BlockchainState::new( + Network::Bitcoin, + None, + &MetricsRegistry::default(), + no_op_logger(), + ); // Headers are processed in chunks of at most MAX_HEADERS_SIZE entries for chunk in bitcoin_headers_to_add.chunks(MAX_HEADERS_SIZE) { let (added_headers, error) = From 2d0f63d4293c377f9eedfd3ededee9ff21ab67f5 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Wed, 17 Sep 2025 07:40:58 -0700 Subject: [PATCH 14/20] Add reviewer comments --- rs/bitcoin/adapter/src/addressbook.rs | 51 +++++++++++++++------ rs/bitcoin/adapter/src/blockchainstate.rs | 2 + rs/bitcoin/adapter/src/connectionmanager.rs | 2 +- 3 files changed, 39 insertions(+), 16 deletions(-) diff --git a/rs/bitcoin/adapter/src/addressbook.rs b/rs/bitcoin/adapter/src/addressbook.rs index 57c5cfeb27eb..a8a2115216b8 100644 --- a/rs/bitcoin/adapter/src/addressbook.rs +++ b/rs/bitcoin/adapter/src/addressbook.rs @@ -251,7 +251,7 @@ impl AddressBook { /// This function retrieves the next seed address from the seed queue. /// If no seeds are found, the seed queue is rebuilt from the DNS seeds. - pub async fn pop_seed(&mut self) -> AddressBookResult { + pub async fn resolve_next_seed(&mut self) -> AddressBookResult { if self.seed_queue.is_empty() { self.build_seed_queue().await?; } @@ -381,7 +381,10 @@ mod test { .build(); let mut book = AddressBook::new(&config, no_op_logger()); - let seed = book.pop_seed().await.expect("there should be 1 seed"); + let seed = book + .resolve_next_seed() + .await + .expect("there should be 1 seed"); let socket_1 = SocketAddr::from_str("127.0.0.1:8444").expect("bad address format"); let address_1 = Address::new(&socket_1, ServiceFlags::NETWORK); @@ -406,7 +409,10 @@ mod test { .build(); let mut book = AddressBook::new(&config, no_op_logger()); - let seed = book.pop_seed().await.expect("there should be 1 seed"); + let seed = book + .resolve_next_seed() + .await + .expect("there should be 1 seed"); let socket_1 = SocketAddr::from_str("127.0.0.1:8444").expect("bad address format"); let address_1 = Address::new(&socket_1, ServiceFlags::NETWORK); @@ -430,7 +436,10 @@ mod test { .with_dns_seeds(vec![String::from("127.0.0.1"), String::from("192.168.1.1")]) .build(); let mut book = AddressBook::new(&config, no_op_logger()); - let seed = book.pop_seed().await.expect("there should be 1 seed"); + let seed = book + .resolve_next_seed() + .await + .expect("there should be 1 seed"); let socket = SocketAddr::from_str("127.0.0.1:8444").expect("bad address format"); let address = Address::new(&socket, ServiceFlags::NETWORK); @@ -473,31 +482,37 @@ mod test { assert_eq!(book.active_addresses.len(), 0); } - /// This function ensures that the [AddressBook::pop_seed](AddressBook::pop_seed) method + /// This function ensures that the [AddressBook::resolve_next_seed](AddressBook::resolve_next_seed) method /// gives the next address in queue but also pushes it to the back of the queue. #[tokio::test] - async fn test_pop_seed() { + async fn test_resolve_next_seed() { let config = ConfigBuilder::default_with(Network::Signet) .with_dns_seeds(vec![String::from("127.0.0.1"), String::from("192.168.1.1")]) .build(); let mut book = AddressBook::new(&config, no_op_logger()); - book.pop_seed().await.expect("there should be 1 seed"); + book.resolve_next_seed() + .await + .expect("there should be 1 seed"); assert_eq!(book.seed_queue.len(), 1); - book.pop_seed().await.expect("there should be 1 seed"); + book.resolve_next_seed() + .await + .expect("there should be 1 seed"); assert_eq!(book.seed_queue.len(), 0); - book.pop_seed() + book.resolve_next_seed() .await - .expect("pop_seed should rebuild seed queue"); + .expect("resolve_next_seed should rebuild seed queue"); assert_eq!(book.seed_queue.len(), 1); // Remove the remaining seed address and then empty the dns_seeds. - book.pop_seed().await.expect("there should be 1 seed"); + book.resolve_next_seed() + .await + .expect("there should be 1 seed"); book.dns_seeds.clear(); - // `pop_seed` should now cause the AddressBookError::NoSeedAddressesFound error. + // `resolve_next_seed` should now cause the AddressBookError::NoSeedAddressesFound error. assert!(matches!( - book.pop_seed().await, + book.resolve_next_seed().await, Err(AddressBookError::NoSeedAddressesFound) )); } @@ -517,7 +532,10 @@ mod test { assert_eq!(book.seed_queue.len(), 0); book.build_seed_queue().await.unwrap(); assert_eq!(book.seed_queue.len(), 1); - let addr_entry = book.pop_seed().await.expect("there should be 1 seed"); + let addr_entry = book + .resolve_next_seed() + .await + .expect("there should be 1 seed"); assert!(addr_entry.addr().is_ipv6()); } @@ -530,7 +548,10 @@ mod test { .with_dns_seeds(vec![String::from("127.0.0.1"), String::from("192.168.1.1")]) .build(); let mut book = AddressBook::new(&config, no_op_logger()); - let seed = book.pop_seed().await.expect("there should be 1 seed"); + let seed = book + .resolve_next_seed() + .await + .expect("there should be 1 seed"); let socket = SocketAddr::from_str("127.0.0.1:8444").expect("bad address format"); let address = Address::new( &socket, diff --git a/rs/bitcoin/adapter/src/blockchainstate.rs b/rs/bitcoin/adapter/src/blockchainstate.rs index e190cc070e1e..f66ebc465d2b 100644 --- a/rs/bitcoin/adapter/src/blockchainstate.rs +++ b/rs/bitcoin/adapter/src/blockchainstate.rs @@ -247,6 +247,8 @@ where .consensus_encode(&mut serialized_block) .map_err(|e| AddBlockError::CouldNotSerialize(block_hash, e.to_string()))?; + // The unwrap below would only fail when the RwLock is poisoned, which will + // not happen here because the use of the lock is never nested. self.block_cache .write() .unwrap() diff --git a/rs/bitcoin/adapter/src/connectionmanager.rs b/rs/bitcoin/adapter/src/connectionmanager.rs index bcb99be5246c..b1f24277cb23 100644 --- a/rs/bitcoin/adapter/src/connectionmanager.rs +++ b/rs/bitcoin/adapter/src/connectionmanager.rs @@ -304,7 +304,7 @@ impl ConnectionManager { ) -> ConnectionManagerResult<()> { self.metrics.connections.inc(); let address_entry_result = if !self.address_book.has_enough_addresses() { - self.address_book.pop_seed().await + self.address_book.resolve_next_seed().await } else { self.address_book.pop() }; From 90a8cc5e0604d5d8544d3384515e2393df8e8dea Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Wed, 17 Sep 2025 08:18:10 -0700 Subject: [PATCH 15/20] Improve comments --- rs/bitcoin/adapter/src/header_cache.rs | 33 +++++++++++++++++++------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/rs/bitcoin/adapter/src/header_cache.rs b/rs/bitcoin/adapter/src/header_cache.rs index 1dad813aaa3c..f6941695ec14 100644 --- a/rs/bitcoin/adapter/src/header_cache.rs +++ b/rs/bitcoin/adapter/src/header_cache.rs @@ -139,9 +139,10 @@ pub trait HeaderCache: Send + Sync { /// Return the number of tips. fn get_num_tips(&self) -> usize; - /// Return all ancestor headers of the given anchor from the cache, including the anchor itself. - /// The return result is ordered by child first. - fn get_ancestors(&self, anchor: BlockHash) -> Vec<(BlockHash, HeaderNode)>; + /// Return the ancestor from the given block hash to the current anchor in the + /// in-memory cache as a chain of headers, where each element is the only child + /// of the next, and the first element (tip) has no child. + fn get_ancestor_chain(&self, from: BlockHash) -> Vec<(BlockHash, HeaderNode)>; /// Prune headers below the anchor_height. fn prune_headers_below_height(&self, anchor_height: BlockHeight); @@ -220,13 +221,13 @@ impl HeaderCache for RwLock Vec<(BlockHash, HeaderNode
)> { - let mut hash = anchor; + fn get_ancestor_chain(&self, from: BlockHash) -> Vec<(BlockHash, HeaderNode
)> { + let mut hash = from; let mut to_persist = Vec::new(); let mut next_hash = None; while let Some(mut node) = self.get_header(hash) { - // Make sure the returned anchor node has no child, and others have a single child - node.children = next_hash.iter().copied().collect::>(); + // The tip in the returned chain will have no child, and the rest have a single child. + node.children = next_hash.into_iter().collect::>(); let prev_hash = node.data.header.prev_block_hash(); to_persist.push((hash, node)); next_hash = Some(hash); @@ -382,6 +383,20 @@ impl LMDBHeaderCache { } } +/// A 2-tier header cache consisting of an in-memory cache, and optionally +/// an on-disk cache. It maintains the following invariants: +/// +/// 1. The on-disk cache contains headers from genesis to the header at an anchor point. +/// +/// 2. The in-memory cache contains headers from the anchor to latest. +/// +/// 3. The on-disk headers form a single list, where the anchor is the tip and has no child. +/// +/// 4. The two caches would overlap only at the anchor header, in which case +/// [get_header] would always return the header stored at in-memory cache. +/// +/// It would behave as only an in-memory cache when on-disk cache is not enabled, +/// and pruning operation would be a no-op. pub struct HybridHeaderCache
{ in_memory: RwLock>, on_disk: Option, @@ -425,7 +440,7 @@ impl HybridHeaderCache
// - if the on-disk cache is configured and has persisted data, it should // contain the genesis header; // - otherwise the in-memory cache hasn't been pruned, and should contain - // the genesis header. + // the genesis header. self.get_header(self.genesis_hash) .unwrap() .data @@ -478,7 +493,7 @@ impl HybridHeaderCache
anchor: BlockHash, ) -> Result<(), LMDBCacheError> { if let Some(on_disk) = &self.on_disk { - let to_persist = self.in_memory.get_ancestors(anchor); + let to_persist = self.in_memory.get_ancestor_chain(anchor); if let Some((hash, first)) = to_persist.first() { let anchor_hash = *hash; let anchor_height = first.data.height; From 1ba634f5f02a4d371afad58b35c1bb75c06bcdfd Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Wed, 17 Sep 2025 09:07:07 -0700 Subject: [PATCH 16/20] Remove debug print --- rs/bitcoin/adapter/src/header_cache.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/rs/bitcoin/adapter/src/header_cache.rs b/rs/bitcoin/adapter/src/header_cache.rs index f6941695ec14..188865331ab3 100644 --- a/rs/bitcoin/adapter/src/header_cache.rs +++ b/rs/bitcoin/adapter/src/header_cache.rs @@ -182,7 +182,6 @@ impl HeaderCache for RwLock HeaderCache for RwLock= anchor_height); From 6d4910503471b12a941385652656d2b1d3336997 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Wed, 17 Sep 2025 09:14:36 -0700 Subject: [PATCH 17/20] Fix --- rs/bitcoin/adapter/src/header_cache.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/rs/bitcoin/adapter/src/header_cache.rs b/rs/bitcoin/adapter/src/header_cache.rs index 188865331ab3..5ceb92715c84 100644 --- a/rs/bitcoin/adapter/src/header_cache.rs +++ b/rs/bitcoin/adapter/src/header_cache.rs @@ -492,14 +492,16 @@ impl HybridHeaderCache
) -> Result<(), LMDBCacheError> { if let Some(on_disk) = &self.on_disk { let to_persist = self.in_memory.get_ancestor_chain(anchor); - if let Some((hash, first)) = to_persist.first() { - let anchor_hash = *hash; - let anchor_height = first.data.height; + // Only persist when there are more than 1 header because + // get_ancestor_chain always returns at least 1 header. + if to_persist.len() > 1 { + let (_, node) = &to_persist[0]; + let anchor_height = node.data.height; on_disk.run_rw_txn(|tx| { for (hash, node) in to_persist { on_disk.tx_add_header(tx, hash, node)?; } - on_disk.tx_update_tip(tx, anchor_hash)?; + on_disk.tx_update_tip(tx, anchor)?; Ok(()) })?; self.in_memory.prune_headers_below_height(anchor_height); From 7ded7e7572dd3340ffd29a279966e95a036bd731 Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Wed, 17 Sep 2025 22:46:39 -0700 Subject: [PATCH 18/20] Address reviewer comment --- .../adapter/src/get_successors_handler.rs | 21 +++- rs/bitcoin/adapter/src/header_cache.rs | 98 +++++++++++++++---- 2 files changed, 96 insertions(+), 23 deletions(-) diff --git a/rs/bitcoin/adapter/src/get_successors_handler.rs b/rs/bitcoin/adapter/src/get_successors_handler.rs index 1cc93be5b271..d1afbd36a896 100644 --- a/rs/bitcoin/adapter/src/get_successors_handler.rs +++ b/rs/bitcoin/adapter/src/get_successors_handler.rs @@ -1,6 +1,6 @@ use std::{ collections::{HashSet, VecDeque}, - sync::Arc, + sync::{Arc, Mutex}, }; use bitcoin::{BlockHash, consensus::Encodable}; @@ -90,6 +90,7 @@ pub struct GetSuccessorsHandler { blockchain_manager_tx: Sender, network: Network, metrics: GetSuccessorMetrics, + pruning_task_handle: Mutex>>, } impl GetSuccessorsHandler { @@ -106,6 +107,7 @@ impl GetSuccessorsHandler { blockchain_manager_tx, network, metrics: GetSuccessorMetrics::new(metrics_registry), + pruning_task_handle: Mutex::new(None), } } @@ -123,11 +125,20 @@ impl GetSuccessorsHandler { .processed_block_hashes .observe(request.processed_block_hashes.len() as f64); - // Spawn persist-to-disk task without waiting for it to finish + // Spawn persist-to-disk task without waiting for it to finish, and make sure there + // is only one task running at a time. let cache = self.state.header_cache.clone(); - tokio::task::spawn_blocking(move || { - cache.persist_and_prune_headers_below_anchor(request.anchor) - }); + let mut handle = self.pruning_task_handle.lock().unwrap(); + let is_finished = handle + .as_ref() + .map(|handle| handle.is_finished()) + .unwrap_or(true); + if is_finished { + *handle = Some(tokio::task::spawn_blocking(move || { + // Error is ignored, since it is a background task + let _ = cache.persist_and_prune_headers_below_anchor(request.anchor); + })); + } let (blocks, next, obsolete_blocks) = { let anchor_height = self diff --git a/rs/bitcoin/adapter/src/header_cache.rs b/rs/bitcoin/adapter/src/header_cache.rs index 5ceb92715c84..1477fe7def9c 100644 --- a/rs/bitcoin/adapter/src/header_cache.rs +++ b/rs/bitcoin/adapter/src/header_cache.rs @@ -21,6 +21,9 @@ use thiserror::Error; /// size. It is a constant because it cannot be changed once DB is created. const MAX_LMDB_CACHE_SIZE: usize = 0x2_0000_0000; // 8GB +/// Database key used to store tip header. +const TIP_KEY: &str = "TIP"; + /// Block header with its height in the blockchain and other info. #[derive(Clone, Debug, Ord, PartialOrd, Eq, PartialEq)] pub struct HeaderData
{ @@ -278,6 +281,16 @@ impl From for LMDBCacheError { } } +/// Macro that logs the error when result is not Ok. +macro_rules! log_err { + ($r:expr, $log:expr, $reason:expr) => { + $r.map_err(|err| { + error!($log, "Error in DB operation {}: {:?}", $reason, err); + err + }) + }; +} + /// Like log_err, but won't log the error if it matches the given error code. macro_rules! log_err_except { ($r:expr, $log:expr, $code:pat, $reason:expr) => { @@ -298,7 +311,11 @@ pub struct LMDBHeaderCache { impl LMDBHeaderCache { /// Load the cache with a genesis header and cache directory. - pub fn new(mut cache_dir: PathBuf, log: ReplicaLogger) -> Self { + pub fn new_with_genesis( + mut cache_dir: PathBuf, + log: ReplicaLogger, + genesis: Tip
, + ) -> Result { cache_dir.push("headers"); let path = cache_dir.as_path(); std::fs::create_dir_all(path).unwrap_or_else(|err| { @@ -308,11 +325,27 @@ impl LMDBHeaderCache { let headers = db_env .create_db(Some("HEADERS"), DatabaseFlags::empty()) .unwrap_or_else(|err| panic!("Error creating db for metadata: {:?}", err)); - LMDBHeaderCache { + let cache = LMDBHeaderCache { log, db_env, headers, - } + }; + // Initialize DB with genesis if there is no tip yet + log_err!( + cache.run_rw_txn(|tx| match cache.tx_get_tip_hash(tx) { + Ok(_) => Ok(()), + Err(LMDBCacheError::Lmdb(lmdb::Error::NotFound)) => { + let hash = genesis.header.block_hash(); + cache.tx_add_header(tx, hash, genesis.clone().into())?; + cache.tx_update_tip(tx, hash)?; + Ok(()) + } + Err(err) => Err(err), + }), + cache.log, + "iniialize genesis" + )?; + Ok(cache) } fn tx_get_header( @@ -338,7 +371,7 @@ impl LMDBHeaderCache { } fn tx_get_tip_hash(&self, tx: &Tx) -> Result { - let mut bytes = tx.get(self.headers, b"TIP")?; + let mut bytes = tx.get(self.headers, &TIP_KEY)?; let hash = ::consensus_decode(&mut bytes)?; Ok(hash) } @@ -356,7 +389,7 @@ impl LMDBHeaderCache { tx: &mut RwTransaction, tip_hash: BlockHash, ) -> Result<(), LMDBCacheError> { - tx.put(self.headers, b"TIP", &tip_hash, WriteFlags::empty())?; + tx.put(self.headers, &TIP_KEY, &tip_hash, WriteFlags::empty())?; Ok(()) } @@ -402,26 +435,30 @@ pub struct HybridHeaderCache
{ } impl HybridHeaderCache
{ - pub fn new(genesis: Header, cache_dir: Option, log: ReplicaLogger) -> Self { - let genesis_hash = genesis.block_hash(); - let on_disk = cache_dir.map(|dir| LMDBHeaderCache::new(dir, log)); + pub fn new(genesis_header: Header, cache_dir: Option, log: ReplicaLogger) -> Self { + let genesis_hash = genesis_header.block_hash(); + let genesis = HeaderData { + work: genesis_header.work(), + header: genesis_header, + height: 0, + }; + let on_disk = cache_dir.map(|dir| { + LMDBHeaderCache::new_with_genesis(dir, log, genesis.clone()) + .expect("Error initializing LMDBHeaderCache") + }); // Try reading the anchor (tip of the chain) from disk. // If it doesn't exist, use genesis header. let anchor = on_disk .as_ref() - .and_then(|cache| { - log_err_except!( + .map(|cache| { + log_err!( cache.run_ro_txn(|tx| cache.tx_get_tip(tx)), cache.log, - LMDBCacheError::Lmdb(lmdb::Error::NotFound), "tx_get_tip()" ) + .expect("LMDBHeaderCache contains no tip") }) - .unwrap_or_else(|| HeaderData { - header: genesis.clone(), - height: 0, - work: genesis.work(), - }); + .unwrap_or(genesis); let in_memory = RwLock::new(InMemoryHeaderCache::new_with_anchor(anchor)); Self { in_memory, @@ -595,12 +632,14 @@ pub(crate) mod test { #[test] fn test_hybrid_header_cache() { + type Header = bitcoin::block::Header; + let dir = tempdir().unwrap(); let network = bitcoin::Network::Bitcoin; let genesis_block_header = network.genesis_block_header(); let genesis_block_hash = genesis_block_header.block_hash(); with_test_replica_logger(|logger| { - let cache = >::new( + let cache = >::new( genesis_block_header, Some(dir.path().to_path_buf()), logger, @@ -653,17 +692,40 @@ pub(crate) mod test { } let tips = get_tips(&cache); assert_eq!(tips.len(), 5); + // Test pruning below genesis, should be no-op + cache + .persist_and_prune_headers_below_anchor(genesis_block_hash) + .unwrap(); + let tips = get_tips(&cache); + assert_eq!(tips.len(), 5); cache .persist_and_prune_headers_below_anchor(intermediate_hash) .unwrap(); + // Check if the chain from genesis to tip can still be found assert!(cache.get_header(genesis_block_hash).is_some()); let mut hash = tip.header.block_hash(); while hash != genesis_block_hash { let header = *next_headers.get(&hash).unwrap(); - assert_eq!(Some(header), cache.get_header(hash).map(|x| x.data.header)); + let node = cache.get_header(hash).unwrap(); + assert_eq!(header, node.data.header); + // If height <= anchor, it can be found on-disk + if node.data.height <= intermediate.height { + let on_disk = cache.on_disk.as_ref().unwrap(); + assert!( + on_disk + .run_ro_txn(|tx| on_disk.tx_get_header::<_, Header>(tx, hash)) + .is_ok() + ); + } + // If height >= anchor, it can be found in-memory + if node.data.height >= intermediate.height { + let in_memory = &cache.in_memory; + assert!(in_memory.get_header(hash).is_some()) + } hash = header.prev_block_hash(); } + // Check if all tip ancestors can be found. let tips = get_tips(&cache); assert_eq!(tips.len(), 3); From 7873b23852f0836f66b76acafd9103833bf244ca Mon Sep 17 00:00:00 2001 From: Paul Liu Date: Thu, 18 Sep 2025 02:14:31 -0700 Subject: [PATCH 19/20] Update rs/bitcoin/adapter/src/header_cache.rs Co-authored-by: mducroux --- rs/bitcoin/adapter/src/header_cache.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/bitcoin/adapter/src/header_cache.rs b/rs/bitcoin/adapter/src/header_cache.rs index 1477fe7def9c..c64867396956 100644 --- a/rs/bitcoin/adapter/src/header_cache.rs +++ b/rs/bitcoin/adapter/src/header_cache.rs @@ -421,7 +421,7 @@ impl LMDBHeaderCache { /// /// 2. The in-memory cache contains headers from the anchor to latest. /// -/// 3. The on-disk headers form a single list, where the anchor is the tip and has no child. +/// 3. The on-disk headers form a linear chain with no forks, where the anchor is the tip and has no child. /// /// 4. The two caches would overlap only at the anchor header, in which case /// [get_header] would always return the header stored at in-memory cache. From 087fe11d2db1071e915ff7a5220859197537471b Mon Sep 17 00:00:00 2001 From: "Paul H. Liu" Date: Thu, 18 Sep 2025 02:18:20 -0700 Subject: [PATCH 20/20] Address reviewer feedback --- rs/bitcoin/adapter/src/header_cache.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/rs/bitcoin/adapter/src/header_cache.rs b/rs/bitcoin/adapter/src/header_cache.rs index c64867396956..961e60157f77 100644 --- a/rs/bitcoin/adapter/src/header_cache.rs +++ b/rs/bitcoin/adapter/src/header_cache.rs @@ -483,7 +483,9 @@ impl HybridHeaderCache
.into_pure_header() } - fn get_header_on_disk(&self, hash: BlockHash) -> Option> { + // Lookup header from the on-disk cache. Return None if it is not found or + // the on-disk cache is not enabled. + fn get_header_from_disk(&self, hash: BlockHash) -> Option> { self.on_disk.as_ref().and_then(|cache| { log_err_except!( cache.run_ro_txn(|tx| cache.tx_get_header(tx, hash)), @@ -498,7 +500,7 @@ impl HybridHeaderCache
pub fn get_header(&self, hash: BlockHash) -> Option> { self.in_memory .get_header(hash) - .or_else(|| self.get_header_on_disk(hash)) + .or_else(|| self.get_header_from_disk(hash)) } /// Add a new header. @@ -711,17 +713,11 @@ pub(crate) mod test { assert_eq!(header, node.data.header); // If height <= anchor, it can be found on-disk if node.data.height <= intermediate.height { - let on_disk = cache.on_disk.as_ref().unwrap(); - assert!( - on_disk - .run_ro_txn(|tx| on_disk.tx_get_header::<_, Header>(tx, hash)) - .is_ok() - ); + assert!(cache.get_header_from_disk(hash).is_some()); } // If height >= anchor, it can be found in-memory if node.data.height >= intermediate.height { - let in_memory = &cache.in_memory; - assert!(in_memory.get_header(hash).is_some()) + assert!(cache.in_memory.get_header(hash).is_some()) } hash = header.prev_block_hash(); }