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
core/vm: add subgroup checks for mul/mulexp for G1/G2 #29637
Conversation
@@ -787,6 +792,11 @@ func (c *bls12381G1MultiExp) Run(input []byte) ([]byte, error) { | |||
if err != nil { | |||
return nil, err | |||
} | |||
// 'point is on curve' check already done, | |||
// Here we need to apply subgroup checks. | |||
if !p.IsInSubGroup() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, this seems un-optimal to me (all these cases). We perform both these calls, with IsOnCurve
done from inside decodePointGX
.
// IsOnCurve returns true if p in on the curve
func (p *G1Affine) IsOnCurve() bool {
var point G1Jac
point.FromAffine(p)
return point.IsOnCurve() // call this function to handle infinity point
}
// IsInSubGroup returns true if p is in the correct subgroup, false otherwise
func (p *G1Affine) IsInSubGroup() bool {
var _p G1Jac
_p.FromAffine(p)
return _p.IsInSubGroup()
}
Seems like it should be faster to do
func (p *G1Affine) IsOnCurveAndInSubGroup() bool {
var point G1Jac
point.FromAffine(p)
point.IsOnCurve() && point.IsInSubGroup()
}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe (probably?) it doesn't make any difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes very little difference I think. The subgroup check is the heaviest, moving from affine to jac is very quick and the curve checks are very quick as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds the subgroup checks as required by the current spec: https://github.com/ethereum/EIPs/pull/8456/files