Fix broken test generators #1575

djrtwo · 2020-01-16T18:18:28Z

Fix BLS test generator by utilizing new py_ecc functions, types, etc. Strip down test cases to only test the 5 BLS functions in the spec
Fix ssz_generic test generator with a simple import fix (Bytes -> ByteList)

djrtwo · 2020-01-16T18:18:50Z

ChihChengLiang · 2020-01-16T20:16:04Z

I'd lean toward modifying the test case to test AggregateVerify and FastAggregateVerify. In short-run, client teams need to update their tests. But the tests should reflect on the specified functions.

ChihChengLiang

Oops, I forgot the thumbs 👍👍👍

tests/generators/bls/main.py

djrtwo · 2020-01-21T18:22:12Z

added the following test cases:

case05_verify_message handling valid and invalid cases of bls.Verify
case08_fast_aggregate_verify handling valid and invalid cases of bls.FastAggregateVerify
case09_aggregate_verify handling valid and invalid cases of bls.AggregateVerify

I also kept in case07_aggregate_pubkeys as it is an independent feature that might very well be used by a validator/tooling in some cases.

ready for additional review @ChihChengLiang

CarlBeek · 2020-01-22T15:12:36Z

tests/generators/bls/main.py

+                'message': encode_hex(msg),
+            },
+            'output': hash_message_compressed(msg)
+        }


 def case03_private_to_public_key():


I am not convinced that we should be explicitly checking this. While I exposed this functionality in py_ecc, the BLS specs don't feature an implementation of this function. Maybe I should have made it a "private" method.

Is this not an exceedingly common operation that we want to test? -- the determinism of moving from a secret integer to a pubkey?

I suppose I might be missing something

I agree that it is useful to test common functionality and that this functionality is probably common, but PrivToPub is a function I made up. It does not exist in the BLS specifications. I opened an issue about this on the specs ~6 months ago, but they didn't feel like this if functionality that should be required as a part of the specs.

tests/generators/bls/main.py

CarlBeek · 2020-01-22T15:17:40Z

tests/generators/bls/main.py

    Output:
        - Message hash as a compressed G2 point
    """
-    z1, z2 = bls.utils.compress_G2(bls.utils.hash_to_G2(msg, domain))
+    z1, z2 = bls.point_compression.compress_G2(bls.hash_to_curve.hash_to_G2(msg, DST))
    return [int_to_hex(z1, G2_COMPRESSED_Z_LEN), int_to_hex(z2, G2_COMPRESSED_Z_LEN)]


 def case01_message_hash_G2_uncompressed():


Is this (and hash_to_G2_compressed) a meaningful test case? I imagine (and hope) that most BLS implementations won't expose the "raw" hash-to-curve functionality.

I don't feel very strongly. These tests are really to ensure that you've integrated and configured your BLS library properly before getting lost in pyspec tests.

Curious to hear @benjaminion or @mratsim's opinion on this

On the whole, I think that the reference tests should test only things used directly in the spec. Implementation-specific unit tests should take care of anything lower-level. So I'd vote for omitting the aggregate pubkeys tests, as well as the hash-to-G2 tests. The remaining BLS reference tests implicitly test these things in any case.

I should add a concrete rationale for the above, which is that it's undesirable to have to expose the inner workings of the BLS stuff just to satisfy the reference tests. If we can avoid doing that then it's easier to keep everything nicely encapsulated.

agree all around. Only the 5 specified BLS functions are now directly tested in this suite

Would love a last set of review @ChihChengLiang and/or @CarlBeek.
The latest commit is simply removing the non-api related tests

My opinions are basically summarised by @benjaminion's comments above. Only test the high level API functionality that we use in the spec offered by a "standard" implementation of the specs.

…erator Co-Authored-By: Carl Beekhuizen <carl@ethereum.org>

CarlBeek

In principal, I think this is good. In accordance with the ideology of only testing what is exposed to the api I think the following tests should be removed:

case01_message_hash_G2_uncompressed()
case02_message_hash_G2_compressed()
case03_private_to_public_key()

CarlBeek · 2020-01-23T16:14:35Z

tests/generators/bls/main.py

+                'message': encode_hex(msg),
+            },
+            'output': hash_message_compressed(msg)
+        }


 def case03_private_to_public_key():


I agree that it is useful to test common functionality and that this functionality is probably common, but PrivToPub is a function I made up. It does not exist in the BLS specifications. I opened an issue about this on the specs ~6 months ago, but they didn't feel like this if functionality that should be required as a part of the specs.

CarlBeek · 2020-01-23T17:08:55Z

tests/generators/bls/main.py

    Output:
        - Message hash as a compressed G2 point
    """
-    z1, z2 = bls.utils.compress_G2(bls.utils.hash_to_G2(msg, domain))
+    z1, z2 = bls.point_compression.compress_G2(bls.hash_to_curve.hash_to_G2(msg, DST))
    return [int_to_hex(z1, G2_COMPRESSED_Z_LEN), int_to_hex(z2, G2_COMPRESSED_Z_LEN)]


 def case01_message_hash_G2_uncompressed():


