Merge pull request #38 from nucypher/validity-checks

.github/workflows/workspace.yml

@@ -127,18 +127,20 @@ jobs:
           key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
           restore-keys: ${{ runner.os }}-cargo-
 
-#      - name: Run benchmarks
-#        uses: boa-dev/criterion-compare-action@v3
-#        if: github.event_name == 'pull_request'
-#        with:
-#          cwd: ${{ matrix.component }}
-#          branchName: ${{ github.base_ref }}
+      - name: Run benchmark for base branch comparison
+        uses: boa-dev/criterion-compare-action@v3
+        if: github.event_name == 'pull_request'
+        with:
+          cwd: ${{ matrix.component }}
+          branchName: ${{ github.base_ref }}
 
       # The next steps have been adapted from https://raw.githubusercontent.com/unicode-org/icu4x/main/.github/workflows/build-test.yml
 
       # Benchmarking & dashboards job > Run benchmark.
 
-      - name: Run benchmark
+      - name: Run benchmark for dashboard
+        # only merges to main (implies PR is finished and approved by this point)
+        if: github.event_name == 'push' && github.ref == 'refs/heads/main' && github.repository == 'nucypher/ferveo'
         run: |
           pushd $PWD && cd ${{ matrix.component }};
           export REL_OUTPUT_PATH="`dirs +1`/benchmarks/perf/${{ matrix.component }}";
@@ -159,25 +161,6 @@ jobs:
         #   # Push it to create a remote branch
         #   $ git push origin <newbranch>:<newbranch>
 
-        # Benchmarking & dashboards job > (unmerged PR only) Convert benchmark output into dashboard HTML in a commit of a branch of the local repo.
-
-      - name: Store benchmark result & create dashboard (unmerged PR only)
-        if: github.event_name == 'pull_request'
-        uses: rhysd/github-action-benchmark@v1
-        with:
-          name: Rust Benchmark
-          tool: 'cargo'
-          output-file-path: ./benchmarks/perf/${{ matrix.component }}/output.txt
-          benchmark-data-dir-path: ./benchmarks/perf/${{ matrix.component }}
-          # Show alert with commit comment on detecting possible performance regression
-          alert-threshold: '200%'  # If for nothing else, enabling the possibility of alerts with meaningful thresholds requires this job to be done per-component
-          fail-on-alert: true
-          gh-pages-branch: unmerged-pr-bench-data  # Requires one-time-only creation of this branch on remote repo.
-          auto-push: false  # Do not store historical benchmark info of unfinished PRs. Commits seem to get made anyways, so make sure
-          # that the branch in `gh-pages-branch` is different from the branch used for merges to main branch.
-          github-token: ${{ secrets.GITHUB_TOKEN }}
-          comment-on-alert: true
-
       # Benchmarking & dashboards job > (PR merge to main only) Convert benchmark output into dashboard HTML in a commit of a branch of the local repo.
 
       - name: Store benchmark result & create dashboard (merge to main only)

ferveo/Cargo.toml

@@ -53,11 +53,17 @@ pprof = { version = "0.6", features = ["flamegraph", "criterion"] }
 name = "pvdkg"
 path = "examples/pvdkg.rs"
 
+#[[bench]]
+#name = "pvdkg"
+#path = "benches/benchmarks/pvdkg.rs"
+#harness = false
+
 [[bench]]
-name = "pvdkg"
-path = "benches/benchmarks/pvdkg.rs"
+name = "benchmarks"
+path = "benches/bench_main.rs"
 harness = false
 
+
 [profile.release]
 opt-level = 3
 lto = true

ferveo/benches/bench_main.rs

@@ -3,6 +3,7 @@ use criterion::criterion_main;
 mod benchmarks;
 
 criterion_main! {
-    benchmarks::pairing::micro,//bench_batch_inverse,
-    benchmarks::pairing::ec,
+    // benchmarks::pairing::micro,//bench_batch_inverse,
+    // benchmarks::pairing::ec,
+    benchmarks::validity_checks::validity_checks,
 }

ferveo/benches/benchmarks/mod.rs

@@ -1,2 +1,3 @@
 //pub mod block_proposer;
-pub mod pairing;
+// pub mod pairing;
+pub mod validity_checks;

ferveo/benches/benchmarks/validity_checks.rs

