HWI icon indicating copy to clipboard operation
HWI copied to clipboard

Improve support for the coming Ledger app 2.1.0

Open bigspider opened this issue 1 year ago • 2 comments

Intro and context

The upcoming version of the Ledger bitcoin app (version 2.1.0) will remove support to the legacy API (1.*).

A number of limitations of the 2.* protocol have been addressed since the first release:

  • Each returned signature is now accompanied by the corresponding pubkey, instead of just the input index.
  • Key origin information is no longer required for external xpubs.
  • Multiple internal xpubs are supported (the device will sign for each internal xpub).
  • Introduces a modified wallet policy language matching this proposal on bitcoin-dev, which addresses some drawbacks of the earlier version (most notably, removing the change/address-index from key information, and adding it to the policy's descriptor template instead).
  • Support for message signing was added.
  • OP_RETURN outputs are now supported.

Since there are incompatibilities, the modified protocol from version 2.1.0 is opt-in (signaled by using 01 instead of 00 in the p1 field of the APDU), and the protocol of version 2.0.* (using 00 in p1) will still be supported in future versions for some time. Nevertheless, the p1 == 0 protocol should is to be considered deprecated from the moment the 2.1.0 app goes live (expected in early November).

All 2.* versions before 2.1.0 support the legacy 1.x protocol; therefore, in order to keep things simple, this PR uses the legacy protocol for any version prior to 2.1.0). I believe disruption will be minimal: users of the 2.0.* app will downgrade their multisig experience to the legacy protocol (which doesn't recognize change addresses), and are therefore recommended to upgrade to 2.1.0. Nothing changes for users that are still on the 1.* app series (upgrade is, of course, still recommended!).

Moreover, the 2.1.0 adds full support to miniscript within wsh descriptors.

Changes in this PR

This is the list of changes in this PR:

  • Recognize "Bitcoin Legacy" and "Bitcoin Testnet Legacy" as a valid app name for the legacy protocol (regardless of the version). It will be deployed and available as an app in the Ledger app store.
  • Completely switch to the updated version (p1 = 1) of the new protocol starting from version 2.1.0; use legacy (1.*) protocol below version 2.1.0. Never retry with the legacy protocol on failure.
  • Remove checks for the limitations that were addressed.
  • Add support for message signing using the new protocol.
  • Add support for displaying multisig addresses.
  • Support multisig wallets with 16 keys (a previous version of the python library was limiting to 15 keys by mistake).

Miniscript support is left for a separate PR (more comments below).

Drawbacks

Signing a psbt and displaying an address requires registering the wallet policy on the device. The current version of HWI does that right before signing (and this PR adds the same behavior for displaying addresses).

This is not secure in a compromised machine if the device participates to multiple multisignature/script wallet accounts, as the adversary could trick the user into spending from a different wallet account.

Moreover, miniscript support in HWI is not in this PR, since it requires larger changes to HWI.

I'm working on a BIP for miniscript wallet policies, and will propose a backwards-compatible and vendor-agnostic way to add native support for wallet policies in a separate PR.

bigspider avatar Oct 13 '22 09:10 bigspider

@achow101, I didn't work on the tests − I'm not yet familiar with the test suite. Please advise accordingly if there are improvements you'd like me to make.

bigspider avatar Oct 13 '22 09:10 bigspider

I managed to run the test suite locally, but it seems to run with an old binary taken from the speculos repository. Not sure how to fix it.

Meanwhile, I rebased to fix a mistake I made in display_multisig_address, and added a few more commits to fix the remaining issues I'm aware of:

  • signmessage failing with the new app because of the requirement of ' instead of h for the hardened character − solved by always normalizing to ';
  • update speculos automation file for some slight UX changes;
  • enabled test for displaymultisigaddress if not legacy, and re-enabled all the multisig tests that were disabled.

Some work is still needed on test_display_address_multisig which is failing. The reason seems to be that HWI tests multisigs with:

  • xpubs like [f5acc2fd/48h/1h/0h/0h/0]<xpub>/0
  • bare pubkeys like [f5acc2fd/48h/1h/1h/0h/0/0]023f9fe73519d0bc86658151ef95d7844ed41033fc2051bf8291a1cf1153fcdf03

Both are not supported, as the app expects xpubs to be in the form: [f5acc2fd/48h/1h/0h/0h]<xpub>/<change>/<addr_index> − which is compatible with the needs of any current multisig software wallet that I'm aware of.

bigspider avatar Oct 14 '22 14:10 bigspider

I managed to run the test suite locally, but it seems to run with an old binary taken from the speculos repository. Not sure how to fix it.

It should be using a self built app, which overwrites the speculos provided binary. I'm working on fixing CI in general right now.

achow101 avatar Nov 02 '22 17:11 achow101

@achow101 I disabled again in 00a8c1b32e02d80fb72dc4fbfaae44b37474ece6 the tests for multisig address display, since I'm not too sure how best to adapt the existing tests. Maybe we could leave that to a separate PR and get the other improvements in?

bigspider avatar Nov 09 '22 21:11 bigspider