overflow
output variable, like for the scalar function. I think this makes handling (and ignoring!) overflow much more explicit in the code. What do you think?
field: Improve docs +tests of secp256k1_fe_set_b32 #1205
pull real-or-random wants to merge 4 commits into bitcoin-core:master from real-or-random:202302-verify-overflow changing 4 files +90 −13-
real-or-random commented at 11:31 am on February 1, 2023: contributorWe could also change the interface to accept an explicit
-
field: Verify field element even after secp256k1_fe_set_b32 fails d93f62e369
-
field: Simplify code in secp256k1_fe_set_b32 ca92a35d01
-
real-or-random force-pushed on Feb 1, 2023
-
real-or-random force-pushed on Feb 1, 2023
-
sipa commented at 8:34 pm on February 1, 2023: contributor
Concept ACK, though is the inclusion of the last commit intentional?
An alternative follow-up could be to split the function in two:
- One that doesn’t return whether overflow occurred, and only promises weak normalization.
- One that fails in case overflow occurred (and returns it), and promises strong normalization.
This would get rid of the fact that this is one of the few places in the codebase where field magnitude/normalization properties depend on runtime values (they’re almost always compile-time known otherwise).
-
real-or-random commented at 8:57 am on February 2, 2023: contributor
Concept ACK, though is the inclusion of the last commit intentional?
Yes, but I’m happy to remove it, of course. The story behind this is that I had forgotten to remove a debug printer first when I opened this PR. I mentioned this to @jonasnick who told me that he also had to write these functions multiple times in the past, so we thought it’s a good idea to have one in the code base. Then I made it a bit nicer and added a commit. And I felt it’s not worth a separate PR (but maybe I was just being lazy there).
-
real-or-random commented at 8:59 am on February 2, 2023: contributor
An alternative follow-up could be to split the function in two: […]
That’s a great idea.
We could still have
overflow
as an explicit argument of the (second) function. What do you think? -
sipa commented at 3:20 pm on February 2, 2023: contributor
We could still have
overflow
as an explicit argument of the (second) function. What do you think?It could, but I think that’d be confusing. The second function cannot handle overflow; if it was provided input that’s out of range, it should return failure, and not make any promises about the parsed
secp256k1_fe
. The caller is supposed to return or handle failure in this case. -
real-or-random commented at 4:09 pm on February 2, 2023: contributorOk yes, understood now.
-
sipa commented at 9:16 pm on February 2, 2023: contributorutACK 727d50aaaee6dbadee3b9395b9141783607e5f69
-
sipa cross-referenced this on Feb 5, 2023 from issue Split fe_set_b32 into reducing and normalizing variants by sipa
-
in src/tests.c:2992 in 727d50aaae outdated
2987+ static const unsigned char zero[32] = { 2988+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 2989+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 2990+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 2991+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 2992+ };
jonasnick commented at 3:33 pm on February 6, 2023:nit: fewer loc, easier to check
0 static const unsigned char zero[32] = { 0x00 };
in src/tests.c:54 in 727d50aaae outdated
43@@ -44,6 +44,21 @@ static int all_bytes_equal(const void* s, unsigned char value, size_t n) { 44 return 1; 45 } 46 47+/* Debug helper for printing arrays of unsigned char. */ 48+static void print_buf(const unsigned char *buf, size_t len) { 49+ size_t i; 50+ printf("{");
jonasnick commented at 3:35 pm on February 6, 2023:This would be extra useful if this helper would also output the name of the printed variable. If
print_buf
was a macro we could do it like this:0 printf("%s = {", #buf);
jonasnick commented at 3:37 pm on February 6, 2023: contributorACK mod nitsfield: Improve docs and tests of secp256k1_fe_set_b32 e9fd3dff76tests: Add debug helper for printing buffers 162da73e9areal-or-random force-pushed on Apr 21, 2023real-or-random commented at 3:21 pm on April 21, 2023: contributorforce-pushed to address @jonasnick’s nitsjonasnick commented at 4:07 pm on April 21, 2023: contributorACK 162da73e9a48875aab1ee6ca1c14f86ca4646946jonasnick merged this on Apr 21, 2023jonasnick closed this on Apr 21, 2023
jonasnick cross-referenced this on Apr 23, 2023 from issue tests: remove extra semicolon in macro by jonasnicksipa referenced this in commit 3353d3c753 on May 11, 2023sipa referenced this in commit b4eb644b6c on May 12, 2023hebasto referenced this in commit 49c52ea2b1 on May 13, 2023vmta referenced this in commit e1120c94a1 on Jun 4, 2023sipa referenced this in commit 901336eee7 on Jun 21, 2023vmta referenced this in commit 8f03457eed on Jul 1, 2023jonasnick cross-referenced this on Jul 24, 2023 from issue Upstream PRs 1268, 1276, 1267, 1265, 1230, 1279, 1273, 1263, 1231, 1285, 1283, 1205, 1286, 1275, 1234, 1239, 1240, 1284, 1277, 1289, 1270, 1296, 1301, 1299, 1066, 1300, 1292, 1305, 1303, 1133, 1306, 1207, 1304, 1307, 1311, 1309, 1312 by jonasnick
This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 05:15 UTC
More mirrored repositories can be found on mirror.b10c.me