Fix clippy warnings and format README #71

kigawas · 2019-08-15T01:29:07Z

Fix #72
Fix #74

kigawas · 2019-08-15T01:29:30Z

Readme was formatted by https://github.com/DavidAnson/vscode-markdownlint

omershlo · 2019-08-15T11:15:29Z

hi @kigawas -
in my audit1 branch I also fixed clippy warnings a couple of days ago (its an audit branch and clippy is part of the audit). I do see that you have some good fixes in this PR as well as the new readme, but I am not sure how to merge them in my branch. Can you help please?

As a side comment we are working on adding tags which will make life easier for all.

kigawas · 2019-08-15T11:34:34Z

We can merge this PR first and resolve conflicts later

README.md

src/protocols/multi_party_ecdsa/gg_2018/party_i.rs

src/protocols/two_party_ecdsa/lindell_2017/party_two.rs

omershlo · 2019-08-15T14:03:13Z

@oleiba

kigawas · 2019-08-16T02:07:08Z

moved some dependencies to dev-dependencies, mainly rocket

kigawas · 2019-09-22T04:08:50Z

For 1, mainly in this commit
8a7d1a5

Rational:
Only deriving ParitialEq for part of messages is not consistent, actually, the comparing equal part should be left to user. Like this

use multi_party_ecdsa::protocols::multi_party_ecdsa::gg_2018::party_i::{
	KeyGenBroadcastMessage1 as KeyGenCommit, KeyGenDecommitMessage1 as KeyGenDecommit,
};
use curv::cryptographic_primitives::proofs::sigma_dlog::DLogProof;
use curv::cryptographic_primitives::secret_sharing::feldman_vss::VerifiableSS;
use curv::FE;

#[derive(Clone, Debug, Serialize, Deserialize)]
pub enum KeyGenMessage {
	CommitAndDecommit(PeerIndex, KeyGenCommit, KeyGenDecommit),
	VSS(PeerIndex, VerifiableSS),
	SecretShare(PeerIndex, FE),
	Proof(PeerIndex, DLogProof),
}

impl PartialEq for KeyGenMessage {
	fn eq(&self, other: &Self) -> bool {
		match (self, other) {
			(Self::CommitAndDecommit(ia, ca, da), Self::CommitAndDecommit(ib, cb, db)) => {
				ia == ib
					&& ca.com == cb.com && ca.e == cb.e
					&& da.blind_factor == db.blind_factor
					&& da.y_i == db.y_i
			}
			(Self::VSS(ia, vssa), Self::VSS(ib, vssb)) => ia == ib && vssa == vssb,
			(Self::SecretShare(ia, ssa), Self::SecretShare(ib, ssb)) => ia == ib && ssa == ssb,
			(Self::Proof(ia, pa), Self::Proof(ib, pb)) => ia == ib && pa == pb,
			_ => false,
		}
	}
}

kigawas · 2019-09-22T04:13:19Z

For 2, I only remove the redundant mod like

// test.rs
#[cfg(test)]
mod tests {  // <- no need
    fn test_a() {}
    fn test_b() {}
}

// lib.rs
mod test;

to

// test.rs
fn test_a() {}
fn test_b() {}

// lib.rs
#[cfg(test)]
mod test;

kigawas · 2019-09-22T04:13:58Z

This should also be applied to bench

kigawas · 2019-09-22T04:21:28Z

Also add test of checking signature against other secp256k1 library (https://github.com/sorpaas/libsecp256k1-rs)

cce7832

omershlo · 2019-09-22T05:47:43Z

some examples for new variables:
raw_msg
raw_pk
compact

I don't understand why they are necessary ?

2)I think that It is impossible to implement PatrtialEq from outside the library if some elements in the struct are private. Usually when there's a PartialEq derive it means that it is was required by some other library. Therefore I am not very excited with removing them for now

kigawas · 2019-09-22T07:15:43Z

If you add more variables, you have to add new names

Simply deriving PartialEq is not enough, user has to adapt it to his own needs, which means that comparing two messages are equal or what makes them equal should be left to user's implementation. It's certainly possible, the example above was copied from real codes I'm writing.

And if you define "messages", obviously they are used outside, and their fields should be public.

kigawas · 2019-09-22T07:24:21Z

Of course, deriving PartialEq for all messages is also a solution, which makes user write less codes.

I have no preference for this though, both of them are okay

Cargo.toml

oleiba · 2019-09-22T09:06:29Z

Cargo.toml

 serde_json = "1.0"
+libsecp256k1 = "0.2.2"


why is this dependency needed?
I don't see it consumed anywhere.
Also curv uses secp256k1 and not libsecp256k1 and for a very good reason I believe - secp256k1 uses C bindings for libsecp256k1 in C, which is by far the most battle tested library for secp256k1...

This is for testing signature against another secp256k1 repo

Rust wrapper of C bindings for libsecp256k1 is not reliable as you think, see rust-bitcoin/rust-secp256k1#163

