[test] add tests to verify support for sending to bech32m address #402

sandipndev · 2021-07-20T08:01:22Z

Description

This PR is in reference to #396
It adds some tests to confirm sending transactions to Bech32m addresses, introduced in BIP350

Note: This change requires bitcoin-core v22.0 to be incorporated into bitcoind.

Checklists

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 Bech32m adoption by rust-bitcoin

RCasatta · 2021-07-20T08:49:35Z

Ideally, I think it's a test to be added (also) to the bdk_blockchain_tests! macro

notmandatory · 2021-07-20T14:27:57Z

Looks like bech32m addresses are not supported yet in rust-bitcoin 0.26.2. The PR that added support is rust-bitcoin/rust-bitcoin#601 and it has other breaking changes that will also need to be fixed in bdk. You can see the errors by updating bdk's Cargo.toml like this:

bitcoin = { git = "https://github.com/rust-bitcoin/rust-bitcoin.git", branch = "master", features = ["use-serde", "base64"] }

Also @RCasatta has a good idea about also updating the bdk_blockchain_tests macro, so this could could be a bit larger task but also a good opportunity to learn about macros. You'll also be able to get to know other parts of bdk by fixing the other errors to make bdk work with the master branch of rust-bitcoin (until a new released version is ready). Want to give it a go?

sandipndev · 2021-07-20T15:11:49Z

Thank you for going through the problem.

We can develop using the master branch but should we release the Bech32m adoption before rust-bitcoin officially releases a version upgrade with the same?

In most places, there is a similar error after changing Cargo.toml to use the master branch of rust-bitcoin.

the trait `InnerXKey` is not implemented for `bitcoin::util::bip32`

I'm on fixing it!

notmandatory · 2021-07-20T15:22:54Z

We can develop using the master branch but should we release the Bech32m adoption before rust-bitcoin officially releases a version upgrade with the same?

You will need to keep this PR in "draft" mode until an official rust-bitcoin release is ready, and then you can put that version in the Cargo.toml. In the mean time you can still work on getting ready for that release by making your fixes against the master branch.

notmandatory · 2021-07-26T19:02:53Z

The 0.27.0 version of rust-bitcoin is now released which includes bech32m support. I was able to get your new test to pass after these steps:

In Cargo.toml update bitcoin version to "^0.27"
Fork and make local clones of the rust-miniscript and rust-electrum-client projects and update both also to the rust-bitcoin version 0.27
In Cargo.toml update the dependencies there for rust-miniscript and rust-electrum-client to the local path of your updated clones of these project, something like this:
```
miniscript = { path = "../rust-miniscript" }
...
electrum-client = { path = "../rust-electrum-client", optional = true }
```

Give it a try and then we can discuss additional tests (as @RCasatta suggested) and how to make the updates to those other projects (if no one else is already working on it).

sandipndev · 2021-07-27T06:01:04Z

For the time being, I changed the dependencies to point to my git fork with updated rust-bitcoin v0.27 in order to check if all the test cases pass.

The added test case for bech32m is passing with cargo test.

But it looks like builds and some other test cases are still failing.

RCasatta · 2021-07-27T16:28:32Z

From a quick look, it seems you need to use patch in Cargo.toml for the rust-bitcoin 0.27 dep so that dependencies of dependencies are replaced as well, see https://doc.rust-lang.org/edition-guide/rust-2018/cargo-and-crates-io/replacing-dependencies-with-patch.html.

Otherwise, you may end up for something that seems the same Object but they are from different crate versions

sandipndev · 2021-09-13T16:33:54Z

Waiting for the following PRs to get merged:

notmandatory · 2021-09-14T00:13:02Z

I created a couple more PRs that also need to be merged to support this PR:

~~1. RCasatta/rust-bitcoincore-rpc#2~~ (optional, not really needed for this PR)

sandipndev · 2021-09-16T05:26:01Z

src/testutils/blockchain_tests.rs

+                test_client.bitcoind.client
+                    .import_descriptors(
+                        vec![
+                            ImportDescriptorRequest::new(wallet_descriptor, false),
+                            ImportDescriptorRequest::new(change_descriptor, false),
+                        ]
+                    ).unwrap();


Updated this section to match the newer prototype of import_descriptors in rust-bitcoincore-rpc

notmandatory · 2021-11-02T23:03:11Z

Track via #63

notmandatory · 2021-11-09T22:02:28Z

This needs to be reworked a little to use raw rpc calls to create our test tr() descriptor wallet as @RCasatta suggested. Once rust-bitcoin/rust-bitcoincore-rpc#174 is merged we can rework the tests to use the lib instead.

Moving to '0.15.0' feature freeze so we have more time to work on it.

notmandatory · 2021-11-11T06:15:13Z

I got the blockchain_tests to work without the core-rpc or bitcoind patches 🎉. Once I have my code cleaned up I'll add it as a commit to this PR. Moving this back into code freeze for 0.14.0.

