Simplify handling of invalid signatures #4372
Conversation
Just throw exceptions when signatures deserialise incorrectly, including deserialisation to points not in the G2 group.
There was a problem hiding this comment.
This generally looks good, I think it would be worth cleaning up how we're handling exceptions though so that we consistently throw a well-typed exception (probably rename DeserializeException to BlsException). it would still extend from RuntimeException so be unchecked but makes handling it upstream so much easier.
bls/src/main/java/tech/pegasys/teku/bls/impl/blst/BlstSignature.java
Outdated
Show resolved
Hide resolved
bls/src/main/java/tech/pegasys/teku/bls/impl/blst/BlstSignature.java
Outdated
Show resolved
Hide resolved
| return new BlstSignature(sum.to_affine()); | ||
| } catch (RuntimeException e) { | ||
| // Blst performs a G2 group membership test on each signature. We end up here if it fails. | ||
| throw new RuntimeException("Failed to aggregate signatures: " + e.getMessage()); |
There was a problem hiding this comment.
I'm really not a fan of throwing a generic RuntimeException here, it's just too generic and you wind up catching RuntimeException all over the place then, potentially sweeping up exceptions you didn't intend to catch. I'd probably rename DeserializeException to BlsException so we can throw it here. And worth keeping the full details of the original exception too with new BlsException("Failed to aggregate signatures", e)
There was a problem hiding this comment.
Good plan - done. I've reworked exceptions generally across the BLS classes. It's not perfect, but simpler than it was.
The previous implementation used a flag to track whether a signature was valid or invalid. An example of an invalid signature was one that encodes a point not in the G2 group.
This led to oddities like having both valid and invalid infinity signatures. It also led to ambiguity about whether we check the G2 group membership at deserialisation or on usage of a signature. In batch verification we ended up doing both (but not explicitly checking the signature's validity in the calling code). This was all a bit messy and hard to maintain.
In this PR we explicitly delegate the group membership check to the Blst library, and we do not perform the group membership check at deserialisation of the signature. This ought to slightly speed things up (due to no longer duplicating the check in batch verification) and should make things more maintainable.
The risk is that if Blst behaviour changes, or we swap in a different library in future, we could inadvertantly end up without the group membership check. I have added some tests to guard against this.
As a bonus, this closes #4111.