Increase precision of g1 and g2.

[roconnor-blockstream]

This allows us to shift by 256+128 = 384 bits, which is a multiple of the limb size of the scalar representation.
This also happens to be the most precision possible for g2 that still fits into a 256-bit value.

[roconnor-blockstream]

Historically speaking the values of g1 and g2 were set in #127 where they were copied from #21. However #21 was using secp256k1_num_mul where there was some advantage to using small values for g1 and g2. But with secp256k1_scalar_mul_shift_var, there is no advantage to using small values for g1 and g2.

Instead there is a very slight advantage of shifting by a multiple of the limb size.

[roconnor-blockstream]

I've updated the proof. Please note that epsilon1 and epsilon2 are now defined to be 2^256 times larger than they were before. But don't worry, they are still pretty small values.

[roconnor-blockstream]

I've added a secp256k1_split_lambda_verify function to verify the results against the bounds computed in the proof.

Be aware that the verification fails with GCC 9.3 and GCC 10.2 because these compilers are badly broken. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95189.

[real-or-random]

Nice. I can create a follow up PR to add a sage script that computes all the constants here.

One could write optimized multiplication routines for the current shorter g1, g2. But that's a lot of work for a tiny bit of performance so unless we have this, I agree that the new constants are an improvement.

[real-or-random]

Be aware that the verification fails with GCC 9.3 and GCC 10.2 because these compilers are badly broken. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95189.

Yeah, I don't think we want "expected failures". But tbh, I don't know what to do. We could disable the tests for these GCC versions but this is not a good idea either. We could also write our own memcmp for the tests but this is strange, too.

[roconnor-blockstream]

What would you do if we already had such a test in place before GCC released their broken compilers?

Keep in mind these broken compilers are already compromising our existing tests @

secp256k1/src/tests.c

Lines 3458 to 3460 in 770b3dc

	CHECK(memcmp(zero, tmp, 16) == 0);
	secp256k1_scalar_get_b32(tmp, &slam);
	CHECK(memcmp(zero, tmp, 16) == 0);

for example. It is simply that those tests are improperly succeeding instead of improperly failing.

[real-or-random]

@roconnor-blockstream That's different.

If the test succeeds incorrectly, then that's a problem because that single test does not work. And yes, maybe we should fix this.

If the tests fail incorrectly, it's much worse. Then the test binary aborts early and a lot of the tests are simply skipped. Moreover, people will be trained to ignore failing tests / Travis, which is bad.

[elichai]

What about the other memcmp's we have in actual production code? (bip340 tag, tweak add check, scratch impl, sha256 selftest) does this bug affect those too?

[roconnor-blockstream]

The fix for the tests that succeed incorrectly is the same as for fixing these tests. Stop using GCC 9.3 and 10.2 or find some flags to disable the use of intrinsics for memcmp for those particular compiler versions that are broken.

Making code accommodations for compilers with such egregious errors doesn't seem reasonable to me.

[real-or-random]

The fix for the tests that succeed incorrectly is the same as for fixing these tests.

I agree.

Stop using GCC 9.3 and 10.2

We're not in control. We control our code, not the compiler. We can blacklist these compilers but that's ugly and people would override it.

or find some flags to disable the use of intrinsics for memcmp for those particular compiler versions that are broken.

-fno-builtin-memcmp should do the trick but then we need to detect GCC versions in autoconf. That's also addiitonal code (autotools code) and work, and not everyone uses autoconf.

[roconnor-blockstream]

I propose we argue about -fno-builtin-memcmp for a few weeks until GCC 9 and GCC 10 have new point releases and then ignore the problem.

[gmaxwell]

Concept ACK, but the test cannot be included as is-- not even in a couple weeks because the broken compilers have been massively shipped and some users will still be running them for a couple of years (e.g. imagine you setup an offline signer using install media...).

