Use fixed width serde impls for keys #406

tcharding · 2022-02-16T16:37:28Z

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.. 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?

src/serde_util.rs

apoelstra · 2022-03-01T13:12:20Z

I think we can just use a fixed-size array of size 74 rather than a Vec. AFAIK there is no object in this library which has a longer serialization.

Kixunil

Interesting idea! I'd have to see some analysis on real impact of this change on existing formats to believe we should do this. I worry many formats will store type along with each item.

Kixunil · 2022-03-01T15:16:51Z

no_std_test/src/main.rs

    let _ = SharedSecret::new(&public_key, &secret_key);
    let _ = ecdh::shared_secret_point(&public_key, &secret_key);

+    #[cfg(feature = "use-serde")] {


This style is inconsistent with the rest of the code and quite confusing.

Do you mean inconsistent with the rust-secp256k1 crate? This is a separate crate and the features are only used, as far as I can tell, to run this crate and selectively run parts of the test (start function). scratch through because removal of Vec removes the need for this feature anyways.

FTR I'm not sure why this is a separate crate, I'm not super deep in the no-std stuff, @elichai do you remember off the top of your head why we had to put it in a separate crate and not in the tests directory (i.e., standard integration tests directory)?

The goal was to simulate macro usage from rust-bitcoin, which has less visibility of symbols than anything within the crate (including, I think (?) stuff in the tests/ directory).

I believe this also predates the tests directory.

Kixunil · 2022-03-01T15:21:34Z

src/key.rs

 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(


This should be deserialize_tuple

Oh so implement de::Visitor for each fixed size type we have and use deserialize_tuple, interesting. I'll give it a shot.

Yes, if you want to keep it generic I suspect you'll have to use PhantomData<fn() -> T>

With a macro for the serde impls, and serializing with tuple this PR is way cleaner now!

src/key.rs

Kixunil · 2022-03-01T15:25:25Z

Cargo.toml

@@ -39,6 +39,7 @@ global-context-less-secure = []
 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`.


Where does it allocate? I don't see any such instance.

Besides, TODO after MSRV bump: use crate renaming trick instead.

Allocates due to use of the Vec in visit_seq method.

Ah, I see. Looks like using array is feasible?

Besides, TODO after MSRV bump: use crate renaming trick instead.

Not sure what you mean here mate, is this comment still relevant with the removal of alloc/vec stuff?

tcharding · 2022-03-01T17:16:03Z

I think we can just use a fixed-size array of size 74 rather than a Vec. AFAIK there is no object in this library which has a longer serialization.

If we follow @Kixunil's suggestion above the Vec goes away also. Commenting to acknowledge seeing this comment (and not ignoring it :)

Kixunil

Generics would be more elegant than macro but without const generics it'd be a bit more boilerplate, so I think it's OK.

Do we know for sure binary codecs don't store type with each element?

src/serde_util.rs

Kixunil · 2022-03-01T18:47:05Z

src/serde_util.rs

+                        v[i] = value;
+                    } else {
+                        let msg = format!("tuple too short, index: {}, expected length: {}", i, $len);
+                        return Err(de::Error::custom(msg));


invalid_length() would be better. Also I think de::Error is a bit confusing because it refers to the trait not type. Err:: would look better but Err is also special - even used here - how come it even works? I suggest either calling it Error or just E.

Will do, thanks.

Kixunil · 2022-03-01T18:53:08Z

src/serde_util.rs

+                        return Err(de::Error::custom(msg));
+                    }
+                }
+                (self.parse_fn)(&v).map_err(de::Error::custom)


This error could be specialized too. Maybe parse_fn should return appropriate serde error already?

Will re-think, thanks.

Kixunil · 2022-03-01T19:02:10Z

src/serde_util.rs

+macro_rules! impl_tuple_visitor {
+    ($thing:ident, $len:expr) => {
+        /// Visitor type for de/ser a 32 byte sequence.
+        #[cfg(any(feature = "std", feature = "alloc"))]


I think this is no longer needed and the reason why test fails. FTR, if it was needed it'd be a demonstration of the compatibility issues I mentioned a few times.

Oh, sloppy work by me. This is not needed.

tcharding · 2022-03-01T19:10:31Z

Do we know for sure binary codecs don't store type with each element?

Don't the unit tests check that? They test the token is exactly the expected type? Or do you mean that we should pick a binary codec and add a unit test that serializes and checks the number of bytes is as expected?

tcharding · 2022-03-01T19:13:35Z

Thanks for the review @Kixunil, I'll re-spin tomorrow.

Kixunil · 2022-03-01T19:27:47Z

I mean this could be regression in data size for some formats. We don't have tests for specific data.

Kixunil · 2022-03-08T09:08:39Z

src/key.rs

+/// Implements de/serialization with the `serde` feature enabled. Serialized data is fixed width (32
+/// bytes) if you use `bincode` to do serialization, other binary formats may add metadata to the
+/// serialized data (e.g. cbor and tycho).
+///


I think it'd be very useful to explicitly say we treat it as tuple of 32 u8s for non-human formats. I think it's a good idea to mention serialization formats but I would word it like this:

"This representation is optimal for e.g. bincode format. Some formats may be less optimal."

While implementing this suggestion I realized that we do not differentiate between human-readable and non-humand-readable in secp as we do in bitcoin. Is this by design or an omission?

This also means that the docs are now slightly misleading because they imply that we do something different for human-readable formats.

Implements de/serialization with the serde feature enabled. We treat the byte value as a tuple of 33 u8s 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).

Damn, I realized it may sound like we don't implement human-readable format. So should say we do it as hex.

I documented the hex string for human-readable formats.

Kixunil · 2022-03-08T09:12:35Z

tests/serde.rs

+    let mut e = cbor::Encoder::from_memory();
+    e.encode(sk.as_ref()).unwrap();
+
+    assert_ne!(e.as_bytes().len(), 32);


If cbor adds a byte for each item shouldn't this be 64?

It only adds metadata if the value is < 24 (no clue why). I've changed the test to check against len==52 and explained the '52' in a code comment. This is a regression test after all so I think its ok to have that hard coded value.

Ah, that's weird.

Kixunil · 2022-03-08T09:18:43Z

tests/serde.rs

+    let pk = PublicKey::from_slice(&PK_BYTES).expect("failed to create pk from slice");
+    let ser = bincode::serialize(&pk).unwrap();
+
+    // We cannot use assert_eq on the whole array because 33 byte array does not implement Debug.


I believe assert_eq!(ser, &PK_BYTES as &[u8]) should work.

Legend, thanks!

tcharding · 2022-03-08T21:57:29Z

Changes in foce-push:

Fixed docs as suggested by @Kixunil
Improved unit test as discussed above

tcharding · 2022-05-11T22:47:30Z

Changes in force-push: Updated serde unit test for XOnlyPublicKey to include the new return type of from_keypair (returns parity now too).

tcharding · 2022-05-11T23:36:19Z

And added the hex docs suggestion from above.

apoelstra

ACK db656a5

Thank you for checking the actual behavior! And for adding integration tests that will detect if we break things in the future.

I am a little bit wary of this PR beacuse it will result in serialization breakage. It will definitely need to go in a major release at least.

tcharding · 2022-05-15T22:29:07Z

I am a little bit wary of this PR beacuse it will result in serialization breakage.

Do you want me to look into adding some sort of compatibility code for migrating from the current serialization to the one in this PR?

apoelstra · 2022-05-18T17:46:59Z

@tcharding sure -- but I'm not optimistic because there is no versioning or anything in our encoding.

One strategy might be to continue to allow decoding the old format, while only encoding the new format ... but that has the potential to just make the breakage harder-to-find for downstream users.

tcharding · 2022-05-19T00:53:06Z

Ok, without a clear positive way forward I'll leave it as is and hide behind 'not version 1.0 yet' :)

tcharding · 2022-06-09T05:28:07Z

Changes in force push: rebase, the only merge conflict was a path (add crate prefix) in unit tests. No other changes.

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 the `bincode` crate. This extra data is unnecessary since we know in advance the length of these types. It would be useful for users of the lib to be able to get a fixed width binary serialization, this can be done but it depends on the crate used to do the serialization. We elect to optimise for `bincode` and add docs noting that other binary serialization crates may differ (rustdocs added in separate patches). Implement a tuple based visitor that encodes the keys as fixed width data. Do fixed width serde implementations for: - `SecretKey` - `PublicKey` - `KeyPair` - `XOnlyPublicKey`

Currently the rustdocs for `KeyPair` are stale in regards to serde, we _do_ implement `Serialize` and `Deserialize` for `KeyPair`. Improve the rustdocs for `KeyPair` by removing stale docs and adding docs on fixed width binary serialization.

We recently added fixed width serialization for some types however serialization is only fixed width when data is serialized with the `bincode` crate. Add rustdocs describing fixed width serde to `SecretKey`, `PublicKey`, and `XOnlyPublicKey` (`KeyPair` is already done).

Add a `tests` directory. Add `serde` tests for the recently added fixed width binary serialization code. Please note, serialization is only fixed width when serialized with the `bincode` crate.

tcharding · 2022-06-13T23:13:01Z

FTR I do not intend to push for this PR to be merged, I am unclear whether it is worth having or not. I will keep rebasing it to give others a chance to comment.

Kixunil · 2022-06-14T08:43:40Z

I think up to 25% space saving is worth breaking already unstable library. :)

apoelstra · 2022-06-15T15:07:46Z

I have mixed feelings, but concept ACK from me because @Kixunil is so persuasive.

Also, selfishly, I think that my personal applications will be unaffected because I always store keys in the ASCII serialization..

Will re-review and merge this shortly.

apoelstra

ACK 3ca7f49

tcharding force-pushed the fixed-width-serde branch from 64754a5 to 50ab92c Compare February 16, 2022 16:41

thomaseizinger reviewed Feb 16, 2022

View reviewed changes

src/serde_util.rs Outdated Show resolved Hide resolved

tcharding force-pushed the fixed-width-serde branch 6 times, most recently from 6e3e256 to d8a0399 Compare February 21, 2022 14:15

tcharding changed the title ~~Use fixed width serde impls for secret/public key~~ Use fixed width serde impls for key Feb 21, 2022

tcharding changed the title ~~Use fixed width serde impls for key~~ Use fixed width serde impls for keys Feb 21, 2022

tcharding force-pushed the fixed-width-serde branch 6 times, most recently from cd44138 to 4e8e095 Compare February 23, 2022 10:33

apoelstra mentioned this pull request Feb 23, 2022

update libsecp to latest ersion; bump major version number #384

Merged

tcharding force-pushed the fixed-width-serde branch from 4e8e095 to a58dc21 Compare February 24, 2022 09:15

tcharding marked this pull request as ready for review February 24, 2022 09:21

tcharding force-pushed the fixed-width-serde branch from a58dc21 to ba9e23d Compare February 28, 2022 11:25

Kixunil reviewed Mar 1, 2022

View reviewed changes

tcharding force-pushed the fixed-width-serde branch from ba9e23d to 4169e1f Compare March 1, 2022 18:22

Kixunil reviewed Mar 1, 2022

View reviewed changes

tcharding force-pushed the fixed-width-serde branch from 4169e1f to 330a18e Compare March 1, 2022 19:53

tcharding force-pushed the fixed-width-serde branch from 4e4f089 to a9a129c Compare March 8, 2022 02:48

Kixunil reviewed Mar 8, 2022

View reviewed changes

tcharding force-pushed the fixed-width-serde branch 2 times, most recently from f498e1b to 7ebb6a5 Compare March 8, 2022 21:56

tcharding force-pushed the fixed-width-serde branch from 7ebb6a5 to f11c1d6 Compare May 11, 2022 22:46

tcharding force-pushed the fixed-width-serde branch 2 times, most recently from f7eba2b to db656a5 Compare May 11, 2022 23:35

apoelstra previously approved these changes May 13, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 76d7cf3 June 9, 2022 05:27

tcharding force-pushed the fixed-width-serde branch from db656a5 to 76d7cf3 Compare June 9, 2022 05:27

tcharding added 4 commits June 9, 2022 16:09

Improve rustdocs for KeyPair

c28808c

Currently the rustdocs for `KeyPair` are stale in regards to serde, we _do_ implement `Serialize` and `Deserialize` for `KeyPair`. Improve the rustdocs for `KeyPair` by removing stale docs and adding docs on fixed width binary serialization.

Add fixed-width-serde integration tests

3ca7f49

Add a `tests` directory. Add `serde` tests for the recently added fixed width binary serialization code. Please note, serialization is only fixed width when serialized with the `bincode` crate.

tcharding force-pushed the fixed-width-serde branch from 76d7cf3 to 3ca7f49 Compare June 9, 2022 06:20

Kixunil mentioned this pull request Jun 10, 2022

Bump crate version to 0.23.0 #444

Merged

apoelstra approved these changes Jun 15, 2022

View reviewed changes

apoelstra merged commit 613d7dc into rust-bitcoin:master Jun 15, 2022

tcharding deleted the fixed-width-serde branch July 21, 2022 03:18

RCasatta mentioned this pull request Sep 30, 2022

bitcoin: bump 0.28.0 -> 0.29.1 ElementsProject/rust-elements#147

Merged

Use fixed width serde impls for keys #406

Use fixed width serde impls for keys #406

Conversation

tcharding commented Feb 16, 2022 • edited

apoelstra commented Mar 1, 2022

Kixunil left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Mar 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Mar 1, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Mar 1, 2022

tcharding commented Mar 1, 2022

Kixunil commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Mar 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Mar 8, 2022

tcharding commented May 11, 2022

tcharding commented May 11, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented May 15, 2022

apoelstra commented May 18, 2022

tcharding commented May 19, 2022

tcharding commented Jun 9, 2022

tcharding commented Jun 13, 2022

Kixunil commented Jun 14, 2022

apoelstra commented Jun 15, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Feb 16, 2022 •

edited

Kixunil left a comment •

edited

tcharding Mar 1, 2022 •

edited

tcharding Mar 8, 2022 •

edited