Serialisation generators for Mary/Allegra #1966

uroboros · 2020-11-04T10:25:41Z

Resolves CAD-1845

mrBliss · 2020-11-04T11:00:59Z

Ledger with Delegation
  Minimal Property Tests
    Chain and Ledger traces cover the relevant cases:                                                   FAIL (10.38s)
      *** Failed! (after 16 tests):
      Exception:
        out of bounds : -15
        CallStack (from HasCallStack):
          error, called at src/Shelley/Spec/Ledger/Coin.hs:83:15 in shelley-spec-ledger-0.1.0.0-EBuMPkcZBOjV3QszYowLH:Shelley.Spec.Ledger.Coin
      Use --quickcheck-replay=644548 to reproduce.

uroboros · 2020-11-04T11:24:24Z

@uroboros Test failure:

Ledger with Delegation
  Minimal Property Tests
    Chain and Ledger traces cover the relevant cases:                                                   FAIL (10.38s)
      *** Failed! (after 16 tests):
      Exception:
        out of bounds : -15
        CallStack (from HasCallStack):
          error, called at src/Shelley/Spec/Ledger/Coin.hs:83:15 in shelley-spec-ledger-0.1.0.0-EBuMPkcZBOjV3QszYowLH:Shelley.Spec.Ledger.Coin
      Use --quickcheck-replay=644548 to reproduce.

🙏

mrBliss

There is now a bunch of overlap and duplication between Test.Cardano.Ledger.ShelleyMA.Serialisation.Generators and Test.Shelley.Spec.Ledger.Serialisation.Generators. For example, compare genBlock in the former to the Arbitrary (Block era) instance in the latter, it's almost a verbatim copy.

Moreover, the instances overlap, e.g., Arbitrary (Block (AllegraEra C_Crypto)) and Arbitrary (Block era).

This approach doesn't work well together with consensus, as we want our generators to be parametric in the era, just like all our Shelley-code. We don't want to duplicate our generators for all eras. In consensus they would be identical copies.

Fortunately, I think there is a good solution:

Move all the Arbitrary instances and generator functions that can be parametric in the era to Test.Shelley.Spec.Ledger.Serialisation.EraIndepGenerators. Don't impose any constraints that can only be satisfied by a single era, e.g., ShelleyTest has Core.TxBody era ~ TxBody era, that's a no go. These generators will require constraints like Arbitrary (Core.TxBody era), etc. You can bundle them in ShelleyBasedTest for convenience.
In Test.Shelley.Spec.Ledger.Serialisation.Generators, define the Arbitrary instances for the Shelley era specific types, i.e., the constraints you had to add to ShelleyBasedTest.
In Test.Cardano.Ledger.ShelleyMA.Serialisation.Generators, define the Arbitrary instances for the Mary and Allegra era specific types.

The good thing about this approach is that it should eliminate almost all duplication. Adding an extra era will only require writing Arbitrary instances for the TxBody, Script, ... types that are different in that era. Moreover, consensus will be able to use these generators, even when new eras are added in the future (as long as the era-specific instances are provided).

mrBliss · 2020-11-04T12:17:04Z

shelley-ma/shelley-ma-test/src/Test/Cardano/Ledger/ShelleyMA/Serialisation/Generators.hs

+ where
+ genScript = genMAScript
+
+instance Arbitrary (MA.TxBody (MaryEra C_Crypto)) where


Why are these all fixed to C_Crypto? (Same for Allegra) Consensus can't use C_Crypto.

Fortunately, you can generalise these all to Mock c => .. c, I have tried it out myself 🙂.

sure, I'll go with Mock c - actually started with that and then specialised ;/

mrBliss · 2020-11-04T12:44:46Z

@uroboros Test failure:

Ledger with Delegation
  Minimal Property Tests
    Chain and Ledger traces cover the relevant cases:                                                   FAIL (10.38s)
      *** Failed! (after 16 tests):
      Exception:
        out of bounds : -15
        CallStack (from HasCallStack):
          error, called at src/Shelley/Spec/Ledger/Coin.hs:83:15 in shelley-spec-ledger-0.1.0.0-EBuMPkcZBOjV3QszYowLH:Shelley.Spec.Ledger.Coin
      Use --quickcheck-replay=644548 to reproduce.

