Skip to content
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

Check recent blocks are not empty #1725

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

Check recent blocks are not empty #1725

wants to merge 1 commit into from
+6 −0

Conversation

rustaceanrob
Copy link
Contributor

Closes #1696

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo 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
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@notmandatory notmandatory added module-blockchain audit Suggested as result of external code audit labels Nov 18, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone Nov 18, 2024
oleonardolima
oleonardolima reviewed Nov 19, 2024
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

It looks good, but it seems to partially address the problem.

I'm trying to wrap my mind on the behavior when the latest_blocks is empty. I wonder if it shouldn't be handled on the upstream fns too?

Otherwise, the same rogue Esplora server would still be able to crash the application as we expect to have a valid point of agreement here:

bdk/crates/esplora/src/blocking_ext.rs

Line 225 in 3bc45b5

let mut tip = point_of_agreement.expect("remote esplora should have same genesis block");

@rustaceanrob
Copy link
Contributor Author

That's a good point. I think these is_empty checks should simply be done in the first line of chain_update and this would be sufficient.

oleonardolima
oleonardolima reviewed Nov 19, 2024
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

Concept ACK f3efbda

notmandatory
notmandatory approved these changes Nov 21, 2024
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

utACK f3efbda

@notmandatory
Copy link
Member

notmandatory commented Nov 21, 2024 •
edited
Loading

Can you add a simple test to exercise this condition in case we change the fix implementation later we make sure this remains fixed.

I also had some discussion with @oleonardolima and @ValuedMammal and it would be good to have the test first and make sure without any changes this does in fact gets to and fails an at expect.

@rustaceanrob
Copy link
Contributor Author

I can try to work on that tomorrow. Heads up I will not be able to work on much next week

@ValuedMammal
Copy link
Contributor

Since the checkpoint is an optional field of the sync/scan response it may also make sense to have chain_update return a Result<Option<CheckPoint>, Error>.

@rustaceanrob
Copy link
Contributor Author

I was thinking that this should be an error variant in the Esplora client, as it should not ever respond with empty blocks when requested. I am unable to do much this week but I can review any new PRs that replace this one

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

@oleonardolima oleonardolima oleonardolima left review comments

@notmandatory notmandatory notmandatory approved these changes

Assignees

@rustaceanrob rustaceanrob

Labels
audit Suggested as result of external code audit module-blockchain
Projects
BDK
Status: Needs Review
Milestone
1.0.0-beta
Development

Successfully merging this pull request may close these issues.

bdk_esplora: handle empty /blocks response
4 participants
@rustaceanrob @notmandatory @ValuedMammal @oleonardolima