trezor / trezor-firmware

:lock: Trezor Firmware Monorepo
https://trezor.io
Other
1.36k stars 660 forks source link

Support `USE_{SOME COIN}` compilation flags #2375

Open krnak opened 2 years ago

krnak commented 2 years ago

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.

andrewkozlik commented 2 years ago

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 commented 2 years ago

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.

matejcik commented 2 years ago

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)

andrewkozlik commented 2 years ago

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.

matejcik commented 2 years ago

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.

andrewkozlik commented 2 years ago

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.

ryankurte commented 2 years ago

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)