Fix qKesKesKeyExpiry to not always be null #4909
Conversation
newhoggy
requested review from
dcoutts,
Jimbo4350,
erikd,
JaredCorduan and
CarlosLopezDeLara
as code owners
| hoistEpochInfo (first (Text.pack . show) . runExcept) | ||
| $ Consensus.interpreterToEpochInfo (Consensus.unsafeExtendSafeZone interpreter) | ||
|
|
||
| newtype Unsafe a = Unsafe { unsafe :: a } deriving (Eq, Show) |
There was a problem hiding this comment.
I like this use of a newtype. I think Unsafe is a bit too scary/general of a name though. Perhaps AssumingNoUpcomingHardfork (or something like that)?
There was a problem hiding this comment.
What about newtype BeyondHorizonEpochInfo = BeyondHorizonEpochInfo (EpochInfo (Either Text))? And we can put a comment with a brief explanation behind the name.
There was a problem hiding this comment.
I've decided to replace the word Unsafe to Tentative because that captures the meaning I wanted to convey.
|
|
||
| newtype Unsafe a = Unsafe { unsafe :: a } deriving (Eq, Show) | ||
|
|
||
| unsafeToEpochInfo :: EraHistory CardanoMode -> Unsafe (EpochInfo (Either Text)) |
There was a problem hiding this comment.
Similarly, I'd suggest naming this toEpochAssumingNoUpcomingHardFork or something like that.
There was a problem hiding this comment.
toBeyondHorizonEpochInfo?
There was a problem hiding this comment.
This is now toTentativeEpochInfo.
newhoggy
force-pushed
the
newhoggy/fix-qKesKesKeyExpiry
branch
2 times, most recently
from
eacb364
to
a7637c1
Compare
newhoggy
force-pushed
the
newhoggy/fix-qKesKesKeyExpiry
branch
2 times, most recently
from
49d13e1
to
77d274f
Compare
| -- | A value that is tentative or produces a tentative value if used. These values | ||
| -- are considered accurate only if some future event such as a hard fork does not | ||
| -- render them invalid. | ||
| newtype Tentative a = Tentative { tentative :: a } deriving (Eq, Show) |
There was a problem hiding this comment.
What other values do we expect to be tentative besides EpochInfo?
There was a problem hiding this comment.
Potentially anything derived from Tentative EpochInfo is a candidate.
newhoggy
force-pushed
the
newhoggy/fix-qKesKesKeyExpiry
branch
from
77d274f
to
e5e4564
Compare
| -- tentative because it uses an interpreter that is extended past the horizon. | ||
| -- This interpreter will compute accurate values into the future as long as a | ||
| -- a hard fork does not happen in the intervening time. Those values are thus | ||
| -- "tentative" because they can change in the event of a hard fork. |
There was a problem hiding this comment.
Just to clarify my understanding, they can change in the event of a hardfork occurring within the horizon?
There was a problem hiding this comment.
They can change in the event of a hardfork occurring after the horizon. Normally the interpreter refuses to give an answer beyond the horizon so we construct a different interpreter that ignores the horizon.
Values before the horizon still won't change.
However, because the type allows for values beyond the horizon to change, it is annotated as Tentative regardless of whether the value can change or not.
Note that
qKesKesKeyExpiryis only an estimate that is usually correct, but it could be wrong due to a hard fork that changes KES parameters.The fix involves uses an unsafe query interpreter that ignores forecasting horizon. Therefore a new
Unsafenewtypehas been introduced to clearly show all the code that uses the unsafe query interpreter so that it doesn't accidentally get used where it shouldn't.Resolves #4396