mlabs-haskell / clb

Cardano Ledger Backend (next-gen PSM)
7 stars 1 forks source link

Fix interval translation #8

Closed uhbif19 closed 3 months ago

uhbif19 commented 4 months ago

Current translation is not matching ledger logic.

One should use Cardano.Ledger.Alonzo.Plutus.TxInfo (transValidityInterval) or Cardano.Ledger.Plutus.TxInfo (slotToPOSIXTime) .

uhbif19 commented 3 months ago

@euonymos I removed that function, but seems like it was not used anywhere. Seems like applyTx should validate it by itself, but I could not find how so far.

euonymos commented 3 months ago

Sorry, which one did you remove? I think it works like that:

  1. transValidityInterval is called when the ledger builds PlutusTxInfo with toPlutusTxInfo and then the following functions finally lead us to the UTXOS rule, which is part of what applyTx is doing:
  2. mkPlutusLanguageContext
  3. mkPlutusScriptContext
  4. collectPlutusScriptsWithContext
  5. scriptsTransition
  6. alonzoEvalScriptsTxValid
  7. utxosTransition

But slotToPOSIXTime in its turn, calls epochInfoSlotToUTCTime, which comes from the consensus layer. That's right, there is no definition for this function in the ledger's codebase. It comes from Cardano.Slotting.EpochInfo module. Finally, it's a field of data EpochInfo m. So this answers your question. It uses "fixed" EpochInfo from Cardano.Slotting.EpochInfo.Impl module which is supposed to be used for "trivial cases, such as in mocks, tests, etc":

-- | The 'EpochInfo' induced by assuming the epoch size and slot length are
-- fixed for the entire system lifetime
fixedEpochInfo :: Monad m => EpochSize -> SlotLength -> EpochInfo m
fixedEpochInfo (EpochSize size) slotLength = EpochInfo
  { epochInfoSize_ = \_ ->
      return $ EpochSize size,
    epochInfoFirst_ = \e -> return $ fixedEpochInfoFirst (EpochSize size) e,
    epochInfoEpoch_ = \sl -> return $ fixedEpochInfoEpoch (EpochSize size) sl,
    epochInfoSlotToRelativeTime_ = \(SlotNo slot) ->
      return $ RelativeTime (fromIntegral slot * getSlotLength slotLength),
    epochInfoSlotLength_ = const $ pure slotLength
  }

So we can install whatever handler we want to.

uhbif19 commented 3 months ago

Sorry, which one did you remove?

All except SlotConfig and refactored stuff for EpochInfo construction you mentioned. And utils. Seems like it was not used, cuz ledger already checks that. And if it does not EpochInfo would cover it, right now all module was not used .

Thing is CEM/cardano-api need another datatype. And it was recently changed lol :D So from SlotConfig we can get both if needed.

I think it works like that:

Yes, I traced most of it :D I think we should document that. This stuff is needed for Plutus time/TxInfo translation. I am wondering about checking real slot. I found it in Ledger as well, so our slot changing code should work just well (but we should probably test it, I documented it in #21).

uhbif19 commented 3 months ago

Quote from my internal notes on matter:

Three datatypes:

* Exported `Interpreter` and hidden `Summary` in ouroboros.
  * `Ouroboros.Consensus.HardFork.History.Summary` contains real logic.
  * Can be queried via DSL in `Ouroboros.Consensus.HardFork.History.Qry`.
  * `summaryToEpochInfo` can be used for conversion
* EraHistory - in cardano-api
  * ...
* EpochInfo - unifying functionalization for all of them, non-serializable in `cardano-base`.
  * Used by `applyTx` and can be used by our code.

Some ADR on this datatypes for `plutus-apps` emulator:
https://github.com/IntersectMBO/plutus-apps/blob/dbafa0ffdc1babcf8e9143ca5a7adde78d021a9a/doc/adr/0012-time-conversion.rst

Where can I push such notes? Is Plutomicon appropriate place? Or I can store it in CEM docs.

uhbif19 commented 3 months ago

@euonymos

Also seems like ouroboros slot counting code (I did not found exact place, only docs) gets one slot per block. So IRL slots are changed with block approved.

Seems like our single-mempool should work, cuz it only contains UTxO state, but real cardano-node somehow updates with each block. Also there is some "ticks", but they semm irrelevant to us.

euonymos commented 3 months ago

I am wondering about checking real slot. I found it in Ledger as well, so our slot changing code should work just well (but we should probably test it, I documented it in #21).

Sorry, what's "a real slot" @uhbif19?

euonymos commented 3 months ago

@uhbif19 I suggest we should provide several ways to count slots:

  1. Each tx ticks a slot - as it was done in PSM
  2. Manual mode - test code should use awaitSlot
  3. (Ability to provide a custom handler that does it desirably?)
uhbif19 commented 3 months ago

@euonymos

I agree, but why not leave this on level of user lib (Atlas/CEM)? For example it might want to fuzz slots.

Also another two options:

  1. Changing time with MonadTimetravel and deriving slot number from that (makes time-based tests/configs much simpler, you want to wait for specific time, not only slot)
  2. Seems like real node somehow advances in blocks, not slots. This might be useful for testing.
euonymos commented 3 months ago

In my opinion, when you are dealing with issues like that the rule of thumb is to provide "sensible defaults" that users can replace if they need to. Or a set of options they can choose from and/or provide their implementation. Option (1) on my list is a time-tested solution that has been working for PSM, let's implement it and use it by default. Does it make sense?

RE TimeMachine -- we cannot support traveling in the past, am I wrong? So we need to restrict what a strategy supplied by a user can do and what cannot.

RE blocks: good point @uhbif19 ! This was missing in the PSM by the way, but this totally makes sense to me. If we don't have the notion of blocks some applications/tests may fail. Let me open a task.

uhbif19 commented 3 months ago

we cannot support traveling in the past

Why not? Slot is just a number in ledger state, which is now set to arbitrary number, is not it?

let's implement it and use it by default

I do not understand why CLB should implement any strategy, because such config options should be reflected in CEM/Atlas again. So config code would be just done twice. I thought that this was idea of CLB, to delegate stuff like that.

Also I do not understand for which time-constrained scripts incrementing slot works. Plutus forces you to work with POSIXTime, so you using slots for it is not simple and requires using conversion utils. And usually scripts take use intervals much more than one slot, actually my research gave me impression that no real scripts use slot-length granularity at all, because otherwise somebody else would probably report same problems with POSiXTime normalization.

euonymos commented 3 months ago

Why not? Slot is just a number in ledger state, which is now set to arbitrary number, is not it?

Two reasons off the top of my head:

  1. It complicates things whereas buys next to nothing. Are there any particular use cases on your mind one might want to have it at all?
  2. A real node won't do that under any circumstance, hence it's not clear whether the ledger will continue to operate properly if we do that.

let's implement it and use it by default

I do not understand why CLB should implement any strategy, because such config options should be reflected in CEM/Atlas again. So config code would be just done twice. I thought that this was idea of CLB, to delegate stuff like that.

Node runs the time. CLB is an emulator of a node. Why do you think it should delegate this to users? My idea was to provide users with sensible defaults, so they can use it (not reimplement) if it works for them well and opt for something custom in the case they need it.

But I don't see the border where we should stop and ask for users' stuff, would be great if we could stipulate it more clearly. Everything needed to implement Ouroborous mini-protocols should be considered (customizable) internals. Time management falls under that category, because acquireLedgerState returns "points" which contain SlotNo:

newtype Point block = Point
    { getPoint :: WithOrigin (Point.Block SlotNo (HeaderHash block))
    }
  deriving (Generic)

But I might be mistaken, let's try to elaborate on this.

Also I do not understand for which time-constrained scripts incrementing slot works. Plutus forces you to work with POSIXTime, so you using slots for it is not simple and requires using conversion utils.

That's a good point indeed! This strategy won't work for time-constrained scripts but might be useful for tracing. So what do such scripts need? I guess instead of awaitSlot, could they need awaitTime? Would you like us to reopen this issue?