Add/improve tests for serialization and commitment compatibility #1486
Conversation
* Add round-trip serialization check * Print out a valid serialization if the test fails, so that we can more easily update the test vectors when we change data structures * Generate test vectors from Rust objects * Add tests for ChainConfig, FeeInfo
jbearer
requested review from
nomaxg,
philippecamacho,
ImJeremyHe,
sveitser and
tbro
as code owners
| // reference. | ||
| let actual = serde_json::to_value(&messages).unwrap(); | ||
| let expected: Value = serde_json::from_str(include_str!("../../data/messages.json")).unwrap(); | ||
| assert_eq!( |
There was a problem hiding this comment.
I think we can make the this a bit more convenient for our future selves. For example we could use https://github.com/rust-pretty-assertions/rust-pretty-assertions, with that if we compare the json strings instead of the Values the output looks pretty nice and indicates where the difference is.
However the output is still pretty big, so we could also write the new JSON out to a file next to messages.json which makes it easier to manually diff it and update the file in the repo if desired.
| // Ensure the current serialization implementation generates the same JSON as the committed | ||
| // reference. | ||
| let actual = serde_json::to_value(&messages).unwrap(); | ||
| let expected: Value = serde_json::from_str(include_str!("../../data/messages.json")).unwrap(); |
There was a problem hiding this comment.
Is there any benefit to including the json file in the test binary here?
There was a problem hiding this comment.
Yeah if we load it at runtime then we have to run the binary from the root of the repo. If you e.g. run cargo test from the sequencer sub-directory you would get a confusing failue
There was a problem hiding this comment.
Actually I guess if I use CARGO_MANIFEST_DIR then the only requirement is you run the tests on the same machine where they were built...which seems reasonable for tests. I can do that to make the binary smaller/faster to build
| let expected: Value = serde_json::from_slice(&expected_bytes).unwrap(); | ||
|
|
||
| // Check that the reference object matches the expected serialized form. | ||
| let actual = serde_json::to_value(&reference).unwrap(); |
There was a problem hiding this comment.
Is there a reason to use serde_json rather than e.g. vbs::Serializer::<hotshot_types::constants::Version01>::serialize that HotShot uses?
It makes the committed file human-readable, but if I understand correctly, I think only checking roundtrip json serialization would not catch a lot of the more subtle errors (e.g. reordering but not renaming enum variants)
There was a problem hiding this comment.
Hmm that's a good point. It is probably a good idea to also check vbs. We can check JSON first so that if your change happens to be caught there, you get a readable error message.
No issue, this is necessary now that we are supporting a deployed network (Cappuccino). We need to avoid breaking changes or at least be conscious of when we are making them.
This PR:
This PR does not:
Catch all possible instances of compatibility breaking changes. The checks in here are based on serialization of specific instantiations of various data structures. Compatibility breakages that only affect variants/structures that are not specifically tested will not trigger a failure. Short term, a combination of these tests and running in staging with multiple different versions at the same time should be sufficient to catch the most common and severe breakages. Long term, we need a deterministic schema for all serialized data structures so we can symbolically check every possible variant for breaking changes.
Key places to review:
Things tested
These tests pass on the release tag used for Cappuccino (
20240517). Thus keeping these tests passing without changes should indicate compatibility with the code that is currently deployed. Branch with these tests cherry-picked on top of20240517: https://github.com/EspressoSystems/espresso-sequencer/tree/jb/backport-ser-test