ellswift: fix probabilistic test failure when swapping sides #1378

1. 
   jonasnick commented at 9:39 am on July 17, 2023: contributor

   Reported by jonatack in https://github.com/bitcoin/bitcoin/issues/28079.

   When configured with --disable-module-ecdh --enable-module-recovery, then ./tests 64 81af32fd7ab8c9cbc2e62a689f642106 fails with

   0src/modules/ellswift/tests_impl.h:396: test condition failed: secp256k1_memcmp_var(share32_bad, share32a, 32) != 0
   

   This tests verifies that changing the party bit of the secp256k1_ellswift_xdh function results in a different share. However, that’s not the case when the secret keys of both parties are the same and this is actually what happens in the observed test failure. The keys can be equal in this test case because they are created by the random_scalar_order_test function whose output is not uniformly random (it’s biased towards 0).

   This commit restores the assumption that the secret keys differ.

2. jonasnick cross-referenced this on Jul 17, 2023 from issue libsecp CI failure [no wallet, libbitcoinkernel] [focal] by jonatack
3. ellswift: fix probabilistic test failure when swapping sides

   When configured with `--disable-module-ecdh --enable-module-recovery`, then
   `./tests  64 81af32fd7ab8c9cbc2e62a689f642106` fails with
   ```
   src/modules/ellswift/tests_impl.h:396: test condition failed: secp256k1_memcmp_var(share32_bad, share32a, 32) != 0
   ```
   
   This tests verifies that changing the `party` bit of the
   `secp256k1_ellswift_xdh` function results in a different share. However, that's
   not the case when the secret keys of both parties are the same and this is
   actually what happens in the observed test failure. The keys can be equal in
   this test case because they are created by the `random_scalar_order_test`
   function whose output is not uniformly random (it's biased towards 0).
   
   This commit restores the assummption that the secret keys differ.

   c424e2fb43
4. jonasnick force-pushed on Jul 17, 2023
5. real-or-random approved
6. 
   real-or-random commented at 9:55 am on July 17, 2023: contributor

   utACK c424e2fb43c8ed959b2af7b2216028ce2a023488
7. 
   real-or-random commented at 11:51 am on July 17, 2023: contributor

   random_scalar_order_test function whose output is not uniformly random (it’s biased towards 0).

   Or more precisely, the individual bytes are biased towards 0x00 but random_scalar_order_test will ensure that the scalar is not zero.

8. 
   sipa commented at 2:03 pm on July 17, 2023: contributor

   utACK c424e2fb43c8ed959b2af7b2216028ce2a023488
9. real-or-random merged this on Jul 17, 2023
10. real-or-random closed this on Jul 17, 2023

    ---

11. real-or-random added the label refactor/smell on Jul 17, 2023
12. real-or-random added the label assurance on Jul 17, 2023
13. fanquake referenced this in commit 56c05c5ec4 on Jul 17, 2023
14. fanquake cross-referenced this on Jul 17, 2023 from issue subtree: update libsecp256k1 to latest master by fanquake
15. fanquake referenced this in commit ff061fde18 on Jul 18, 2023
16. fanquake referenced this in commit 84c5416b03 on Jul 19, 2023
17. sidhujag referenced this in commit 381fa93615 on Jul 19, 2023
18. hebasto referenced this in commit 270d2b37b8 on Jul 21, 2023
19. jonasnick cross-referenced this on Jul 27, 2023 from issue Upstream PRs 1357, 1345, 1358, 1368, 1369, 1323, 1364, 1370, 1367, 1344, 1371, 1363, 1340, 1366, 1313, 1378, 1298, 1382, 1383, 1384, 1386 by jonasnick

Contributors
jonasnick real-or-random sipa

Labels
assurance refactor/smell