refactor(esplora): clear remaining panic paths #2053
Conversation
crates/esplora/src/async_ext.rs
Outdated
| let last_index = | ||
| last_index.ok_or_else(|| Box::new(esplora_client::Error::InvalidResponse))?; |
There was a problem hiding this comment.
One suggestion is to leave last_index as an Option and wrap stop_gap in Some(..) when doing the comparison.
But there's a much better way to handle this, which is to introduce an unused_count variable. Now when processing handles, if txs.is_empty() we increment the unused count, else update the last active index (and reset the unused count). Finally, break once the unused count reaches the gap limit. AFAICT there'd be no reason to keep track of the last_index scanned.
There was a problem hiding this comment.
Took your suggestion and worked it in; didn't want to push it to this branch yet if I had the wrong idea, but here is the commit reez@35538e2 on a branch that builds on this branch.
I felt like I had to keep two small guardrails from the old logic:
processed_any(still surface the “no handles ran” error)gap_limit = stop_gap.max(1)(keeps old “stop_gap==0 still means one empty derivation” behavior)
There was a problem hiding this comment.
I don't think it's an error if handles turns up empty - we need a way to break from the loop once all of the keychain-spks are consumed.
There was a problem hiding this comment.
Maybe stop_gap = stop_gap.max(parallel_requests), since we check at least 1 spk-index for every request in parallel_requests.
There was a problem hiding this comment.
Super helpful thanks mammal, I made the below changes to that my side branch esplora+mammal that I can eventually fold back into this esplora branch if I'm on the right track
I don't think it's an error if handles turns up empty - we need a way to break from the loop once all of the keychain-spks are consumed.
Good call. I made this change 36073d0 based on that
Maybe stop_gap = stop_gap.max(parallel_requests), since we check at least 1 spk-index for every request in parallel_requests.
Good catch, made another change 590edcf based on this
There was a problem hiding this comment.
pulled these changes into this pr now
crates/esplora/src/blocking_ext.rs
Outdated
| let (index, txs, evicted) = handle.join().expect("thread must not panic")?; | ||
| let handle_result = handle | ||
| .join() | ||
| .map_err(|_| Box::new(esplora_client::Error::InvalidResponse))?; |
There was a problem hiding this comment.
Because you're dropping the error it won't be very informative to propagate it upward. Instead this module can define its own Error type to handle errors that aren't covered by esplora_client::Error.
There was a problem hiding this comment.
Tried to address this with new commit 9ea0dd0, trying to do the most simple thing I could think of that addresses what you mentioned
reez
changed the title
evanlinjin
left a comment
evanlinjin
left a comment
There was a problem hiding this comment.
Thanks for working on this!
Some great simplifications in logic. Some parts are over-engineered.
Note that panics and expects indicate that "we should never hit this - unless there is a bug!" and I think it's justified to have them (where reasonable).
crates/esplora/src/blocking_ext.rs
Outdated
| .join() | ||
| .map_err(Error::from_thread_panic)?; | ||
| let (index, txs, evicted) = handle_result?; | ||
| processed_any = true; |
|
Updated PR due to great feedback from Evan, Mammal, Leonardo. Noteworthy:
|
evanlinjin
left a comment
evanlinjin
left a comment
There was a problem hiding this comment.
Avoid returning errors for internal logic bugs in BDK itself.
| pub enum Error { | ||
| Client(esplora_client::Error), | ||
| Checkpoint(CheckPoint<BlockHash>), | ||
| } |
There was a problem hiding this comment.
We're missing doc-comments for the Error type and it's variants!
We should make it obvious to the reader when the Error::CheckPoint variant occurs.
Edit: I don't think the CheckPoint variant is needed, so this Error type is not needed either.
| tip = tip | ||
| .extend(conflicts.into_iter().rev().map(|b| (b.height, b.hash))) | ||
| .expect("evicted are in order"); | ||
| .map_err(Error::Checkpoint)?; |
There was a problem hiding this comment.
I still think panic/expect here is more appropriate.
CheckPoint::extendonly errors if a block is not higher than the previous.- The logic to populate
conflictsuses heights fromlocal_tip.local_tipis guaranteed to have ordered heights (since it is aCheckPointtype).
3 participants
Description
This PR clears remaining panic paths from bdk_esplora, replacing them with proper error handling.
Attempting to address bitcoindevkit/bdk_wallet#30 for Esplora
Notes to the reviewers
Open to any and all feedback on this.
Other PR's made along these lines:
unwrap()s andexpect()s #1981 (electrum)chain_updateerrors if no point of connection #1971 (esplora)Changelog notice
bdk_esplora.rs.Checklists
All Submissions:
New Features:
Bugfixes: