Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken test generators #1575

Merged
merged 6 commits into from
Jan 23, 2020
Merged

Fix broken test generators #1575

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6e73df0
fix broken bls generators
djrtwo Jan 16, 2020
2a0f680
fix ssz_generic test generator
djrtwo Jan 16, 2020
f012844
Add additional bls tests for IETF standards to bls gens
djrtwo Jan 21, 2020
dd9325d
G2Basic to G2ProofOfPossession for private_to_public_key bls test gen…
djrtwo Jan 22, 2020
3b80e12
only expose BLS tests for the used APIs
djrtwo Jan 23, 2020
67763ae
PR feedback
djrtwo Jan 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 42 additions & 44 deletions tests/generators/bls/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ def hash(x):

F2Q_COEFF_LEN = 48
G2_COMPRESSED_Z_LEN = 48
DST = b'BLS_SIG_BLS12381G2-SHA256-SSWU-RO_POP_'
djrtwo marked this conversation as resolved.
Show resolved Hide resolved


def int_to_hex(n: int, byte_length: int = None) -> str:
Expand All @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is unused

byte_value = int_to_big_endian(n)
if byte_length:
byte_value = byte_value.rjust(byte_length, b'\x00')
djrtwo marked this conversation as resolved.
Show resolved Hide resolved
return byte_value


def hex_to_int(x: str) -> int:
return int(x, 16)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line 44 DOMAINS can now be safely removed

Expand Down Expand Up @@ -57,13 +65,11 @@ def hex_to_int(x: str) -> int:
]


def hash_message(msg: bytes,
domain: bytes) -> Tuple[Tuple[str, str], Tuple[str, str], Tuple[str, str]]:
def hash_message(msg: bytes) -> Tuple[Tuple[str, str], Tuple[str, str], Tuple[str, str]]:
"""
Hash message
Input:
- Message as bytes32
- domain as bytes8
Output:
- Message hash as a G2 point
"""
Expand All @@ -72,49 +78,44 @@ def hash_message(msg: bytes,
int_to_hex(fq2.coeffs[0], F2Q_COEFF_LEN),
int_to_hex(fq2.coeffs[1], F2Q_COEFF_LEN),
]
for fq2 in bls.utils.hash_to_G2(msg, domain)
for fq2 in bls.hash_to_curve.hash_to_G2(msg, DST)
]


def hash_message_compressed(msg: bytes, domain: bytes) -> Tuple[str, str]:
def hash_message_compressed(msg: bytes) -> Tuple[str, str]:
CarlBeek marked this conversation as resolved.
Show resolved Hide resolved
"""
Hash message
Input:
- Message as bytes32
- domain as bytes8
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():
Copy link
Contributor

@CarlBeek CarlBeek Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for msg in MESSAGES:
for domain in DOMAINS:
yield f'uncom_g2_hash_{encode_hex(msg)}_{encode_hex(domain)}', {
'input': {
'message': encode_hex(msg),
'domain': encode_hex(domain),
},
'output': hash_message(msg, domain)
}
yield f'uncom_g2_hash_{encode_hex(msg)}', {
'input': {
'message': encode_hex(msg),
},
'output': hash_message(msg)
}


def case02_message_hash_G2_compressed():
for msg in MESSAGES:
for domain in DOMAINS:
yield f'com_g2_hash_{encode_hex(msg)}_{encode_hex(domain)}', {
'input': {
'message': encode_hex(msg),
'domain': encode_hex(domain),
},
'output': hash_message_compressed(msg, domain)
}
yield f'com_g2_hash_{encode_hex(msg)}', {
'input': {
'message': encode_hex(msg),
},
'output': hash_message_compressed(msg)
}


def case03_private_to_public_key():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pubkeys = [bls.privtopub(privkey) for privkey in PRIVKEYS]
pubkeys = [bls.G2Basic.PrivToPub(privkey) for privkey in PRIVKEYS]
djrtwo marked this conversation as resolved.
Show resolved Hide resolved
pubkeys_serial = ['0x' + pubkey.hex() for pubkey in pubkeys]
for privkey, pubkey_serial in zip(PRIVKEYS, pubkeys_serial):
yield f'priv_to_pub_{int_to_hex(privkey)}', {
Expand All @@ -126,39 +127,36 @@ def case03_private_to_public_key():
def case04_sign_messages():
for privkey in PRIVKEYS:
for message in MESSAGES:
for domain in DOMAINS:
sig = bls.sign(message, privkey, domain)
full_name = f'{int_to_hex(privkey)}_{encode_hex(message)}_{encode_hex(domain)}'
yield f'sign_msg_case_{(hash(bytes(full_name, "utf-8"))[:8]).hex()}', {
'input': {
'privkey': int_to_hex(privkey),
'message': encode_hex(message),
'domain': encode_hex(domain),
},
'output': encode_hex(sig)
}
sig = bls.G2ProofOfPossession.Sign(privkey, message)
full_name = f'{int_to_hex(privkey)}_{encode_hex(message)}'
yield f'sign_msg_case_{(hash(bytes(full_name, "utf-8"))[:8]).hex()}', {
'input': {
'privkey': int_to_hex(privkey),
'message': encode_hex(message),
},
'output': encode_hex(sig)
}


# TODO: case05_verify_messages: Verify messages signed in case04
# It takes too long, empty for now


def case06_aggregate_sigs():
for domain in DOMAINS:
for message in MESSAGES:
sigs = [bls.sign(message, privkey, domain) for privkey in PRIVKEYS]
yield f'agg_sigs_{encode_hex(message)}_{encode_hex(domain)}', {
'input': [encode_hex(sig) for sig in sigs],
'output': encode_hex(bls.aggregate_signatures(sigs)),
}
for message in MESSAGES:
sigs = [bls.G2ProofOfPossession.Sign(privkey, message) for privkey in PRIVKEYS]
yield f'agg_sigs_{encode_hex(message)}', {
'input': [encode_hex(sig) for sig in sigs],
'output': encode_hex(bls.G2ProofOfPossession.Aggregate(sigs)),
}


def case07_aggregate_pubkeys():
pubkeys = [bls.privtopub(privkey) for privkey in PRIVKEYS]
pubkeys = [bls.G2Basic.PrivToPub(privkey) for privkey in PRIVKEYS]
pubkeys_serial = [encode_hex(pubkey) for pubkey in pubkeys]
yield f'agg_pub_keys', {
'input': pubkeys_serial,
'output': encode_hex(bls.aggregate_pubkeys(pubkeys)),
'output': encode_hex(bls.G2ProofOfPossession._AggregatePKs(pubkeys)),
}


Expand Down
4 changes: 2 additions & 2 deletions tests/generators/ssz_generic/ssz_container.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from ssz_test_case import invalid_test_case, valid_test_case
from eth2spec.utils.ssz.ssz_typing import SSZType, Container, byte, uint8, uint16, \
uint32, uint64, List, Bytes, Vector, Bitvector, Bitlist
uint32, uint64, List, ByteList, Vector, Bitvector, Bitlist
from eth2spec.utils.ssz.ssz_impl import serialize
from random import Random
from typing import Dict, Tuple, Sequence, Callable
Expand Down Expand Up @@ -32,7 +32,7 @@ class ComplexTestStruct(Container):
A: uint16
B: List[uint16, 128]
C: uint8
D: Bytes[256]
D: ByteList[256]
E: VarTestStruct
F: Vector[FixedTestStruct, 4]
G: Vector[VarTestStruct, 2]
Expand Down