trezor-firmware icon indicating copy to clipboard operation
trezor-firmware copied to clipboard

Support `USE_{SOME COIN}` compilation flags

Open krnak opened this issue 3 years ago • 7 comments

Compilation flags of type USE_* are not separable, i.e. they can be all disabled (by BITCOIN_ONLY=1), or all enabled (otherwise), but they cannot be set individually.

For example for USE_ETHEREUM=0 I get this error

In file included from embed/extmod/modtrezorcrypto/modtrezorcrypto.c:40:
embed/extmod/modtrezorcrypto/modtrezorcrypto-bip32.h: In function 'mod_trezorcrypto_HDNode_ethereum_pubkeyhash':
embed/extmod/modtrezorcrypto/modtrezorcrypto-bip32.h:437:3: error: implicit declaration of function 'hdnode_get_ethereum_pubkeyhash' [-Werror=implicit-function-declaration]
  437 |   hdnode_get_ethereum_pubkeyhash(&o->hdnode, (uint8_t *)pkh.buf);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Compilation flags will play a critical role in getting new coins and features into overflowing flash memory.

To be specific I need to disable some coins to push Zcash shielded transaction into flash.

krnak avatar Jul 07 '22 20:07 krnak

The USE_ETHEREUM, USE_MONERO, USE_CARDANO, USE_NEM and USE_EOS are read only by trezor-crypto build, whereas the firmware build reads the BITCOIN_ONLY flag. Thus when one unsets the USE_ETHEREUM and BITCOIN_ONLY flags, the firmware will build with Ethereum but trezor-crypto will build without it, resulting in the error we see in the OP. It would seem to make a lot of sense to unify this in favor of the first set of flags which are additive, see https://github.com/trezor/trezor-firmware/issues/2370, and get rid of the BITCOIN_ONLY flag in code. In that case it would also make sense to add the following flags: USE_BINANCE, USE_BITCOINLIKE, USE_RIPPLE, USE_STELLAR, USE_TEZOS, USE_WEBAUTHN, USE_ZCASH.

What do you think @matejcik?

andrewkozlik avatar Jul 11 '22 08:07 andrewkozlik

Actually we should ideally be distinguishing between Zcash shielded and unshielded, because the memory overhead of shielded transactions appears to be overwhelming. So USE_BITCOINLIKE (including Zcash unshielded) vs. USE_ZCASH_SHIELDED.

andrewkozlik avatar Jul 11 '22 08:07 andrewkozlik

I don't see the need for per-coin use flags, there is no plan to mix-and-match.

We can flip "bitcoin-only" to something like "USE_ALL_FEATURES", or optionally "USE_ALTCOINS" and "USE_WEBAUTHN", and then add "USE_ZCASH_SHIELDED" (which also implies inclusion of Zcash unshielded)

matejcik avatar Jul 11 '22 09:07 matejcik

I don't see the need for per-coin use flags, there is no plan to mix-and-match.

True, what I am proposing is very fine-grained. However, considering that we are running out of flash memory, we will have to resort to creating specialized firmwares, such as the one for Zcash-shielded transactions. I imagine this would then be helpful in creating thematic firmwares, such as "privacy firmware" with BTC + Zcash (shielded) + Monero or something similar.

andrewkozlik avatar Jul 11 '22 10:07 andrewkozlik

we will have to resort to creating specialized firmwares

I am not yet convinced that "specialized firmwares" are a sensible way forward here. Some sort of pluggable architecture ala Ledger is an equally convincing alternative.

that said, I generally agree with you but I wouldn't go ahead with the fine-grained control until we actually start using it.

matejcik avatar Jul 11 '22 11:07 matejcik

A pluggable architecture would be a nice solution, but we are very far from being able to implement that, whereas specialized firmwares is something that we are already doing. Anyway, I can understand the hesitation around prematurely implementing fine-grained control, so let's deal with this as we go and focus effort on https://github.com/trezor/trezor-firmware/issues/2370.

andrewkozlik avatar Jul 11 '22 15:07 andrewkozlik

definitely +1 to being able to switch in and out features as required (even as an interim measure), the flash size constraint has been a real challenge for #2372, particularly because we do need to be able to test -each- coin (and will going forward also need to include our own)

(the version of this i started in case it is of use to y'all: per-coin-flags.txt, though there are still a lot of places to propagate these changes)

ryankurte avatar Jul 12 '22 00:07 ryankurte