Can't import tr descriptor with xpub that has no origin information

sparrowwallet / sparrow

Desktop Bitcoin Wallet focused on security and privacy. Free and open source.

https://sparrowwallet.com/

Apache License 2.0

1.36k stars 192 forks source link

Can't import tr descriptor with xpub that has no origin information #1443

Closed LLFourn closed 4 months ago

LLFourn commented 5 months ago

There are several bugs with respect to importing descriptors into sparrow (given my understanding of them).

Can't import descriptors without fingerprint

This is a valid descriptor and should work:

tr(tpubD6NzVbkrYhZ4WgbWCZzth8yn8BEuHduo3Yqn1mNrPF5o48BWW9FS4dkytsAEcLtDjJNP6m1TMmFRVL6KKsDZJow7JQpwpdnnK2ZwaWCyt8r/<0;1>/*)#ckrgkmx6

But sparrow refuses to import it

~Gives out wrong derivation paths for addresses~ see correction comment

Ok so let's say you transform this into

tr([4c16798e/0]tpubD6NzVbkrYhZ4WgbWCZzth8yn8BEuHduo3Yqn1mNrPF5o48BWW9FS4dkytsAEcLtDjJNP6m1TMmFRVL6KKsDZJow7JQpwpdnnK2ZwaWCyt8r/<0;1>/*)

It will start giving out the correct addresses but give the wrong paths:

The actual path of that address is 0/0

Change this first strange index to 1337:

tr([4c16798e/1337]tpubD6NzVbkrYhZ4WgbWCZzth8yn8BEuHduo3Yqn1mNrPF5o48BWW9FS4dkytsAEcLtDjJNP6m1TMmFRVL6KKsDZJow7JQpwpdnnK2ZwaWCyt8r/<0;1>/*)

And you get the same addresses!

LLFourn commented 5 months ago

Oops I just realized that the fingerprint derivation path is not a bug since that's the origin information about the key. The bug is just that I can't import a key without origin information. Furthermore, I can't import a descriptor with origin information that is is just the master path.

This is the explicit (and syntactically correct I think) form of the original:

tr([4c16798e/m]tpubD6NzVbkrYhZ4WgbWCZzth8yn8BEuHduo3Yqn1mNrPF5o48BWW9FS4dkytsAEcLtDjJNP6m1TMmFRVL6KKsDZJow7JQpwpdnnK2ZwaWCyt8r/<0;1>/*)

This doesn't work since it edits away the origin info afterwards.

Interestingly, if I put it in wrongly first I can manually change the derivation path to just m which then works:

I think that the no derivation path form should work too (but it doesn't):

tr([4c16798e]tpubD6NzVbkrYhZ4WgbWCZzth8yn8BEuHduo3Yqn1mNrPF5o48BWW9FS4dkytsAEcLtDjJNP6m1TMmFRVL6KKsDZJow7JQpwpdnnK2ZwaWCyt8r/<0;1>/*)

I forgot to mention this is v1.9.1

craigraw commented 5 months ago

Thanks for the bug report.

This is the explicit (and syntactically correct I think) form of the original: tr([4c16798e/m]tpubD6NzVbkrYhZ4WgbWCZzth8yn8BEuHduo3Yqn1mNrPF5o48BWW9FS4dkytsAEcLtDjJNP6m1TMmFRVL6KKsDZJow7JQpwpdnnK2ZwaWCyt8r/<0;1>/*)

I'm not sure this is syntactically correct - it doesn't fit the description for key origin information in https://github.com/bitcoin/bips/blob/master/bip-0380.mediawiki#key-expressions. Sparrow is currently creating output descriptors that look like this though, so that's probably a bug.

I think that the no derivation path form should work too (but it doesn't): tr([4c16798e]tpubD6NzVbkrYhZ4WgbWCZzth8yn8BEuHduo3Yqn1mNrPF5o48BWW9FS4dkytsAEcLtDjJNP6m1TMmFRVL6KKsDZJow7JQpwpdnnK2ZwaWCyt8r/<0;1>/*)

Agreed, this should work.

I am testing fixes for these edge cases, but it would be good to get confirmation on this, so let me know if you disagree with the above.

LLFourn commented 5 months ago

Thanks for the bug report.

This is the explicit (and syntactically correct I think) form of the original: tr([4c16798e/m]tpubD6NzVbkrYhZ4WgbWCZzth8yn8BEuHduo3Yqn1mNrPF5o48BWW9FS4dkytsAEcLtDjJNP6m1TMmFRVL6KKsDZJow7JQpwpdnnK2ZwaWCyt8r/<0;1>/*)

I'm not sure this is syntactically correct - it doesn't fit the description for key origin information in https://github.com/bitcoin/bips/blob/master/bip-0380.mediawiki#key-expressions. Sparrow is currently creating output descriptors that look like this though, so that's probably a bug.

I think that the no derivation path form should work too (but it doesn't): tr([4c16798e]tpubD6NzVbkrYhZ4WgbWCZzth8yn8BEuHduo3Yqn1mNrPF5o48BWW9FS4dkytsAEcLtDjJNP6m1TMmFRVL6KKsDZJow7JQpwpdnnK2ZwaWCyt8r/<0;1>/*)

Agreed, this should work.

I am testing fixes for these edge cases, but it would be good to get confirmation on this, so let me know if you disagree with the above.

I agree. You're interpretation is correct. I thought it was syntactically correct since rust bitcoin's bip32 module supported it.

craigraw commented 4 months ago

Fixed in https://github.com/sparrowwallet/drongo/commit/f3ee296280789e6c7e57f03036139b80f9f6e3a7. Sparrow will now follow BIP380 in requiring key origins to have a master fingerprint. A key derivation path is not necessary, and will be entirely omitted from the output descriptor if not present.