Enhance bdk chain structures by vladimirfomene · Pull Request #1084 · bitcoindevkit/bdk

vladimirfomene · 2023-08-21T09:19:55Z

Description

Fixes #1061

Changelog notice

Move WalletUpdate to the wallet module
Remove ForEachTxout trait completely
Refactor ElectrumExt to not use WalletUpdate.

Checklists

All Submissions:

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

LLFourn · 2023-08-22T03:38:36Z

What's the motivation for removing ForEachTxOut? I think it serves a purpose in that it allows you to scan several things that are bunches of txouts (including things that are not transactions).

evanlinjin · 2023-08-22T07:53:33Z

What's the motivation for removing ForEachTxOut? I think it serves a purpose in that it allows you to scan several things that are bunches of txouts (including things that are not transactions).

This was my suggestion and I wrote it on the ticket. I don't think it is needed? The Indexer trait has the method Indexer::index_txout(&mut self, outpoint: OutPoint, txout: &TxOut) -> Self::ChangeSet, and we can have another method:

pub trait Indexer {
    /* ... */

    fn index_txouts<'a>(
        &mut self,
        txouts: impl IntoIterator<Item = (OutPoint, &'a TxOut)>,
    ) -> Self::ChangeSet
    where
        Self::ChangeSet: Append + Default,
    {
        let mut changeset = Self::ChangeSet::default();
        for (outpoint, txout) in txouts {
            Append::append(&mut changeset, self.index_txout(outpoint, txout));
        }
        changeset
    }
}

Now anything that implements IntoIterator<Item = (OutPoint, &TxOut)> will work.

vladimirfomene · 2023-08-22T13:39:14Z

Will rebase this once MSRV fixes make it to master.

crates/bdk/src/wallet/mod.rs

crates/chain/src/keychain/txout_index.rs

evanlinjin

Thanks for the work so far. Sorry about my incremental reviews.

crates/bdk/src/wallet/mod.rs

crates/electrum/src/electrum_ext.rs

LLFourn · 2023-08-25T07:06:47Z

What's the motivation for removing ForEachTxOut? I think it serves a purpose in that it allows you to scan several things that are bunches of txouts (including things that are not transactions).

This was my suggestion and I wrote it on the ticket. I don't think it is needed? The Indexer trait has the method Indexer::index_txout(&mut self, outpoint: OutPoint, txout: &TxOut) -> Self::ChangeSet, and we can have another method:

So if I want to index a Block what should I do?. I can iterate manually over every TxOut and call this but that seems much more cumbersome than it needs to be?

vladimirfomene · 2023-08-25T09:20:27Z

@LLFourn I'm thinking you will use index_tx here instead of index_txout.

crates/electrum/src/electrum_ext.rs

LLFourn · 2023-08-25T09:44:59Z

@LLFourn I'm thinking you will use index_tx here instead of index_txout.

Understood. I think might be as easy as we need to make it. Thanks.

crates/electrum/src/electrum_ext.rs

@LLFourn

We remove `ElectrumUpdate` and return tuples instead for `ElectrumExt` methods. We introduce the `IncompleteTxGraph` structure to specifically hodl the incomplete `TxGraph`. This change is motivated by @LLFourn's comment: bitcoindevkit@794bf37#r1305432603

…ainTxoutIndex

notmandatory

ACK c56728f

I confirmed example_electrum and example_esplora scan and sync commands still work. Used my test signet descriptors:

cargo run -- --network signet "wpkh(tprv8ZgxMBicQKsPdy6LMhUtFHAgpocR8GC6QmwMSFpZs7h6Eziw3SpThFfczTDh5rW2krkqffa11UpX3XkeTTB2FvzZKWXqPY54Y6Rq4AQ5R8L/84'/1'/0'/0/*)" "wpkh(tprv8ZgxMBicQKsPdy6LMhUtFHAgpocR8GC6QmwMSFpZs7h6Eziw3SpThFfczTDh5rW2krkqffa11UpX3XkeTTB2FvzZKWXqPY54Y6Rq4AQ5R8L/84'/1'/0'/1/*)" scan

notmandatory · 2023-09-02T23:42:40Z

One small nit, it looks like most of the commits include breaking changes so commit messages should include a ! to denote commit breaks old APIs. Doesn't need to be fixed for this PR but to keep in mind for future.

evanlinjin · 2023-09-03T13:24:14Z

ACK c56728f

LLFourn

Hey @vladimirfomene thanks for doing this refactoring.

I've pointed out a bunch of ways we can simplify things in both wallet types and electrum_ext. Many of the changes requested are not to do with this PR but I think this is the place to do them.

[edit] I also pushed 4104206

crates/bdk/src/wallet/mod.rs

crates/chain/src/keychain/txout_index.rs

crates/chain/src/lib.rs

crates/electrum/src/electrum_ext.rs

crates/chain/src/keychain/txout_index.rs

LLFourn

This LGTM! ACK ee01b0f

left a couple of small comments.

crates/electrum/src/electrum_ext.rs

evanlinjin

Mostly nits and documentation suggestions.

crates/chain/src/indexed_tx_graph.rs

crates/chain/src/spk_txout_index.rs

And signature of `example_cli::KeychainChangeSet` is changed.

evanlinjin

ACK 1ff806c

vladimirfomene changed the title ~~Enhance bdk chain structures~~ WIP: Enhance bdk chain structures Aug 21, 2023

