contexts: Forbid destroying, cloning and randomizing the static context #1170

pull real-or-random wants to merge 3 commits into bitcoin-core:master from real-or-random:202212-static-ctx-api changing 5 files +155 −41
  1. real-or-random commented at 3:52 pm on December 7, 2022: contributor

    As discussed in #1126.

    For randomization, this has a history. Initially, this threw the illegal callback but then we changed it to be a no-op on non-signing contexts: https://github.com/bitcoin-core/secp256k1/commit/6198375218b8132f016b701ef049fb295ca28c95 But this was with (non-static) none/verification contexts in mind, not with the static context. If we anyway forbid cloning the static context, you should never a way to randomize a copy of the static context. (You need a copy because the static context itself is not writable. But you cannot obtain a copy except when using memcpy etc.)

  2. real-or-random force-pushed on Dec 7, 2022
  3. real-or-random commented at 4:04 pm on December 7, 2022: contributor

    force-pushed to clarify a comment and fix a commit message, ready for review

    edit: okay, really ready now, I promise.

  4. real-or-random force-pushed on Dec 7, 2022
  5. sipa commented at 4:28 pm on December 7, 2022: contributor 
    0src/tests.c:7420: test condition failed: !secp256k1_context_is_proper(ctx)
  6. real-or-random force-pushed on Dec 7, 2022
  7. real-or-random commented at 4:29 pm on December 7, 2022: contributor
    …. should be fixed. (If you think you can write a single line of code without rerunning the tests…)
  8. sipa commented at 10:04 pm on December 7, 2022: contributor
    utACK 29cac3633a329776b64997b9ae7837ee010cf1e4
  9. real-or-random commented at 11:14 am on December 8, 2022: contributor

    I’m starting to think that it would in fact be preferable not to have this in 0.2.0.

    The changes here are not crucial or urgent. And they’re strictly speaking still breaking changes, and so they’d fit a 0.3.0 or 1.0.0 nicely, where the version bump will indicate that the API has changed.

  10. sipa commented at 3:09 pm on December 8, 2022: contributor

    @real-or-random Sounds good. Making breaking changes (no matter how small) after we’ve started doing releases is better in reducing API ambiguity.

    Are there perhaps still some cleanups from this PR we’d want?

  11. real-or-random commented at 3:27 pm on December 8, 2022: contributor

    Are these perhaps still some cleanups from this PR we’d want?

    The second commit is certainly a (tiny) fix. We should abort the API function (with return) for the case that a callback returns (callbacks aren’t supposed to return but yeah.)

    Let me create a PR that doesn’t have the breaking API changes and the test changes.

  12. real-or-random cross-referenced this on Dec 8, 2022 from issue Change ARG_CHECK_NO_RETURN to ARG_CHECK_VOID which returns (void) by real-or-random
  13. real-or-random commented at 3:36 pm on December 8, 2022: contributor

    Let me create a PR that doesn’t have the breaking API changes and the test changes.

    #1171, marking this one a draft.

  14. real-or-random marked this as a draft on Dec 8, 2022
  15. in src/tests.c:256 in 29cac3633a outdated 
    278+    CHECK(my_sttc != NULL);
279+    memcpy(my_sttc, secp256k1_context_static, sizeof(secp256k1_context));
280+    CHECK(!secp256k1_ecmult_gen_context_is_built(&my_sttc->ecmult_gen_ctx));
281 
282     ecount = 0;
283     ecount2 = 10;
    real-or-random commented at 3:37 pm on December 8, 2022:
    unused variable.
  16. in src/tests.c:253 in 29cac3633a outdated 
    275-
