Fix the PSBT JSON serialization, using serde-derive by stevenroose · Pull Request #508 · rust-bitcoin/rust-bitcoin

stevenroose · 2020-10-25T18:37:34Z

This re-introduces serde-derive, but adds version pinning instructions to the README.

I hope that's fine. I failed to add hex support for the values of maps (like partial_sigs: BTreeMap<PublicKey, Vec<u8>>). For that I fear we'd need some helper crate like serde_with. Even though, I'm sure in theory it's possible to do manually, but I couldn't figure it out.

dr-orlovsky

This looks good since gets rid of a lot of serde impl macros! utACK

apoelstra · 2020-12-21T15:53:22Z

src/util/psbt/mod.rs

trailing whitespace here

This line also doesn't compile for me

cargo +stable test '--features=rand use-serde base64' # 2020-12-21T16:02:46 / cargo 1.48.0 (65cbdd2dc 2020-10-14) Command failed: cargo +stable test --features=rand use-serde base64 Compiling serde_json v1.0.44 Compiling serde_test v1.0.118 Compiling bitcoin v0.25.2 (/tmp/tmpwjnlqhmp) error[E0433]: failed to resolve: use of undeclared crate or module `bip32` --> src/util/psbt/mod.rs:356:31 | 356 | let xpub: bip32::ExtendedPubKey = | ^^^^^ use of undeclared crate or module `bip32` error: aborting due to previous error

Instead, implement Debug in impl_bytes_newtype.

src/serde_utils.rs

apoelstra · 2020-12-21T21:07:40Z

README.md

I don't think there's any reason to downgrade serde. I am testing on 1.29 fine without it.

If I drop that line, I get this:

error: no matching version `= 1.0.118` found for package `serde_derive` location searched: registry `https://github.com/rust-lang/crates.io-index` versions found: 1.0.98 required by package `serde v1.0.118` ... which is depended on by `bitcoin v0.25.2 (file:///home/steven/code/rust/bitcoin)`

Because a higher serde will require a higher serde_derive. Somehow Cargo is not very smart about all that and it can't realize that it has a pin on _derive and then look for a matching serde.

The script I use is

set -e MSRV="1.29.0" CMD="rustup run ${MSRV}" rm -f Cargo.lock $CMD cargo generate-lockfile $CMD cargo update --package "cc" --precise "1.0.41" $CMD cargo update --package "serde" --precise "1.0.98" $CMD cargo update --package "serde_derive" --precise "1.0.98" $CMD cargo test --features "use-serde rand base64"

Perhaps we should commit that.

None of my scripts pin serde and everything works fine for me.

Oh, I'm lying, it's just that my scripts never made any progress on this PR because of the other compilation issue.

I just tried and this worked:
rm -rf target/ && rm Cargo.lock && cargo +1.29.0 test

@elichai you have to add --features=use-serde to see the failure

sgeisler · 2020-12-22T12:55:45Z

src/serde_utils.rs

One slightly annoying consequence of directly serializing the tuple instead of a wrapper is that the value is serialized as array of u8s instead of hex like everything else.

I think this is more than slightly annoying but it's also really unclear how to fix it.

It might be worthwhile to duplicate this entire module, changing U to Vec<u8> and doing hex-serialization.

Could we get away with having a Preferred(De)Serialize trait which is identical to the (De)Serialize traits and has a default impl just passing through to the serde version? But for certain types we can implement default (de)serialization methods.

stevenroose · 2020-12-27T23:10:55Z

Pushed a few more changes:

only do the special treatment for human-readable serializers (JSON)
add special module for BTreeMap<K, Vec<u8>> where the K is serialized as a string and the value is hex
add special module for BTreeMap<K, Vec<u8>> where the K is NOT serialized as a string and the value is hex

So that we can later distinguish other modules over maps.

Non-human-readable serialization (binary) doesn't need hexification.

apoelstra · 2020-12-28T23:13:23Z

src/util/psbt/mod.rs

            }],
        };
        let encoded = ::serde_json::to_string(&psbt).unwrap();
+        println!("encoded PSBT: {}", encoded);


debug printf

Is that a problem? It's in a unit test, so it will only be printed when used with --nocapture. I can remove it if it's a problem. When changing any of the PSBT fields, this helps looking at how the JSON actually looks.

(F.e. the Transactioin.witness field still is Vec<Vec<u8>> which doesn't print as hex. I tried fixing that too, but I got tired of fighting serde for a while, might do that in another PR.)

Yeah, that's fair, you can leave it in.

apoelstra

ack 3fd94ef

sgeisler

utACK 3fd94ef

This was referenced Oct 25, 2020

PSBT serde implementation #497

Merged

Change Cargo.toml serde dev deps #495

Closed

Add JsonSchema Support for Several Types #501

Closed

apoelstra mentioned this pull request Nov 6, 2020

Fixing serde impl for PSBT inputs #512

Merged

stevenroose force-pushed the fix-json-psbt branch from e51969b to b427ed7 Compare December 21, 2020 12:13

dr-orlovsky mentioned this pull request Dec 21, 2020

bump version to 0.26 #533

Merged

dr-orlovsky previously approved these changes Dec 21, 2020

View reviewed changes

apoelstra reviewed Dec 21, 2020

View reviewed changes

Remove the impl_array_newtype_show macro

1f65c19

Instead, implement Debug in impl_bytes_newtype.

stevenroose dismissed dr-orlovsky’s stale review via 27654a5 December 21, 2020 16:51

stevenroose force-pushed the fix-json-psbt branch from b427ed7 to 27654a5 Compare December 21, 2020 16:51

sgeisler reviewed Dec 21, 2020

View reviewed changes

src/serde_utils.rs Outdated Show resolved Hide resolved

src/serde_utils.rs Outdated Show resolved Hide resolved

apoelstra reviewed Dec 21, 2020

View reviewed changes

stevenroose force-pushed the fix-json-psbt branch 2 times, most recently from ca41df6 to e5f285f Compare December 22, 2020 12:46

stevenroose added 2 commits December 22, 2020 12:51

Replace serde_struct_impl with derive-based impls

e406afe

psbt::raw: Use hex::format_hex to format the raw key bytes

a9445dd

sgeisler reviewed Dec 22, 2020

View reviewed changes

stevenroose force-pushed the fix-json-psbt branch from e5f285f to f54dcbb Compare December 27, 2020 23:08

stevenroose and others added 6 commits December 27, 2020 23:14

Add serde_utils module to fix JSON serialization

8bc6565

psbt: Fix serde for maps with non-string keys and binary values

e308f64

serde_util: Rename the btreemap module to btreemap_as_seq

f6a734a

So that we can later distinguish other modules over maps.

psbt: Serialize maps with byte values using hex

429e46c

serde_utils: Only use special serialization for human-readable

f54dcbb

Non-human-readable serialization (binary) doesn't need hexification.

ci: pin serde and serde-derive for Rust 1.29.0

3fd94ef

apoelstra force-pushed the fix-json-psbt branch from 816d98e to 3fd94ef Compare December 28, 2020 19:43

apoelstra reviewed Dec 28, 2020

View reviewed changes

apoelstra approved these changes Dec 29, 2020

View reviewed changes

sgeisler approved these changes Dec 29, 2020

View reviewed changes

sgeisler merged commit ed9856f into rust-bitcoin:master Dec 30, 2020

Conversation

stevenroose commented Oct 25, 2020

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgeisler Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenroose commented Dec 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sgeisler left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sgeisler Dec 22, 2020 •

edited

Loading