bdk v1.0.0-alpha.0 by LLFourn · Pull Request #793 · bitcoindevkit/bdk

LLFourn · 2022-11-03T08:57:06Z

We prepare the BDK repo for a major restructuring 🔥. This PR maintains the existing wallet API as much as possible and adds very little.

Things Done

database modules removed
blockchain gutted but new esplora syncing code added (this will be gone soon hopefully).
minimal API changes.
Many macros removed.
no longer applicable examples removed.
Much conditional compilation removed. Can compile with --all-features now.
All wallet tests passing
TestClient moved into its own repo
Example using esplora

APIs changed

wallet no longer has a sync method. This is replaced with apply_wallet_scan.
address "caching" is gone. You can just change the derivation index with ensure_derived_up_to which sets your derivation to at least the argument. Unlike ensure_addresses_cached used to do this will alter what getting a new address gives you.
AddressIndex::Reset is gone. This thing didn't make much sense and is hard to do with the more sane internals we've established. Changing the derivation index changes what script pubkeys the wallet will search so this is dangerous. We plan to add method like trim_unused which lowers the derivation index to the highest unused index. Applications must handle giving out old addresses manually now (which I think is good).

Unfinished work

esplora example doesn't work for mempool transactions yet (seems like our esplora in testclient doesn't index mempool??).
we need to figure out a way to retrieve and store transaction timestamps (we're currently just setting them to u64::MAX). In bdk_core we never got around to doing this but it needs to be done.
A few insights we got from doing this PR should be applied to bdk_core first.
doctests not working.

Notes to the reviewers

Try not to review the actual changes. This PR will be forced pushed a bit so it will be likely wasted.
I think I did a faithful job of translating the tests. A bit of review here would be helpful.

I do think it would be good to merge this PR soon into the v1 branch so we have something to work off once unfinished work is done.
Checking out the branch and poke around and give feedback would be the most helpful thing.

Run the (sort of) working example:

cargo run --example esplora --features="bdk_test_client/bitcoind_22_0 bdk_test_client/esplora esplora"

Checklists

All Submissions:

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

Bugfixes:

This pull request breaks the existing API
I'm linking the issue being fixed by this PR (there's too many!)

.github/workflows/cont_integration.yml

afilini

I still need to finish a couple of files but since it's taking me so long I'll start by posting those comments

afilini · 2022-11-04T11:29:04Z

src/wallet/mod.rs

+                if block.header.prev_blockhash != exp_hash {
+                    panic!("block hash does not match!");
+                }
+            }


I guess otherwise you should check it's genesis block

afilini · 2022-11-07T11:20:25Z

src/blockchain/esplora.rs

+    /// transaction output along with the update. After `stop_gap` script pubkeys have no related
+    /// transactions it terminates (or if the iterator is exhausested).
+    ///
+    /// `parallel_requests` indicates that the client may make that may **additional** requests in


"that the client may make additional requests"

afilini · 2022-11-07T11:23:08Z

src/blockchain/esplora.rs

+            })
+            .collect::<Vec<_>>();
+
+        let n_handles = handles.len();


This can be moved down i guess? n_handles is not used in the loop

afilini · 2022-11-07T11:28:17Z

src/blockchain/esplora.rs

