Is the BLS test aggregate_na_pubkeys correct?

[benjaminion]

There is a new BLS test aggregate_na_pubkeys with these contents:

input: []
output:

There seem to be two issues here:

1. Naming. We don't have a function to aggregate public keys in the spec, only signatures. The other tests in this directory aggregate signatures. So the name should be something like aggregate_na_signatures, right? Or aggregate_for_na_pubkeys maybe.
2. More significantly, the output. If I correctly followed the discussions below, and assuming this is about signatures rather than pubkeys, then the output ought to be 0x0000...0000 (96 bytes), right?

I'm happy to do a PR to fix this, but just want to confirm I'm not missing something.

I am aware of the conversations here, here, and here.

[hwwhww]

@benjaminion

1. Naming

Yes! Agreed with aggregate_na_signatures. Also need to fix the comment to signature instead of pubkey. 👍

2. More significantly, the output. If I correctly followed the discussions below, and assuming this is about signatures rather than pubkeys, then the output ought to be 0x0000...0000 (96 bytes), right?

The reasons for using None as the output:

• IETF spec defines Precondition: n >= 1, otherwise return INVALID for Aggregate as well, not only for pubkeys. So the issue is how to represent the INVALID in our test suite. It's as invalid as other invalid signature input, e.g., if the input is [0x00..00] then it can't be aggregated. We throw an exception in Python.
• In PySpec, if we invoke this case in consensus code in the future, it will throw an exception to signal validation error.
• We already use None to represent invalid state transition cases in the test vector, so following this pattern, we use None for this invalid BLS calls as well.

Do you think it makes sense? It does look a bit weird in YAML format though.

[mratsim]

Can't we just use "Invalid" spelled out?

[benjaminion]

I thought we'd agreed to use 0x00...00 to represent the invalid signature 🤷‍♂️. But, no matter, we can manage as long as it's clear.

Update: I've read through everything more closely, and am now throwing an exception when trying to aggregate an empty list of signatures, which seems to be the final conclusion of the discussion. It's either that or return null.

[prestonvanloon]

Just to clarify, if the output is None then we are supposed to test that there was an exception or error?

[hwwhww]

@benjaminion

0x00...00 to represent the invalid signature

IMO 96-byte output for the invalid cases might be confusing. That means we have to do assert aggregate_signature != NO_SIGNATURE after we called it.

now throwing an exception when trying to aggregate an empty list of signatures

Thanks! I think for your implementation, it's similar to how you show it as other invalid cases you've handled already.

@mratsim

Can't we just use "Invalid" spelled out?

Good question! I'm not against it. /cc @protolambda

@prestonvanloon

Just to clarify, if the output is None then we are supposed to test that there was an exception or error?

Correct. :)

[benjaminion]

Seems to be sorted.