Merge #406: Use fixed width serde impls for keys

3ca7f49 Add fixed-width-serde integration tests (Tobin Harding)
bf9f556 Add rustdocs describing fixed width serde (Tobin Harding)
c28808c Improve rustdocs for KeyPair (Tobin Harding)
6842383 Use fixed width serde impls for keys (Tobin Harding)

Pull request description:

  Currently we serialize keys using the `BytesVisitor`, this causes the serialized data to contain additional metadata encoding the length (an extra 8 bytes) when serialized with [bincode.](https://docs.rs/bincode/latest/bincode/index.html). This extra data is unnecessary since we know in advance the length of these two types.

  We do not control the data output by serialization of our types because it depends on which crate is used to do the serialization. This PR improves the situation for serialization using the `bincode` crate, and this PR introduces mentions of `bincode` in the rustdocs, is this acceptable? See below for a table that describes binary serialization by other crates.

  Implement a sequence based visitor that encodes the keys as fixed width data for:

  - `SecretKey`
  - `PublicKey`
  - `KeyPair`
  - `XOnlyPublicKey`

  Fixes: #295

  **Question**: PR only does keys, do we want to do signatures as well?

ACKs for top commit:
  apoelstra:
    ACK 3ca7f49

Tree-SHA512: 77babce74fa9f0981bb3b869c4e77a68a4d1ec28d22d2c3be4305e27ef01d4828dac210e20b968cbbe5de8a0563cd985d7969bccf75cfe627a34a116fed1a5df

Author: apoelstra

Cargo.toml

@@ -50,6 +50,12 @@ rand = "0.8"
 rand_core = "0.6"
 serde_test = "1.0"
 bitcoin_hashes = "0.10"
+bincode = "1.3.3"
+
+# cbor does not build on WASM, we use it in a single trivial test (an example of when
+# fixed-width-serde breaks down). Just run the test when on an x86_64 machine.
+[target.'cfg(target_arch = "x86_64")'.dependencies]
+cbor = "0.4.1"
 
 [target.wasm32-unknown-unknown.dev-dependencies]
 wasm-bindgen-test = "0.3"

src/key.rs

@@ -25,6 +25,9 @@ use crate::Error::{self, InvalidPublicKey, InvalidPublicKeySum, InvalidSecretKey
 use crate::ffi::{self, CPtr, impl_array_newtype};
 use crate::ffi::types::c_uint;
 
+#[cfg(feature = "serde")]
+use serde::ser::SerializeTuple;
+
 #[cfg(feature = "global-context")]
 use crate::{Message, ecdsa, SECP256K1};
 #[cfg(all(feature  = "global-context", feature = "rand-std"))]
@@ -33,6 +36,12 @@ use crate::Scalar;
 
 /// Secret 256-bit key used as `x` in an ECDSA signature.
 ///
+/// # Serde support
+///
+/// Implements de/serialization with the `serde` feature enabled. We treat the byte value as a tuple
+/// of 32 `u8`s for non-human-readable formats. This representation is optimal for for some formats
+/// (e.g. [`bincode`]) however other formats may be less optimal (e.g. [`cbor`]).
+///
 /// # Examples
 ///
 /// Basic usage:
@@ -45,6 +54,8 @@ use crate::Scalar;
 /// let secret_key = SecretKey::new(&mut rand::thread_rng());
 /// # }
 /// ```
+/// [`bincode`]: https://docs.rs/bincode
+/// [`cbor`]: https://docs.rs/cbor
 pub struct SecretKey([u8; constants::SECRET_KEY_SIZE]);
 impl_array_newtype!(SecretKey, u8, constants::SECRET_KEY_SIZE);
 impl_display_secret!(SecretKey);
@@ -68,6 +79,12 @@ pub const ONE_KEY: SecretKey = SecretKey([0, 0, 0, 0, 0, 0, 0, 0,
 
 /// A Secp256k1 public key, used for verification of signatures.
 ///
+/// # Serde support
+///
+/// Implements de/serialization with the `serde` feature enabled. We treat the byte value as a tuple
+/// of 33 `u8`s for non-human-readable formats. This representation is optimal for for some formats
+/// (e.g. [`bincode`]) however other formats may be less optimal (e.g. [`cbor`]).
+///
 /// # Examples
 ///
 /// Basic usage:
@@ -81,6 +98,8 @@ pub const ONE_KEY: SecretKey = SecretKey([0, 0, 0, 0, 0, 0, 0, 0,
 /// let public_key = PublicKey::from_secret_key(&secp, &secret_key);
 /// # }
 /// ```
+/// [`bincode`]: https://docs.rs/bincode
+/// [`cbor`]: https://docs.rs/cbor
 #[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)]
 #[repr(transparent)]
 pub struct PublicKey(ffi::PublicKey);
@@ -315,7 +334,11 @@ impl serde::Serialize for SecretKey {
             let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2];
             s.serialize_str(crate::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization"))
         } else {
-            s.serialize_bytes(&self[..])
+            let mut tuple = s.serialize_tuple(constants::SECRET_KEY_SIZE)?;
+            for byte in self.0.iter() {
+                tuple.serialize_element(byte)?;
+            }
+            tuple.end()
         }
     }
 }
@@ -329,10 +352,11 @@ impl<'de> serde::Deserialize<'de> for SecretKey {
                 "a hex string representing 32 byte SecretKey"
             ))
         } else {
-            d.deserialize_bytes(super::serde_util::BytesVisitor::new(
+            let visitor = super::serde_util::Tuple32Visitor::new(
                 "raw 32 bytes SecretKey",
                 SecretKey::from_slice
-            ))
+            );
+            d.deserialize_tuple(constants::SECRET_KEY_SIZE, visitor)
         }
     }
 }
@@ -649,7 +673,11 @@ impl serde::Serialize for PublicKey {
         if s.is_human_readable() {
             s.collect_str(self)
         } else {
-            s.serialize_bytes(&self.serialize())
+            let mut tuple = s.serialize_tuple(constants::PUBLIC_KEY_SIZE)?;
+            for byte in self.serialize().iter() { // Serialize in compressed form.
+                tuple.serialize_element(&byte)?;
+            }
+            tuple.end()
         }
     }
 }
@@ -663,10 +691,11 @@ impl<'de> serde::Deserialize<'de> for PublicKey {
                 "an ASCII hex string representing a public key"
             ))
         } else {
-            d.deserialize_bytes(super::serde_util::BytesVisitor::new(
-                "a bytestring representing a public key",
+            let visitor = super::serde_util::Tuple33Visitor::new(
+                "33 bytes compressed public key",
                 PublicKey::from_slice
-            ))
+            );
+            d.deserialize_tuple(constants::PUBLIC_KEY_SIZE, visitor)
         }
     }
 }
@@ -687,13 +716,10 @@ impl Ord for PublicKey {
 ///
 /// # Serde support
 ///
-/// [`Serialize`] and [`Deserialize`] are not implemented for this type, even with the `serde`
-/// feature active. This is due to security considerations, see the [`serde_keypair`] documentation
-/// for details.
-///
-/// If the `serde` and `global-context` features are active `KeyPair`s can be serialized and
-/// deserialized by annotating them with `#[serde(with = "secp256k1::serde_keypair")]`
-/// inside structs or enums for which [`Serialize`] and [`Deserialize`] are being derived.
+/// Implements de/serialization with the `serde` and_`global-context` features enabled. Serializes
+/// the secret bytes only. We treat the byte value as a tuple of 32 `u8`s for non-human-readable
+/// formats. This representation is optimal for for some formats (e.g. [`bincode`]) however other
+/// formats may be less optimal (e.g. [`cbor`]). For human-readable formats we use a hex string.
 ///
 /// # Examples
 ///
@@ -708,8 +734,8 @@ impl Ord for PublicKey {
 /// let key_pair = KeyPair::from_secret_key(&secp, &secret_key);
 /// # }
 /// ```
-/// [`Deserialize`]: serde::Deserialize
-/// [`Serialize`]: serde::Serialize
+/// [`bincode`]: https://docs.rs/bincode
+/// [`cbor`]: https://docs.rs/cbor
 #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct KeyPair(ffi::KeyPair);
 impl_display_secret!(KeyPair);
@@ -966,7 +992,11 @@ impl serde::Serialize for KeyPair {
             s.serialize_str(crate::to_hex(&self.secret_bytes(), &mut buf)
                 .expect("fixed-size hex serialization"))
         } else {
-            s.serialize_bytes(&self.0[..])
+            let mut tuple = s.serialize_tuple(constants::SECRET_KEY_SIZE)?;
+            for byte in self.secret_bytes().iter() {
+                tuple.serialize_element(&byte)?;
+            }
+            tuple.end()
         }
     }
 }
