SecretStore: threshold ECDSA PoC #7615

svyatonik · 2018-01-18T10:50:17Z

It is a test of crypto-math behind possible implementation of ECDSA-compatible SS session.

The only known limitation is that it requires 2 * t <= N. I.e. it isn't possible to make ECDSA signature when key was generated, for example 4-of-6 session.
Workaround is to temporarily (for the duration of this session) decrease key threshold (see #7562 for PoC). But we need to agree on this (it is a part of #7271 discussion, actually).

So if this PR will be merged before #7271 is reviewed, I'll start adding ECDSA session without automatic threshold decreasing && wait until this solution will be reviewed.

svyatonik · 2018-01-18T10:50:34Z

ethkey/src/secret.rs

@@ -48,6 +48,11 @@ impl Secret {
 		Secret { inner: h }
 	}

+	/// Creates zero key, which is invalid for crypto operations, but valid for math operation.
+	pub fn zero() -> Self {
+		Secret { inner: Default::default() }


Didn't wanted to implement Default for Secret, since it actually produces invalid Secret.

svyatonik · 2018-01-18T10:50:46Z

ethkey/src/secret.rs

-		let mut key_secret = self.to_secp256k1_secret()?;
-		let other_secret = other.to_secp256k1_secret()?;
-		key_secret.add_assign(&SECP256K1, &other_secret)?;
+		match (self.is_zero(), other.is_zero()) {


The reason behind these changes is that starting with this PR, I need to handle 0 EC scalars.
0 is the invalid key in secp256k1 && library will return an error when trying to work with it.
Long story short: it wasn't a best idea to use Secret for EC arithmetic (the initial assumption was that it will minimize U256 <-> Secret conversions).
And the general solution is either to find&use/create separate EC-arithmetic library.
If you're strong against these changes - please comment && I'll close this PR && will switch to separate type for EC calculations first.

not strongly against these changes, but down the road it could be nice to have a separate type for secp256k1 scalars which has std::ops traits implemented for it.

rphmeier · 2018-01-19T18:53:33Z

secret_store/src/key_server_cluster/math.rs

+	fn run_reciprocal_protocol(t: usize, artifacts: &KeyGenerationArtifacts) -> Vec<Secret> {
+		// === Given a secret x mod r which is shared among n players, it is
+		// === required to generate shares of 1/x mod r with out revealing
+		// === any information about x or 1/x.


nit: can we frame that as a multiplicative inverse instead of 1/x?

Sure :) It is just easier to write 1/x instead of multiplicative inverse of x. Did inv(x) sounds good, or it is better to write its 'full name'?

inv(x) is fine for me

rphmeier · 2018-01-19T18:59:05Z

secret_store/src/key_server_cluster/math.rs

+		if let Some(mut secret_required) = secret_required {
+			for polynom1 in polynoms1.iter_mut().take(n - 1) {
+				let secret_coeff1 = generate_random_scalar().unwrap();
+				secret_required.sub(&secret_coeff1).unwrap();


is that tested to wrap around?

Thanks for reviewing! Yes - it will wrap around in case of underflow. Please see run_zero_key_generation - 0 is passed here as secret_required => any non-zero value (which is generated by generate_random_scalar) will cause underflow => yes it is tested.

rphmeier · 2018-01-19T19:46:37Z

Did an initial review of the code and it looks fine. I'll dive into the math tomorrow.
Just wondering if you've seen this paper which I was linked to recently: https://eprint.iacr.org/2016/013.pdf as it's newer than the one linked in the PR and seems to get rid of the n >= 2t+1 requirement.

svyatonik · 2018-01-19T20:06:26Z

@rphmeier Thanks for the link!!! I'll read this paper on the weekend, but iirc last time I was searching for threshold-optimal ECDSA (this was ~2 months ago), every paper had its own 'issue' - like one was strictly about 2-of-2 scheme, another required 2 * t nodes to recover the key, another was about using different key generation scheme (secret = multiplication of shares, instead of addition [that's what we currently use]), another was about 'combinatorics threshold' (i.e. generate t-of-n shares for each possible combination of nodes). I'll mark PR as gotissues until I'm done with reading.

svyatonik · 2018-01-22T04:54:49Z

So the key generation scheme in this paper differs from what we currently use (at first glance). Since the most of SS functionality is based on current KG scheme (like Enc/Dec + migration), I'll opt for using proposed (in this PR) signature generation prototype now (I was asked several times for this feature) + detailed review (and possibly including it to the SS [TODO for myself: research if shares migration is possible in this scheme]) of your paper later.

svyatonik · 2018-01-25T10:39:59Z

Found some issue with serialize_ecdsa_signature

svyatonik · 2018-01-25T12:29:39Z

Turned out to be issue with nonce_public calculation - fixed it. I've also:

extracted some parts of tests into separate functions, because I'll need these for ECDSA session implementation;
added tests for different key sharing schemes;
added random message hash generation to the test (previously it always was less than EC order).

debris · 2018-02-07T08:53:23Z

@rphmeier could you review this pr again? I don't feel qualified to review it without diving into math

rphmeier · 2018-02-15T00:44:07Z

secret_store/src/key_server_cluster/math.rs

+	let scalar: U256 = hash.into();
+	let scalar: H256 = (scalar % math::curve_order()).into();
+	let scalar = Secret::from_slice(&*scalar);
+	scalar.check_validity()?;


any secret in 0..curve_order is valid though, right?

yes. except for 0 itself

rphmeier · 2018-02-15T00:45:32Z

secret_store/src/key_server_cluster/math.rs

+
+/// Compute R part of ECDSA signature.
+#[cfg(test)]
+pub fn compute_ecdsa_r(nonce_public: &Public) -> Result<Secret, Error> {


I think this is better explained as x_coordinate(point) than as "r", which requires diving a bit more into implementations of ecdsa

I've added public_x and public_y (and used public_x in this function), but left compute_ecdsa_r - it is easier to think of R and S parts in ECDSA session (next PR). I.e. leave the fact that R is actually a point.x behind 'math-wall'.

rphmeier · 2018-02-15T00:46:18Z

secret_store/src/key_server_cluster/math.rs

+	let mut signature_v = {
+		let nonce_public_x = &nonce_public[0..32];
+		let nonce_public_x: U256 = nonce_public_x.into();
+		let nonce_public_x: H256 = nonce_public_x.into();


let nonce_public_x = H256::from_slice(&nonce_public[0..32])

rphmeier

haven't dived too deep into the math, but the logic around interpolation of inv(nonce) and the s value seems fine

rphmeier · 2018-02-15T01:49:09Z

secret_store/src/key_server_cluster/math.rs

+	debug_assert!(id_numbers.len() >= double_t + 1);
+	debug_assert_eq!(signature_s_shares.len(), id_numbers.len());
+
+	compute_joint_secret_from_shares(double_t,


This interpolation doesn't seem to be possible with only t participants (as it's polluted by the shares of the z blinding factor).

Sorry - don't get it - whether this is an issue, or a question :) Interpolation is not possible with t+1 participants - yes. iirc, the reason is that two t-degree polynoms are multiplied when inv(nonce) is calculated => it requires 2*t+1 points to interpolate.

yes, but if running compute_ecdsa_s is part of a t-of-n scheme then I don't understand how it could ever complete (relevant to my other comment)

or have I got it backwards somehow, and it's actually a 2t+1-of-n scheme because we have to interpolate such polynomials?

signature_s_shares are the shares of S in (2*t+1)-of-N scheme. So what compute_ecdsa_s does is exactly interpolating 2*t-degree polynomial.

rphmeier · 2018-02-15T01:50:52Z

secret_store/src/key_server_cluster/math.rs

+			let mul_shares = run_multiplication_protocol(t, &artifacts.secret_shares, &nonce_inv_shares);
+
+			// compute shares for s portion of signature: nonce_inv * (message_hash + secret * signature_r)
+			// every node broadcasts this share


if each node's participation (or at least 2t + 1) is required to interpolate the polynomial here, then can it really be considered a t-of-n scheme?

Replaced with 2*t+1 (it is actually compute_ecdsa_s who take care of this, but it might be confusing to see n in this test)

svyatonik added 6 commits January 18, 2018 10:08

SecretStore: ECDSA PoC

6ded2e3

SecretStore: fixed ECDSA serialization + cleanup

d31bc35

Merge branch 'master' into secretstore_ecdsa_poc

ae1eeab

removed unused param

0aa6dfd

removed unused method

5ddca3b

removed debug unwrap

d7ed776

svyatonik added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jan 18, 2018

svyatonik commented Jan 18, 2018

View reviewed changes

5chdn added this to the 1.10 milestone Jan 18, 2018

rphmeier reviewed Jan 19, 2018

View reviewed changes

svyatonik added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 19, 2018

1/x -> inv(x)

bf225bb

svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Jan 22, 2018

5chdn modified the milestones: 1.10, 1.11 Jan 23, 2018

svyatonik added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 25, 2018

SecretStore: merged fixes from ECDSA session branch

93f1555

svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Jan 25, 2018

once again 1/* -> inv(*)

3ca79ce

svyatonik mentioned this pull request Jan 30, 2018

SecretStore: ECDSA session for cases when 2*t < N #7739

Merged

rphmeier reviewed Feb 15, 2018

View reviewed changes

rphmeier approved these changes Feb 15, 2018

View reviewed changes

rphmeier reviewed Feb 15, 2018

View reviewed changes

svyatonik added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 15, 2018

fixed grumbles

33b0e45

svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Feb 15, 2018

debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 15, 2018

debris merged commit 37bfcb7 into master Feb 15, 2018

debris deleted the secretstore_ecdsa_poc branch February 15, 2018 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SecretStore: threshold ECDSA PoC #7615

SecretStore: threshold ECDSA PoC #7615

svyatonik commented Jan 18, 2018

svyatonik Jan 18, 2018

svyatonik Jan 18, 2018 •

edited

Loading

rphmeier Feb 15, 2018

rphmeier Jan 19, 2018

svyatonik Jan 19, 2018

rphmeier Jan 19, 2018

rphmeier Jan 19, 2018

svyatonik Jan 19, 2018

rphmeier commented Jan 19, 2018 •

edited

Loading

svyatonik commented Jan 19, 2018

svyatonik commented Jan 22, 2018 •

edited

Loading

svyatonik commented Jan 25, 2018

svyatonik commented Jan 25, 2018

debris commented Feb 7, 2018

rphmeier Feb 15, 2018

svyatonik Feb 15, 2018

rphmeier Feb 15, 2018

svyatonik Feb 15, 2018

rphmeier Feb 15, 2018

rphmeier left a comment •

edited

Loading

rphmeier Feb 15, 2018

svyatonik Feb 15, 2018

rphmeier Feb 15, 2018

rphmeier Feb 15, 2018 •

edited

Loading

svyatonik Feb 15, 2018

rphmeier Feb 15, 2018 •

edited

Loading

svyatonik Feb 15, 2018

SecretStore: threshold ECDSA PoC #7615

SecretStore: threshold ECDSA PoC #7615

Conversation

svyatonik commented Jan 18, 2018

Choose a reason for hiding this comment

svyatonik Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier commented Jan 19, 2018 • edited Loading

svyatonik commented Jan 19, 2018

svyatonik commented Jan 22, 2018 • edited Loading

svyatonik commented Jan 25, 2018

svyatonik commented Jan 25, 2018

debris commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svyatonik Jan 18, 2018 •

edited

Loading

rphmeier commented Jan 19, 2018 •

edited

Loading

svyatonik commented Jan 22, 2018 •

edited

Loading

rphmeier left a comment •

edited

Loading

rphmeier Feb 15, 2018 •

edited

Loading

rphmeier Feb 15, 2018 •

edited

Loading