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

Jump to bottom
Open
reez wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor(esplora): clear remaining panic paths #2053

reez wants to merge 1 commit into from

Conversation

@reez
Copy link

@reez reez commented Oct 1, 2025 •
edited
Loading

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:

Changelog notice

  • Clear remaining panic paths from bdk_esplora.rs.

Checklists

All Submissions:

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
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

ValuedMammal
ValuedMammal reviewed Oct 1, 2025
View reviewed changes
@oleonardolima oleonardolima moved this to In Progress in BDK Chain Oct 1, 2025
@reez reez mentioned this pull request Oct 2, 2025
Open
@reez reez marked this pull request as ready for review October 6, 2025 16:48
ValuedMammal
ValuedMammal reviewed Oct 6, 2025
View reviewed changes
@reez reez changed the title (draft) refactor(esplora): clear remaining panic paths refactor(esplora): clear remaining panic paths Oct 9, 2025
evanlinjin
evanlinjin requested changes Oct 17, 2025
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

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).

reez
reez commented Oct 22, 2025
View reviewed changes
reez
reez commented Oct 22, 2025
View reviewed changes
reez
reez commented Oct 22, 2025
View reviewed changes
reez
reez commented Oct 22, 2025
View reviewed changes
reez
reez commented Oct 22, 2025
View reviewed changes
reez
reez commented Oct 22, 2025
View reviewed changes
reez
reez commented Oct 22, 2025
View reviewed changes
reez
reez commented Oct 22, 2025
View reviewed changes
@reez
Copy link
Author

reez commented Oct 30, 2025

Updated PR due to great feedback from Evan, Mammal, Leonardo.

Noteworthy:

  • Introduced a crate-wide Error enum (in lib.rs) with Client and Checkpoint variants. Both async and blocking extensions now return this so real Esplora failures bubble up via Client while checkpoint ordering issues surface as Checkpoint instead of panicking or masquerading as InvalidResponse
  • Removed the custom ThreadPanic and reverted as suggested so we only guard against genuine remote errors
  • Dropped the processed_any guard added previously
  • Eliminated tests that provided no major value

evanlinjin
evanlinjin requested changes Nov 9, 2025
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Avoid returning errors for internal logic bugs in BDK itself.

crates/esplora/src/lib.rs Outdated
Comment on lines 42 to 45
pub enum Error {
Client(esplora_client::Error),
Checkpoint(CheckPoint<BlockHash>),
}
Copy link
Member

@evanlinjin evanlinjin Nov 9, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

@reez reez Nov 18, 2025

Choose a reason for hiding this comment

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

sounds good, Error type removed

crates/esplora/src/blocking_ext.rs Outdated
Comment on lines 245 to 247
tip = tip
.extend(conflicts.into_iter().rev().map(|b| (b.height, b.hash)))
.expect("evicted are in order");
.map_err(Error::Checkpoint)?;
Copy link
Member

@evanlinjin evanlinjin Nov 9, 2025

Choose a reason for hiding this comment

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

I still think panic/expect here is more appropriate.

  1. CheckPoint::extend only errors if a block is not higher than the previous.
  2. The logic to populate conflicts uses heights from local_tip. local_tip is guaranteed to have ordered heights (since it is a CheckPoint type).

Copy link
Author

@reez reez Nov 18, 2025

Choose a reason for hiding this comment

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

makes sense, I reverted this so it uses expect again

@reez reez force-pushed the esplora branch 3 times, most recently from a5bf97f to d4244e3 Compare November 13, 2025 22:48
@reez
Copy link
Author

reez commented Nov 18, 2025

I believe all review comments are addressed now; to recap current state this PR adds logic around removing panic tied to remote input, while leaving panics where appropriate based on PR review feedback. Thanks again for taking time to review this as I iterated on it (happy to continue tweaking if needed)!

