Onvej sl/legacy zkp

[onvej-sl]

Implements #1874.

Regarding 05351ea: The size of the context depends on architecture, it's 184 for 32-bit architecture and 208 for 64-bit architecture. SECP256K1_CONTEXT_SIZE must be at least the size (but doesn't have to be equal). In 05351ea I set SECP256K1_CONTEXT_SIZE to 184 for device build and to 208 for emulator build. There is no problem to have SECP256K1_CONTEXT_SIZE same for all builds if you think it's better.

@hiviah Please review f6268db and the changes in legacy/firmware/trezor.c
@andrewkozlik Please review the rest.

[andrewkozlik]

LGTM. Haven't given much attention to the build script, but seems OK too.

[invd]

@onvej-sl : I've noticed a side problem in the current Makefile that is touched by 0b29dbc :

In file included from ../vendor/secp256k1-zkp/src/secp256k1.c:12:
In file included from ../vendor/secp256k1-zkp/src/assumptions.h:12:
../vendor/secp256k1-zkp/src/util.h:75:12: fatal error: 'valgrind/memcheck.h' file not found
#  include <valgrind/memcheck.h>
           ^~~~~~~~~~~~~~~~~~~~~
1 error generated.

From what I can see, the default Makefile settings assume that Valgrind libraries are installed on the system, and the above mentioned error turns up if they're not. VALGRIND is set to 1 by default, which makes the crypto test code #include it, so this error is triggered there as well.

trezor-firmware/crypto/Makefile

Lines 53 to 57 in fd0d1ed

	VALGRIND ?= 1
	CFLAGS += -I.
	CFLAGS += -I..
	CFLAGS += -DVALGRIND=$(VALGRIND)

Up until the secp256k1-zkp addition, the "normal" build did not require Valgrind libraries outside of the tests.

This has now changed when delivering the $(CFLAGS) to secp256k1-zkp since it also reacts to the VALGRIND define:
https://github.com/ElementsProject/secp256k1-zkp/blob/6db00f5b2e0986add852fa808ab485a25a9278f7/src/util.h#L74

Note that secp256k1-zkp includes the libraries on #if defined(VALGRIND) whereas the Trezor code uses #if VALGRIND, so the behavior for VALGRIND=0 or an undefined VALGRIND is conflicting between them.

I think it is reasonable to require the Valgrind libraries for the regular builds but recommend

1. documenting the dependency
2. evaluating if the VALGRIND logic can be improved

[prusnak]

I am OK with changing our logic from #if VALGRIND to #ifdef VALGRIND to make things simpler.

[hiviah]

Changes in firmware/trezor.c LGTM.

[onvej-sl]

@hiviah Did you review f6268db as well?

[onvej-sl]

Up until the secp256k1-zkp addition, the "normal" build did not require Valgrind libraries outside of the tests.

I'm not sure, what you mean by "normal" build. Unless I mess something up, this build of secp256k1-zkp shouldn't be used anywhere other than in tests.

[invd]

Up until the secp256k1-zkp addition, the "normal" build did not require Valgrind libraries outside of the tests.

I'm not sure, what you mean by "normal" build. Unless I mess something up, this build of secp256k1-zkp shouldn't be used anywhere other than in tests.

You're right, I was thinking of other legacy Makefile usage that is unrelated. Other parties may be using the crypto code for more than testing (it was formerly distributed as trezor-crypto for that purpose, after all) but I understand that this is not really supported. We previously discussed this area in the internal ticket no 86, but I think that is not as relevant to this PR.

[hiviah]

@hiviah Did you review f6268db as well?

Yes, seems fine.

[onvej-sl]

You're right, I was thinking of other legacy Makefile usage that is unrelated. Other parties may be using the crypto code for more than testing (it was formerly distributed as trezor-crypto for that purpose, after all) but I understand that this is not really supported.

I understand. Are you OK with 562164a? If someone wants to compile the code without valgrind, they must use VALGRIND=0, which I think is intuitive. However, valgrind is on by default, since the main use case is testing.

[invd]

I understand. Are you OK with 562164a? If someone wants to compile the code without valgrind, they must use VALGRIND=0, which I think is intuitive. However, valgrind is on by default, since the main use case is testing.

I think the ifdef valgrind doesn't work as intended since

• the variable use is case-sensitive
• for ifdef VALGRIND in combination with VALGRIND ?= 1, VALGRIND=0 doesn't skip the ifdef. VALGRIND= works but is less intuitive, I think.

ifeq ($(VALGRIND),1) looks like a more direct way to test against the 1 default value.

[onvej-sl]

@invd Is that OK? dc78566

[invd]

@invd Is that OK? dc78566

Yes, I think that works. Thanks for the followup!