Test that consensus emits valid CBOR #323

dnadales · 2023-08-25T07:51:16Z

Check that the encoders used in the serialization roundtrip are valid.
Add a unit test to check that the encoders used to encode the Cardano examples are valid.

Closes IntersectMBO/ouroboros-network#3099.

jorisdral · 2023-08-25T08:50:56Z

ouroboros-consensus/src/unstable-consensus-testlib/Test/Util/Serialisation/Roundtrip.hs

+        bs    = toLazyByteString enc_a
+
+    validFlatTerm (toFlatTerm enc_a)             ?! "Encoded flat term is not valid: " <> show enc_a
+    (bsRem, a' ) <- deserialiseFromBytes dec bs `onError` showByteString bs


Would it make sense to use fromFlatTerm instead of going through bytestrings? Or maybe do both to be extra sure we are catching all bugs?

jorisdral · 2023-08-25T09:03:52Z

ouroboros-consensus-cardano/test/cardano-test/Test/Consensus/Cardano/Serialisation.hs

+encodersProduceValidCBOR ::
+     forall blk . (SerialiseDiskConstraints blk)
+  => CodecConfig blk
+  -> Examples blk
+  -> TestTree


Maybe this function could go into the consensus testlib

Yeah, then it could also be re-used in Test.Consensus.{Byron,Shelley}.Serialisation 👍

jorisdral · 2023-08-28T12:25:13Z

ouroboros-consensus-cardano/test/shelley-test/Test/Consensus/Shelley/Serialisation.hs

 import           Test.Util.Serialisation.Roundtrip

+


Redundant newline

jorisdral · 2023-08-28T12:30:43Z

ouroboros-consensus/src/unstable-consensus-testlib/Test/Util/Serialisation/Golden.hs

+  -> TestTree
+encodersProduceValidCBOR codecConfig examples =
+    testGroup "Encoders produce valid CBOR" $
+       [ test exampleBlock          "Block"                  (encodeDisk codecConfig)


I wonder if something like testProperty "Block" $ roundtrip (encodeDisk codecConfig) (decodeDisk codecConfig) exampleBlock could also work, since it removes a bit of duplication

amesgen · 2023-08-28T12:44:24Z

ouroboros-consensus/src/unstable-consensus-testlib/Test/Util/Serialisation/Roundtrip.hs

@@ -78,31 +81,53 @@ roundtrip enc dec = roundtrip' enc (const <$> dec)

 -- | Roundtrip property for values annotated with their serialized form
 --
+-- In addition, we check that the encoded CBOR is valid using [validFlatTerm](https://hackage.haskell.org/package/cborg-0.2.9.0/docs/Codec-CBOR-FlatTerm.html#v:validFlatTerm).


Using Haddock markup:

Suggested change

-- In addition, we check that the encoded CBOR is valid using [validFlatTerm](https://hackage.haskell.org/package/cborg-0.2.9.0/docs/Codec-CBOR-FlatTerm.html#v:validFlatTerm).

-- In addition, we check that the encoded CBOR is valid using 'validFlatTerm'.

jorisdral

Looks good! I only have some minor comments

jorisdral · 2023-09-06T08:55:26Z

ouroboros-consensus/src/unstable-consensus-testlib/Test/Util/Serialisation/Examples.hs

Just to be sure: nothing changed when moving these definitions from the Golden module to this module?

Nope, I just extracted the relevant bits and pieces.

jorisdral · 2023-09-06T09:01:04Z

ouroboros-consensus-diffusion/test/mock-test/Test/ThreadNet/BFT.hs

@@ -28,6 +28,7 @@ import           Test.ThreadNet.Util.SimpleBlock
 import           Test.Util.HardFork.Future (singleEraFuture)
 import           Test.Util.Orphans.Arbitrary ()
 import           Test.Util.Serialisation.Roundtrip
+import           Test.Util.Serialisation.SomeResult ()


I'm slightly surprised this is necessary, since the SomeResult module contains no orphan instances. What happens if you remove it?

Good catch!

jorisdral · 2023-09-06T09:05:25Z

ouroboros-consensus/src/unstable-consensus-testlib/Test/Util/Serialisation/Roundtrip.hs

@@ -28,30 +28,38 @@ module Test.Util.Serialisation.Roundtrip (
  , roundtrip_SerialiseNodeToNode
  , roundtrip_all
  , roundtrip_envelopes
+    -- * Unit tests


Suggested change

-- * Unit tests

-- * Roundtrip tests for examples

jorisdral · 2023-09-06T09:06:59Z

ouroboros-consensus/src/unstable-consensus-testlib/Test/Util/Serialisation/Roundtrip.hs

+examplesRoundtrip ::
+     forall blk . (SerialiseDiskConstraints blk, Eq blk, Show blk, LedgerSupportsProtocol blk)
+  => CodecConfig blk
+  -> Examples blk
+  -> [TestTree]
+examplesRoundtrip codecConfig examples =
+  [ testRoundtripFor "Block"                 (encodeDisk codecConfig) (decodeDisk codecConfig)           exampleBlock
+  , testRoundtripFor "Header hash"           encode                   (const <$> decode)                 exampleHeaderHash
+  , testRoundtripFor "Ledger state"          (encodeDisk codecConfig) (const <$> decodeDisk codecConfig) exampleLedgerState
+  , testRoundtripFor "Annotated tip"         (encodeDisk codecConfig) (const <$> decodeDisk codecConfig) exampleAnnTip
+  , testRoundtripFor "Chain dependent state" (encodeDisk codecConfig) (const <$> decodeDisk codecConfig) exampleChainDepState
+  , testRoundtripFor "Extended ledger state" encodeExt                (const <$> decodeExt)              exampleExtLedgerState
+  ]
+  where


The body before the where should probably have an additional two-space indentation

Missed that. Thanks!

jorisdral · 2023-09-06T09:07:17Z

ouroboros-consensus/src/unstable-consensus-testlib/Test/Util/Serialisation/Roundtrip.hs

+examplesRoundtrip ::
+     forall blk . (SerialiseDiskConstraints blk, Eq blk, Show blk, LedgerSupportsProtocol blk)
+  => CodecConfig blk
+  -> Examples blk
+  -> [TestTree]
+examplesRoundtrip codecConfig examples =
+  [ testRoundtripFor "Block"                 (encodeDisk codecConfig) (decodeDisk codecConfig)           exampleBlock
+  , testRoundtripFor "Header hash"           encode                   (const <$> decode)                 exampleHeaderHash
+  , testRoundtripFor "Ledger state"          (encodeDisk codecConfig) (const <$> decodeDisk codecConfig) exampleLedgerState
+  , testRoundtripFor "Annotated tip"         (encodeDisk codecConfig) (const <$> decodeDisk codecConfig) exampleAnnTip
+  , testRoundtripFor "Chain dependent state" (encodeDisk codecConfig) (const <$> decodeDisk codecConfig) exampleChainDepState
+  , testRoundtripFor "Extended ledger state" encodeExt                (const <$> decodeExt)              exampleExtLedgerState
+  ]
+  where


I wonder if we can trade in some horizontal real estate for vertical real estate

I tried putting the type in one line but I think it's too long. However, I corrected the indentation in testRoundtripFor.

jorisdral · 2023-09-06T09:10:14Z

ouroboros-consensus/src/unstable-consensus-testlib/Test/Util/Serialisation/Roundtrip.hs

-    validFlatTerm (toFlatTerm enc_a)             ?! "Encoded flat term is not valid: " <> show enc_a
-    (bsRem, a' ) <- deserialiseFromBytes dec bs `onError` showByteString bs
+    validFlatTerm flatTerm_a                     ?! "Encoded flat term is not valid: " <> show enc_a
+    -- TODO: requires https://github.com/well-typed/cborg/pull/325


Should this be reflected in an issue, such that we don't forget about completing this TODO once a new version of cborg is released?

I added a s-r-p that contains the cborg fix. I can remove this comment.

jorisdral · 2023-09-06T09:15:16Z

ouroboros-consensus-cardano/test/shelley-test/Test/Consensus/Shelley/Serialisation.hs

+    [ testGroup "Shelley examples roundtrip" $ examplesRoundtrip ShelleyCodecConfig Shelley.Examples.examplesShelley
+    , testGroup "Mary examples roundtrip"    $ examplesRoundtrip ShelleyCodecConfig Shelley.Examples.examplesMary
+    , testGroup "Allegra examples roundtrip" $ examplesRoundtrip ShelleyCodecConfig Shelley.Examples.examplesAllegra
+    , testGroup "Alonzo examples roundtrip"  $ examplesRoundtrip ShelleyCodecConfig Shelley.Examples.examplesAlonzo
+    , testGroup "Babbage examples roundtrip" $ examplesRoundtrip ShelleyCodecConfig Shelley.Examples.examplesBabbage
+    , testGroup "Conway examples roundtrip"  $ examplesRoundtrip ShelleyCodecConfig Shelley.Examples.examplesConway


Random idea I had: maybe we should store Shelley-based era examples in an NP Examples ShelleyEras and map the roundtrip test function over it. That way, we won't forget about adding a roundtrip test when we add a new shelley-based era

That's an excellent idea! Thanks. Let me try and see what I can do.

This required to factor out 'SomeResult' as well.

jorisdral

LGTM!

jorisdral · 2023-09-20T08:14:21Z

ouroboros-consensus/src/unstable-consensus-testlib/Test/Util/Serialisation/Roundtrip.hs

@@ -124,16 +127,20 @@ roundtrip' enc dec a = checkRoundtripResult $ do
        flatTerm_a = toFlatTerm enc_a

    validFlatTerm flatTerm_a                     ?! "Encoded flat term is not valid: " <> show enc_a
-    a' <- fromFlatTerm dec flatTerm_a
-    a == a' bs                                   ?! show a <> " /= " <> show (a' bs)
+    -- TODO: the decode test via FlatTerm will currently fail because https://github.com/input-output-hk/cardano-ledger/issues/3741


Does the issue link back to this part of the code, such that we don't forget re-enabling it?

Good point, I'll have to add a permalink after merging this one

Function `Test.Util.Serialisation.Roundtrip.roundtrip'` has been adapted to check that the resulting encoding is valid according to [validFlatTerm](https://hackage.haskell.org/package/cborg-0.2.9.0/docs/Codec-CBOR-FlatTerm.html#v:validFlatTerm). We also test that the CBOR encoders for Cardano examples are valid by testing them with the `roundtrip'` function.

This reverts commit f8426b2 from #323. These tests turned out to be flaky, see eg [here](https://github.com/input-output-hk/ouroboros-consensus/actions/runs/6252676869/job/16976447179?pr=354#step:15:1589).

Also inlcudes changelog entries for #366 and the first commit in #323 (second commit was reverted in #362).

dnadales requested a review from a team as a code owner August 25, 2023 07:51

dnadales self-assigned this Aug 25, 2023

jorisdral approved these changes Aug 25, 2023

View reviewed changes

jorisdral approved these changes Aug 28, 2023

View reviewed changes

amesgen reviewed Aug 28, 2023

View reviewed changes

dnadales force-pushed the dnadales/ouroboros-network/3099-test-that-consensus-emits-valid-CBOR branch from d600ae3 to d1048b0 Compare September 4, 2023 17:06

jorisdral approved these changes Sep 6, 2023

View reviewed changes

Factor out Consensus' Examples data type into a separate module

c3e230a

This required to factor out 'SomeResult' as well.

dnadales force-pushed the dnadales/ouroboros-network/3099-test-that-consensus-emits-valid-CBOR branch 2 times, most recently from 2fcfa97 to 7923927 Compare September 19, 2023 15:41

jorisdral approved these changes Sep 20, 2023

View reviewed changes

dnadales force-pushed the dnadales/ouroboros-network/3099-test-that-consensus-emits-valid-CBOR branch from 7923927 to f8426b2 Compare September 20, 2023 11:41

dnadales enabled auto-merge September 20, 2023 11:41

dnadales added this pull request to the merge queue Sep 20, 2023

Merged via the queue into main with commit f743b8f Sep 20, 2023
9 of 11 checks passed

dnadales deleted the dnadales/ouroboros-network/3099-test-that-consensus-emits-valid-CBOR branch September 20, 2023 11:47

amesgen mentioned this pull request Sep 22, 2023

Revert "Test that Consensus emits valid CBOR" #362

Merged

amesgen mentioned this pull request Sep 28, 2023

Cut releases for Node 8.5 #377

Merged

github-merge-queue bot pushed a commit that referenced this pull request Sep 28, 2023

Cut releases for Node 8.5 (#377)

e3917f6

Also inlcudes changelog entries for #366 and the first commit in #323 (second commit was reverted in #362).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test that consensus emits valid CBOR #323

Test that consensus emits valid CBOR #323

dnadales commented Aug 25, 2023 •

edited

jorisdral Aug 25, 2023

jorisdral Aug 25, 2023

amesgen Aug 25, 2023

jorisdral Aug 28, 2023

jorisdral Aug 28, 2023

amesgen Aug 28, 2023

jorisdral left a comment

jorisdral Sep 6, 2023

dnadales Sep 15, 2023

jorisdral Sep 6, 2023

dnadales Sep 15, 2023

jorisdral Sep 6, 2023

jorisdral Sep 6, 2023

dnadales Sep 18, 2023

jorisdral Sep 6, 2023

dnadales Sep 18, 2023

jorisdral Sep 6, 2023

dnadales Sep 15, 2023

jorisdral Sep 6, 2023

dnadales Sep 15, 2023

jorisdral left a comment

jorisdral Sep 20, 2023

dnadales Sep 20, 2023

	-- In addition, we check that the encoded CBOR is valid using [validFlatTerm](https://hackage.haskell.org/package/cborg-0.2.9.0/docs/Codec-CBOR-FlatTerm.html#v:validFlatTerm).
	-- In addition, we check that the encoded CBOR is valid using 'validFlatTerm'.

Test that consensus emits valid CBOR #323

Test that consensus emits valid CBOR #323

Conversation

dnadales commented Aug 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisdral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisdral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnadales commented Aug 25, 2023 •

edited