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

ps_proof benchmark fails #20

Closed
grandchildrice opened this issue Nov 5, 2023 · 5 comments
Closed

ps_proof benchmark fails #20

grandchildrice opened this issue Nov 5, 2023 · 5 comments

Comments

@grandchildrice
Copy link
Contributor

Hi I'm 0xvon. I'd like to compare some algorithms for blind signatures and its SPKs.
That library especially seems to be so helpful for the performance analysis. However, I failed running ps_proof benchmark due to an error.

Issue Description

When I run the benchmark of ps signature's SPK whis that command:

cargo bench --bench=ps_proof

The error causes with that message.

...
Creating proof for Proof-of-knowledge of signature and corresponding multi-message of size 4/Reveali... #4
                        time:   [6.5353 ms 6.6979 ms 6.8960 ms]
                        change: [-3.8391% +0.2687% +4.7065%] (p = 0.89 > 0.05)
                        No change in performance detected.
Found 12 outliers among 100 measurements (12.00%)
  1 (1.00%) high mild
  11 (11.00%) high severe
thread 'main' panicked at benches/benches/ps_proof.rs:150:14:
called `Result::unwrap()` on an `Err` value: MessageInputError(NoMessagesProvided)
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::result::unwrap_failed
   3: ps_proof::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
error: bench failed, to rerun pass `-p benches --bench ps_proof`

cause

I guess the function UnpackedBlindedMessages::new() doesn't allow empty messages.

solution

I propose two directions to fix this issue. Can you please give me some comments for it?
If you don't take time to fix that quickly, I can help.

  1. Fix benches to skip for empty messages
  2. Fix impl to allow empty messages
@lovesh
Copy link
Member

lovesh commented Nov 5, 2023

Hi @0xvon . Thanks for pointing this out and proposing to help. The error is due to a deliberate design choice. It fails when all messages are being revealed which is quite unlikely in practice. But on rethinking, it should be allowed and the calling code should enforce this restriction if needed. So UnpackedBlindedMessages needs to be changed. Also the test empty_proof needs to be updated to not throw an error.

@grandchildrice
Copy link
Contributor Author

I agree with that! Let me try that issue. I'll send a PR today.

@grandchildrice
Copy link
Contributor Author

@lovesh

Btw, I found that ps_proof's prove_group bench uses the index i instead of j.
For that, when I bench for no less than 30 messages, index out of bounds error causes.
Can I fix like this? I made sure that works.

--- a/benches/benches/ps_proof.rs
+++ b/benches/benches/ps_proof.rs
@@ -76,7 +76,7 @@ fn pok_sig_benchmark(c: &mut Criterion) {
         let sig = &sigs_range[i];
 
         let mut prove_group = c.benchmark_group(format!("Creating proof for Proof-of-knowledge of signature and corresponding multi-message of size {}", count));
-        for (_j, r_count) in k.iter().enumerate() {
+        for (j, r_count) in k.iter().enumerate() {
             prove_group.bench_with_input(
                 BenchmarkId::from_parameter(format!("Revealing {} messages", r_count)),
                 &r_count,
@@ -89,7 +89,7 @@ fn pok_sig_benchmark(c: &mut Criterion) {
                                     .iter()
                                     .enumerate()
                                     .merge_join_by(
-                                        revealed_indices[i].iter(),
+                                        revealed_indices[j].iter(),
                                         |(m_idx, _), reveal_idx| m_idx.cmp(reveal_idx),
                                     )
                                     .map(|either| match either {

@grandchildrice
Copy link
Contributor Author

I created a PR for that two issues. Please check when you have time.
#21

@lovesh
Copy link
Member

lovesh commented Nov 6, 2023

@0xvon Thanks for the PR. And yes, you are correct about the loop index j. Thanks for fixing that too.

@lovesh lovesh closed this as completed Nov 7, 2023
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