ZenGo-X / curv

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

Secp256k1Point bincode deserialization error #60

Open kigawas opened 4 years ago

kigawas commented 4 years ago
use bincode;
use curv::GE;

fn main() {
    let p = GE::random_point();
    let encoded = bincode::serialize(&p).unwrap();
    let decoded: GE = bincode::deserialize(encoded.as_slice()).unwrap();
}
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Custom("invalid type: sequence, expected Secp256k1Point")', src/libcore/result.rs:1084:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[package]
name = "test"
version = "0.1.0"
edition = "2018"

[dependencies]
serde = "1.0"
bincode = "1.1"

[dependencies.curv]
git = "https://github.com/KZen-networks/curv"
tag = "v0.2.0"
features =  ["ec_secp256k1"]
omershlo commented 4 years ago

Secp256k1 is a struct with a string for the purpose of the point and the coordinates taken from secp256k1 library. Default Serde will not work here and therefore we implemented Deserialize / Serialize ourselves: https://github.com/KZen-networks/curv/blob/master/src/elliptic/curves/secp256_k1.rs#L534 please use this instead.

kigawas commented 4 years ago

Why can't I use bincode's serialize_struct?

https://github.com/servo/bincode/blob/master/src/ser/mod.rs#L190

omershlo commented 4 years ago

I don't know :(

djc commented 4 years ago

That's what this is intended to solve. However, testing it is painful at the moment because of how your crates use Git tags for versioning rather than having an actual crate registry (which would allow use of semver).

omershlo commented 4 years ago

Can you elaborate more on that point? Are you saying that uploading the libraries to crates.io would have make more sense in some sense ?

djc commented 4 years ago

Yes, uploading them to crates.io would definitely make things easier. However, I suppose at least in the case of curv and rust-paillier there are already crates on there. Maybe @mortendahl @mcornejo @kali would allow you to publish to crates.io/crates/paillier? curv is currently squatted on crates.io so it might have to be renamed somehow.

omershlo commented 4 years ago

I am not religious about naming. The only issue that stops us from uploading everything is Curv dependency on a specific commit of Zcash library to support the curve Jubjub. All the other libraries are dependent on Curv so they are stuck as well.

omershlo commented 4 years ago

We uploaded https://crates.io/crates/emerald-city/0.2.0 just because we removed the Zcash dependency

djc commented 4 years ago

Having to depend on a particular commit is in itself a bit suspicious (same with the rust-gmp fork). Would be nice to work with the zcash folks to see if some of their crates could get onto crates.io in a version that you can consume.

As it is, if you make a curv change you'll also have to immediately retag rust-paillier and zk-paillier again to prevent duplicates in downstream dependency graphs, so that sucks a bit. Maybe a custom registry could be a decent option? Either with https://blog.mgattozzi.dev/turning-github-into-your-own-registry/ or using Cloudsmith.

mortendahl commented 4 years ago

Maybe @mortendahl @mcornejo @kali would allow you to publish to crates.io/crates/paillier?

If you guys are still using rust-paillier then I'll make it a priority to get any existing and new PRs merged and published.