Skip to content

Commit c496cde

Browse files
ValuedMammalValuedMammal
committed
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
2 parents 5050537 + 568a366 commit c496cde

File tree

2 files changed

+130
-33
lines changed

2 files changed

+130
-33
lines changed

‎crates/esplora/src/async_ext.rs

Lines changed: 65 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -205,12 +205,16 @@ async fn fetch_block<S: Sleeper>(
205205

206206
// We avoid fetching blocks higher than previously fetched `latest_blocks` as the local chain
207207
// tip is used to signal for the last-synced-up-to-height.
208-
let &tip_height = latest_blocks
209-
.keys()
210-
.last()
211-
.expect("must have atleast one entry");
212-
if height > tip_height {
213-
return Ok(None);
208+
match latest_blocks.keys().last().copied() {
209+
None => {
210+
debug_assert!(false, "`latest_blocks` should not be empty");
211+
return Ok(None);
212+
}
213+
Some(tip_height) => {
214+
if height > tip_height {
215+
return Ok(None);
216+
}
217+
}
214218
}
215219

216220
Ok(Some(client.get_block_hash(height).await?))
@@ -227,27 +231,36 @@ async fn chain_update<S: Sleeper>(
227231
anchors: &BTreeSet<(ConfirmationBlockTime, Txid)>,
228232
) -> Result<CheckPoint, Error> {
229233
let mut point_of_agreement = None;
234+
let mut local_cp_hash = local_tip.hash();
230235
let mut conflicts = vec![];
236+
231237
for local_cp in local_tip.iter() {
232238
let remote_hash = match fetch_block(client, latest_blocks, local_cp.height()).await? {
233239
Some(hash) => hash,
234240
None => continue,
235241
};
236242
if remote_hash == local_cp.hash() {
237-
point_of_agreement = Some(local_cp.clone());
243+
point_of_agreement = Some(local_cp);
238244
break;
239-
} else {
240-
// it is not strictly necessary to include all the conflicted heights (we do need the
241-
// first one) but it seems prudent to make sure the updated chain's heights are a
242-
// superset of the existing chain after update.
243-
conflicts.push(BlockId {
244-
height: local_cp.height(),
245-
hash: remote_hash,
246-
});
247245
}
246+
local_cp_hash = local_cp.hash();
247+
// It is not strictly necessary to include all the conflicted heights (we do need the
248+
// first one) but it seems prudent to make sure the updated chain's heights are a
249+
// superset of the existing chain after update.
250+
conflicts.push(BlockId {
251+
height: local_cp.height(),
252+
hash: remote_hash,
253+
});
248254
}
249255

250-
let mut tip = point_of_agreement.expect("remote esplora should have same genesis block");
256+
let mut tip = match point_of_agreement {
257+
Some(tip) => tip,
258+
None => {
259+
return Err(Box::new(esplora_client::Error::HeaderHashNotFound(
260+
local_cp_hash,
261+
)));
262+
}
263+
};
251264

252265
tip = tip
253266
.extend(conflicts.into_iter().rev())
@@ -545,7 +558,7 @@ mod test {
545558
local_chain::LocalChain,
546559
BlockId,
547560
};
548-
use bdk_core::ConfirmationBlockTime;
561+
use bdk_core::{bitcoin, ConfirmationBlockTime};
549562
use bdk_testenv::{anyhow, bitcoincore_rpc::RpcApi, TestEnv};
550563
use esplora_client::Builder;
551564

@@ -557,6 +570,41 @@ mod test {
557570
}};
558571
}
559572

573+
// Test that `chain_update` fails due to wrong network.
574+
#[tokio::test]
575+
async fn test_chain_update_wrong_network_error() -> anyhow::Result<()> {
576+
let env = TestEnv::new()?;
577+
let base_url = format!("http://{}", &env.electrsd.esplora_url.clone().unwrap());
578+
let client = Builder::new(base_url.as_str()).build_async()?;
579+
let initial_height = env.rpc_client().get_block_count()? as u32;
580+
581+
let mine_to = 16;
582+
let _ = env.mine_blocks((mine_to - initial_height) as usize, None)?;
583+
while client.get_height().await? < mine_to {
584+
std::thread::sleep(Duration::from_millis(64));
585+
}
586+
let latest_blocks = fetch_latest_blocks(&client).await?;
587+
assert!(!latest_blocks.is_empty());
588+
assert_eq!(latest_blocks.keys().last(), Some(&mine_to));
589+
590+
let genesis_hash =
591+
bitcoin::constants::genesis_block(bitcoin::Network::Testnet4).block_hash();
592+
let cp = bdk_chain::CheckPoint::new(BlockId {
593+
height: 0,
594+
hash: genesis_hash,
595+
});
596+
597+
let anchors = BTreeSet::new();
598+
let res = chain_update(&client, &latest_blocks, &cp, &anchors).await;
599+
use esplora_client::Error;
600+
assert!(
601+
matches!(*res.unwrap_err(), Error::HeaderHashNotFound(hash) if hash == genesis_hash),
602+
"`chain_update` should error if it can't connect to the local CP",
603+
);
604+
605+
Ok(())
606+
}
607+
560608
/// Ensure that update does not remove heights (from original), and all anchor heights are
561609
/// included.
562610
#[tokio::test]

‎crates/esplora/src/blocking_ext.rs

Lines changed: 65 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -190,12 +190,16 @@ fn fetch_block(
190190

191191
// We avoid fetching blocks higher than previously fetched `latest_blocks` as the local chain
192192
// tip is used to signal for the last-synced-up-to-height.
193-
let &tip_height = latest_blocks
194-
.keys()
195-
.last()
196-
.expect("must have atleast one entry");
197-
if height > tip_height {
198-
return Ok(None);
193+
match latest_blocks.keys().last().copied() {
194+
None => {
195+
debug_assert!(false, "`latest_blocks` should not be empty");
196+
return Ok(None);
197+
}
198+
Some(tip_height) => {
199+
if height > tip_height {
200+
return Ok(None);
201+
}
202+
}
199203
}
200204

201205
Ok(Some(client.get_block_hash(height)?))
@@ -212,27 +216,36 @@ fn chain_update(
212216
anchors: &BTreeSet<(ConfirmationBlockTime, Txid)>,
213217
) -> Result<CheckPoint, Error> {
214218
let mut point_of_agreement = None;
219+
let mut local_cp_hash = local_tip.hash();
215220
let mut conflicts = vec![];
221+
216222
for local_cp in local_tip.iter() {
217223
let remote_hash = match fetch_block(client, latest_blocks, local_cp.height())? {
218224
Some(hash) => hash,
219225
None => continue,
220226
};
221227
if remote_hash == local_cp.hash() {
222-
point_of_agreement = Some(local_cp.clone());
228+
point_of_agreement = Some(local_cp);
223229
break;
224-
} else {
225-
// it is not strictly necessary to include all the conflicted heights (we do need the
226-
// first one) but it seems prudent to make sure the updated chain's heights are a
227-
// superset of the existing chain after update.
228-
conflicts.push(BlockId {
229-
height: local_cp.height(),
230-
hash: remote_hash,
231-
});
232230
}
231+
local_cp_hash = local_cp.hash();
232+
// It is not strictly necessary to include all the conflicted heights (we do need the
233+
// first one) but it seems prudent to make sure the updated chain's heights are a
234+
// superset of the existing chain after update.
235+
conflicts.push(BlockId {
236+
height: local_cp.height(),
237+
hash: remote_hash,
238+
});
233239
}
234240

235-
let mut tip = point_of_agreement.expect("remote esplora should have same genesis block");
241+
let mut tip = match point_of_agreement {
242+
Some(tip) => tip,
243+
None => {
244+
return Err(Box::new(esplora_client::Error::HeaderHashNotFound(
245+
local_cp_hash,
246+
)));
247+
}
248+
};
236249

237250
tip = tip
238251
.extend(conflicts.into_iter().rev())
@@ -498,6 +511,7 @@ fn fetch_txs_with_outpoints<I: IntoIterator<Item = OutPoint>>(
498511
#[cfg(test)]
499512
mod test {
500513
use crate::blocking_ext::{chain_update, fetch_latest_blocks};
514+
use bdk_chain::bitcoin;
501515
use bdk_chain::bitcoin::hashes::Hash;
502516
use bdk_chain::bitcoin::Txid;
503517
use bdk_chain::local_chain::LocalChain;
@@ -522,6 +536,41 @@ mod test {
522536
}};
523537
}
524538

539+
// Test that `chain_update` fails due to wrong network.
540+
#[test]
541+
fn test_chain_update_wrong_network_error() -> anyhow::Result<()> {
542+
let env = TestEnv::new()?;
543+
let base_url = format!("http://{}", &env.electrsd.esplora_url.clone().unwrap());
544+
let client = Builder::new(base_url.as_str()).build_blocking();
545+
let initial_height = env.rpc_client().get_block_count()? as u32;
546+
547+
let mine_to = 16;
548+
let _ = env.mine_blocks((mine_to - initial_height) as usize, None)?;
549+
while client.get_height()? < mine_to {
550+
std::thread::sleep(Duration::from_millis(64));
551+
}
552+
let latest_blocks = fetch_latest_blocks(&client)?;
553+
assert!(!latest_blocks.is_empty());
554+
assert_eq!(latest_blocks.keys().last(), Some(&mine_to));
555+
556+
let genesis_hash =
557+
bitcoin::constants::genesis_block(bitcoin::Network::Testnet4).block_hash();
558+
let cp = bdk_chain::CheckPoint::new(BlockId {
559+
height: 0,
560+
hash: genesis_hash,
561+
});
562+
563+
let anchors = BTreeSet::new();
564+
let res = chain_update(&client, &latest_blocks, &cp, &anchors);
565+
use esplora_client::Error;
566+
assert!(
567+
matches!(*res.unwrap_err(), Error::HeaderHashNotFound(hash) if hash == genesis_hash),
568+
"`chain_update` should error if it can't connect to the local CP",
569+
);
570+
571+
Ok(())
572+
}
573+
525574
/// Ensure that update does not remove heights (from original), and all anchor heights are
526575
/// included.
527576
#[test]

0 commit comments

Comments
 (0)