search

wizardsardine / liana

The missing safety net for your coins
https://wizardsardine.com/liana
BSD 3-Clause "New" or "Revised" License
295 stars 49 forks source link

Coldcard signing device integration #580

Closed darosior closed 5 months ago

darosior commented 11 months ago

Coldcard recently announced experimental Miniscript signing support. This issue is for discussing and tracking the integration of Coldcard with Liana.

darosior commented 11 months ago

We should be able to interface with the CC using https://github.com/alfred-hodler/rust-coldcard.

scgbckbone commented 11 months ago

COLDCARD (CC) air-gaped (better than USB) integration is done:

  1. users can export xpubs via Advanced/Tools -> Export Wallet -> Generic JSON or Settings -> Multisig Wallets -> Export XPUB
  2. xpub from step 1. can be imported to liana UI when policy is created
  3. copy liana generated miniscript descriptor, paste it to file on SD card and import via Settings -> Miniscript -> Import From File (or via NFC, or VDISK)
  4. Once miniscript wallet is registered, users can sign (PSBTs)

CC USB:

darosior commented 11 months ago

Thank you for the details. At the moment our signing device UX is centered around a USB connection using async-hwi. We are interested in supporting alternative interfaces to signing devices in the future but short term we'd need a full USB integration.

what is missing is USB command that would allow us to import/register miniscript via USB (I guess you want/need this?)

Yes.

pythcoiner commented 11 months ago

sted in supporting alternative interfaces to signing devices in the future but short term we'd need a full USB integration.

if this crate used for CC integration, under Linux, udev rules specified here and here should be handle by package install if .deb package or manually (I guess) for AppImage.

darosior commented 9 months ago

We would probably need https://github.com/Coldcard/firmware/pull/244 to be included in https://github.com/alfred-hodler/rust-coldcard.

scgbckbone commented 9 months ago

FYI: that change is now in master and released on pypi

https://github.com/Coldcard/ckcc-protocol/releases/tag/v1.4.0

https://pypi.org/project/ckcc-protocol/

scgbckbone commented 8 months ago

enroll miniscript via USB in released edge version https://coldcard.com/downloads/2023-10-26T1343-v6.2.1X-mk4-coldcard.dfu

edouardparis commented 7 months ago

For information,

Firmware v6.2.1:

rust-coldcard (not blocking, we can still fork, but better to have it clean)

scgbckbone commented 7 months ago

does not permit to generate descriptor with the same xpub multiple time in it.

I thought that it is enough for you guys to allow same origin but different accounts (a.k.a origin derivation) what is the reason to have exact same key in multiple logic legs?

Miniscript enroll through usb does not attach a file name to the policy but only the checksum

there is not filename sent over USB, just a stream of data - that is why you see checksum fallback. With SD card you get filename. Yet this should be easy enough to implement that maybe I'm down...

User can only check the policy derived address through the device by looking himself in the Address explorer

unfortunately yes - this is current limitation - we need a new command...

I'll look into this and give you more info soon - thanks for integrating us!!

darosior commented 7 months ago

I thought that it is enough for you guys to allow same origin but different accounts (a.k.a origin derivation) what is the reason to have exact same key in multiple logic legs?

For instance you may want (in fact you often do) the same signer in multiple recovery paths, or both in the primary path and in a recovery path. In this case a single xpub is either queried from the signing device or fetched from a cosigner and a different unhardened derivation is used per spending path.

-------- Original Message -------- On Nov 18, 2023, 9:16 PM, scgbckbone wrote:

does not permit to generate descriptor with the same xpub multiple time in it.

I thought that it is enough for you guys to allow same origin but different accounts (a.k.a origin derivation) what is the reason to have exact same key in multiple logic legs?

Miniscript enroll through usb does not attach a file name to the policy but only the checksum

there is not filename sent over USB, just a stream of data - that is why you see checksum fallback. With SD card you get filename. Yet this should be easy enough to implement that maybe I'm down...

