Add `bitcoind_rpc` chain source module. #1041

evanlinjin · 2023-07-20T01:36:07Z

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

Add test: we should detect when an initially-confirmed transaction is "unconfirmed" during a reorg.
Improve example_bitcoind_rpc: add live command.
Improve docs.
Reintroduce CheckPoint.

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

crates/bitcoind_rpc/Cargo.toml

LLFourn

I've made some comments. It looks awesome. I think this will be a "flagship" example on how to use bdk so I think we should spend the time to get it right. The block data processing pipeline is too complicated for what the problem is. I think IndexedTxGraph needs to be able to process block data and filter out what's relevant itself. This is needed for CBF block processing as well so calling out to weird helper methods in the bitcoind_rpc crate seems counter productive.

crates/bitcoind_rpc/src/lib.rs

example-crates/example_rpc/src/main.rs

crates/bitcoind_rpc/src/lib.rs

example-crates/example_rpc/src/main.rs

notmandatory · 2023-08-23T17:38:10Z

@evanlinjin this looks like a really powerful way to sync with a core node. I read through the code and I think I understand the state-model approach within the Emitter::emit_block. Have you and @LLFourn chatted yet about his ideas to simplify it somehow? From an outside user the APIs look easy enough to use but if there are opportunities to simplify the internals it's worth discussing.

You look to be covering the main test scenarios, 1. sync blocks before and after reorg, 2. sync txs in mempool and after being included in a block. Is it worth adding a test to verify confirmed tx are unconfirmed after a reorg?

evanlinjin · 2023-10-09T13:53:19Z

@LLFourn the file is 104MB, with a start height of 778,000

UPDATE: Okay I accidentally called batch_insert_unconfirmed instead of batch_insert_relevant_confirmed. So most of the content is mempool txs.

UPDATE 2: Yeah with these settings, when a wallet is fully synced, loading from db takes less than 100ms.

So you can pass in the esplora/electrum/bitcoind_rpc server details in the example. Co-authored-by: LLFourn <lloyd.fourn@gmail.com>

This is useful for block-by-block chain sources. We can determine the tx's anchor based on the block, block height and tx position in the block.

* `CheckPoint::from_header` allows us to construct a checkpoint from block header. * `CheckPoint::into_update` transforms the cp into a `local_chain::Update`.

Co-authored-by: Steve Myers <steve@notmandatory.org>

For `IndexedTxGraph`: - Remove `InsertTxItem` type (this is too complex). - `batch_insert_relevant` now uses a simple tuple `(&tx, anchors)`. - `batch_insert` is now also removed, as the same functionality can be done elsewhere. - Add internal helper method `index_tx_graph_changeset` so we don't need to create a seprate `TxGraph` update in each method. - `batch_insert_<relevant>_unconfirmed` no longer takes in an option of last_seen. - `batch_insert_unconfirmed` no longer takes a reference of a transaction (since we apply all transactions anyway, so there is no need to clone). For `TxGraph`: - Add `batch_insert_unconfirmed` method.

Instead of inserting anchors and seen_at timestamp in the same method, we have three separate methods. This makes the API easier to understand and makes `IndexedTxGraph` more consistent with the `TxGraph` API.

Instead of comparing the blockhash against the emitted_blocks map to see whether the block is part of the emitter's best chain, we reduce the `last_mempool_tip` height to the last agreement height during the polling logic. The benefits of this is we have tighter bounds for avoiding re- emission. Also, it will be easier to replace `emitted_blocks` to a `CheckPoint` (since we no longer rely on map lookup).

* `bdk_chain` dependency is added. In the future, we will introduce a separate `bdk_core` crate to contain shared types. * replace `Emitter::new` with `from_height` and `from_checkpoint` * `from_height` emits from the given start height * `from_checkpoint` uses the provided cp to find agreement point * introduce logic that ensures emitted blocks can connect with receiver's `LocalChain` * in our rpc example, we can now `expect()` chain updates to always since we are using checkpoints and receiving blocks in order

vladimirfomene

Great work @evanlinjin ! Just a couple of questions and nits.

vladimirfomene · 2023-10-09T13:21:34Z

crates/bitcoind_rpc/src/lib.rs

+
+    /// The latest first-seen epoch of emitted mempool transactions. This is used to determine
+    /// whether a mempool transaction is already emitted.
+    last_mempool_time: usize,


Why define this as usize? Can we have it as u64 to avoid conversion later?

vladimirfomene · 2023-10-09T13:23:57Z

crates/bitcoind_rpc/src/lib.rs

+    /// that the block is no longer in the best chain, it will be popped off from here.
+    last_cp: Option<CheckPoint>,
+
+    /// The block result returned from rpc of the last-emitted block. As this result contains the


I think using GetBlockResult will make this sentence clearer.

vladimirfomene · 2023-10-09T15:00:19Z

crates/chain/src/indexed_tx_graph.rs

+    /// To filter out irrelevant transactions, use [`batch_insert_relevant_unconfirmed`] instead.
+    ///
+    /// [`batch_insert_relevant_unconfirmed`]: IndexedTxGraph::batch_insert_relevant_unconfirmed
+    pub fn batch_insert_unconfirmed(


When will this be useful? Can we add docs around when a user will want to use this API?

When we want to track transactions, but don't want to filter them out.

example-crates/example_bitcoind_rpc_polling/src/main.rs

* avoid holding mutex lock over io * document `CHANNEL_BOUND` const * use the `relevant` variant of `batch_insert_unconfirmed` * print elapsed time in stdout for various updates

Also better docs for `Emitter` fields.

notmandatory

Re ACK 85c6253

I don't see any critical issues remaining and any final nits can be fixed in follow-up PRs.

evanlinjin added this to the 1.0.0-alpha.2 milestone Jul 20, 2023

evanlinjin self-assigned this Jul 20, 2023

evanlinjin force-pushed the bitcoind_rpc_chain_src branch 3 times, most recently from a0cb7f8 to ae3ee39 Compare July 28, 2023 11:15

nondiremanuel mentioned this pull request Jul 29, 2023

W31 Lib Team Call bitcoindevkit/.github#70

Closed

19 tasks

evanlinjin force-pushed the bitcoind_rpc_chain_src branch 8 times, most recently from 658259c to b5b2367 Compare August 1, 2023 07:44

evanlinjin marked this pull request as ready for review August 1, 2023 07:45

evanlinjin force-pushed the bitcoind_rpc_chain_src branch 2 times, most recently from 019f9f1 to 19e51d4 Compare August 5, 2023 04:29

evanlinjin requested review from LLFourn and danielabrozzoni August 5, 2023 04:40

evanlinjin force-pushed the bitcoind_rpc_chain_src branch from 19e51d4 to 18374e7 Compare August 7, 2023 08:34

evanlinjin mentioned this pull request Aug 7, 2023

Applying blocks API #896

Closed

5 tasks

notmandatory reviewed Aug 8, 2023

View reviewed changes

crates/bitcoind_rpc/Cargo.toml Outdated Show resolved Hide resolved

nondiremanuel mentioned this pull request Aug 8, 2023

W33 Lib Team Call bitcoindevkit/.github#72

Closed

26 tasks

LLFourn reviewed Aug 9, 2023

View reviewed changes

evanlinjin force-pushed the bitcoind_rpc_chain_src branch from 18374e7 to 169c21e Compare August 9, 2023 08:56

nondiremanuel mentioned this pull request Aug 22, 2023

W35 Lib Team Call bitcoindevkit/.github#74

Closed

30 tasks

evanlinjin force-pushed the bitcoind_rpc_chain_src branch 3 times, most recently from 9e873b5 to e1a9124 Compare August 26, 2023 14:23

evanlinjin force-pushed the bitcoind_rpc_chain_src branch 2 times, most recently from 68ed908 to 074ba6a Compare October 9, 2023 13:48

evanlinjin and others added 12 commits October 9, 2023 22:14

feat(example_cli): allow chain specific args in examples

f795a43

So you can pass in the esplora/electrum/bitcoind_rpc server details in the example. Co-authored-by: LLFourn <lloyd.fourn@gmail.com>

feat(chain): add AnchorFromBlockPosition trait

b3db5ca

This is useful for block-by-block chain sources. We can determine the tx's anchor based on the block, block height and tx position in the block.

chain: add helper methods on CheckPoint

43bc813

* `CheckPoint::from_header` allows us to construct a checkpoint from block header. * `CheckPoint::into_update` transforms the cp into a `local_chain::Update`.

chain: add batch-insert methods for IndexedTxGraph

240657b

feat(bitcoind_rpc): introduce bitcoind_rpc crate

bb7424d

test(bitcoind_rpc): initial tests for Emitter

a73dac2

test(bitcoind_rpc): add no_agreement_point test

4f10463

Co-authored-by: Steve Myers <steve@notmandatory.org>

feat(example_bitcoind_rpc_polling): add example for RPC polling

150d6f8

chain: split IndexedTxGraph::insert_tx into 3 methods

6d4b33e

Instead of inserting anchors and seen_at timestamp in the same method, we have three separate methods. This makes the API easier to understand and makes `IndexedTxGraph` more consistent with the `TxGraph` API.

evanlinjin force-pushed the bitcoind_rpc_chain_src branch from 074ba6a to e360879 Compare October 9, 2023 14:15

vladimirfomene reviewed Oct 9, 2023

View reviewed changes

evanlinjin force-pushed the bitcoind_rpc_chain_src branch from e360879 to 397e900 Compare October 9, 2023 16:37

evanlinjin added 2 commits October 10, 2023 01:04

example_bitcoind_rpc: tweaks

b69c13d

* avoid holding mutex lock over io * document `CHANNEL_BOUND` const * use the `relevant` variant of `batch_insert_unconfirmed` * print elapsed time in stdout for various updates

docs(bitcoind_rpc): better Emitter::mempool explanation

85c6253

Also better docs for `Emitter` fields.

evanlinjin force-pushed the bitcoind_rpc_chain_src branch from 397e900 to 85c6253 Compare October 9, 2023 17:04

notmandatory approved these changes Oct 9, 2023

View reviewed changes

notmandatory merged commit 38d69c9 into bitcoindevkit:master Oct 9, 2023
12 checks passed

notmandatory mentioned this pull request Oct 11, 2023

Release 1.0.0-alpha.2 #1168

Closed

12 tasks

notmandatory mentioned this pull request Nov 13, 2023

[chain] Consider Merging IndexedTxGraph and TxGraph into one thing #1147

Open

This was referenced Jan 7, 2024

Remove batch_insert_unconfirmed from TxGraph and IndexedTxGraph #1260

Open

Introduce block-by-block API to bdk::Wallet and add RPC wallet example #1172

Merged

evanlinjin mentioned this pull request Jan 11, 2024

An alternative approach to AnchorFromBlockPosition. #1266

Open

This was referenced Mar 6, 2024

Implement PrunedBlockchain backend: RPC on pruned node #530

Closed

Update bdk_bitcoind_rpc module to work with a pruned bitcoind node #1371

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `bitcoind_rpc` chain source module. #1041

Add `bitcoind_rpc` chain source module. #1041

evanlinjin commented Jul 20, 2023 •

edited

LLFourn left a comment

notmandatory commented Aug 23, 2023

evanlinjin commented Oct 9, 2023 •

edited

vladimirfomene left a comment

vladimirfomene Oct 9, 2023

vladimirfomene Oct 9, 2023

vladimirfomene Oct 9, 2023

evanlinjin Oct 9, 2023 •

edited

notmandatory left a comment

Add bitcoind_rpc chain source module. #1041

Add bitcoind_rpc chain source module. #1041

Conversation

evanlinjin commented Jul 20, 2023 • edited

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

LLFourn left a comment

Choose a reason for hiding this comment

notmandatory commented Aug 23, 2023

evanlinjin commented Oct 9, 2023 • edited

vladimirfomene left a comment

Choose a reason for hiding this comment

vladimirfomene Oct 9, 2023

Choose a reason for hiding this comment

vladimirfomene Oct 9, 2023

Choose a reason for hiding this comment

vladimirfomene Oct 9, 2023

Choose a reason for hiding this comment

evanlinjin Oct 9, 2023 • edited

Choose a reason for hiding this comment

notmandatory left a comment

Choose a reason for hiding this comment

Add `bitcoind_rpc` chain source module. #1041

Add `bitcoind_rpc` chain source module. #1041

evanlinjin commented Jul 20, 2023 •

edited

evanlinjin commented Oct 9, 2023 •

edited

evanlinjin Oct 9, 2023 •

edited