Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coldcard signing device integration #580

Closed
darosior opened this issue Jul 21, 2023 · 73 comments
Closed

Coldcard signing device integration #580

darosior opened this issue Jul 21, 2023 · 73 comments
Assignees
Labels
Feature New feature or functionality. GUI gui related RFC Needs conceptual discussion and/or feedback from users Signing devices Anything related to signing devices ("hardware wallets")

Comments

@darosior
Copy link
Contributor

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

@darosior darosior added RFC Needs conceptual discussion and/or feedback from users GUI gui related Feature New feature or functionality. Signing devices Anything related to signing devices ("hardware wallets") labels Jul 21, 2023
@darosior
Copy link
Contributor Author

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

@scgbckbone
Copy link

scgbckbone commented Jul 25, 2023

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:

  • both ckcc or HWI can be used (tested with newest HWI 2.3.0)
  • you can get xpub and sign tx via USB
  • what is missing is USB command that would allow us to import/register miniscript via USB (I guess you want/need this?)

@darosior
Copy link
Contributor Author

darosior commented Jul 25, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

darosior commented Sep 11, 2023

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

@scgbckbone
Copy link

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
Copy link

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

@edouardparis
Copy link
Member

For information,

Firmware v6.2.1:

  • does not permit to generate descriptor with the same xpub multiple time in it. (blocking)
  • Miniscript enroll through usb does not attach a file name to the policy but only the checksum. (not blocking)
  • User can only check the policy derived address through the device by looking himself in the Address explorer (not blocking)

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

  • requires MSRV bump to 1.70, it may be not a problem for us anymore (See)
  • I did a PR to add miniscript enroll

@scgbckbone
Copy link

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
Copy link
Contributor Author

darosior commented Nov 18, 2023 via email

@darosior
Copy link
Contributor Author

