docknetwork / crypto

Rust crypto library for data privacy tools
Apache License 2.0
78 stars 21 forks source link

ps_proof benchmark fails #20

Closed 0xvon closed 8 months ago

0xvon commented 8 months ago

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 commented 8 months ago

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.

0xvon commented 8 months ago

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

0xvon commented 8 months ago

@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 {
0xvon commented 8 months ago

I created a PR for that two issues. Please check when you have time. https://github.com/docknetwork/crypto/pull/21

lovesh commented 8 months ago

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