Skip to content

refactor(esplora): clear remaining panic paths #2053

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

refactor(esplora): clear remaining panic paths #2053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0cbf254
refactor(esplora): clear remaining panic paths
reez Sep 29, 2025
9ea0dd0
refactor(esplora): define blocking Error enum
reez Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions crates/esplora/src/async_ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ async fn chain_update<S: Sleeper>(

tip = tip
.extend(conflicts.into_iter().rev().map(|b| (b.height, b.hash)))
.expect("evicted are in order");
.map_err(|_| Box::new(esplora_client::Error::InvalidResponse))?;

for (anchor, _txid) in anchors {
let height = anchor.block_id.height;
Expand Down Expand Up @@ -314,8 +314,9 @@ where
type TxsOfSpkIndex = (u32, Vec<esplora_client::Tx>, HashSet<Txid>);

let mut update = TxUpdate::<ConfirmationBlockTime>::default();
let mut last_index = Option::<u32>::None;
let mut last_active_index = Option::<u32>::None;
let mut consecutive_unused = 0usize;
let gap_limit = stop_gap.max(parallel_requests.max(1));

loop {
let handles = keychain_spks
Expand Down Expand Up @@ -352,8 +353,10 @@ where
}

for (index, txs, evicted) in handles.try_collect::<Vec<TxsOfSpkIndex>>().await? {
last_index = Some(index);
if !txs.is_empty() {
if txs.is_empty() {
consecutive_unused = consecutive_unused.saturating_add(1);
} else {
consecutive_unused = 0;
last_active_index = Some(index);
}
for tx in txs {
Expand All @@ -368,13 +371,7 @@ where
.extend(evicted.into_iter().map(|txid| (txid, start_time)));
}

let last_index = last_index.expect("Must be set since handles wasn't empty.");
let gap_limit_reached = if let Some(i) = last_active_index {
last_index >= i.saturating_add(stop_gap as u32)
} else {
last_index + 1 >= stop_gap as u32
};
if gap_limit_reached {
if consecutive_unused >= gap_limit {
break;
}
}
Expand Down Expand Up @@ -571,6 +568,15 @@ mod test {
}};
}

#[test]
fn ensure_last_index_none_returns_error() {
let last_index: Option<u32> = None;
let err = last_index
.ok_or_else(|| Box::new(esplora_client::Error::InvalidResponse))
.unwrap_err();
assert!(matches!(*err, esplora_client::Error::InvalidResponse));
}
Comment on lines +571 to +578
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed. If this fails, it's a problem with rust.


// Test that `chain_update` fails due to wrong network.
#[tokio::test]
async fn test_chain_update_wrong_network_error() -> anyhow::Result<()> {
Expand Down
123 changes: 101 additions & 22 deletions crates/esplora/src/blocking_ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,54 @@ use bdk_core::{
BlockId, CheckPoint, ConfirmationBlockTime, Indexed, TxUpdate,
};
use esplora_client::{OutputStatus, Tx};
use std::any::Any;
use std::fmt;
use std::thread::JoinHandle;

use crate::{insert_anchor_or_seen_at_from_status, insert_prevouts};

/// [`esplora_client::Error`]
pub type Error = Box<esplora_client::Error>;
#[derive(Debug)]
pub enum Error {
Client(esplora_client::Error),
ThreadPanic(Option<String>),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems over-engineered. Thread panics are highly unlikely because:

  • No explicit panics in the thread - all errors are propagated.
  • We only call esplora_client methods - if the method panics, it's a bug in the esplora_client library and should be addressed in the upstream crate.

ThreadPanic is really only protection against bugs in the dependency chain (should be addressed upstream) or extreme memory exhaustion (which would crash the application anyway).

}

impl Error {
fn from_thread_panic(err: Box<dyn Any + Send>) -> Self {
if let Ok(msg) = err.downcast::<String>() {
Self::ThreadPanic(Some(*msg))
} else if let Ok(msg) = err.downcast::<&'static str>() {
Self::ThreadPanic(Some(msg.to_string()))
} else {
Self::ThreadPanic(None)
}
}
}

impl From<esplora_client::Error> for Error {
fn from(err: esplora_client::Error) -> Self {
Self::Client(err)
}
}

impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::Client(err) => write!(f, "{err}"),
Self::ThreadPanic(Some(msg)) => write!(f, "worker thread panicked: {msg}"),
Self::ThreadPanic(None) => write!(f, "worker thread panicked"),
}
}
}

impl std::error::Error for Error {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
match self {
Self::Client(err) => Some(err),
_ => None,
}
}
}

