These are all the architecture macros I could find with known endianness. Use those as a fallback when BYTE_ORDER isn’t available.
See #787 (comment)
It also adds a SHA256 selftest, so that improperly overriding the endianness detection will be detected at runtime.
ACK 7fde5566a91ffc7fee4606a0a7b6dc6fb5b47444
looks fine, works on the weird things I’ve tested– hakiu, gcc 2.95 i386, openwatcom, debian mips.
185@@ -186,6 +186,26 @@ static SECP256K1_INLINE void *manual_alloc(void** prealloc_ptr, size_t alloc_siz
186 #if defined(_MSC_VER) && defined(_WIN32) && !defined(SECP256K1_LITTLE_ENDIAN)
187 # define SECP256K1_LITTLE_ENDIAN
188 #endif
189+#if !defined(SECP256K1_LITTLE_ENDIAN) && !defined(SECP256K1_BIG_ENDIAN)
#if ? It breaks with the idea of the code above, which tries to detect from all available sources instead and then fail below if both are defined, e.g., if a user supplied #define is inconsistent with our detection. Seems a tiny bit safer.
SECP256K1_BIG_ENDIAN, then __x86_64__ will not be checked at all due to this line (if I’m not getting it wrong entirely).
That is my understanding of the behaviour. If some dumb header has x86_64 in it why shouldn’t the user should be able to override?
I’ve seen stuff set things like that just to make stuff compile when some code was erroneously testing i386 to determine if the processor was 32-bit.
Ok, we can discuss this but it should be consistent at least. The existing code is different, see my commit message in #787.
The macros are carefully written to err on the side of the caution, e.g., we #error if the user manually configures a different endianness than what we detect.
So really we have 3 potentially different sources:
SECP256K1_{LITTLE,BIG}_ENDIAN__BYTE_ORDER__Should all 3 be consistent with each other, if they’re available? That means no mechanism is left for the user to override if the detection is somehow wrong. Should we have a USE_FORCE_{BIG,LITTLE}_ENDIAN for that case?
One possibility is just using SECP256K1_{BIG,LITTLE}_ENDIAN. If exactly one of those two is set, that’s what is used. If not, all other sources that are found have to agree.
I’ve changed the approach here to only check the system macros in case SECP256K1_{BIG,LITTLE}_ENDIAN aren’t explicitly set, so it’s possible to override a possible incorrect detection. I’ve also merged all the macros into one check for each of LE and BE.
In GCC/Clang we could always do the detection, and give a warning if it doesn’t match an explicit SECP256K1_{BIG,LITTLE}_ENDIAN, but not all compilers have a way of doing so in a standard way.
Also permit it being overridden by explicitly passing SECP256K1_{BIG,LITTLE}_ENDIANI’m ok with this but then we should really add a SHA256 self-test or something similar to context creation. For example, one scenario that concerns me is the user provides a custom nonce derivation function for Schnorr signing and for whatever reason uses the wrong endianness.
Then the endianness affects the challenge hash but possibly not the nonce, so this could mean that two different challenge hashes are signed for the same nonce.
Self tests are good. There could be a context creation argument to disable it if you really didn’t want it.
How would you imagine the schnorr hash would change but the nonce wouldn’t? maybe that’s something that could be specifically fought against.
How would you imagine the schnorr hash would change but the nonce wouldn’t?
As I wrote above, this could happen with a user-provided nonce function which just doesn’t care about the endianness assumptions in our code. But that’s the only scenario that comes to my mind.
145@@ -144,8 +146,12 @@ secp256k1_context* secp256k1_context_preallocated_create(void* prealloc, unsigne
146 }
147
148 secp256k1_context* secp256k1_context_create(unsigned int flags) {
149- size_t const prealloc_size = secp256k1_context_preallocated_size(flags);
150- secp256k1_context* ctx = (secp256k1_context*)checked_malloc(&default_error_callback, prealloc_size);
151+ size_t prealloc_size;
152+ secp256k1_context* ctx;
153+
154+ CHECK(secp256k1_selftest());
CHECK is intended to be used in the tests only. It calls abort() (hardcoded) instead of the error callback, so can’t even override this on compile time.
151+ size_t prealloc_size;
152+ secp256k1_context* ctx;
153+
154+ CHECK(secp256k1_selftest());
155+ prealloc_size = secp256k1_context_preallocated_size(flags);
156+ ctx = (secp256k1_context*)checked_malloc(&default_error_callback, prealloc_size);
I think this is what we should do, namely call default_error_callback, which can at least be overridden at compile time. We do this here because we can’t call the error callback unfortunately. It’s just not yet determined at context creation time. I’m going to add a note to #780, maybe we can improve this in the future.
Another advantage of the (default) error callback is that we can provide a more meaningful message, which may even hint at endianness.
default_error_callback, and tested that it works by incorrectly forcing a build with SECP256K1_BIG_ENDIAN.
Very neat!
I grepped for CHECK and apparently we violate this rule in one more place, namely when checking computation of an inverse by gmp. I guess noone noticed this so far simply because if you don’t have abort() on an embedded system, you’ll probably not want to use gmp anyway. We should fix this one too (in a different PR) and then probably move CHECK out of util.h.
edit: Hm now I vaguely recall having that discussion before but I can’t find it. I think we didn’t want to pass the callback down to the arithmetic functions. Maybe the problem just disappears if we can get rid of GMP. :shrug:
I guess noone noticed this so far simply because if you don’t have abort() on an embedded system
It was intentional. Search IRC logs. :) I believe our thinking was that a clean crash in an “impossible” state is preferable to giving a wrong result… returning an error isn’t useful because the caller can’t reasonable be expected to handle impossible errors.
It might still be preferable to keep a sanity check on the inversion after getting rid of libgmp: it’s absurdly cheap and any fast inversion algorithm is fairly complicated. I think if every piece of secp256k1 computation could be checked as relatively cheaply as inversion it might make sense to add additional checks but alas few to nothing else is so cheap. to check. :P
I guess noone noticed this so far simply because if you don’t have abort() on an embedded system
It was intentional. Search IRC logs. :) I believe our thinking was that a clean crash in an “impossible” state is preferable to giving a wrong result… returning an error isn’t useful because the caller can’t reasonable be expected to handle impossible errors.
I don’t understand. Both calling abort() and the error callback are clean crashes. What I’m saying is that your platform may not have abort(), so you can’t even compile our gmp inversion wrapper there.
147@@ -144,8 +148,14 @@ secp256k1_context* secp256k1_context_preallocated_create(void* prealloc, unsigne
148 }
149
150 secp256k1_context* secp256k1_context_create(unsigned int flags) {
151- size_t const prealloc_size = secp256k1_context_preallocated_size(flags);
152- secp256k1_context* ctx = (secp256k1_context*)checked_malloc(&default_error_callback, prealloc_size);
153+ size_t prealloc_size;
154+ secp256k1_context* ctx;
155+
156+ if (!secp256k1_selftest()) {
secp256k1_context_preallocated_create, which already calls the self-tests, so we don’t need to call it again here. (Unless we’re worried that future additions to the selftest should be run before secp256k1_context_preallocated_size or checked_malloc but assume this is not what you had in mind here.)
secp256k1_context_create was called secp256k1_context_preallocated_create. Fixed.
It also adds a SHA256 selftest, so that improperly overriding the endianness detection will be detected at runtime.
I don’t understand the rationale behind the selftest. Improper endianness setting would also be detected by the regular tests. Or is an endianness self test useful because it can be difficult to port the tests to the target platform (I know one example where this was the case)?
ACK 8bc6aeffa9a191e677cb9e3a22fff130f16990f3 looks good and I also ran the tests on MIPS-BE and verified that forcing it to LE makes the runtime test fail. @jonasnick right now the tests are hard to make run on any low memory target. That should be improved but runtime tests have been discussed for a while– the tests may never get run by the particular target– e.g. probably not anywhere where the build system isn’t used, probably not by 99% of bitcoin users that compile their own bitcoin, and they test a different binary (due to the VERIFY_CHECK macros) which may hide miscompilation in the real binary. If sha256’s implementation is made developer-substitutable errors by the library user that could potentially cause silent bad nonce generation… etc.
What I’d suggested before is to have a context creation argument to bypass them if you really must (e.g. rapid context creation) but otherwise, some simple runtime sanity checks seem prudent.
@jonasnick right now the tests are hard to make run on any low memory target.
Is this because of heap allocation? too much stack allocation? something else? if you could specify the requirements/how can I check it myself I can start working on it if it’s feasible to fix without adding too much complexity