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

Simplified EsploraExt API #1380

Merged
merged 13 commits into from Apr 22, 2024
Merged

Simplified EsploraExt API #1380

merged 13 commits into from Apr 22, 2024

Conversation

evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented Mar 21, 2024 •
edited

Fixes #1354

Description

Built on top of both #1369 and #1373, we simplify the EsploraExt API by removing the update_local_chain method and having full_scan and sync update the local chain in the same call. The full_scan and sync methods now takes in an additional input (local_tip) which provides us with the view of the LocalChain before the update. These methods now return structs FullScanUpdate and SyncUpdate.

The examples are updated to use this new API. TxGraph::missing_heights and tx_graph::ChangeSet::missing_heights_from are no longer needed, therefore they are removed.

Additionally, we used this opportunity to simplify the logic which updates LocalChain. We got rid of the local_chain::Update struct (which contained the update CheckPoint tip and a bool which signaled whether we want to introduce blocks below point of agreement). It turns out we can use something like CheckPoint::insert so the chain source can craft an update based on the old tip. This way, we can make better use of merge_chains' optimization that compares the Arc pointers of the local and update chain (before we were crafting the update chain NOT based on top of the previous local chain). With this, we no longer need the Update::introduce_older_block field since the logic will naturally break when we reach a matching Arc pointer.

Notes to the reviewers

  • Obtaining the LocalChain's update now happens within EsploraExt::full_scan and EsploraExt::sync. Creating the LocalChain update is now split into two methods (fetch_latest_blocks and chain_update) that are called before and after fetching transactions and anchors.
  • We need to duplicate code for bdk_esplora. One for blocking and one for async.

Changelog notice

  • Changed EsploraExt API so that sync only requires one round of fetching data. The local_chain_update method is removed and the local_tip parameter is added to the full_scan and sync methods.
  • Removed TxGraph::missing_heights and tx_graph::ChangeSet::missing_heights_from methods.
  • Introduced CheckPoint::insert which allows convenient checkpoint-insertion. This is intended for use by chain-sources when crafting an update.
  • Refactored merge_chains to also return the resultant CheckPoint tip.
  • Optimized the update LocalChain logic - use the update CheckPoint as the new CheckPoint tip when possible.

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

@notmandatory notmandatory added this to the 1.0.0-alpha milestone Mar 21, 2024
@notmandatory notmandatory added the api A breaking API change label Mar 21, 2024
@evanlinjin evanlinjin force-pushed the clean_esplora branch 11 times, most recently from 1bb6193 to abddc33 Compare March 26, 2024 15:00
@evanlinjin evanlinjin marked this pull request as ready for review March 26, 2024 15:01
@evanlinjin
Copy link
Member Author

@notmandatory ready for you to base #1194 on!

LLFourn
LLFourn reviewed Mar 27, 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.

Left some comments so I can understand a few things better before giving a full review.

crates/esplora/src/blocking_ext.rs Outdated Show resolved Hide resolved
crates/esplora/src/blocking_ext.rs Outdated Show resolved Hide resolved
crates/esplora/src/blocking_ext.rs Outdated Show resolved Hide resolved
crates/esplora/src/blocking_ext.rs Outdated Show resolved Hide resolved
crates/esplora/src/blocking_ext.rs Outdated Show resolved Hide resolved
@LLFourn LLFourn mentioned this pull request Mar 27, 2024
Merged
5 tasks
@notmandatory
Copy link
Member

@notmandatory ready for you to base #1194 on!

I finished rebasing #1194 on this PR and everything is working fine.

@notmandatory notmandatory mentioned this pull request Mar 29, 2024
Closed
8 tasks
@evanlinjin
Copy link
Member Author

@notmandatory I forgot to mention I also need to rebase this PR on master 😬

@evanlinjin evanlinjin force-pushed the clean_esplora branch 5 times, most recently from e59a142 to a9aa7c4 Compare April 2, 2024 13:23
@evanlinjin evanlinjin force-pushed the clean_esplora branch 2 times, most recently from eaebdee to e5c81a2 Compare April 12, 2024 04:07
@LagginTimes LagginTimes mentioned this pull request Apr 12, 2024
Merged
5 tasks
evanlinjin and others added 7 commits April 16, 2024 17:51
@evanlinjin
feat(testenv): add make_checkpoint_tip
1e99793 
This creates a checkpoint linked list which contains all blocks.
@evanlinjin
feat(esplora)!: remove EsploraExt::update_local_chain
bd62aa0 
Previously, we would update the `TxGraph` and `KeychainTxOutIndex`
first, then create a second update for `LocalChain`. This required
locking the receiving structures 3 times (instead of twice, which
is optimal).

This PR eliminates this requirement by making use of the new `query`
method of `CheckPoint`.

Examples are also updated to use the new API.
@evanlinjin
chore(chain)!: rm missing_heights and missing_heights_from methods
886d72e 
These methods are no longer needed as we can determine missing heights
directly from the `CheckPoint` tip.
@evanlinjin
feat(testenv): add genesis_hash method
494d253 
This gets the genesis hash of the env blockchain.
@evanlinjin
test(esplora): add test_finalize_chain_update
a6e613e 
We ensure that calling `finalize_chain_update` does not result in a
chain which removed previous heights and all anchor heights are
included.
@evanlinjin
test(esplora): move esplora tests into src files
519cd75 
Since we want to keep these methods private.
@evanlinjin @LLFourn
feat(chain): introduce CheckPoint::insert
eded1a7 
Co-authored-by: LLFourn <lloyd.fourn@gmail.com>
@evanlinjin @LLFourn
feat(esplora)!: simplify chain update logic
72fe65b 
Co-authored-by: LLFourn <lloyd.fourn@gmail.com>
@evanlinjin evanlinjin force-pushed the clean_esplora branch 2 times, most recently from 690cccb to a504a1d Compare April 17, 2024 02:56
@evanlinjin
feat(chain)!: rm local_chain::Update
77d3595 
The intention is to remove the `Update::introduce_older_blocks`
parameter and update the local chain directly with `CheckPoint`.

This simplifies the API and there is a way to do this efficiently.
@evanlinjin @ValuedMammal
feat(chain): refactor merge_chains
96a9aa6 
`merge_chains` now returns a tuple of the resultant checkpoint AND
changeset. This is arguably a more readable/understandable setup.

To do this, we had to create `CheckPoint::apply_changeset` which is kept
as a private method.

Thank you @ValuedMammal for the suggestion.

Co-authored-by: valuedvalued mammal <valuedmammal@protonmail.com>
LLFourn
LLFourn approved these changes Apr 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.

ACK 96a9aa6

@evanlinjin evanlinjin merged commit 8e73998 into bitcoindevkit:master Apr 22, 2024
12 checks passed
@danielabrozzoni danielabrozzoni mentioned this pull request May 2, 2024
Closed
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValuedMammal ValuedMammal ValuedMammal left review comments

@LLFourn LLFourn LLFourn approved these changes

@notmandatory notmandatory Awaiting requested review from notmandatory

Assignees

@evanlinjin evanlinjin

Labels
api A breaking API change module-blockchain
Projects
BDK
Archived in project
Milestone
1.0.0-alpha
Development

Successfully merging this pull request may close these issues.

The TxGraph::missing_heights implementation is problematic.
4 participants
@evanlinjin @notmandatory @LLFourn @ValuedMammal