Decent change management (using multipath descriptors) #74
Conversation
It supports multipath descriptors. We'll need to find a solution for the release
For multipath descriptors support
We'll change the semantic of the descriptor, so we need to make sure nothing accesses it with the old semantic.
We'll be able to derive change addresses too
Our bitcoind watchonly wallet could, maybe, have other descriptors that were imported. Sounds pretty unlikely since we use a dedicated wallet but hey. More importantly, we'll need to know the parent descriptor of the coin in order to recognize it as newly received or change.
In config, expect to be given a multipath descriptor that contains a derivation path for both receive and change addresses, but only for those. Instead of 'xpub/*', start using 'xpub/0/*' and 'xpub/1/*'. When creating the watchonly wallet on bitcoind import both the receive and change descriptors. When polling, check for coins on both descriptors.
The multipath descriptor has very different properties than the receive and change ones. Use a newtype to assist us in differentiating those.
This doubles the storage required but there is no way around it if we want the poller to detect those coins without grinding.
So we know what descriptor to use when spending it.
| @@ -57,7 +59,8 @@ CREATE TABLE coins ( | |||
| * we can get the derivation index from the parent descriptor from bitcoind. | |||
| */ | |||
| CREATE TABLE addresses ( | |||
| address TEXT NOT NULL UNIQUE, | |||
| receive_address TEXT NOT NULL UNIQUE, | |||
| change_address TEXT NOT NULL UNIQUE, | |||
There was a problem hiding this comment.
why having row in database with the tuple (receive and change) and not having a is_change bool with single address rows ?
There was a problem hiding this comment.
Maybe i misunderstand your question, but you need an address->derivation index mapping. You necessarily need a key per address in this mapping.
There was a problem hiding this comment.
I meant why storing:
| receive_address | change_address | derivation_index |
|---|---|---|
| addr1 | addr2 | 1 |
and not
| address | is_change | derivation_index |
|---|---|---|
| addr1 | false | 1 |
| addr2 | true | 1 |
You still have the mapping address->derivation index
There was a problem hiding this comment.
Because it's wasteful? That's 2 added integer per derivation index. What advantage would it have?
There was a problem hiding this comment.
To only store address that we use, but maybe that is not our process:
If we do no spend at all and receive only deposit until derivation_index = 10, does it means that we want to store the 10 change addresses that are not used at all ?
There was a problem hiding this comment.
I understand now that it can bring much more complexity to the code, and the table is more for short term caching until core take the burden. I am ok to not modify it
There was a problem hiding this comment.
This mapping is for detecting coins up to the gap limit, not to store addresses we used. Necessarily we need to have addresses up to the gap limit.
If we do no spend at all and receive only deposit until derivation_index = 10, does it means that we want to store the 10 change addresses that are not used at all ?
Are we sure we are never ever going to receive a payment on an address before derivation index 10? I don't think so at all. What if we gave away this address already? We trust others to not use it? What if we didn't but another wallet did? What if we recovered from backup? Etc..
|
Is it possible to add in this PR, helpers that are retrieving the timelock, the user and the heir multiXkey from the multipathdescriptor ? |
|
It'd be pretty orthogonal to this PR. But i can implement that for sure in another PR! Note you can already query the timelock though. Note for posterity: had a chat with Edouard and we won't do that for now. |
| let change_desc = self | ||
| .config | ||
| .main_descriptor | ||
| .receive_descriptor() |
There was a problem hiding this comment.
This was obviously wrong! I was using the receive key chain for the change address. See #79.
This fixes #18 by implementing the de-facto standard of using a
/0/*keychain for receiving addresses and a/1/*keychain for change addresses. Note that once we'll have multisig, reusing addresses will still be possible since wallet don't share the same "next derivation index".In order to avoid forcing the user to configure and backup two almost identical descriptors, we make use of the recently proposed BIP389 (bitcoin/bips#1354). In order to prevent as much as possible introducing a backward incompatibility in the configuration file after the first release, we restrict the usage of multipath descriptors to
<0;1>here.We now derive public keys from
xpub/0/*andxpub/1/*while we were previously deriving them fromxpub/*.This triggered a pretty invasive refactoring, as most parts of the codebase had to be updated to support the new change/receive separation (even the functional test miniscript dependency had to be updated, see darosior/python-bip380#21).
Broadly, this: