tangle-network / cggmp-threshold-ecdsa

MPC protocols for threshold ECDSA
GNU General Public License v3.0
46 stars 10 forks source link

Refactor to monolithic workspace #26

Closed tmpfs closed 1 year ago

tmpfs commented 1 year ago

Summary of changes

tmpfs commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

The duplicates ZK pdl stuff is probably entirely duplicated. No difference afaik.

On Tue, Oct 3, 2023 at 6:01 AM muji @.***> wrote:

@drewstone https://github.com/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.

— Reply to this email directly, view it on GitHub https://github.com/webb-tools/cggmp-threshold-ecdsa/pull/26#issuecomment-1744047233, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADELLF4QMEMR6B5NABQJZALX5NWZNAVCNFSM6AAAAAA5OXB2OOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBUGA2DOMRTGM . You are receiving this because you were mentioned.Message ID: @.***>

davidsemakula commented 1 year ago

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) 🙂

ivokub commented 1 year ago

@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.