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.

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

Simplify chain update #9

Open
wants to merge 8 commits into
base: clean_esplora
Choose a base branch
from

Conversation

LLFourn
Copy link

@LLFourn LLFourn commented Apr 2, 2024

No description provided.

These methods allow us to query for checkpoints contained within the
linked list by height. This is useful to determine checkpoints to fetch
for chain sources without having to refer back to the `LocalChain`.

Currently this is not implemented efficiently, but in the future, we
will change `CheckPoint` to use a skip list structure.
We impl `PartialEq` on `CheckPoint` instead of directly on `LocalChain`.
We also made the implementation more efficient.
This creates a checkpoint linked list which contains all blocks.
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.
These methods are no longer needed as we can determine missing heights
directly from the `CheckPoint` tip.
This gets the genesis hash of the env blockchain.
We ensure that calling `finalize_chain_update` does not result in a
chain which removed previous heights and all anchor heights are
included.
I felt things didn't have to be so complicated. The chain could easily
be updated in one function rather than spread across two. The logic
needs at least one less loop.
Copy link
Owner

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

This looks really good. Thank you for showing me a much cleaner approach. My only concern is the following:

https://github.com/bitcoindevkit/bdk/blob/e5c81a231b15118f41a1fcb97c527008efc4c5a9/crates/esplora/src/blocking_ext.rs#L146-L150

The reason why I split chain_update_... into two methods is we want to fetch latest blocks before getting transaction update. We are using the local tip to determine "last-scanned block" and there are users which want to alternate between Esplora and block-by-block sources.

I will try update this to fetch latest blocks before getting transactions.

@LLFourn
Copy link
Author

LLFourn commented Apr 15, 2024

This looks really good. Thank you for showing me a much cleaner approach. My only concern is the following:

https://github.com/bitcoindevkit/bdk/blob/e5c81a231b15118f41a1fcb97c527008efc4c5a9/crates/esplora/src/blocking_ext.rs#L146-L150

The reason why I split chain_update_... into two methods is we want to fetch latest blocks before getting transaction update.

I don't think so. We want these two things to be completely decoupled. A chain update is orthogonal to the transaction update except that you want to try and grab heights that transactions seem to exist at. I didn't understand the motivation you gave. Why can't you switch between block-by-block and esplora here?

@evanlinjin evanlinjin force-pushed the clean_esplora branch 4 times, most recently from a504a1d to 77d3595 Compare April 17, 2024 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants