Also switch the (unused) verification code to low-s instead of even-s. #3637

1. 
   sipa commented at 1:24 AM on February 7, 2014: member

   a81cd968 introduced a malleability breaker for signatures (using an even value for S). In e0e14e43 this was changed to the lower of two potential values, rather than the even one. Only the signing code was changed though, the (for now unused) verification code wasn't adapted.

   Note to @gavinandresen: this brings the total number of test cases to 111.

2. sipa added this to the milestone 0.10.0 on Feb 7, 2014
3. 
   gmaxwell commented at 2:10 AM on February 7, 2014: contributor

   So, this test isn't tight: there are a small number of signature values for which S doesn't have the high bit set (and so is shorter than the length limit) but is still greater than the order. These could still be mutated under this rule.

   In discussion with Pieter, he pointed out that the probability of hitting one of these values was less than 1 in 2^127 and that this way avoids exposing the order of the curve to script.cpp. Certainly the test is much easier implemented this way... which may be attractive if it is ever to become a network rule (in order to actually kill malleability). But then again, not being tight is kinda ugly.

   I could argue either way for this, but I think this particular issue deserves a bit of consideration.

4. 
   sipa commented at 11:15 PM on February 22, 2014: member

   Rebased, and addressed @gmaxwell's comment.

5. Also switch the (unused) verification code to low-s instead of even-s.

   a81cd968 introduced a malleability breaker for signatures
   (using an even value for S). In e0e14e43 this was changed to
   the lower of two potential values, rather than the even one.
   Only the signing code was changed though, the (for now unused)
   verification code wasn't adapted.

   6fd7ef2bbf
6. in src/key.cpp:None in 8a5a98293f outdated

    331 | @@ -332,30 +332,58 @@ class CECKey {
    332 |      }
    333 |  };
    334 |  
    335 | +int CompareBigEndian(const unsigned char *c1, size_t l1, const unsigned char *c2, size_t l2) {
    336 | +    while (l1 > l2) {
   

   ---

   ---

   

   gavinandresen commented at 8:34 PM on February 28, 2014:

   This really looks like eleven > twelve at first glance. How about c1/len1 c2/len2 ... (sad; I do like that l1 looks like eleven...)

7. in src/key.cpp:None in 8a5a98293f outdated

    354 | +        c2++;
    355 | +        l1--;
    356 | +    }
    357 | +    return 0;
    358 | +}
    359 | +
   

   ---

   ---

   

   gavinandresen commented at 8:36 PM on February 28, 2014:

   Comment for these would be welcome; maybe:

   // Order of the secp256k1 ECC curve, in big-endian
   

8. 
   sipa commented at 7:42 PM on March 10, 2014: member

   Rebased + addressed @gavinandresen 's comments.

9. 
   BitcoinPullTester commented at 8:31 PM on March 10, 2014: none

   Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/6fd7ef2bbf1f941c8dee302ffdeb44e603148723 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

10. 
    gavinandresen commented at 3:47 PM on March 13, 2014: contributor

    ACK.

11. laanwj merged this on May 9, 2014
12. laanwj closed this on May 9, 2014

    ---

13. laanwj referenced this in commit 72f754cf51 on May 9, 2014
14. DrahtBot locked this on Sep 8, 2021

Milestone
0.10.0