Rebased version of `[Alternative] Allow deserializing from owned types` + support for new schnorr module #270

thomaseizinger · 2021-01-12T01:07:46Z

This PR is a rebase of #218 that uses the new visitor module for schnorr signatures and public keys.

The way we call the visitor module is slightly different in all the call-sites:

sometimes we pass the bytes slice by indexing: &self[..]
sometimes we call serialize()
sometimes we call serialize_der()

For the deserialization, all the expectation strings are obviously different.

It would be possible to work around these differences in a macro and thereby reduce the code duplication. However, I hesitated to do so because it creates even more indirection which makes it IMO harder to understand what is going on. In the end, the implementations are only forwarding to the visitor module.

I decided to squash all commits of #218 together to make sure it is still an atomic commit. This one also uses the new visitor module for the newly added schnorr keys and signatures. 1f08a31 therefore represents the state of #218 modulo the schnorr additions.

The remaining two patches are minor refactorings and additional tests on top of this.

Co-authored-by: Elichai Turkel <elichai.turkel@gmail.com> Co-authored-by: Sebastian Geisler <sebastian@blockstream.io>

These tests are testing more than just the signature serialization.

elichai · 2021-01-12T09:02:24Z

I guess serialization of KeyPair is blocked by bitcoin-core/secp256k1#845

sgeisler

Thank you for taking the time to rework this PR, this seems to have become a real team effort ^^ I think removing the macros is nice for readability and jumping around in the source and comes at a low enough price in LOC.

src/serde_util.rs

The visitor works with all types that implement `FromStr`. Whether or not that ends up being hex encoding depends on the implementation of `FromStr`.

This visitor is meant to deserialize strings using `FromStr` not bytes.

thomaseizinger · 2021-01-12T23:17:25Z

@sgeisler @elichai I renamed the visitor to FromStrVisitor which seems more appropriate.

While doing that, I noticed that FromStrVisitor overrides visit_bytes which IMO is not correct. Removing it shows that we don't have any tests that cover this implementation so I went ahead and committed that as well. Any objections?

tcharding

Except for the long paths, and for what its worth, looks good to me.

tcharding · 2021-01-13T03:11:37Z

src/key.rs

+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(


Perhaps remove super:: and add a use statement that imports using the fully qualified path (although I would remove serde_util:: also). Then we can remove super:: at no additional cost all the way through this PR. (I'm still singing the same old tune :)

I am not sure how useful a use statement will be.

This code is feature-gated so I would either have to feature-gate the use statement at the top of the file as well or accept that rustc will issue a dead-code warning if we compile without the serde feature flag.

For feature-gated code, I usually err on the side of having full-qualified paths, esp. if they only occur once within the code that is gated.

We could potentially include a use super::serde_util; at the beginning of the function and then reference both FromStrVisitor and BytesVisitor as serde_util::FromStrVisitor.

Thanks for the response, I didn't catch the feature gated nuance when I commented. Not sure which part of the comment @sgeisler is ok'ing but I like use statement inside the function option. Its pretty minor though, so don't sweat it :)

thomaseizinger · 2021-02-08T01:30:37Z

@apoelstra friendly ping to review this please :)

sgeisler

utACK c2fd5ce

Something missing here that should ideally be discussed as a separate PR before the next release since it would probably derail this PR again: schnorr secret key serialization.

thomaseizinger · 2021-02-08T23:18:38Z

utACK c2fd5ce

Something missing here that should ideally be discussed as a separate PR before the next release since it would probably derail this PR again: schnorr secret key serialization.

Yes please! This PR fixes a bug whereas schnorr secret key serialization is "simply" a missing feature.

We just happen to touch the schnorr module in these patches to remain moderately consistent :)

apoelstra

ack c2fd5ce

This looks great! Thanks for removing the macros. This is much cleaner than what we had before (though a bit more repetitive).

I think merging should be blocked on #288 because the build is broken in current Rust nightly and we need to fix that.

elichai · 2021-04-07T15:24:32Z

src/key.rs

