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.

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

Should we validate the pubkeys parameters of fast_aggregate_verify? #40

Closed
ChihChengLiang opened this issue Oct 5, 2020 · 2 comments
Closed

Comments

@ChihChengLiang
Copy link

Hi Kirk,

Currently, the fast_aggregate_verify returns true if the aggregate_public_key is an identity key because we don't run key_validate checks for the input public keys.

The IETF spec defines the behavior of FastAggregateVerify when the preconditions are met. But whether to check those preconditions are left for implementors to decide.

Here are some options:

  • In Eth2's test cases, the validation is expected. It might be safer if all the client teams implement the same behavior.
  • The nim-blscurve implements two versions, one with the check and one without.
@kirk-baird
Copy link
Member

This is a good point. I think it is safe to not check these pre-conditions for fast_aggregate_verify().
This is because one of the pre-conditions is that the public key is verified by proof of possession before calling fast_aggregate_verify().

However since the check is so cheap it's beneficial to add it :)

Added in #41

@kirk-baird
Copy link
Member

I have prevented PublicKeys from being infinity by doing subgroup checks and infinity checks during deserialisation so this should no longer be an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants