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.

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

Tame TransitionImpossible et al #364

Closed
wants to merge 1 commit into from
Closed

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented Sep 23, 2023

Single commit PR; see the message.

@nfrisby nfrisby requested a review from a team as a code owner September 23, 2023 00:22
Copy link
Contributor Author

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

A couple quick TODOs

oneForecast sno pairs (Current start AnnForecast{..}) =
case annForecastEnd of
oneForecast sno staticTransitions crosses (Current start AnnForecast{..}) =
(\k -> maybe (neverEnd <$> forecastFor annForecast sno) k annForecastEnd) $ \case
Copy link
Contributor Author

@nfrisby nfrisby Sep 23, 2023

Choose a reason for hiding this comment

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

Accept the additional index indent for the unchanged lines instead of golfing.

@@ -583,7 +624,8 @@ inspectHardForkLedger = go
map liftEvent $
inspectLedger c (currentState before) (currentState after)

, case (pss, confirmedBefore, confirmedAfter) of
-- TODO assert currentStart before == currentStart after?
, (\k -> case singleEraTransition' pc ps (currentStart before) of FixedTransition{} -> []; EventualTransition f -> k f) $ \f -> case (pss, f (currentState before), f (currentState after)) of
Copy link
Contributor Author

@nfrisby nfrisby Sep 23, 2023

Choose a reason for hiding this comment

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

Accept the additional index indent for the unchanged lines instead of golfing.

This commit renames TransitionImpossible to TransitionNever for two reasons.
First, this is a more precise name. Second, this commit refines the
singleEraTransition method so that TransitionNever is actually an absorbing
state. Before this commit, TransitionImpossible could arise under certain
circumstances but then the TransitionInfo for _the same era_ could later become
TransitionUnknown. But now it is invariant that once the TransitionInfo for an
era is ever TransitionNever then it always will be and moreover that era will
indeed never end.

This invariant is enforced in two ways. singleEraTransition can now induce
TransitionNever, but only when that decision is independent of the ledger
state, ie it must be a function only of inputs that the HFC does not vary
(ledger config, EraParams, start of the era). The only other place
TransitionNever arises is in the Unary.Isomorphism class, and it now has a law
requiring that singleEraTransition only induce TransitionNever.
TriggerHardForkNever -> Nothing
TriggerHardForkAtEpoch epoch -> Just epoch
TriggerHardForkAtVersion shelleyMajorVersion ->
TriggerHardForkNever -> FixedTransition Nothing
Copy link
Contributor Author

@nfrisby nfrisby Sep 25, 2023

Choose a reason for hiding this comment

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

Bah! This line would reintroduce the bug fixed by IntersectMBO/ouroboros-network#3754

For example, I think TriggerHardForkNever should be renamed to TriggerHardForkCannotBeKnown. Either that, or protocolInfoCardano should use TriggerHardForkAtVersion (maxBound?) even for the last era. 🤔

@amesgen
Copy link
Member

amesgen commented Oct 11, 2023

Partly superseded by #396, closing for now, but can serve as inspiration in the context of #420.

@amesgen amesgen closed this Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants