secp256k1
secp256k1 copied to clipboard
bitcoin-core
Abstract out and merge all the magnitude/normalized logic
Right now, all the logic for propagating/computing the magnitude/normalized fields in secp256k1_fe (when VERIFY is defined) and the code for checking it, is duplicated across the two field implementations. I believe that is undesirable, as these properties should purely be a function of the performed fe_ functions, and not of the choice of field implementation. This becomes even uglier with #967, which would copy all that, and even needs an additional dimension that would then need to be added to the two other fields. It's also related to #1001, which I think will become easier if it doesn't need to be done/reasoned about separately for every field.
This PR moves all logic around these fields (collectively called field verification) to implementations in field_impl.h, which dispatch to renamed functions in field_*_impl.h for the actual implementation.
Fixes #1060.
Concept ACK
But is there you reason why you prefer to do this before #1001? (@peterdettman and I argued to in #1060 that it may make sense to solve #1001.)
I haven't looked at the details here but I think this will non-trivially conflict with https://github.com/bitcoin-core/secp256k1/pull/1062. @sipa can you have a look and suggest how we should move forward?
@real-or-random
But is there you reason why you prefer to do this before #1001? (@peterdettman and I argued to in #1060 that it may make sense to solve #1001.)
The impetus for this is really that I want to bring #967 up-to-date and more production-ready, and doing it properly I think would be very unclear without a change like this (having details from the "old" fields leak into the verify checks of the new one etc).
I'll comment in #1060, but I think this PR makes it at least obvious in what places norm/mag are not compile-time constants.
I haven't looked at the details here but I think this will non-trivially conflict with #1062. @sipa can you have a look and suggest how we should move forward?
It will. That's one of the reasons why I kept the changes for each function in separate commits. So I'm happy to rebase this after #1062, or otherwise incorporate the commits here.