Implement linked-list `LocalChain` and update chain-src crates/examples #1034

evanlinjin · 2023-07-19T10:27:52Z

Fixes #997
Replaces #1002

Description

This PR changes the LocalChain implementation to have blocks stored as a linked-list. This allows the data-src thread to hold a shared ref to a single checkpoint and have access to the whole history of checkpoints without cloning or keeping a lock on LocalChain.

The APIs of bdk::Wallet, esplora and electrum are also updated to reflect these changes. Note that the esplora crate is rewritten to anchor txs in the confirmation block (using the esplora API's tx status block_hash). This guarantees 100% consistency between anchor blocks and their transactions (instead of anchoring txs to the latest tip). ExploraExt now has separate methods for updating the TxGraph and LocalChain.

A new method TxGraph::missing_blocks is introduced for finding "floating anchors" of a TxGraph update (given a chain).

Additional changes:

test_local_chain.rs is refactored to make test cases easier to write. Additional tests are also added.
Examples are updated.
Exclude example-cli *.db files in .gitignore.
Rm duplicate bdk_tmp_plan module.

Notes to the reviewers

This is the smallest possible division of #1002 without resulting in PRs that do not compile. Since we have changed the API of LocalChain, we also need to change esplora, electrum crates and examples alongside bdk::Wallet.

Changelog notice

Implement linked-list LocalChain. This allows the data-src thread to hold a shared ref to a single checkpoint and have access to the whole history of checkpoints without cloning or keeping a lock on LocalChain.
Rewrote esplora chain-src crate to anchor txs to their confirmation blocks (using esplora API's tx-status block_hash).

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

LLFourn

Looks great. A few API things to touch up in my view.

crates/bdk/Cargo.toml

crates/chain/src/local_chain.rs

crates/chain/src/tx_graph.rs

crates/chain/src/local_chain.rs

example-crates/wallet_esplora_async/src/main.rs

vladimirfomene

Just a few nits and few questions.

crates/chain/src/keychain.rs

crates/chain/src/local_chain.rs

crates/chain/src/tx_graph.rs

crates/chain/tests/test_local_chain.rs

crates/electrum/src/electrum_ext.rs

crates/esplora/src/async_ext.rs

crates/esplora/src/lib.rs

vladimirfomene

Just nits:

crates/chain/src/keychain.rs

evanlinjin · 2023-07-26T07:05:51Z

Something that I need help with is how we should test the bdk_esplora crate. Should we do a full blown integration-test environment? What's the best way to spin up an esplora backend? Docker? Or should we mock up the esplora API?

vladimirfomene · 2023-07-26T07:47:41Z

Something that I need help with is how we should test the bdk_esplora crate. Should we do a full blown integration-test environment? What's the best way to spin up an esplora backend? Docker? Or should we mock up the esplora API?

Using electrsd in conjunction with bitcoind allows us to have integration test environment both for Esplora and Electrum.

crates/chain/src/tx_graph.rs

crates/chain/tests/test_tx_graph.rs

This commit changes the `LocalChain` implementation to have blocks stored as a linked-list. This allows the data-src thread to hold a shared ref to a single checkpoint and have access to the whole history of checkpoints without cloning or keeping a lock on `LocalChain`. The APIs of `bdk::Wallet`, `esplora` and `electrum` are also updated to reflect these changes. Note that the `esplora` crate is rewritten to anchor txs in the confirmation block (using the esplora API's tx status block_hash). This guarantees 100% consistency between anchor blocks and their transactions (instead of anchoring txs to the latest tip). `ExploraExt` now has separate methods for updating the `TxGraph` and `LocalChain`. A new method `TxGraph::missing_blocks` is introduced for finding "floating anchors" of a `TxGraph` update (given a chain). Additional changes: * `test_local_chain.rs` is refactored to make test cases easier to write. Additional tests are also added. * Examples are updated. * Fix `tempfile` dev dependency of `bdk_file_store` to work with MSRV Co-authored-by: LLFourn <[email protected]>

@LLFourn

Shout out to @LLFourn for these suggestions. * Improve/fix `LocalChain` documentation * Refactor `TxGraph::missing_blocks` to make it more explicit that `last_block` has state. * `update_local_chain` method of `EsploraExt` and `EsploraAsyncExt` now returns a `local_chain::Update` instead of just a `CheckPoint`.

@vladimirfomene

Thank you @vladimirfomene for these suggestions. * Rename `LocalUpdate::keychain` to `LocalUpdate::last_active_indices`. * Change docs for `CheckPoint` to be more descriptive. * Fix incorrect logic in `update_local_chain` for `EsploraExt` and `EsploraAsyncExt`.

Added additional tests for unnecessary duplicate heights

danielabrozzoni

I reviewed the LocalChain code and left a few questions.

No need to wait for my complete review before merging, I don't think I can provide a meaningful review or a sensible ACK

crates/chain/src/local_chain.rs

danielabrozzoni · 2023-07-31T14:23:11Z

crates/chain/src/local_chain.rs

-            self.0
-        )
+    /// Iterate over checkpoints in decending height order.
+    pub fn iter_checkpoints(&self) -> CheckPointIter {


Do you think it makes sense to have the method name reflect the fact that the checkpoints are iterated backwards, from highest to lowest height?

I think that will provide better clarity. Do you have any suggestions? @LLFourn what are your thoughts?

Delete the method. You can just get the tip and iter backwards right?

[edit] oh but I see that since the tip in an Option that's annoying. I think documenting the order is fine for me.

crates/chain/src/local_chain.rs

LLFourn

Thanks. Made some mostly nits and documentation suggestions.

crates/chain/src/local_chain.rs

crates/bdk/src/wallet/mod.rs

example-crates/wallet_esplora_async/src/main.rs

LLFourn · 2023-08-01T09:06:25Z

example-crates/example_electrum/src/main.rs

-            let (_, index_additions) = graph.index.reveal_to_target_multi(&final_update.keychain);
+            let (_, index_additions) = graph
+                .index
+                .reveal_to_target_multi(&final_update.last_active_indices);


This is not specifically to do with this PR. I think we are making suboptimal use of our API. Only in scan do we need to do this work so it should only occur in that branch to make that clear to the user. The changes can be staged there and only committed out here.

With sync, it's still possible to find a transaction that will increase last active indices right?

No.

[edit] ok yes. But the point is we never need to do reveal_to_target_multi in sync. I want to get across that reveal_to_target_multi is a specialized operation that you won't be doing during normal wallet operation.

@LLFourn are you saying we should depend on lookahead + scanning in the transactions?

In sync you don't need to "look ahead" you are only scanning the addresses you've already revealed.

@LLFourn

Thank you to @LLFourn and @danielabrozzoni for these suggestions.

LLFourn · 2023-08-02T05:19:26Z

ACK b206a98

LLFourn

Sorry forgot to approve

f41cc1c fix: s/index_tx_graph/indexed_tx_graph/g (LLFourn) da8cfd3 feat: add cli example for `esplora` (志宇) Pull request description: ### Description This PR builds on top of #1034 and adds a cli-example for our `esplora` chain-src crate. ### Notes to the reviewers Don't merge this until #1034 is merged. The only relevant commit is 5ff0412. ### Changelog notice * Add cli-example for `esplora`. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: ~* [ ] I've added tests for the new feature~ * [x] I've added docs for the new feature ACKs for top commit: danielabrozzoni: ACK f41cc1c notmandatory: ACK f41cc1c Tree-SHA512: a41fa456a9509f75feea0af013deaaad846cc6b60e5e6671672630a716e8c962361cbc9bb2d62c68e96d5fdb9e580912c19ff5fcab1acaf604b5b4a10eb40cee

85c6253 docs(bitcoind_rpc): better `Emitter::mempool` explanation (志宇) b69c13d example_bitcoind_rpc: tweaks (志宇) 5f34df8 bitcoind_rpc!: bring back `CheckPoint`s to `Emitter` (志宇) 57590e0 bitcoind_rpc: rm `BlockHash` from `Emitter::last_mempool_tip` (志宇) 6d4b33e chain: split `IndexedTxGraph::insert_tx` into 3 methods (志宇) 4f5695d chain: improvements to `IndexedTxGraph` and `TxGraph` APIs (志宇) 150d6f8 feat(example_bitcoind_rpc_polling): add example for RPC polling (志宇) 4f10463 test(bitcoind_rpc): add no_agreement_point test (志宇) a73dac2 test(bitcoind_rpc): initial tests for `Emitter` (志宇) bb7424d feat(bitcoind_rpc): introduce `bitcoind_rpc` crate (志宇) 240657b chain: add batch-insert methods for `IndexedTxGraph` (志宇) 43bc813 chain: add helper methods on `CheckPoint` (志宇) b3db5ca feat(chain): add `AnchorFromBlockPosition` trait (志宇) f795a43 feat(example_cli): allow chain specific args in examples (志宇) Pull request description: ### Description This PR builds on top of #1034 and adds the `bitcoind_rpc` chain-src module and example. ### Notes to the reviewers Don't merge this until #1034 is in! ### Changelog notice * Add `bitcoind_rpc` chain-source module. * Add `example_bitcoind_rpc` example module. * Add `AnchorFromBlockPosition` trait which are for anchors that can be constructed from a given block, height and position in block. * Add helper methods to `IndexedTxGraph` and `TxGraph` for batch operations and applying blocks directly. * Add helper methods to `CheckPoint` for easier construction from a block `Header`. ### Checklists * [x] Add test: we should detect when an initially-confirmed transaction is "unconfirmed" during a reorg. * [x] Improve `example_bitcoind_rpc`: add `live` command. * [x] Improve docs. * [x] Reintroduce `CheckPoint`. #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [x] I've added tests for the new feature * [x] I've added docs for the new feature ACKs for top commit: notmandatory: Re ACK 85c6253 Tree-SHA512: 88dbafbebaf227b18c69f2ea884e3e586bf9c11e5e450eb4872ade1d1ccd5cf1e33ce9930a6f5aa918baa3e92add7503858b039b8c9d553a281ad6d833f08a49

evanlinjin requested review from danielabrozzoni and LLFourn July 19, 2023 10:29

notmandatory assigned evanlinjin Jul 19, 2023

notmandatory added this to the 1.0.0-alpha.2 milestone Jul 19, 2023

This was referenced Jul 20, 2023

Add cli esplora example #1040

Merged

Add bitcoind_rpc chain source module. #1041

Merged

LLFourn requested changes Jul 20, 2023

View reviewed changes

vladimirfomene reviewed Jul 20, 2023

View reviewed changes

evanlinjin force-pushed the local_chain_linked_list branch 12 times, most recently from 56d7c52 to 8220573 Compare July 24, 2023 07:51

vladimirfomene approved these changes Jul 25, 2023

View reviewed changes

crates/chain/src/keychain.rs Outdated Show resolved Hide resolved

evanlinjin requested a review from LLFourn July 26, 2023 07:06

LLFourn reviewed Jul 26, 2023

View reviewed changes

crates/chain/src/tx_graph.rs Show resolved Hide resolved

crates/chain/src/tx_graph.rs Outdated Show resolved Hide resolved

crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved

crates/chain/tests/test_tx_graph.rs Outdated Show resolved Hide resolved

evanlinjin force-pushed the local_chain_linked_list branch 2 times, most recently from a3dca14 to bef9b0b Compare July 28, 2023 03:27

evanlinjin and others added 3 commits July 28, 2023 11:30

Add exclusion of example cli *.db files in .gitignore

af705da

fix: rm duplicate bdk_tmp_plan module

315e7e0

evanlinjin added 2 commits July 28, 2023 11:30

Refactor debug_assertions checks for LocalChain

8bf7a99

evanlinjin force-pushed the local_chain_linked_list branch from bef9b0b to 00e8a47 Compare July 28, 2023 03:30

evanlinjin added 2 commits July 28, 2023 11:37

fix: TxGraph::missing_blocks logic

bea8e5a

Added additional tests for unnecessary duplicate heights

evanlinjin force-pushed the local_chain_linked_list branch from 00e8a47 to bea8e5a Compare July 28, 2023 03:37

evanlinjin requested a review from LLFourn July 28, 2023 03:37

nondiremanuel mentioned this pull request Jul 29, 2023

W31 Lib Team Call bitcoindevkit/.github#70

Closed

19 tasks

evanlinjin mentioned this pull request Jul 31, 2023

Implement linked-list LocalChain and add rpc-chain module/example #1002

Closed

7 tasks

danielabrozzoni reviewed Jul 31, 2023

View reviewed changes

LLFourn requested changes Aug 1, 2023

View reviewed changes

fix: Even more refactoring to code and documentation

b206a98

Thank you to @LLFourn and @danielabrozzoni for these suggestions.

evanlinjin force-pushed the local_chain_linked_list branch from f258599 to b206a98 Compare August 1, 2023 10:36

LLFourn approved these changes Aug 3, 2023

View reviewed changes

evanlinjin merged commit d73669e into bitcoindevkit:master Aug 3, 2023

notmandatory mentioned this pull request Aug 7, 2023

Remove TransactionDetails from Wallet API #1048

Merged

5 tasks

darosior mentioned this pull request Aug 7, 2023

Introduce new bdk_coin_select implementation #924

Closed

nondiremanuel mentioned this pull request Aug 8, 2023

W33 Lib Team Call bitcoindevkit/.github#72

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement linked-list `LocalChain` and update chain-src crates/examples #1034

Implement linked-list `LocalChain` and update chain-src crates/examples #1034

evanlinjin commented Jul 19, 2023 •

edited

Loading

LLFourn left a comment

vladimirfomene left a comment

vladimirfomene left a comment

evanlinjin commented Jul 26, 2023

vladimirfomene commented Jul 26, 2023

danielabrozzoni left a comment

danielabrozzoni Jul 31, 2023

evanlinjin Aug 1, 2023

LLFourn Aug 2, 2023 •

edited

Loading

LLFourn left a comment

LLFourn Aug 1, 2023

evanlinjin Aug 1, 2023

LLFourn Aug 2, 2023 •

edited

Loading

evanlinjin Aug 2, 2023 •

edited

Loading

LLFourn Aug 3, 2023

LLFourn commented Aug 2, 2023

LLFourn left a comment

Implement linked-list LocalChain and update chain-src crates/examples #1034

Implement linked-list LocalChain and update chain-src crates/examples #1034

Conversation

evanlinjin commented Jul 19, 2023 • edited Loading

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

LLFourn left a comment

Choose a reason for hiding this comment

vladimirfomene left a comment

Choose a reason for hiding this comment

vladimirfomene left a comment

Choose a reason for hiding this comment

evanlinjin commented Jul 26, 2023

vladimirfomene commented Jul 26, 2023

danielabrozzoni left a comment

Choose a reason for hiding this comment

danielabrozzoni Jul 31, 2023

Choose a reason for hiding this comment

evanlinjin Aug 1, 2023

Choose a reason for hiding this comment

LLFourn Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

LLFourn left a comment

Choose a reason for hiding this comment

LLFourn Aug 1, 2023

Choose a reason for hiding this comment

evanlinjin Aug 1, 2023

Choose a reason for hiding this comment

LLFourn Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

evanlinjin Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

LLFourn Aug 3, 2023

Choose a reason for hiding this comment

LLFourn commented Aug 2, 2023

LLFourn left a comment

Choose a reason for hiding this comment

Implement linked-list `LocalChain` and update chain-src crates/examples #1034

Implement linked-list `LocalChain` and update chain-src crates/examples #1034

evanlinjin commented Jul 19, 2023 •

edited

Loading

LLFourn Aug 2, 2023 •

edited

Loading

LLFourn Aug 2, 2023 •

edited

Loading

evanlinjin Aug 2, 2023 •

edited

Loading