sparrow icon indicating copy to clipboard operation
sparrow copied to clipboard

import descriptor 'multi' support

Open DanConwayDev opened this issue 2 years ago • 2 comments

I really appropriate the effort you have put into making such a fully featured wallet.

Adding output descriptor with the format wsh(multi(2,xpub1/*,xpub2/*,xpub3/*))#checksum gets translated into wsh(sortedmulti(2,Keystore1,KeyStore2,Keystore3)) which doesn't find any of the wallets transactions.

I'm therefore pretty surprised that tests that use the specter-multisig-wallet.json wsh multi descriptor pass.

Is this a bug or are threshold signature alternatives to sortedmulti not supported? (multi, multi_a and sortedmulti_a)

Also, from a UX standard point I would prefer it to warn me that it is going to assume default derivations / master fingerprints rather than force me to input them individually for every key.

Once again - I really appropriate your valuable contribution in the space.

DanConwayDev avatar Oct 07 '22 21:10 DanConwayDev

Is this a bug or are threshold signature alternatives to sortedmulti not supported? (multi, multi_a and sortedmulti_a)

Sparrow conforms to BIP67, meaning all keys in a multisig script are sorted lexicographically in order to generate the same addresses regardless of the arbitrary order of creating the keystores. This rules out multi in favour of sortedmulti. Taproot multisig is not yet supported (specifications are still not finalized), which rules out multi_a and sortedmulti_a.

Also, from a UX standard point I would prefer it to warn me that it is going to assume default derivations / master fingerprints rather than force me to input them individually for every key.

It's not clear to me what you mean here? What behaviour would you like to see? Sparrow makes the fields for both master fingerprint and derivation clear and visible.

craigraw avatar Oct 10 '22 06:10 craigraw

I fully agree that wallets should be discouraging the use of multi.

I would suggest that a warning message should appear to indicate that a legacy version of threshold signatures is being used and a suggestion that the user should move to a sortedmulti wallet. This could either 1) state that the import descriptor is not supported or 2) give the user to option to proceed in any case.

Here is the case for allow the user continue with the legacy multi wallet:

  1. Some users have arranged their affairs such that changing their multi-sig setup is non-trivial
  2. Some users would like to be able to view their previous transaction history for many wallets within a single secure desktop setup. Sparrow wallet, with its advanced features and options is the ideal wallet for this

User Interaction

As a app developer I understand that building great UX without spying on users to find out exactly how they are (miss)using it is hard. It is difficult to know what mistakes users are making and test changes to keep them on the straight and narrow.

I have outlined my interactions 1) underpin my suggestion of a warning message and 2) to help you understand how I was using it in case you find anything actionable.

Thanks again for your significant contribution to the ecosystem.

  1. File > Import Wallet
  2. Optput Descriptor (Import File...)
  • select txt file > name wallet
  • select 'has existing transaction', select date, click 'Import Wallet'
  • enter password 'test' and confirm it, click 'Set Password'
  • 'Encrypting Wallet' appears with a loading bar right at the bottom of the page and disappears when the loading bar completes.
  1. I wait a few seconds for something to happen...
  2. I wait a few more seconds for something to happen...
  • File > Open Wallet... > select wallet-name.mv.db > Open > enter password and press 'Unlock'
  • "Invaid Password, the wallet password was invalid. Try again?"
  • type the password again > same error
  • press cancel and open it again > same error
  1. assume i got the password wrong so repeat step 2 - 4 > same error.
  2. turn off networking and repeat steps 2-4 with 'No Password' > this time the error is 'Error Opening Wallet, Wallet file is not valid'.
  • checks import file - looks like a wallet descriptor to me...
  1. File > New Wallet
  2. click Script Policy 'Edit' button > past in text value of wallet descriptor file.
  • great populated as 'Multi Signature' with the correct cosigners and the N keystores. Lovely interface - clear and well presented.
  • 'Apply' button is disabled... Keystore tab headings have a red shadow... the Keystore labels within the descriptor are red...must be an issue with each xpub... it imports fine in bitcoin core...looks a bit closer...'Master Fingerprint' and 'Derivation' appear to have placeholder values that aren't entered...I'll enter a the placeholder as the master fingerprint... looks like a bunch of 0's. click on input and all placeholder disappears...how many 0's? I'll just start typing... ah the input loses its red shadow at some-point - that should be about right...derivation looks a bit harder...opens a text editor and copies the placeholder... types it back into the input... copies and pastes for N keys...presses 'Apply' sees it is connecting to bitcoin core and searching the blockchain... brilliant...waits..thats now disappeared...clicks UTXOs...no transactions....looks back at settings...imports the wallet descriptor into bitcoin core again and it loads it fine...goes down masterfinger print and derivation rabbit whole for quite a while...tries out a few different derivation paths, master finger prints and different ways to embed it directly into the descriptor rather than using the UI...eventually sees that it is using 'sortedmulti' rather than 'multi'...pastes it in a few times to confirm the behaviour...searches the codebase and identifies where it changes all multisig options to 'multi'...finds the .json file in the test director that uses 'wsh(multi'... raises the ticket.

DanConwayDev avatar Oct 10 '22 21:10 DanConwayDev

Thanks for the detailed notes!

Supporting non-BIP67 wallets is unfortunately not a priority - as you say it's discouraged and hopefully there are no current implementations that create such wallets.

I tested importing the multi output descriptor in the testcase you linked to above, setting a password, closing and then loading the wallet. Everything worked fine. I'm unsure why you got the results you did - certainly the networking status makes no difference. Can you reproduce this with the example output descriptor I use?

I've added a warning in ab2c7769 when creating/editing the output descriptor in the Settings tab and using multi instead of sortedmulti:

Screenshot 2022-10-24 at 16 46 25

craigraw avatar Oct 24 '22 14:10 craigraw

Closing this off as solved.

craigraw avatar Feb 15 '23 11:02 craigraw