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

Refactor to monolithic workspace #26

Merged
merged 41 commits into from
Oct 5, 2023
Merged

Refactor to monolithic workspace #26

merged 41 commits into from
Oct 5, 2023

Conversation

tmpfs
Copy link
Contributor

@tmpfs tmpfs commented Oct 2, 2023

Summary of changes

  • Add multi-party-ecdsa as workspace member
  • Add fs-dkr as workspace member
  • Update nightly channel to a more recent version (for workspace.dependencies)
  • Tweak formatting configuration for smaller screens
  • Fix clippy errors for multi-party-ecdsa and fs-dkr
  • Disable link check CI action due to false positives
  • Improve build/test on MacOS when GMP library is installed via homebrew
  • Add workflow to check wasm32-unknown-unknown compilation
  • Re-export multi_party_ecdsa library
  • Remove two_party_ecdsa protocols (Lindell 2017 and CCLST 2019)
  • Remove gg2018 protocol
  • Tidy workspace dependencies
  • Flatten multi_party_ecdsa module namespaces with backwards compatible re-export
  • Migrate to stable toolchain
  • Remove duplicate types from party_i.

@tmpfs tmpfs requested a review from drewstone October 2, 2023 01:05
@tmpfs tmpfs marked this pull request as draft October 2, 2023 01:05
Switch to stable channel for workspace.dependencies.
Must also use the fork in the main Cargo.toml file otherwise we get
conflicts with trait implementations when running the tests.
@tmpfs
Copy link
Contributor Author

tmpfs commented Oct 2, 2023

Also seeing more flaky tests:

running 21 tests
test utilities::aff_g::tests::test_affine_g_proof ... ok
test utilities::dec_q::tests::test_paillier_decryption_modulo_q ... ok
test utilities::enc::tests::test_paillier_encryption_in_range_proof ... ok
test presign::state_machine::test::presign_all_parties_works ... FAILED
test utilities::mta::range_proofs::tests::alice_zkp ... ok
test utilities::log_star::tests::test_log_star_proof ... ok
test utilities::mta::test::test_mta ... ok
test sign::state_machine::test::sign_all_parties_works ... ok
test utilities::mul::tests::test_paillier_mul ... ok
test utilities::mul_star::tests::test_mul_star_proof ... ok
test utilities::zk_pdl_with_slack::test::test_zk_pdl_with_slack ... ok
test utilities::zk_pdl_with_slack::test::test_zk_pdl_with_slack_soundness - should panic ... ok
test utilities::zk_pdl::test::test_zk_pdl ... ok
test presign::state_machine::test::presign_threshold_works ... ok
test sign::state_machine::test::sign_threshold_works ... ok
test utilities::mta::range_proofs::tests::bob_zkp ... ok
test refresh::state_machine::test::test_dkr_with_remove_parties ... ok
test refresh::state_machine::test::test_dkr_with_new_threshold ... ok
test refresh::state_machine::test::test_dkr_with_no_new_parties ... ok
test refresh::state_machine::test::test_dkr_with_replace_parties ... ok
test refresh::state_machine::test::test_dkr_with_new_parties ... ok

failures:

---- presign::state_machine::test::presign_all_parties_works stdout ----
thread 'presign::state_machine::test::presign_all_parties_works' panicked at src/utilities/aff_g/mod.rs:264:79:
called `Result::unwrap()` on an `Err` value: [199, 98, 88, 168, 209, 114, 97, 207, 204, 6, 166, 49, 81, 61, 203, 98, 159, 250, 107, 48, 54, 57, 8, 20, 216, 141, 31, 160, 208, 205, 170]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@drewstone
Copy link
Contributor

Also seeing more flaky tests:

