ZenGo-X / curv

Rust language general purpose elliptic curve cryptography.
MIT License
264 stars 111 forks source link

Suboptimal serialization of certain structs #138

Closed DmytroTym closed 2 years ago

DmytroTym commented 2 years ago

Serialization of BigInt with gmp backend is autoimplemented and turns out to use roughly 2.4 times more space than needed, and for Secp256k1's Points and Scalars it's 2 times more. Moreover, Points are serialized in uncompressed format. This leads to overheads in memory usage when transmitting protocol messages, built from aforementioned types. Somewhat related to https://github.com/ZenGo-X/curv/issues/118

survived commented 2 years ago

On which version of curv you encountered this problem? Latest curv v0.8.0-rc3 serializes points in compressed form (including secp256k1)

DmytroTym commented 2 years ago

It's v0.7 used by multi-party-ecdsa. We tried using v0.8.0-rc3 instead, but It seems there are some conflicts:

Updating crates.io index error: failed to select a version for signature. ... required by package ecdsa v0.12.1 ... which is depended on by p256 v0.9.0 ... which is depended on by curv-kzen v0.8.0-rc3 ... which is depended on by multi-party-ecdsa v0.7.3 versions that meet the requirements >=1.3.0, <1.4.0 are: 1.3.1, 1.3.0

all possible versions conflict with previously selected packages.

previously selected package signature v1.2.2 ... which is depended on by ecdsa v0.8.0 ... which is depended on by p256 v0.5.0 ... which is depended on by curv-kzen v0.7.1 ... which is depended on by bulletproof v1.1.6 (https://github.com/KZen-networks/bulletproofs?tag=v1.1.6#39e94f7b) ... which is depended on by centipede v0.2.12 (https://github.com/KZen-networks/centipede?tag=v0.2.12#d426710e) ... which is depended on by multi-party-ecdsa v0.7.3

nskybytskyi commented 2 years ago

@survived I've got one more question related to this issue. Secp256k1Scalar in v0.7.3 used to implement serde::Serialize, but it was inefficient, as already noted by @DmytroTym

Now, curv v0.8.0-rc3 implements efficient serialization to bytes, but it does it via the .serialize() method instead. Can I ask what is the reasoning behind this decision?

It appears to me that the previous design was handier for users. Specifically, it is much easier to derive serde::Serialize for custom structures like

#[derive(Serialize)]
struct OhWowSuchStructMuchFE {
    doge: FE,
    bonk: FE,
}

than to implement it by hand for every such struct.

However, this no longer works with curv v0.8.0-rc3 😢

survived commented 2 years ago

We still have serde::{Serialize, Deserialize} implemented for points/scalars. Can be checked in the documenation, see: point serialization, deserialization (the same for scalars). Serialization/deserialization traits can be derived in this way:

use curv::elliptic::curves::{Scalar, Secp256k1};

#[derive(Serialize, Deserialize)]
struct OhWowSuchStructMuchFE {
    doge: Scalar<Secp256k1>,
    bonk: Scalar<Secp256k1>,
}

If your struct is generic over choice of curve, then you need to use this trick:

use curv::elliptic::curves::{Scalar, Curve};

#[derive(Serialize, Deserialize)]
#[serde(bound="")]
struct OhWowSuchStructMuchFE<E: Curve> {
    doge: Scalar<E>,
    bonk: Scalar<E>,
}

This little line #[serde(bound="")] does all the magic. It's required because serde, by default, tries to bound generic params to be (de)serializable (ie. it puts E: Serialize + Deserialize), and this little line disables this behaviour.