Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Secp256k1Point bincode deserialization error #60

Open
kigawas opened this issue Sep 2, 2019 · 10 comments
Open

Secp256k1Point bincode deserialization error #60

kigawas opened this issue Sep 2, 2019 · 10 comments
Labels
help wanted Extra attention is needed

Comments

@kigawas
Copy link
Contributor

kigawas commented Sep 2, 2019

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 omershlo added bug Something isn't working and removed bug Something isn't working labels Sep 2, 2019
@omershlo
Copy link
Contributor

omershlo commented Sep 2, 2019

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
Copy link
Contributor Author

kigawas commented Sep 2, 2019

Why can't I use bincode's serialize_struct?

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

@omershlo omershlo added the help wanted Extra attention is needed label Sep 2, 2019
@omershlo
Copy link
Contributor

omershlo commented Sep 2, 2019

I don't know :(

@djc
Copy link
Contributor

djc commented Sep 5, 2019

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
Copy link
Contributor

omershlo commented Sep 5, 2019

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
Copy link
Contributor

djc commented Sep 5, 2019

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
Copy link
Contributor

omershlo commented Sep 5, 2019

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
Copy link
Contributor

omershlo commented Sep 5, 2019

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

@djc
Copy link
Contributor

djc commented Sep 5, 2019

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants