Test that UTxO tables are updated correctly on era transitions #4073

bartfrenk · 2022-10-13T08:17:25Z

Description

Test that UTxO tables are computed correctly on era transitions, for all era transitions.

Closes #4043.

Checklist

Branch
- Commit sequence broadly makes sense
- Commits have useful messages
- New tests are added if needed and existing tests are updated
Pull Request
- Self-reviewed the diff
- Useful pull request description at least containing the following information:
  - What does this PR change?
  - Why these changes were needed?
  - How does this affect downstream repositories and/or end-users?
  - Which ticket does this PR close (if any)? If it does, is it linked?
- Reviewer requested

dnadales

Looks good so far.

ouroboros-consensus-byron-test/src/Test/Consensus/Byron/Generators.hs

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

dnadales · 2022-10-17T12:47:18Z

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

+      (fixedEpochInfo (sgEpochLength shelleyGenesis) tpraosSlotLength)
+      (MaxMajorProtVer 1000)
+
+shelleyGenesis :: ShelleyGenesis era


If we use a fixed Shelley genesis we should justify why. I think in this case the justification might be that we are interested in the initial UTxO set, which is not affected by the genesis config. But we need to confirm this.

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

ouroboros-consensus-byron-test/src/Test/Consensus/Byron/Generators.hs

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

dnadales · 2022-10-19T15:41:01Z

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

+tests = testGroup "UpdateTablesOnEraTransition" $ toTestTrees predicates tls
+  where
+    tls = translateLedgerState hardForkEraTranslation
+    predicates :: InPairs TranslationPredicate (CardanoEras Crypto)


Why not define a [TestTree] directly? Is toTestTrees necessary?

I suspect this can be indeed reshaped to just define a [TestTree] and avoid having to re-zip the InPairss. Was there a specific reason to not do so?

Well, I couldn't do it.

To define the properties I need to have access to the appropriate TranslateEra x y values. I chose the direction of zipping the predicates with what is loosely, InPairs TranslateEra (CardanoEras c).

What do you guys think about b762d98

@bartfrenk feel free to discard this commit and force push if you prefer to keep toTestTree.

If the commit above makes sense, we should probably address this:

-- | TODO: we should simply expose 'translateLedgerStateByronToShelleyWrapper' -- and other translations in ' Ouroboros.Consensus.Cardano.CanHardFork'.

If we discard this commit we should remove the "boom" string and use a more appropriate label.

dnadales · 2022-10-19T15:47:26Z

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

+                  , Show (LedgerConfig dest)
+                  , Show (LedgerState src EmptyMK)) => Show (TestSetup src dest)
+
+-- todo: Useful to merge some of these instances?


I don't see how we could merge the Byron to Shelley instances. I'm not sure about the others. Do you know what prevents us from defining a:

instance Arbitrary (TestSetup (ShelleyBlock p era0) (ShelleyBlock p era1))

Even further

instance Arbitrary (TestSetup (ShelleyBlock p (PreviousEra era)) (ShelleyBlock p era))

Although the protos don't need to be equal...

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

ouroboros-consensus/src/Ouroboros/Consensus/Util.hs

dnadales · 2022-10-20T08:41:11Z

I forgot to add these remaining tasks:

Add to the AddingAnEra.md document instructions to add a new era translation test when adding a new era.
Use quickcheck's cover to check that we're testing important cases.

jasagredo

Looks good overall! I would do some cosmetic changes as proposed but overall it looks nice

jasagredo · 2022-10-20T08:45:00Z

ouroboros-consensus/src/Ouroboros/Consensus/HardFork/Combinator/Util/InPairs.hs

+  Constraints
+-------------------------------------------------------------------------------}
+
+type family AllPairs


I always have trouble deciding if these should be classes or closed type families...

This is adapted from Maguire's Thinking with types. I can kind-of see how to do it with classes, but I like the terseness of this definition, and I think, but do not know, the class-one will be more convoluted.

ouroboros-consensus/src/Ouroboros/Consensus/HardFork/Combinator/Util/InPairs.hs

ouroboros-consensus/src/Ouroboros/Consensus/Util.hs

jasagredo · 2022-10-20T08:53:47Z

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

+                  , Show (LedgerConfig dest)
+                  , Show (LedgerState src EmptyMK)) => Show (TestSetup src dest)
+
+-- todo: Useful to merge some of these instances?


Even further

instance Arbitrary (TestSetup (ShelleyBlock p (PreviousEra era)) (ShelleyBlock p era))

Although the protos don't need to be equal...

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

jasagredo · 2022-10-20T09:01:26Z

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

+tests = testGroup "UpdateTablesOnEraTransition" $ toTestTrees predicates tls
+  where
+    tls = translateLedgerState hardForkEraTranslation
+    predicates :: InPairs TranslationPredicate (CardanoEras Crypto)


I suspect this can be indeed reshaped to just define a [TestTree] and avoid having to re-zip the InPairss. Was there a specific reason to not do so?

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

bartfrenk · 2022-10-26T08:22:15Z

bors r+

4073: Test that UTxO tables are updated correctly on era transitions r=bartfrenk a=bartfrenk # Description Test that UTxO tables are computed correctly on era transitions, for all era transitions. Closes #4043. Co-authored-by: Bart Frenk <bart.frenk@gmail.com>

iohk-bors · 2022-10-26T10:45:14Z

Timed out.

bartfrenk · 2022-10-26T12:00:23Z

bors r+

4073: Test that UTxO tables are updated correctly on era transitions r=bartfrenk a=bartfrenk # Description Test that UTxO tables are computed correctly on era transitions, for all era transitions. Closes #4043. Co-authored-by: Bart Frenk <bart.frenk@gmail.com>

iohk-bors · 2022-10-26T14:15:14Z

Timed out.

dnadales · 2022-10-26T16:05:02Z

We should also test with empty UTxO, shouldn't we?

      +++ OK, passed 100 tests (100% UTxO set is not empty).

It seems we aren't at the moment.

Co-authored-by: Damian Nadales <damian.only@gmail.com>

bartfrenk changed the base branch from master to feature/utxo-hd October 13, 2022 08:18

dnadales reviewed Oct 17, 2022

View reviewed changes

bartfrenk force-pushed the bartfrenk/4043/era-transition-tests branch 2 times, most recently from aa80b45 to 12effd8 Compare October 18, 2022 15:01

bartfrenk added 4 commits October 19, 2022 12:08

Add utility function to check for emptiness of anti-diffs

322c636

Add generators for Byron ledger states and configs

2235847

Add utility functions

0a263e9

Add tests

280e92c

bartfrenk force-pushed the bartfrenk/4043/era-transition-tests branch from 2f08815 to 280e92c Compare October 19, 2022 10:21

bartfrenk requested a review from jasagredo October 19, 2022 10:29

bartfrenk marked this pull request as ready for review October 19, 2022 10:30

bartfrenk requested review from nfrisby, coot and bolt12 as code owners October 19, 2022 10:30

bartfrenk removed request for nfrisby, coot and bolt12 October 19, 2022 10:30

dnadales approved these changes Oct 19, 2022

View reviewed changes

jasagredo approved these changes Oct 20, 2022

View reviewed changes

bartfrenk added 6 commits October 20, 2022 12:37

Update AddingAnEra

35c638c

Add coverage checking

d568e9f

Update wording of doc for the Byron ledger state generator

03c1779

Reword definition of AllPairs

1d879c2

Incorporate review comments

93a0c6d

Further review comments

06513e7

Remove toTestTrees.

b762d98

dnadales merged commit ac3ff43 into feature/utxo-hd Nov 2, 2022

iohk-bors bot deleted the bartfrenk/4043/era-transition-tests branch November 2, 2022 11:00

jasagredo pushed a commit that referenced this pull request Nov 17, 2022

Test that UTxO tables are updated correctly on era transitions (#4073)

7b8529b

Co-authored-by: Damian Nadales <damian.only@gmail.com>

jasagredo pushed a commit that referenced this pull request Dec 2, 2022

Test that UTxO tables are updated correctly on era transitions (#4073)

ee389a6

Co-authored-by: Damian Nadales <damian.only@gmail.com>

jasagredo pushed a commit that referenced this pull request Dec 2, 2022

Test that UTxO tables are updated correctly on era transitions (#4073)

087becd

Co-authored-by: Damian Nadales <damian.only@gmail.com>

jasagredo pushed a commit that referenced this pull request Jan 18, 2023

Test that UTxO tables are updated correctly on era transitions (#4073)

adecd1a

Co-authored-by: Damian Nadales <damian.only@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test that UTxO tables are updated correctly on era transitions #4073

Test that UTxO tables are updated correctly on era transitions #4073

bartfrenk commented Oct 13, 2022 •

edited

Loading

dnadales left a comment

dnadales Oct 17, 2022

dnadales Oct 19, 2022

jasagredo Oct 20, 2022

bartfrenk Oct 20, 2022

bartfrenk Oct 20, 2022

dnadales Oct 26, 2022

dnadales Oct 19, 2022

jasagredo Oct 20, 2022

dnadales commented Oct 20, 2022 •

edited by bartfrenk

Loading

jasagredo left a comment

jasagredo Oct 20, 2022

bartfrenk Oct 20, 2022

jasagredo Oct 20, 2022

jasagredo Oct 20, 2022

bartfrenk commented Oct 26, 2022

iohk-bors bot commented Oct 26, 2022

bartfrenk commented Oct 26, 2022

iohk-bors bot commented Oct 26, 2022

dnadales commented Oct 26, 2022

Test that UTxO tables are updated correctly on era transitions #4073

Test that UTxO tables are updated correctly on era transitions #4073

Conversation

bartfrenk commented Oct 13, 2022 • edited Loading

Description

Checklist

dnadales 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

dnadales commented Oct 20, 2022 • edited by bartfrenk Loading

jasagredo 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

bartfrenk commented Oct 26, 2022

iohk-bors bot commented Oct 26, 2022

bartfrenk commented Oct 26, 2022

iohk-bors bot commented Oct 26, 2022

dnadales commented Oct 26, 2022

bartfrenk commented Oct 13, 2022 •

edited

Loading

dnadales commented Oct 20, 2022 •

edited by bartfrenk

Loading