PR feedback from @real-of-random

Co-authored-by: Sean Andersen <6730974+andozw@users.noreply.github.com>

Author: RandomLattice

src/modules/ecdh/tests_impl.h

@@ -168,10 +168,8 @@ static void test_ecdh_wycheproof(void) {
         parsed_ok = secp256k1_ec_pubkey_parse(CTX, &point, pk, testvectors[t].pk_len);
 
         expected_result = testvectors[t].expected_result;
-
         CHECK(parsed_ok == expected_result);
-
-        if (!parsed_ok && expected_result == 0) {
+        if (!parsed_ok) {
             continue;
         }
 

tools/tests_wycheproof_generate_ecdh.py

@@ -13,16 +13,15 @@
 from wycheproof_utils import to_c_array
 
 def should_skip_flags(test_vector_flags):
-    # skip these vectors because they are for ASN.1 encoding issues and other curves
+    # skip these vectors because they are for ASN.1 encoding issues and other curves.
+    # for more details, see https://github.com/bitcoin-core/secp256k1/pull/1492#discussion_r1572491546
     flags_to_skip = {"InvalidAsn", "WrongCurve"}
     return any(flag in test_vector_flags for flag in flags_to_skip)
 
 def should_skip_tcid(test_vector_tcid):
-    '''
-    We skip some test case IDs that have a public key whose custom ASN.1 representation explicitly
-    encodes some curve parameters that are invalid. libsecp256k1 never parses this part so we do
-    not care testing those. See https://github.com/bitcoin-core/secp256k1/pull/1492#discussion_r1572491546
-    '''
+    # We skip some test case IDs that have a public key whose custom ASN.1 representation explicitly
+    # encodes some curve parameters that are invalid. libsecp256k1 never parses this part so we do
+    # not care testing those. See https://github.com/bitcoin-core/secp256k1/pull/1492#discussion_r1572491546
     tcids_to_skip = [496, 497, 502, 503, 504, 505, 507]
     return test_vector_tcid in tcids_to_skip
 
@@ -37,12 +36,12 @@ def parse_der_pk(s):
             L = int(s[2])
             offset = 1
         elif L == 0x82:
-            L = 256*int(s[2]) + int(s[3])
+            L = 256 * int(s[2]) + int(s[3])
             offset = 2
         else:
             raise ValueError("invalid L")
-    value = s[(offset+2):(L+2+offset)]
-    rest = s[(L+2+offset):]
+    value = s[(offset + 2):(L + 2 + offset)]
+    rest = s[(L + 2 + offset):]
 
     if len(rest) > 0 or tag == 0x06: # OBJECT IDENTIFIER
         return parse_der_pk(rest)
@@ -54,12 +53,14 @@ def parse_der_pk(s):
 
 def parse_public_key(pk):
     der_pub_key = parse_der_pk(unhexlify(pk))   # Convert back to str and strip off the `0x`
-    return hexlify(der_pub_key).decode('utf-8')[2:]
+    return hexlify(der_pub_key).decode()[2:]
 
 def normalize_private_key(sk):
     # Ensure the private key is at most 64 characters long, retaining the last 64 if longer.
+    # In the wycheproof test vectors, some private keys have leading zeroes
     normalized = sk[-64:].zfill(64)
-    assert len(normalized) == 64, "private key must be exactly 64 characters long."
+    if len(normalized) != 64:
+        raise ValueError("private key must be exactly 64 characters long.")
     return normalized
 
 def normalize_expected_result(er):
@@ -71,25 +72,19 @@ def normalize_expected_result(er):
 with open(filename_input) as f:
     doc = json.load(f)
 
-num_groups = len(doc['testGroups'])
-
 num_vectors = 0
 offset_sk_running, offset_pk_running, offset_shared = 0, 0, 0
-out = ""
+test_vectors_out = ""
 private_keys = ""
 shared_secrets = ""
 public_keys = ""
 cache_sks = {}
 cache_public_keys = {}
 
-for i in range(num_groups):
-    group = doc['testGroups'][i]
-    num_tests = len(group['tests'])
+for group in doc['testGroups']:
     assert group["type"] == "EcdhTest"
     assert group["curve"] == "secp256k1"
-    for j in range(num_tests):
-        test_vector = group['tests'][j]
-
+    for test_vector in group['tests']:
         if should_skip_flags(test_vector['flags']) or should_skip_tcid(test_vector['tcId']):
             continue
 
@@ -102,8 +97,6 @@ def normalize_expected_result(er):
         sk_size = len(private_key) // 2
         pk_size = len(public_key) // 2
 
-        shared_secrets += ",\n  " if num_vectors and shared_size else ""
-
         new_sk = False
         sk = to_c_array(private_key)
         sk_offset = offset_sk_running
@@ -132,11 +125,13 @@ def normalize_expected_result(er):
         else:
             pk_offset = cache_public_keys[pk]
 
+
+        shared_secrets += ",\n  " if num_vectors and shared_size else ""
         shared_secrets += to_c_array(test_vector['shared'])
         wycheproof_tcid = test_vector['tcId']
 
-        out += "  /" + "* tcId: " + str(test_vector['tcId']) + ". " + test_vector['comment'] + " *" + "/\n"
-        out += f"  {{{pk_offset}, {pk_size}, {sk_offset}, {sk_size}, {offset_shared}, {shared_size}, {expected_result}, {wycheproof_tcid} }},\n"
+        test_vectors_out += "  /" + "* tcId: " + str(test_vector['tcId']) + ". " + test_vector['comment'] + " *" + "/\n"
+        test_vectors_out += f"  {{{pk_offset}, {pk_size}, {sk_offset}, {sk_size}, {offset_shared}, {shared_size}, {expected_result}, {wycheproof_tcid} }},\n"
         if new_sk:
             offset_sk_running += sk_size
         if new_pk:
@@ -167,5 +162,5 @@ def normalize_expected_result(er):
 print("static const unsigned char wycheproof_ecdh_shared_secrets[]  = { " + shared_secrets + "};\n")
 
 print("static const wycheproof_ecdh_testvector testvectors[SECP256K1_ECDH_WYCHEPROOF_NUMBER_TESTVECTORS] = {")
-print(out)
+print(test_vectors_out)
 print("};")

tools/wycheproof_utils.py

@@ -8,5 +8,5 @@
 def to_c_array(x):
     if x == "":
         return ""
-    s = ',0x'.join(a+b for a,b in zip(x[::2], x[1::2]))
+    s = ',0x'.join(a + b for a, b in zip(x[::2], x[1::2]))
     return "0x" + s