@@ -980,19 +1010,26 @@ impl<'de> serde::Deserialize<'de> for KeyPair {
                 "a hex string representing 32 byte KeyPair"
             ))
         } else {
-            d.deserialize_bytes(super::serde_util::BytesVisitor::new(
+            let visitor = super::serde_util::Tuple32Visitor::new(
                 "raw 32 bytes KeyPair",
                 |data| unsafe {
                     let ctx = Secp256k1::from_raw_all(ffi::secp256k1_context_no_precomp as *mut ffi::Context);
                     KeyPair::from_seckey_slice(&ctx, data)
                 }
-            ))
+            );
+            d.deserialize_tuple(constants::SECRET_KEY_SIZE, visitor)
         }
     }
 }
 
 /// An x-only public key, used for verification of Schnorr signatures and serialized according to BIP-340.
 ///
+/// # Serde support
+///
+/// Implements de/serialization with the `serde` feature enabled. We treat the byte value as a tuple
+/// of 32 `u8`s for non-human-readable formats. This representation is optimal for for some formats
+/// (e.g. [`bincode`]) however other formats may be less optimal (e.g. [`cbor`]).
+///
 /// # Examples
 ///
 /// Basic usage:
@@ -1006,6 +1043,8 @@ impl<'de> serde::Deserialize<'de> for KeyPair {
 /// let xonly = XOnlyPublicKey::from_keypair(&key_pair);
 /// # }
 /// ```
+/// [`bincode`]: https://docs.rs/bincode
+/// [`cbor`]: https://docs.rs/cbor
 #[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)]
 pub struct XOnlyPublicKey(ffi::XOnlyPublicKey);
 
@@ -1429,7 +1468,11 @@ impl serde::Serialize for XOnlyPublicKey {
         if s.is_human_readable() {
             s.collect_str(self)
         } else {
-            s.serialize_bytes(&self.serialize())
+            let mut tuple = s.serialize_tuple(constants::SCHNORR_PUBLIC_KEY_SIZE)?;
+            for byte in self.serialize().iter() {
+                tuple.serialize_element(&byte)?;
+            }
+            tuple.end()
         }
     }
 }
@@ -1443,10 +1486,11 @@ impl<'de> serde::Deserialize<'de> for XOnlyPublicKey {
                 "a hex string representing 32 byte schnorr public key"
             ))
         } else {
-            d.deserialize_bytes(super::serde_util::BytesVisitor::new(
+            let visitor = super::serde_util::Tuple32Visitor::new(
                 "raw 32 bytes schnorr public key",
                 XOnlyPublicKey::from_slice
-            ))
+            );
+            d.deserialize_tuple(constants::SCHNORR_PUBLIC_KEY_SIZE, visitor)
         }
     }
 }
@@ -1492,6 +1536,8 @@ pub mod serde_keypair {
 #[cfg(test)]
 #[allow(unused_imports)]
 mod test {
+    use super::*;
+
     use core::str::FromStr;
 
     #[cfg(any(feature = "alloc", feature = "std"))]
@@ -1949,6 +1995,7 @@ mod test {
         static SK_STR: &'static str = "\
             01010101010101010001020304050607ffff0000ffff00006363636363636363\
         ";
+        #[cfg(fuzzing)]
         static PK_BYTES: [u8; 33] = [
             0x02,
             0x18, 0x84, 0x57, 0x81, 0xf6, 0x31, 0xc4, 0x8f,
@@ -1971,22 +2018,32 @@ mod test {
         #[cfg(fuzzing)]
         let pk = PublicKey::from_slice(&PK_BYTES).expect("pk");
 
-        assert_tokens(&sk.compact(), &[Token::BorrowedBytes(&SK_BYTES[..])]);
-        assert_tokens(&sk.compact(), &[Token::Bytes(&SK_BYTES)]);
-        assert_tokens(&sk.compact(), &[Token::ByteBuf(&SK_BYTES)]);
+        assert_tokens(&sk.compact(), &[
+            Token::Tuple{ len: 32 },
+            Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1),
+            Token::U8(0), Token::U8(1), Token::U8(2), Token::U8(3), Token::U8(4), Token::U8(5), Token::U8(6), Token::U8(7),
+            Token::U8(0xff), Token::U8(0xff), Token::U8(0), Token::U8(0), Token::U8(0xff), Token::U8(0xff), Token::U8(0), Token::U8(0),
+            Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99),
+            Token::TupleEnd
+        ]);
 
         assert_tokens(&sk.readable(), &[Token::BorrowedStr(SK_STR)]);
         assert_tokens(&sk.readable(), &[Token::Str(SK_STR)]);
         assert_tokens(&sk.readable(), &[Token::String(SK_STR)]);
 
-        assert_tokens(&pk.compact(), &[Token::BorrowedBytes(&PK_BYTES[..])]);
-        assert_tokens(&pk.compact(), &[Token::Bytes(&PK_BYTES)]);
-        assert_tokens(&pk.compact(), &[Token::ByteBuf(&PK_BYTES)]);
+        assert_tokens(&pk.compact(), &[
+            Token::Tuple{ len: 33 },
+            Token::U8(0x02),
+            Token::U8(0x18), Token::U8(0x84), Token::U8(0x57), Token::U8(0x81), Token::U8(0xf6), Token::U8(0x31), Token::U8(0xc4), Token::U8(0x8f),
+            Token::U8(0x1c), Token::U8(0x97), Token::U8(0x09), Token::U8(0xe2), Token::U8(0x30), Token::U8(0x92), Token::U8(0x06), Token::U8(0x7d),
+            Token::U8(0x06), Token::U8(0x83), Token::U8(0x7f), Token::U8(0x30), Token::U8(0xaa), Token::U8(0x0c), Token::U8(0xd0), Token::U8(0x54),
+            Token::U8(0x4a), Token::U8(0xc8), Token::U8(0x87), Token::U8(0xfe), Token::U8(0x91), Token::U8(0xdd), Token::U8(0xd1), Token::U8(0x66),
+            Token::TupleEnd
+        ]);
 
         assert_tokens(&pk.readable(), &[Token::BorrowedStr(PK_STR)]);
         assert_tokens(&pk.readable(), &[Token::Str(PK_STR)]);
         assert_tokens(&pk.readable(), &[Token::String(PK_STR)]);
-
     }
 
     #[test]
@@ -2066,10 +2123,14 @@ mod test {
         ";
 
         let sk = KeyPairWrapper(KeyPair::from_seckey_slice(&crate::SECP256K1, &SK_BYTES).unwrap());
-
-        assert_tokens(&sk.compact(), &[Token::BorrowedBytes(&SK_BYTES[..])]);
-        assert_tokens(&sk.compact(), &[Token::Bytes(&SK_BYTES)]);
-        assert_tokens(&sk.compact(), &[Token::ByteBuf(&SK_BYTES)]);
+        assert_tokens(&sk.compact(), &[
+            Token::Tuple{ len: 32 },
+            Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1),
+            Token::U8(0), Token::U8(1), Token::U8(2), Token::U8(3), Token::U8(4), Token::U8(5), Token::U8(6), Token::U8(7),
+            Token::U8(0xff), Token::U8(0xff), Token::U8(0), Token::U8(0), Token::U8(0xff), Token::U8(0xff), Token::U8(0), Token::U8(0),
+            Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99), Token::U8(99),
+            Token::TupleEnd
+        ]);
 
         assert_tokens(&sk.readable(), &[Token::BorrowedStr(SK_STR)]);
         assert_tokens(&sk.readable(), &[Token::Str(SK_STR)]);
@@ -2221,4 +2282,38 @@ mod test {
 
         assert_eq!(got, want)
     }
+
+    #[test]
+    #[cfg(not(fuzzing))]
+    #[cfg(all(feature = "global-context", feature = "serde"))]
+    fn test_serde_x_only_pubkey() {
+        use serde_test::{Configure, Token, assert_tokens};
+
+        static SK_BYTES: [u8; 32] = [
+            1, 1, 1, 1, 1, 1, 1, 1,
+            0, 1, 2, 3, 4, 5, 6, 7,
+            0xff, 0xff, 0, 0, 0xff, 0xff, 0, 0,
+            99, 99, 99, 99, 99, 99, 99, 99
+        ];
+
+        static PK_STR: &'static str = "\
+            18845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166\
+        ";
+
+        let kp = KeyPair::from_seckey_slice(&crate::SECP256K1, &SK_BYTES).unwrap();
+        let (pk, _parity) = XOnlyPublicKey::from_keypair(&kp);
+
+        assert_tokens(&pk.compact(), &[
+            Token::Tuple{ len: 32 },
+            Token::U8(0x18), Token::U8(0x84), Token::U8(0x57), Token::U8(0x81), Token::U8(0xf6), Token::U8(0x31), Token::U8(0xc4), Token::U8(0x8f),
+            Token::U8(0x1c), Token::U8(0x97), Token::U8(0x09), Token::U8(0xe2), Token::U8(0x30), Token::U8(0x92), Token::U8(0x06), Token::U8(0x7d),
+            Token::U8(0x06), Token::U8(0x83), Token::U8(0x7f), Token::U8(0x30), Token::U8(0xaa), Token::U8(0x0c), Token::U8(0xd0), Token::U8(0x54),
+            Token::U8(0x4a), Token::U8(0xc8), Token::U8(0x87), Token::U8(0xfe), Token::U8(0x91), Token::U8(0xdd), Token::U8(0xd1), Token::U8(0x66),
+            Token::TupleEnd
+        ]);
+
+        assert_tokens(&pk.readable(), &[Token::BorrowedStr(PK_STR)]);
+        assert_tokens(&pk.readable(), &[Token::Str(PK_STR)]);
+        assert_tokens(&pk.readable(), &[Token::String(PK_STR)]);
+    }
 }

src/schnorr.rs

@@ -569,9 +569,14 @@ mod tests {
         assert_tokens(&sig.readable(), &[Token::Str(SIG_STR)]);
         assert_tokens(&sig.readable(), &[Token::String(SIG_STR)]);
 
-        assert_tokens(&pk.compact(), &[Token::BorrowedBytes(&PK_BYTES[..])]);
-        assert_tokens(&pk.compact(), &[Token::Bytes(&PK_BYTES[..])]);
-        assert_tokens(&pk.compact(), &[Token::ByteBuf(&PK_BYTES[..])]);
+        assert_tokens(&pk.compact(), &[
+            Token::Tuple{ len: 32 },
+            Token::U8(24), Token::U8(132), Token::U8(87), Token::U8(129), Token::U8(246), Token::U8(49), Token::U8(196), Token::U8(143),
+            Token::U8(28), Token::U8(151), Token::U8(9), Token::U8(226), Token::U8(48), Token::U8(146), Token::U8(6), Token::U8(125),
+            Token::U8(6), Token::U8(131), Token::U8(127), Token::U8(48), Token::U8(170), Token::U8(12), Token::U8(208), Token::U8(84),
+            Token::U8(74), Token::U8(200), Token::U8(135), Token::U8(254), Token::U8(145), Token::U8(221), Token::U8(209), Token::U8(102),
+            Token::TupleEnd
+        ]);
 
         assert_tokens(&pk.readable(), &[Token::BorrowedStr(PK_STR)]);
         assert_tokens(&pk.readable(), &[Token::Str(PK_STR)]);