Rework slotting functions

[rvl]

Overview

I am looking at the changes in #2245 related to slotting.

The way the error handling is, it seems like we are walking around with our shoelaces tied together.

Times relative to system start

Part of the problem is that converting a UTC time into a time relative to the system start can fail. (UTCTimeToRel :: UTCTime -> Qry (Maybe Cardano.RelativeTime)). But, really in most cases it won't. The best way to deal with that is to push relative time conversions outside of the queries. Then the caller of the slotting can make decisions about how to get a RelativeTime.

Things like converting a slot range to a time range become much simpler.

Shorten Slotting module.

We already chopped out a lot of old code in #2351.

But if we use unembellished types from ouroboros-consensus, this module can be even shorter. Qry is now just a ReaderT wrapper around HF.Qry.

Running a query is to just apply the start time parameter to runReaderT and then call Ouroboros.Consensus.HardFork.History.Qry.interpretQuery.

Mixing with IO

Sometimes we want to know the chain-relative time for "now". A simplification is to assume that "now" is always after the start time. In the corner case, it suffices to use the relative time of 0. Now callers of this function don't need to worry about whether it can fail somehow.

Relative times are usually better - e.g. Sync progress

The calculation of sync progress with UTC is something like (b - a - s) / (b - s). But with relative times it's just a / b.

Todo

Separate running a query from obtaining the time interpreter

Currently the TimeInterpreter is fetched from the network layer through an IORef (or similar). Then the query is run in IO, when most of the time it's a pure computation (due to the changes above). This just muddles things up.

Stake pool retirement time

Because stake pool retirement epochs are almost certainly past time safe zone, we need to use the unsafeExtend function to assume that the current epoch is the final epoch.

Transaction expiry (TTL)

Expiry times need to be converted fo/from slots. But these are rarely going to be outside of the safe zone. If they are, it is fine to refuse to convert, and show "Nothing" (for incoming tx) or reject the payment request (for outgoing tx).

[Anviking]

Ok, separating the RelativeTime Maybe problem seems fair.

Currently the TimeInterpreter is fetched from the network layer through an IORef (or similar). Then the query is run in IO, when most of the time it's a pure computation (due to the changes above). This just muddles things up.

Maybe. I think there's value in allowing the TimeInterpreter including the IORef to escape from the NetworkLayer and to be modified by combinators.

[Anviking]

Ok, neat!

[Anviking]

Rebasing #2245 I found that if m ~ ExceptT PastHorizonException IO then suddenly the currentRelativeTime can fail… with PastHorizonException, which doesn't make sense.

I changed it to

currentRelativeTime :: MonadIO m => TimeInterpreter n -> m RelativeTime

but we can deal with that in #2245.

[rvl]

OK thanks.
Maybe we should change MonadIO to just IO, or MonadTime (from io-sim-classes package).

[Anviking]

@rvl I rebased off master and added a hint fix commit. I'll leave it to you to squash and merge.

Although the tests are suddenly failing 🤔 That seems weird.

test/unit/Cardano/DB/Sqlite/DeleteSpec.hs:45:9:
--
  | 1) Cardano.DB.Sqlite.Delete.RefCount waitForFree waits for withRef to finish
  | expected: ((),False)
  | but got: ((),True)
  |  
  | To rerun use: --match "/Cardano.DB.Sqlite.Delete/RefCount/waitForFree waits for withRef to finish/"
  |  
  | Randomized with seed 2076715646

[rvl]

Thanks.
I reverted the change I made to a slotRangeFromTimeRange test case, and fixed the code accordingly. And squashed everything.

I have never seen that test failure locally. It looks like a new one.

bors r+

[iohk-bors]

Build succeeded:

• ci/hydra-build:required
• buildkite/cardano-wallet