Script serialization/deserialization in serde non-human-readable format#596
Script serialization/deserialization in serde non-human-readable format#596dr-orlovsky merged 2 commits intorust-bitcoin:masterfrom
Conversation
thomaseizinger
left a comment
There was a problem hiding this comment.
It would be nice to have format independent tests that additionally cover all the different input forms as well (borrowed vs owned data).
src/blockdata/script.rs
Outdated
There was a problem hiding this comment.
serde-test would allow you to test the specifics of human-readable / non-human-readable serialization without having to choose a particular format. At the same time, it also allows you to test the various overloads (visit_string, visit_str, etc).
Not testing these aspects caused a nasty bug that was only fixed recently in rust-secp256k1.
I think our use of custom Serialize/Deserialize implementations warrants more thorough testing to make sure they are actually correct.
There was a problem hiding this comment.
I agree, but seems out of scope a bit. Would be nice to generally do this and not just in one place.
There was a problem hiding this comment.
A long time ago I tried using serde-test. I recall spending absolutely forever fighting its model of borrowed vs owned data and then finding that this didn't even match serde-json's model so that all my effort making serde-test work was useless because the encoding was still broken.
There was a problem hiding this comment.
Oh, that is interesting. Thanks for sharing!
I had a good experience with it when ironing out the serialization bugs in rust-secp which was precisely about owned vs borrowed data. That is why I mentioned it here.
There was a problem hiding this comment.
tACK bac186ce021b00309e1b6c3ab82acd50122d4eff
Test log:
Apr 27 22:28:47.915 INFO testinator: Installing rust toolchain 'nightly'
Apr 27 22:29:19.953 INFO testinator: Installing rust toolchain 'stable'
Apr 27 22:29:20.543 INFO testinator: Installing rust toolchain '1.29.0'
Apr 27 22:29:21.725 INFO testinator: Preparing environment for rust stable tests (8 configurations)
Apr 27 22:29:21.725 INFO testinator: Preparing environment for rust nightly tests (9 configurations)
Apr 27 22:29:21.726 INFO testinator: Preparing environment for rust 1.29.0 tests (8 configurations)
Apr 27 22:32:17.622 INFO testinator: Running rust stable tests in /tmp/rust-bitcoin-stable.2EM87r7uno4c/rust-bitcoin
Apr 27 22:32:17.625 INFO testinator: Running rust 1.29.0 tests in /tmp/rust-bitcoin-1.29.0.bF8jXIWdsRy1/rust-bitcoin
Apr 27 22:32:17.625 DEBUG testinator: Generating lock file with rust=1.29.0
Apr 27 22:32:17.627 INFO testinator: Running rust nightly tests in /tmp/rust-bitcoin-nightly.iN36gSQTxCUw/rust-bitcoin
Apr 27 22:32:22.469 DEBUG testinator: Pinning cc to 1.0.41
Apr 27 22:32:22.605 DEBUG testinator: Pinning serde to 1.0.98
Apr 27 22:32:22.735 DEBUG testinator: Pinning serde_derive to 1.0.98
Apr 27 22:32:22.855 DEBUG testinator: Pinning byteorder to 1.3.4
Apr 27 22:33:22.537 INFO testinator: Test rust=nightly, features=[secp-recovery] succeeded!
Apr 27 22:33:39.914 INFO testinator: Test rust=stable, features=[secp-recovery] succeeded!
Apr 27 22:34:12.065 INFO testinator: Test rust=1.29.0, features=[secp-recovery] succeeded!
Apr 27 22:34:23.907 INFO testinator: Test rust=nightly, features=[use-serde] succeeded!
Apr 27 22:34:42.622 INFO testinator: Test rust=stable, features=[use-serde] succeeded!
Apr 27 22:34:51.587 INFO testinator: Test rust=nightly, features=[base64] succeeded!
Apr 27 22:35:13.978 INFO testinator: Test rust=stable, features=[base64] succeeded!
Apr 27 22:35:15.442 INFO testinator: Test rust=nightly, features=[secp-lowmemory] succeeded!
Apr 27 22:35:49.065 INFO testinator: Test rust=nightly, features=[rand] succeeded!
Apr 27 22:35:52.181 INFO testinator: Test rust=stable, features=[secp-lowmemory] succeeded!
Apr 27 22:36:18.400 INFO testinator: Test rust=nightly, features=[unstable] succeeded!
Apr 27 22:36:19.274 INFO testinator: Test rust=1.29.0, features=[use-serde] succeeded!
Apr 27 22:36:20.871 INFO testinator: Test rust=stable, features=[rand] succeeded!
Apr 27 22:36:56.034 INFO testinator: Test rust=nightly, features=[bitcoinconsensus] succeeded!
Apr 27 22:37:10.447 INFO testinator: Test rust=stable, features=[bitcoinconsensus] succeeded!
Apr 27 22:37:19.061 INFO testinator: Test rust=1.29.0, features=[base64] succeeded!
Apr 27 22:37:33.771 INFO testinator: Test rust=nightly, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,unstable,bitcoinconsensus] succeeded!
Apr 27 22:38:00.851 INFO testinator: Test rust=nightly, features=[] succeeded!
Apr 27 22:38:03.322 INFO testinator: Test rust=stable, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!
Apr 27 22:38:33.628 INFO testinator: Test rust=1.29.0, features=[secp-lowmemory] succeeded!
Apr 27 22:38:33.977 INFO testinator: Test rust=stable, features=[] succeeded!
Apr 27 22:39:22.560 INFO testinator: Test rust=1.29.0, features=[rand] succeeded!
Apr 27 22:40:13.605 INFO testinator: Test rust=1.29.0, features=[bitcoinconsensus] succeeded!
Apr 27 22:41:07.957 INFO testinator: Test rust=1.29.0, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!
Apr 27 22:41:55.166 INFO testinator: Test rust=1.29.0, features=[] succeeded!
Apr 27 22:41:58.979 INFO testinator: Fuzzing deserialize_script
Apr 27 22:43:14.208 INFO testinator: Successfully fuzzed deserialize_script
Apr 27 22:43:14.208 INFO testinator: Fuzzing uint128_fuzz
Apr 27 22:44:16.182 INFO testinator: Successfully fuzzed uint128_fuzz
Apr 27 22:44:16.182 INFO testinator: Fuzzing deserialize_amount
Apr 27 22:45:17.195 INFO testinator: Successfully fuzzed deserialize_amount
Apr 27 22:45:17.195 INFO testinator: Fuzzing deserialize_transaction
Apr 27 22:46:19.178 INFO testinator: Successfully fuzzed deserialize_transaction
Apr 27 22:46:19.178 INFO testinator: Fuzzing deser_net_msg
Apr 27 22:47:22.230 INFO testinator: Successfully fuzzed deser_net_msg
Apr 27 22:47:22.230 INFO testinator: Fuzzing deserialize_address
Apr 27 22:48:23.059 INFO testinator: Successfully fuzzed deserialize_address
Apr 27 22:48:23.059 INFO testinator: Fuzzing deserialize_block
Apr 27 22:49:25.213 INFO testinator: Successfully fuzzed deserialize_block
Apr 27 22:49:25.213 INFO testinator: Fuzzing outpoint_string
Apr 27 22:50:27.057 INFO testinator: Successfully fuzzed outpoint_string
Apr 27 22:50:27.057 INFO testinator: Fuzzing deserialize_psbt
Apr 27 22:51:30.131 INFO testinator: Successfully fuzzed deserialize_psbt
|
I expect this is an API breaking change, since data serialized with previous version in binary format will not be deserializable after this PR by the same code deserializing it before |
|
Needs rebase. concept ACK |
514052d to
3ccfb09
Compare
|
rebased |
sanket1729
left a comment
There was a problem hiding this comment.
utack 3ccfb09be257a21909926ce78a43034a37b136f7
|
Again needs rebase :( |
|
rebased |
|
Merging since all requirements are met and we was not merging it only to wait for a major version |
close #595
At the moment
Scriptfor serde non-human-readable format is serialized as bytes of the hex format, doubling the space requiredThis fix by serializing in bytes if the encoding format is non-human-readable (such as using bincode or serde_cbor)