dfns / cggmp21

State-of-art threshold ECDSA in Rust
Apache License 2.0
41 stars 6 forks source link

Bincode: Cannot serialize IncompleteKeyShare<E> #89

Closed tbraun96 closed 3 months ago

tbraun96 commented 3 months ago

When running this function, serialization using bincode2 fails:

pub async fn run_and_serialize_keygen<
    'r,
    E: Curve,
    S: SecurityLevel,
    H: Digest<OutputSize = U32> + Clone + Send + 'static,
    D,
    R,
>(
    tracer: &mut PerfProfiler,
    eid: dfns_cggmp21::ExecutionId<'r>,
    i: u16,
    n: u16,
    t: u16,
    party: MpcParty<Msg<E, S, H>, D>,
    mut rng: R,
) -> Result<Vec<u8>, JobError>
where
    D: Delivery<Msg<E, S, H>>,
    R: RngCore + CryptoRng,
{
    let builder = KeygenBuilder::<E, S, H>::new(eid, i, n);
    let incomplete_key_share = builder
        .set_progress_tracer(tracer)
        .set_threshold(t)
        .start(&mut rng, party)
        .await
        .map_err(|err| JobError {
            reason: format!("Keygen protocol error (run_and_serialize_keygen): {err:?}"),
        })?;
    bincode2::serialize(&incomplete_key_share).map_err(|err| JobError {
        reason: format!("Keygen protocol error (run_and_serialize_keygen - serde): {err:?}"),
    })
}

The internal implementation for Serialize does not work.

Using the latest version of main/master branch.

drewstone commented 3 months ago

With bincode too it persists. I've reproduced it in the repo in one of the tests: https://github.com/dfns/cggmp21/pull/90

survived commented 3 months ago

It seems that this happens bacause bincode isn't compatible with #[serde(flatten)] tag. For instance, this also fails to serialize:

#[derive(serde::Serialize, serde::Deserialize)]
struct Foo {
    i: u16,
    #[serde(flatten)]
    bar: Bar,
    x: u32,
}

#[derive(serde::Serialize, serde::Deserialize)]
struct Bar {
    thing: String,
    list: Vec<u8>,
}

#[test]
fn this_fails() {
    let foo = Foo {
        i: 1,
        bar: Bar {
            thing: "123".into(),
            list: vec![1, 2, 3],
        },
        x: 7,
    };

    bincode::serialize(&foo).unwrap();
}

In the last release, we have introduced some changes in key share struct, so we had to add #[serde(flatten)] in order to be able to deserialize the key shares generated in previous versions of the library. Unfortunately it broke compatibility with bincode, and I don't think we can fix that.

We haven't mentioned it in the doc, but we actually wouldn't advise using bincode or similar formats with our library at all. If I'm not mistaken, bincode doesn't serialize field names, which makes it more difficult to add new fields as the library grows. Between the versions, we try to keep the serialization format compatible, but that requires the serialization backend to meet certain requirements, e.g. serialize field names. I will update the docs to reflect these requirements.

Among binary formats, feel free to use ciborium. It achieves smaller size, but still have some redundancy so we can freely update the key share type. If you need the serialized key share to be as small as possible, then I'd recommend serializing the key share manually: all its fields are public.

If you were using bincode to serialize the key shares before the update, you'll need to follow migration process as below:

  1. Import previous version of cggmp21 library, in cargo toml:
    cggmp21-v01 = { package = "cggmp21", version = "0.1" }
  2. Deserialize old key shares as cggmp21_v01::key_share::IncompleteKeyShare and manually map onto cggmp21::key_share::IncompleteKeyShare (newest version)
  3. Then serialize the key share with any compatible serde backend
survived commented 3 months ago

Turned out that using #[serde(flatten)] has broken serialization with CBOR due to unrelated reason, so we removed usage of this attribute in #93. Bincode should work again. However, as I noted above, bincode usage is still not advised. It may get broken again. It may not be backwards-compatible between the versions of the library.