Extensive interface and operations tests for secp256k1_ec_pubkey_parse. (and some API paranoia improvements) #319

1. 
   gmaxwell commented at 11:56 pm on September 27, 2015: contributor

   This also makes use of optional valgrind instrumentation if -DVALGRIND is set.

   0This also moves secp256k1.c above secp256k1.h in tests.c or otherwise
   1 we get non-null macros on the public functions which may defeat some
   2 of the VERIFY checks.
   

2. 
   gmaxwell commented at 11:58 pm on September 27, 2015: contributor

   Perhaps foolishly I made the flags handling in secp256k1_ec_pubkey_serialize absolutely strict. Is this what we want to do? … would we really ever want to add any more flags there? (hybrid? 0_o).
3. in src/secp256k1.c: in 475880283f outdated

   161@@ -159,6 +162,11 @@ int secp256k1_ec_pubkey_serialize(const secp256k1_context* ctx, unsigned char *o
   162     secp256k1_ge Q;
   163 
   164     (void)ctx;
   165+    VERIFY_CHECK(ctx != NULL);
   166+    ARG_CHECK(output != NULL);
   167+    ARG_CHECK(outputlen != NULL);
   168+    ARG_CHECK(pubkey != NULL);
   169+    ARG_CHECK((flags | SECP256K1_EC_COMPRESSED) == SECP256K1_EC_COMPRESSED);
   

   ---

   ---

   

   sipa commented at 11:30 pm on September 29, 2015:

   There is more advanced flag checking in #324. Can you leave it for now?

   ---

   

   gmaxwell commented at 11:41 pm on September 29, 2015:

   Sure. Do you mean leave all this checking out or just the flag part?

   ---

   

   sipa commented at 1:24 am on September 30, 2015:

   Just the flag part.
4. 
   gmaxwell commented at 4:39 am on September 30, 2015: contributor

   Okay, flags test gone.
5. in src/tests.c: in 719f0c3c9d outdated

   1973+        }
   1974+    };
   1975+#define SECP256K1_EC_PARSE_TEST_NINVALID (7)
   1976+    const unsigned char invalid[SECP256K1_EC_PARSE_TEST_NINVALID][64] = {
   1977+        {
   1978+            /* x is third root of -8, y is abs(x^3+7); also on the curve for y^2 = x^3 + 9. */
   

   ---

   ---

   

   sipa commented at 2:55 pm on September 30, 2015:

   I don’t think ‘abs’ has much meaning in a finite field…

   ---

   

   sipa commented at 7:36 pm on September 30, 2015:

   Do you mean sqrt?
6. in src/tests.c: in e80959175b outdated

   1800+            pubkeyc[0] = i;
   1801+            /* What sign does this point have? */
   1802+            ysign = (input[63] & 1) + 2;
   1803+            /* For the current type (i) do we expect parsing to work? Handled all of compressed/uncompressed/hybrid. */
   1804+            xpass = xvalid && (pubkeyclen == 33) && ((i & 254) == 2);
   1805+            ypass = xvalid && yvalid && ((i & 4) == ((pubkeyclen == 65) << 2)) &&
   

   ---

   ---

   

   sipa commented at 2:02 pm on October 13, 2015:

   I’m confused about what ypass means, if it can also be true for 33-byte pubkeys?

   ---

   

   sipa commented at 2:19 pm on October 13, 2015:

   Got it. It means “when taking the current prefix byte + subsequence of input, we can parse and reserialize the result as uncompressed, and it will be identical to the input”. Took me 10 minutes :)
7. in src/tests.c: in e80959175b outdated

   2075+        CHECK(ecount == 0);
   2076+        CHECK(secp256k1_pubkey_load(ctx, &ge, &pubkey) == 0);
   2077+        CHECK(ecount == 1);
   2078+    }
   2079+    /* 33 bytes claimed on otherwise valid input starting with 0x04, fail, zeroize output, no illegal arg error. */
   2080+    memset(&pubkey, 0xfe, sizeof(pubkey));
   

   ---

   ---

   

   sipa commented at 2:28 pm on October 13, 2015:

   Seems it starts with 0xfe, not with 0x04?

   ---

   

   sipa commented at 2:31 pm on October 13, 2015:

   Nevermind, pubkeyc vs pubkey…
8. in src/tests.c: in e80959175b outdated

   2108+    CHECK(ecount == 1);
   2109+    /* 66 bytes claimed, fail, zeroize output, no illegal arg error. */
   2110+    memset(&pubkey, 0xfe, sizeof(pubkey));
   2111+    ecount = 0;
   2112+    VG_UNDEF(&pubkey, sizeof(pubkey));
   2113+    CHECK(secp256k1_ec_pubkey_parse(ctx, &pubkey, pubkeyc, 66) == 0);
   

   ---

   ---

   

   sipa commented at 2:35 pm on October 13, 2015:

   That looks like an error. We know ec_pubkey_parse won’t go look beyond the 65th byte, but it’s a bit ugly that the correctness of the test code relies on that. Can you add a padding byte to pubkeyc?
9. 
   sipa commented at 2:36 pm on October 13, 2015: contributor

   ACK with tiny nits.
10. Add ARG_CHECKs to secp256k1_ec_pubkey_parse/secp256k1_ec_pubkey_serialize

    This also makes secp256k1_ec_pubkey_parse's init of pubkey more unconditional.

    ee2cb4007d
11. Extensive interface and operations tests for secp256k1_ec_pubkey_parse.

    This also makes use of optional valgrind instrumentation if -DVALGRIND
     is set.
    
    This also moves secp256k1.c above secp256k1.h in tests.c or otherwise
     we get non-null macros on the public functions which may defeat some
     of the VERIFY checks.

    67f7da4087
12. 
    gmaxwell commented at 2:00 am on October 14, 2015: contributor

    Nits should all be corrected now.
13. sipa merged this on Oct 14, 2015
14. sipa closed this on Oct 14, 2015

    ---

15. sipa referenced this in commit c98df263ed on Oct 14, 2015

Contributors
gmaxwell sipa