Tapsigner

[scgbckbone]

• add Tapsigner to HWI
• add Tapsigner simulator
• testing
• adjust README.md in tests

[prusnak]

Please rebase on top of current master to remove the merge commit.

[scgbckbone]

Please rebase on top of current master to remove the merge commit.

will do squash and rebase after this is reviewed and ready to go - I assume this will need some more commits (build is failing + implementation of review comments)

[scgbckbone]

Few notes here. We use password field for CVC, which is something like pin code but you have to provide it for every auth command. Therefore it is not suitable for us to use PIN (with sendpin/promptpin) as PIN does not unlock the card for some amount of time as with Coldcard or Trezor, instead we would need to use sendpin before every command which is not very practical to do.

• added togglepassphrase command which is interactive way how to change CVC. (keep in mind passphrase is CVC for Tapsigner and there is no way how to add bip39 passphrase to the card so I think it is ok)
• added restore command which will not restore the card as it is not possible, but is the way how to decrypt backup file with card a dump xprv to console (unsafe but I have a warning there)
• I have tested this with core v23.0 with all supported script types, you can check here and here (signer is tapsigner simulator)
• with core you need to specify signer with CVC(password) on command line like this -signer='./hwi.py -p 123456' which definitely has some security implications for the user (but you still need to tap with the physical card).
• I have squashed and rebased on master as @prusnak proposed

Any review is greatly appreciated.

[achow101]

Please rebase now that CI is fixed.

[scgbckbone]

rebased on current master

[scgbckbone]

https://github.com/coinkite/coinkite-tap-proto/pull/24 merged

[scgbckbone]

updated poetry.lock with poetry update && poetry lock after addition of coinkite-tap-protocol --> without this update installing via poetry would produce solver error

[achow101]

Also, rebase for CI to be not as dead.

[scgbckbone]

@achow101 rebased and fixed - will squash those commits after - I think it is easier for you to review changes unsquashed

[scgbckbone]

coldcard-multisig.patch was outdated - fixed in https://github.com/bitcoin-core/HWI/pull/598/commits/38be6be5a9151d940badb20b90a60cc677c428f2

[scgbckbone]

not sure about CI --> seems to be full of random failures and timeouts

[achow101]

Please squash/adjust so that each commit passes. Refactors should be in their own commit at the beginning.

[achow101]

I'm a bit concerned about the extra dependencies that this introduces. Up to know, HWI has only interacted with devices over USB, and the USB dependency stack is relatively well understood and under control. However this now introduces a completely new dependency stack to deal with NFC readers. Does this require us to ship new driver dependencies such as libusb? I'm highly hesitant to introduce a new device that is the only one that is going to use new driver dependencies.

[scgbckbone]

52aa685f95b01a9480c0fb07acb7e433dc98ef2a Moved Tapsigner and its dependencies to extras. This aims to partly alleviate your concerns with regards to sdist and wheel distributions. In sdist and wheel, only references to dependencies are stored. With normal pip install ... it won't install Tapsigner and its dependencies - so user has to specify .[tapsigner] or -E tpsigner to actually install/use it.

In this revision, however I still kept Tapsigner in binary distributions as I was not sure how "bit concerned" you are. I can remove Tapsigner from build_{bin,wine}.sh meaning that it would only ship by wheel and sdist not binaries. Or make separate binaries with tapsigner support? not sure what is your opinion on this @achow101 ?

[scgbckbone]

52aa685f95b01a9480c0fb07acb7e433dc98ef2a Moved Tapsigner and its dependencies to extras. This aims to partly alleviate your concerns with regards to sdist and wheel distributions. In sdist and wheel, only references to dependencies are stored. With normal pip install ... it won't install Tapsigner and its dependencies - so user has to specify .[tapsigner] or -E tpsigner to actually install/use it.

In this revision, however I still kept Tapsigner in binary distributions as I was not sure how "bit concerned" you are. I can remove Tapsigner from build_{bin,wine}.sh meaning that it would only ship by wheel and sdist not binaries. Or make separate binaries with tapsigner support? not sure what is your opinion on this @achow101 ?

@prusnak @Sjors any opinions on this ?

[Sjors]

I don't understand Python dependencies that well. But if a lot a extra stuff is needed for NFC, can we (initially) have the user install those seperately and not fail if they're missing?

[scgbckbone]

I don't understand Python dependencies that well. But if a lot a extra stuff is needed for NFC, can we (initially) have the user install those separately and not fail if they're missing?

This is already done with regards to python dependencies (tapsigner dependencies are part of extras). Namely coinkite-tap-protocol and pyscard. Not installed by default.

My question is whether you would accept additional dependencies (swig libpcsclite-dev) in binaries. Currently these two additional dependencies are needed for tapsigner to work. Other option is to not provide tapsigner support in binaries at all. User would need to install swig and libpcsclite-dev on their own and can only use sdist and wheel distributions with tapsigner explicitly specified as extra during installation (see above)

[Sjors]

I hadn't thought about binaries. At some point I think hardware wallet companies should make their own binaries that are compatible with the HWI interface. However, that's not very practical right now, as e.g. Bitcoin Core won't let you configure a different -externalsigner per wallet.

User would need to install swig and libpcsclite-dev on their own

This might be the best option for now. Using NFC in a desktop environment, which afaik is where HWI is mostly used, is still a bit niche (though I plan to try it).

We could also ship separate binaries that include NFC support, if those are not too painful to generate automatically.

Another option might be to include the dependenceis, but leave the functionality disabled until the user calls hwi --enable-nfc, but that's not really easier for the end user than the above options.

[scgbckbone]

Fair points. I will remove NFC dependencies from binaries and therefore tapsigner support will be purely optional and only available via sdist and wheel. Need to make tests work somehow. Thanks for your input @Sjors

[scgbckbone]

@Sjors I use HID Omnikey 5022 CL (if you want to play with coinkite NFC cards on desktop)

[Sjors]

I ordered the ACR122U because a guy on Reddit said so.

[scgbckbone]

his reditt post is for libnfc which isn't what is used to talk to the reader (sclite) ... so doesn't apply

Seems like the one you choose coinkite lists as "not recommended" here https://github.com/coinkite/coinkite-tap-proto/blob/master/README.md#requirements

Check coinkite store https://store.coinkite.com/store/category/accessories correct readers are available and there is also 25% discount today

[Sjors]

Oh man, why are there multiple NFC libraries for Linux?

[prusnak]

Check coinkite store

"IMPORTANT: Incompatible with Mk4 NFC features! Works only with NFC cards."

Wow, so Coldcard and Tapsigner use incompatible NFC standards?

[scgbckbone]

Tapsigner is now completely optional and not included in binary distributions. Tests are handled to not fail if extras are not installed.