Skip to content

perf(bls): remove redundant G2 subgroup check in signature verification#782

Open
sashass1315 wants to merge 1 commit intoa16z:masterfrom
sashass1315:perf/bls-remove-redundant-subgroup-check
Open

perf(bls): remove redundant G2 subgroup check in signature verification#782
sashass1315 wants to merge 1 commit intoa16z:masterfrom
sashass1315:perf/bls-remove-redundant-subgroup-check

Conversation

@sashass1315
Copy link
Copy Markdown

The [Signature::verify()](

impl Signature {
pub fn verify(&self, msg: &[u8], aggregate_public_key: &G1Affine) -> bool {
let sig_point = if let Ok(point) = self.point() {
point
} else {
return false;
};
// Subgroup check for signature
if !subgroup_check_g2(&sig_point) {
return false;
}
method was performing an explicit subgroup check via
[subgroup_check_g2()]
ate2_evaluation(sig_point, &generator_g1_negative, &msg_hash, &key_point)
}
/// Verifies a G2 point is in subgroup `r`.
fn subgroup_check_g2(point: &G2Affine) -> bool {
const CURVE_ORDER: &str = "73EDA753299D7D483339D80809A1D80553BDA402FFFE5BFEFFFFFFFF00000001";
let r = hex_to_scalar(CURVE_ORDER).unwrap();
after calling G2Affine::from_compressed(). However, from_compressed() already performs a subgroup check internally via is_torsion_free() - this is a documented property of the bls12_381 crate.
The explicit check was performing an expensive scalar multiplication (point * r where r is the 255-bit curve order) that duplicated work already done during decompression.

Remove the redundant [subgroup_check_g2()] function and its call from [Signature::verify()]. Also remove the now-unused [hex_to_scalar()] helper and the Scalar import. This eliminates one full G2 scalar multiplicationper signature verification (~30-50% speedup for the verification path).

@sashass1315
Copy link
Copy Markdown
Author

bench rsults:
before:
image
after:
image

@karen-sarkisyan
Copy link
Copy Markdown
Contributor

For context, a similar optimization has been postponed, see here #751 (comment)

@sashass1315
Copy link
Copy Markdown
Author

For context, a similar optimization has been postponed, see here #751 (comment)

Ohh sorry, didn't really thought about looking at PRs. So I close this PR then? if it's yours idea

@karen-sarkisyan
Copy link
Copy Markdown
Contributor

For context, a similar optimization has been postponed, see here #751 (comment)

Ohh sorry, didn't really thought about looking at PRs. So I close this PR then? if it's yours idea

No of course not, let's keep it. Just added a pointer to remember that it's been discussed before.

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

Successfully merging this pull request may close these issues.

2 participants