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

Move ticked LedgerView out of ticked ChainDepState #354

Closed
wants to merge 2 commits into from

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented Sep 19, 2023

While reviewing PR #340, we realized the following change might make the ConsensusProtocol class (and how the HFC depends on it) clearer.

The key idea: let Ticked ChainDepState be exactly a ticked ChainDepState and nothing beyond that. This requires also introducing PreparedChainDepState (and sadly PreparedHeaderState) to bear the burden that was formerly on Ticked ChainDepState. While we're at it, we also fold the other "determined" argument (SlotNo) into the Prepared* types, so make the ConsensusProtocol methods even simpler.

@nfrisby
Copy link
Contributor Author

nfrisby commented Sep 19, 2023

The first commit is the core change and everything necessary so that lib:ouroboros-consensus builds. The second commit is propagating the change to the rest of the repository.

The key benefit is that the Ticked ChainDepState types no longer need to carry
the LedgerView.

The one downside is that this introduces an error case in the
HardForkCombinator, since PreparedChainDepState will hold a Ticked LedgerView
and a Ticked ChainDepState side-by-side there, and its only guaranteed that
those be in the same era (ie the same summand in the telescope) by the
invariant on PreparedChainDepState.

We consider the resulting exactness of the Ticked ChainDepState to be worth the
impossible alternative in distribPreparedChainDepState. In particular, it will
unblock another refinement we're working on related to ticking ChainDepStates
across era transitions.

P.S. - This is effectively reverting 16d61bf.
@nfrisby nfrisby force-pushed the nfrisby/prepared-chain-dep-state branch from 06b7e9d to 6c0b232 Compare September 20, 2023 18:32
@nfrisby
Copy link
Contributor Author

nfrisby commented Sep 20, 2023

The GHC 9.6 warnings in WithEarlyExit seem to be https://gitlab.haskell.org/ghc/ghc/-/issues/23143

@nfrisby
Copy link
Contributor Author

nfrisby commented Sep 20, 2023

I think this PR is Ready for Review---except I'm having second thoughts about the Ticked type itself! :/

These are our Ticked instances, excluding those for testing.

instance,Ticked,PBftLedgerView
instance,Ticked,Praos.LedgerView
instance,Ticked,TPraos.LedgerView
instance,Ticked,WrapLedgerView
instance,Ticked,HardForkLedgerView_

instance,Ticked,PBftState
instance,Ticked,TPraosState
instance,Ticked,PraosState
instance,Ticked,PraosChainDepState
instance,Ticked,WrapChainDepState
instance,Ticked,HardForkChainDepState
instance,Ticked,ExtLedgerState.HeaderState

instance,Ticked,LedgerState,ByronBlock
instance,Ticked,LedgerState,ShelleyBlock
instance,Ticked,LedgerState,HardForkBlock
instance,Ticked,ExtLedgerState

The ticked ledger views are somewhat suspicious, since LedgerView p is never actually constructed by the Consensus Layer, only Ticked (LedgerView p) is. So, it seems reasonable to simply redefine LedgerView p to be what is currently Ticked (LedgerView p).

The ticked chain-dependent states and ticked ledger states could similarly be redefined as PreHeaderData p and PreBlockData blk families, for the data necessary to validate/apply a header and the data necessary to validate/apply a block.

Thus there'd be no more Ticked data family. Moreover, it would have been replaced by LedgerView p, PreHeaderData p, and PreBlockData blk, at which point it's now much less awkward that PreHeaderData sometimes includes the LedgerView than it was when PreHeaderData was named Ticked (ChainDepState p).

The last remaining motivation for this PR would be for PreHeaderData to not contain a LedgerView so that PR #340 's (tick-translate)*-tick scheme would be able to tick the chain dep state of SingleEraBlocks across era transitions without necessarily having a LedgerView (at the first slot of the new era). However:

  • There are other ways that could be achieved (eg yet another type family PreHeaderDataWithoutLedgerView or some such).
  • We already know that the (tick-translate)*-tick scheme is still somewhat ad-hoc. So I hesitate to use it alone to justify this PR.

@amesgen
Copy link
Member

amesgen commented Sep 21, 2023

Summary from my PoV: I think there are a few possible motivations for this PR:

  1. It is a bit weird that before this PR Ticked (ChainDepState p) contains Ticked (LedgerView p)

This is true, but also not a super important point IMO. Indeed, if we would get rid of Ticked as argued above, it would be even less motivating.

  1. The introduction of HorizonView in SingleEraProtocol becomes very simple with this change.

This was the original motivation to consider this change, but there are alternatives for this purposes, namely the last two bullet points in #354 (comment).

  1. This PR makes it hard(er) to provide inconsistent LedgerView/SlotNo arguments to tickChainDepState and then checkIsLeader/(re)updateChainDepState.

This point is unaffected by whether or not we remove Ticked. Is it enough to justify this PR?

@amesgen
Copy link
Member

amesgen commented Oct 23, 2023

We don't think this is important enough to warrent working on it ATM.

@amesgen amesgen closed this Oct 23, 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.

None yet

2 participants