ThreadNet: add ShelleyAllegra tests #2641

nfrisby · 2020-09-28T21:01:46Z

This follows up #2627 by adding tests that exercise the ability to hard from from a Shelley-based era to the next Shelley-based era (in this case, Shelley to Allegra).

This PR introduces the TwoEras.hs infrastructure. For every era we have, we should have a two-era test that has a primary interest of whether or not a proposal in that era can successfully induce a hard fork, especially in the presence of a short network partition. I suggest, as part of Issue IntersectMBO/ouroboros-consensus#651, we should continue to emphasize these "Will it fork?" tests.

As of this PR, Cardano.hs tests that we can hard fork out of Byron and ShelleyAllegra.hs tests that we can hard fork out of Shelley. At the moment, both have two eras, and so they can share the infrastructure that was factored out into TwoEras.hs

Eventually Cardano.hs will have 3-4 eras. At that time, I propose we introduce a Byron______.hs module that specifically tests leaving Byron. Maybe to Byron again, or still to Shelley, etc. To summarize: we should have a separate test dedicated to testing each adjacent pair of eras on mainnet and one more that tests a hard fork from the last mainnet era to another copy of itself.

Edit: Edsko and I have something along these lines in mind while working on Issue 2472, the rewrite. But we hope to have it be a tidy consolidated thing instead of as explicit as the above proposal.

nfrisby · 2020-10-07T17:27:07Z

This is timing out on the mac-mini build: ShelleyShelley tests give 900 seconds of silence. I'm going to add a debug tracers so that it's not 900 seconds of silence to see how long it actually takes.

nfrisby · 2020-10-07T20:41:05Z

I rebased, picking up diffs for most recent changes on master.
I added an extra commit that echos the milliseconds-since-process-start each time a ShelleyShelley invocation begins. This should prevent the "900 seconds of silence" on the mac-mini CI. Then we can inspect the logs and see how long it's actually taking. On the nightly runs, each ShelleyShelley invocation takes 40.5 seconds on average. And we're slashing the requested tests by 1/10, so I think this is running 10 tests, each of which is 40.5 seconds.

I'll admit, that's the first time I did the math. So if the variance is too high, maybe the total time is likely to be more than double the average... especially if the mac-mini build is relatively slow. I'll have some data after this CI build, and then we can consider disabling this test outside of the nightly.

nfrisby · 2020-10-08T16:21:57Z

My latest push here disables random transactions in ShelleyShelley. That gives a 10x speed-up. (See IntersectMBO/cardano-ledger#1901.)

Remaining item is to fix the translation of the ledger view forecast.

mrBliss · 2020-10-12T16:41:51Z

I'll take over this PR so that @nfrisby can focus on the ThreadNet rewrite.

My latest push here disables random transactions in ShelleyShelley. That gives a 10x speed-up. (See input-output-hk/cardano-ledger-specs#1901.)

I'll rebase this tomorrow, not that #2681 has been merged. That PR disables the Shelley transaction generator, which this PR also does.

Remaining item is to fix the translation of the ledger view forecast.

The master branch includes #2661, which provides a correct translation function which I'll use.

we should have a separate test dedicated to testing each adjacent pair of eras on mainnet

Agreed. As of #2666, we have the Allegra and Mary eras. So I'm thinking of changing ShelleyShelley to ShelleyAllegra.

At the moment, there is no real difference between Shelley->Shelley, Shelley->Allegra, or Allegra->Mary (Allegra and Mary are currently identical to Shelley). That will soon change with #2679, after which I'll also introduce MaryAllegra. I think the pattern synonyms from ShelleyShelley can be generalised to any two eras.

I would still like to have at least one test running all the hard forks of CardanoBlock, i.e., all 3 of them (Byron->Shelley->Allegra->Mary), since that's what mainnet is running.

nfrisby · 2020-10-12T17:18:51Z

I would still like to have at least one test running all the hard forks of CardanoBlock, i.e., all 3 of them (Byron->Shelley->Allegra->Mary), since that's what mainnet is running.

Yep, that's what I had in mind 👍

Do note: such a test will require a lot of slots; those epochs are long. Also, the current ThreadNet tests cannot easily (at all?) express the necessary transactions for transitioning from eg the second era to the third. At the moment, the "crucial tx" infra we use for these can only (easily?) express the transactions necessary for leaving the first era. (This is compounded by needing to know a valid UTxO for use as an input.)

Fixing this is something we're keeping in mind for the ThreadNet rewrite.

Thanks for taking this over.

mrBliss · 2020-10-22T07:39:31Z

FYI, I have been working on this in the background. I have refactored some things to make the two-era infrastructure generic and reusable with a minimum of boilerplate (e.g., #2687). I am currently distracted by some other ledger integration related things, but will soon get back to this.

mrBliss

Update:

Rebased
Removed some unused code from TwoEras that was copied from Cardano, e.g., TestSetup.
Changed the names in TwoEras to no longer assume Byron and Shelley. See the TODOs.
Generalised the ShelleyShelley infra to ShelleyBasedHardFork. This generalisation ironically does not permit ShelleyShelley as it wants a translation between the two eras.
Changed the ShelleyShelley test to ShelleyAllegra, as this is the next hard fork, so it's important to test it. (Note that we will extend Cardano with Allegra and Mary soon).

I have left some comments on the TODOs that still need addressing.

.buildkite/slow-ThreadNet-tests.sh

mrBliss · 2020-11-16T15:53:43Z

ouroboros-consensus-cardano-test/src/Test/ThreadNet/Infra/TwoEras.hs

+    --   sufficiently strict upper bound to preclude a Chain Growth violation,
+    --   since Chain Growth isn't technically violated until @2k@ slots have
+    --   passed (it's also relevant that the net forges at full round-robin
+    --   speed until the partition starts). (TODO does that cover Shelley too?)


TODO This function will be used by Cardano (first era = Byron) and ShelleyAllegra (first era = Shelley), but currently assumes Byron is the first era.

(Note that Cardano will have be generalised to 3 and 4 eras soon.)

mrBliss · 2020-11-16T15:53:54Z

ouroboros-consensus-cardano-test/src/Test/ThreadNet/Infra/TwoEras.hs

+          , (1,  Just $ 4 * k + 1)
+          , (1,  Just $ 4 * k + 1 + quorum)
+          , (20, assert (numFirstEraEpochs == (1 :: Int)) $
+                 Just $ byronEpochSize (SecurityParam k))


TODO Byron-specific

ouroboros-consensus-cardano-test/test/Test/ThreadNet/ShelleyAllegra.hs

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

ouroboros-consensus-cardano-test/test/Test/ThreadNet/ShelleyAllegra.hs

+          --- TODO this comment and code are wrong
+
+          BoolProps.requiredIf $
+          -- The active slots in the first two Shelley epochs are all overlay
+          -- slots, so the first Shelley block will arise from one of those.
+          not $ Set.null overlaySlots


@nfrisby I don't understand this. Should this be fixed before merging? I'm inclined to leave it as-is, as the rewrite is coming.

mrBliss · 2020-11-20T12:30:20Z

ouroboros-consensus-test/src/Test/ThreadNet/TxGen.hs

+-- Choose @NP WrapTxGenExtra xs@ for the instance of the 'TxGenExtra' type
+-- family, where @xs@ matches the concrete instantiation.
+testGenTxsHfc ::
+     forall xs. (All TxGen xs, CanHardFork xs)


This takes instances and produces a function, it should takes functions and produce a function.

That would require taking an NP of testGenTxs, which is annoying. Leave it.

ouroboros-consensus-test/src/Test/ThreadNet/TxGen.hs

ouroboros-consensus/ouroboros-consensus.cabal

ouroboros-consensus/src/Ouroboros/Consensus/HardFork/Combinator/Binary.hs

mrBliss · 2020-11-20T12:46:29Z

ouroboros-consensus-cardano-test/src/Test/ThreadNet/Infra/ShelleyBasedHardFork.hs

+pattern ShelleyBasedHardForkNodeToNodeVersion1 =
+    HardForkNodeToNodeEnabled
+      HardForkSpecificNodeToNodeVersion1
+      (  EraNodeToNodeEnabled ShelleyNodeToNodeVersion1


Use maxBound

Uggh, can't use maxBound here, this also has to be a pattern of course.

ouroboros-consensus-cardano-test/src/Test/ThreadNet/Infra/ShelleyBasedHardFork.hs

mrBliss · 2020-11-20T12:56:32Z

ouroboros-consensus-cardano-test/src/Test/ThreadNet/TxGen/Allegra.hs

+import           Test.ThreadNet.TxGen (TxGen (..))
+
+-- | Dummy generator until CAD-2119 is done, i.e., the transaction generator in
+-- the ledger has been generalised over the eras.


Check whether this has been done since.

IntersectMBO/cardano-ledger#1997 has been merged, but there isn't yet an instance for Allegra/Mary as far as I can see. IntersectMBO/cardano-ledger#2000 is open.

mrBliss · 2020-11-20T13:00:55Z

ouroboros-consensus-cardano-test/test/Test/ThreadNet/ShelleyAllegra.hs

+          --- TODO this comment and code are wrong
+
+          BoolProps.requiredIf $
+          -- The active slots in the first two Shelley epochs are all overlay
+          -- slots, so the first Shelley block will arise from one of those.
+          not $ Set.null overlaySlots


@nfrisby I don't understand this. Should this be fixed before merging? I'm inclined to leave it as-is, as the rewrite is coming.

We will be adding `Binary` shortly.

`protocolInfoShelleyBased` can be used for each Shelley-based epoch. This can in the future be used for Allegra, Mary, ..., too. Reimplement the existing `protocolInfoShelley` in terms of it.

mrBliss · 2020-11-23T11:21:53Z

Since the ThreadNet tests pass (including a nightly) with some of the Byron-specific bits in place, I'm going to merge it as-is. We can improve them later.

mrBliss · 2020-11-23T11:22:01Z

bors merge

2641: ThreadNet: add ShelleyAllegra tests r=mrBliss a=nfrisby Fixes #2633. This follows up #2627 by adding tests that exercise the ability to hard from from a Shelley-based era to the next Shelley-based era (in this case, Shelley to Allegra). This PR introduces the `TwoEras.hs` infrastructure. For every era we have, we should have a two-era test that has a primary interest of whether or not a proposal in that era can successfully induce a hard fork, especially in the presence of a short network partition. I suggest, as part of Issue #2472, we should continue to emphasize these "Will it fork?" tests. As of this PR, `Cardano.hs` tests that we can hard fork out of Byron and `ShelleyAllegra.hs` tests that we can hard fork out of `Shelley`. At the moment, both have two eras, and so they can share the infrastructure that was factored out into `TwoEras.hs` Eventually `Cardano.hs` will have 3-4 eras. At that time, I propose we introduce a `Byron______.hs` module that specifically tests leaving Byron. Maybe to Byron again, or still to Shelley, etc. To summarize: we should have a separate test dedicated to testing each adjacent pair of eras on mainnet and one more that tests a hard fork from the last mainnet era to another copy of itself. Edit: Edsko and I have something along these lines in mind while working on Issue 2472, the rewrite. But we hope to have it be a tidy consolidated thing instead of as explicit as the above proposal. Co-authored-by: Thomas Winant <thomas@well-typed.com> Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>

iohk-bors · 2020-11-23T13:28:53Z

Timed out.

mrBliss · 2020-11-23T14:03:31Z

bors merge

2641: ThreadNet: add ShelleyAllegra tests r=mrBliss a=nfrisby Fixes #2633. This follows up #2627 by adding tests that exercise the ability to hard from from a Shelley-based era to the next Shelley-based era (in this case, Shelley to Allegra). This PR introduces the `TwoEras.hs` infrastructure. For every era we have, we should have a two-era test that has a primary interest of whether or not a proposal in that era can successfully induce a hard fork, especially in the presence of a short network partition. I suggest, as part of Issue #2472, we should continue to emphasize these "Will it fork?" tests. As of this PR, `Cardano.hs` tests that we can hard fork out of Byron and `ShelleyAllegra.hs` tests that we can hard fork out of `Shelley`. At the moment, both have two eras, and so they can share the infrastructure that was factored out into `TwoEras.hs` Eventually `Cardano.hs` will have 3-4 eras. At that time, I propose we introduce a `Byron______.hs` module that specifically tests leaving Byron. Maybe to Byron again, or still to Shelley, etc. To summarize: we should have a separate test dedicated to testing each adjacent pair of eras on mainnet and one more that tests a hard fork from the last mainnet era to another copy of itself. Edit: Edsko and I have something along these lines in mind while working on Issue 2472, the rewrite. But we hope to have it be a tidy consolidated thing instead of as explicit as the above proposal. Co-authored-by: Thomas Winant <thomas@well-typed.com> Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>

mrBliss · 2020-11-23T15:13:41Z

bors ping

iohk-bors · 2020-11-23T15:13:43Z

pong

mrBliss · 2020-11-23T15:13:55Z

bors merge

iohk-bors · 2020-11-23T15:13:57Z

Already running a review

mrBliss · 2020-11-23T16:01:57Z

Bors' job in Hydra finished 1h ago: https://hydra.iohk.io/eval/981006 🙄

iohk-bors · 2020-11-23T16:20:37Z

Timed out.

mrBliss · 2020-11-23T16:20:48Z

bors merge

iohk-bors · 2020-11-23T17:35:26Z

Build succeeded:

nfrisby requested review from mrBliss and edsko September 28, 2020 21:01

nfrisby added consensus issues related to ouroboros-consensus protocol testing testing labels Sep 28, 2020

nfrisby mentioned this pull request Sep 28, 2020

InspectLedger instance for Shelley #2627

Merged

nfrisby force-pushed the nfrisby/issue-2633-hardfork-from-Shelley branch 3 times, most recently from c2c49f7 to d41f2af Compare September 28, 2020 21:57

nfrisby force-pushed the nfrisby/issue-2633-hardfork-from-Shelley branch from d41f2af to b8f7b30 Compare October 6, 2020 02:27

nfrisby force-pushed the nfrisby/issue-2633-hardfork-from-Shelley branch from b8f7b30 to 7537ad8 Compare October 7, 2020 20:36

nfrisby force-pushed the nfrisby/issue-2633-hardfork-from-Shelley branch from 7537ad8 to 5b0fb8b Compare October 8, 2020 16:20

mrBliss force-pushed the nfrisby/issue-2633-hardfork-from-Shelley branch 2 times, most recently from a83db56 to 52c362e Compare October 27, 2020 08:34

mrBliss force-pushed the nfrisby/issue-2633-hardfork-from-Shelley branch from 52c362e to d26ca87 Compare November 16, 2020 15:50

mrBliss reviewed Nov 16, 2020

View reviewed changes

mrBliss changed the title ~~ThreadNet: add ShelleyShelley tests~~ ThreadNet: add ShelleyAllegra tests Nov 16, 2020

mrBliss force-pushed the nfrisby/issue-2633-hardfork-from-Shelley branch 2 times, most recently from 55d949a to 554773a Compare November 17, 2020 11:17

mrBliss reviewed Nov 20, 2020

View reviewed changes

edsko approved these changes Nov 20, 2020

View reviewed changes

mrBliss added 3 commits November 23, 2020 11:58

Provide an opt-in implementation of TxGen for the HFC

6ca8a4c

Move Nary and Unary under a new Embed hierarchy

2f8a514

We will be adding `Binary` shortly.

Add a generic protocolInfoBinary

b0472f0

Generalise protocolInfoShelley to protocolInfoShelleyBased

5e0fe60

`protocolInfoShelleyBased` can be used for each Shelley-based epoch. This can in the future be used for Allegra, Mary, ..., too. Reimplement the existing `protocolInfoShelley` in terms of it.

mrBliss force-pushed the nfrisby/issue-2633-hardfork-from-Shelley branch from 554773a to 1e077c4 Compare November 23, 2020 10:58

nfrisby added 2 commits November 23, 2020 12:01

cardano-test: factor out TwoEras infra; intro ShelleyAllegra test

305e739

buildkite: add slow ShelleyAllegra test to nightly.yml

55eb8b0

mrBliss force-pushed the nfrisby/issue-2633-hardfork-from-Shelley branch from 1e077c4 to 55eb8b0 Compare November 23, 2020 11:01

mrBliss mentioned this pull request Nov 23, 2020

Update dependency on ouroboros-network IntersectMBO/cardano-node#2114

Merged

iohk-bors bot merged commit 44b61df into master Nov 23, 2020

iohk-bors bot deleted the nfrisby/issue-2633-hardfork-from-Shelley branch November 23, 2020 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ThreadNet: add ShelleyAllegra tests #2641

ThreadNet: add ShelleyAllegra tests #2641

nfrisby commented Sep 28, 2020 •

edited by mrBliss

Loading

nfrisby commented Oct 7, 2020

nfrisby commented Oct 7, 2020

nfrisby commented Oct 8, 2020

mrBliss commented Oct 12, 2020

nfrisby commented Oct 12, 2020

mrBliss commented Oct 22, 2020

mrBliss left a comment

mrBliss Nov 16, 2020

mrBliss Nov 16, 2020

mrBliss Nov 16, 2020

mrBliss Nov 20, 2020

mrBliss Nov 20, 2020

mrBliss Nov 23, 2020

mrBliss Nov 20, 2020

mrBliss Nov 20, 2020

mrBliss Nov 20, 2020

mrBliss Nov 23, 2020

mrBliss Nov 20, 2020

mrBliss commented Nov 23, 2020

mrBliss commented Nov 23, 2020

iohk-bors bot commented Nov 23, 2020

mrBliss commented Nov 23, 2020

mrBliss commented Nov 23, 2020

iohk-bors bot commented Nov 23, 2020

mrBliss commented Nov 23, 2020

iohk-bors bot commented Nov 23, 2020

mrBliss commented Nov 23, 2020

iohk-bors bot commented Nov 23, 2020

mrBliss commented Nov 23, 2020

iohk-bors bot commented Nov 23, 2020

ThreadNet: add ShelleyAllegra tests #2641

ThreadNet: add ShelleyAllegra tests #2641

Conversation

nfrisby commented Sep 28, 2020 • edited by mrBliss Loading

nfrisby commented Oct 7, 2020

nfrisby commented Oct 7, 2020

nfrisby commented Oct 8, 2020

mrBliss commented Oct 12, 2020

nfrisby commented Oct 12, 2020

mrBliss commented Oct 22, 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

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 23, 2020

mrBliss commented Nov 23, 2020

iohk-bors bot commented Nov 23, 2020

mrBliss commented Nov 23, 2020

mrBliss commented Nov 23, 2020

iohk-bors bot commented Nov 23, 2020

mrBliss commented Nov 23, 2020

iohk-bors bot commented Nov 23, 2020

mrBliss commented Nov 23, 2020

iohk-bors bot commented Nov 23, 2020

mrBliss commented Nov 23, 2020

iohk-bors bot commented Nov 23, 2020

nfrisby commented Sep 28, 2020 •

edited by mrBliss

Loading