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

Let ledger determine whether Shelley forecasting succeeds #4146

Closed
wants to merge 1 commit into from

Conversation

amesgen
Copy link
Member

@amesgen amesgen commented Nov 14, 2022

Description

Remove a pure exception in Shelley forecasting by letting the ledger determine whether a forecasting operation succeeds instead of reimplementing the stability window check manually.

Historically, it seems this was done this way as at, the slot we are forecasting from, was not simply the tip slot of the given ledger state as it is now, but rather an explicit argument:

https://github.com/input-output-hk/ouroboros-network/blob/0934a3cb1e24ecbfcbab4e40522d99ffcd60feaf/ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Ledger.hs#L322-L326

TODO Potential implications to (double)check:

  • How fast does the Ledger short-circuit on failure?
  • Do we need the new assert?
  • Should we also assert that for < maxFor on success?
  • Right now, there is no obvious function FutureLedgerViewError -> OutsideForecastRange, espc. as FutureLedgerViewError is backed by a (not statically guaranteed to be empty!) list. How desirable is it to change this?

Checklist

  • Branch
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • New tests are added if needed and existing tests are updated
    • If this branch changes Consensus and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
  • 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

@amesgen amesgen force-pushed the amesgen/shelley-forecast-refactor branch from dd0d9b7 to dcb0af6 Compare November 15, 2022 00:02
@amesgen amesgen added the consensus issues related to ouroboros-consensus label Nov 15, 2022
@amesgen
Copy link
Member Author

amesgen commented Nov 22, 2022

See #4146 (comment)

@amesgen amesgen closed this Nov 22, 2022
@amesgen amesgen deleted the amesgen/shelley-forecast-refactor branch November 22, 2022 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants