Skip to content

Commit

Permalink
Merge #406: Use fixed width serde impls for keys
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@apoelstra
apoelstra committed Jun 15, 2022
2 parents 73ad30d + 3ca7f49 commit 613d7dc
Show file tree
Hide file tree
Showing 5 changed files with 282 additions and 36 deletions.
6 changes: 6 additions & 0 deletions Cargo.toml
View file
Expand Up @@ -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"
Expand Down
161 changes: 128 additions & 33 deletions src/key.rs
View file
Expand Up @@ -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"))]
Expand All @@ -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:
Expand All @@ -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);
Expand All @@ -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:
Expand All @@ -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);
Expand Down Expand Up @@ -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()
}
}
}
Expand All @@ -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)
}
}
}
Expand Down Expand Up @@ -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()
}
}
}
Expand All @@ -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)
}
}
}
Expand All @@ -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
///
Expand All @@ -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);
Expand Down Expand Up @@ -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()
}
}
}
Expand All @@ -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:
Expand All @@ -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);

Expand Down Expand Up @@ -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()
}
}
}
Expand All @@ -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)
}
}
}
Expand Down Expand Up @@ -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"))]
Expand Down Expand Up @@ -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,
Expand All @@ -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]
Expand Down Expand Up @@ -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)]);
Expand Down Expand Up @@ -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)]);
}
}
11 changes: 8 additions & 3 deletions src/schnorr.rs
View file
Expand Up @@ -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)]);
Expand Down

0 comments on commit 613d7dc

Please sign in to comment.