Fix VERIFY calculations in _fe_cmov methods #266

pull peterdettman wants to merge 2 commits into bitcoin-core:master from peterdettman:cmov-verify changing 3 files +38 −14

peterdettman commented at 4:23 AM on July 4, 2015: contributor

Proposed changes to address #265.
Fix VERIFY calculations in _fe_cmov methods a0601cd79c
sipa commented at 2:54 PM on July 5, 2015: contributor

Untested ACK
Add specific VERIFY tests for _fe_cmov 3f3964e49c
apoelstra commented at 9:03 PM on July 7, 2015: contributor

tested ACK

in src/field_10x26_impl.h:None in 3f3964e49c

1082 | @@ -1083,8 +1083,10 @@ static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe_t *r, const secp256k
1083 |      r->n[8] = (r->n[8] & mask0) | (a->n[8] & mask1);
1084 |      r->n[9] = (r->n[9] & mask0) | (a->n[9] & mask1);
1085 |  #ifdef VERIFY
1086 | -    r->magnitude = (r->magnitude & mask0) | (a->magnitude & mask1);
1087 | -    r->normalized = (r->normalized & mask0) | (a->normalized & mask1);
1088 | +    if (a->magnitude > r->magnitude) {
1089 | +        r->magnitude = a->magnitude;
1090 | +    }

dcousens commented at 3:22 AM on July 8, 2015:

r->magnitude = MAX(a->magnitude, r->magnitude)

Is clearer IMHO, given the short as hell variable names. The compiler probably already makes the branchless optimization though, but conceptually it could be faster too.

peterdettman commented at 2:15 PM on July 8, 2015: contributor

@dcousens I couldn't see any existing usage of min/max in the code, so I just kept it simple here. Not sure it's important for VERIFY checks anyway, but I think it's pretty common for the compiler to eliminate any branch here (at least when optimizations are enabled).
sipa commented at 8:59 PM on July 8, 2015: contributor

MAX is not standard C afaik? we can define it ourselves, of course.

I don't care about performance inside VERIFY. It should be correct, and readable. Usin MAX/MIN helps there, but I think we should do perhaps introduce those independently in another PR if we want it.
dcousens commented at 9:01 PM on July 8, 2015: contributor

@sipa sure, I see no reason to hold up the PR for it. Just wanted to flag it.
sipa merged this on Jul 8, 2015
sipa closed this on Jul 8, 2015
sipa referenced this in commit 0cbc8600f3 on Jul 8, 2015
dcousens cross-referenced this on Jul 8, 2015 from issue Use MAX/MIN over cumbersome statements by dcousens

Contributors

Linked (view graph)

#265 Are _fe_cmov methods handling VERIFY fields correctly?#268 Use MAX/MIN over cumbersome statements