pray

Hasn't this happened before?

mrBliss · 2020-11-04T12:45:34Z

implement PredicateFailure generators for ShelleyMAEra

FYI, consensus will remain blocked until this is done.

uroboros · 2020-11-04T12:56:53Z

implement PredicateFailure generators for ShelleyMAEra

FYI, consensus will remain blocked until this is done.

👍

uroboros · 2020-11-04T12:57:22Z

@uroboros Test failure:

Ledger with Delegation
  Minimal Property Tests
    Chain and Ledger traces cover the relevant cases:                                                   FAIL (10.38s)
      *** Failed! (after 16 tests):
      Exception:
        out of bounds : -15
        CallStack (from HasCallStack):
          error, called at src/Shelley/Spec/Ledger/Coin.hs:83:15 in shelley-spec-ledger-0.1.0.0-EBuMPkcZBOjV3QszYowLH:Shelley.Spec.Ledger.Coin
      Use --quickcheck-replay=644548 to reproduce.

pray

Hasn't this happened before?

still finding that out - but this PR has nothing to do with it I'm sure

uroboros · 2020-11-06T08:00:21Z

@mrBliss I've made these changes

resolved the duplication (genBlock) between Shelley/ShelleyMA generators
ShelleyMA generators are now polymorphic over Crypto
Shelley generators are now fixed to ShelleyEra - this was what was giving overlapping instances with ShelleyMA (which were fixed to MaryEra/AllegraEra)

From your side you'd now have to import both

Test.Shelley.Spec.Ledger.Serialisation.Generators ()
Test.Cardano.Ledger.ShelleyMA.Serialisation.Generators ()

How does this work for you?

mrBliss

Thanks @uroboros for working on this!

We're getting closer, but it's still not quite what consensus needs 🙂.

The diamond we talked about:

1.         shared
          /   |  \
         /    |    \
2. Shelley  Allegra  Mary
         \    |     /
          \   |    /
3.     top-level shared

The Arbitrary instances that are shared among all eras. For example Arbitrary (HashHeader crypto), Arbitrary (Update era), etc. These live in EraIndepGenerators.
The Arbitrary instances for the types that differ from era to era, i.e., Value, TxBody, Script. So the Arbitrary (MA.TxBody (MaryEra c)), ... instances in ShelleyMA/../Generators and the Arbitrary (TxBody (ShelleyEra c)), ... instances in Shelley/../Generators.
The top-level Arbitrary instances for Block era, Tx era, LedgersPredicateFailure era, ..., that consensus wants. Note that these are parameterised by the era. These should live in EraIndepGenerators.

In the current state of the PR, 1 and 2 are done 👍. But 3 is not yet done, as there are still separate Arbitrary (Block (ShelleyEra c)) and Arbitrary (Block (MaryEra c)) instances (same for Tx, ...), instead of a single Arbitrary (Block era) instance. And it is exactly this what consensus needs, as our code is parametric in the era.

mrBliss · 2020-11-06T08:07:18Z

...and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Serialisation/Generators.hs

 where
 arbitrary = genericArbitraryU
+
 -- we don't have a shrinker for Value, so we do not shrink this
 -- predicate failure, as its constructor contains Value
 shrink pf = [pf]


BTW, if you can't shrink, return []. I think the current code might cause an infinite shrink.

uroboros · 2020-11-06T15:29:49Z

@mrBliss in this episode! ...

the following are now era parametrised in EraIndep.hs
- Block era, Tx era instances
- all PredicateFail instances (except the UTXO one, which is specialised per era)
EraIndep no longer refs (or is referenced by) Shelley.Generators and Shelley.MAGenerators - they are independent

What I could not quite get for you is the "top of the diamond" - but in effect EraIndep.hs becomes your entry point, and must be imported alongside the specialised ones:

