Remove ConwayNewEpochPredFailure

[aniketd]

Description

Closes #4662

Checklist

• Commits in meaningful sequence and with useful messages.
• Tests added or updated when needed.
• CHANGELOG.md files updated for packages with externally visible changes.
  NOTE: New section is never added with the code changes. (See RELEASING.md).
• Versions updated in .cabal and CHANGELOG.md files when necessary, according to the
  versioning process.
• Version bounds in .cabal files updated when necessary.
  NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
• Code formatted (use scripts/fourmolize.sh).
• Cabal files formatted (use scripts/cabal-format.sh).
• CDDL files are up to date (use scripts/gen-cddl.sh)
• hie.yaml updated (use scripts/gen-hie.sh).
• Self-reviewed the diff.

[Soupstraw]

I guess you can turn that check into an assertion, that way we'll be able to notice if something's wrong with the reward update in one of our tests. It seems like too expensive of a check to keep it in the production version of ledger and run it every epoch boundary.

[lehins]

Removal itself looks good, but it would be nice not to loose the invariant check, so STS assertion seems like the most logical choice.

Also, could you please apply all of the exact same changes to the Shelley NEWEPOCH rule. It suffers from all the same problems.

[lehins]

STS Assertion is not the same as assert provided by GHC

[lehins]

This is definitely unacceptable.
Test values have no business being used in production code. Not ever!

[lehins]

Beautiful! Thank you!

[lehins]

FYI the test failure that you see on CI in cardano-ledger-shelley-test could have been avoided if we had an STS assertion instead. In any case, it is a poorly written test anyways, since it starts with a totally arbitrary NewEpochState and it only executed for Shelley. Feel free to remove this test, I've created a ticket to bring it back with a more reasonable approach: #5047

[aniketd]

I guess you can turn that check into an assertion, that way we'll be able to notice if something's wrong with the reward update in one of our tests.

@Soupstraw @lehins reverting the blooper of the STS assertion to a GHC assert does indeed reveal in cabal test cardano-ledger-shelley-test --test-options '-p TICKF' that the assert is triggered. This failure was earlier being discarded by the test, which it cannot now, because I turned it into an exception.

-- | This property test checks the correctness of the TICKF transation.
-- TICKF is used by the consensus layer to get a ledger view in a computationally
-- cheaper way than using the TICK rule.
-- Therefore TICKF and TICK need to compute the same ledger view.
propTickfPerservesLedgerView :: NewEpochState ShelleyEra -> Property
propTickfPerservesLedgerView nes =
  let (EpochNo e) = nesEL nes
      slot = slotFromEpoch (EpochNo $ e + 1)
      nes' = setDepositsToObligation (filterEmptyRewards nes)
      tickNes = runShelleyBase $ applySTSTest @(ShelleyTICK ShelleyEra) (TRC ((), nes', slot))
      tickFNes = runShelleyBase $ applySTSTest @(ShelleyTICKF ShelleyEra) (TRC ((), nes', slot))
   in fromMaybe discard $ do
        Right tickNes' <- pure tickNes
        Right tickFNes' <- pure tickFNes
        pure $ currentLedgerView tickNes' === currentLedgerView tickFNes'

testTickF :: TestTree
testTickF = testProperty "TICKF properties" propTickfPerservesLedgerView

I think that the removal of this predicate failure is dependent on refactoring of the pulser. What do you guys think? Is there another way?

---

I didn't see your comment earlier @lehins. 👍