@oleonardolima oleonardolima mentioned this pull request Nov 20, 2025
Open
3 tasks
ValuedMammal
ValuedMammal reviewed Dec 2, 2025
View reviewed changes
Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK d4244e3

Note: I think it would make sense to use gap_limit = stop_gap.max(parallel_requests). Related discussion #2053 (comment) and bitcoindevkit/bdk_wallet#63.

@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Chain Dec 2, 2025
@ValuedMammal ValuedMammal added this to the Chain 0.24.0 milestone Dec 2, 2025
@ValuedMammal ValuedMammal added the bug Something isn't working label Dec 2, 2025
@reez
Copy link
Author

reez commented Dec 2, 2025

ACK d4244e3

Note: I think it would make sense to use gap_limit = stop_gap.max(parallel_requests). Related discussion #2053 (comment) and bitcoindevkit/bdk_wallet#63.

@ValuedMammal I just updated PR to use gap_limit = stop_gap.max(parallel_requests) now 👍

@ValuedMammal
Copy link
Collaborator

ValuedMammal commented Dec 2, 2025 •
edited
Loading

Need to debug a CI fail. Maybe try rebasing this PR onto master.

Edit: As a workaround, pinning the rust toolchain to nightly-2025-11-26 seems to have fixed it https://github.com/ValuedMammal/bdk/actions/runs/19878874973/job/56972371610, although I haven't pinpointed the exact issue.

ACK 074aed1

@thunderbiscuit
Copy link
Member

thunderbiscuit commented Dec 10, 2025

This is a bit of a blocker for the 2.3 release of bdk-ffi, I'd like to include it in there if possible. @reez do you mind trying the pin workaround proposed by @ValuedMammal?

@reez
Copy link
Author

reez commented Dec 10, 2025

This is a bit of a blocker for the 2.3 release of bdk-ffi, I'd like to include it in there if possible. @reez do you mind trying the pin workaround proposed by @ValuedMammal?

@thunderbiscuit sure let me give my context

@ValuedMammal has ACK’d this PR a few times, so after his last comment #2053 (comment) and because I didn’t want him to have to keep review+ACK’ing if no change actually needed on this PR I pinged him to ask if he wanted me to try either/both of those in this PR or keep the PR as is. My understanding is keep the PR as is (and maybe there would be a new issue opened scoped to resolve the coverage job), so I explicitly did not do the pin workaround for those reasons.

Additionally @oleonardolima has reviewed this previously and also let me know he’d be giving this PR a final review too, I know he’s been traveling a bunch though so there’s no rush from me on that.

Then from the bdk-ffi side of things my view point is this would be nice-to-have but is not a blocker for 2.3 release.

@ValuedMammal
Copy link
Collaborator

ValuedMammal commented Dec 10, 2025

and maybe there would be a new issue opened scoped to resolve the coverage job

The CI fix is now in a separate PR #2080.

@reez
Copy link
Author

reez commented Dec 10, 2025

#2080

Thanks for the heads up!

I'll try to keep an eye on that PR so when it gets merged I can rebase this PR on it if needed for this PR to be merged.

@reez
Copy link
Author

reez commented Dec 12, 2025

#2080

Thanks for the heads up!

I'll try to keep an eye on that PR so when it gets merged I can rebase this PR on it if needed for this PR to be merged.

Rebased PR on master now that #2080 was merged, CI passing ✅

ValuedMammal
ValuedMammal approved these changes Dec 12, 2025
View reviewed changes
Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK 92dc6dd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ValuedMammal ValuedMammal ValuedMammal approved these changes

@evanlinjin evanlinjin Awaiting requested review from evanlinjin

@oleonardolima oleonardolima Awaiting requested review from oleonardolima

Requested changes must be addressed to merge this pull request.

Assignees

@reez reez

Labels

bug Something isn't working

Projects

BDK Chain
Status: Needs Review

Milestone

Chain 0.24.0

Development

Successfully merging this pull request may close these issues.

4 participants

@reez @ValuedMammal @thunderbiscuit @evanlinjin