Merge pull request #93 from dfns/fix-serde

Fix serde issue

Author: survived

Cargo.lock

@@ -6,4 +6,3 @@ members = [
     "key-share",
     "tests",
 ]
-

Cargo.toml

@@ -1,11 +1,16 @@
 # Changelog
 
 ## v0.2.1
+* Fix key share (de)serialization issue [#93]
 * Add a notice about the serialization to key share docs [#91]
 
 [#91]: https://github.com/dfns/cggmp21/pull/91
+[#93]: https://github.com/dfns/cggmp21/pull/93
 
 ## v0.2.0
+**YANKED**: this release is yanked because it had an issue with key share (de)serialization
+that was addressed in v0.2.1
+
 * Add support of HD wallets compatible with BIP-32 and SLIP-10 [#68],
   [#74], [#75]
 * Prohibit key shares with zero secret share or secret key [#82]
@@ -16,5 +21,7 @@
 [#82]: https://github.com/dfns/cggmp21/pull/82
 
 ## v0.1.0
+**YANKED**: this release is yanked because it had an issue with key share (de)serialization
+that was addressed in v0.2.1
 
 Initial release

key-share/CHANGELOG.md

@@ -21,6 +21,8 @@ use core::ops;
 use generic_ec::{serde::CurveName, Curve, NonZero, Point, Scalar, SecretScalar};
 use generic_ec_zkp::polynomial::lagrange_coefficient;
 
+#[cfg(feature = "serde")]
+mod serde_fix;
 #[cfg(feature = "spof")]
 pub mod trusted_dealer;
 mod utils;
@@ -107,19 +109,81 @@ use serde_with::As;
 /// If you need the smallest size of serialized key share, we advise implementing serialization manually (all fields of
 /// the key share are public!).
 #[derive(Clone)]
-#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
-#[cfg_attr(feature = "serde", serde(bound = ""))]
 pub struct DirtyCoreKeyShare<E: Curve> {
     /// Index of local party in key generation protocol
     pub i: u16,
     /// Public key info
-    #[cfg_attr(feature = "serde", serde(flatten))]
     pub key_info: DirtyKeyInfo<E>,
     /// Secret share $x_i$
-    #[cfg_attr(feature = "serde", serde(with = "As::<generic_ec::serde::Compact>"))]
     pub x: NonZero<SecretScalar<E>>,
 }
 
+#[cfg(feature = "serde")]
+impl<E: Curve> serde::Serialize for DirtyCoreKeyShare<E> {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        // See [`crate::serde_fix`] module docs
+        let Self {
+            i,
+            key_info:
+                DirtyKeyInfo {
+                    curve,
+                    shared_public_key,
+                    public_shares,
+                    vss_setup,
+                    #[cfg(feature = "hd-wallets")]
+                    chain_code,
+                },
+            x,
+        } = &self;
+        serde_fix::ser::CoreKeyShare {
+            i,
+            curve,
+            shared_public_key,
+            public_shares,
+            vss_setup,
+            x,
+            #[cfg(feature = "hd-wallets")]
+            chain_code,
+        }
+        .serialize(serializer)
+    }
+}
+
+#[cfg(feature = "serde")]
+impl<'de, E: Curve> serde::Deserialize<'de> for DirtyCoreKeyShare<E> {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        // See [`crate::serde_fix`] module docs
+        let serde_fix::de::CoreKeyShare {
+            curve,
+            i,
+            shared_public_key,
+            public_shares,
+            vss_setup,
+            x,
+            #[cfg(feature = "hd-wallets")]
+            chain_code,
+        } = serde::Deserialize::deserialize(deserializer)?;
+        Ok(Self {
+            i,
+            key_info: DirtyKeyInfo {
+                curve,
+                shared_public_key,
+                public_shares,
+                vss_setup,
+                #[cfg(feature = "hd-wallets")]
+                chain_code,
+            },
+            x,
+        })
+    }
+}
+
 /// Public Key Info
 ///
 /// Contains public information about the TSS key, including shared public key, commitments to

key-share/src/lib.rs

@@ -0,0 +1,64 @@
+//! Fixes serde serialization of the key share
+//!
+//! This module contains structs that help us implementing `serde::{Serialize, Deserialize}`
+//! for [`crate::DirtyCoreKeyShare`].
+//!
+//! Context: we used to have key share struct flatten, with all DirtyKeyInfo fields being
+//! right in the key share. However, at some point, we've decided to move all public common
+//! fields into a separate structure. In order to keep serialization format compatible,
+//! we used `#[serde(flatten)]` attribute, to make on-wire data appear like all the fields
+//! are still in the same struct. It turned out, that `#[serde(flatten)]` is buggy, and,
+//! specifically, it does not preserve `is_human_readable` flag, which broke (de)serialization
+//! code and compatibility with old key shares.
+//!
+//! See the issue for more details on the `serde` problem: <https://github.com/serde-rs/serde/issues/2704>
+//!
+//! Until the issue in `serde` crate is addressed, we have to use a workaround in this module.
+//! We basically reimplement `flatten` attribute manually at the cost of extra allocations.
+
+use generic_ec::{serde::CurveName, Curve, NonZero, Point, SecretScalar};
+use serde_with::As;
+
+macro_rules! core_key_share {
+    ($($(#[$attr:meta])*pub $field:ident: $ty:ty),+$(,)?) => {
+        pub mod ser {
+            use super::*;
+
+            #[derive(serde::Serialize)]
+            #[serde(bound = "")]
+            pub struct CoreKeyShare<'a, E: Curve> {$(
+                $(#[$attr])*
+                pub $field: &'a $ty,
+            )+}
+        }
+        pub mod de {
+            use super::*;
+
+            #[derive(serde::Deserialize)]
+            #[serde(bound = "")]
+            pub struct CoreKeyShare<E: Curve> {$(
+                $(#[$attr])*
+                pub $field: $ty,
+            )+}
+        }
+    };
+}
+
+core_key_share! {
+    pub curve: CurveName<E>,
+    pub i: u16,
+    #[serde(with = "As::<generic_ec::serde::Compact>")]
+    pub shared_public_key: NonZero<Point<E>>,
+    #[serde(with = "As::<Vec<generic_ec::serde::Compact>>")]
+    pub public_shares: Vec<NonZero<Point<E>>>,
+    pub vss_setup: Option<crate::VssSetup<E>>,
+    #[cfg(feature = "hd-wallets")]
+    #[cfg_attr(
+        feature = "serde",
+        serde(default),
+        serde(with = "As::<Option<crate::utils::HexOrBin>>")
+    )]
+    pub chain_code: Option<slip_10::ChainCode>,
+    #[serde(with = "As::<generic_ec::serde::Compact>")]
+    pub x: NonZero<SecretScalar<E>>,
+}

key-share/src/serde_fix.rs

@@ -0,0 +1,3 @@
+This folder contains core key shares (aka `cggmp21::IncompleteKeyShare` or `key_share::CoreKeyShare`)
+generated at different version of the library. We use them to make sure that newer versions of the
+`key_share` crate are able to deserialize older key shares.

test-data/old-shares/README.md

@@ -0,0 +1,14 @@
+{
+  "curve": "secp256k1",
+  "i": 0,
+  "shared_public_key": "03ea3bac8aa37f22a9a6862fd78c05f13bfc8ce32ff86b745949a3fb9ea1d41b8c",
+  "public_shares": [
+    "0297b263571c7f0556d8fffc5476ca0eedd81753f8db773f8099cd441a130cac80",
+    "0217fbd8dac7a60a53b6958e0de0844c73d72bba7d897981b33be39a70f629b85f",
+    "034c7e81b999a5b0d63b64c3b02ed1f500230875d2a0f3841516293db5291a83bc",
+    "0377c80d50ed8e71c06a8348fd09c10ce3fbaa7c93b029aac4b60980134f972b51",
+    "03a19446e07ca07b04c1bbe0e1381854246b943ef3d7a3422e8c3b8b22aae3ab76"
+  ],
+  "vss_setup": null,
+  "x": "b1903373aadd5c3c509814552606ddabe77fd0fdba4f589a56866f7b88e4ac4d"
+}

test-data/old-shares/v0.1.1/secp256k1-threshold-false.cbor

@@ -0,0 +1,38 @@
+{
+  "curve": "secp256k1",
+  "i": 0,
+  "shared_public_key": "02078005f22cb615d85097d9a825aac2c4f0a6a4417873abf1fe2b4fda4bda99e2",
+  "public_shares": [
+    "03915d143b9cf4b8f6e95ea9425777c70056a4f3c694539f10595b8e2ef2cf9df9",
+    "03903e8903d49697991198cf2fc5890f5ce55ae955d5615b326b9782b773bca581",
+    "027b4f4c8a43c93c29279e3c0a2ef54f0fd63bf3b394105319378a243bce31d207",
+    "026a9d37cfc5d70b94c35274e3cf15aa6141ad77c76ab7cfb029d7f69405eae839",
+    "03b1c4a488c4cd6be65a2b1b805f7e63da18ee8ce8ae3dd8a459f995f2717cc602"
+  ],
+  "vss_setup": {
+    "min_signers": 3,
+    "I": [
+      {
+        "curve": "secp256k1",
+        "scalar": "0000000000000000000000000000000000000000000000000000000000000001"
+      },
+      {
+        "curve": "secp256k1",
+        "scalar": "0000000000000000000000000000000000000000000000000000000000000002"
+      },
+      {
+        "curve": "secp256k1",
+        "scalar": "0000000000000000000000000000000000000000000000000000000000000003"
+      },
+      {
+        "curve": "secp256k1",
+        "scalar": "0000000000000000000000000000000000000000000000000000000000000004"
+      },
+      {
+        "curve": "secp256k1",
+        "scalar": "0000000000000000000000000000000000000000000000000000000000000005"
+      }
+    ]
+  },
+  "x": "49a7c9da6f3815a0c52352c3e44f325dd1d2d9f506b2f5e840f75ba66e93ff22"
+}

test-data/old-shares/v0.1.1/secp256k1-threshold-false.json

@@ -0,0 +1,14 @@
+{
+  "curve": "secp256r1",
+  "i": 0,
+  "shared_public_key": "02714f76bc665803b80d6cd7f1d651bbfb2fd29f504f11deecd5111665ec8e5336",
+  "public_shares": [
+    "02905d81b11fd9af73900ab8b5bc70dd5f9c81a7afb7a0d0168f027f2998c3e4d2",
+    "026bdfffccb9a446a69e9efaf7f4d5208c9b264b1b6a1ac5e878083442287dad55",
+    "0326481d4aaa11b1654d8fd193fd9f4a39d47af8fc3532d281593676fcdd2e92bf",
+    "02085baeef45ce5c1daf216a5c34e39f6e4c08d3b5f85b60194b5c6244ec7e8ba5",
+    "0301b710133950ea505556a0e9dba5becbb59551b3519a8701bf8df9a8734fbbbd"
+  ],
+  "vss_setup": null,
+  "x": "27b3c8fcc99e566514d507db7a377c9948062dde1559b514ddea31d58732e2f9"
+}