My opinions are basically summarised by @benjaminion's comments above. Only test the high level API functionality that we use in the spec offered by a "standard" implementation of the specs.

djrtwo · 2020-01-23T17:21:44Z

Sorry, accidentally didn't push my local commit with the changes. Thanks for the review

CarlBeek

Nitpicks, but LGTM. 👍

tests/generators/bls/main.py

ChihChengLiang

My nitpicks

ChihChengLiang · 2020-01-23T17:25:27Z

tests/generators/bls/main.py

+        byte_value = byte_value.rjust(byte_length, b'\x00')
+    return byte_value
+
+
 def hex_to_int(x: str) -> int:
    return int(x, 16)



The line 44 DOMAINS can now be safely removed

ChihChengLiang · 2020-01-23T17:41:36Z

tests/generators/bls/main.py

@@ -29,6 +30,13 @@ def int_to_hex(n: int, byte_length: int = None) -> str:
    return encode_hex(byte_value)


+def int_to_bytes(n: int, byte_length: int = None) -> bytes:


This function is unused

ChihChengLiang · 2020-01-23T18:22:26Z

Oops, an hour late, never mind 😂

djrtwo · 2020-01-23T18:36:05Z

Thanks! Fixed in v010x

djrtwo added 2 commits January 16, 2020 11:09

fix broken bls generators

6e73df0

fix ssz_generic test generator

2a0f680

djrtwo requested review from protolambda and ChihChengLiang January 16, 2020 18:18

ChihChengLiang approved these changes Jan 17, 2020

View reviewed changes

mratsim mentioned this pull request Jan 17, 2020

[BLS - hash_to_curve] Implicit null-byte terminated strings should be explicitly spec-ed #1576

Closed

mratsim reviewed Jan 17, 2020

View reviewed changes

tests/generators/bls/main.py Show resolved Hide resolved

benjaminion reviewed Jan 17, 2020

View reviewed changes

tests/generators/bls/main.py Outdated Show resolved Hide resolved

benjaminion mentioned this pull request Jan 21, 2020

V0.10.1 Consensys/teku#1100

Merged

6 tasks

Add additional bls tests for IETF standards to bls gens

f012844

djrtwo force-pushed the broken-gens branch from c00048b to f012844 Compare January 21, 2020 18:19

CarlBeek reviewed Jan 22, 2020

View reviewed changes

djrtwo and others added 2 commits January 22, 2020 11:32

G2Basic to G2ProofOfPossession for private_to_public_key bls test gen…

dd9325d

…erator Co-Authored-By: Carl Beekhuizen <carl@ethereum.org>

only expose BLS tests for the used APIs

3b80e12

djrtwo changed the base branch from master to v010x January 23, 2020 01:15

djrtwo mentioned this pull request Jan 23, 2020

Release v0.10.1 to master #1589

Merged

CarlBeek approved these changes Jan 23, 2020

View reviewed changes

CarlBeek reviewed Jan 23, 2020

View reviewed changes

PR feedback

67763ae

djrtwo merged commit ec00f6d into v010x Jan 23, 2020

djrtwo deleted the broken-gens branch January 23, 2020 17:36

ChihChengLiang reviewed Jan 23, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix broken test generators #1575

Fix broken test generators #1575

djrtwo commented Jan 16, 2020 •

edited

Loading

djrtwo commented Jan 16, 2020

ChihChengLiang commented Jan 16, 2020

ChihChengLiang left a comment

djrtwo commented Jan 21, 2020

CarlBeek Jan 22, 2020

djrtwo Jan 22, 2020

CarlBeek Jan 23, 2020

CarlBeek Jan 22, 2020 •

edited

Loading

djrtwo Jan 22, 2020

benjaminion Jan 22, 2020

benjaminion Jan 22, 2020

djrtwo Jan 23, 2020

CarlBeek Jan 23, 2020

CarlBeek left a comment

CarlBeek Jan 23, 2020

CarlBeek Jan 23, 2020

djrtwo commented Jan 23, 2020

CarlBeek left a comment

ChihChengLiang left a comment

ChihChengLiang Jan 23, 2020

ChihChengLiang Jan 23, 2020

ChihChengLiang commented Jan 23, 2020

djrtwo commented Jan 23, 2020

		@@ -29,6 +30,13 @@ def int_to_hex(n: int, byte_length: int = None) -> str:
		return encode_hex(byte_value)


		def int_to_bytes(n: int, byte_length: int = None) -> bytes:

Fix broken test generators #1575

Fix broken test generators #1575

Conversation

djrtwo commented Jan 16, 2020 • edited Loading

djrtwo commented Jan 16, 2020

ChihChengLiang commented Jan 16, 2020

ChihChengLiang left a comment

Choose a reason for hiding this comment

djrtwo commented Jan 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlBeek Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlBeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrtwo commented Jan 23, 2020

CarlBeek left a comment

Choose a reason for hiding this comment

ChihChengLiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChihChengLiang commented Jan 23, 2020

djrtwo commented Jan 23, 2020

djrtwo commented Jan 16, 2020 •

edited

Loading

CarlBeek Jan 22, 2020 •

edited

Loading