Of course, I'm just saying the rust wrapper is with problem

Can you please point me to where libsecp256k1 is consumed?

libp2p and substrate, is it enough to convince you?
https://github.com/libp2p/rust-libp2p
https://github.com/paritytech/substrate/

Even it is not widely used, there is no hurt if you use it for testing against signature

And why we don't use C bindings?
The answer is:
rust-bitcoin/rust-secp256k1#163

That is to say, it'll break the test

oleiba · 2019-09-22T09:27:58Z

benches/multi_party_ecdsa/gg18/keygen.rs

-        let party_keys_vec = (0..n.clone())
-            .map(|i| Keys::create(i))
-            .collect::<Vec<Keys>>();
+        let (t, n) = (t as usize, n as usize);


is redefining absolutely necessary here? are they used into APIs that require usize?
Maybe this PR should focus on changing the requirement of the APIs to use u16 rather than the consuming code in benches?
e.g.

https://github.com/kigawas/multi-party-ecdsa/blob/keygen-clippy/src/protocols/multi_party_ecdsa/gg_2018/party_i.rs#L56

https://github.com/kigawas/multi-party-ecdsa/blob/keygen-clippy/src/protocols/multi_party_ecdsa/gg_2018/party_i.rs#L147

https://github.com/kigawas/multi-party-ecdsa/blob/keygen-clippy/src/protocols/multi_party_ecdsa/gg_2018/party_i.rs#L161

This is just a handy redefinition to avoid lots of as usize below

So it doesn't fix #72?

should've already fixed

I'll also fix other usize

About party_index, since secret shares (in curv) are taking usize, I'll just keep it here

benches/two_party_ecdsa/lindell_2017/keygen.rs

examples/common.rs

oleiba

Thank you for your work. I added comments I'll be glad to discuss.

omershlo · 2019-09-22T12:05:55Z

@kigawas is it possible to divide this PR into few smaller PRs?
I think it will be easier to digest and merge faster overall.

kigawas · 2019-09-22T12:16:04Z

@omershlo
mainly 3 parts

fix clippy warnings and switch to rust 2018
refactor test and demo
minor derive traits change

you can check it by commit

omershlo · 2019-10-26T22:49:12Z

Hi @kigawas , we recently merged a PR containing known issues raised by the audit. Can you take the latest v0.2.4 into this PR?

@oleiba do you still have open issues here?

…-ecdsa into keygen-clippy

omershlo · 2019-10-31T15:21:23Z

src/protocols/multi_party_ecdsa/gg_2018/party_i.rs

-        assert_eq!(decom_vec.len(), params.share_count);
-        assert_eq!(bc1_vec.len(), params.share_count);
+        assert_eq!(decom_vec.len() as u16, params.share_count);
+        assert_eq!(com_vec.len() as u16, params.share_count);


I don't think this rename is necessary.

omershlo · 2019-10-31T15:30:00Z

src/protocols/multi_party_ecdsa/gg_2018/party_i.rs