running 21 tests
test utilities::aff_g::tests::test_affine_g_proof ... ok
test utilities::dec_q::tests::test_paillier_decryption_modulo_q ... ok
test utilities::enc::tests::test_paillier_encryption_in_range_proof ... ok
test presign::state_machine::test::presign_all_parties_works ... FAILED
test utilities::mta::range_proofs::tests::alice_zkp ... ok
test utilities::log_star::tests::test_log_star_proof ... ok
test utilities::mta::test::test_mta ... ok
test sign::state_machine::test::sign_all_parties_works ... ok
test utilities::mul::tests::test_paillier_mul ... ok
test utilities::mul_star::tests::test_mul_star_proof ... ok
test utilities::zk_pdl_with_slack::test::test_zk_pdl_with_slack ... ok
test utilities::zk_pdl_with_slack::test::test_zk_pdl_with_slack_soundness - should panic ... ok
test utilities::zk_pdl::test::test_zk_pdl ... ok
test presign::state_machine::test::presign_threshold_works ... ok
test sign::state_machine::test::sign_threshold_works ... ok
test utilities::mta::range_proofs::tests::bob_zkp ... ok
test refresh::state_machine::test::test_dkr_with_remove_parties ... ok
test refresh::state_machine::test::test_dkr_with_new_threshold ... ok
test refresh::state_machine::test::test_dkr_with_no_new_parties ... ok
test refresh::state_machine::test::test_dkr_with_replace_parties ... ok
test refresh::state_machine::test::test_dkr_with_new_parties ... ok

failures:

---- presign::state_machine::test::presign_all_parties_works stdout ----
thread 'presign::state_machine::test::presign_all_parties_works' panicked at src/utilities/aff_g/mod.rs:264:79:
called `Result::unwrap()` on an `Err` value: [199, 98, 88, 168, 209, 114, 97, 207, 204, 6, 166, 49, 81, 61, 203, 98, 159, 250, 107, 48, 54, 57, 8, 20, 216, 141, 31, 160, 208, 205, 170]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

CC @davidsemakula

@drewstone
Copy link
Contributor

@tmpfs looking at the setup for fs-dkr, the RingPedersenProof seems to use a security threshold in M and run iterations for the prover/verifier. This is a pedersen proof which mimics a DLogProof. It could be possible to use this instead of the CompositeDLogProof that the multi-party-ecdsa is using. Would be worth having Ivo take a look. Just a thought that popped into my head reviewing this.

@tmpfs
Copy link
Contributor Author

tmpfs commented Oct 3, 2023

@drewstone , we need to do something with the duplicate mta, zk_pdl and zk_pdl_with_slack modules - they exist in both cggmp-theshold-ecdsa utilties and multi-party-ecdsa utilities. I would assume we would prefer the cggmp-threshold-ecdsa versions - are they interoperable?

I will leave resolving the utilities to a separate PR.

@tmpfs
Copy link
Contributor Author

tmpfs commented Oct 3, 2023

Logging another flaky test failure in fs-dkr, later I will move this to single issue:

---- test::tests::test1 stdout ----
thread 'test::tests::test1' panicked at 'index 248 out of bounds: 248', /Users/muji/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bitvec-1.0.1/src/slice/api.rs:2594:13


failures:
    test::tests::test1

@tmpfs tmpfs marked this pull request as ready for review October 3, 2023 02:00
@tmpfs
Copy link
Contributor Author

tmpfs commented Oct 3, 2023

@drewstone, I think this is as far as this PR should go, please review 🙏

We can resolve the utilities duplication (also the two party_i modules are very similar - can we just use the newer CGGMP one for both protocols?) in later PRs.

@drewstone
Copy link
Contributor

drewstone commented Oct 3, 2023 via email

@davidsemakula
Copy link
Contributor

davidsemakula commented Oct 3, 2023

Also seeing more flaky tests:

