Skip to content

Use descriptor wallet for Bitcoin Core >= v0.21.0#737

Merged
ben-kaufman merged 7 commits into
cryptoadvance:masterfrom
Sjors:2020/12/core-21
Feb 16, 2021
Merged

Use descriptor wallet for Bitcoin Core >= v0.21.0#737
ben-kaufman merged 7 commits into
cryptoadvance:masterfrom
Sjors:2020/12/core-21

Conversation

@Sjors
Copy link
Copy Markdown
Contributor

@Sjors Sjors commented Dec 4, 2020

First part of #707.

This PR checks if Bitcoin Core is v0.21.0 or above. If so it creates a descriptor wallet.

We then check for each wallet if it's a descriptor wallet (since the node may have been upgraded after a wallet was created), using getwalletinfo.

Descriptor wallets use importdescriptors instead of importmulti. This means the getnewaddress RPC works as well as the Receive tab in the Bitcoin Core GUI.

Balance is checked with mine rather than watchonly for descriptor wallets.

For descriptor wallets we no longer refill the keypool. There's a bunch of other address management code that can probably be skipped, but I don't know how it works. I bypassed the self._addresses.add stuff in keypoolrefill, which seems to work.

@Sjors Sjors force-pushed the 2020/12/core-21 branch 6 times, most recently from 62efd30 to 54799c2 Compare December 4, 2020 19:33
@stepansnigirev
Copy link
Copy Markdown
Contributor

Thanks a lot for the PR! Looks good.
I would like to test it a bit before merging, but I am really excited about this PR and how much we can simplify with Core 21!

self._addresses are currently used to get the derivation path and derive descriptors for receiving addresses without rpc calls to Core, but I think we can slightly refactor that and only store labels there. We can derive descriptors for receiving addresses from the index, without lookups (that makes more sense).

I'll have a look over the weekend what we need to change for that and also test your PR :)

@Sjors
Copy link
Copy Markdown
Contributor Author

Sjors commented Dec 5, 2020

There's also the deriveaddressess RPC call. It might be best if you add some commits to this PR to deal with that stuff.

Another thing that would be useful is if the test suite can download Bitcoin Core binaries and test against different versions (since we really should test against 0.21 and an older version). This script makes that very easy: https://github.com/bitcoin/bitcoin/blob/master/test/get_previous_releases.py

@k9ert
Copy link
Copy Markdown
Contributor

k9ert commented Dec 15, 2020

For the reference, i'm adding this link which seems to be the best source in order to understand what this PR is about:
bitcoin/bitcoin#15764

@Sjors
Copy link
Copy Markdown
Contributor Author

Sjors commented Dec 17, 2020

I rebased on master, so there's no need for the merge commit...

@ben-kaufman
Copy link
Copy Markdown
Contributor

Looks great, thanks! The only thing which is an issue currently is that without the keypool management from Specter the addresses caching in CSV stops working, and we currently rely on that for quite a bit. We'll have to figure out first how to keep the addresses caching up to date properly without relying on keypool refill events on Specter.

@ben-kaufman
Copy link
Copy Markdown
Contributor

Hi @Sjors, I've worked on this PR now to fix the address caching issue, and also a coin control issue and support the new add_inputs flag in walletcreatefundedpsbt.

Would it be fine if I just push directly over here to this PR?

@Sjors
Copy link
Copy Markdown
Contributor Author

Sjors commented Feb 15, 2021

@ben-kaufman that's fine by me

@ben-kaufman
Copy link
Copy Markdown
Contributor

Great, just pushed it, tested with Core 0.21 and 0.20, and everything looks good to me now

@Sjors
Copy link
Copy Markdown
Contributor Author

Sjors commented Feb 15, 2021

tACK c8dd881

Tested on Signet with Bitcoin Core v0.21 by creating a new 2-of-2 wallet, receiving coins on it and then sending them.

@ben-kaufman ben-kaufman merged commit 790bfbb into cryptoadvance:master Feb 16, 2021
@Sjors Sjors deleted the 2020/12/core-21 branch February 16, 2021 09:03
Comment on lines +194 to +202
len(
[
tx
for tx in self._transactions
if self._transactions[tx]["category"] != "send"
and not self._transactions[tx]["address"]
]
)
!= 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In #1316 i'm currently migrating to v0.21.1 and the tests are hanging in this endless loop.
How does this loop is suppose to end, @Sjors ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@stepansnigirev a Python strangle?

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.

4 participants