darosior commented Nov 19, 2023

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
Copy link

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
Copy link
Contributor Author

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:

  • Keys for the non-timelocked spending path are xpubBob/<0;1>/* and xpubAlice/<0;1>/*
  • Keys for first timelocked spending path are xpubBob/<2;3>/* and xpubAlice/<2;3>/*
  • Keys for second timelocked spending path are xpubBob/<4;5>/* and xpubAlice/<4;5>/*

Makes sense?

@scgbckbone
Copy link

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

@darosior
Copy link
Contributor Author

darosior commented Nov 20, 2023

Multipath descriptors are not yet merged into Bitcoin Core, but the current implementation does support them (see bitcoin/bitcoin#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
Copy link

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
Copy link
Contributor Author

[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
Copy link

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
Copy link
Contributor Author

Yeah i don't think we should do that:

  • It wouldn't make it compatible with BIP44
  • Users may still want to use different accounts, that's separate from using more than 1 key in a single script
  • It would be a breaking change so i would rather only do that if there is a significant upside

@scgbckbone
Copy link

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
Copy link
Contributor Author

Yes

@scgbckbone
Copy link

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

@darosior
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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.

@edouardparis
Copy link
Member

Thank you for your help.

So far so good, I have an implementation of the liana wallet gui with coldcard support here #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
Copy link
Collaborator

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

@scgbckbone
Copy link

scgbckbone commented Dec 21, 2023

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
Copy link

scgbckbone commented Dec 21, 2023

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

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

@edouardparis
Copy link
Member

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

#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.

@pythcoiner
Copy link
Collaborator

#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

@matthiasdebernardini
Copy link

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

what do you mean from your view? I am using the API right now, it doesn't work how you are describing it (though it would be nice) you have to send it a psbt to sign, if that goes well, then you can ask it for the finalized transaction.

I am not sure which feature flag you are referring to which allows an API to sign and return the transaction in one go, if its there please let me know since I could trim away some code. Thank you.

@pythcoiner
Copy link
Collaborator

@scgbckbone, does both mk3/mk4 support miniscript or only mk4? what about limitations?

@scgbckbone
Copy link

@scgbckbone, does both mk3/mk4 support miniscript or only mk4? what about limitations?

only Mk4 and only in edge firmware release

miniscript limitations: https://github.com/Coldcard/firmware/blob/edge/docs/miniscript.md#limitations
taproot and minitapscript limitations https://github.com/Coldcard/firmware/blob/edge/docs/taproot.md

@scgbckbone
Copy link

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

what do you mean from your view? I am using the API right now, it doesn't work how you are describing it (though it would be nice) you have to send it a psbt to sign, if that goes well, then you can ask it for the finalized transaction.

I am not sure which feature flag you are referring to which allows an API to sign and return the transaction in one go, if its there please let me know since I could trim away some code. Thank you.

this is misunderstanding and you're right you need 2 steps - what I was describing was cli signing workflow https://github.com/Coldcard/ckcc-protocol/blob/11c711e929a090ec29ccd2a05d094aa3d2cbc113/ckcc/cli.py#L481-L548

@pythcoiner
Copy link
Collaborator

rstanding and you're right you need 2 steps - what I was describing was cli signing workflow

is mk3 on roadmap or not planned to support miniscript on it?

@darosior
Copy link
Contributor Author

darosior commented Jan 5, 2024

@edouardparis mentioned to me this is close to being done. #847 should be testable by anyone with a coldcard soon!

@pythcoiner
Copy link
Collaborator

@scgbckbone currently doing a round of test on #847, while verifying address on CC, if i want to Press (4) to view QR Code on the coldcard, it crash:

image

I had to restart the CC to go back normal

@scgbckbone
Copy link

great find @pythcoiner! thank you. Patch here Coldcard/firmware#317

@pythcoiner
Copy link
Collaborator

@scgbckbone is it normal that CC follow the normal miniscript flow even if the wallet 'allias' used for registration and the one use on psbt sign does not match, by example using async-hwi cli tool:

  • I register descriptor:

    hwi wallet register --name "Liana" --policy <descriptor>
    
  • Then i ask to sign w/ a different wallet name:

    hwi psbt sign --wallet-name "Liana2" --wallet-policy <descriptor> --psbt <psbt>
    

and CC let me sign, is it about CC just check if the descriptor is registered but not if wallet_name/aliases match?

@scgbckbone
Copy link

There is currently no way to specify target wallet name for signing on CC back-end. It is always chosen automatically trying to match PSBT to some registered wallet.

Looking at async-hwi CC PR it seems to me your name flag is just ignored. Coldcard.sign_tx only takes PSBT as argument.

There is currently no way to specify target wallet name for signing on CC back-end

I'm planning to add this in the next iteration. Can be really useful if you have two policies where only order of keys is different (keys are the same just policy is different). But we do not have (yet) policy or descriptor in PSBT - meaning that our tx to registered wallet matching can fail in those situation.

@pythcoiner
Copy link
Collaborator

ok, so it means, we (now) just send psbt and CC just iterate trough the 'wallet' list to check if one matching?

@scgbckbone
Copy link

yes

@pythcoiner
Copy link
Collaborator

@scgbckbone, does the CC HSM mode functionning w/ miniscript wallets?

@scgbckbone
Copy link

@scgbckbone, does the CC HSM mode functionning w/ miniscript wallets?

yes

@edouardparis
Copy link
Member

#847 is merged, it relies on https://github.com/wizardsardine/async-hwi/ master which relies on this current branch of rust-coldcard alfred-hodler/rust-coldcard#11

We will update later with proper crates versions when crates are published.

Thank you all for contributing !

@kloaec
Copy link
Collaborator

kloaec commented Jan 16, 2024

Congratulations everyone :)
@scgbckbone do you have an ETA for publishing the corresponding firmware publicly? Happy to announce it with you in any way or form.

@scgbckbone
Copy link

thanks guys!

We're "almost ready", just need to do the pre-release dance, I hope it'll be out this week... Keep you posted

@scgbckbone
Copy link

@scgbckbone
Copy link

I tested liana master - no issues found. UX is very smooth, like it a lot

@kloaec
Copy link
Collaborator

kloaec commented Feb 26, 2024

Hello @scgbckbone and @edouardparis , finally received a Coldcard Q, i upgraded to 0.0.6Q.
After adding the udev, i can get the xpub no problem, but i cannot register the descriptor. It briefly shows "receiving" or something on the screen, but then goes back to the menu and i get an error on Liana. "Decoding(Protocol("Unknown cmd")).

Might be that the Q doesn't have Miniscript support yet? Or do we need to change something in our async-hwi?

@darosior
Copy link
Contributor Author

i upgraded to 0.0.6Q.

Is this the Edge firmware?

@kloaec
Copy link
Collaborator

kloaec commented Feb 26, 2024

All versions are Beta for Q, no Edge releases.

@scgbckbone
Copy link

We currently do not have edge Q - so no miniscript/tr support in there - soon TM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or functionality. GUI gui related RFC Needs conceptual discussion and/or feedback from users Signing devices Anything related to signing devices ("hardware wallets")
Projects
No open projects
Status: Done
Development

No branches or pull requests

6 participants