+        if test_b_vec_and_com {
+            Ok({
+                let gamma_sum = tail.fold(head.g_gamma_i, |acc, x| acc + x.g_gamma_i);
+                gamma_sum * delta_inv


Please don't remove the R or otherwise add a comment that this is R

omershlo · 2019-10-31T15:53:20Z

@kigawas I see you changed the param file to params.json. In that case can you also provide such file in the repo (replacing the old params file) ?

omershlo · 2019-10-31T16:01:12Z

I finished my review. I approve once the comments above are resolved.
I am also wondering if we should add to this pr #72 ... wdyt?

Fix key_gen.rs clippy warnings and format README

8401fbc

kigawas changed the title ~~Fix key_gen.rs clippy warnings and format README~~ Fix clippy warnings and format README Aug 15, 2019

kigawas changed the title ~~Fix clippy warnings and format README~~ Fix clippy warnings and format README in bench Aug 15, 2019

kigawas changed the title ~~Fix clippy warnings and format README in bench~~ Fix clippy warnings and format README Aug 15, 2019

Fix more clippy warnings and add .rustfmt.toml

3b17ed3

omershlo self-requested a review August 15, 2019 05:17

omershlo self-assigned this Aug 15, 2019

kigawas added 5 commits August 15, 2019 14:59

Fix more clippy warnings

9a72448

Improve test

5df4f5d

Improve test

f868dcb

Shebang for demo/run.sh

863640c

Improve demo/run.sh

eee722a

omershlo requested a review from talbeerysec August 15, 2019 10:28

kigawas mentioned this pull request Aug 15, 2019

t and n should not be usize #72

Closed

omershlo requested a review from oleiba August 15, 2019 11:49

omershlo reviewed Aug 15, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

omershlo reviewed Aug 15, 2019

View reviewed changes

src/protocols/multi_party_ecdsa/gg_2018/party_i.rs Show resolved Hide resolved

omershlo reviewed Aug 15, 2019

View reviewed changes

src/protocols/two_party_ecdsa/lindell_2017/party_two.rs Show resolved Hide resolved

Fix readme table

9580338

kigawas force-pushed the keygen-clippy branch from 81df474 to 9580338 Compare August 15, 2019 13:24

omershlo closed this Aug 15, 2019

omershlo reopened this Aug 15, 2019

kigawas added 2 commits August 16, 2019 09:16

Improve sm_manager.rs

5324c6c

Move demo to examples folder

bd3356d

Finish fix clippy warnings

b7676eb

oleiba reviewed Sep 22, 2019

View reviewed changes

Cargo.toml Show resolved Hide resolved

oleiba reviewed Sep 22, 2019

View reviewed changes

Cargo.toml Show resolved Hide resolved

oleiba reviewed Sep 22, 2019

View reviewed changes

benches/two_party_ecdsa/lindell_2017/keygen.rs Show resolved Hide resolved

oleiba reviewed Sep 22, 2019

View reviewed changes

examples/common.rs Show resolved Hide resolved

oleiba suggested changes Sep 22, 2019

View reviewed changes

default features false of rocket

5dcb715

kigawas added 3 commits September 23, 2019 21:26

fix bench

9d65342

fix pk slice

10c0325

Merge branch 'master' into keygen-clippy

51f43af

Merge branch 'master' of https://github.com/KZen-networks/multi-party…

eb5ecf9

…-ecdsa into keygen-clippy

kigawas requested review from omershlo and oleiba October 29, 2019 01:47

omershlo reviewed Oct 31, 2019

View reviewed changes

omershlo merged commit 019ed6a into ZenGo-X:master Nov 3, 2019

kigawas deleted the keygen-clippy branch November 8, 2019 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix clippy warnings and format README #71

Fix clippy warnings and format README #71

kigawas commented Aug 15, 2019 •

edited

Loading

kigawas commented Aug 15, 2019

omershlo commented Aug 15, 2019

kigawas commented Aug 15, 2019

omershlo commented Aug 15, 2019

kigawas commented Aug 16, 2019

kigawas commented Sep 22, 2019

kigawas commented Sep 22, 2019

kigawas commented Sep 22, 2019

kigawas commented Sep 22, 2019

omershlo commented Sep 22, 2019 •

edited

Loading

kigawas commented Sep 22, 2019

kigawas commented Sep 22, 2019 •

edited

Loading

oleiba Sep 22, 2019

kigawas Sep 22, 2019

kigawas Sep 22, 2019

oleiba Sep 23, 2019 •

edited

Loading

kigawas Sep 23, 2019 •

edited

Loading

kigawas Sep 23, 2019

kigawas Sep 23, 2019 •

edited

Loading

oleiba Sep 22, 2019 •

edited

Loading

kigawas Sep 22, 2019

oleiba Sep 23, 2019

kigawas Sep 23, 2019 •

edited

Loading

kigawas Sep 23, 2019 •

edited

Loading

kigawas Sep 23, 2019

kigawas Sep 23, 2019 •

edited

Loading

oleiba left a comment

omershlo commented Sep 22, 2019

kigawas commented Sep 22, 2019

omershlo commented Oct 26, 2019

omershlo Oct 31, 2019

omershlo Oct 31, 2019

omershlo commented Oct 31, 2019

omershlo commented Oct 31, 2019

Fix clippy warnings and format README #71

Fix clippy warnings and format README #71

Conversation

kigawas commented Aug 15, 2019 • edited Loading

kigawas commented Aug 15, 2019

omershlo commented Aug 15, 2019

kigawas commented Aug 15, 2019

omershlo commented Aug 15, 2019

kigawas commented Aug 16, 2019

kigawas commented Sep 22, 2019

kigawas commented Sep 22, 2019

kigawas commented Sep 22, 2019

kigawas commented Sep 22, 2019

omershlo commented Sep 22, 2019 • edited Loading

kigawas commented Sep 22, 2019

kigawas commented Sep 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleiba Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

kigawas Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kigawas Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

oleiba Sep 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kigawas Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

kigawas Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kigawas Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

oleiba left a comment

Choose a reason for hiding this comment

omershlo commented Sep 22, 2019

kigawas commented Sep 22, 2019

omershlo commented Oct 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omershlo commented Oct 31, 2019

omershlo commented Oct 31, 2019

kigawas commented Aug 15, 2019 •

edited

Loading

omershlo commented Sep 22, 2019 •

edited

Loading

kigawas commented Sep 22, 2019 •

edited

Loading

oleiba Sep 23, 2019 •

edited

Loading

kigawas Sep 23, 2019 •

edited

Loading

kigawas Sep 23, 2019 •

edited

Loading

oleiba Sep 22, 2019 •

edited

Loading

kigawas Sep 23, 2019 •

edited

Loading

kigawas Sep 23, 2019 •

edited

Loading

kigawas Sep 23, 2019 •

edited

Loading