Improve support for the coming Ledger app 2.1.0

[bigspider]

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]

@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]

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.

[achow101]

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.

[bigspider]

@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?

[achow101]

please rebase

[bigspider]

@achow101 rebased Dropped the commit updating speculos automation file since you already did that on master.

[bigspider]

Added a commit to address the type check issues. Not sure about the timeout, perhaps it just needs a bit more time?

On my machine, python run_tests.py --ledger takes 475 s and all tests pass.

[achow101]

Not sure about the timeout, perhaps it just needs a bit more time?

No, it's just timing out after an hour. This is a very frequent issue with our CI that I have not been able to figure out.

[achow101]

Can you please squash the type check fixes into the appropriate commits? I believe they all belong in a93dc15be313fcf398b0fd9c2b44cd5d02816155 "Add implementation of display_multisig_address to ledger.py".

I'm a little concerned about downgrading Ledger 2.0.x users to the legacy protocol. This might be completely unexpected for them. Is there a reason to do that?

[bigspider]

Can you please squash the type check fixes into the appropriate commits? I believe they all belong in a93dc15 "Add implementation of display_multisig_address to ledger.py".

Done, and also squashed two consecutive commits tampering with test_ledger.py into fa3c0599869d651ab8af1becdfbd83d9ca83259c.

I'm a little concerned about downgrading Ledger 2.0.x users to the legacy protocol. This might be completely unexpected for them. Is there a reason to do that?

It's not ideal, but I think supporting the 2.0.x version brings too much complexity for too little benefit; it's hard to be sure things are right without running the test suite specifically for the 2.0.x versions; moreover different versions of the 2.0.x have different limitations, while they are all solved in 2.1.0 (plus new things like OP_RETURN and sighash flags).

There is virtually no difference for single-sig users, and for multisig users the downgrade is from a slightly suboptimal experience to a slightly more suboptimal experience...

I expect the protocol to be a lot more stable after 2.1.0, so future changes can be soft-forked in more easily and this can be hopefully a one-time event.

Removing the legacy code also reduces the attack surface of the app, so that's one more reason to incentivize users away from the 2.0.x series and considering it deprecated.

Btw: the 2.1.0 should be released in around 3 weeks − tentative date 7th December.

[achow101]

ACK fa3c0599869d651ab8af1becdfbd83d9ca83259c