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

Provide an 'EpochInfo' that can fail to ledger. #3770

Merged
merged 1 commit into from
May 26, 2022
Merged

Conversation

nc6
Copy link
Contributor

@nc6 nc6 commented May 25, 2022

The ledger now supports an EpochInfo (Either Text), and uses the
Left constructor to indicate a horizon error. Unfortunately, while the
epoch info being provided had the correct type, it was implemented in
a degenerate way - the Left constructor would never be used and
instead a pure error was thrown on failures.

This PR instead provides a correct EpochInfo to the ledger, where the
failure is reflected in the Left constructor.

The ledger now supports an `EpochInfo (Either Text)`, and uses the
`Left` constructor to indicate a horizon error. Unfortunately, while the
epoch info being provided had the correct _type_, it was implemented in
a degenerate way - the `Left` constructor would never be used and
instead a pure error was thrown on failures.

This PR instead provides a correct `EpochInfo` to the ledger, where the
failure is reflected in the `Left` constructor.
@nc6 nc6 requested review from nfrisby and dnadales as code owners May 25, 2022 11:25
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

LGTM.

To be more comprehensive I think we should audit the other uses of toPureEpochInfo, and indeed perhaps rename it to make it clear it throws exceptions. We should file a TODO issue for that, it doesn't need to be in this PR of course.

@nc6
Copy link
Contributor Author

nc6 commented May 25, 2022

I did go through the other uses of toPureEpochInfo and think they are all legit. We otherwise use it only for epoch/slot computations for the current slot.

@nc6
Copy link
Contributor Author

nc6 commented May 25, 2022

bors merge

iohk-bors bot added a commit that referenced this pull request May 25, 2022
3770: Provide an 'EpochInfo' that can fail to ledger. r=nc6 a=nc6

The ledger now supports an `EpochInfo (Either Text)`, and uses the
`Left` constructor to indicate a horizon error. Unfortunately, while the
epoch info being provided had the correct _type_, it was implemented in
a degenerate way - the `Left` constructor would never be used and
instead a pure error was thrown on failures.

This PR instead provides a correct `EpochInfo` to the ledger, where the
failure is reflected in the `Left` constructor.

Co-authored-by: Nicholas Clarke <[email protected]>
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

Hmm. I wonder if SL.mkShelleyGlobals used to take a pure EpochInfo and was changed to take an Either one, and the integration PR for that changed missed this because toPureEpochInfo is polymorphic.

So, we could try changing toPureEpochInfo to return Identity instead of Applicative f => f to try to prevent similar from happening again.

I created https://input-output.atlassian.net/browse/CAD-4433 for this tech debt ^^^.

@nfrisby
Copy link
Contributor

nfrisby commented May 25, 2022

I did go through the other uses of toPureEpochInfo and think they are all legit. We otherwise use it only for epoch/slot computations for the current slot.

I see a few remaining uses of toPureEpochInfo.

  • tickChainDepState @Praos passes it to isNewEpoch to check whether we're ticking past an epoch boundary. It converts the slot we're ticking to an epoch (which could fail) and compares epochs -- I think it should instead convert the end of the current epoch to a slot (which can't fail) and then compare slots

  • checkIsLeader @TPraos uses it to compute the firstSlot of the epoch of the given slot when calling SL.lookupInOverlaySchedule firstSlot .... As in the previous case, that translation can also fail, but I don't see an alternative that can't fail.

  • tickChainDepState @TPraos uses it as does tickChainDepState @Praos above

So I think we should fixup the tickChainDepState calls to use unfailing translations instead of failing translations.

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 25, 2022

Timed out.

@nc6
Copy link
Contributor Author

nc6 commented May 26, 2022

bors retry

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 26, 2022

@iohk-bors iohk-bors bot merged commit 3c043a3 into master May 26, 2022
@iohk-bors iohk-bors bot deleted the nc/epochTime branch May 26, 2022 08:32
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.

3 participants