Script serialization/deserialization in serde non-human-readable format #596
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RCasatta
changed the title
dr-orlovsky
previously approved these changes
Apr 26, 2021
Comment on lines
+1272
to
+1305
| let json = ::serde_json::to_string(&script).unwrap(); | ||
| assert_eq!(json, "\"000102\""); | ||
| let bincode = ::bincode::serialize(&script).unwrap(); |
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.
sgeisler
previously approved these changes
Apr 27, 2021
There was a problem hiding this comment.
tACK bac186c
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 |
sgeisler
added
the
API break
|
Needs rebase. concept ACK |
RCasatta
force-pushed
the
script_ser
branch
2 times, most recently
from
514052d
to
3ccfb09
Compare
|
rebased |
|
Again needs rebase :( |
|
rebased |
dr-orlovsky
approved these changes
Jun 16, 2021
apoelstra
approved these changes
Jul 8, 2021
|
Merging since all requirements are met and we was not merging it only to wait for a major version |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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)