import Test.Shelley.Spec.Ledger.Serialisation.EraIndepGenerators () -- shelley-spec-ledger-test
import Test.Shelley.Spec.Ledger.Serialisation.Generators ()         -- shelley-spec-ledger-test
import Test.Cardano.Ledger.ShelleyMA.Serialisation.Generators ()    -- cardano-ledger-shelley-ma-test

mrBliss

@mrBliss in this episode! ...

* the following are now era parametrised in `EraIndep.hs`
  
  * `Block era, Tx era` instances
  * all `PredicateFail` instances (except the UTXO one, which is specialised per era)

* `EraIndep` no longer refs (or is referenced by) `Shelley.Generators` and `Shelley.MAGenerators` - they are independent

Excellent!

What I could not quite get for you is the "top of the diamond" - but in effect EraIndep.hs becomes your entry point, and must be imported alongside the specialised ones:
import Test.Shelley.Spec.Ledger.Serialisation.EraIndepGenerators () -- shelley-spec-ledger-test
import Test.Shelley.Spec.Ledger.Serialisation.Generators ()         -- shelley-spec-ledger-test
import Test.Cardano.Ledger.ShelleyMA.Serialisation.Generators ()    -- cardano-ledger-shelley-ma-test

That's fine!

I'll try it out in consensus right now, to see whether it all works out as expected.

mrBliss · 2020-11-06T15:15:55Z

shelley-ma/shelley-ma-test/src/Test/Cardano/Ledger/ShelleyMA/Serialisation/Generators.hs

+ Arbitrary (StrictSeq (TxOut era)),
+ Arbitrary (StrictSeq (DCert era)),
+ Arbitrary (Wdrl era),
+ Arbitrary Coin,
+ Arbitrary (StrictMaybe (Update era)),
+ Arbitrary (StrictMaybe (MetaDataHash era)),
+ Arbitrary (StrictMaybe SlotNo)


I think you can simplify these constraints, e.g., Arbitrary (StrictSeq (TxOut era)) -> Arbitrary (TxOut era) and Arbitrary (StrictMaybe a) -> Arbitrary a. Enabling the Wredundant-constraints or -Wsimplifiable-class-constraints constraints should warn you about this. The same is true for a bunch of other instances.

also here, the StrictSeq is defined in EraIndep which I don't ref here, which prevents the simplifying to Arbitrary (TxOut era)

mrBliss · 2020-11-06T15:16:19Z

shelley-ma/shelley-ma-test/src/Test/Cardano/Ledger/ShelleyMA/Serialisation/Generators.hs

+ Arbitrary SlotNo,
+ Arbitrary Coin,
+ Arbitrary ByteString,
+ Arbitrary Network,
+ Arbitrary DeltaCoin


Don't these exist already?

these do exist in EraIndep - but I'm avoiding to reference that here so that we can use EraIndep as an entry point

What's your objection to depending on EraIndep here? I would understand objecting to the other direction.

good point! I'll do that and mop up the constraints

mrBliss · 2020-11-06T15:25:47Z

...er/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Serialisation/EraIndepGenerators.hs

+ Arbitrary (Core.Value era),
+ Arbitrary (Core.Script era)
+ ) =>
+ Arbitrary (Tx era)


mrBliss · 2020-11-06T15:25:54Z

...er/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Serialisation/EraIndepGenerators.hs

+ Arbitrary (Core.Value era),
+ Arbitrary (Core.Script era)
+ ) =>
+ Arbitrary (Block era)


mrBliss · 2020-11-06T16:24:04Z

I have just tried it out in consensus, and everything works 🎉

mrBliss · 2020-11-06T16:28:03Z

If you simplify these constraints (see inline comments), this is ready to merge for me. Thanks again @uroboros.

uroboros · 2020-11-06T16:40:12Z

I have just tried it out in consensus, and everything works 🎉

yay!

Resolves CAD-1845 * Extract EraIndepGenerators from the ShelleyEra serialisation generators so that we can re-use EraIndep as a base for ShelleyMAEra serialisation generators * Specialise Shelley Era generators to ShelleyEra (rather than just the `ShelleyTest` constraint) - otherwise we'd have Overlapping Instances between Shelley/MA * ShelleyMAEra generators are fixed to MaryEra C_Crypto and AllegraEra C_Crypto Note: To import Shelley + ShelleyMA arb instances, you need ``` import Test.Shelley.Spec.Ledger.Serialisation.EraIndepGenerators () -- shelley-spec-ledger-test import Test.Shelley.Spec.Ledger.Serialisation.Generators () -- shelley-spec-ledger-test import Test.Cardano.Ledger.ShelleyMA.Serialisation.Generators () -- cardano-ledger-shelley-ma-test ```

The ledger state format changed because of IntersectMBO/cardano-ledger#1968 The decoder is backwards-compatible, so the ledger doesn't break binary compatibility. The other visible change is IntersectMBO/cardano-ledger#1966

uroboros requested review from mrBliss, JaredCorduan, nc6 and TimSheard November 4, 2020 10:25

mrBliss suggested changes Nov 4, 2020

View reviewed changes

uroboros force-pushed the uroboros/shelley-ma-serialize-generators branch from 47d28ee to 16fe142 Compare November 4, 2020 12:44

uroboros force-pushed the uroboros/shelley-ma-serialize-generators branch 2 times, most recently from b5f2519 to dc7bfd5 Compare November 6, 2020 07:50

mrBliss reviewed Nov 6, 2020

View reviewed changes

uroboros force-pushed the uroboros/shelley-ma-serialize-generators branch 2 times, most recently from febb953 to ec7640b Compare November 6, 2020 15:20

mrBliss reviewed Nov 6, 2020

View reviewed changes

mrBliss self-requested a review November 6, 2020 16:27

mrBliss approved these changes Nov 6, 2020

View reviewed changes

uroboros force-pushed the uroboros/shelley-ma-serialize-generators branch 2 times, most recently from 1780d94 to 761b96e Compare November 6, 2020 18:27

uroboros force-pushed the uroboros/shelley-ma-serialize-generators branch from 761b96e to ec12be7 Compare November 6, 2020 18:41

uroboros merged commit 29527de into master Nov 7, 2020

iohk-bors bot deleted the uroboros/shelley-ma-serialize-generators branch November 7, 2020 05:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialisation generators for Mary/Allegra #1966

Serialisation generators for Mary/Allegra #1966

uroboros commented Nov 4, 2020 •

edited

Loading

mrBliss commented Nov 4, 2020

uroboros commented Nov 4, 2020

mrBliss left a comment

mrBliss Nov 4, 2020

uroboros Nov 4, 2020

mrBliss commented Nov 4, 2020

mrBliss commented Nov 4, 2020

uroboros commented Nov 4, 2020

uroboros commented Nov 4, 2020

uroboros commented Nov 6, 2020

mrBliss left a comment

mrBliss Nov 6, 2020

uroboros commented Nov 6, 2020 •

edited

Loading

mrBliss left a comment

mrBliss Nov 6, 2020

uroboros Nov 6, 2020

mrBliss Nov 6, 2020

uroboros Nov 6, 2020

mrBliss Nov 6, 2020

uroboros Nov 6, 2020

mrBliss Nov 6, 2020

mrBliss Nov 6, 2020

mrBliss commented Nov 6, 2020

mrBliss commented Nov 6, 2020

uroboros commented Nov 6, 2020

Serialisation generators for Mary/Allegra #1966

Serialisation generators for Mary/Allegra #1966

Conversation

uroboros commented Nov 4, 2020 • edited Loading

mrBliss commented Nov 4, 2020

uroboros commented Nov 4, 2020

mrBliss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrBliss commented Nov 4, 2020

mrBliss commented Nov 4, 2020

uroboros commented Nov 4, 2020

uroboros commented Nov 4, 2020

uroboros commented Nov 6, 2020

mrBliss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uroboros commented Nov 6, 2020 • edited Loading

mrBliss 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

mrBliss commented Nov 6, 2020

mrBliss commented Nov 6, 2020

uroboros commented Nov 6, 2020

uroboros commented Nov 4, 2020 •

edited

Loading

uroboros commented Nov 6, 2020 •

edited

Loading