Update to curv@0.9

[tmpfs]

No description provided.

[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 👍

[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

[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 🙏

[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: #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: ZenGo-X/curv#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 👍 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 tmpfs#1 so that benchmarks can run?