/// Trait to extend the functionality of [`esplora_client::BlockingClient`].
///
Expand Down Expand Up @@ -241,15 +283,13 @@ fn chain_update(
let mut tip = match point_of_agreement {
Some(tip) => tip,
None => {
return Err(Box::new(esplora_client::Error::HeaderHashNotFound(
local_cp_hash,
)));
return Err(esplora_client::Error::HeaderHashNotFound(local_cp_hash).into());
}
};

tip = tip
.extend(conflicts.into_iter().rev().map(|b| (b.height, b.hash)))
.expect("evicted are in order");
.map_err(|_| Error::from(esplora_client::Error::InvalidResponse))?;
Copy link
Author

@reez reez Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add error variant, Checkpoint error.

Checkpoint and wraps checkpoint type.

put in lib.rs, have async and blocking use new error


for (anchor, _) in anchors {
let height = anchor.block_id.height;
Expand Down Expand Up @@ -282,8 +322,10 @@ fn fetch_txs_with_keychain_spks<I: Iterator<Item = Indexed<SpkWithExpectedTxids>
type TxsOfSpkIndex = (u32, Vec<esplora_client::Tx>, HashSet<Txid>);

let mut update = TxUpdate::<ConfirmationBlockTime>::default();
let mut last_index = Option::<u32>::None;
let mut last_active_index = Option::<u32>::None;
let mut consecutive_unused = 0usize;
let mut processed_any = false;
let gap_limit = stop_gap.max(1);

loop {
let handles = keychain_spks
Expand Down Expand Up @@ -316,13 +358,22 @@ fn fetch_txs_with_keychain_spks<I: Iterator<Item = Indexed<SpkWithExpectedTxids>
.collect::<Vec<JoinHandle<Result<TxsOfSpkIndex, Error>>>>();

if handles.is_empty() {
if !processed_any {
return Err(esplora_client::Error::InvalidResponse.into());
Copy link
Author

@reez reez Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you are trying to catch a empty-input? You'll need a justification on why an empty input should result in an error. My understanding is that the caller may create a FullScanRequest with an empty keychain and if they do, it's not problematic at all and we should just return nothing (which looks like what the original code is doing).

break;
}

for handle in handles {
let (index, txs, evicted) = handle.join().expect("thread must not panic")?;
last_index = Some(index);
if !txs.is_empty() {
let handle_result = handle
.join()
.map_err(Error::from_thread_panic)?;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont need this if not worried about panic on thread panic

let (index, txs, evicted) = handle_result?;
processed_any = true;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont need processed_any

if txs.is_empty() {
consecutive_unused = consecutive_unused.saturating_add(1);
} else {
consecutive_unused = 0;
last_active_index = Some(index);
}
for tx in txs {
Expand All @@ -337,13 +388,7 @@ fn fetch_txs_with_keychain_spks<I: Iterator<Item = Indexed<SpkWithExpectedTxids>
.extend(evicted.into_iter().map(|txid| (txid, start_time)));
}

let last_index = last_index.expect("Must be set since handles wasn't empty.");
let gap_limit_reached = if let Some(i) = last_active_index {
last_index >= i.saturating_add(stop_gap as u32)
} else {
last_index + 1 >= stop_gap as u32
};
if gap_limit_reached {
if consecutive_unused >= gap_limit {
break;
}
}
Expand Down Expand Up @@ -406,7 +451,7 @@ fn fetch_txs_with_txids<I: IntoIterator<Item = Txid>>(
std::thread::spawn(move || {
client
.get_tx_info(&txid)
.map_err(Box::new)
.map_err(Error::from)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map this more explicitly

.map(|t| (txid, t))
})
})
Expand All @@ -417,7 +462,10 @@ fn fetch_txs_with_txids<I: IntoIterator<Item = Txid>>(
}

for handle in handles {
let (txid, tx_info) = handle.join().expect("thread must not panic")?;
let handle_result = handle
.join()
.map_err(Error::from_thread_panic)?;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont worry about panic here

let (txid, tx_info) = handle_result?;
if let Some(tx_info) = tx_info {
if inserted_txs.insert(txid) {
update.txs.push(tx_info.to_tx().into());
Expand Down Expand Up @@ -468,7 +516,7 @@ fn fetch_txs_with_outpoints<I: IntoIterator<Item = OutPoint>>(
std::thread::spawn(move || {
client
.get_output_status(&op.txid, op.vout as _)
.map_err(Box::new)
.map_err(Error::from)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

be more explicit here too

})
})
.collect::<Vec<JoinHandle<Result<Option<OutputStatus>, Error>>>>();
Expand All @@ -478,7 +526,10 @@ fn fetch_txs_with_outpoints<I: IntoIterator<Item = OutPoint>>(
}

for handle in handles {
if let Some(op_status) = handle.join().expect("thread must not panic")? {
let handle_result = handle
.join()
.map_err(Error::from_thread_panic)?;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont worry about panic here

if let Some(op_status) = handle_result? {
let spend_txid = match op_status.txid {
Some(txid) => txid,
None => continue,
Expand Down Expand Up @@ -511,7 +562,7 @@ fn fetch_txs_with_outpoints<I: IntoIterator<Item = OutPoint>>(
#[cfg(test)]
#[cfg_attr(coverage_nightly, coverage(off))]
mod test {
use crate::blocking_ext::{chain_update, fetch_latest_blocks};
use crate::blocking_ext::{chain_update, fetch_latest_blocks, Error};
use bdk_chain::bitcoin;
use bdk_chain::bitcoin::hashes::Hash;
use bdk_chain::bitcoin::Txid;
Expand All @@ -529,6 +580,34 @@ mod test {
}};
}

#[test]
fn thread_join_panic_maps_to_error() {
let handle = std::thread::spawn(|| -> Result<(), Error> {
panic!("expected panic for test coverage");
});

let res = (|| -> Result<(), Error> {
let handle_result = handle
.join()
.map_err(Error::from_thread_panic)?;
handle_result
})();

assert!(matches!(
res.unwrap_err(),
Error::ThreadPanic(_)
));
}

#[test]
fn ensure_last_index_none_returns_error() {
let last_index: Option<u32> = None;
let err = last_index
.ok_or_else(|| Error::from(esplora_client::Error::InvalidResponse))
.unwrap_err();
assert!(matches!(err, Error::Client(esplora_client::Error::InvalidResponse)));
}

Comment on lines +583 to +610
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to test rust for bugs in our repo.

macro_rules! local_chain {
[ $(($height:expr, $block_hash:expr)), * ] => {{
#[allow(unused_mut)]
Expand Down
Loading