Serde regression tests #724

tcharding · 2021-11-25T23:52:56Z

Attempts to add regression tests for all types defined in rust-bitcoin that implement Serialize/Deserialize.

Add a tests directory and implement regression tests in there
Use files for input hex and output bincode to reduce source file clutter
Copy test block and include_bytes! usage from RCasatta's PR
Uses Kixunil's macro suggested below
Adds a single regression test to util/taproot.rs for private types

Note to reviewers

Uses JSON for opcodes in a separate file (tests/regression_opcodes.rs), for all other tests uses bincode.
Bypasses the order issue for maps by only serializing maps with a single element - is this correct?

Fixes #723

apoelstra · 2021-12-21T15:27:01Z

Cool! concept ACK.

Kixunil

Concept ACK

src/blockdata/opcodes.rs

tcharding · 2022-01-10T04:07:33Z

There are a few private types in util/taproot.rs and I'm not sure if we need to explicitly test them or if the hex used to create a ControlBlock is enough. I wrote the following test but Rust 1.29 build fails for some reason (cannot compare an array with a vector) even though all the integration tests are like this?

Do we need this one?


    // FIXME: Is this redundant because of serde_regression_taproot_builder() in tests/regression.rs?
    // TaprootMerkleBranch and NodeInfo are private and cannot be explicitly created in regression.rs.
    #[test]
    #[cfg(feature = "serde")]
    fn serde_regression_node_info() {
        use bincode::serialize;

        let s1 = Script::from(vec![0u8, 1u8, 2u8]);
        let s2 = Script::from(vec![3u8, 6u8, 9u8]);
        let ver = LeafVersion::from_consensus(0).unwrap();
        let mut l1 = LeafInfo::new(s1, ver);
        let mut l2 = LeafInfo::new(s2, ver);

        let h1 = sha256::Hash::from_str("cafec44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852babe").unwrap();
        let h2 = sha256::Hash::from_str("deadc44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852beef").unwrap();
        let b1 = TaprootMerkleBranch(vec![h1, h2]);
        let b2 = TaprootMerkleBranch(vec![h2, h1]);
        l1.merkle_branch = b1;
        l2.merkle_branch = b2;


        let info = NodeInfo {
            hash: sha256::Hash::from_str("cafeccafe8fc1c149afbf4c8996fb92427ae41e4649b934ca495991bbabebabe").unwrap(),
            leaves: vec![l1, l2],
        };

        let got = serialize(&info).unwrap();
        let want = [32, 0, 0, 0, 0, 0, 0, 0, 202, 254, 204, 175, 232, 252, 28, 20, 154, 251, 244, 200, 153, 111, 185, 36, 39, 174, 65, 228, 100, 155, 147, 76, 164, 149, 153, 27, 186, 190, 186, 190, 2, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 0, 0, 0, 0, 0, 0, 1, 2, 0, 2, 0, 0, 0, 0, 0, 0, 0, 32, 0, 0, 0, 0, 0, 0, 0, 202, 254, 196, 66, 152, 252, 28, 20, 154, 251, 244, 200, 153, 111, 185, 36, 39, 174, 65, 228,100, 155, 147, 76, 164, 149, 153, 27, 120, 82, 186, 190, 32, 0, 0, 0, 0, 0, 0, 0, 222, 173, 196, 66, 152, 252, 28, 20, 154, 251, 244, 200, 153, 111, 185, 36, 39, 174, 65, 228, 100, 155, 147, 76,164, 149, 153, 27, 120, 82, 190, 239, 3, 0, 0, 0, 0, 0, 0, 0, 3, 6, 9, 0, 2, 0, 0, 0, 0, 0, 0, 0, 32, 0, 0, 0, 0, 0, 0, 0, 222, 173, 196, 66, 152, 252, 28, 20, 154, 251, 244, 200, 153, 111, 185,36, 39, 174, 65, 228, 100, 155, 147, 76, 164, 149, 153, 27, 120, 82, 190, 239, 32, 0, 0, 0, 0, 0, 0, 0, 202, 254, 196, 66, 152, 252, 28, 20, 154, 251, 244, 200, 153, 111, 185, 36, 39, 174, 65, 228, 100, 155, 147, 76, 164, 149, 153, 27, 120, 82, 186, 190];
        assert_eq!(got, want)
    }

Kixunil · 2022-01-10T09:29:01Z

Private types shouldn't need serde impls. If they do (because something in the crate uses them inside) then tests are useful to pinpoint the source of problem if something goes wrong.

