Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix epoch rule and tests #3801

Merged
merged 7 commits into from
Oct 24, 2023
Merged

Fix epoch rule and tests #3801

merged 7 commits into from
Oct 24, 2023

Conversation

lehins
Copy link
Contributor

@lehins lehins commented Oct 21, 2023 •
edited

Description

A whole collection of bug is fixed in this PR:

In the EPOCH Rule

  • totalObligation for the utxosDeposited field was computed before proposal deposits were returned
  • DRepPulser was started with a state that was not fully updated. Creation of the pulser must be the last step in the EPOCH rule
  • DRepPulser was started with an epoch number value that was one into the future, which would result in proposals expiring one epoch too soon and theoretically, although very unlikely, could affect DRep expiry as well.

In the translation

When NewEpochState was translated from Babbage to Conway, only the pparams in the EnactState were updated, but not the state of the Pulser. Which normally would result in PParams to be reset at the very first Epoch in Conway, however, together with a bug in the EPOCH rule this resulted in alternation of emptyPParams with actual PParams

In testing

  • PParams where not set properly. Similar problem to the one mentioned above. Only the PParams in the EnactState where updated, but not in the DRepPulser. This bug was present both in the modifyPParams function and in the emptyImpNES
  • ConwayFeatures in cardano-ledger-test had wrong assumptions on the state of proposals.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
    versioning process.
  • The version bounds in .cabal files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
  • All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages. New section is never added with the code changes. (See RELEASING.md)
  • Code is formatted with fourmolu (use scripts/fourmolize.sh)
  • Cabal files are formatted (use scripts/cabal-format.sh)
  • hie.yaml has been updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

@lehins lehins changed the title Fix epoch rule and imp testing Fix epoch rule and tests Oct 22, 2023
@lehins lehins force-pushed the lehins/fix-epoch-rule-and-imp-testing branch from 0599fc8 to 3789175 Compare October 22, 2023 01:33
@lehins lehins marked this pull request as ready for review October 22, 2023 01:34
TimSheard
TimSheard approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@TimSheard TimSheard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardest thing to follow is what is the state of the pulser at the beginning of time. Do we start in the epoch rule? or just after the the epoch rule has run. The state of the pulser will have to be different, if we choose one or the other.

eras/conway/impl/src/Cardano/Ledger/Conway/Governance.hs Show resolved Hide resolved
eras/conway/impl/src/Cardano/Ledger/Conway/Translation.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-test/src/Test/Cardano/Ledger/Examples/ConwayFeatures.hs Show resolved Hide resolved
libs/cardano-ledger-test/src/Test/Cardano/Ledger/Examples/ConwayFeatures.hs Show resolved Hide resolved
@lehins
Copy link
Contributor Author

lehins commented Oct 23, 2023

The hardest thing to follow is what is the state of the pulser at the beginning of time.

The state of the pulser at the beginning of time is in complete state with everything empty and the EnactState matches the EnactState in the GovState

Do we start in the epoch rule? or just after the the epoch rule has run. The state of the pulser will have to be different, if we choose one or the other.

Putting initialization of pulser at the beginning of the EPOCH rule will not make it any simpler, since no matter how you set it up, the very first thing we must do is actually extract the result of the previous pulser, because that is what we need to enact in the EPOCH rule. So, you still have the same problem of what is your initial state of the pulser

@lehins lehins force-pushed the lehins/fix-epoch-rule-and-imp-testing branch from 3789175 to a237b92 Compare October 23, 2023 20:31
lehins added a commit that referenced this pull request Oct 23, 2023
@lehins
Add changelog for recent #3759 PR as well as for this one #3801
7d4685c
@lehins lehins force-pushed the lehins/fix-epoch-rule-and-imp-testing branch from a237b92 to 7d4685c Compare October 23, 2023 20:59
@lehins lehins enabled auto-merge October 23, 2023 21:00
@lehins
ConwayEPOCH restructure. Fixing a collection of bugs:
b4754a3 
* Redo how we construct DRepPulser
* PParams are now properly updated and rotation of enact state is correct
* `totalObligation` for the `utxosDeposited` field was computed before
  proposal deposits were returned
* `DRepPulser` was started with a state that was not fully updated. Creating
  of the pulser must be the last step in the EPOCH rule
* `DRepPulser` was started with an epoch number value that was one into the
  future, which resulted in proposals expiring one epoch too soon and
  theoretically, although very unlikely, could have affect DRep expiry as well.
* When `NewEpochState` was translated from Babbage to Conway, only the pparams
  in the `EnactState` were updated, but not the state of the Pulser. Which
  normally would result in `PParams` to be reset at the very first Epoch in
  Conway, however, together with a bug in the EPOCH rule this resulted in
  alternation of `emptyPParams` with actual `PParams`
@lehins
Fix PParams modification in ImpTest in all eras.
ded4722 
* Prior to Conway eras where also changing the previous params, which is
  not needed.
* Conway ImpTest did not chnage the future PParams
* Also change a misleading name of a test helper `isGovActionAccepted`
  to `canGovActionBeDRepAccepted`
@lehins
Make ratify rule a bit more consistent
52b20ca 
* Also add ratification info to logging in imp testing
@lehins
Cleanup and reorganize EPOCH rule
00fac22
@lehins
Fix ConwayFeatures tests to account for recent fixes
92d4d68
@lehins
Add changelog for recent #3759 PR as well as for this one #3801
8b3bbe8
@lehins lehins force-pushed the lehins/fix-epoch-rule-and-imp-testing branch from 7d4685c to aafef22 Compare October 23, 2023 21:15
@lehins lehins mentioned this pull request Oct 23, 2023
Merged
9 tasks
@lehins lehins force-pushed the lehins/fix-epoch-rule-and-imp-testing branch from aafef22 to baac6be Compare October 24, 2023 10:54
@lehins lehins merged commit 8415ca5 into master Oct 24, 2023
10 of 15 checks passed
@iohk-bors iohk-bors bot deleted the lehins/fix-epoch-rule-and-imp-testing branch October 24, 2023 13:11
@lehins lehins mentioned this pull request Oct 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimSheard TimSheard TimSheard approved these changes

@aniketd aniketd Awaiting requested review from aniketd

@teodanciu teodanciu Awaiting requested review from teodanciu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@lehins @TimSheard