Update to curv@0.9

[tmpfs]

Hi @survived, I am trying to get everything correct with version numbers but am not sure what is wrong as I seem to be including two versions of curv right now which is giving me the wrong compiler errors.

When I run cargo tree -i curv-kzen I get:

  curv-kzen:0.8.2
  curv-kzen:0.9.0

And cargo tree -i curv-kzen:0.8.2 yields:

curv-kzen v0.8.2
├── kzen-paillier v0.4.1
│   └── zk-paillier v0.4.2 (https://github.com/ZenGo-X/zk-paillier?tag=v0.4.2#00dfebc2)
│       └── multi-party-ecdsa v0.7.3 (/home/muji/git/consensys/multi-party-ecdsa)
└── multi-party-ecdsa v0.7.3 (/home/muji/git/consensys/multi-party-ecdsa)

I am pulling zk-paillier from the new v0.4.2 branch which depends on kzen-paillier using 0.4 so it should be pulling down 0.4.2 which depends on curv-kzen@0.9 but instead it is using the old 0.4.1 version.

Any idea what I am doing wrong here?

[tmpfs]

Phew, I figured it out, I needed to remove the lock file!

Now on to fixing the API errors :+1:

[tmpfs]

Hi @survived, hit a snag with this refactor. It seems that ECPoint is not implemented for Point::<Secp256k1> but I think it should be.

These are the relevant compiler error(s):

error[E0277]: the trait bound `Point<Secp256k1>: ECPoint` is not satisfied
   --> src/protocols/multi_party_ecdsa/gg_2020/party_i.rs:165:6
    |
59  | pub struct Keys<P = Point::<Secp256k1>>
    |            ---- required by a bound in this
60  | where
61  |     P: ECPoint,
    |        ------- required by this bound in `gg_2020::party_i::Keys`
...
165 | impl Keys {
    |      ^^^^ the trait `ECPoint` is not implemented for `Point<Secp256k1>`

error[E0277]: the trait bound `Point<Secp256k1>: ECPoint` is not satisfied
  --> src/protocols/multi_party_ecdsa/gg_2020/state_machine/keygen/rounds.rs:56:11
   |
56 |     keys: Keys,
   |           ^^^^ the trait `ECPoint` is not implemented for `Point<Secp256k1>`
   |
  ::: src/protocols/multi_party_ecdsa/gg_2020/party_i.rs:59:12
   |
59 | pub struct Keys<P = Point::<Secp256k1>>
   |            ---- required by a bound in this
60 | where
61 |     P: ECPoint,
   |        ------- required by this bound in `gg_2020::party_i::Keys`

And this is the source code where the problem lies:

#[derive(Derivative, Serialize, Deserialize)]
#[derivative(Clone(bound = "P: Clone, P::Scalar: Clone"))]
#[derivative(Debug(bound = "P: Debug, P::Scalar: Debug"))]
pub struct Keys<P = Point::<Secp256k1>>
where
    P: ECPoint,
{
    pub u_i: P::Scalar,
    pub y_i: P,
    pub dk: DecryptionKey,
    pub ek: EncryptionKey,
    pub party_index: usize,
    pub N_tilde: BigInt,
    pub h1: BigInt,
    pub h2: BigInt,
    pub xhi: BigInt,
    pub xhi_inv: BigInt,
}

I don't understand yet what the Derivative attribute macro is doing but it means we cannot quickly switch out the ECPoint trait for concrete types to try to move past this compiler error.

Looking in curv I see that ECPoint is implemented for Secp256k1Point which I believe is now a deprecated API that has been replaced by Point::<Curve>.

I am assuming the correct approach to this problem would be to implement ECPoint for Point::<Secp256k1>, is that correct?

[survived]

Hi @tmpfs, I see the confusion — ECPoint is not meant to be used directly anymore. We use Point<E>, Scalar<E> where E implements Curve trait. So, Keys should be rewritten in this way to be generic over choice of curve:

#[derive(Serialize, Deserialize, Clone, Debug)]
#[serde(bounds = "")]
pub struct Keys<E: Curve>
where
    P: ECPoint,
{
    pub u_i: Scalar<E>,
    pub y_i: Point<E>,
    pub dk: DecryptionKey,
    pub ek: EncryptionKey,
    pub party_index: usize,
    pub N_tilde: BigInt,
    pub h1: BigInt,
    pub h2: BigInt,
    pub xhi: BigInt,
    pub xhi_inv: BigInt,
}

I'd highlight two changes: a. You can see that derivative is not needed anymore, it can be replaced with regular derive(Clone, Debug) since any Point<E>/Scalar<E> implement Clone+Debug. b. We have to add #[serde(bounds = "")] line to help serde properly derive (de)serialization traits implementation (it basically says that (de)serialization is defined for any E)

After you update the Keys<E> struct, you can choose exact curve by specifying generic E, e.g.: Keys<Secp256k1>.

Documentation has a simple example of writing the code generic over choice of curve, check out Diffie-Hellman example

[survived]

@tmpfs are you aware of that your PR is not based on latest master? It's actually based on master~2. But I think it should be easy to rebase

[tmpfs]

@tmpfs are you aware of that your PR is not based on latest master? It's actually based on master~2. But I think it should be easy to rebase

Hmm, not sure what I did wrong there but master merged successfully :+1:

[tmpfs]

@survived, not quite there with typing Keys correctly yet, using Curve as the type parameter fails in object safety due to the PartialEq supertrait (I believe).

I tried this:

#[derive(Serialize, Deserialize, Clone, Debug)]
pub struct Keys<E = Curve> {
    pub u_i: Scalar::<E>,
    pub y_i: Point::<E>,
    pub dk: DecryptionKey,
    pub ek: EncryptionKey,
    pub party_index: usize,
    pub N_tilde: BigInt,
    pub h1: BigInt,
    pub h2: BigInt,
    pub xhi: BigInt,
    pub xhi_inv: BigInt,
}

And the compiler complains:

error[E0038]: the trait `Curve` cannot be made into an object
  --> src/protocols/multi_party_ecdsa/gg_2020/party_i.rs:57:21
   |
57 | pub struct Keys<E = Curve> {
   |                     ^^^^^ `Curve` cannot be made into an object
   |
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
  --> /home/muji/.cargo/registry/src/github.com-1ecc6299db9ec823/curv-kzen-0.9.0/src/elliptic/curves/traits.rs:20:18
   |
20 | pub trait Curve: PartialEq + Clone + fmt::Debug + 'static {
   |                  ^^^^^^^^^ the trait cannot be made into an object because it uses `Self` as a type parameter

The PartialEq trait uses Self as a type parameter:

pub trait PartialEq<Rhs = Self> where
    Rhs: ?Sized, { ... }

Any ideas on how to fix this?

[tmpfs]

@survived, so i can move on to the next batch of errors I am just using the concrete type for now and removing the Curve type parameter:

#[derive(Serialize, Deserialize, Clone, Debug)]
pub struct Keys {
    pub u_i: Scalar::<Secp256k1>,
    pub y_i: Point::<Secp256k1>,
    pub dk: DecryptionKey,
    pub ek: EncryptionKey,
    pub party_index: usize,
    pub N_tilde: BigInt,
    pub h1: BigInt,
    pub h2: BigInt,
    pub xhi: BigInt,
    pub xhi_inv: BigInt,
}

Is that ok for now?

[tmpfs]

@survived, any advice on the best way to handle the signature change on VerifiableSS::share(), previously it returned a Vec<Scalar<E>> and now it is the SecretShares<E> type.

In particular, because it is no longer a Vec I am not sure how to handle lines like this:

https://github.com/tmpfs/multi-party-ecdsa/blob/master/src/protocols/multi_party_ecdsa/gg_2018/test.rs#L96

[survived]

Is that ok for now?

Yeah, it's okay for now. I'll change it later to be generic over choice of curve — our downstream lib requires this.

In particular, because it is no longer a Vec

It derefs to a slice. Particularly, you can use .to_vec() if you need a vec, eg secrets.to_vec()

[tmpfs]

It derefs to a slice. Particularly, you can use .to_vec() if you need a vec, eg secrets.to_vec()

Oops, I should have noticed that!

Yeah, it's okay for now. I'll change it later to be generic over choice of curve — our downstream lib requires this.

Happy to circle back to this later and implement it under your guidance once we have the basic curv upgrade completed, thanks :+1:

[tmpfs]

@survived, how should I call zeroize() on a Scalar now?

I am getting lots of these errors:

error[E0599]: the method `zeroize` exists for struct `Scalar<Secp256k1>`, but its trait bounds were not satisfied
   --> src/protocols/two_party_ecdsa/lindell_2017/party_one.rs:165:22
    |
165 |         secret_share.zeroize();
    |                      ^^^^^^^ method cannot be called on `Scalar<Secp256k1>` due to unsatisfied trait bounds
    |
   ::: /home/muji/.cargo/registry/src/github.com-1ecc6299db9ec823/curv-kzen-0.9.0/src/elliptic/curves/wrappers/scalar.rs:36:1
    |
36  | pub struct Scalar<E: Curve> {
    | ---------------------------
    | |
    | doesn't satisfy `Scalar<Secp256k1>: DefaultIsZeroes`
    | doesn't satisfy `Scalar<Secp256k1>: Zeroize`
    |
    = note: the following trait bounds were not satisfied:
            `Scalar<Secp256k1>: DefaultIsZeroes`
            which is required by `Scalar<Secp256k1>: Zeroize`

The zeroize::Zeroize trait is in scope.

[survived]

@tmpfs, just delete .zeroize() calls on scalars! Scalars are zeroed out on drop by default since curv@0.8.

[tmpfs]

@survived, it is compiling now :+1: I will move on to fixing the benchmarks, examples and any tests next.

[tmpfs]

@survived, I believe based on the information here that the changes for pk_to_key_slice() are correct, please verify :pray:

[tmpfs]

Ok, so the benchmarks are running ok and the tests/examples are compiling but I haven't checked running the examples yet as there are two failing tests:

• protocols::multi_party_ecdsa::gg_2018::test::test_sign_n5_t2_ttag4
• protocols::multi_party_ecdsa::gg_2018::test::test_sign_n8_t4_ttag6

I will try to figure out my error tomorrow! @survived, if you have a moment if would be great if you could take a look and see if anything jumps out at you that would cause the tests to fail! Thanks :pray:

[survived]

Sure, @tmpfs, I will take a look too

[tmpfs]

FYI, the error causing the failing tests is originating from the phase5d() function here: https://github.com/tmpfs/multi-party-ecdsa/blob/curv-0.8/src/protocols/multi_party_ecdsa/gg_2018/party_i.rs#L714.

[survived]

@tmpfs gg18 tests were failing because of this 🙂

[tmpfs]

@tmpfs gg18 tests were failing because of this slightly_smiling_face

Great catch @survived, what was I thinking adding those parentheses :disappointed:

[tmpfs]

I will have a go at resolving the conflicts now!

[tmpfs]

@survived, I had completely removed zeroize as it was redundant but after rebasing against master it was introduced again, I have removed it again under the assumption that BigInt also calls zero() on drop - correct me if I am wrong here please!

[tmpfs]

@survived, rebased and fixed the compiler errors so I think that if my assumption about removing the zeroize attributes from the new src/utilities/mta/range_proof.rs file are correct then this is ready for review :+1:

[tmpfs]

Oh, i still need to make Keys and LocalKeys generic over Curve too but now is probably a good time to review everything so far!

[tmpfs]

@survived, I have now updated Keys and LocalKey to be generic over Curve so I think this is ready for review/merge when you have time, thanks :+1:

[DmytroTym]

@survived, I had completely removed zeroize as it was redundant but after rebasing against master it was introduced again, I have removed it again under the assumption that BigInt also calls zero() on drop - correct me if I am wrong here please!

I don't think this is the case tbh. From https://github.com/ZenGo-X/curv/blob/master/src/arithmetic/big_gmp.rs I get the impression that while zeroize is implemented for BigInt, it is not called on drop.

[DmytroTym]

I don't think this is the case tbh. From https://github.com/ZenGo-X/curv/blob/master/src/arithmetic/big_gmp.rs I get the impression that while zeroize is implemented for BigInt, it is not called on drop.

That's incorrect, if you're using gmp backend, BigInts are actually zeroized on drop. num-bigint backend however does not support zeroization on drop atm, but hopefully will very soon.

[tmpfs]

Thanks @DmytroTym for the information. I will restore the Zeroize attributes and dependency and raise a separate issue to remove them once num-bigint supports zeroization on drop.

[tmpfs]

@survived, I finished fixing all the clippy warnings. I wasn't sure the preferred approach for using the unit type as Error but figured that using custom errors is better than ignoring those clippy warnings so I hope that is ok :pray:

[DmytroTym]

Hello! Does the code from this PR compile for everyone? One error that I get is from gg18_sign_client. Here a usize is passed into SignKeys::create method, that actually expects a u16.

[survived]

@DmytroTym, sound like it's me who introduced this error. I'll fix the examples, thanks!

[survived]

@DmytroTym, fixed!

[DmytroTym]

Hey @tmpfs and @survived. I fixed one line that prevented benchmarks from running, but otherwise imo this PR is pretty safe to merge. After all, it does not change any logic and I think that compiler and tests should have done a good job of preventing any accidental changes that would break something. Plus two pair of eyes looked at the code. @tmpfs, as to this issue: https://github.com/ZenGo-X/multi-party-ecdsa/issues/148. As far as I understand, any efforts of zeroizing things would have to be in a new major version of curv as this PR: https://github.com/ZenGo-X/curv/pull/154 introduces breaking changes? Anyway, properly zeroizing num-bigint values would be tricky and probably won't be properly implemented soon. @survived please correct me if I'm wrong here. I'm not sure what should we do here, probably just give up on zeroizing num-bigint?

[tmpfs]

Thanks @DmytroTym - would be great to land this :+1: Maybe just go ahead and close #148 if we are unlikely to add zeroizing in num-bigint.

[DmytroTym]

@tmpfs I cannot really close the issue, we should wait for @survived :) Meanwhile, could you please merge in https://github.com/tmpfs/multi-party-ecdsa/pull/1 so that benchmarks can run?