+            d.deserialize_bytes(super::serde_util::BytesVisitor::new(
+                "a bytestring representing a public key",
+                PublicKey::from_slice


Isn't this a breaking change? currently bytes will deserialize as hex: https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/macros.rs#L116-L125

(I'm not saying it's bad, just trying to know if this might break downstream users)

I don't think it is!

The lines you commented on are within the else branch of d.is_human_readable whereas the code you linked to is within d.is_human_readable.

Also, I think the linked piece of code was never actually called because I deleted a legacy of it in c2fd5ce and all tests still pass!

hmm I see, you're right.

thomaseizinger · 2021-04-09T01:45:25Z

Now that #288 is closed as a result of #290 being merged, any chance we can get this in? :)

elichai

Verified that this produces the same result as before and deserializes to the same result on these serializations:

serde_json
bincode
serde_cbor
serde_yaml
rmp-serde
toml
serde-pickle
ron
bson
json5
flexbuffers

This also fixed deserialization for these:

serde_yaml
serde-pickle
json5
bson

The code I've used to verify: https://github.com/elichai/rust-secp256k1-serde-test

elichai and others added 3 commits January 12, 2021 11:51

Replace serde macros with generic visitor module

1f08a31

Co-authored-by: Elichai Turkel <elichai.turkel@gmail.com> Co-authored-by: Sebastian Geisler <sebastian@blockstream.io>

Update test names to reflect what they are doing

b4040f0

These tests are testing more than just the signature serialization.

Extend schnorr-sig serde test suite with owned and slice variants

18890d3

thomaseizinger mentioned this pull request Jan 12, 2021

[Alternative] Allow deserializing from owned types #218

Closed

sgeisler previously approved these changes Jan 12, 2021

View reviewed changes

src/serde_util.rs Outdated Show resolved Hide resolved

thomaseizinger added 2 commits January 13, 2021 09:54

Rename HexVisitor to FromStrVisitor

e6e23e9

The visitor works with all types that implement `FromStr`. Whether or not that ends up being hex encoding depends on the implementation of `FromStr`.

Remove visit_bytes implementation from FromStrVisitor

c2fd5ce

This visitor is meant to deserialize strings using `FromStr` not bytes.

thomaseizinger dismissed sgeisler’s stale review via c2fd5ce January 12, 2021 22:58

tcharding reviewed Jan 13, 2021

View reviewed changes

thomaseizinger mentioned this pull request Jan 14, 2021

Fix Optional Amount Serialization rust-bitcoin/rust-bitcoin#552

Merged

sgeisler approved these changes Feb 8, 2021

View reviewed changes

apoelstra approved these changes Mar 12, 2021

View reviewed changes

thomaseizinger mentioned this pull request Mar 14, 2021

Cannot let warp handle deserialisation of CreateSwapPayload comit-network/waves#83

Open

elichai reviewed Apr 7, 2021

View reviewed changes

elichai approved these changes Apr 21, 2021

View reviewed changes

elichai merged commit 3c2bee3 into rust-bitcoin:master Apr 21, 2021

apoelstra mentioned this pull request Apr 27, 2021

release secp256k1 0.20.2 #297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebased version of `[Alternative] Allow deserializing from owned types` + support for new schnorr module #270

Rebased version of `[Alternative] Allow deserializing from owned types` + support for new schnorr module #270

thomaseizinger commented Jan 12, 2021 •

edited

Loading

elichai commented Jan 12, 2021

sgeisler left a comment

thomaseizinger commented Jan 12, 2021

tcharding left a comment

tcharding Jan 13, 2021

thomaseizinger Jan 13, 2021

tcharding Jan 18, 2021

thomaseizinger commented Feb 8, 2021

sgeisler left a comment

thomaseizinger commented Feb 8, 2021

apoelstra left a comment

elichai Apr 7, 2021

thomaseizinger Apr 7, 2021

elichai Apr 8, 2021

thomaseizinger commented Apr 9, 2021

elichai left a comment

Rebased version of [Alternative] Allow deserializing from owned types + support for new schnorr module #270

Rebased version of [Alternative] Allow deserializing from owned types + support for new schnorr module #270

Conversation

thomaseizinger commented Jan 12, 2021 • edited Loading

elichai commented Jan 12, 2021

sgeisler left a comment

Choose a reason for hiding this comment

thomaseizinger commented Jan 12, 2021

tcharding left a comment

Choose a reason for hiding this comment

tcharding Jan 13, 2021

Choose a reason for hiding this comment

thomaseizinger Jan 13, 2021

Choose a reason for hiding this comment

tcharding Jan 18, 2021

Choose a reason for hiding this comment

thomaseizinger commented Feb 8, 2021

sgeisler left a comment

Choose a reason for hiding this comment

thomaseizinger commented Feb 8, 2021

apoelstra left a comment

Choose a reason for hiding this comment

elichai Apr 7, 2021

Choose a reason for hiding this comment

thomaseizinger Apr 7, 2021

Choose a reason for hiding this comment

elichai Apr 8, 2021

Choose a reason for hiding this comment

thomaseizinger commented Apr 9, 2021

elichai left a comment

Choose a reason for hiding this comment

Rebased version of `[Alternative] Allow deserializing from owned types` + support for new schnorr module #270

Rebased version of `[Alternative] Allow deserializing from owned types` + support for new schnorr module #270

thomaseizinger commented Jan 12, 2021 •

edited

Loading