Merge #1971: esplora: chain_update errors if no point of connection

568a366 refactor(esplora): in debug build assert that `latest_blocks` is not empty (valued mammal)
dd394cb fix(esplora): `chain_update` errors if no point of connection (valued mammal)

Pull request description:

  ### Description

  Before, the `chain_update` function might have panicked if the local checkpoint was not on the same network as the remote server. Now if we have iterated all of the blocks of the local CP and do not find a point of agreement, then we return early with a `esplora_client::Error::HeaderHashNotFound`.

  cc bitcoindevkit/bdk_wallet#30

  ### Notes to the reviewers

  ### Changelog notice

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo +nightly fmt` and `cargo clippy` before committing

  #### New Features:

  * [ ] I've added tests for the new feature
  * [ ] I've added docs for the new feature

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  ValuedMammal:
    ACK 568a366

Tree-SHA512: 8dceaf24f1214d3463e14c7b5965beb34138985319962af04d1027028da2fddece185a7437a33713020e38b2a4129bd14e1dc0a3735e9ece15ab35a5f828360a

Author: ValuedMammal

crates/esplora/src/async_ext.rs

@@ -205,12 +205,16 @@ async fn fetch_block<S: Sleeper>(
 
     // We avoid fetching blocks higher than previously fetched `latest_blocks` as the local chain
     // tip is used to signal for the last-synced-up-to-height.
-    let &tip_height = latest_blocks
-        .keys()
-        .last()
-        .expect("must have atleast one entry");
-    if height > tip_height {
-        return Ok(None);
+    match latest_blocks.keys().last().copied() {
+        None => {
+            debug_assert!(false, "`latest_blocks` should not be empty");
+            return Ok(None);
+        }
+        Some(tip_height) => {
+            if height > tip_height {
+                return Ok(None);
+            }
+        }
     }
 
     Ok(Some(client.get_block_hash(height).await?))
@@ -227,27 +231,36 @@ async fn chain_update<S: Sleeper>(
     anchors: &BTreeSet<(ConfirmationBlockTime, Txid)>,
 ) -> Result<CheckPoint, Error> {
     let mut point_of_agreement = None;
+    let mut local_cp_hash = local_tip.hash();
     let mut conflicts = vec![];
+
     for local_cp in local_tip.iter() {
         let remote_hash = match fetch_block(client, latest_blocks, local_cp.height()).await? {
             Some(hash) => hash,
             None => continue,
         };
         if remote_hash == local_cp.hash() {
-            point_of_agreement = Some(local_cp.clone());
+            point_of_agreement = Some(local_cp);
             break;
-        } else {
-            // it is not strictly necessary to include all the conflicted heights (we do need the
-            // first one) but it seems prudent to make sure the updated chain's heights are a
-            // superset of the existing chain after update.
-            conflicts.push(BlockId {
-                height: local_cp.height(),
-                hash: remote_hash,
-            });
         }
+        local_cp_hash = local_cp.hash();
+        // It is not strictly necessary to include all the conflicted heights (we do need the
+        // first one) but it seems prudent to make sure the updated chain's heights are a
+        // superset of the existing chain after update.
+        conflicts.push(BlockId {
+            height: local_cp.height(),
+            hash: remote_hash,
+        });
     }
 
-    let mut tip = point_of_agreement.expect("remote esplora should have same genesis block");
+    let mut tip = match point_of_agreement {
+        Some(tip) => tip,
+        None => {
+            return Err(Box::new(esplora_client::Error::HeaderHashNotFound(
+                local_cp_hash,
+            )));
+        }
+    };
 
     tip = tip
         .extend(conflicts.into_iter().rev())
@@ -545,7 +558,7 @@ mod test {
         local_chain::LocalChain,
         BlockId,
     };
-    use bdk_core::ConfirmationBlockTime;
+    use bdk_core::{bitcoin, ConfirmationBlockTime};
     use bdk_testenv::{anyhow, bitcoincore_rpc::RpcApi, TestEnv};
     use esplora_client::Builder;
 
@@ -557,6 +570,41 @@ mod test {
         }};
     }
 
+    // Test that `chain_update` fails due to wrong network.
+    #[tokio::test]
+    async fn test_chain_update_wrong_network_error() -> anyhow::Result<()> {
+        let env = TestEnv::new()?;
+        let base_url = format!("http://{}", &env.electrsd.esplora_url.clone().unwrap());
+        let client = Builder::new(base_url.as_str()).build_async()?;
+        let initial_height = env.rpc_client().get_block_count()? as u32;
+
+        let mine_to = 16;
+        let _ = env.mine_blocks((mine_to - initial_height) as usize, None)?;
+        while client.get_height().await? < mine_to {
+            std::thread::sleep(Duration::from_millis(64));
+        }
+        let latest_blocks = fetch_latest_blocks(&client).await?;
+        assert!(!latest_blocks.is_empty());
+        assert_eq!(latest_blocks.keys().last(), Some(&mine_to));
+
+        let genesis_hash =
+            bitcoin::constants::genesis_block(bitcoin::Network::Testnet4).block_hash();
+        let cp = bdk_chain::CheckPoint::new(BlockId {
+            height: 0,
+            hash: genesis_hash,
+        });
+
+        let anchors = BTreeSet::new();
+        let res = chain_update(&client, &latest_blocks, &cp, &anchors).await;
+        use esplora_client::Error;
+        assert!(
+            matches!(*res.unwrap_err(), Error::HeaderHashNotFound(hash) if hash == genesis_hash),
+            "`chain_update` should error if it can't connect to the local CP",
+        );
+
+        Ok(())
+    }
+
     /// Ensure that update does not remove heights (from original), and all anchor heights are
     /// included.
     #[tokio::test]

crates/esplora/src/blocking_ext.rs

@@ -190,12 +190,16 @@ fn fetch_block(
 
     // We avoid fetching blocks higher than previously fetched `latest_blocks` as the local chain
     // tip is used to signal for the last-synced-up-to-height.
-    let &tip_height = latest_blocks
-        .keys()
-        .last()
-        .expect("must have atleast one entry");
-    if height > tip_height {
-        return Ok(None);
+    match latest_blocks.keys().last().copied() {
+        None => {
+            debug_assert!(false, "`latest_blocks` should not be empty");
+            return Ok(None);
+        }
+        Some(tip_height) => {
+            if height > tip_height {
+                return Ok(None);
+            }
+        }
     }
 
     Ok(Some(client.get_block_hash(height)?))
@@ -212,27 +216,36 @@ fn chain_update(
     anchors: &BTreeSet<(ConfirmationBlockTime, Txid)>,
 ) -> Result<CheckPoint, Error> {
     let mut point_of_agreement = None;
+    let mut local_cp_hash = local_tip.hash();
     let mut conflicts = vec![];
+
     for local_cp in local_tip.iter() {
         let remote_hash = match fetch_block(client, latest_blocks, local_cp.height())? {
             Some(hash) => hash,
             None => continue,
         };
         if remote_hash == local_cp.hash() {
-            point_of_agreement = Some(local_cp.clone());
+            point_of_agreement = Some(local_cp);
             break;
-        } else {
-            // it is not strictly necessary to include all the conflicted heights (we do need the
-            // first one) but it seems prudent to make sure the updated chain's heights are a
-            // superset of the existing chain after update.
-            conflicts.push(BlockId {
-                height: local_cp.height(),
-                hash: remote_hash,
-            });
         }
+        local_cp_hash = local_cp.hash();
+        // It is not strictly necessary to include all the conflicted heights (we do need the
+        // first one) but it seems prudent to make sure the updated chain's heights are a
+        // superset of the existing chain after update.
+        conflicts.push(BlockId {
+            height: local_cp.height(),
+            hash: remote_hash,
+        });
     }
 
-    let mut tip = point_of_agreement.expect("remote esplora should have same genesis block");
+    let mut tip = match point_of_agreement {
+        Some(tip) => tip,
+        None => {
+            return Err(Box::new(esplora_client::Error::HeaderHashNotFound(
+                local_cp_hash,
+            )));
+        }
+    };
 
     tip = tip
         .extend(conflicts.into_iter().rev())
@@ -498,6 +511,7 @@ fn fetch_txs_with_outpoints<I: IntoIterator<Item = OutPoint>>(
 #[cfg(test)]
 mod test {
     use crate::blocking_ext::{chain_update, fetch_latest_blocks};
+    use bdk_chain::bitcoin;
     use bdk_chain::bitcoin::hashes::Hash;
     use bdk_chain::bitcoin::Txid;
     use bdk_chain::local_chain::LocalChain;
@@ -522,6 +536,41 @@ mod test {
         }};
     }
 
+    // Test that `chain_update` fails due to wrong network.
+    #[test]
+    fn test_chain_update_wrong_network_error() -> anyhow::Result<()> {
+        let env = TestEnv::new()?;
+        let base_url = format!("http://{}", &env.electrsd.esplora_url.clone().unwrap());
+        let client = Builder::new(base_url.as_str()).build_blocking();
+        let initial_height = env.rpc_client().get_block_count()? as u32;
+
+        let mine_to = 16;
+        let _ = env.mine_blocks((mine_to - initial_height) as usize, None)?;
+        while client.get_height()? < mine_to {
+            std::thread::sleep(Duration::from_millis(64));
+        }
+        let latest_blocks = fetch_latest_blocks(&client)?;
+        assert!(!latest_blocks.is_empty());
+        assert_eq!(latest_blocks.keys().last(), Some(&mine_to));
+
+        let genesis_hash =
+            bitcoin::constants::genesis_block(bitcoin::Network::Testnet4).block_hash();
+        let cp = bdk_chain::CheckPoint::new(BlockId {
+            height: 0,
+            hash: genesis_hash,
+        });
+
+        let anchors = BTreeSet::new();
+        let res = chain_update(&client, &latest_blocks, &cp, &anchors);
+        use esplora_client::Error;
+        assert!(
+            matches!(*res.unwrap_err(), Error::HeaderHashNotFound(hash) if hash == genesis_hash),
+            "`chain_update` should error if it can't connect to the local CP",
+        );
+
+        Ok(())
+    }
+
     /// Ensure that update does not remove heights (from original), and all anchor heights are
     /// included.
     #[test]