Add fallback LE/BE for architectures with known endianness + SHA256 selftest

[sipa]

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.

[gmaxwell]

Great work! testing now.

[gmaxwell]

I can confirm that this works on the systems that were broken for me before (debian + GCC 2.95 and 32-bit haiku). I'm installing a mips (be) host right now to check what macros it has.

[real-or-random]

This could be helpful: https://github.com/rofl0r/endianness.h/blob/master/endianness.h#L52 but we should not overdo it I think .

[sipa]

@real-or-random Might as well use something that has had other eyes on it. I've switched to it.

[luke-jr]

utACK

[gmaxwell]

ACK 7fde556

looks fine, works on the weird things I've tested-- hakiu, gcc 2.95 i386, openwatcom, debian mips.

[sipa]

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.

[real-or-random]

I'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.

[gmaxwell]

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.

[real-or-random]

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.

[sipa]

I've added a single SHA256 selftest, as that should be enough to detect an incorrectly-configured endianness. Other selftests could be added later.

[real-or-random]

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. 🤷

[sipa]

@real-or-random My preference is getting rid of libgmp :)

[gmaxwell]

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

[real-or-random]

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.

[real-or-random]

ACK 8bc6aef I read the diff, and tested that the self-test passes/fails with/without the correct endianness setting

[jonasnick]

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)?

[gmaxwell]

ACK 8bc6aef 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.

[elichai]

@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

[sipa]

I think this is ready.

[jonasnick]

concept ACK