Change the array to slice to fix compile issue:

let want = &[32, 0 ...] as &[_];

RCasatta

I think we need one of the following:

move tests/bincode and tests/hex inside test_data
create tests/data and move test_data, tests/bincode and tests/hex inside it

tcharding · 2022-01-11T01:33:24Z

Private types shouldn't need serde impls. If they do (because something in the crate uses them inside) then tests are useful to pinpoint the source of problem if something goes wrong.

Cool, I think I'll leave this one out. Since we cannot guarantee that the regression tests test all the serde impls I think its probably better to just keep the public API tests, then they are all in one place. Also this test, as its written, won't help pin point any errors that much more than the ControlBlock test. Unless anyone thinks otherwise ...

Change the array to slice to fix compile issue:
let want = &[32, 0 ...] as &[_];

Oh nice, thanks.

tcharding · 2022-01-11T01:44:56Z

I think we need one of the following:

* move `tests/bincode` and `tests/hex` inside `test_data`

* create `tests/data` and move `test_data`, `tests/bincode` and `tests/hex` inside it

Good suggestion, I elected to move the files from test_data to a newly created tests/data/ directory.

Kixunil

Did only a light review. Since this is not very urgent we can afford a bit of bikesheding, right?

tests/regression.rs

tests/regression_opcodes.rs

tests/regression.rs

src/blockdata/block.rs

tcharding · 2022-01-11T22:33:13Z

Since this is not very urgent we can afford a bit of bikesheding, right?

Bikeshed all day! Thanks for the review, I learned a bunch from this review.

tests/serde.rs

tests/util.rs

apoelstra

ACK 3ef6935149892e6ab7c6320606f4fbe593d262bc

Kixunil

Can't do a full review RN just note that I'd prefer having short test strings (e.g. hex public key) inline in tests. At least unless they are some shared canonical test vectors. Wouldn't block the PR for that though.

Kixunil · 2022-08-29T13:08:36Z

tests/data/serde/README.md

+Files here contain hex strings and binary data representing types used for
+regression testing.
+
+- *_hex: consensus encoded types represented as hex strings


Why do we need _hex files? Shouldn't we test hex decoding separately?

TBH I'd rather drop the binary vectors than the hex ones.

Agreed that they're pretty-much redundant and we shouldn't duplicate everything.

hmm, I can't really remember why I put some in hex (I did the bulk of this work in November last year) but my guess was that I was lazy and just used hex strings when they appeared elsewhere in the codebase (the binary stuff I generated for each type manually). I do remember going through at the end and moving the hex stuff to files instead of inline. My reasoning was that the data is not meaningful so better to conceal it in a file and keep the test source code clean. I can't remember why I used binary instead of hex for the manually generated types, perhaps because we have the two blocks there in tests/data that are in binary - if we move to hex we should be uniform. We exclude the tests/ directory in Cargo.toml so data size is not a consideration. I do not know what other trade offs there are between the two?

Hex helps reviewers see the data without using hexdump and on github. But is it worth complicating the test code that now has to parse hex too? Maybe with my hex_lit macro it wouldn't be so bad but still, if someone makes a mistake in hex parsing impl he'll get tons of unrelated errors.

I think it's fine -- usually they'll just be "parsing" the hex to a Vec<u8> then parsing from there as though it were binary data. And hex->byte vector is write-once code.

We should never need the hex crate.

We should probably add a deserialize_hex method analogous to serialize_hex which uses the bitcoin_hashes::hex module.

I don't get it, we are explicitly trying to test the serde implementations, we have to use a crate serializes using serde to do so, right?

I was not aware that hex uses serde. What serialization format does it use?

Looks like it just serializes as ascii chars: https://docs.rs/hex/latest/hex/serde/fn.serialize.html

It's simple enough for us to NIH it.

sanket1729

reACK 3ef6935149892e6ab7c6320606f4fbe593d262bc

apoelstra · 2022-09-02T13:56:24Z

We have two ACKs but I'm not clear whether we want to modify this PR to answer "what should we be doing about hex vs binary" question here.

tcharding · 2022-09-05T23:43:52Z

I'm not clear whether we want to modify this PR to answer "what should we be doing about hex vs binary" question here.

I am unclear also.

Kixunil · 2022-09-14T10:19:04Z

When in doubt I usually prefer "make some progress unless this affects API". This already improves things from previous state of the code and we can resolve the remaining issues later.

Unfortunately this needs rebase.

tcharding · 2022-09-15T00:32:12Z

Rebased and used qualified path for LockTime and PackedLockTime.

apoelstra

ACK 9a4c79a390bcedcb78c65a5688f269c36f090823

tcharding · 2022-10-25T00:44:00Z

Rebased and added tests for Work, Target, relative::LockTime types (Height and Time incl.)

tcharding · 2022-10-25T00:44:53Z

FTR I have no clue if this covers all types since its been open for so long, I'm not super enthusiastic about auditing again while the PR is open, if/when it merges I can re-audit the whole codebase though.

apoelstra

ACK 16fd5f6e8160317b7053b74dc12a1692f1d94475

In order that we can safely change/maintain de/serialization code we need to have regression tests with hard coded serializations for each type that implements serde. It is enough to test a single serde data format, use JSON for `opcodes` and bincode for other types. Do regression testing in a newly added `tests` module.

tcharding · 2022-10-28T23:56:34Z

Re-base and remove the patch to move test data since its on master already.

apoelstra

ACK 962abcc

sanket1729

ACK 962abcc. This has been open for a long time. Merging this in the interest of progress.

tcharding mentioned this pull request Nov 25, 2021

Add regression tests for all serde serializations #723

Closed

Kixunil reviewed Dec 21, 2021

View reviewed changes

src/blockdata/opcodes.rs Outdated Show resolved Hide resolved

tcharding force-pushed the serde-tests branch 4 times, most recently from 100b5d5 to ac6552f Compare January 10, 2022 03:55

tcharding marked this pull request as ready for review January 10, 2022 04:07

tcharding requested review from Kixunil and RCasatta January 10, 2022 04:07

RCasatta reviewed Jan 10, 2022

View reviewed changes

apoelstra mentioned this pull request Jan 10, 2022

Serde serialization vs consensus serialization. #765

Open

Kixunil reviewed Jan 11, 2022

View reviewed changes

tcharding force-pushed the serde-tests branch 5 times, most recently from 7db1726 to 95356d1 Compare January 12, 2022 08:47

Kixunil reviewed Jan 12, 2022

View reviewed changes

tests/serde.rs Outdated Show resolved Hide resolved

tcharding force-pushed the serde-tests branch 2 times, most recently from 9a86e87 to ac33d08 Compare January 13, 2022 03:10

Kixunil reviewed Jan 13, 2022

View reviewed changes

tests/util.rs Outdated Show resolved Hide resolved

Kixunil reviewed Jan 13, 2022

View reviewed changes

tests/util.rs Outdated Show resolved Hide resolved

tcharding force-pushed the serde-tests branch 2 times, most recently from 35eba47 to 80d9879 Compare January 19, 2022 03:48

tcharding dismissed apoelstra’s stale review via 3ef6935 August 25, 2022 22:33

tcharding force-pushed the serde-tests branch from 33ca78c to 3ef6935 Compare August 25, 2022 22:33

tcharding added this to the 0.30.0 milestone Aug 26, 2022

apoelstra previously approved these changes Aug 26, 2022

View reviewed changes

Kixunil reviewed Aug 29, 2022

View reviewed changes

sanket1729 previously approved these changes Sep 1, 2022

View reviewed changes

tcharding dismissed stale reviews from sanket1729 and apoelstra via 9a4c79a September 15, 2022 00:31

tcharding force-pushed the serde-tests branch from 3ef6935 to 9a4c79a Compare September 15, 2022 00:31

apoelstra previously approved these changes Sep 15, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 75fff7a October 25, 2022 00:43

tcharding force-pushed the serde-tests branch from 9a4c79a to 75fff7a Compare October 25, 2022 00:43

tcharding force-pushed the serde-tests branch 2 times, most recently from 458add6 to 16fd5f6 Compare October 25, 2022 03:06

apoelstra previously approved these changes Oct 25, 2022

View reviewed changes

tcharding mentioned this pull request Oct 25, 2022

Move test_data/* to tests/data #1347

Closed

tcharding dismissed apoelstra’s stale review via 962abcc October 28, 2022 23:56

tcharding force-pushed the serde-tests branch from 16fd5f6 to 962abcc Compare October 28, 2022 23:56

apoelstra approved these changes Nov 4, 2022

View reviewed changes

sanket1729 approved these changes Dec 1, 2022

View reviewed changes

sanket1729 merged commit df57a19 into rust-bitcoin:master Dec 1, 2022

tcharding deleted the serde-tests branch December 4, 2022 22:05

Serde regression tests #724

Serde regression tests #724

Uh oh!

Conversation

Uh oh!

Note to reviewers

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!