Skip to content
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

multi descriptors are dangerously converted to sortedmulti #79

Closed
turkycat opened this issue Feb 12, 2025 · 4 comments · Fixed by #80
Closed

multi descriptors are dangerously converted to sortedmulti #79

turkycat opened this issue Feb 12, 2025 · 4 comments · Fixed by #80

Comments

@turkycat
Copy link

I'm trying out your wallet, and I think it seems great so far! I would love to find a replacement for Sparrow.

Unfortunately, I have an issue with it which makes it incompatible with a wallet I am trying to import which is a deal breaker for me.

I went through the steps to import a wallet, and pasted in my testnet descriptor

wsh(multi(2,[ed80faab/48h/1h/0h/2h]tpubDFSeJaooCFfYxYbWYUYw5ZRwHTGxSkkBCjXTMDSDFeyMfDHQKF9fzWVeRGhxCtVxL2ArZiMNAKXoaGbFrVy1GEu9o9Ni4c71MoEWGsV5ZBS/0/*,[c0640ad9/48h/1h/0h/2h]tpubDEvqpafwUhWSK2D3b3KPLrdaBsFisxqupBJLkKrvFt468f9s5QJKZy7GheFGoMyaJQAaV72gAf5Bigc8Frymj1tTQ1mQx8yCUpeGg2QN7KT/0/*,[4923b317]tpubD6NzVbkrYhZ4WaMzUcZBkvm9ywxAR1z3aJZrnPH4e9gigfrXPazNjKw3UEUWeKWLAY1CSTQLphmv7CmEHe6jUMfnoK4f88QD8YYbGgdTcsq/0/*))#c7dlylt6

note that this descriptor uses multi, not sortedmulti.

please see this screenshot I created when building a guide to demonstrate that I am importing a multi descriptor:

Image

Everything seemed to work fine, but when I went to the newly-created wallet and double-checked the descriptor, it has been automagically changed to sortedmulti!

Image

multi should be allowed

You may argue that there is no reason to use multi, but please allow me to present a case for it.

  • bip67 (which describes the process to sorting public keys) predates the descriptor standard. If there was no value in using multi, then why did the descriptor standard include it?
  • By accepting the descriptor and then converting it, you are seriously endangering your users who do not understand the change.
  • You claim this software is backed by BDK. BDK supports multi descriptors. (My use of the word 'claim' is only due to the fact that I haven't verified this).
  • By using multi I can create a proof of which keys were used to sign my transactions, since I know which order they must appear in the locking script.
  • I know of at least two enterprise-level companies who use multi in their wallet solutions.
  • multi is yet another customization parameter for an advanced wallet which, similar to custom derivation paths and passphrases, can allow advanced users to create custom and complex configurations of wallets should they feel comfortable doing so.
@andreasgriffin
Copy link
Owner

Thank you for opening this issue. I will take a close look at this asap!

@andreasgriffin
Copy link
Owner

andreasgriffin commented Feb 13, 2025

Thank you again for finding this bug.

Bitcoin Safe will (for now) not support multi()
Reason: A descriptor can be a tree of nested descriptors. This is a UX nightmare and complexity that probably will end up as foot-guns. Therefore Bitcoin Safe only allows a very small selection (templates) of descriptors to be used (see other discussion #64 ).
While multi is allowed in descriptors, it is yet another foot-gun for normal users, who do not understand why the order of xpubs should matter.

I would be interested to learn however why the enterprises use multi instead of sortedmulti. In principle Bitcoin Safe could use all bdk supported descriptors in a reduced UI (no keystore UI, no signing UI, ....).

@andreasgriffin
Copy link
Owner

andreasgriffin commented Feb 13, 2025

Merged andreasgriffin/bitcoin-usb#33
Will be included with the next release: #80

@turkycat
Copy link
Author

throwing an exception is an improvement for the safety of users, but it's disappointing to not support the full descriptor standard. I won't be able to use or recommend your wallet for this reason.

anyways, thanks for your quick response and action

andreasgriffin added a commit that referenced this issue Feb 16, 2025
- Fixes failed Camera open on Mac
andreasgriffin/bitcoin-qr-tools#44
- Fixes #79
- Fixes RBF TXO adding 
- Fixes Nostr Sync&Chat at initial connection
andreasgriffin/bitcoin-nostr-chat#24
- Added 🇲🇲 Burmese - မြန်မာ, 🇰🇷 Korean - 한국어
- Added qr scanning animation
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 a pull request may close this issue.

2 participants