notmandatory assigned vladimirfomene Aug 21, 2023

vladimirfomene force-pushed the enhance-bdk-chain-structures branch 4 times, most recently from 2f3b9d9 to d640543 Compare August 22, 2023 13:35

vladimirfomene marked this pull request as ready for review August 22, 2023 13:38

nondiremanuel added this to the 1.0.0-alpha.3 milestone Aug 22, 2023

nondiremanuel mentioned this pull request Aug 22, 2023

W35 Lib Team Call bitcoindevkit/.github#74

Closed

30 tasks

vladimirfomene force-pushed the enhance-bdk-chain-structures branch from d640543 to ebe28bd Compare August 23, 2023 07:21

vladimirfomene changed the title ~~WIP: Enhance bdk chain structures~~ Enhance bdk chain structures Aug 23, 2023

evanlinjin reviewed Aug 23, 2023

View reviewed changes

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

vladimirfomene force-pushed the enhance-bdk-chain-structures branch from 5ca099f to 7d7ec20 Compare August 24, 2023 13:09

evanlinjin requested changes Aug 24, 2023

View reviewed changes

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

evanlinjin requested changes Aug 24, 2023

View reviewed changes

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

crates/bdk/src/wallet/mod.rs Outdated Show resolved Hide resolved

crates/electrum/src/electrum_ext.rs Show resolved Hide resolved

crates/electrum/src/electrum_ext.rs Show resolved Hide resolved

LLFourn reviewed Aug 25, 2023

View reviewed changes

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

LLFourn reviewed Aug 25, 2023

View reviewed changes

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

vladimirfomene force-pushed the enhance-bdk-chain-structures branch 2 times, most recently from 0c2ddec to 2d482fd Compare August 25, 2023 18:56

evanlinjin requested changes Aug 26, 2023

View reviewed changes

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

evanlinjin force-pushed the enhance-bdk-chain-structures branch 3 times, most recently from 74f213e to 2377826 Compare August 26, 2023 12:35

evanlinjin mentioned this pull request Aug 26, 2023

Implement TxGraph::map_anchor method #1096

Closed

evanlinjin and others added 2 commits September 3, 2023 01:51

refactor: Remove scan and scan_txout from SpkTxoutIndex and Keych…

c56728f

…ainTxoutIndex

evanlinjin force-pushed the enhance-bdk-chain-structures branch 2 times, most recently from 905101d to c56728f Compare September 2, 2023 18:22

notmandatory approved these changes Sep 2, 2023

View reviewed changes

evanlinjin requested a review from LLFourn September 4, 2023 18:47

feat: impl Append for lots of tuples

4104206

LLFourn requested changes Sep 5, 2023

View reviewed changes

vladimirfomene force-pushed the enhance-bdk-chain-structures branch from e89c839 to 9669b36 Compare September 6, 2023 07:01

evanlinjin reviewed Sep 6, 2023

View reviewed changes

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

vladimirfomene force-pushed the enhance-bdk-chain-structures branch 2 times, most recently from f8d3070 to ee01b0f Compare September 6, 2023 07:55

LLFourn reviewed Sep 7, 2023

View reviewed changes

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

LLFourn approved these changes Sep 7, 2023

View reviewed changes

crates/electrum/src/electrum_ext.rs Outdated Show resolved Hide resolved

vladimirfomene force-pushed the enhance-bdk-chain-structures branch 3 times, most recently from 1b1eb8f to 1530568 Compare September 8, 2023 14:18

nondiremanuel mentioned this pull request Sep 10, 2023

W37 Lib Team Call bitcoindevkit/.github#76

Closed

41 tasks

evanlinjin requested changes Sep 13, 2023

View reviewed changes

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

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

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

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

vladimirfomene force-pushed the enhance-bdk-chain-structures branch 2 times, most recently from 0ad05a6 to 69d6825 Compare September 13, 2023 09:55

refactor: improve docs, cleanup unnecessary types and improve code

d43ae02

vladimirfomene force-pushed the enhance-bdk-chain-structures branch from 69d6825 to d43ae02 Compare September 13, 2023 10:58

fix(chain)!: rm weird From impl

1ff806c

And signature of `example_cli::KeychainChangeSet` is changed.

evanlinjin approved these changes Sep 14, 2023

View reviewed changes

evanlinjin merged commit c20a4da into bitcoindevkit:master Sep 15, 2023

nondiremanuel mentioned this pull request Sep 23, 2023

W39 Lib Team Call bitcoindevkit/.github#78

Closed

40 tasks

notmandatory mentioned this pull request Oct 11, 2023

Release 1.0.0-alpha.2 #1168

Closed

12 tasks

Conversation

vladimirfomene commented Aug 21, 2023

Description

Changelog notice

Checklists

All Submissions:

Uh oh!

LLFourn commented Aug 22, 2023

Uh oh!

evanlinjin commented Aug 22, 2023

Uh oh!

vladimirfomene commented Aug 22, 2023

Uh oh!

Uh oh!

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LLFourn commented Aug 25, 2023

Uh oh!

vladimirfomene commented Aug 25, 2023

Uh oh!

Uh oh!

Uh oh!

LLFourn commented Aug 25, 2023

Uh oh!

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Sep 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evanlinjin commented Sep 3, 2023

Uh oh!

LLFourn left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LLFourn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

notmandatory commented Sep 2, 2023 •

edited

Loading

LLFourn left a comment •

edited

Loading