-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Document / assert non-portable assumptions #792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
(edit: keeping a up-to-date list here)
This is a shot in the dark but @roconnor-blockstream do you know some more? |
Might be better to make that config macro get size_t's bits and base the limit on that. Otherwise its reducing portability for unexposed configurability. |
Sorry, I can't follow. Which macro? |
Line 58 in 770b3dc
|
FWIW, these assumptions are asserted in Bitcoin Core: https://github.com/bitcoin/bitcoin/blob/master/src/compat/assumptions.h |
Nope, test_fixed_wnaf_small has some bugs where it uses 32-bit values as int arguments, but other than that and the multi-ecmult tests (which need some fixes to reduce memory usage before I can tests) the tests pass on a 16-bit platform. (nor have I run recovery or ecdh tests yet, but -- generally seems like it works) |
We have a SHA256 self-test now (#799). |
I tried to make that work but it's not that easy. In the precompiler we don't have sizeof (and we can't emulate it because struct padding is unknown etc). In the static assert hack, we can't check for overflow. Maybe it's best to simply set the maximum to something like 10 if SIZE_MAX is smaller than UINT32_MAX. That's not precise but should be good enough in practice. What window size did you use for your 16-bit tests?
Are you willing to fix these? |
ok @roconnor-blockstream suggests something along the lines of
Indeed, this should do it if we additionally check that the shift within |
c0041b5 Add static assertion that uint32_t is unsigned int or wider (Tim Ruffing) Pull request description: Solves one item in #792 . ACKs for top commit: sipa: utACK c0041b5 elichai: ACK c0041b5 Tree-SHA512: 9f700e89be39e15983260da94642593d16b9c437171e10377837ac73731ca7ba5dd7e328b3d93d0a24d143fb9e73abd11c578f6b58e2f94c82b783e977173b0c
If you really have some exotic system, then you might not run ./configure or the tests either. I think the safest way is to use some C static assert hack e.g., https://stackoverflow.com/a/56742411. These looks very ugly but they work and the ugliness is hidden behind a macro. We should also check other simple assumptions such as
sizeof(int) == 4
etc. Alternatively, we could perform some self-tests at context creation time, and call the error callback if they fail. The simple ones will be optimized away by the compiler, and we should have some kind of self-test anyway if we want to implement a SHA256 override (see the discussion in #702).Originally posted by @real-or-random in #767 (comment)
@gmaxwell notes there that the static assert hack may not be the best idea.
The text was updated successfully, but these errors were encountered: