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

Simplify Esplora::update_local_chain and add tests #1267

Merged
merged 6 commits into from
Jan 31, 2024
Merged

Simplify Esplora::update_local_chain and add tests #1267

merged 6 commits into from
Jan 31, 2024

Conversation

evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented Jan 11, 2024 •
edited

Fixes #1199

Description

After a second look at the update_local_chain implementations, it was clear that they were over complicated. This PR simplifies the EsploraExt::update_local_chain method(s) of the bdk_esplora crate and adds a whole bunch of tests.

Notes to the reviewers

The description of #1199 is very brief, however @danielabrozzoni mentioned about potentially-problematic logic with ASSUME_FINAL_DEPTH. The logic was indeed convoluted and it did not need to be that way. This PR removes the need for ASSUME_FINAL_DEPTH.

Changelog notice

Fixed

  • Simplified EsploraExt::update_local_chain logic.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Bugfixes:

* [ ] This pull request breaks the existing API
* [ ] I've added tests to reproduce the issue which are now passing (there are now lots of tests though)

  • I'm linking the issue being fixed by this PR

@evanlinjin evanlinjin self-assigned this Jan 11, 2024
@evanlinjin evanlinjin marked this pull request as ready for review January 11, 2024 17:54
@evanlinjin evanlinjin changed the title Esplora improvements. Simplify Esplora::update_local_chain and add tests. Jan 11, 2024
@nondiremanuel nondiremanuel added this to the 1.0.0 milestone Jan 12, 2024
@evanlinjin evanlinjin force-pushed the issue/1199 branch 2 times, most recently from 810e9c7 to 6fd083e Compare January 15, 2024 16:15
@notmandatory notmandatory added the api A breaking API change label Jan 16, 2024
@evanlinjin
Copy link
Member Author

@notmandatory this doesn't really change the API

LLFourn
LLFourn reviewed Jan 19, 2024
View reviewed changes
Copy link
Contributor

@LLFourn LLFourn 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. One thing looks like it needs to be updated.

crates/esplora/src/async_ext.rs Outdated Show resolved Hide resolved
LLFourn
LLFourn reviewed Jan 22, 2024
View reviewed changes
Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

I found some more things that could be simplified as part of this.

crates/esplora/src/async_ext.rs Outdated Show resolved Hide resolved
crates/esplora/src/async_ext.rs Outdated Show resolved Hide resolved
@evanlinjin evanlinjin removed the api A breaking API change label Jan 29, 2024
@evanlinjin evanlinjin changed the title Simplify Esplora::update_local_chain and add tests. Simplify Esplora::update_local_chain and add tests Jan 29, 2024
@LLFourn
Copy link
Contributor

LLFourn commented Jan 30, 2024

self-ACK: 216648b

Pushed a comment and docs better explaining what @evanlinjin was trying to explain to me here: #1267 (comment)

evanlinjin
evanlinjin commented Jan 30, 2024
View reviewed changes
Copy link
Member Author

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

ACK 216648b

Thank you @LLFourn for the improved docs.

danielabrozzoni
danielabrozzoni reviewed Jan 30, 2024
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

A few nits, otherwise LGTM

crates/esplora/src/async_ext.rs Outdated Show resolved Hide resolved
crates/esplora/src/async_ext.rs Show resolved Hide resolved
crates/esplora/src/async_ext.rs Outdated Show resolved Hide resolved
crates/esplora/src/async_ext.rs
})
};

// We have found point of agreement so the update will connect!
Copy link
Member

Choose a reason for hiding this comment

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

What if no point of agreement is found? Is this possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should never happen because the genesis block must connect! However, let's say hypothetically, the genesis doesn't connect (because the wallet and esplora are on different networks), the update formed will try replace the genesis, resulting in the local_chain::MissingGenesisError which is the error that we want!

crates/esplora/tests/blocking_ext.rs Outdated Show resolved Hide resolved
crates/esplora/tests/blocking_ext.rs Outdated Show resolved Hide resolved
@notmandatory notmandatory mentioned this pull request Jan 30, 2024
Closed
23 tasks
notmandatory
notmandatory reviewed Jan 31, 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.

Found a couple typos that should be fixed.

crates/esplora/src/async_ext.rs Outdated Show resolved Hide resolved
crates/esplora/src/blocking_ext.rs Outdated Show resolved Hide resolved
@notmandatory
Copy link
Member

To fix CI in cont_integration.yml try replacing:

          cargo update -p zip --precise "0.6.2"

with:

          cargo update -p zstd-sys --precise "2.0.8+zstd.1.5.5"`

evanlinjin
evanlinjin commented Jan 31, 2024
View reviewed changes
Copy link
Member Author

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

ACK c5afbaa

Thank you @notmandatory for spotting the typos and fixing the CI!

notmandatory
notmandatory approved these changes Jan 31, 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 c5afbaa

@notmandatory notmandatory merged commit 8ad52f7 into bitcoindevkit:master Jan 31, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notmandatory notmandatory notmandatory approved these changes

@LLFourn LLFourn Awaiting requested review from LLFourn

@danielabrozzoni danielabrozzoni Awaiting requested review from danielabrozzoni

Assignees

@evanlinjin evanlinjin

Labels
None yet
Projects
BDK
Archived in project
Milestone
1.0.0-alpha
Development

Successfully merging this pull request may close these issues.

bdk + esplora update_local_chain logic might be off
5 participants
@evanlinjin @LLFourn @notmandatory @danielabrozzoni @nondiremanuel