Upgrade to rust-bitcoin 0.29 by afilini · Pull Request #770 · bitcoindevkit/bdk

afilini · 2022-09-30T14:43:45Z

Description

Upgrade BDK to rust-bitcoin 0.29

Missing pieces:

rust-miniscript update_output_with_descriptor - Add PsbtOutputExt::update_with_descriptor rust-bitcoin/rust-miniscript#465
rust-miniscript 8.0.0 release - Release 8.0.0 rust-bitcoin/rust-miniscript#462
Upgrade rust-hwi to bitcoin 0.29 Bump rust-bitcoin to 0.29 rust-hwi#50
Upgrade esplora-client to bitcoin 0.29 Upgrade bitcoin requirement from 0.28.1 to 0.29.1 rust-esplora-client#20
Upgrade rand to 0.8 like secp256k1 did

Notes to the reviewers

The commits still need to be reordered and cleaned up

Changelog notice

Upgrade rust-bitcoin to 0.29
Remove deprecated "address validators"

Checklists

All Submissions:

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

notmandatory · 2022-10-22T19:19:09Z

I added a link in the description to bitcoindevkit/rust-esplora-client#20, it should be merged shortly and then we can create a new rsut-esplora-client release.

notmandatory · 2022-10-25T20:55:20Z

src/testutils/blockchain_tests.rs

@@ -1,8 +1,18 @@
+// Bitcoin Dev Kit
+//
+// Copyright (c) 2020-2021 Bitcoin Dev Kit Developers


Small nit, the year should be bumped to 2022 or soon 2023. But I think we should do this for all the files that need updating in one PR. I'll open a PR and we can discuss there.

src/wallet/coin_selection.rs

notmandatory · 2022-10-25T21:10:54Z

src/wallet/mod.rs

        let addr = wallet.get_address(AddressIndex::New).unwrap();

-        let path = vec![("rn4nre9c".to_string(), vec![0])]
+        let path = vec![("e5mmg3xh".to_string(), vec![0])]


Just curious, why did the policy path id change here? is it related to the miniscript update?

notmandatory · 2022-10-25T21:17:08Z

src/wallet/utils.rs

-        assert!(result);
-    }
-
-    #[test]


Were the tests here removed because they aren't needed/valid? or are did they break and need to be fixed?

notmandatory

ACK c7a43d9

I was only able to do a shallow review, and had a few questions but no show stoppers. Since this is primarily version bumping dependencies I'm counting our our test coverage to ensure nothing important was broke.

danielabrozzoni

I have some comments, I still need to review src/descriptor/mod.rs

danielabrozzoni · 2022-10-26T07:43:10Z

Cargo.toml


+# This feature is used to run `cargo check` in our CI targeting wasm. It's not recommended
+# for libraries to explicitly include the "getrandom/js" feature, so we only do it when
+# necessary for running our CI. See: https://docs.rs/getrandom/0.2.8/getrandom/#webassembly-support


nit: add something along the lines as

If you plan to use BDK with wasm, don't include this feature, but include getrandom instead. More info here: link to README

I found this a little confusing too, but I was thinking a little example project would be the best way to demo how to use bdk + wasm.

src/wallet/coin_selection.rs

danielabrozzoni · 2022-10-26T07:46:10Z

examples/address_validator.rs

@@ -1,63 +0,0 @@
-// Bitcoin Dev Kit


Why are address validators being removed? Is it because they are replaced by something in the new rust-bitcoin? (If you could expand a bit more in the changelog as well, it'd be great!)

Ohhh they were deprecated already, sorry

danielabrozzoni · 2022-10-26T07:59:54Z

src/wallet/utils.rs

+                >= self
+                    .create_height
+                    .unwrap_or(0)
+                    .checked_add(n.to_consensus_u32())


if n is in seconds, doing create_height + *seconds value* is wrong

Ugggggh this is the good ol' #642, which we never bothered to fix (an attempt was made in #669)

We ignored this for 4 months now, it can wait a little bit longer

danielabrozzoni · 2022-10-27T12:33:02Z

src/descriptor/policy.rs

            }
+
+            // Unsupported
+            Terminal::RawPkH(_) => None,


I am not really sure what's going on here, can you add a little comment explaining why it's unsupported? (If it's trivial, it's ok if you just reply to this comment without commenting the code :))

danielabrozzoni · 2022-10-27T12:39:19Z

src/wallet/mod.rs

-        psbt_input.redeem_script = derived_descriptor.psbt_redeem_script();
-        psbt_input.witness_script = derived_descriptor.psbt_witness_script();
+        psbt_input
+            .update_with_descriptor_unchecked(&derived_descriptor)


Can you add a comment explaining why it's unchecked here?

afilini added this to the Release 0.24.0 Feature Freeze milestone Sep 30, 2022

evanlinjin mentioned this pull request Oct 9, 2022

WIP: bdk_core::coin_select integration #773

Closed

8 tasks

notmandatory assigned afilini Oct 9, 2022

afilini mentioned this pull request Oct 10, 2022

Problem at use bdk 0.19 with wasm #646

Closed

afilini force-pushed the upgrade/rust-bitcoin-29 branch from 4116dfa to 8cab6f0 Compare October 24, 2022 09:35

This was referenced Oct 24, 2022

W43 BDK Library Team Call bitcoindevkit/.github#30

Closed

Use Esplora's get_block_hash() method #783

Closed

benthecarman mentioned this pull request Oct 24, 2022

Implement BDK Database types for gloo::LocalStorage MutinyWallet/mutiny-node#5

Merged

afilini added 3 commits October 25, 2022 11:08

Remove deprecated address validators

9854fd3

Upgrade rand to 0.8

ae4f4e5

Upgrade to rust-bitcoin 0.29

1ffd59d

afilini force-pushed the upgrade/rust-bitcoin-29 branch from 15c9385 to 1ffd59d Compare October 25, 2022 09:18

afilini marked this pull request as ready for review October 25, 2022 09:19

Remove unused code

c7a43d9

notmandatory mentioned this pull request Oct 25, 2022

Fix how descriptor checksums are calculated #765

Merged

5 tasks

notmandatory reviewed Oct 25, 2022

View reviewed changes

src/wallet/coin_selection.rs Show resolved Hide resolved

notmandatory reviewed Oct 25, 2022

View reviewed changes

notmandatory approved these changes Oct 25, 2022

View reviewed changes

notmandatory mentioned this pull request Oct 27, 2022

Release 0.24.0 #787

Closed

24 tasks

notmandatory merged commit 5720e38 into bitcoindevkit:master Oct 27, 2022

danielabrozzoni reviewed Oct 27, 2022

View reviewed changes

Conversation

afilini commented Sep 30, 2022 • edited by notmandatory 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 Oct 22, 2022

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!

notmandatory 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!

danielabrozzoni 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!

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 Oct 27, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

afilini commented Sep 30, 2022 •

edited by notmandatory

Loading

notmandatory left a comment •

edited

Loading

danielabrozzoni Oct 27, 2022 •

edited

Loading