At a minimum the tests should add an explicit memcmp bug test so that the error the user gets is a useful report that tells them that their compiler is faulty and not all other tests are bypassed in case the user wants to continue with the broken compiler. But I think the library should probably ship a workaround (see my comments in #823).

And congrats on finding a compiler bug (though you weren't the first in the case)-- good testing (sadly) finds compiler bugs. The fact that none of the other tests find this suggest to me that there isn't enough mutation testing going on here (or we just got really unlucky, and none of the mutation hittable differences with constant comparisons have an early null).

[real-or-random]

The fact that none of the other tests find this suggest to me that there isn't enough mutation testing going on here (or we just got really unlucky, and none of the mutation hittable differences with constant comparisons have an early null).

We can for sure have better mutation testing but in our defense, we use memcmp in the tests mostly to assert equality and not inequality, and this is a natural thing to do. There are a few inequality checks with constants, e.g.,

secp256k1/src/modules/extrakeys/tests_impl.h

Line 432 in d7838ba

CHECK(memcmp(&keypair, zeros96, sizeof(keypair)) != 0);

But in this case, GCC emits a memcmp call, and I can't convince it to emit wrong code. It's not that easy to hit this bug, it seems to depend on specific contexts and optimizations.

[roconnor-blockstream]

Tests now pass. 😠

[sipa]

utACK d91408e

[sipa]

If we want to add unit tests for lambda splitting that result in numbers very close to the bound:

• split(0x7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0) = (0xa2a8918ca85bafe22016d0b917e4dd76, -0x59de565a2c9d0e2d4373f7623c1d7cd7) [where 0xa2a89... = k1_bound]
• split(0x2c9c52b33fa3cf1f5ad9e3fd77ed9ba54b294b893722e9a500e698ca4cf7632e) = (0x7221bf6b0087441437aa3fd4855ff261, 0x8a65287bd47179fb2be08846cea267eb) [where 0x8a652... = k2_bound-1]

I found these by randomly generating r1 values (within range 0..k1_bound) and r2 values (very close to k2_bound) such that split(r1+lambda*r2)=(r1,r2), and observing that valid r1 values were within a very small range. As I brought r2 closer and closer to k2_bound, the range became narrowing, making it easy to see where it converges. Turns out that it works all the way up to k2_bound-1.

The same approach works for r1 very close to k1_bound, except it needs r2 values in range (-k2_bound...0). There it actually lets us go to k1_bound itself.

[gmaxwell]

That scalar that creates maximal values might be a useful scalar for ecmult tests and as a private key for ECDH too bad you don't achieve the k2_bound.

[sipa]

Here is a commit to add tests for splitting and EC multiplications with those scalars: https://github.com/sipa/secp256k1/commits/202009_pr822. The endomorphism test fails when I introduce small deviations in g1/g2 (though only when the error is around ~2^128 at least).

Feel free to cherry pick, or I can open as a separate PR later.

[sipa]

As it appears that all these large-output values follow a simple pattern ((((ORDER+a)/2 + LAMBDA*b) % ORDER) for small signed odd integers a and small signed integers b), I've expanded the set a bit and added a comment.

[roconnor-blockstream]

Should I add

#ifdef VERIFY
#include <string.h>
#endif

?

[real-or-random]

Should I add

#ifdef VERIFY
#include <string.h>
#endif

?

I don't have strong opinions (and I assume noone has). If you add it, I'll remove it in #825 . :P

[sipa]

Should I add

If we expect to merge #825 afterwards, no need.

[roconnor-blockstream]

I added the include of string.h. I've updated the documentation. I've pick @sipa's tests.

I expect the k2_bound is unachievable. With some effort could probably enumerate the few possible candidate points that could possibly achieve the k2_bound and inspect them to eliminate them as possibilities.

[gmaxwell]

looks good to me

[sipa]

ACK 57d4b0d

Reviewed the diff with the earlier version, created a merge of this with #826 and built/tested it (40000 runs of the unit tests with it).

@roconnor-blockstream It's my belief that the k2_bound is indeed unreachable; if it was, one of the inputs in the scalars_near_split_bounds constant now should trigger it.

[gmaxwell]

For your consideration, I've made some test improvements: https://github.com/gmaxwell/secp256k1/commit/6c3e1fab91766aa0c424b678371a0c8099726655

[gmaxwell]

The bounds check in secp256k1_scalar_split_lambda_verify now doesn't avoid triggering the GCC bug, it just falsely passes in the presence of it (e.g. set the two bounds to all zeros and it keeps passing). Not ideal considering essentially all developers are using impacted compilers. I didn't realize this at first and burnt a few cpu years running tests that could not fail. It appears to be right with the code in #825.

[sipa]

PR that merges this (including @gmaxwell's extra tests) with others: #830

[elichai]

Is it expected to see any meaningful difference in performance here? because I can't see any in the benchmakrs

[real-or-random]

No, this just saves a few instructions and we run this only once per ecmult.

[sipa]

Merged as part of #830.