276-    memset(&zero_pubkey, 0, sizeof(zero_pubkey));
277+    my_sttc = malloc(sizeof(*secp256k1_context_static));
278+    CHECK(my_sttc != NULL);
279+    memcpy(my_sttc, secp256k1_context_static, sizeof(secp256k1_context));
280+    CHECK(!secp256k1_ecmult_gen_context_is_built(&my_sttc->ecmult_gen_ctx));
    real-or-random commented at 3:37 pm on December 8, 2022: 
    0    CHECK(!secp256k1_context_is_proper(my_sttc));

    (but my_sttc should probably go away).

  17. real-or-random commented at 3:38 pm on December 8, 2022: contributor
    A cleaner way will be to split run_context_tests() into one function for normal contexts (which has its separate local my_ctx) and one for static contexts (which can use the global sttc).
  18. real-or-random cross-referenced this on Jan 4, 2023 from issue tests: Tidy context tests by real-or-random
  19. real-or-random force-pushed on Jan 4, 2023
  20. real-or-random commented at 5:32 pm on January 4, 2023: contributor
    (force-pushed a rebased version here but please ignore… the plan is to get #1186 merged first and then I need to reapply the interesting commits from here on top of it)
  21. real-or-random referenced this in commit 0eb3000417 on Jan 6, 2023
  22. real-or-random force-pushed on Jan 17, 2023
  23. real-or-random force-pushed on Jan 17, 2023
  24. real-or-random marked this as ready for review on Jan 17, 2023
  25. real-or-random commented at 11:34 am on January 17, 2023: contributor

    Ready for review.

    I should have done the last commit first, but I want to avoid the effort to reorder unless someone insists. @apoelstra This introduces the macro you proposed. I initially didn’t plan to add it in this PR but I got too annoyed by the ecounts. Another PR can use it in the rest of the tests.

  26. real-or-random force-pushed on Jan 17, 2023
  27. real-or-random commented at 2:44 pm on January 17, 2023: contributor
    force-pushed to make LSan happy (hopefully)
  28. real-or-random force-pushed on Jan 17, 2023
  29. in src/tests.c:252 in d2e9636230 outdated 
    246@@ -247,7 +247,16 @@ static void run_static_context_tests(int use_prealloc) {
247 
248     {
249         int ecount = 0;
250+        unsigned char seed[32] = {0x17};
251         secp256k1_context_set_illegal_callback(STATIC_CTX, counting_illegal_callback_fn, &ecount);
252+
    apoelstra commented at 12:50 pm on January 18, 2023:

    In d2e9636230e58f23282418d17b973645f792693e:

    trailing whitespace

    real-or-random commented at 1:00 pm on January 18, 2023:
    fixed
  30. in src/tests.c:59 in 0403278581 outdated 
    57+    ILLEGAL = 0; \
58+    secp256k1_context_set_illegal_callback(ctx, check_illegal_helper_callback_fn, NULL); \
59+    { CHECK((expr) == 0); }                                       \
60+    secp256k1_context_set_illegal_callback(ctx, NULL, NULL); \
61+    CHECK(ILLEGAL == 1); \
62+} while(0);
    apoelstra commented at 12:53 pm on January 18, 2023:

    In 040327858133fec6a6ccaaf6b053d9b058e7ae57:

    I’d prefer that ILLEGAL be a local variable and that we used the data pointer to pass it into the callback function, rather than having it be a global.

    real-or-random commented at 1:04 pm on January 18, 2023:

    Yeah, I had feared this makes the job of the optimizer harder, but maybe that concern is unnecessary (or even wrong).

    I’ll fix it.

    apoelstra commented at 1:22 pm on January 18, 2023:
    It probably will make the optimizer’s job harder but I doubt it’ll be noticeable, and these are only unit tests.
    real-or-random commented at 1:34 pm on January 18, 2023:
    fixed
  31. real-or-random force-pushed on Jan 18, 2023
  32. real-or-random force-pushed on Jan 18, 2023
  33. real-or-random force-pushed on Jan 18, 2023
  34. apoelstra commented at 3:08 pm on January 18, 2023: contributor

    It looks like 13ac8fa2b198e69b826ad241f4291195f5c9c4b4 doesn’t compile on its own.

     0                                                                                                                                
 12023-01-18T14:44:03 13ac8fa2 (232): Command failed: make -j8                                                                                                                                                                     
 22023-01-18T14:44:03 13ac8fa2 (232): In file included from src/assumptions.h:12,                                                                                                                                                  
 3                 from src/secp256k1.c:23,                                                                                                                                                                                        
 4                 from src/tests.c:13:                                                                                                                                                                                            
 5src/tests.c: In function ‘run_proper_context_tests’:                                                                                                                                                                             
 6src/tests.c:346:30: error: ‘my_ctx_fresh’ undeclared (first use in this function)                                                                                                                                                
 7  346 |     CHECK(context_eq(my_ctx, my_ctx_fresh));                                                                                                                                                                             
 8      |                              ^~~~~~~~~~~~                                                                                                                                                                                
 9src/util.h:69:39: note: in definition of macro ‘EXPECT’                                                                                                                                                                          
10   69 | #define EXPECT(x,c) __builtin_expect((x),(c))                                                                                                                                                                            
11      |                                       ^                                                                                                                                                                                  
12src/tests.c:346:5: note: in expansion of macro ‘CHECK’                                                                                                                                                                           
13  346 |     CHECK(context_eq(my_ctx, my_ctx_fresh));                                                                                                                                                                             
14      |     ^~~~~                                                                                                                                                                                                                
15src/tests.c:346:30: note: each undeclared identifier is reported only once for each function it appears in                                                                                                                       
16  346 |     CHECK(context_eq(my_ctx, my_ctx_fresh));                                                                                                                                                                             
17      |                              ^~~~~~~~~~~~                                                                                                                                                                                src/util.h:69:39: note: in definition of macro ‘EXPECT’                                                                                                                                                                          
18   69 | #define EXPECT(x,c) __builtin_expect((x),(c))                                                                                                                                                                            
19      |                                       ^                                                                                                                                                                                  
20src/tests.c:346:5: note: in expansion of macro ‘CHECK’                                                                                                                                                                           
21  346 |     CHECK(context_eq(my_ctx, my_ctx_fresh));                                                                                                                                                                             
22      |     ^~~~~                                                                                                                                                                                                                
23In file included from src/assumptions.h:12,                                                                                                                                                                                      
24                 from src/secp256k1.c:23,                                                                                                                                                                                        
25                 from src/tests.c:13:
  35. contexts: Forbid cloning/destroying secp256k1_context_static 4b6df5e33e
  36. contexts: Forbid randomizing secp256k1_context_static 61841fc9ee
  37. real-or-random force-pushed on Jan 18, 2023
  38. real-or-random commented at 3:48 pm on January 18, 2023: contributor
    Ooops, that line ended up in the wrong commit after rebasing. Should be fixed!
  39. apoelstra approved
  40. apoelstra commented at 4:46 pm on January 18, 2023: contributor
    ACK 68622799302d4c6d2189f5bdf6f48bc54872363a
  41. tests: Add CHECK_ILLEGAL(_VOID) macros and use in static ctx tests e39d954f11
  42. in src/tests.c:55 in 6862279930 outdated 
    50+    /* long descriptive variable name to avoid accidental shadowing and have clear error message */ \
51+    int32_t count_of_calls_to_illegal_callback = 0; \
52+    secp256k1_context_set_illegal_callback(ctx, \
53+        counting_illegal_callback_fn, &count_of_calls_to_illegal_callback); \
54+    { CHECK((expr) == 0); } \
55+    secp256k1_context_set_illegal_callback(ctx, NULL, NULL); \
    sipa commented at 10:24 pm on January 18, 2023:
    Would it make sense to make the macro save and restore the former illegal callback?
    real-or-random commented at 11:01 pm on January 18, 2023:
    Hm, yes, it should probably do that! (It feels a bit unclean to have a public setter function but not a getter. But I don’t think we want one… If you ask me, I’d rather would like to get rid of dynamic callbacks entirely, though I’m not sure yet how this interacts with tests.)
    real-or-random commented at 12:37 pm on January 19, 2023:
    done, and also improved the macro defs a bit further
  43. real-or-random force-pushed on Jan 19, 2023
  44. apoelstra approved
  45. apoelstra commented at 1:24 pm on January 19, 2023: contributor
    ACK e39d954f118a29db2c33e9a9a507053fff5573ed
  46. sipa commented at 5:00 pm on January 19, 2023: contributor
    utACK e39d954f118a29db2c33e9a9a507053fff5573ed
  47. sipa merged this on Jan 19, 2023
  48. sipa closed this on Jan 19, 2023
  49. sipa cross-referenced this on Jan 19, 2023 from issue Signed-digit multi-comb ecmult_gen algorithm by sipa
  50. sipa cross-referenced this on Jan 19, 2023 from issue Signed-digit based ecmult_const algorithm by sipa
  51. dhruv referenced this in commit 4d33046ce3 on Feb 1, 2023
  52. dhruv referenced this in commit 55e7f2cf2b on Feb 2, 2023
  53. stratospher referenced this in commit 647f63669e on Feb 6, 2023
  54. dhruv referenced this in commit a4351c0df6 on Feb 20, 2023
  55. stratospher referenced this in commit 23f825fc8b on Feb 21, 2023
  56. hebasto referenced this in commit 7c0cc5d976 on Mar 7, 2023
  57. dhruv referenced this in commit a5df79db12 on Mar 7, 2023
  58. dhruv referenced this in commit 77b510d84c on Mar 7, 2023
  59. real-or-random cross-referenced this on Mar 8, 2023 from issue release: prepare for 0.3.0 by jonasnick
  60. sipa referenced this in commit 763079a3f1 on Mar 8, 2023
  61. div72 referenced this in commit 945b094575 on Mar 14, 2023
  62. vmta referenced this in commit e1120c94a1 on Jun 4, 2023
  63. vmta referenced this in commit 8f03457eed on Jul 1, 2023
  64. jonasnick cross-referenced this on Jul 20, 2023 from issue Upstream PRs 1160, 1193, 1169, 1190, 1192, 1194, 1196, 1195, 1170, 1172, 1200, 1199, 1203, 1201, 1206, 1078, 1209, 979, 1212, 1218, 1217, 1221, 1222 by jonasnick


real-or-random sipa apoelstra

github-metadata-mirror

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-11-24 13:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me