Be stricter with side effects in VERIFY #1485

1. 
   real-or-random commented at 6:06 pm on January 17, 2024: contributor

   Adds a rule to CONTRIBUTING.md and makes the code adhere to it.
2. real-or-random added the label assurance on Jan 17, 2024
3. real-or-random added the label refactor/smell on Jan 17, 2024
4. real-or-random added the label meta/development on Jan 17, 2024
5. 
   real-or-random commented at 6:06 pm on January 17, 2024: contributor

   cc @theStack because you worked on #1393
6. in src/group_impl.h:252 in a24dd539f6 outdated

   249+        size_t i_ver;
   250+        for (i_ver = 0; i_ver < len; i_ver++) {
   251+            SECP256K1_GE_VERIFY(&r[i_ver]);
   252+        }
   253     }
   254 #endif
   

   ---

   ---

   

   real-or-random commented at 6:09 pm on January 17, 2024:

   These loops caught my attention initially. Here we set i inside VERIFY block, but we also use it outside. I think this should be avoided out of an abundance of caution.
7. real-or-random force-pushed on Jan 17, 2024
8. khademreza5 approved
9. in CONTRIBUTING.md:53 in 690a890f03 outdated

   48@@ -49,6 +49,10 @@ In addition, libsecp256k1 tries to maintain the following coding conventions:
   49 * Operations involving secret data should be tested for being constant time with respect to the secrets (see [src/ctime_tests.c](src/ctime_tests.c)).
   50 * Local variables containing secret data should be cleared explicitly to try to delete secrets from memory.
   51 * Use `secp256k1_memcmp_var` instead of `memcmp` (see [#823](https://github.com/bitcoin-core/secp256k1/issues/823)).
   52+* When a test binary is compiled, the `VERIFY` macro will be defined.
   53+  * Use `VERIFY_CHECK(cond)` for assertions. The will only be enabled if `VERIFY` is defined.
   

   ---

   ---

   

   theStack commented at 1:02 pm on January 19, 2024:

   Seems like a missing character typo:

   0  * Use `VERIFY_CHECK(cond)` for assertions. They will only be enabled if `VERIFY` is defined.
   

   (or The check ..., These?)

   ---

   

   real-or-random commented at 7:05 am on April 16, 2024:

   Fixed (and restructured a bit).
10. 
    theStack commented at 1:04 pm on January 19, 2024: contributor

    Concept ACK
11. real-or-random force-pushed on Apr 16, 2024
12. theStack approved
13. 
    theStack commented at 0:15 am on June 26, 2024: contributor

    Code-review ACK f65f1a43e8389cdcce2cdfc5e7df9ffeb872b134
14. real-or-random force-pushed on Jun 26, 2024
15. 
    real-or-random commented at 9:35 am on June 26, 2024: contributor

    thanks, rebased.
16. in src/modules/ellswift/main_impl.h:269 in f3b91f0303 outdated

    265@@ -266,11 +266,11 @@ static int secp256k1_ellswift_xswiftec_inv_var(secp256k1_fe *t, const secp256k1_
    266         secp256k1_fe_mul(&q, &q, &s);                   /* q = s*(4*(u^3+7)+3*u^2*s) */
    267         secp256k1_fe_negate(&q, &q, 1);                 /* q = -s*(4*(u^3+7)+3*u^2*s) */
    268         if (!secp256k1_fe_is_square_var(&q)) return 0;
    269-        ret = secp256k1_fe_sqrt(&r, &q);                /* r = sqrt(-s*(4*(u^3+7)+3*u^2*s)) */
    270+        ret_ver = secp256k1_fe_sqrt(&r, &q);                /* r = sqrt(-s*(4*(u^3+7)+3*u^2*s)) */
    

    ---

    ---

    

    theStack commented at 5:39 pm on June 26, 2024:

    white-space-identation-nit (to be adjusted to the comments above):

    0        ret_ver = secp256k1_fe_sqrt(&r, &q);            /* r = sqrt(-s*(4*(u^3+7)+3*u^2*s)) */
    

    ---

    

    real-or-random commented at 10:54 am on June 27, 2024:

    fixed
17. in src/modules/ellswift/main_impl.h:290 in f3b91f0303 outdated

    286@@ -287,8 +287,8 @@ static int secp256k1_ellswift_xswiftec_inv_var(secp256k1_fe *t, const secp256k1_
    287     }
    288 
    289     /* Let w = sqrt(s). */
    290-    ret = secp256k1_fe_sqrt(&m, &s);                    /* m = sqrt(s) = w */
    291-    VERIFY_CHECK(ret);
    292+    ret_ver = secp256k1_fe_sqrt(&m, &s);                    /* m = sqrt(s) = w */
    

    ---

    ---

    

    theStack commented at 5:40 pm on June 26, 2024:

    0    ret_ver = secp256k1_fe_sqrt(&m, &s);                /* m = sqrt(s) = w */
    

    ---

    

    real-or-random commented at 10:55 am on June 27, 2024:

    fixed
18. theStack approved
19. 
    theStack commented at 5:41 pm on June 26, 2024: contributor

    re-ACK f3b91f0303a1d3885df61f87150ee19b1d20da9f
20. Be stricter with side effects in VERIFY

    Adds a rule to CONTRIBUTING.md and makes the code adhere to it.

    ee7083feb5
21. real-or-random force-pushed on Jun 27, 2024
22. theStack approved
23. 
    theStack commented at 11:24 am on June 27, 2024: contributor

    re-ACK ee7083feb5fedcb217c584656271cbce8cf107af (only whitespace fixes since my previous ACK)
24. gatleas17 approved
25. bitcoin-core deleted a comment on Aug 5, 2024
26. bitcoin-core deleted a comment on Aug 5, 2024

Contributors
real-or-random khademreza5 theStack gatleas17

Labels
assurance refactor/smell meta/development