-
Notifications
You must be signed in to change notification settings - Fork 23
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
First tick across the era boundary when translating the ledger/chain-dep state #340
Conversation
...consensus-cardano/src/ouroboros-consensus-cardano/Ouroboros/Consensus/Cardano/CanHardFork.hs
Outdated
Show resolved
Hide resolved
fad165f
to
5d8e37e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass 👍
...ros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/HardFork/Combinator/State/Types.hs
Show resolved
Hide resolved
...consensus-cardano/src/ouroboros-consensus-cardano/Ouroboros/Consensus/Cardano/CanHardFork.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/HardFork/Combinator.hs
Outdated
Show resolved
Hide resolved
@@ -205,10 +206,11 @@ injectInitialExtLedgerState cfg extLedgerState0 = | |||
|
|||
targetEraLedgerState :: LedgerState (HardForkBlock (x ': xs)) | |||
targetEraLedgerState = | |||
HardForkLedgerState $ | |||
-- TODO discarding ledger events, probably fine? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also guess it's fine, but it's slightly tricky since this whole function is an (important) optimization (for the benchmarking team)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a justification.
|
||
-- | Extend the telescope until the specified slot is within the era at the tip | ||
-- | ||
-- TODO documentation (in particular that this now does ticking!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO agreed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be there now 👍
-> SlotNo | ||
-> HardForkState LedgerState xs | ||
-> LedgerResult (LedgerState (HardForkBlock xs)) (HardForkState LedgerState xs) | ||
extendToSlot ledgerCfg@HardForkLedgerConfig{..} slot ledgerSt@(HardForkState st) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I anticipate we should be ticking N times if we do N translations. Do you agree?
And I think I see that natural pairing in howExtend
. Would you add a comment there highlighting it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just a note to self: GH labels this Outdated, but the comment isn't there yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be there now 👍
223d5e7
to
2948b49
Compare
7ead180
to
862d6e4
Compare
...oros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/HardFork/Combinator/Embed/Nary.hs
Outdated
Show resolved
Hide resolved
(TPraosState lastSlot st) = | ||
projectHorizonView _cfg _lv = TickedTrivial | ||
|
||
-- TODO comment explaining why we're not using the 'SL.tickChainDepState' function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also TODO: open a PR upstream deleting that function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #350 for the natural larger task.
We could also go the 'opposite" route and add a property test that our vendored tick function behaves just as the one from ledger (for an interim period until #350 is done).
I locally tested that this works (and that introducing bugs in TPraos tickChainDepState
causes the test to fail), but I am not sure if it is worth it:
Click to see compatibility test for TPraos ticking
{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE NamedFieldPuns #-}
{-# LANGUAGE StandaloneDeriving #-}
module Test.Consensus.Protocol.TPraos.LedgerCompat (tests) where
import qualified Cardano.Ledger.BaseTypes as SL
import qualified Cardano.Ledger.Chain as SL
import Cardano.Ledger.Crypto
import qualified Cardano.Protocol.TPraos.API as SL
import Cardano.Slotting.EpochInfo.API (epochInfoEpoch,
generalizeEpochInfo)
import Data.Functor.Identity
import Ouroboros.Consensus.Block
import Ouroboros.Consensus.HardFork.History.EpochInfo
(toPureEpochInfo)
import Ouroboros.Consensus.HardFork.History.Util (addSlots)
import Ouroboros.Consensus.Protocol.Abstract
import Ouroboros.Consensus.Protocol.TPraos
import Test.Cardano.Ledger.Shelley.Utils (testGlobals)
import Test.Cardano.Protocol.TPraos.Arbitrary ()
import Test.QuickCheck
import Test.Tasty
import Test.Tasty.QuickCheck
tests :: TestTree
tests = testGroup "TPraos Ledger compat"
[ testProperty "tick" testTickCompat
]
data TickSetup p = TickSetup {
tsConsensusConfig :: Blind (ConsensusConfig p)
, tsTickedLedgerView :: Ticked (LedgerView p)
, tsSlotNo :: SlotNo
, tsChainDepState :: ChainDepState p
}
instance Crypto c => Arbitrary (TickSetup (TPraos c)) where
arbitrary = do
initialNonce <- arbitrary
let tpraosParams = TPraosParams {
tpraosSlotsPerKESPeriod = SL.slotsPerKESPeriod globals
, tpraosLeaderF = SL.activeSlotCoeff globals
, tpraosSecurityParam = SecurityParam $ SL.securityParameter globals
, tpraosMaxKESEvo = SL.maxKESEvo globals
, tpraosQuorum = SL.quorum globals
, tpraosMaxMajorPV = MaxMajorProtVer $ SL.maxMajorPV globals
, tpraosMaxLovelaceSupply = SL.maxLovelaceSupply globals
, tpraosNetworkId = SL.networkId globals
, tpraosInitialNonce = initialNonce
, tpraosSystemStart = SL.systemStart globals
}
tsConsensusConfig = Blind TPraosConfig {
tpraosParams
, tpraosEpochInfo = generalizeEpochInfo $ SL.epochInfoPure globals
}
chainCheckPParams <-
SL.ChainChecksPParams <$> arbitrary <*> arbitrary <*> arbitrary
tsTickedLedgerView <- fmap TickedPraosLedgerView $
SL.LedgerView <$> arbitrary <*> arbitrary <*> arbitrary <*> arbitrary <*> pure chainCheckPParams
slotAlreadyTickedTo <- frequency
[ (9 , NotOrigin <$> chooseEnum (0, 1000))
, (1, pure Origin)
]
slotOffset <- chooseEnum (0, 200)
chainDepState <- SL.ChainDepState <$> arbitrary <*> arbitrary <*> arbitrary
let tsChainDepState = TPraosState {
tpraosStateLastSlot = slotAlreadyTickedTo
, tpraosStateChainDepState = chainDepState
}
tsSlotNo = slotOffset `addSlots` fromWithOrigin 0 slotAlreadyTickedTo
pure TickSetup {
tsConsensusConfig
, tsTickedLedgerView
, tsSlotNo
, tsChainDepState
}
where
globals :: SL.Globals
globals = testGlobals
testTickCompat :: Blind (TickSetup (TPraos StandardCrypto)) -> Property
testTickCompat (Blind ts) =
consensusImpl === ledgerImpl
where
consensusImpl =
tickedTPraosStateChainDepState
$ tickChainDepState cfg tlv tsSlotNo cds
ledgerImpl =
SL.tickChainDepState
(mkShelleyGlobals cfg)
(getTickedPraosLedgerView tlv)
tickingToNewEpoch
(tpraosStateChainDepState cds)
tickingToNewEpoch =
epochNo (NotOrigin tsSlotNo) /= epochNo (tpraosStateLastSlot cds)
where
ei = toPureEpochInfo $ tpraosEpochInfo cfg
epochNo = runIdentity . epochInfoEpoch ei . fromWithOrigin 0
TickSetup {
tsConsensusConfig = Blind cfg
, tsTickedLedgerView = tlv
, tsSlotNo
, tsChainDepState = cds
} = ts
0d61028
to
d90792b
Compare
bf6c179
to
4e9939d
Compare
4e9939d
to
5deb3cc
Compare
5deb3cc
to
c589290
Compare
-- | A projection of 'LedgerView' containing only what is needed for ticking | ||
-- | ||
-- For single-era protocols, there are further constraints on this type, see | ||
-- 'Ouroboros.Consensus.HardFork.Combinator.Abstract.SingleEraBlock.eraTransitionHorizonView'. | ||
-- Usually, 'HorizonView' will be a singleton type. | ||
-- | ||
-- For the 'Ouroboros.Consensus.HardFork.Combinator.Basics.HardForkProtocol' | ||
-- combinator, this is the same as the 'LedgerView'. | ||
type family HorizonView p :: Type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR: @nfrisby raised the possibility of moving this out of the generic ConsensusProtocol
type class (which would stay as before this PR) and instead into SingleEraProtocol
. This seems clearly desirable (less unneeded flexibility in one of our core abstractions), but only if it does not come with too much of its own complexity:
- For
HorizonView
, this seems to work fine, one could movetickChainDepState_
andprojectHorizonView
intoSingleEraProtocol
, and require the lawtickChainDepState == tickChainDepState_ . projectHorizonView
. - The extra
Ticked LedgerView
argument in the non-tick functions is what is actually causing most of the code changes, but it is a bit unclear a priori how to handle that nicely.
Historical note: The non-tick functions in ConsensusProtocol
were actually present in the past (16d61bf#diff-f738455b7b0823897ece16f7b9667badfd57532722a250f27f0f507161d2cf5c), but were removed again due to the additional error
calls in the HFC (that are readded in this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second bullet point is enabled by #354.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first bullet point will be handled by a82b126.
This adds an extremely simple mechanism that increments protocol versions on era boundaries. The motivation is to showcase that due to how HFC ticking is implemented, this does not do what one would expect. See #340 for a concrete scenario where this behavior is problematic.
c589290
to
8608c57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have gone through the commits and I think everything is very clear and properly documented. I have no judgement whether more tests would be needed or not.
@@ -138,7 +139,8 @@ prop_simple_hfc_convergence testSetup@TestSetup{..} = | |||
counterexample ("eraSizeA: " <> show eraSizeA) $ | |||
tabulate "epochs in era A" [labelEraSizeA] $ | |||
prop_general args testOutput .&&. | |||
prop_allExpectedBlocks | |||
prop_allExpectedBlocks .&&. | |||
prop_finalProtVers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm: we don't add an expected failure here because even if this commit won't be squashed, when bisecting we don't care about compiling the tests? Or do we? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests are passing at all intermediate commits; note that prop_finalProtVers
initially just showcases the bug
https://github.com/input-output-hk/ouroboros-consensus/blob/e55c607ba31a9f74a7fa9deb986e47ec60783526/ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/HardFork/Combinator.hs#L361-L368
and is then in a later commit, once the bug is fixed, replaced by the "correct" assertion:
https://github.com/input-output-hk/ouroboros-consensus/blob/3fdf4a2efaad33e7fdee0b99e57c94c96a762e41/ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/HardFork/Combinator.hs#L362-L365
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine, though my initial expectation matches Damian's: that the test would fail until the bug is fixed.
To the broader point: I think it's fine for tests to fail between commits inside of a PR; but must pass before merge.
ouroboros-consensus-cardano/src/byron/Ouroboros/Consensus/Byron/Ledger/Ledger.hs
Show resolved
Hide resolved
@@ -280,6 +282,57 @@ forecastAcrossShelley cfgFrom cfgTo transition forecastFor ledgerStateFrom | |||
(SL.stabilityWindow (shelleyLedgerGlobals cfgFrom)) | |||
(SL.stabilityWindow (shelleyLedgerGlobals cfgTo)) | |||
|
|||
translateLedgerStateAcrossShelley :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it'd be worth adding a comment on what's going on in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In particular, contributors might be puzzled about the unticking part. Perhaps it's worth replicating here what was discussed in the issue that motivated this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a Haddock comment that at least highlights that the below works perfectly fine when eraTo ~ eraFrom
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a Haddock comment that at least highlights that the below works perfectly fine when
eraTo ~ eraFrom
.
It doesn't as that would contradict SL.PreviousEra eraTo ~ eraFrom
.
-- * then translate the ledger state into the new era (using | ||
-- 'translateLedgerState'). | ||
-- | ||
-- The final tick to the given slot does /not/ happen here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel a bit uneasy about the fact that this function has to mention what happens elsewhere. I guess there's no way to avoid it given the current design, but it seems a very delicate piece of logic is scattered around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, in this particular case, it actually happens in the same file above in applyChainTickLedgerResult
, will add a reference.
EDIT: see 161bbb9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have sworn that I commented in this thread in my Review just now...
My recommendation was ultimately to rename the function extendToEraOfSlot
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR: You are probably referring to #340 (comment)
161bbb9
to
cbd72f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've finished a pass. All the commits are very nicely organized. Thank you so much for that.
I sadly still don't feel like I have clarity on what order to merge this with the others, particularly #354
-- https://iohk.io/en/blog/posts/2021/03/29/the-secure-transition-to-decentralization/ | ||
-- for additional context. | ||
-- | ||
-- We are not aware of any other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
There's an extra line break.
-
We should recommend in this comment that if some such deployment does in fact exist they could workaround this override in the
translateChainDepState
function, since that is applied after this "incorrect" tick logic.
@@ -138,7 +139,8 @@ prop_simple_hfc_convergence testSetup@TestSetup{..} = | |||
counterexample ("eraSizeA: " <> show eraSizeA) $ | |||
tabulate "epochs in era A" [labelEraSizeA] $ | |||
prop_general args testOutput .&&. | |||
prop_allExpectedBlocks | |||
prop_allExpectedBlocks .&&. | |||
prop_finalProtVers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine, though my initial expectation matches Damian's: that the test would fail until the bug is fixed.
To the broader point: I think it's fine for tests to fail between commits inside of a PR; but must pass before merge.
|
||
-- | The protocol version. Increments at epoch boundaries whenever | ||
-- 'InitiateAtoB' would signal an era transition. | ||
, lgrA_protVer :: Word16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend using Integer
instead of a finite type. No need to introduce the scent of overflows.
-- should be @'succ' 'initProtVer'@), this is currently not the case. | ||
-- Subsequent commits will fix this. | ||
all (== initProtVer) finalProtVers | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional extra newline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, will remove 👍
|
||
k = lcfgA_k cfg | ||
|
||
firstSlotNextEpoch = runIdentity $ do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My gut tells me there's an existing function for this somewhere...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only know of the functions in Ouroboros.Consensus.Ledger.Protocol.Util
(in particular isNewEpoch
), but we can't use them here without shuffling things around as ouroboros-consensus-diffusion does not depend on ouroboros-consensus-protocol. I will keep it like this for now, but we could also start some EpochInfo
-related utilities in ouroboros-consensus
such that they can be used in both oc-diffusion
and oc-protocol
.
-- The final tick to the given slot does /not/ happen here, but rather in | ||
-- 'applyChainTickLedgerResult' above. | ||
-- | ||
-- It can be the case that the telescope is extended multiple times, for example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to elaborate on "empty eras". Two flavors:
- There are no epochs in the era: eg our benchmarking hacking.
- There are epochs in the era but no blocks in any of those epochs: never happens on a healthy Cardano ledger, but some other ledger might allow that 🤷.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, for the example here, I will make it specifically about the zero-epoch eras for benchmarking as that is the most relevant case today.
@@ -247,6 +248,14 @@ newtype instance Ticked (SL.LedgerView c) = TickedPraosLedgerView { | |||
getTickedPraosLedgerView :: SL.LedgerView c | |||
} | |||
|
|||
newtype TPraosHorizonView = TPraosHorizonView { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... want to make this empty?
|
||
-- We can't use 'SL.tickChainDepState' as it unnecessarily takes the | ||
-- entire 'SL.LedgerView' as an argument. Hence, we inline it here; future | ||
-- work should include moving the TPraos logic entirely to Consensus. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-- work should include moving the TPraos logic entirely to Consensus. | |
-- work should include moving the TPraos logic entirely to the ouroboros-consensus repository. |
import qualified Ouroboros.Consensus.Protocol.TPraos as TPraos | ||
import Ouroboros.Consensus.Protocol.Translate (TranslateProto (..)) | ||
|
||
{------------------------------------------------------------------------------- | ||
Translation from transitional Praos | ||
-------------------------------------------------------------------------------} | ||
|
||
instance (c1 ~ c2) => TranslateProto (TPraos c1) (TPraos c2) where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the error message would actually be easier to understand if the head was TranslateProto (TPraos c) (TPraos c)
?
And same for Praos
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have to play around to find this out, I think the way how it is written here can sometimes be beneficial because it causes instances selection to pick this instance even if it does not (yet) know c1 ~ c2
(a similar example is discussed here).
@@ -253,7 +255,7 @@ newtype TPraosHorizonView = TPraosHorizonView { | |||
} | |||
|
|||
newtype instance Ticked TPraosHorizonView = TickedTPraosHorizonView { | |||
tickedTpraosHorizonViewExtraEntropy :: SL.Nonce | |||
tickedTPraosHorizonViewExtraEntropy :: SL.Nonce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: preferably this diff is squashed into the commit that introduces TickedTPraosHorizonView
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a82b126 is important.
We should squash it down.
This adds an extremely simple mechanism that increments protocol versions on era boundaries. The motivation is to showcase that due to how HFC ticking is implemented, this does not do what one would expect. See #340 for a concrete scenario where this behavior is problematic.
Trivial generalization that will be used in subsequent commits
In a subsequent commit, we will use this extra information when invoking `State.align`.
Initial motivation for this is the need (in a later commit) to implement the translation of the last *ticked* Byron ledger state to the first Shelley state, which becomes possible as `ByronTransition` is part of the ticked Byron ledger state. Apart from the translation to Shelley, this tip block number is only used to check whether an update proposal became stable, so it makes sense to make it part of `ByronTransition` in any case.
This doesn't actually change any code, and is mostly done to highlight the changes to this function in the next commit.
See #339 for explanation/motivation.
In a single-era context, this change makes no difference for the slot field in the unticked `ChainDepState (T)Praos`, as the slot we last ticked to will always be the slot of the last applied block. However, in a later commit, we will change how the HFC does cross-era ticking, namely by first ticking just across the epoch boundary using the old era's logic, then translating, and then ticking to the target slot using the new era's logic. - Without this change, as the slot in the unticked `ChainDepState` is not update by ticking, we would conclude twice that we are ticking across an epoch, which is incorrect. - With this change, only the first tick considers itself to cross an epoch boundary. Hence, after the translation step, the slot in the unticked `ChainDepState` will *not* contain the slot of the last applied block, but rather the slot just after the era boundary (as we ticked to that slot in the first tick). Note that this is purely an intermediary state in the HFC.
Context: This commit enables changing the HFC protocol logic for ticking. This change is based on the observation that all single-era `ConsensusProtocol`s except TPraos do not actually need the `LedgerView` when ticking at all; they just store it in the `Ticked ChainDepState` such that it can be used by other functions from `ConsensusProtocol`, such as `updateChainDepState` and `checkIsLeader`. Hence, we can introduce a new type family `HorizonView` (that will be set to `()` for all single-era protocols except TPraos), and decompose `tickChainDepState` into `tickChainDepState_` (which now only takes a horizon view instead of a ledger view) and `projectHorizonView` (which allows one to always get a horizon view from a ledger view). TPraos and the HardForkProtocol are just minimally modified to make them compile; they will be adapted in subsequent commits. Co-authored-by: Nicolas Frisby <[email protected]>
This commit minimizes the `HorizonView` for TPraos from the full `LedgerView` to just the extra entropy nonce (a ledger protocol parameter), which is the only part of the `LedgerView` needed for ticking. Co-authored-by: Nicolas Frisby <[email protected]>
Sole motivation is to make HFC protocol ticking analogous to HFC ledger ticking which was already changed in an earlier commit. Co-authored-by: Nicolas Frisby <[email protected]>
a82b126
to
d77452d
Compare
Closing for now, see #339 (comment) |
Closes #339, closes IntersectMBO/cardano-ledger#3491
This PR is intended to be reviewed commit by commit, which should all compile and pass all tests individually.
On a high level, this PR changes the way we do cross-era ticking in the HFC from translate-then-tick to tick-then-translate-then-tick for both the ledger state and, for consistency, also for the chain-dep/protocol state. For motivation, see #339, for further directions in this area, see #345.
The first commit enriches an existing test to showcase the problem, it is then fixed in a later commit.
TODO:
ChainDepState
cross-era ticking. We could do this using actual Cardano blocks.