Update BLS test suite to BLS standard draft v2 format

[hwwhww]

1. Make sure that BLS -Verify APIs would only return True or False, no exceptions.
2. Use eth2spec.utils.bls wrapper instead of py_ecc for test generator (I'm worried about that it's possible py_ecc throws Exceptions)
3. Add assertions in the test generator
4. Add some special test cases for the -Verify APIs
5. Clean up the test format documents

[djrtwo]

Do we have test cases that are throwing?

[hwwhww]

@djrtwo

Do we have test cases that are throwing?

The len(pubkey) == 0 case I added. In py_ecc I suggested we throw errors for debugging-friendly, but that leads to inconsistency, my bad! (/cc @CarlBeek)

In the BLS standard spec:

1. The -Verify APIs and Aggregate API have this precondition where n is the length of pubkeys or the length of signatures: "n >= 1, otherwise return INVALID"
2. The Aggregate API should return "signature, an octet string encoding a aggregated signature that combines all inputs; or INVALID".
   • However, we don't have such INVALID as the output from Aggregate, so what we do is throwing Exception.
3. The -Verify APIs should return "result, either VALID or INVALID".
   • For the preconditions, what py_ecc do is throwing Exception (similar to Aggregate)
   • If the preconditions pass, then the verification would return True or False.

I added len(pubkey) == 0 test cases for the -Verify APIs, but it's a bit tricky to determine what the output of Aggregate([]) should be... maybe None? Do you think we have to add this test case?

[djrtwo]

Aggregate([]) should be... maybe None

is this not defined in the ietf spec?

[hwwhww]

is this not defined in the ietf spec?

The result is defined as "INVALID", and I can't find the formal definition of INVALID from the spec.

Just looked around other implementations:

1. milagro rust: it returns custom error
2. herumi: it seems to me that it returns default signature (0x00...): https://github.com/herumi/bls/blob/78a130231b67490fdde4ef5c454685c04698804b/test/bls_test.hpp#L432-L433 ...? /cc @herumi 👋

---

We only use Aggregate(sigs) in validator guide (get_aggregate_signature) in the spec. We can update the description from "Collect attestations seen via gossip ..." to "Collect attestations seen via gossip ... , where len(attestations) > 0".

Update: addressed it in ab3cbda

Update 2: get_shard_transition also has a bls.Aggregate but it has been handled.

[herumi]

I think that it is better Aggregate([]) sets zero because

• Verify always returns false for zero signature except for the secret key is zero. It is mathematically exact.
• Many languages (including Python, Haskell) return zero for sum([]).
• If blsAggregateSignature is changed to return true/false from now, It's afraid that many existing applications (including non-Ethereum application) remain not to check the results.

[CarlBeek]

Specifically, the spec has a precondition that n>=1 so I think there is some lenience in implementation here.

Personally, I feel that the type of Aggregate should either be a Maybe Signature or return the point at infinity, but I don't think that that is what the spec defines. As was mentioned, they return INVALID for empty aggregates which we have been mapping to False everywhere else. (eg. spec defines Verify to return {VALID, INVALID} which in py_ecc we define as Verify -> bool.)

Verify always returns false for zero signature except for the secret key is zero. It is mathematically exact.

This assumes that the point at infinity is mapped to 0, which is not necessarily the case. The encoding of a point uses the second most significant bit in addition to all zeros to encode infinity.

[herumi]

This assumes that the point at infinity is mapped to 0, which is not necessarily the case. The encoding of a point uses the second most significant bit in addition to all zeros to encode infinity.

Yes, "zero signature" means inifinity. The array of zero bytes is my library's internal representaion, which is converted to c00...0 by serializeToHexStr().

[hwwhww]

@herumi @CarlBeek thanks for the reply!

I understand the standard v.s. usability tradeoff that you both mentioned.

We have made explicit handling in Eth2 spec to avoid Aggregate([]) as possible (ref: #1713 discussion). Having said that, if somehow, unfortunately, Aggregate([]) happens during consensus validation, it is considered as an invalid case.

So I think it's correct that py_ecc & milagro-rust throw Error/Exception and handle the error in client implementation. As for herumi-bls, for safety, maybe the Eth2 clients can add a wrapper to throw an exception or handle 0xc0...00 result especially after calling Aggregate? (currently, we only call Aggregate(sigs) in the validator guide spec)
/cc @prestonvanloon

As for the test vector, I'm inclined to return None since we use None for the invalid case for the state transition test vector.

[djrtwo]

Looks good.

At this point, I'm down to just follow IETF which it seems like our interpretation of Aggregate([]) does so

[herumi]

Okay, I'll modify Go/Rust/WebAssembly binding of my library to throw an exception.