Serde/bincode serialization of SecretKey and PublicKey are wasteful, compared with newtypes wrapping [u8; 32] or [u8; 33].

[fadedbee]

Using serde/bincode to serialize SecretKey or PublicKey results in an unneeded eight-byte length.

This length is unneeded, as we already know that the SecretKey is 32 bytes and the PublicKey is 33 bytes.

        #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
        struct Hash([u8; 32]);

        let hash = Hash([1u8; 32]);
        let hash_bin = bincode::serialize(&hash).unwrap();
		assert_eq!(hash_bin, [1u8; 32]);

        let skey = SecretKey::from_slice(&[2u8; 32]).unwrap();
        let skey_bin = bincode::serialize(&skey).unwrap();
		assert_eq!(skey_bin, [2u8; 32]);

fails with:

thread 'tests::it_works' panicked at 'assertion failed: `(left == right)`
  left: `[32, 0, 0, 0, 0, 0, 0, 0, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2]`,
 right: `[2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2]`'

[real-or-random]

Maybe I'm misunderstanding but I think that's none of our responsibility. If you don't like the bincode serialization, then you need to reconfigure bincode or use a different serializer. Or what do you suggest we should do to resolve this issue?

[fadedbee]

Thanks for looking at this issue.

I think the problem might be: https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/macros.rs#L58 used in https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/key.rs#L216

Perhaps the s.serialize_bytes(&self[..]) is saying to serializers: "here comes a variable length array of bytes, you'll need to encode it as you would a Vec<u8>, rather than as a [u8; 32]".

The fact that bincode encodes a newtype like struct Hash([u8; 32]) without a length prefix indicates that it might not be bincode's fault. (That's why I put struct Hash in the code snippet.)

[real-or-random]

Ok I see. I forgot that we special serde code.
This was added in 3825ed3.

Now I wonder whether this was intentional or not. Having the length at least makes sure that we're not mixing secret keys and public keys which happen to have different length. (Except for x-only pubkeys :/). Maybe @jonasnick remembers, he wrote that code.

[fadedbee]

I assume that this can't be fixed, as it would be a breaking change to other people's previously serialized keys.

How could I work around this issue?

Is keeping my own private fork of the secp256k1 crate the way to do this in Rust, or is there a better way?

[jonasnick]

@real-or-random Pretty sure this was not intentional :/ Apparently the code was copied from bitcoin_hashes.

[thomaseizinger]

Perhaps the s.serialize_bytes(&self[..]) is saying to serializers: "here comes a variable length array of bytes, you'll need to encode it as you would a Vec<u8>, rather than as a [u8; 32]".

Indeed, I think the most efficient serialization could be achieved with serialize_tuple.

I assume that this can't be fixed, as it would be a breaking change to other people's previously serialized keys.

It would be a breaking change but I don't think this should prevent us from changing it. The serialized format is "just" another API and hence can be broken with any minor version given that we are pre 1.0, no?

How could I work around this issue?

You can either create a new-type wrapper and implement Serialize and Deserialize for that or create a module with serialize and deserialize functions and use the serde(with) attribute everywhere where you want to use your new implementation.

If we decide to "fix" this and use a more efficient serialization, we could also provide such a module that implements the old format to allow people to opt into the old behaviour and make migration easier.

[elichai]

Our manual serde implementation is giving us too much troubles :/
whenever we bump a MSRV again it would be nice to move to serde_derive, although that will break downstream users so idk what will be the right way

[apoelstra]

Being pre-1.0 does not let us do breaking changes in minor versions.

Also wrapping [u8; 33] will be far less efficient for json, because it will serialize stuff like [ 1, 2, 3, 4 ] instead of 01020304. So we will need custom serialization regardless.

I'm also doubtful that serializing as a sequence/tuple will be more efficient than serialize_bytes for all serializers.

I also like the fact that you can unambiguously deserialize strings of bytes and decide based on length what kind of hash they are likely to be.

[thomaseizinger]

Being pre-1.0 does not let us do breaking changes in minor versions.

Unless I am misinterpreting point 4 of the spec, anything can change < 1.0.0

Cargo's interpretation of semver says something similar.

Cargo uses the convention that only changes in the left-most non-zero component are considered incompatible.

---

Also wrapping [u8; 33] will be far less efficient for json, because it will serialize stuff like [ 1, 2, 3, 4 ] instead of 01020304. So we will need custom serialization regardless.

I agree with the custom serialization but that seems to be more a question of handling serializer.is_human_readable properly, no?

I'm also doubtful that serializing as a sequence/tuple will be more efficient than serialize_bytes for all serializers.

That is a good point. According to the serde documentation, serialize_bytes is already a special case of serialize_seq. I guess what would be needed is something like serialize_tuple_bytes? As long as that doesn't exist, calling serialize_bytes seems to be the most appropriate thing to do.

[elichai]

Being pre-1.0 does not let us do breaking changes in minor versions.

Unless I am misinterpreting point 4 of the spec, anything can change < 1.0.0

Cargo's interpretation of semver says something similar.

Cargo uses the convention that only changes in the left-most non-zero component are considered incompatible.

https://doc.rust-lang.org/cargo/reference/semver.html#change-categories Says:

This guide uses the terms "major" and "minor" assuming this relates to a "1.0.0" release or later. Initial development releases starting with "0.y.z" can treat changes in "y" as a major release, and "z" as a minor release. "0.0.z" releases are always major changes. This is because Cargo uses the convention that only changes in the left-most non-zero component are considered incompatible.

[thomaseizinger]

Okay, thanks for pointing that out. Kind of confusing that they change the terminology IMO but as far as I can see, we are essentially saying the same thing: Changing the implementation in a breaking way is fine with the next 0.x release because it is considered a new major version prior to 1.0

[dr-orlovsky]

These are related:

• Script serialization/deserialization in serde non-human-readable format rust-bitcoin#596
• Efficiently serialize non Encodable data rust-bitcoin#595

I think discussions and solutions from there can be applied to rust-secp256k1 as well

[tcharding]

A solution to this issue is trivial but I'm having trouble distilling consensus out of the thread.

@apoelstra do you still hold the following two concerns?

I'm also doubtful that serializing as a sequence/tuple will be more efficient than serialize_bytes for all serializers.

I also like the fact that you can unambiguously deserialize strings of bytes and decide based on length what kind of hash they are likely to be.

(FTR I don't really understand this comment, can you clarify please. How is having the extra 8 bytes of length data different from just having the 32 bytes?)

A 'solution' looks like:

        if s.is_human_readable() {
            let mut buf = [0u8; 64];
            s.serialize_str(::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization"))
        } else {
            let mut seq = s.serialize_tuple(constants::SECRET_KEY_SIZE)?;
            for element in &self[..] {
                seq.serialize_element(element)?;
            }
            seq.end()
        }