+            })
+            .collect::<Vec<_>>();
+
+        if !blocks_at_end.contains(&tip_at_start) {


Esplora returns the last 10 blocks, so in theory here we should also make sure that the height of tip_at_start is in this range, otherwise we need to fetch more blocks.

afilini · 2022-11-07T11:30:28Z

src/blockchain/esplora.rs

+            })
+            .collect::<Vec<_>>();
+
+        if !blocks_at_end.contains(&tip_at_start) {


Same as above

danielabrozzoni

Just an initial review, I still need to look at half of src/wallet/mod.rs (I reviewed until before apply_update) and a bunch of other files. I'm posting this half-baked review so that you can start replying to all my questions (sorry, lol)

danielabrozzoni · 2022-11-14T13:40:50Z

src/wallet/mod.rs

-    /// Return the address for a specific descriptor index and reset the current descriptor index
-    /// used by `AddressIndex::New` and `AddressIndex::LastUsed` to this value.
-    ///
-    /// Use with caution, if an index is given that is less than the current descriptor index
-    /// then the returned address and subsequent addresses returned by calls to `AddressIndex::New`
-    /// and `AddressIndex::LastUsed` may have already been used. Also if the index is reset to a
-    /// value earlier than the [`crate::blockchain::Blockchain`] stop_gap (default is 20) then a
-    /// larger stop_gap should be used to monitor for all possibly used addresses.
-    Reset(u32),


Could you expand a bit on why is Reset being removed?

The reason is to create the invariant that every UTXO you have must have a derivation index less than or equal to your current derivation index. Clearly this cannot hold if you allow "resetting" your derivation index. Why do we want this invariant? It simplifies API and internals. There is no longer two indexes you are tracking: (i) where you are cached up to, (ii) where you are giving out addresses at. You just have one number which does handles both things.

However we should allow for "trimming" your derivation index down to your last active index. This will be needed in order to do stop_gap style syncing with things like CBF and rpc block streaming. The algorithm would be to set your derivation index to last_active_index + stop_gap before applying each block. Then when you've reached the tip you can trim to the last_active_index and the wallet becomes operational.

Personally I think we should re-enable this feature to reset to the largest of:

First unused index

Parameter in Reset(param)

It shouldn't be complicated to implement with the KeychainTxOutIndex API.

I think we would need to "unreveal" method in KeychainTxOutIndex first I think. I still find this very questionable. Why would you reveal an address and then not actually reveal it. In any case this feature shouldn't be done as an AddressIndex.

danielabrozzoni · 2022-11-14T14:01:59Z

src/wallet/mod.rs

+pub enum ApplyUpdateErr {
+    /// The data in the update was somehow incompatible with the existing chain's data.
+    Chain(UpdateFailure),
+    /// The update was compatible but it was missing **full** transactions for some txids it found.


Just to make sure that I understand: this error is thrown if the Update is containing a certain txidA in the new_txids, but there's not a transaction with txid equal to txidA in the blocks between the last valid checkpoint and new_tip. Is that right?

Edit: I tried to read apply_update and concluded that I'm probably wrong

It is thrown when we are told there is a relevant txid in some block but we haven't provided the actual transaction. This comes up with electrum which first returns you a list of txids which you would go and turn into an update. The wallet would then tell you which transactions it actually needs you to download with that error. You go and fetch them and re-apply it.

src/wallet/mod.rs

danielabrozzoni · 2022-11-14T14:17:00Z

src/wallet/mod.rs

+    pub fn list_unspent(&self) -> Vec<LocalUtxo> {
+        self.spk_tracker
+            .iter_unspent(&self.sparse_chain, &self.tx_graph)
+            .map(|((keychain, _), txo)| LocalUtxo {


This reminds me: the 1.0 release is a good opportunity to rename LocalUtxo to LocalTxOut :)

Note i didn't do this here. I think it's possible we could get rid of this type in favor of using a bdk_chain one.

danielabrozzoni · 2022-11-14T14:30:59Z

src/wallet/mod.rs

+            if self.sparse_chain.transaction_height(tx.txid()).is_some() {
+                continue;
+            }


This checks seems relatively cheap, maybe it should be done first, before the match?

src/wallet/mod.rs

LLFourn · 2022-11-16T03:58:27Z

Just pushed changes to:

Remove some stuff that wasn't relevant to this PR and snuck through unintentionally. This includes apply_blocks which @evanlinjin pushed recently. We agreed that it would be better for that to come in with the changes that will use it.
Fix the esplora example by waiting a bit. H/t @afilini @danielabrozzoni.

tnull · 2022-12-02T10:16:45Z

I'm currently trying wrap my head around how/if this new syncing mechanism could be used to better integrate with LDK. IIUC, the main idea here is that instead of having a monolithic sync() method, this follows three-step protocol:

the wallet generates a list of items (descriptors) it is interested in
this list is passed to the Esplora client which retrieves the relevant updates
the updates are applied to the Wallet

I think generally a similar scheme could be used to update the LDK-internal wallet (e.g., via the Confirm interface). However, I think in order to make this work we'd need (just some assorted notes for now):

The methods allowing to express interest in a set of items and to apply them would need to be based on suitable abstractions (e.g, an UpdatableWallet trait or similar, etc.), which a part of LDK can implement/work with.
To be able to express interest in the status of particular outputs and/or txids, not just descriptors.
Have access to additional data items for each applied update (block headers, index of transactions within a block, confirmation height) for any registered items (transactions/output spends).
The ordering in which the updates are applied/returned needs to be deterministic and chain order. In particular any unconfirmations of items would need to be handed over before any reconfirmations.

Would be interested in any feedback regarding this, in particular whether we could find any abstractions that would allow us to further integrate the sync mechanisms or the wallets themselves, for that matter.

LLFourn · 2022-12-13T05:44:41Z

* The methods allowing to express interest in a set of items and to apply them would need to be based on suitable abstractions (e.g, an `UpdatableWallet` trait or similar, etc.), which a part of LDK can implement/work with.

Can these abstraction be defined in LDK or do you think they need to be in BDK?

* To be able to express interest in the status of particular outputs and/or txids, not just descriptors.

I added an issue for this: LLFourn/bdk_core_staging#84. Currently we don't require you pass a descriptor (at least over at the bdk_core repo). You just pass in an iterator of script pubkeys you are interested in. I think we can add to that iterators of outpoints and txids to that.

* Have access to additional data items for each applied update (block headers, index of transactions within a block, confirmation height) for any registered items (transactions/output spends).

We have designed the internals to allow this. I created an issue (LLFourn/bdk_core_staging#85) for someone to actually do the work to include the all this info. Currently (though not in this PR yet) we only include the confirmation height as "additional data" (and sometimes the confirmation time as well).

* The ordering in which the updates are applied/returned needs to be deterministic and chain order. In particular any unconfirmations of items would need to be handed over before any reconfirmations.

I don't understand exactly this requirement but our update logic makes it hard to get this wrong I hope. There's an issue to make the ordering even more picky LLFourn/bdk_core_staging#70 but I'm not sure if this addresses your concern.

Would be interested in any feedback regarding this, in particular whether we could find any abstractions that would allow us to further integrate the sync mechanisms or the wallets themselves, for that matter.

Thanks for making this comment and apologies for the late reply. I'm going to be updating this PR soon so you can have a look at the state of the art.

tnull · 2022-12-13T11:06:37Z

Can these abstraction be defined in LDK or do you think they need to be in BDK?

Hm, I think BDK would need to provide some kind of API that we can work with, e.g., some trait-based interface that we could implement.

I added an issue for this: LLFourn/bdk_core_staging#84. Currently we don't require you pass a descriptor (at least over at the bdk_core repo). You just pass in an iterator of script pubkeys you are interested in. I think we can add to that iterators of outpoints and txids to that.

We have designed the internals to allow this. I created an issue (LLFourn/bdk_core_staging#85) for someone to actually do the work to include the all this info. Currently (though not in this PR yet) we only include the confirmation height as "additional data" (and sometimes the confirmation time as well).

Nice, looking forward to these!

I don't understand exactly this requirement but our update logic makes it hard to get this wrong I hope. There's an issue to make the ordering even more picky LLFourn/bdk_core_staging#70 but I'm not sure if this addresses your concern.

Yeah, I think the details of the implementation should lie on our side, but the main point w.r.t. BDK would be that the interface would give us the updates in topological chain order.

Thanks for making this comment and apologies for the late reply. I'm going to be updating this PR soon so you can have a look at the state of the art.

Sounds good, thank you! :)

LLFourn · 2022-12-24T02:29:41Z

Status update.

Here's the scope of the PR and where we are on it:

We've decided to expand the scope of this PR to include persistence and as we agreed it is important to guarantee that every time you give out an address you have persisted that fact before you return it.
We've decided to turn the repo in to a cargo workspace and move all the concrete blockchain data sources out of the bdk crate so it has minimal dependencies. I'll do that as part of this PR with a minimal working.

So remaining:

Make wallet auto-persist to something with a simple trait that allows you to insert changesets perhaps with some distinction between updating chain data and derivation indices. Derivation indices must be guaranteed while chain data can just be refetched.
Fix docs and doc tests
Make repo into a workspace.

There is one issue that should be done in bdk_core staging: LLFourn/bdk_core_staging#114

LLFourn · 2023-02-15T02:12:51Z

Status update:

I implemented persistence as I imagined. See persist.rs. Any change you make to the wallet will be staged and then you can call commit to save it to the persistence backend. Some changes (like getting a new address) will be committed right away for safety reasons.
All wallet tests have been moved into the /tests folder.
Added Wallet::cancel_tx which allows you to tell the wallet you will no longer be using a tx that the builder generated. This means the wallet will use that change address in the future (since it was never broadcast). In the future this will also unreserve utxos (see Let TxBuilder avoid previously used UTXOs (UTXO locking) bdk_wallet#166).

Remaining for v1.0.0-alpha.0 (help needed):

Doc review and making doc tests work
Make bdk into a workspace
put bdk_esplora and bdk_electrum ported from bdk_core_staging examples.

LLFourn · 2023-02-15T02:23:02Z

Just reading back @tnull's comments:

Yeah, I think the details of the implementation should lie on our side, but the main point w.r.t. BDK would be that the interface would give us the updates in topological chain order.

Ok so our design does not enforce this: "changesets" can insert transactions that are ancestors of transactions that have already appeared in previous changesets. This will never happen in most workflows (and can be guaranteed never to happen if you update your state purely by applying new blocks sequentially).

Other than sending bdk's changesets over to LDK maybe we can just create a custom "update" for it from our own chain data. We store and index a lot of stuff in memory so if the requirement is answering questions like "have these outputs been spent and by what tx" it should be easy. More sophisticated things like "notify me when this output is spent (or is no longer spent)" should be doable as well (but not yet implemented in an easy to consume way).

LLFourn · 2023-02-16T00:42:35Z

One thing I think I forgot to implement is an API to expose SpkTxOutIndex::send_and_received method which is needed for it to be feature compatible with legacy bdk since TransactionDetails had sent and received fields and that was stored in the DB. From now on we want to calculate it on the fly.

Cargo.toml

LLFourn · 2023-02-21T05:29:48Z

I've just make the repo into a workspace. Existing bdk is in crates (it will be joined by bdk_chain and bdk_esplora etc eventually). There is also an example-crates directory where you can put examples that are crates themselves and add them to the workspace. I put to placeholder crates there to demonstrate for electrum and esplora.

The final task for bdk-v1.0.0-aplha.0 is to make those two example crates using Wallet, to clean up documentation/README and make CI pass.

notmandatory

ACK 8d56d55

CI updated, and tests passing. Organization looks good. There are small nits we could still fix but it's good enough for a first alpha release and will be easier to collaborate on additional changes once this PR is merged.

LLFourn is squashing these to get them all signed: Remove useless clippy allow ci: use clippy action [ci] remove check for features=default

danielabrozzoni

I pushed a commit with some small doc fixes.

ACK bc3e05c

We are not using them, removed in bitcoindevkit#793 more specifically in 3f5a78a NOTE: speculos can be run under Nix - https://github.com/alamgu/speculos/tree/with-nix-build - LedgerHQ/speculos#323

27a63ab chore: typos fixed (Einherjar) Pull request description: ### Description Fixes the typos and remove unused speculos dockerfiles that was done in #1165. Moving these changes into this PR to be merged first. Then, we can rebase #1165 and make it only related to CI and Nix. (Maybe do a big squash 😄) ## Note to Reviewers About the speculos stuff, we are not using them, removed in #793, more specifically in 3f5a78a. ### Changelog notice - Fix typos in codebase and docs - Remove unused CI tests with hardware signer Dockerfiles ### 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 * [ ] I've added docs for the new feature #### Bugfixes: * [ ] This pull request breaks the existing API * [ ] I've added tests to reproduce the issue which are now passing * [ ] I'm linking the issue being fixed by this PR ACKs for top commit: danielabrozzoni: ACK 27a63ab Tree-SHA512: a01101d0741e2b0e1d1254b5cae670c5a936bb0b89332c102feb57d58d2b9ae995ed4436068b0dc5fae73dbe22431c3143d6e04cdc12eab991bd726cfd2fbe25

8694624 Bump `bip39` to v2.0 (Elias Rohrer) Pull request description: We previously bumped the `bip39` version to 2.0 [in the 0.2X release branch](#875). Back then, bumping it in `master` was [assumed unnecessary](#875 (comment)). It seems this was erroneous, as the current `1.0.1` dependency is present since #793, which was merged before the bump in `release/0.27`. Here, we therefore bump the crate version in `master` after all. ACKs for top commit: notmandatory: ACK 8694624 Tree-SHA512: a109219bc97bb8e965e8b10e72439aa898b710d1d1a154801ce499ad47475a6b23448d85e0de3f306f990573d1fccdae7d587ed41676a01f91d66a719782eae1

LLFourn force-pushed the bdk_core_init branch from bf2ba37 to cbe573e Compare November 3, 2022 08:59

danielabrozzoni reviewed Nov 3, 2022

View reviewed changes

.github/workflows/cont_integration.yml Outdated Show resolved Hide resolved

LLFourn mentioned this pull request Nov 4, 2022

Transaction Sorting and Querying #794

Closed

LLFourn marked this pull request as draft November 4, 2022 05:58

notmandatory mentioned this pull request Nov 7, 2022

W45 BDK Library Team Call bitcoindevkit/.github#31

Closed

24 tasks

afilini reviewed Nov 8, 2022

View reviewed changes

danielabrozzoni reviewed Nov 14, 2022

View reviewed changes

LLFourn mentioned this pull request Nov 23, 2022

Simple file persistence LLFourn/bdk_core_staging#67

Merged

LLFourn force-pushed the bdk_core_init branch from e681ece to 46dd898 Compare December 23, 2022 05:36

LLFourn force-pushed the bdk_core_init branch 2 times, most recently from 795d55a to a8291b8 Compare January 10, 2023 04:10

LLFourn mentioned this pull request Jan 18, 2023

no_std support #205

Closed

3 tasks

LLFourn force-pushed the bdk_core_init branch from a8291b8 to 5b483b8 Compare February 2, 2023 05:00

LLFourn force-pushed the bdk_core_init branch from 5b483b8 to 7e8f849 Compare February 15, 2023 02:01

LLFourn mentioned this pull request Feb 19, 2023

Features/txbuilder expose params #839

Closed

5 tasks

notmandatory reviewed Feb 20, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

notmandatory reviewed Feb 20, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

LLFourn force-pushed the bdk_core_init branch 2 times, most recently from 26ae2ce to e67610a Compare February 21, 2023 05:26

LLFourn force-pushed the bdk_core_init branch 2 times, most recently from d314d9c to b603abb Compare February 21, 2023 05:32

danielabrozzoni force-pushed the bdk_core_init branch 4 times, most recently from 331fdb7 to 8d56d55 Compare March 10, 2023 12:14

rajarshimaitra mentioned this pull request Mar 10, 2023

Nit fixes on documentation #888

Merged

3 tasks

notmandatory approved these changes Mar 13, 2023

View reviewed changes

LLFourn force-pushed the bdk_core_init branch from 8d56d55 to 8eac1ae Compare March 13, 2023 03:29

[ci] misc fixes

352f95f

LLFourn is squashing these to get them all signed: Remove useless clippy allow ci: use clippy action [ci] remove check for features=default

LLFourn force-pushed the bdk_core_init branch from 8eac1ae to 352f95f Compare March 13, 2023 03:33

notmandatory mentioned this pull request Mar 13, 2023

W11 BDK Lib Team Call bitcoindevkit/.github#48

Closed

22 tasks

Doc fixes

bc3e05c

danielabrozzoni approved these changes Mar 13, 2023

View reviewed changes

notmandatory merged commit cd4945a into bitcoindevkit:master Mar 14, 2023

evanlinjin mentioned this pull request Mar 19, 2023

Implement EsploraExt for esplora_client::AsyncClient #880

Closed

notmandatory mentioned this pull request Mar 20, 2023

Release 1.0.0-alpha.0 #910

Closed

29 tasks

DanGould mentioned this pull request Mar 24, 2023

Socratic Seminar 14 Reading List (March 2023) arminsabouri/bostonbitdevs#15

Closed

notmandatory assigned LLFourn Mar 26, 2023

notmandatory added the BDK 1.0.0 label Mar 26, 2023

notmandatory removed the BDK 1.0.0 label May 6, 2023

realeinherjar mentioned this pull request Oct 20, 2023

ci: move to Nix #1165

Closed

45 tasks

realeinherjar mentioned this pull request Nov 15, 2023

chore: fix typos #1220

Merged

8 tasks

storopoli mentioned this pull request Jan 6, 2024

ci: move to Nix #1257

Closed

45 tasks

tnull mentioned this pull request Jan 7, 2024

Bump bip39 dependency to v2.0 #1259

Merged

Conversation

LLFourn commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things Done

APIs changed

Unfinished work

Notes to the reviewers

Checklists

All Submissions:

Bugfixes:

Uh oh!

Uh oh!

afilini left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielabrozzoni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LLFourn Nov 16, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LLFourn commented Nov 16, 2022

Uh oh!

tnull commented Dec 2, 2022

Uh oh!

LLFourn commented Dec 13, 2022

Uh oh!

tnull commented Dec 13, 2022

Uh oh!

LLFourn commented Dec 24, 2022

Uh oh!

LLFourn commented Feb 15, 2023

Uh oh!

LLFourn commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LLFourn commented Feb 16, 2023

Uh oh!

Uh oh!

Uh oh!

LLFourn commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

danielabrozzoni left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

LLFourn commented Nov 3, 2022 •

edited

Loading

LLFourn Nov 16, 2022 •

edited

Loading

LLFourn commented Feb 15, 2023 •

edited

Loading

LLFourn commented Feb 21, 2023 •

edited

Loading