xmr: monero crypto functions, tests

[prusnak]

original PR: #162

• basic crypto in ed25519 (ge, fe, sc) missing from the trezor-crypto, required for Monero.
• Monero specific functions (e.g., sub-address computation, Pedersen commitments, base58 block-based, hash_to_scalar, hash_to_point, derivation to scalar)
• serialization routines - simple variable sized integer serialization
• memory optimized range proof, Borromean.
• tests - should be covered pretty well.

[prusnak]

9c1dd96 - moved stuff not related to monero into:

• ed25519-donna/ge25519.{c,h} - ge25519 related functions
• ed25519-donna/modm-donna-32bit.{c,h} - bignum256modm related functions

[ph4r05]

I see. Just a quick question:

• The corresponding tests will be also moved from the monero test file to the main test file? Just curious.
• Is modtrezorcrypto-monero going to stay the same / provide the same API for the micropython code? It might be not worth to move these non-monero ed25519 specific functions to other modules as none of existing provide such low level API.

Notes to myself:

• Adapt py-trezor-crypto

[prusnak]

The corresponding tests will be also moved from the monero test file to the main test file? Just curious.

Unsure for now, I might move them to test-ge25519 and test-modm

Is modtrezorcrypto-monero going to stay the same

I don't plan to change this

[onvej-sl]

I suggest

• moving bignum25519 related functions (curve25519_*) from ge25519.c to curve25519-donna-32bit.c and
• moving ge25519 related functions (ge25519_*) from ge25519.c to ed25519-donn-impl-base.c.

What do you think, @prusnak?

There are a lot of unused functions in ge25519.c: curve25519_set_d, curve25519_set_2d, curve25519_set_sqrtneg1, ge25519_set_base, ge25519_neg_partial, ge25519_neg_full, ge25519_reduce. Are you going to use them in future, @ph4r05?

[onvej-sl]

The following issue is related to the trezor-core monero pull request. Perhaps, it'll be a subject of a long-term plan with trezor-crypto.

I don't like the interface between trezor-crypto and trezor-core. It doesn't properly encapsulate cryptographic primitives. For example:

• trezor-crypto contains functions unrelated to cryptography (base58.c).
• Some cryptography is implemented in trezor-crypto (xmr_gen_range_sig), some in trezor-core (gen_mlsag_ext, generate_signature).
• As a consequence of the previous point, trezor-core imports functions which, in my opinion, shouldn't leave trezor-crypto (ge25519_add).

[ph4r05]

Some functions are already used internally in the trezor-crypto, e.g., curve25519_neg is already used.

The functions you list are not used at the moment in trezor-core, may be later used when implementing multisigs / bulletproofs.

[ph4r05]

@onvej-sl I don't share the opinion that having range sig being in trezor-crypto and MLSAG in trezor-core is some kind of a problem. Here is my reasoning:

Range signatures xmr_gen_range_sig are CPU intensive and memory intensive operations which were originally implemented in python (trezor-core) but it was not feasible to run on the Trezor device due to a small amount of RAM and long computation times. I had to optimize the algorithm and port it to C so it is feasible to run it on the real hardware and run it fast.

Range signature is a well-contained problem with no allocations needed, simple API. For memory and timing reasons its implemented directly in trezor-crypto (as it brings real benefit to the user).

On the other hand, MLASG and other ring signatures are built from building blocks in python for easier development, code readability, maintenance and debugging. Porting to C is not that straightforward and I really don't see any benefit here. The memory and CPU is not the problem as in the case of range signatures so I think it is fine to have it in Python. Porting to C would also increase complexity of trezor-crypto and could lead to bugs.

Using small and easily auditable & testable building blocks, such as ge25519_add (fast, in C) to build more complex schemes in high level language is, in my opinion, a scalable and secure way to build the system. I am not really into porting all Monero crypto schemes to C as it would be very time consuming and prone to errors. With this reasoning I could port whole Monero implementation to C directly which is not an option for me :D

Having access to low-level features also speeds up development of new features, such as multisigs / bulletproofs.

And frankly I don't see why trezor-core should not have access to crypto primitives such as ge25519_add

[ph4r05]

After some time, when Monero code in trezor-core stabilizes, proves over time and new features are added the trezor-crypto <-> trezor-core probably changes but I don't think this is the first phase stuff.

MLSAG may need to be slightly changed when implementing multisigs (I made some preparations already but we will see after this phase starts) so I think it is definitelly better to have it in Python for now.

[ph4r05]

I was also looking into Bulletproofs for a while and due to large memory allocations I will implement it in Python, as a prototype. As I will need to dynamically allocate large key vectors and gc.collect() them after they are not needed anymore. And for Bulletproofs I need low level crypto API such as mulsub256_modm. Bulletproofs are rather complicated and I don't think it is doable directly in C on Trezor without prototyping and testing it in Python first.

[onvej-sl]

