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.

Sign up for GitHub

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

Jump to bottom

Add an optimization for trivial batch sizes. #151

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add an optimization for trivial batch sizes. #151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benches/ed25519_benchmarks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ mod ed25519_benches {
}

fn verify_batch_signatures(c: &mut Criterion) {
static BATCH_SIZES: [usize; 8] = [4, 8, 16, 32, 64, 96, 128, 256];
static BATCH_SIZES: [usize; 11] = [0, 1, 2, 4, 8, 16, 32, 64, 96, 128, 256];

c.bench_function_over_inputs(
"Ed25519 batch signature verification",
Expand Down
9 changes: 9 additions & 0 deletions src/batch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,15 @@ pub fn verify_batch(
}.into());
}

// If we have been given a trivial batch size, we can exit early
// without going through the trouble of batch verification.
if signatures.len() == 0 {
return Ok(());
} else if signatures.len() == 1 {
use crate::ed25519::signature::Verifier;
return public_keys[0].verify(messages[0], &signatures[0]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Thinking out loud here. I'm not entirely sure if I'm okay with mixing the verification equations because of the subtle differences between the two (but anyone should feel free to try to convince me). See for example my and @valerini's comments on this issue. I suppose in general the single verification is safer than the linearised batching formula, but I still worry that mixing the two behaviours will result in difficult to debug verification errors like the following situation:

  1. I call verify_batch() on bad signature (s1,R1), which fails.
  2. I call verify_batch() on bad signature (s1,R1) and crafted signature (s2,R2) which probabilistically cancels out the linearisation factors and succeeds.

However, I suppose the above behaviour is better than the current behaviour where calling verify_batch() on (s2,R2) alone would probabilistically succeed.

Okay, I think I've convinced myself that this behaviour is safer. My only remaining concern is the debugging confusions it might cause with a signature not verifying if passed into this function by itself versus in a batch with N>1.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @isislovecruft - I am afraid you have the wrong person, probably looking for @kchalkias instead

Copy link
Contributor

@huitseeker huitseeker Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Table 3 in the paper details the issue at hand:
Selection_097

Dalek has a cofactorless single verification with a cofactored batch verification (column "[2]+[3]"), so we can create what you'd call "false negatives" from the PoV of batch verification as a heuristic: a set S of signatures that will (probabilistically) pass batch verification but fail (iterated) single verification.

The present PR eliminates this possibility when N = |S| = 1 by making sure only single verification applies, but does not change the case |S| > 1, as you mentioned.

I think the way to fix the issue is to:

  1. add a cofactored single signature verification,
  2. make sure the batch verification is only available when using this cofactored verification by uniting them both under a "cofactored" feature flag (which would replace the current "batch" feature)
  3. then implement the spirit of this PR: reduce the batch verification call when N = 1 to the (now cofactored) single verification.

I'm happy to help with 1. & 2.

}

// Convert all signatures to `InternalSignature`
let signatures = signatures
.iter()
Expand Down