@@ -0,0 +1,109 @@
+#![allow(clippy::redundant_closure)]
+#![allow(clippy::unit_arg)]
+
+use ark_bls12_381::Bls12_381;
+pub use ark_bls12_381::Bls12_381 as EllipticCurve;
+use criterion::{black_box, criterion_group, BenchmarkId, Criterion};
+use digest::crypto_common::rand_core::SeedableRng;
+use ferveo::*;
+use ferveo_common::ExternalValidator;
+use rand::prelude::StdRng;
+
+const NUM_SHARES_CASES: [usize; 5] = [4, 8, 16, 32, 64];
+
+// TODO: Can we expose ferveo test methods to reuse `setup_dkg` et al instead of reimplementing it here?
+
+fn gen_keypairs(num: u32) -> Vec<ferveo_common::Keypair<EllipticCurve>> {
+    let rng = &mut ark_std::test_rng();
+    (0..num)
+        .map(|_| ferveo_common::Keypair::<EllipticCurve>::new(rng))
+        .collect()
+}
+
+fn gen_validators(
+    keypairs: &[ferveo_common::Keypair<EllipticCurve>],
+) -> Vec<ExternalValidator<EllipticCurve>> {
+    (0..keypairs.len())
+        .map(|i| ExternalValidator {
+            address: format!("validator_{}", i),
+            public_key: keypairs[i].public(),
+        })
+        .collect()
+}
+
+fn setup_dkg(
+    validator: usize,
+    shares_num: u32,
+) -> PubliclyVerifiableDkg<EllipticCurve> {
+    let keypairs = gen_keypairs(shares_num);
+    let validators = gen_validators(&keypairs);
+    let me = validators[validator].clone();
+    PubliclyVerifiableDkg::new(
+        validators,
+        Params {
+            tau: 0,
+            security_threshold: shares_num / 3,
+            shares_num,
+            retry_after: 1,
+        },
+        &me,
+        keypairs[validator],
+    )
+    .expect("Setup failed")
+}
+
+fn setup(
+    shares_num: u32,
+    rng: &mut StdRng,
+) -> (PubliclyVerifiableDkg<Bls12_381>, Message<Bls12_381>) {
+    let mut transcripts = vec![];
+    for i in 0..shares_num {
+        let mut dkg = setup_dkg(i as usize, shares_num);
+        transcripts.push(dkg.share(rng).expect("Test failed"));
+    }
+    let dkg = setup_dkg(0, shares_num);
+    let transcript = transcripts[0].clone();
+    (dkg, transcript)
+}
+
+pub fn bench_verify_full(c: &mut Criterion) {
+    let mut group = c.benchmark_group("PVSS VALIDITY CHECKS");
+    group.sample_size(10);
+
+    let rng = &mut StdRng::seed_from_u64(0);
+
+    for shares_num in NUM_SHARES_CASES {
+        let (dkg, transcript) = setup(shares_num as u32, rng);
+        let transcript = &transcript;
+
+        let pvss_verify_optimistic = {
+            move || {
+                if let Message::Deal(ss) = transcript {
+                    black_box(ss.verify_optimistic());
+                } else {
+                    panic!("Expected Deal");
+                }
+            }
+        };
+        let pvss_verify_full = {
+            move || {
+                if let Message::Deal(ss) = transcript {
+                    black_box(ss.verify_full(&dkg));
+                } else {
+                    panic!("Expected Deal");
+                }
+            }
+        };
+
+        group.bench_function(
+            BenchmarkId::new("pvss_verify_optimistic", shares_num),
+            |b| b.iter(|| pvss_verify_optimistic()),
+        );
+        group.bench_function(
+            BenchmarkId::new("pvss_verify_full", shares_num),
+            |b| b.iter(|| pvss_verify_full()),
+        );
+    }
+}
+
+criterion_group!(validity_checks, bench_verify_full);

ferveo/src/lib.rs

