Incorrect conversion of validity interval's upper bound

[ak3n]

Recently, we have switched to cardano-ledger in plutus-apps instead of using our own validation rules. We have found that transVITime behaviour seems to be incorrect.

It takes the upper bound and uses PV1.to function that creates a closed upper bound.

transVITime pp ei sysS (ValidityInterval SNothing (SJust i)) = do
  t <- slotToPOSIXTime pp ei sysS i
  pure $ PV1.to t

...

to :: a -> Interval a
to s = Interval (LowerBound NegInf True) (upperBound s)

upperBound :: a -> UpperBound a
upperBound a = UpperBound (Finite a) True -- Boolean flag, whether a bound is inclusive or not.

While the validity interval is defined as a half open interval, open on the top.

-- | ValidityInterval is a half open interval. Closed on the bottom, open on the top.
--   A SNothing on the bottom is negative infinity, and a SNothing on the top is positive infinity
data ValidityInterval = ValidityInterval
  { invalidBefore :: !(StrictMaybe SlotNo),
    invalidHereafter :: !(StrictMaybe SlotNo)
  }

The proper implementation should be

transVITime pp ei sysS (ValidityInterval SNothing (SJust i)) = do
  t <- slotToPOSIXTime pp ei sysS i
  pure $
    PV1.Interval
      (PV1.LowerBound PV1.NegInf True)
      (PV1.strictUpperBound t)

(implemented in https://github.com/ak3n/cl/commit/53d324d5a820105a640ae8f36088006eacb0a400 and tested with plutus-apps test-suite)

It seems it can't be easily fixed as it would change the on-chain behaviour.

[JaredCorduan]

I agree, this is a bug. The ledger is translating [-∞, ttl_slot) to [-∞, ttl_posix] inside the transaction context. Thank you for raising this @ak3n ! We will do our best to fix this at the next hardfork.

[koslambrou]

@JaredCorduan Is this something we can work on so that we can be sure it's going into the next HF?

[JaredCorduan]

@JaredCorduan Is this something we can work on so that we can be sure it's going into the next HF?

yes, absolutely, thank you! I think it should be an easy fix.

I suspect that this bug has never actually be exercised, though y'all might know better than me. In circumstances where a bug requires a hard fork to fix, and the fix is extremely localized, I recommend using the HardFork module technique. What's great is that transVITime already takes the protocol parameters as an input, so the change will be very minimal.

The ideas is very simple, we just switch on the major protocol version. We don't have time to get this into the upcoming intra-era hardfork, so we'll need to switch on major protocol version 9. Before 9, we support the bug, starting at 9 we do the correct thing. After the hard fork, we can go back and see if the bug was ever exercised. If it was not, we can remove the switch altogether. If not, no worries, we just have to live with the wart forever.

The fix is probably about the same number of characters as this comment, so if you'd rather leave it to the ledger team, that's fine too. But I'm always excited to get more understanding between teams on how things work :).