Make use of abstract unit tests for BLST implementation

[Nashatyrev]

PR Description

Turned out that there are 3 BLST abstract unit tests which were formerly derived just by the Mikuli implementation tests, but were actually intended to test BLST implementation as well.

• Remove succeedsWhenPassingEmptyListToAggregatePublicKeysDoesNotThrowException : aggregating empty set of public keys should formerly return 'infinite point' public key. Don't think this is a case any more as the spec now explicitly prohibits 'infinite points'
• Return 'true' for batchVerify with empty batches list
• aggregateVerifyDuplicateMessages: for some reasons in Mikuli we prohibited duplicate messages when doing aggregateVerify. That condition was added in V0.10.1 #1100. @benjaminion could you please check this if you by any chance remember what was the reason behind this check?
• fix BatchSignatureVerifierTest#shouldRaiseExceptionIfNoValidPublicKeys: throwing IndexOutOfBoundsException was just a side effect. Returning false from batchVerify() is the expected behavior
• Prohibit duplicate messages for aggregateVerify as per BLS spec.

Fixed Issue(s)

Fix #4240

Documentation

• I thought about documentation and added the documentation label to this PR if updates are required.

Changelog

• I thought about adding a changelog entry, and added one if I deemed necessary.

[benjaminion]

aggregateVerifyDuplicateMessages: for some reasons in Mikuli we prohibited duplicate messages when doing aggregateVerify. That condition was added in V0.10.1 #1100. @benjaminion could you please check this if you by any chance remember what was the reason behind this check?

This is in the BLS signature spec:

Procedure:

1. If any two input messages are equal, return INVALID.

[Nashatyrev]

This is in the BLS signature spec:

Procedure:

1. If any two input messages are equal, return INVALID.

Ok, returned back the testcase and prohibited duplicate messages. Just curious what is the reason of this restriction in the BLS spec? Is it because of some kind of rogue public-key attack is available in that case?

[benjaminion]

Just curious what is the reason of this restriction in the BLS spec? Is it because of some kind of rogue public-key attack is available in that case?

I think probably, yes, because of rogue public key attack. See https://crypto.stanford.edu/~dabo/pubs/papers/BLSmultisig.html for example (just above section 2). Since we already prove knowledge of secret key (KOSK), this restriction is unnecessary, but I guess because the BLS spec is generic we must apply the unique messages condition as well.

[ajsutton]

Looks good generally. I'm a little nervous about the new duplicate message check and how it might affect batching of signature verification for gossip.

[ajsutton]

We're now iterating the list of messages twice, seems like it would be better to unroll these streams into an old style for loop and do both checks in a single pass. That would also let us exit early if duplicate messages were found. ie

final Set<Bytes> seenMessages = new HashSet<>();
    for (PublicKeyMessagePair keysToMessage : keysToMessages) {
      // according to BLS spec aggregateVerify allows only distinct messages
      // https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-bls-signature-04#section-3.1.1
      if (!seenMessages.add(keysToMessage.getMessage())) {
        return false;
      }
      
      if (((BlstPublicKey)keysToMessage.getPublicKey()).isInfinity()) {
        return false;
      }
    }

[benjaminion]

In the original conception of the BLS libs, only the methods in BLS.java are guaranteed to be standards-conforming; anything lower-level can be as hacky as we like.

Thus, probably the right thing to do is to leave the duplicate message check in BLS.java (where it already exists), and omit it from the implementation.

Batch verification can then skip the duplicate message check, which will be perfectly ok since we should not be vulnerable to a rogue key attack. Also, batch verification is not in the standard, so we can implement it as we wish. When we run external tests we should hook them into the standards-conforming BLS.java methods, nothing lower level. Thoughts?

[asanso]

My 2 cent. The test seems to target the Basic scheme while in Ethereum we are in the Pop realm (where duplicates are not an issue).
Now Teku doesn't do a clear separation between the 3 schemes specified in the draft (like e.g. py_ecc that implements all 3). I guess before to think about a fix (either delete the test or fix the code) you should decide what you what to support. cc @protolambda

[benjaminion]

Thanks, @asanso Having looked more closely at the POP scheme, aggregateVerify under that should be identical to coreAggregateVerify, which I think I missed previously. This means that we should remove the duplicate message checks everywhere.

We're not aiming to provide a complete BLS implementation, only what's required for Teku/Eth2, so following the POP spec is fine and resolves all of the above discussion :-)

[Nashatyrev]

Thanks @asanso, good catch 👍
Removed 2 duplicate checks and corresponding unit tests

[benjaminion]

Looking good! Sorry for taking so long to get to this,