@@ -83,6 +83,7 @@ mod test_dkg_full {
         let plaintext = tpke::checked_decrypt_with_shared_secret(
             &ciphertext,
             aad,
+            &dkg.pvss_params.g_inv(),
             &shared_secret,
         )
         .unwrap();
@@ -130,6 +131,7 @@ mod test_dkg_full {
         let plaintext = tpke::checked_decrypt_with_shared_secret(
             &ciphertext,
             aad,
+            &dkg.pvss_params.g_inv(),
             &shared_secret,
         )
         .unwrap();

ferveo/src/vss/pvss.rs

@@ -41,6 +41,12 @@ pub struct PubliclyVerifiableParams<E: PairingEngine> {
     pub h: E::G2Projective,
 }
 
+impl<E: PairingEngine> PubliclyVerifiableParams<E> {
+    pub fn g_inv(&self) -> E::G1Prepared {
+        E::G1Prepared::from(-self.g.into_affine())
+    }
+}
+
 /// Each validator posts a transcript to the chain. Once enough
 /// validators have done this (their total voting power exceeds
 /// 2/3 the total), this will be aggregated into a final key
@@ -128,6 +134,8 @@ impl<E: PairingEngine, T> PubliclyVerifiableSS<E, T> {
 
     /// Part of checking the validity of an aggregated PVSS transcript
     ///
+    /// Implements check #4 in 4.2.3 section of https://eprint.iacr.org/2022/898.pdf
+    ///
     /// If aggregation fails, a validator needs to know that their pvss
     /// transcript was at fault so that the can issue a new one. This
     /// function may also be used for that purpose.
@@ -138,26 +146,25 @@ impl<E: PairingEngine, T> PubliclyVerifiableSS<E, T> {
         dkg.domain.fft_in_place(&mut commitment);
 
         // Each validator checks that their share is correct
-        dkg.validators.iter().zip(self.shares.iter()).all(
-            |(validator, share)| {
-                // ek is the public key of the validator
-                // TODO: Is that the ek = [dk]H key?
-                let ek = validator
+        dkg.validators
+            .iter()
+            .zip(self.shares.iter())
+            .all(|(validator, y_i)| {
+                // TODO: Check #3 is missing
+                // See #3 in 4.2.3 section of https://eprint.iacr.org/2022/898.pdf
+
+                // Validator checks checks aggregated shares against commitment
+                let ek_i = validator
                     .validator
                     .public_key
                     .encryption_key
                     .into_projective();
-                // Validator checks checks aggregated shares against commitment
-                // TODO: Check #3 is missing
-                // See #3 in 4.2.3 section of https://eprint.iacr.org/2022/898.pdf
-                let y = *share;
-                let a = commitment[validator.share_index];
-                // We verify that e(G, Y_j) = e(A_j, ek_j) for all j
+                let a_i = commitment[validator.share_index];
+                // We verify that e(G, Y_i) = e(A_i, ek_i) for validator i
                 // See #4 in 4.2.3 section of https://eprint.iacr.org/2022/898.pdf
                 // e(G,Y) = e(A, ek)
-                E::pairing(dkg.pvss_params.g, y) == E::pairing(a, ek)
-            },
-        )
+                E::pairing(dkg.pvss_params.g, *y_i) == E::pairing(a_i, ek_i)
+            })
     }
 }
 
@@ -335,6 +342,28 @@ mod test_pvss {
         assert!(!pvss.verify_optimistic());
     }
 
+    /// Check that if PVSS shares are tampered with, the full verification fails
+    #[test]
+    fn test_verify_pvss_bad_shares() {
+        let rng = &mut ark_std::test_rng();
+        let dkg = setup_dkg(0);
+        let s = Fr::rand(rng);
+        let pvss = Pvss::<EllipticCurve>::new(&s, &dkg, rng).unwrap();
+
+        // So far, everything works
+        assert!(pvss.verify_optimistic());
+        assert!(pvss.verify_full(&dkg));
+
+        // Now, we're going to tamper with the PVSS shares
+        let mut bad_pvss = pvss;
+        bad_pvss.shares[0] = G2::zero();
+
+        // Optimistic verification should not catch this issue
+        assert!(bad_pvss.verify_optimistic());
+        // Full verification should catch this issue
+        assert!(!bad_pvss.verify_full(&dkg));
+    }
+
     /// Check that happy flow of aggregating PVSS transcripts
     /// Should have the correct form and validations pass
     #[test]

tpke-wasm/benches/benchmarks.rs

@@ -45,7 +45,7 @@ pub fn bench_encrypt_combine(c: &mut Criterion) {
                 for share in decryption_shares {
                     ss_builder.add_decryption_share(&share);
                 }
-                ss_builder.build();
+                ss_builder.build(&ciphertext);
             })
         }
     }