running 21 tests
test utilities::aff_g::tests::test_affine_g_proof ... ok
test utilities::dec_q::tests::test_paillier_decryption_modulo_q ... ok
test utilities::enc::tests::test_paillier_encryption_in_range_proof ... ok
test presign::state_machine::test::presign_all_parties_works ... FAILED
test utilities::mta::range_proofs::tests::alice_zkp ... ok
test utilities::log_star::tests::test_log_star_proof ... ok
test utilities::mta::test::test_mta ... ok
test sign::state_machine::test::sign_all_parties_works ... ok
test utilities::mul::tests::test_paillier_mul ... ok
test utilities::mul_star::tests::test_mul_star_proof ... ok
test utilities::zk_pdl_with_slack::test::test_zk_pdl_with_slack ... ok
test utilities::zk_pdl_with_slack::test::test_zk_pdl_with_slack_soundness - should panic ... ok
test utilities::zk_pdl::test::test_zk_pdl ... ok
test presign::state_machine::test::presign_threshold_works ... ok
test sign::state_machine::test::sign_threshold_works ... ok
test utilities::mta::range_proofs::tests::bob_zkp ... ok
test refresh::state_machine::test::test_dkr_with_remove_parties ... ok
test refresh::state_machine::test::test_dkr_with_new_threshold ... ok
test refresh::state_machine::test::test_dkr_with_no_new_parties ... ok
test refresh::state_machine::test::test_dkr_with_replace_parties ... ok
test refresh::state_machine::test::test_dkr_with_new_parties ... ok

failures:

---- presign::state_machine::test::presign_all_parties_works stdout ----
thread 'presign::state_machine::test::presign_all_parties_works' panicked at src/utilities/aff_g/mod.rs:264:79:
called `Result::unwrap()` on an `Err` value: [199, 98, 88, 168, 209, 114, 97, 207, 204, 6, 166, 49, 81, 61, 203, 98, 159, 250, 107, 48, 54, 57, 8, 20, 216, 141, 31, 160, 208, 205, 170]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

CC @davidsemakula

@drewstone @tmpfs The issue is with the current NIZK challenge implementation which performs a fallible conversion from Digest::OutputSize (essentially a 32 bytes array) through BigInt::to_bytes (a Vec<u8> that loses any insignificant bytes e.g. leading zeros) and finally to [u8; 32] (via TryInto::try_into) (i.e. the required input for ChaChaRng::from_seed) thus losing bytes and panicking in cases where the SHA256 hash has any leading zero bytes.

See a minimal example failure case below:

#[test]
fn test_big_int() {
    let bytes = [0, 1];
    let big_int = BigInt::from_bytes(&bytes);
    let bytes_big_int = big_int.to_bytes();

    assert_eq!(bytes.len(), bytes_big_int.len()); // Fails because LHS = 2, while RHS = 1
}

A fix for this would be to go directly from Digest::OutputSize to [u8; 32] which would be an "infallible" conversion.

Another issue is that PaillierAffineOpWithGroupComInRangeProof (and other ZKP types) are generic over H: Digest + Clone, but this current NIZK challenge implementation doesn't account for this by using ChaChaRng::from_seed whose input is [u8; 32]. So a "full" fix would require providing a SeedableRng implementation as a generic parameter depending on the size of the output of the Digest implementation used. (But the crate only uses SHA256 ATM, so may be this part is not of very high priority?).

Finally, as noted above, this NIZK challenge implementation is used in other ZKPs as well, so these fixes should probably be applied to those as well.

I'm a bit too busy elsewhere to write the fixes myself ATM but may be this can help someone else fix the issue(s) 🙂

Copy link
Contributor

@drewstone drewstone left a comment

Choose a reason for hiding this comment

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

LG!

@drewstone drewstone merged commit be6242e into main Oct 5, 2023
5 checks passed
@tmpfs tmpfs mentioned this pull request Oct 6, 2023
@ivokub
Copy link
Contributor

ivokub commented Oct 6, 2023

@tmpfs looking at the setup for fs-dkr, the RingPedersenProof seems to use a security threshold in M and run iterations for the prover/verifier. This is a pedersen proof which mimics a DLogProof. It could be possible to use this instead of the CompositeDLogProof that the multi-party-ecdsa is using. Would be worth having Ivo take a look. Just a thought that popped into my head reviewing this.

Will have a look.

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.

4 participants