Now works with latest released versions of rust-bitcoincore-rpc and bitcoind. Once these crates are updated to support creating descriptor wallets and add importdescriptors and bech32m support this test will need to be updated.

notmandatory

ACK afa1ab4

notmandatory · 2021-11-12T16:53:09Z

FYI, here is an explanation from stackexchange that we followed for creating a P2TR wallet in bitcoind and then creating a deposit address for it: https://bitcoin.stackexchange.com/questions/108006/how-to-make-a-taproot-transaction-with-bitcoin-cli

sandipndev · 2021-11-14T20:23:18Z

Awesome, thanks for fixing this PR. Calling the RPC commands, for now, seems to work perfectly and tests run - which is fantastic and ACK afa1ab4 🎉

rajarshimaitra

tACK afa1ab4.

below are few minor question and suggestions.

src/testutils/blockchain_tests.rs

sandipndev commented Sep 16, 2021

View reviewed changes

notmandatory added this to the Release 0.14.0 Feature Freeze milestone Oct 19, 2021

notmandatory mentioned this pull request Oct 26, 2021

switch back to use bitcoincore-rpc, remove create_wallet descriptor p… rust-bitcoin/bitcoind#37

Merged

notmandatory mentioned this pull request Nov 2, 2021

Taproot / Schnorr #63

Closed

8 tasks

notmandatory modified the milestones: Release 0.14.0 Feature Freeze, Release 0.15.0 Feature Freeze Nov 9, 2021

notmandatory modified the milestones: Release 0.15.0 Feature Freeze, Release 0.14.0 Feature Freeze Nov 11, 2021

sandipndev and others added 3 commits November 11, 2021 08:20

Added a wallet unit test to send to a Bech32m address

54f61d1

Added wallet blockchain test to send to Bech32m address

632422a

Fix blockchain_tests::test_send_to_bech32m_addr

afa1ab4

Now works with latest released versions of rust-bitcoincore-rpc and bitcoind. Once these crates are updated to support creating descriptor wallets and add importdescriptors and bech32m support this test will need to be updated.

notmandatory marked this pull request as ready for review November 11, 2021 21:59

notmandatory approved these changes Nov 12, 2021

View reviewed changes

rajarshimaitra reviewed Nov 16, 2021

View reviewed changes

src/testutils/blockchain_tests.rs Show resolved Hide resolved

src/testutils/blockchain_tests.rs Show resolved Hide resolved

src/testutils/blockchain_tests.rs Show resolved Hide resolved

notmandatory merged commit afa1ab4 into bitcoindevkit:master Nov 17, 2021

notmandatory mentioned this pull request Nov 30, 2021

Add support for sending to bech32m addresses #396

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test] add tests to verify support for sending to bech32m address #402

[test] add tests to verify support for sending to bech32m address #402

sandipndev commented Jul 20, 2021 •

edited

Loading

RCasatta commented Jul 20, 2021

notmandatory commented Jul 20, 2021

sandipndev commented Jul 20, 2021

notmandatory commented Jul 20, 2021

notmandatory commented Jul 26, 2021 •

edited

Loading

sandipndev commented Jul 27, 2021

RCasatta commented Jul 27, 2021 •

edited

Loading

sandipndev commented Sep 13, 2021

notmandatory commented Sep 14, 2021 •

edited

Loading

sandipndev Sep 16, 2021

notmandatory commented Nov 2, 2021

notmandatory commented Nov 9, 2021

notmandatory commented Nov 11, 2021 •

edited

Loading

notmandatory left a comment

notmandatory commented Nov 12, 2021

sandipndev commented Nov 14, 2021

rajarshimaitra left a comment

[test] add tests to verify support for sending to bech32m address #402

[test] add tests to verify support for sending to bech32m address #402

Conversation

sandipndev commented Jul 20, 2021 • edited Loading

Description

Checklists

New Features:

RCasatta commented Jul 20, 2021

notmandatory commented Jul 20, 2021

sandipndev commented Jul 20, 2021

notmandatory commented Jul 20, 2021

notmandatory commented Jul 26, 2021 • edited Loading

sandipndev commented Jul 27, 2021

RCasatta commented Jul 27, 2021 • edited Loading

sandipndev commented Sep 13, 2021

notmandatory commented Sep 14, 2021 • edited Loading

sandipndev Sep 16, 2021

Choose a reason for hiding this comment

notmandatory commented Nov 2, 2021

notmandatory commented Nov 9, 2021

notmandatory commented Nov 11, 2021 • edited Loading

notmandatory left a comment

Choose a reason for hiding this comment

notmandatory commented Nov 12, 2021

sandipndev commented Nov 14, 2021

rajarshimaitra left a comment

Choose a reason for hiding this comment

sandipndev commented Jul 20, 2021 •

edited

Loading

notmandatory commented Jul 26, 2021 •

edited

Loading

RCasatta commented Jul 27, 2021 •

edited

Loading

notmandatory commented Sep 14, 2021 •

edited

Loading

notmandatory commented Nov 11, 2021 •

edited

Loading