Introduce `bdk_core` by evanlinjin · Pull Request #1569 · bitcoindevkit/bdk

evanlinjin · 2024-08-23T15:09:50Z

Based on #1568
Closes #1543

Description

Introduce bdk_core crate. Move types over from bdk_chain. Chain sources (bdk_electrum, bdk_esplora and bdk_bitcoind_rpc) now only depend on bdk_core.

Notes to the reviewers

Please review commit-by-commit. I've moved things over, but slight API changes were necessary (mentioned in the commit messages).

Changelog notice

Add bdk_core crate which contains core types that were previously in bdk_chain. Including: BlockId, ConfirmationBlockTime, CheckPoint, CheckPointIter, tx_graph::Update and spk_client-types.
Change chain sources (bdk_esplora, bdk_electrum and bdk_bitcoind_rpc) to only depend on bdk_core.

Checklists

All Submissions:

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

notmandatory · 2024-08-23T23:32:39Z

Please keep track if any types or functions were modified other than just because things were moved around. Even though there are a lot of changes here if no types or logic were changed it shouldn't be too hard to review.

crates/core/src/lib.rs

This is an initial version with `chain_data` types ported over. Types ported over include `BlockId`, `ConfirmationBlockTime`. The impls for `Anchor` and `AnchorFromBlockPosition` of these types are moved to where the traits are defined.

to be a separate function: `apply_changeset_to_checkpoint`.

Also add `CheckPoint::eq_ptr` which compares the internal pointers of two `CheckPoint`s. This is used as an optimisation when comparing two chains.

Also introduced extension trait for builder methods that take in a `KeychainTxOutIndex`. `Indexed<T>` and `KeychainIndexed<T>` are also moved to `bdk_core`.

`.populate_tx_cache` has been changed to take in a collection of `Arc<Transaction>`.

Helper methods have been changed to use concrete types (instead of `A: Anchor` generic) - since `Anchor` is still part of `bdk_chain`. Also fix esplora docs.

notmandatory

ACK dafb9aa

Great to see this PR only moves code around and decouple the chain clients from the chain module; plus some small docs and helper function cleanup. Your commits were concise and well documented which helped a lot with review.

I reran examples as a simple smoke test with no problems.

LLFourn · 2024-08-25T04:56:03Z

crates/core/src/lib.rs

+/// Core structures for [`TxGraph`].
+///
+/// [`TxGraph`]: https://docs.rs/bdk_chain/latest/bdk_chain/tx_graph/struct.TxGraph.html
+pub mod tx_graph {


We shouldn't mention TxGraph here too much. Now this thing lives here the Update can be used by any other software to process transaction updates. I'll push a commit.

LLFourn · 2024-08-25T05:07:08Z

crates/core/src/spk_client.rs

+    /// [`TxGraph`](../../bdk_chain/tx_graph/struct.TxGraph.html).
+    pub graph_update: crate::tx_graph::Update<A>,
+    /// The update to apply to the receiving
+    /// [`LocalChain`](../../bdk_chain/local_chain/struct.LocalChain.html).


Heh I guess this link is one way to do it but I think it's a mistake to mention LocalChain and TxGraph here. We are decoupling these things. What if we make a new chain type wouldn't a CheckPoint also be a valid update for it? (I'm pushing a commit).

LLFourn · 2024-08-25T05:10:01Z

crates/electrum/src/bdk_electrum_client.rs

+    /// [`bdk_chain`]: ../bdk_chain/index.html
+    /// [`CalculateFeeError::MissingTxOut`]: ../bdk_chain/tx_graph/enum.CalculateFeeError.html#variant.MissingTxOut
+    /// [`Wallet.calculate_fee`]: ../bdk_wallet/struct.Wallet.html#method.calculate_fee
+    /// [`Wallet.calculate_fee_rate`]: ../bdk_wallet/struct.Wallet.html#method.calculate_fee_rate


⛏️ I'm against these kinds of doc links because they go stale without us noticing. Also they potentially link to incompatible versions. What if we change that method name etc?

It's enough to explain the concept.

Here are the remaining ones after a5d076f:

crates/core/src/spk_client.rs:113: /// This is used to update [`LocalChain`](../../bdk_chain/local_chain/struct.LocalChain.html). crates/core/src/spk_client.rs:124: /// [`KeychainTxOutIndex`](../../bdk_chain/indexer/keychain_txout/struct.KeychainTxOutIndex.html). crates/core/src/spk_client.rs:358: /// This is used to update [`LocalChain`](../../bdk_chain/local_chain/struct.LocalChain.html). crates/electrum/src/bdk_electrum_client.rs:119: /// [`bdk_chain`]: ../bdk_chain/index.html crates/electrum/src/bdk_electrum_client.rs:120: /// [`CalculateFeeError::MissingTxOut`]: ../bdk_chain/tx_graph/enum.CalculateFeeError.html#variant.MissingTxOut crates/electrum/src/bdk_electrum_client.rs:121: /// [`Wallet.calculate_fee`]: ../bdk_wallet/struct.Wallet.html#method.calculate_fee crates/electrum/src/bdk_electrum_client.rs:122: /// [`Wallet.calculate_fee_rate`]: ../bdk_wallet/struct.Wallet.html#method.calculate_fee_rate crates/electrum/src/bdk_electrum_client.rs:188: /// [`bdk_chain`]: ../bdk_chain/index.html crates/electrum/src/bdk_electrum_client.rs:189: /// [`CalculateFeeError::MissingTxOut`]: ../bdk_chain/tx_graph/enum.CalculateFeeError.html#variant.MissingTxOut crates/electrum/src/bdk_electrum_client.rs:190: /// [`Wallet.calculate_fee`]: ../bdk_wallet/struct.Wallet.html#method.calculate_fee crates/electrum/src/bdk_electrum_client.rs:191: /// [`Wallet.calculate_fee_rate`]: ../bdk_wallet/struct.Wallet.html#method.calculate_fee_rate

We shouldn't refer to `tx_graph` in `bdk_core`. TxGraph happens to consume `Update` but it doesn't conceptually own the definition of an update to transactions. I tried to also remove a lot of references to "graph" where they shouldn't be. "graph" is a very general kind of data structure so we should avoid referring to it where it's not relevant. `tx_update` is much better than `graph_update`.

LLFourn

I added a a5d076f:

We shouldn't refer to tx_graph in bdk_core. TxGraph happens to consume Update but it doesn't conceptually own the definition of an update to transactions.
I tried to also remove a lot of references to "graph" where they shouldn't be. "graph" is a very general kind of data structure so we should avoid referring to it where it's not relevant. tx_update is much better than graph_update.

Self-ACK: a5d076f

evanlinjin

ACK a5d076f

TxUpdate makes sense. The doc changes make things clearer and easier to maintain.

ValuedMammal · 2024-08-25T15:56:02Z

crates/electrum/src/bdk_electrum_client.rs

-            .full_txs()
-            .map(|tx_node| (tx_node.txid, tx_node.tx));
-
+    pub fn populate_tx_cache(&self, txs: impl IntoIterator<Item = impl Into<Arc<Transaction>>>) {


Ideally this function could accept an iterator of (Txid, Arc<Transaction>) tuples

This function is called at startup time so computing txid here is not really worth optimizing out.

evanlinjin force-pushed the bdk_core branch 4 times, most recently from 911085a to c711102 Compare August 23, 2024 16:42

notmandatory assigned evanlinjin Aug 23, 2024

notmandatory added module-blockchain api A breaking API change labels Aug 23, 2024

notmandatory added this to the 1.0.0-beta milestone Aug 23, 2024

notmandatory reviewed Aug 23, 2024

View reviewed changes

crates/core/src/lib.rs Outdated Show resolved Hide resolved

evanlinjin force-pushed the bdk_core branch 5 times, most recently from 92e442c to 6275e72 Compare August 24, 2024 16:06

evanlinjin added 8 commits August 24, 2024 16:12

feat!: introduce bdk_core

6f7626a

This is an initial version with `chain_data` types ported over. Types ported over include `BlockId`, `ConfirmationBlockTime`. The impls for `Anchor` and `AnchorFromBlockPosition` of these types are moved to where the traits are defined.

refactor(chain): change CheckPoint::apply_changeset

a86c878

to be a separate function: `apply_changeset_to_checkpoint`.

feat!: move CheckPoint to bdk_core

77e31c7

Also add `CheckPoint::eq_ptr` which compares the internal pointers of two `CheckPoint`s. This is used as an optimisation when comparing two chains.

feat!: move tx_graph::Update to bdk_core

bdea871

feat!: move spk_clients to bdk_core

ab0315d

Also introduced extension trait for builder methods that take in a `KeychainTxOutIndex`. `Indexed<T>` and `KeychainIndexed<T>` are also moved to `bdk_core`.

feat(electrum)!: depend on bdk_core instead of bdk_chain

0d302f5

`.populate_tx_cache` has been changed to take in a collection of `Arc<Transaction>`.

feat(esplora)!: depend on bdk_core instead of bdk_chain

fea8eed

Helper methods have been changed to use concrete types (instead of `A: Anchor` generic) - since `Anchor` is still part of `bdk_chain`. Also fix esplora docs.

feat(bitcoind_rpc)!: depend on bdk_core instead of bdk_chain

dafb9aa

evanlinjin force-pushed the bdk_core branch from 6275e72 to dafb9aa Compare August 24, 2024 16:23

evanlinjin changed the title ~~bdk_core WIP WIP WIP~~ Introduce bdk_core Aug 24, 2024

evanlinjin marked this pull request as ready for review August 24, 2024 16:29

notmandatory approved these changes Aug 24, 2024

View reviewed changes

LLFourn reviewed Aug 25, 2024

View reviewed changes

LLFourn force-pushed the bdk_core branch from 8c581e2 to a5d076f Compare August 25, 2024 05:42

LLFourn approved these changes Aug 25, 2024

View reviewed changes

evanlinjin commented Aug 25, 2024

View reviewed changes

evanlinjin merged commit e9d0cd9 into bitcoindevkit:master Aug 25, 2024

evanlinjin mentioned this pull request Aug 25, 2024

Redo tx test #1338

Closed

8 tasks

ValuedMammal reviewed Aug 25, 2024

View reviewed changes

notmandatory mentioned this pull request Aug 25, 2024

Release 1.0.0-beta.2 #1571

Closed

31 tasks

evanlinjin mentioned this pull request Sep 4, 2024

Introduce bdk_core crate #1155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `bdk_core`#1569

Introduce `bdk_core`#1569
evanlinjin merged 9 commits intobitcoindevkit:masterfrom
evanlinjin:bdk_core

evanlinjin commented Aug 23, 2024 •

edited

Loading

Uh oh!

notmandatory commented Aug 23, 2024

Uh oh!

Uh oh!

notmandatory left a comment

Uh oh!

LLFourn Aug 25, 2024

Uh oh!

LLFourn Aug 25, 2024

Uh oh!

LLFourn Aug 25, 2024 •

edited

Loading

Uh oh!

LLFourn left a comment

Uh oh!

evanlinjin left a comment

Uh oh!

ValuedMammal Aug 25, 2024

Uh oh!

LLFourn Aug 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

evanlinjin commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

Uh oh!

notmandatory commented Aug 23, 2024

Uh oh!

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

LLFourn Aug 25, 2024

Choose a reason for hiding this comment

Uh oh!

LLFourn Aug 25, 2024

Choose a reason for hiding this comment

Uh oh!

LLFourn Aug 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LLFourn left a comment

Choose a reason for hiding this comment

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

ValuedMammal Aug 25, 2024

Choose a reason for hiding this comment

Uh oh!

LLFourn Aug 25, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

evanlinjin commented Aug 23, 2024 •

edited

Loading

LLFourn Aug 25, 2024 •

edited

Loading