Skip to content

Adding importdescriptors for Descriptor-Based Wallets #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 15, 2021

Conversation

sandipndev
Copy link

@sandipndev sandipndev commented Aug 30, 2021

This PR adds the following features:

  1. importdescriptors RPC command
  2. Bech32m address support
  3. Updated createwallet to add in descriptors parameter

The integration_test/run.sh script was updated to support the latest versioning of Bitcoin Core.

I figure this would be very useful for anyone creating descriptor based wallets, especially with Taproot coming in!

@sandipndev
Copy link
Author

Hi, I am facing a challenge in this PR.

There seems to be a difference between the results coming from bitcoin-cli and bitcoin json-rpc when the same payload is being passed to both of them.

Reproduction:

  1. I am using bitcoind v22.0rc2, which is unreleased and compiled from source with wallet support.
  2. I commented out the kill on this line here and ran ./run.sh.
  3. With the kill now gone, bitcoind is running and listening to RPC on port 12349.
  4. Paste the payload, it contains the descriptors:
PAYLOAD="[{\"active\":true,\"desc\":\"wpkh(tprv8ZgxMBicQKsPeTNzU5evMToRhHA9h3UCxbpzrzjUWUd6TQktkhmY82dQx5f6QaFpSWMzZxKz16xQFGeW7ykPjYuTetU6ep9aFTpAt7jKhPU/44'/0'/0'/0/*)#w2lyh4jx\",\"internal\":false,\"range\":[0,100],\"timestamp\":\"now\"},{\"active\":true,\"desc\":\"wpkh(tprv8ZgxMBicQKsPeTNzU5evMToRhHA9h3UCxbpzrzjUWUd6TQktkhmY82dQx5f6QaFpSWMzZxKz16xQFGeW7ykPjYuTetU6ep9aFTpAt7jKhPU/44'/0'/0'/0/*)#w2lyh4jx\",\"internal\":true,\"range\":[0,999],\"timestamp\":\"now\"}]"
  1. Run bitcoin-cli command: (It should pass with success messages)
bitcoin-cli -regtest -datadir=/tmp/rust_bitcoincore_rpc_test/2 --rpcport="12349" importdescriptors $PAYLOAD
  1. Restart ./run.sh
  2. Now, we are going to pass the same payload via json-rpc using curl. Run the curl command:
curl --user $(cat /tmp/rust_bitcoincore_rpc_test/2/regtest/.cookie) --data-binary "{\"jsonrpc\": \"1.0\", \"id\": \"test\", \"method\": \"importdescriptors\", \"params\": $PAYLOAD}" -H 'content-type: text/plain;' http://localhost:12349/ | jq

It returns an error code -1 with the importdescriptors help message.
This is what is confusing to me, why does the same payload produce different results?

This is also the reason the test corresponding to this PR gets failed. Can anyone help me out on this?

@notmandatory
Copy link

I was able to get the request passed and successful via the rpc-client test. I had to make the request with a single argument which contains the a json array value containing the json descriptor objects. I'm now getting an error parsing the success result, but once that's fixed I can review it with you.

@sandipndev
Copy link
Author

Thanks to @notmandatory, we resolved this issue. It's ready to review now!

@sandipndev sandipndev marked this pull request as ready for review September 2, 2021 18:03
Copy link

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 4c93f8b

@RCasatta
Copy link
Owner

RCasatta commented Sep 3, 2021

from a quick look, the PR is good but I am not yet sure I am going to maintain this repo unless there are no other options.

This is a topic we will discuss in the next bdk meeting

@notmandatory
Copy link

notmandatory commented Sep 10, 2021

@sandipndev can you also make a version of this PR for the https://github.com/rust-bitcoin/rust-bitcoincore-rpc?

@RCasatta we discussed the issue a little in our last bdk team chat and I think the consensus was to submit PRs to both your fork and the upstream until upstream gets caught up.

…scriptors parameter and added bech32m address type
Copy link
Owner

@RCasatta RCasatta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack a3df1c2

@RCasatta RCasatta merged commit a3df1c2 into RCasatta:master Sep 15, 2021
@notmandatory
Copy link

post merge ACK a3df1c2

good job @sandipndev! 🎉 don't forget to also update rust-bitcoin#199 to match this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants