Skip to content

Commit

Permalink
Use fixed width serde impls for keys
Browse files Browse the repository at this point in the history 
Currently we serialize keys using the `BytesVisitor`, this causes the
serialized data to contain additional metadata encoding the length (an
extra 8 bytes). This extra data is unnecessary since we know in advance
the length of these two types.

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

Do fixed width serde imples for:

- SecretKey
- PublicKey
- KeyPair
- XOnlyPublicKey
  • Loading branch information
@tcharding
tcharding committed Feb 21, 2022
1 parent d0e5c54 commit d8a0399
Show file tree
Hide file tree
Showing 8 changed files with 194 additions and 57 deletions.
1 change: 1 addition & 0 deletions Cargo.toml
View file
Expand Up @@ -32,6 +32,7 @@ global-context = ["std"]
secp256k1-sys = { version = "0.4.2", default-features = false, path = "./secp256k1-sys" }
bitcoin_hashes = { version = "0.10", optional = true }
rand = { version = "0.6", default-features = false, optional = true }
# Our custom serde code requires an allocator, enable secp256k1 feature `std` or `alloc`.
serde = { version = "1.0", default-features = false, optional = true }


Expand Down
14 changes: 9 additions & 5 deletions contrib/test.sh
View file
@@ -1,7 +1,7 @@
#!/bin/sh -ex

# TODO: Add "alloc" once we bump MSRV to past 1.29
FEATURES="bitcoin_hashes global-context lowmemory rand rand-std recovery serde std"
FEATURES="bitcoin_hashes global-context lowmemory rand rand-std recovery std"

# Use toolchain if explicitly specified
if [ -n "$TOOLCHAIN" ]
Expand All @@ -24,8 +24,8 @@ if [ "$DO_FEATURE_MATRIX" = true ]; then
cargo test --all --no-default-features

# All features
cargo build --all --no-default-features --features="$FEATURES"
cargo test --all --no-default-features --features="$FEATURES"
cargo build --all --no-default-features --features="$FEATURES serde"
cargo test --all --no-default-features --features="$FEATURES serde"
# Single features
for feature in ${FEATURES}
do
Expand All @@ -35,9 +35,12 @@ if [ "$DO_FEATURE_MATRIX" = true ]; then

# Other combos
RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS=$RUSTFLAGS cargo test --all
RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS=$RUSTFLAGS cargo test --all --features="$FEATURES"
RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS=$RUSTFLAGS cargo test --all --features="$FEATURES serde"
cargo test --all --features="rand rand-std"
cargo test --all --features="rand serde"
# serde requires an allocator
cargo build --all --no-default-features --features=alloc,serde
cargo test --all --no-default-features --features=alloc,serde

if [ "$DO_BENCH" = true ]; then # proxy for us having a nightly compiler
cargo test --all --all-features
Expand All @@ -52,7 +55,7 @@ fi

# Docs
if [ "$DO_DOCS" = true ]; then
cargo doc --all --features="$FEATURES"
cargo doc --all --features="$FEATURES serde"
fi

# Webassembly stuff
Expand All @@ -77,6 +80,7 @@ if [ "$DO_ASAN" = true ]; then
cargo test --lib --all --features="$FEATURES" -Zbuild-std --target x86_64-unknown-linux-gnu &&
cargo run --release --manifest-path=./no_std_test/Cargo.toml | grep -q "Verified Successfully"
cargo run --release --features=alloc --manifest-path=./no_std_test/Cargo.toml | grep -q "Verified alloc Successfully"
cargo run --release --features=use-serde --manifest-path=./no_std_test/Cargo.toml | grep -q "Verified Successfully"
fi

# Test if panic in C code aborts the process (either with a real panic or with SIGILL)
Expand Down
3 changes: 2 additions & 1 deletion no_std_test/Cargo.toml
View file
Expand Up @@ -5,10 +5,11 @@ authors = ["Elichai Turkel <elichai.turkel@gmail.com>"]

[features]
alloc = ["secp256k1/alloc", "wee_alloc"]
use-serde = ["alloc", "secp256k1/serde"]

[dependencies]
wee_alloc = { version = "0.4.5", optional = true }
secp256k1 = { path = "../", default-features = false, features = ["serde", "rand", "recovery"] }
secp256k1 = { path = "../", default-features = false, features = ["rand", "recovery"] }
libc = { version = "0.2", default-features = false }
serde_cbor = { version = "0.10", default-features = false } # A random serializer that supports no-std.

Expand Down
47 changes: 25 additions & 22 deletions no_std_test/src/main.rs
View file
Expand Up @@ -65,16 +65,10 @@ use core::fmt::{self, write, Write};
use core::intrinsics;
use core::panic::PanicInfo;

use secp256k1::ecdh::SharedSecret;
use secp256k1::ffi::types::AlignedType;
use secp256k1::rand::{self, RngCore};
use secp256k1::serde::Serialize;
use secp256k1::*;

use serde_cbor::de;
use serde_cbor::ser::SliceWrite;
use serde_cbor::Serializer;

struct FakeRng;
impl RngCore for FakeRng {
fn next_u32(&mut self) -> u32 {
Expand Down Expand Up @@ -116,25 +110,34 @@ fn start(_argc: isize, _argv: *const *const u8) -> isize {
let new_rec_sig = ecdsa::RecoverableSignature::from_compact(&data, rec_id).unwrap();
assert_eq!(rec_sig, new_rec_sig);

let mut cbor_ser = [0u8; 100];
let writer = SliceWrite::new(&mut cbor_ser[..]);
let mut ser = Serializer::new(writer);
sig.serialize(&mut ser).unwrap();
let size = ser.into_inner().bytes_written();
let new_sig: ecdsa::Signature = de::from_mut_slice(&mut cbor_ser[..size]).unwrap();
assert_eq!(sig, new_sig);

let _ = SharedSecret::new(&public_key, &secret_key);
let mut x_arr = [0u8; 32];
let y_arr = SharedSecret::new_with_hash(&public_key, &secret_key, |x,y| {
x_arr = x;
y.into()
});
assert_ne!(x_arr, [0u8; 32]);
assert_ne!(&y_arr[..], &[0u8; 32][..]);
#[cfg(feature = "use-serde")] {
use secp256k1::serde::Serialize;
use serde_cbor::de;
use serde_cbor::ser::SliceWrite;
use serde_cbor::Serializer;

let mut cbor_ser = [0u8; 100];
let writer = SliceWrite::new(&mut cbor_ser[..]);
let mut ser = Serializer::new(writer);
sig.serialize(&mut ser).unwrap();
let size = ser.into_inner().bytes_written();
let new_sig: ecdsa::Signature = de::from_mut_slice(&mut cbor_ser[..size]).unwrap();
assert_eq!(sig, new_sig);
}

#[cfg(feature = "alloc")]
{
use secp256k1::ecdh::SharedSecret;

let _ = SharedSecret::new(&public_key, &secret_key);
let mut x_arr = [0u8; 32];
let y_arr = SharedSecret::new_with_hash(&public_key, &secret_key, |x,y| {
x_arr = x;
y.into()
});
assert_ne!(x_arr, [0u8; 32]);
assert_ne!(&y_arr[..], &[0u8; 32][..]);

let secp_alloc = Secp256k1::new();
let public_key = PublicKey::from_secret_key(&secp_alloc, &secret_key);
let message = Message::from_slice(&[0xab; 32]).expect("32 bytes");
Expand Down
124 changes: 98 additions & 26 deletions src/key.rs
View file
Expand Up @@ -28,6 +28,9 @@ use Verification;
use constants;
use ffi::{self, CPtr};

#[cfg(feature = "serde")]
use serde::ser::SerializeTuple;

#[cfg(feature = "global-context")]
use {Message, ecdsa, SECP256K1};
#[cfg(all(feature = "global-context", feature = "rand-std"))]
Expand Down Expand Up @@ -302,21 +305,25 @@ impl ::serde::Serialize for SecretKey {
let mut buf = [0u8; 64];
s.serialize_str(::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization"))
} else {
s.serialize_bytes(&self[..])
let mut seq = s.serialize_tuple(constants::SECRET_KEY_SIZE)?;
for byte in self.0.iter() {
seq.serialize_element(byte)?;
}
seq.end()
}
}
}

#[cfg(feature = "serde")]
#[cfg_attr(docsrs, doc(cfg(feature = "serde")))]
#[cfg(all(feature = "serde", any(feature = "std", feature = "alloc")))]
#[cfg_attr(docsrs, doc(cfg(all(feature = "serde", any(feature = "std", feature = "alloc")))))]
impl<'de> ::serde::Deserialize<'de> for SecretKey {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
if d.is_human_readable() {
d.deserialize_str(super::serde_util::FromStrVisitor::new(
"a hex string representing 32 byte SecretKey"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
d.deserialize_seq(super::serde_util::SeqVisitor::new(
"raw 32 bytes SecretKey",
SecretKey::from_slice
))
Expand Down Expand Up @@ -615,24 +622,29 @@ impl From<ffi::PublicKey> for PublicKey {
#[cfg_attr(docsrs, doc(cfg(feature = "serde")))]
impl ::serde::Serialize for PublicKey {
fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
// FIXME: human_readable serializes the whole 64 bytes but non-human-readable does 33 (compressed). Is this correct?
if s.is_human_readable() {
s.collect_str(self)
} else {
s.serialize_bytes(&self.serialize())
let mut seq = s.serialize_tuple(constants::PUBLIC_KEY_SIZE)?;
for byte in self.serialize().iter() { // Serialize in compressed form.
seq.serialize_element(&byte)?;
}
seq.end()
}
}
}

#[cfg(feature = "serde")]
#[cfg_attr(docsrs, doc(cfg(feature = "serde")))]
#[cfg(all(feature = "serde", any(feature = "std", feature = "alloc")))]
#[cfg_attr(docsrs, doc(cfg(all(feature = "serde", any(feature = "std", feature = "alloc")))))]
impl<'de> ::serde::Deserialize<'de> for PublicKey {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<PublicKey, D::Error> {
if d.is_human_readable() {
d.deserialize_str(super::serde_util::FromStrVisitor::new(
"an ASCII hex string representing a public key"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
d.deserialize_seq(super::serde_util::SeqVisitor::new(
"a bytestring representing a public key",
PublicKey::from_slice
))
Expand Down Expand Up @@ -929,21 +941,25 @@ impl ::serde::Serialize for KeyPair {
s.serialize_str(::to_hex(&self.serialize_secret(), &mut buf)
.expect("fixed-size hex serialization"))
} else {
s.serialize_bytes(&self.0[..])
let mut seq = s.serialize_tuple(constants::SECRET_KEY_SIZE)?;
for byte in self.serialize_secret().iter() {
seq.serialize_element(&byte)?;
}
seq.end()
}
}
}

#[cfg(feature = "serde")]
#[cfg_attr(docsrs, doc(cfg(feature = "serde")))]
#[cfg(all(feature = "serde", any(feature = "std", feature = "alloc")))]
#[cfg_attr(docsrs, doc(cfg(all(feature = "serde", any(feature = "std", feature = "alloc")))))]
impl<'de> ::serde::Deserialize<'de> for KeyPair {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
if d.is_human_readable() {
d.deserialize_str(super::serde_util::FromStrVisitor::new(
"a hex string representing 32 byte KeyPair"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
d.deserialize_seq(super::serde_util::SeqVisitor::new(
"raw 32 bytes KeyPair",
|data| unsafe {
let ctx = Secp256k1::from_raw_all(ffi::secp256k1_context_no_precomp as *mut ffi::Context);
Expand Down Expand Up @@ -1350,24 +1366,29 @@ impl From<::key::PublicKey> for XOnlyPublicKey {
#[cfg_attr(docsrs, doc(cfg(feature = "serde")))]
impl ::serde::Serialize for XOnlyPublicKey {
fn serialize<S: ::serde::Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
// FIXME: human_readable serializes the whole 64 bytes but non-human-readable does 32 bytes (x co-ordinate). Is this correct?
if s.is_human_readable() {
s.collect_str(self)
} else {
s.serialize_bytes(&self.serialize())
let mut seq = s.serialize_tuple(constants::SCHNORRSIG_PUBLIC_KEY_SIZE)?;
for byte in self.serialize().iter() {
seq.serialize_element(&byte)?;
}
seq.end()
}
}
}

#[cfg(feature = "serde")]
#[cfg_attr(docsrs, doc(cfg(feature = "serde")))]
#[cfg(all(feature = "serde", any(feature = "std", feature = "alloc")))]
#[cfg_attr(docsrs, doc(cfg(all(feature = "serde", any(feature = "std", feature = "alloc")))))]
impl<'de> ::serde::Deserialize<'de> for XOnlyPublicKey {
fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
if d.is_human_readable() {
d.deserialize_str(super::serde_util::FromStrVisitor::new(
"a hex string representing 32 byte schnorr public key"
))
} else {
d.deserialize_bytes(super::serde_util::BytesVisitor::new(
d.deserialize_seq(super::serde_util::SeqVisitor::new(
"raw 32 bytes schnorr public key",
XOnlyPublicKey::from_slice
))
Expand Down Expand Up @@ -1911,6 +1932,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 @@ -1933,22 +1955,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 @@ -2027,12 +2059,52 @@ mod test {

let sk = KeyPairWrapper(KeyPair::from_seckey_slice(&::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)]);
assert_tokens(&sk.readable(), &[Token::String(SK_STR)]);
}

#[test]
#[cfg(all(feature = "global-context", feature = "serde"))]
fn test_serde_x_only_pubkey() {
use serde_test::{Configure, Token, assert_tokens};
use key::KeyPair;

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\
";

// FIXME: Do we need to cfg fuzzing/not-fuzzing keypair?
let kp = KeyPair::from_seckey_slice(&::SECP256K1, &SK_BYTES).unwrap();
let pk = 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)]);
}
}
3 changes: 3 additions & 0 deletions src/lib.rs
View file
Expand Up @@ -163,6 +163,9 @@
#![cfg_attr(all(test, feature = "unstable"), feature(test))]
#![cfg_attr(docsrs, feature(doc_cfg))]

#[cfg(all(feature = "serde", not(feature = "std"), not(feature = "alloc")))]
compile_error!("Feature serde requires an allocator, please enable 'std' or 'alloc'");

#[macro_use]
pub extern crate secp256k1_sys;
pub use secp256k1_sys as ffi;
Expand Down

0 comments on commit d8a0399

Please sign in to comment.