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

Provide slot-to-time conversion to ledger #3036

Merged
merged 1 commit into from
Apr 27, 2021
Merged

Provide slot-to-time conversion to ledger #3036

merged 1 commit into from
Apr 27, 2021

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented Apr 6, 2021 •
edited
Loading

Fixes #3052.

Addresses CAD-2713. The ledger needs to be able to convert slots to UTC times, so as of this PR, the HFC now provides that to the ledger. See the commit message for more details.

@nfrisby nfrisby added the consensus issues related to ouroboros-consensus label Apr 6, 2021
@nfrisby
Copy link
Contributor Author

nfrisby commented Apr 6, 2021

This depends on two upstream PRs which I haven't opened yet. I want to review this PR with the team, to decided if the results of this initial plan look like we had anticipated.

In particular, having to provide a slot length in order to create even a degenerate, fixed EpochInfo creates some awkward partial functions in this draft PR. I'm not excited about it, at the moment. I'm wondering if we should create a separate SlotInfo, analogous to EpochInfo. The HFC always supports both, but we're apparently currently using EpochInfo in non-HFC contexts (eg in tests) that don't necessarily have a slot-length easily/clearly available. The other option is to determine that there should be a clear way to provide slot length to even those use cases and do the necessary diff to pipe it through.

@nfrisby
Copy link
Contributor Author

nfrisby commented Apr 6, 2021 •
edited
Loading

The tracking issue in a GitHub repository for this work is IntersectMBO/cardano-base#205

@nfrisby nfrisby force-pushed the nfrisby/CAD-2713-time-conversions-in-EpochInfo branch 2 times, most recently from 9da0afb to b1d7cec Compare April 7, 2021 01:02
@nfrisby
Copy link
Contributor Author

nfrisby commented Apr 8, 2021

This is Blocked on IntersectMBO/cardano-ledger#2223

cc: @nc6 @polinavino I opened that PR, but I'm hoping the Ledger team will finish it off without me. My initial commit there just gets it to build; in particular it does not yet use the new epochInfoSlotToRelativeTime field for its intended purpose regarding Plutus script execution.

@nc6
Copy link
Contributor

nc6 commented Apr 22, 2021

The PR has now been merged in the ledger, so this commit is good to go.

@nfrisby nfrisby force-pushed the nfrisby/CAD-2713-time-conversions-in-EpochInfo branch from b1d7cec to f873a40 Compare April 26, 2021 22:53
nfrisby
nfrisby commented Apr 26, 2021
View reviewed changes
cabal.project
@@ -204,10 +206,17 @@ source-repository-package
tag: f73079303f663e028288f9f4a9e08bcca39a923e
--sha256: 1n87i15x54s0cjkh3nsxs4r1x016cdw1fypwmr68936n3xxsjn6q

-- The r0 revision of this quickcheck-state-machine-0.7 on Hackage adds a lower
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As John Ky noted elsewhere, we no longer need this since we're only on GHC 8.10 now.

nfrisby
nfrisby commented Apr 26, 2021
View reviewed changes
ouroboros-consensus/src/Ouroboros/Consensus/BlockchainTime/WallClock/Types.hs
@@ -26,49 +25,16 @@ module Ouroboros.Consensus.BlockchainTime.WallClock.Types (
, SlotLength
) where

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this module's content was recently upstreamed to https://github.com/input-output-hk/cardano-base/blob/master/slotting/src/Cardano/Slotting/Time.hs

@nfrisby nfrisby force-pushed the nfrisby/CAD-2713-time-conversions-in-EpochInfo branch from f873a40 to 410c8b6 Compare April 26, 2021 23:04
@nfrisby nfrisby marked this pull request as ready for review April 26, 2021 23:05
@nfrisby nfrisby force-pushed the nfrisby/CAD-2713-time-conversions-in-EpochInfo branch from 410c8b6 to 610fd58 Compare April 26, 2021 23:46
@nfrisby
Update cardano-ledger-specs and add SystemStart to TPraosParams
7cea373 
Most of the content that this commit removes from
`Ouroboros.Consensus.BlockchainTime.WallClock.Types` was recently upstreamed in
PR IntersectMBO/cardano-base#212. This PR updates `ouroboros-network` in
turn.

Combined with PR IntersectMBO/cardano-ledger#2223, this commit will
discharge CAD-2713 via the new `EpochInfo` field `epochInfoSlotToRelativeTime_`
et al.

As of PR IntersectMBO/cardano-ledger#2223 being merged, the Shelley
`Globals` now requires a `SystemStart`. In order to pipe it through to our
ledger invocations, we must add it to `ConsensusConfig` for `TPraos`.
@nfrisby nfrisby force-pushed the nfrisby/CAD-2713-time-conversions-in-EpochInfo branch from 610fd58 to 7cea373 Compare April 26, 2021 23:47
@nfrisby nfrisby removed request for coot and karknu April 26, 2021 23:49
@nfrisby nfrisby mentioned this pull request Apr 26, 2021
Closed
@nfrisby nfrisby force-pushed the nfrisby/CAD-2713-time-conversions-in-EpochInfo branch from 2076877 to 7cea373 Compare April 27, 2021 15:13
@EncodePanda
Copy link
Contributor

Fixes #3096

EncodePanda
EncodePanda approved these changes Apr 27, 2021
View reviewed changes
Copy link
Contributor

@EncodePanda EncodePanda left a comment

Choose a reason for hiding this comment

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

Reviewed on meet call

@nfrisby
Copy link
Contributor Author

nfrisby commented Apr 27, 2021

bors r+

iohk-bors bot added a commit that referenced this pull request Apr 27, 2021
@iohk-bors @nfrisby
Merge #3036
666c761 
3036: Provide slot-to-time conversion to ledger r=nfrisby a=nfrisby

Fixes #3052.

Addresses CAD-2713. The ledger needs to be able to convert slots to UTC times, so as of this PR, the HFC now provides that to the ledger. See the commit message for more details.

Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 27, 2021

Build failed:

@nfrisby
Copy link
Contributor Author

nfrisby commented Apr 27, 2021 •
edited
Loading

Ugh. the Ouroboros.Consensus.Cardnao.Node build timed out; cf Issue IntersectMBO/ouroboros-consensus#606.

I'll get Hydra to try again.

PS - I did, and it succeeded. But bors apparently didn't notice.

@nfrisby
Copy link
Contributor Author

nfrisby commented Apr 27, 2021

bors retry

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 27, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 9b279c7 into master Apr 27, 2021
@iohk-bors iohk-bors bot deleted the nfrisby/CAD-2713-time-conversions-in-EpochInfo branch April 27, 2021 17:55
@nfrisby nfrisby linked an issue Apr 27, 2021 that may be closed by this pull request
Update ledger-spec dependency #3096
Closed
@nfrisby nfrisby mentioned this pull request Apr 27, 2021
Closed
@EncodePanda
Copy link
Contributor

awesome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EncodePanda EncodePanda EncodePanda approved these changes

Assignees
No one assigned
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update ledger-spec dependency Update a cardano-base dependency including epochInfoSlotToRelativeTime
3 participants
@nfrisby @nc6 @EncodePanda