And frankly I don't see why trezor-core should not have access to crypto primitives such as ge25519_add

Because it's difficult to develop and maintain such a low-level code. It requires trezor-core developers to understand how ed25519-donna works. Which isn't easy since it uses several coordinates formates, reduced and fully reduced form of numbers, consttime and vartime functions.

I see you prefer trezor-crypto consists only of those functions which are CPU or memory intensive and/or easy to implement in C, which I partially agree with. But I'd rather trezor-crypto be a cryptographic library with a well-defined interface.

[ph4r05]

I see. This is one of the reasons why the API I export from the trezor-crypto abstracts from the point coordinate representation. In the API there is only one form of the point representation - Extended Edwards (x, y, z, t). In the trezor-core it is an opaque python object so python-core developer does not need to know these technicalities.

API: embed/extmod/modtrezorcrypto/modtrezorcrypto-monero.h

Eg. ge25519_add takes the point objects (there is only one type of point object) and returns a result of the operation, which is again the same point type, in the Extended Edwards coordinates (which is irrelevant for the trezor-core), fully normalized (z=1).

The work with scalars is even simpler as those are just simple numbers mod curve order.

In the API design, I tried to avoid pitfalls like partially reduced points and different point formats.

I also assumed that if somebody is going to change MLSAG code (which uses ge25519 primitives) has some minimal background on elliptic curves.

[tsusanka]

Just a side note: I think we're touching an issue of trezor-crypto becoming a library having two goals. First it is a cryptographic library, but second it is also becoming a place of all performance sensitive code.

It's something we were discussing internally briefly and I think it is on the long term roadmap. The ideal state IMHO is to have a cryptographic library (trezor-crypto) and then a simple way how to incorporate performance sensitive C code into trezor-core. Each trezor-core app would have some C code directly in the trezor-core app folder, which would also significantly increase its readability.

That being said, I think it's beyond the scope of this issue and is something we should tackle later on.

[onvej-sl]

In the API there is only one form of the point representation - Extended Edwards (x, y, z, t)

What about point_norm which calls ge25519_norm? There is a normalised (with z=1) and general form of a point.

[ph4r05]

Point-related functions, (such as point addition, scalar multiplication) in trezor-core should return already normalized points (z=1).

If the norming is to be left out, the operations could not be chained arbitrarily as the result is invalid.

UPDATE: point_norm is not used in the python code and can be removed - leftover from the initial development. Normalization is mainly needed after ge25519_scalarmult, ge25519_scalarmult_base_niels, which is already done in trezor-crypto - in my Monero code.

[ph4r05]

Note: Point normalization operation is typically performed when compressing coordinate point representation to the 32 B array as z needs to be 1. It requires to compute inversion which is not for free.

On the other hand, the original Monero C++ code typically operates on 32 B byte arrays (points, scalars) by decompressing and compressing it after each result so they are doing normalization in each step, basically.

There are some optimized chunks in the Monero C++ code, e.g., range sig verification, which improves blockchain scanning (still takes 3 days to verify the blockchain). Optimized chunks are using different point representations to avoid redundant normalizations but in general cases, it is not a performance issue for the sake of correct computation, easy development and maintenance.

[onvej-sl]

Point-related functions, (such as point addition, scalar multiplication) in trezor-core should return already normalized points (z=1).

I don't think they do. Try:

START_TEST(test_norm_ge25519_add)
{
	static const struct {
		char *pt;
	} tests[] = {
		{
			"b7a8b2f3dbfd41b38d20aec733a316dbfc2633503799cd36f38570cafc8ea887"
		}
	};

	ge25519 pt1, pt2;
	bignum25519 z, o;
	for (size_t i = 0; i < (sizeof(tests) / sizeof(*tests)); i++) {
	      	ck_assert_int_eq(ge25519_unpack_vartime(&pt1, fromhex(tests[i].pt)), 1);
	      	ck_assert_int_eq(ge25519_check(&pt1), 1);

	      	curve25519_set(z, 1);
	      	curve25519_sub_reduce(z, pt1.z, z);
	      	ck_assert_int_eq(curve25519_isnonzero(z), 0);

	      	ge25519_add(&pt2, &pt1, &pt1, 0);

	      	curve25519_set(o, 1);
	      	curve25519_sub_reduce(z, pt2.z, o);
	      	ck_assert_int_eq(curve25519_isnonzero(z), 0); //fail
	}

}
END_TEST

[ph4r05]

Ah I see.
The thing is normalization is needed after multiplication (I think, it been a while I implemented this part).

Normalization is performed there explicitly:

void ge25519_scalarmult_base_wrapper(ge25519 *r, const bignum256modm s){
	ge25519_scalarmult_base_niels(r, ge25519_niels_base_multiples, s);
	ge25519_norm(r, r);
}

void ge25519_scalarmult_wrapper(ge25519 *r, const ge25519 *P, const bignum256modm a){
	ge25519_scalarmult(r, P, a);
	ge25519_norm(r, r);
}

ge25519_add trully can return non-normalized point. In spite of that, all the Monero crypto code works properly without need to bother with the normalization issue - python Monero code does not call normalization at all.

So the question is:

• Is there a python way (using extmod-monero) to get an invalid computation because of addition not normalizing points all the time? There is obviously none in the current Monero code so non-norming in this particular case does not cause any trouble to the trezor-core user.

• If such PoC exists in the python I can easily add additional norming step in the trezor-core etmod-monero module so python layer gets normed point always. Will that fix it for you?

[onvej-sl]

Is there a python way (using extmod-monero) to get invalid computation because of addition not normalizing points all the time?

Honestly, I don't know what (or when) operations do require normalised points. I'll try to figure it out.

[ph4r05]

Hm.. It seems we could get rid of the point normalization in general. I think there is just a problem with ge25519_eq which does not work well with non-normed points.

[ph4r05]

So, Extended Edwards coordinates (X, Y, Z, T) where it holds:

x=X/Z
y=Y/Z
x*y=T/Z

Thus lets have:

P1=(X1, Y1, Z1, T1)
P2=(X2, Y2, Z2, T2)

x1 = X1/Z1, y1 = Y1/Z1
x2 = X2/Z2, y2 = Y2/Z2

To check if P1==P2 we check x1==x2 && y1==y2, right?

That would be:

X1/Z1 - X2/Z2 == 0 && Y1/Z1 - Y2/Z2 == 0

Which should be equal as: (pls check)

X1*Z2 - X2*Z1 == 0 && Y1*Z2 - Y2*Z1 == 0

Is that correct?
UPDATE: I will get to that later today. The normalization wont be needed after all.

[ph4r05]

OK found it.

The first thing: ge25519_eq is correct. The problem is it performed also point-on-curve ge25519_check check (which was extraneous) and the check does not work with non-normalized points (@onvej-sl pls check). I removed this check from ge25519_eq so normalization is not required. Point check is meaningful operation after decompressing the point - the point is normalized after decompression - so the check works.

Another thing: The problem is that ge25519_scalarmult is not returning completely valid Extended Edwards point. The last line manipulating the result before returning:

ge25519_p1p1_to_partial(r, &t);

I've fixed so it returns always fully valid extended Edwards point. Because of this no normalization is required. This saves also performance as we avoid modular inversion.

@prusnak @onvej-sl pls consider cherry-picking 3f4cfec commit which contains changes described in this comment, also the a6d34c9.

I did also a separate PR fixing mentioned issues of ge25519_scalarmult in https://github.com/trezor/trezor-crypto/pull/171/files

[onvej-sl]

X1Z2 - X2Z1 == 0 && Y1Z2 - Y2Z1 == 0
Is that correct?

You also need to check Z1 and Z2 are non-zero, which wasn't necessary before since the ge25519_check.

the check does not work with non-normalized points

It does work since the equation you are using (y * y - x * x - z * z - ed25519.d * t * t) is the equation of the Edwards curve in projective coordinates. I also did some testes to be absolutely sure.

I've fixed so it returns always fully valid extended Edwards point. Because of this no normalization is required.

Could you explain it to me? I can't see a connection between normalisation and extended coordinates.

[ph4r05]

Aah OK then. Thanks! So I will add a check for the z coordinate. I will double check why I assumed the ge25519_check is not working but I think it was because of invalid T coordinate from scalarmult. Normalization was just a workaround to get full and valid extended edwards point. As scalarmult computed only with _partial the T coordinate was invalid. Normalization recomputes T coordinate from X, Y, Z which are OK for partially computed point so normalization fixed it. But it is not needed if scalarmult returns proper full point - just one more multiplication is needed.

[ph4r05]

Just to sum-up discussion with @onvej-sl:

• We agreed that ge25519_eq don't need to check for Z==0 as the valid point cannot have Z==0. The points can be created only by unpack which does not allow invalid points or by operations which return valid points. We can either note the fact both points for ge25519_eq need to be valid in the comments or return ge25519_check to the ge25519_eq to make it more robust, but quite redundant. Up to discussion.

• ge25519_check works for non-normalized points. There was a misunderstanding of the tests failing and wrong assumptions (scalarmult returns partial points only, not full). I've created new PRs ed25519: scalarmult fix so dst can be src #171 ed25519: double scalarmult fix - return full point #172. Whether to use the change I proposed or different possible alternatives is up to discussion.

Correct?

[onvej-sl]

Correct?

Absolutely.

And we decided ge25519_eq to be more robust and to check point validity.

[onvej-sl]

ACK

Nevertheless, I created this issue, since I'm not one hundred percent sure that there isn't any unexpected edge case behaviour.

Please consider merging this PR before.

[prusnak]

Thanks!