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

Support USE_{SOME COIN} compilation flags #2375

Open
krnak opened this issue Jul 7, 2022 · 7 comments
Open

Support USE_{SOME COIN} compilation flags #2375

krnak opened this issue Jul 7, 2022 · 7 comments
Labels
code Code improvements

Comments

@krnak
Copy link
Contributor

krnak commented Jul 7, 2022

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 krnak added the code Code improvements label Jul 7, 2022
@krnak krnak changed the title USE_* compilation flags are broken Support USE_{SOME COIN} compilation flags Jul 8, 2022
@andrewkozlik
Copy link
Contributor

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

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

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

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

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

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

@ryankurte
Copy link

ryankurte commented Jul 12, 2022

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code improvements
Projects
Status: No status
Development

No branches or pull requests

4 participants