search

sparrowwallet / sparrow

Desktop Bitcoin Wallet focused on security and privacy. Free and open source.
https://sparrowwallet.com/
Apache License 2.0
1.29k stars 185 forks source link

Support for multisig "show address" feature on Ledger? #471

Open mflaxman opened 2 years ago

mflaxman commented 2 years ago

Ledger Version info:

Sparrow:

I get the following error: The Ledger Nano S and X do not support P2SH address display

Screen Shot 2022-03-18 at 4 03 38 PM

Does Ledger really not support this? I wanted to test what they're talking about in this blog post and am a bit confused.

Nit: I get that same error message regardless of whether the wallet is p2sh or p2wsh.

Great work on Sparrow btw, I've been quite impressed with all the progress!

mflaxman commented 2 years ago

I should note that the Ledger does appear to handle change address validation correctly on the spend, although it throws a weird error about using an unusual path (despite it being the standard path that Ledger default exported)

mflaxman commented 2 years ago

I'm thinking the issue is tied to the need to register the quorum on the device, and that I need to download the Ledger Live software for that? https://blog.ledger.com/bitcoin-2/

The screenshot in the blog post does indeed show a valid P2WSH address on the display of the device:

>>> from buidl import address_to_script_pubkey
>>> addr = "tb1qwuxulrpu5d02eag4tphxhamaa24s8sk8d5s7kw340cesr0wf87csks3c9a"
>>> type(address_to_script_pubkey(addr))
buidl.script.P2WSHScriptPubKey
craigraw commented 2 years ago

Great work on Sparrow btw, I've been quite impressed with all the progress!

Thanks!

The The Ledger Nano S and X do not support P2SH address display message is coming from HWI.

Incidentally, there has been a significant update to Ledger support in the upcoming 2.1.0 version of HWI, as discussed here: https://github.com/bitcoin-core/HWI/pull/550. The error is covered there, and described as 'expected behaviour', which I take to mean that Ledger does not support this functionality (despite the blog post)? The error text has been updated in the new version, but remains present.

mflaxman commented 2 years ago

Update: link to PoC implementation on Specter-Desktop if it's helpful for implementation: https://github.com/cryptoadvance/specter-desktop/pull/1513

From the November 2021 Ledger blog post I was under the impression that this was a feature that end-users could use. It seems like there's not yet any Coordinator software (Ledger Live, Sparrow, Specter, etc) capable of coordinating a multisig transaction with Ledger in the custom way it wants.

This ticket is not so much of a feature request (although more multisig Signers would be great!) mostly just updating the github issue with commentary for others that are confused why they can't get multisig transaction to work as described in Ledger's blog post.

mflaxman commented 2 years ago

Update: I was able to get a POC working in python: https://gist.github.com/mflaxman/386990be297e9a595936e8142e815e52

bigspider commented 2 years ago

Hello! First of all, great work!

And thanks @mflaxman for checking out the new Ledger app, and making a POC.

I'd really hope to get integrations in the ecosystem, and Sparrow is certainly high on my list.

Indeed, HWI's new release only partially supports the new features: the new registration flow for multisignature requires the software wallet to store a 32-byte hmac that proves that the wallet was already registered; HWI doesn't have an API for that, so instead it registers wallets at signing time (and then throws away the hmac). Therefore, at this time a full integration is not possible using just HWI (but it is possible with the native client library for the new app).

I made a more general proposal that extends to arbitrary miniscript wallets: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-May/020423.html − I'd love to hear your comments if you think any improvement is necessary. The more the standard works for everybody, the easier it will be to integrate and interoperate in a miniscript world.

Of course, please let me know if I can help in any way in integrating the new app.

craigraw commented 2 years ago

Although I'd like Sparrow to support Ledger's full multisig functionality, it's not particularly practical to support different applications for the large and growing number of hardware wallets. I'd like to pursue whether further capability in HWI is possible, and I've left a comment to this effect: https://github.com/bitcoin-core/HWI/issues/596#issuecomment-1124637176

I made a more general proposal that extends to arbitrary miniscript wallets

In this proposal I appreciate the attempt to narrow the scope of what kind of wallets are supported (which makes for more practical implementation), although I haven't fully understood what kind of wallets are not supported. Further I appreciate the conciseness achieved by the key information vector, but it was a bit unclear how the whole wallet policy looks (for example, when presented in an interface or written down). I am also following the conversation on bitcoin-dev.

PS I've made several unsuccessful attempts to request a demo device from Ledger (I don't find the simulator satisfactory to test fully) but I'm now finally considering buying a device. Is the Nano S still suitable for the foreseeable future?

bigspider commented 2 years ago

Although I'd like Sparrow to support Ledger's full multisig functionality, it's not particularly practical to support different applications for the large and growing number of hardware wallets. I'd like to pursue whether further capability in HWI is possible, and I've left a comment to this effect: bitcoin-core/HWI#596 (comment)

I agree that a proper HWI integration would be the best (and highest impact) way forward. Maybe I'll have a thought on how the HWI integration should look like − I think it is possible to add the feature without any breaking change in the current API.

In this proposal I appreciate the attempt to narrow the scope of what kind of wallets are supported (which makes for more practical implementation), although I haven't fully understood what kind of wallets are not supported. Further I appreciate the conciseness achieved by the key information vector, but it was a bit unclear how the whole wallet policy looks (for example, when presented in an interface or written down). I am also following the conversation on bitcoin-dev.

Maybe I should have added more examples in the post, indeed. I will probably follow up with another email with examples and clarifications, thanks for pointing that out.

bigspider commented 2 years ago

I realized I didn't fully answer your question:

although I haven't fully understood what kind of wallets are not supported.

The goal is to support all sane miniscript wallets. I cannot think of examples of interesting wallets that are not following that model. Perhaps the only interesting extensions could be: