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.

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

Use EpochNo instead of SlotNo in Pool, Cert and Certs rules #4757

Closed
wants to merge 1 commit into from

Conversation

teodanciu
Copy link
Contributor

Description

Slightly simplify rules by not passing slotNo around when not required.

Closes #4692

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • 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)
  • 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)
  • 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

@teodanciu teodanciu requested a review from a team as a code owner November 15, 2024 16:46
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

@teodanciu I think you misread the ticket or maybe it just doesn't explain the goal clearly.

The goal is to reduce computation. Se my comment in the PR

Comment on lines +207 to +209
cEpoch <- liftSTS $ do
ei <- asks epochInfoPure
epochInfoEpoch ei slot
Copy link
Collaborator

Choose a reason for hiding this comment

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

The ultimate goal of this PR should be to make this call done only once! That is why description of the ticket says it should be done somewhere in LEDGERS rule.

The point is that we do not want to compute the same value over and over again for every transaction in a block, and here even worse for every certificate in a transaction. (Note that checkSlotNotTooLate also does the same computation with epochInfoEpoch) In other words we need to reduce number of calls to logic in epochInfoEpoch to an absolute minimum.

It does mean that some rules (like DELPL) will have to accept both: current slot and current epoch. That is a small price to pay in complexity for avoiding the same calculation done a whole bunch of times during validation of every block.

@lehins
Copy link
Collaborator

lehins commented Nov 15, 2024

FYI. applyTx from Cardano.Ledger.Shelley.API.Mempool will also be affected by my comment. Which has two alternative solutions that I can think of, but we can talk about if you run into problems in the ApplyTx interface

@teodanciu
Copy link
Contributor Author

Closing, since i redid all in this PR: #4765

@teodanciu teodanciu closed this Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass curEpochNo instead of curSlotNo to all the rules that don't need SlotNo
2 participants