User can only check the policy derived address through the device by looking himself in the Address explorer

unfortunately yes - this is current limitation - we need a new command...

I'll look into this and give you more info soon - thanks for integrating us!!

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

darosior commented 7 months ago

To expand on why we use a different derivation of the xpub instead of different accounts. Using a different account requires a hardened derivation. It is unnecessary and requires a back and forth between either the signing device or the other participants for each key. It's a much better UX for the device / participants to share a single xpub to be used at different unhardened derivations across spending paths. In addition, it makes descriptor verification on the signing device screen much more bearable for the user: there is only a single xpub to verify per participant instead of N.

@scgbckbone do you think you can release an updated edge firmware which relaxes this check? I expect it would be a small change: the unique constraint should be on pairs of (xpub, der_path) instead of xpub singletons. There is no reason for checking xpub duplicates as long as they have different derivation indexes. Miniscript enforces that there is no duplicate public key so a signature for one path cannot be used for another. It doesn't apply here. EDIT: in fact you'd need to also relax this requirement:

subderivation must be <0;1>/* (may be omitted during the import - is implied)

My bad for not checking limitations / reaching out earlier.

scgbckbone commented 7 months ago

if I understand correctly: you use BIP44 change index to differentiate signers ? or you instead have /X/change/idx/ where you have three (or more) unhardened sub-derivation instead?

darosior commented 7 months ago

In order to respect wallet policies, and avoid an otherwise somewhat expensive additional derivation step, we instead increase the indexes in the multipath derivation step: /<0;1>/* for the first xpub, /<2;3>/* for the second, etc..

For instance with Alice and Bob each having an xpub in each spending path of a descriptor with 2 recovery paths:

Makes sense?

scgbckbone commented 7 months ago

yes - but seems non-standard (wrt BIP44) and/or hacky - does reference client allows these descriptors? I assume yes...

darosior commented 7 months ago

Multipath descriptors are not yet merged into Bitcoin Core, but the current implementation does support them (see https://github.com/bitcoin/bitcoin/pull/22838). This is absolutely standard (see BIP389).

Why do you bring up BIP44? It's an ancient standard irrelevant here: it doesn't consider how multiple keys from the same signer may be used in a single script. In any case it's mostly outdated by descriptors.

scgbckbone commented 7 months ago

I know about 22838, my q was about having something else in b44 change index besides <0;1> in core - but core does not care I see now.

it doesn't consider how multiple keys from the same signer may be used in a single script. In any case it's mostly outdated by descriptors.

I always thought it was the concept of account that is going to be used - hardened origin derivation - but your explanation above make sense with regards to performance and UX - seems there is also no change in security assumptions using unhardened - as it all still comes from one seed.

Is this the best way forward? One can still loose his descriptor info and be left out with seeds only - those are the moments when one appreciates standard derivation paths

BIP44? It's an ancient standard irrelevant here

Many wallets implement it, many wallets may have issues with descriptor where change is not <0;1> (us as an example).

darosior commented 7 months ago

[BIP44 is] an ancient standard irrelevant here

Many wallets implement it, many wallets may have issues with descriptor where change is not <0;1> (us as an example).

My bad, i overlooked BIP44 prescribes using 0 and 1 for external / internal key chains.

Is this the best way forward? One can still loose his descriptor info and be left out with seeds only - those are the moments when one appreciates standard derivation paths

Well if you do good luck remembering all the information needed to reconstruct the Scripts you used. I don't think relying on implicit information should ever be considered in the context of funds backup, and especially not for wallets using (somewhat) advanced scripts. Using an additional derivation step instead of increasing the indexes at the multipath step would not help with that at all. (In fact it would even make a bruteforce search less efficient heh!)

scgbckbone commented 7 months ago

one idea - but still non-standard --> move account from hardened to unhardened and preserve change.

from: m / purpose' / coin_type' / account' / change / address_index

to (non-hardened account): m / purpose' / coin_type' / account / change / address_index

still meh - maybe I even like yours better

darosior commented 7 months ago

Yeah i don't think we should do that:

scgbckbone commented 7 months ago

I'm out of better ideas here... Am I right if I look at this like: external chain is always EVEN and internal ODD?

darosior commented 7 months ago

Yes

scgbckbone commented 7 months ago

maybe worth trying to extend the BIP44>Change section with odd/even would be backwards compatible

darosior commented 7 months ago

Can't you implement compatibility with what we are doing currently (which is perfectly standard)? No other signing device which implemented miniscript descriptors support (Ledger, Bitbox, Jade, Specter) have this limitation.

scgbckbone commented 7 months ago

maybe worth trying to extend the BIP44>Change section with odd/even would be backwards compatible

was just off-topic comment - I will implement compat. Just not as optimistic as you are about the scope of this in firmware.

darosior commented 7 months ago

Yes i hadn't understood you would only provide signatures for BIP44 paths. We can't help you with this unfortunately but feel free to reach out anytime if you need feedback or testing.

darosior commented 7 months ago

Just letting you know that we are planning on feature freezing v4 in about a week. I know it's tight. We really wanted to have Coldcard support in this one, so we can maybe extend it to 2 weeks. Worst case we can always merge it in master once you are done patching the derivation indexes: we can expect people using the Edge firmware to be able to compile the master branch of Liana.

edouardparis commented 7 months ago

Miniscript enroll through usb does not attach a file name to the policy but only the checksum

there is not filename sent over USB, just a stream of data - that is why you see checksum fallback. With SD card you get filename. Yet this should be easy enough to implement that maybe I'm down...

I was thinking of a way to solve this minor issue without breaking API compatibility. I may suggest to have a new request rename_file <hash of file> <name>, that the client could use after uploading the file and before enrolling the miniscript. The hash gives an insurance that the client uploading the file must be the only one able to update its name.

scgbckbone commented 7 months ago

Miniscript enroll through usb does not attach a file name to the policy but only the checksum

there is not filename sent over USB, just a stream of data - that is why you see checksum fallback. With SD card you get filename. Yet this should be easy enough to implement that maybe I'm down...

I was thinking of a way to solve this minor issue without breaking API compatibility. I may suggest to have a new request rename_file <hash of file> <name>, that the client could use after uploading the file and before enrolling the miniscript. The hash gives an insurance that the client uploading the file must be the only one able to update its name.

Yes, we will provide new api call to rename - but as it was tagged as optional - not sure when I get to it.

scgbckbone commented 7 months ago

Just letting you know that we are planning on feature freezing v4 in about a week. I know it's tight. We really wanted to have Coldcard support in this one, so we can maybe extend it to 2 weeks. Worst case we can always merge it in master once you are done patching the derivation indexes: we can expect people using the Edge firmware to be able to compile the master branch of Liana.

Will let you know in coming days - already have a (low confidence) POC

darosior commented 7 months ago

Any chance we can start testing against the POC?

scgbckbone commented 7 months ago

.dfu sent to darosior@protonmail.com

DFU does not contain eny USB commands yet - just subderivation thing

darosior commented 7 months ago

Thanks. I'm extending the feature freeze to next Tuesday but won't got past that. Hopefully this can make it in!

edouardparis commented 7 months ago

We are making good progress !

registration

Successfully registered the descriptor:

wsh(or_d(multi(2,[c35acf90/48'/1'/0'/2']tpubDEMvpuTGLMW12uUh23draRCHUM38kxjhaWTd2wTpDREsLvU1SKEgkjD98jZpLsvEnpoZxS2VzK6FmcHvNqwG3po2oqSCTeXJ1ndUrdikNSX/<0;1>/*,[89809180/48'/1'/0'/2']tpubDEpi8rCEo1Z8ge1z2JKixw33Q15CLDyPXWZraMveGuMDVFtT5miB3zMfsPV8HLopaP2LkFAzqKynp23qJfyVRqc2Dtd8fdp5YimmpyCxMQm/<0;1>/*),and_v(v:thresh(2,pkh([c35acf90/48'/1'/0'/2']tpubDEMvpuTGLMW12uUh23draRCHUM38kxjhaWTd2wTpDREsLvU1SKEgkjD98jZpLsvEnpoZxS2VzK6FmcHvNqwG3po2oqSCTeXJ1ndUrdikNSX/<2;3>/*),a:pkh([89809180/48'/1'/0'/2']tpubDEpi8rCEo1Z8ge1z2JKixw33Q15CLDyPXWZraMveGuMDVFtT5miB3zMfsPV8HLopaP2LkFAzqKynp23qJfyVRqc2Dtd8fdp5YimmpyCxMQm/<2;3>/*)),older(2))))#jf2fuepj

Would be very nice to have a command to check if descriptor is already registered. It would be useful for client also to know if user did enroll the policy or not, because miniscript_enroll command returns early while user just has the policy displayed and did not confirm yet.

receiving

While retrieving the addresses to check if they are correctly derived from the device, I see it is missing for all of them 3 letters.

cat ~/Downloads/addresses.csv 
"Index","Payment Address","Script","Derivation","Derivation","Derivation","Derivation"
0,"tb1qha903wht___ex4zgj9fn5zjylg4wqdeq6vvy777zj0cue0963sls68mupl","83522103a3357292442f361e0ac483170145186ceebe4af949a489877a1263243ba5da5321038adc9591b3f9198fd1c9c8ad8158a360e85c46b6f3964b9c14647eb7d70c313d52ae736476a914d8450876ff15cfafc02cedc8c8954f434bf2736e88ac6b76a914e34dffc02c945592b7ee621f4719cdbfae15e6cc88ac6c93528852b268","c35acf90/48h/1h/0h/2h/0/0","89809180/48h/1h/0h/2h/0/0","c35acf90/48h/1h/0h/2h/2/0","89809180/48h/1h/0h/2h/2/0"
1,"tb1q57m06c2u___h9lttnpqmy6jdflrzasrpjvf52gdaevxygyz2n99qthy905","8352210314333db137ab6f094bda94986d593a716b36d55b4d708121b302852659c1c74d21028092db3eac8a0ba2ec3d3cb1deae78574a6ef28a77a732da8a1213f0b38f112352ae736476a9142c16bf16fb2f180db40b0113cc2f655bec86c94388ac6b76a9140d4fa616846570f302306c4034133821320d635888ac6c93528852b268","c35acf90/48h/1h/0h/2h/0/1","89809180/48h/1h/0h/2h/0/1","c35acf90/48h/1h/0h/2h/2/1","89809180/48h/1h/0h/2h/2/1"
2,"tb1q6pj23jxy___7x7ejf3k9mevc9x96xkduz58wxmdkl33les9x240qpmwv00","83522102c1a049b5d957f03377c850be6440f90ab775c8468bd7c1fcfc4f2cb96ff9b1d421031a5885d1323907e3f3b41c21112a8b53df0a3f0051d339ba278e82681d538aa652ae736476a914dd7eda655514e637cc44cd7e8920617211d6de3a88ac6b76a91462db5b0da09fda1c755e7b3a776632bad3abb93a88ac6c93528852b268","c35acf90/48h/1h/0h/2h/0/2","89809180/48h/1h/0h/2h/0/2","c35acf90/48h/1h/0h/2h/2/2","89809180/48h/1h/0h/2h/2/2"
3,"tb1qajur504h___963huv3jr0z9ara9nkm5lvjkpawkkqu0rs55p7c3qehx3en","835221033c13d8b18f500712d6124bf6c36a7372988ea03717859376cf571e0ac5d1c140210344895c960cda6fb04ea744da65eda70762ff87e423a93ec4b77371796ee3de0852ae736476a9142ff024fdcc52c5cc04e06862f6a83120963b24e788ac6b76a9143adcdaa16a20554e2875d7c0b683bbf86a240ee888ac6c93528852b268","c35acf90/48h/1h/0h/2h/0/3","89809180/48h/1h/0h/2h/0/3","c35acf90/48h/1h/0h/2h/2/3","89809180/48h/1h/0h/2h/2/3"

missing --------------^

signing

While signing:

output:

0.01000000 XTN
 - to address -
tb1qjcrjp2y3m5q3dc74d9d2jg09m228r27j8hv34t

Network fee:
0.00001065 XTN

Change back:
0.48998935 XTN
 - to address -
tb1qa3hefxln4frk6su9t72z6yqulk5utpjvc4svaktxjen5p73nddjqd9uwgt
IFUZPWjbrb9brQ0S1tcyBGyUA8dC55Mgg3/Fwq9Jj5t6RvJQi8abDWmob+KPxPCK8Zj/EPkVCWKqSWpPUFM1Tso=

I believe it is a bug, information must be displayed to user and then signature passed through usb. It is also not clear for which pubkey the signature was made.

darosior commented 7 months ago

requires MSRV bump to 1.70, it may be not a problem for us anymore (See)

I'm fine with it as long as it doesn't also bump the glibc version we're linking against.

darosior commented 6 months ago

I'm punting this to v5 as we are feature freezing now.

scgbckbone commented 6 months ago

poc0.dfu sent to darosior@protonmail.com

DFU contains:

edouardparis commented 6 months ago

Thanks for the new version of .dfu, here my feedback after trying the enroll and the signing

Coldcard Firmware 6.2.1X 2023-12-04

Registration

While trying to register a descriptor using the new .dfu

{"name":"edouard","desc":"wsh(or_d(multi(2,[c35acf90/48'/1'/0'/2']tpubDEMvpuTGLMW12uUh23draRCHUM38kxjhaWTd2wTpDREsLvU1SKEgkjD98jZpLsvEnpoZxS2VzK6FmcHvNqwG3po2oqSCTeXJ1ndUrdikNSX/<0;1>/*,[89809180/48'/1'/0'/2']tpubDEpi8rCEo1Z8ge1z2JKixw33Q15CLDyPXWZraMveGuMDVFtT5miB3zMfsPV8HLopaP2LkFAzqKynp23qJfyVRqc2Dtd8fdp5YimmpyCxMQm/<0;1>/*),and_v(v:thresh(2,pkh([c35acf90/48'/1'/0'/2']tpubDEMvpuTGLMW12uUh23draRCHUM38kxjhaWTd2wTpDREsLvU1SKEgkjD98jZpLsvEnpoZxS2VzK6FmcHvNqwG3po2oqSCTeXJ1ndUrdikNSX/<2;3>/*),a:pkh([89809180/48'/1'/0'/2']tpubDEpi8rCEo1Z8ge1z2JKixw33Q15CLDyPXWZraMveGuMDVFtT5miB3zMfsPV8HLopaP2LkFAzqKynp23qJfyVRqc2Dtd8fdp5YimmpyCxMQm/<2;3>/*)),older(2))))#jf2fuepj"}

I got an error: Decoding(Protocol(\"Confused descriptor.py:483\")) I tried then to register it through the VirtDisk by copy pasting the json in a file, upload it on device and use Settings>Miniscript>Import From File>Press (2) But then I got this error on the device screen

Wrong checksum jf2fuepj"}, expected td8w6jxg descriptor.py:483

Signing

I got the same problem that in this comment

scgbckbone commented 6 months ago

descriptor.py::483 is checksum error too... your checksum is correct - I'll check, thanks. For now if you use json import with name you do not need to send checksum anymore = workaround

I have no idea what SignMode is but if we finalize tx, finalized network serialized tx is returned - if you do not want to finalize it and instead want signed PSBT use finalize=False on ckcc.protocol.sign_transaction

VerifySigned is probably using visualize flags on ckcc.protocol.sign_transaction?

While retrieving the addresses to check if they are correctly derived from the device, I see it is missing for all of them 3 letters.

won't fix for now - use new usb cmd to get whole address

Would be very nice to have a command to check if descriptor is already registered

already provided list command

matthiasdebernardini commented 6 months ago

signing

While signing:

  • using SignMode::Finalize, the transaction is correctly displayed with the detected change, but the return value is the finalized transaction and not the signed psbt or the signatures alone, I can still try to extract the pubkey and the sig.
  • using SignMode::VerifySigned, nothing is displayed on the screen, instead the expected screen is flushed to usb with the signature:

Is this a feature request or how the api works now? for me it returns the () type regardless of signing mode.

edouardparis commented 6 months ago

From my view, it is the how the api work through https://github.com/alfred-hodler/rust-coldcard library. For my case I was missing a specific flag https://github.com/alfred-hodler/rust-coldcard/pull/9/files

edouardparis commented 6 months ago

As I am failing to registering descriptor with json format, and because get "Decoding(Protocol(\"Unknown cmd\"))" for the miniscript command msas, I am thinking that I may have received a wrong .dfu, poke @darosior

scgbckbone commented 6 months ago

poc0.dfu sent to darosior@protonmail.com

DFU contains:

* subderivation thing

* descriptor import filles content can be wrapped in json and give name `{"name": "ms0", "desc": "<descriptor>"}`

* wallet name is unique ID

* fixes bug that allowed import of duplicate miniscript wallets

* USB miniscript interface (updated ckcc [here](https://github.com/Coldcard/ckcc-protocol/pull/35)):

  * **ls**
  * **get** (descriptor + name in JSON) by wallet name
  * **del**
  * **addr** (get internal/external address by index)
  * new **enroll** from string (old from file preserved)

you need poc0.dfu (sent as second email to @darosior )

edouardparis commented 6 months ago

Apologies, I have it, I may have failed the upgrade.

Edit: Yes, it works, thanks ! Is it possible I failed to upgrade by not scrolling in the confirmation screen before clicking on the check mark button ?

scgbckbone commented 6 months ago

Is it possible I failed to upgrade by not scrolling in the confirmation screen before clicking on the check mark button ?

no

edouardparis commented 6 months ago

Thank you for your help.

So far so good, I have an implementation of the liana wallet gui with coldcard support here https://github.com/wizardsardine/liana/pull/847

One UX improvement on the firmware side would be to hold the miniscript enroll return while the user is verifying the descriptor on the screen like the sign transaction command and return an error if user refused. Using miniscript_get in a loop after miniscript_enroll is not a solution, because it cannot differentiate if the user is holding the verification or cancelled.

Once the poc.dfu is approved, I will open the PR to alfred-hodler/rust-coldcard with the new commands.

pythcoiner commented 6 months ago

does CC have a kind simulator? i would like to test #847, but don't have a CC

scgbckbone commented 6 months ago

does CC have a kind simulator? i would like to test #847, but don't have a CC

https://github.com/Coldcard/firmware/blob/master/README.md#linux

EDIT: you need to wait until I publish the new edge branch

scgbckbone commented 6 months ago

@pythcoiner you need scgbckbone:miniscript_usb from https://github.com/Coldcard/firmware/pull/310 (make sure to properly adjust ckcc-protocol submodule to scgbckbone:miniscript_usb_int from https://github.com/Coldcard/ckcc-protocol/pull/35

it is already merged in edge branch (un-released)

edouardparis commented 6 months ago

does CC have a kind simulator? i would like to test https://github.com/wizardsardine/liana/pull/847, but don't have a CC

847 use https://github.com/wizardsardine/async-hwi/pull/57 that use a special branch of https://github.com/alfred-hodler/rust-coldcard/ that supports only hid interface. I am not sure you can test it with a simulator.

pythcoiner commented 6 months ago

847 use wizardsardine/async-hwi#57 that use a special branch of https://github.com/alfred-hodler/rust-coldcard/ that supports only hid interface. I am not sure you can test it with a simulator.

Ok, I'll try to find a CC in my new place in january