*EraBlockHeader typeclasses deprecating BHeaderView#5560
*EraBlockHeader typeclasses deprecating BHeaderView#5560
Conversation
523aa23 to
6974084
Compare
fcee41c to
3863781
Compare
3863781 to
ab6354e
Compare
teodanciu
left a comment
There was a problem hiding this comment.
Looks good to me, some questions and suggestions.
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/src/Cardano/Ledger/Shelley/BlockBody/Internal.hs
Outdated
Show resolved
Hide resolved
|
|
||
| initialRules = [] | ||
| transitionRules = [conwayBbodyTransition @era >> alonzoBbodyTransition @era] | ||
| transitionRules = [alonzoBbodyTransition @era >> conwayBbodyTransition @era] -- QUESTION: would sorting this cause breakage? |
There was a problem hiding this comment.
How come you changed the order of these? I think it shouldn't matter, but is there a particular reason to do it?
There was a problem hiding this comment.
I think it shouldn't matter
I think so too 👍
but is there a particular reason to do it?
Just intuitively, it reads more "appropriate" that checks and validations from earlier rules would be processed first, although the >> does not signify a value dependency, it does order the execution. But it could mean that predicate failures ordering is also changed.
It is not clear whether the potential change in predicate failure ordering will affect any downstream, such as the CLI. @neilmayhew do you have any idea about this?
| TRC (BbodyEnv pp account, BbodyState ls b, DijkstraBbodySignal blk@Block {blockBody}) <- | ||
| judgmentContext | ||
|
|
||
| Shelley.validateBodySize @era |
There was a problem hiding this comment.
Same question about the ordering of these checks - any particularly reason you reversed them?
There was a problem hiding this comment.
Same as for conway.
A block is made up of a header and a body. Ledger needs to process blocks but is nescient of the header type. One such header type is defined in the tpraos package and another is defined in consensus. Both these packages depend on core and the eras and so ledger cannot import from them. BHeaderView was an inferior[1] arrangement to convert a Block h era into a type with known fields. EraBlockHeader now defines a set of lenses for this, instead. Consensus[2] calls ledger's applyBlock and the BBODY rules, which need access to the values from the header, now with the help of lenses. So we wrap the unknown header type h in a GADT to pass to BBODY rules, called BbodySignal. For tests, we replace BHeaderView with TestBlockHeader and makeHeaderView with mkTestBlockHeaderNoNonce. Ref: #5541 [1]: #5467 (comment) [2]: https://github.com/IntersectMBO/ouroboros-consensus/blob/1940605fbbfb50032ab78e2268b85065c703cd2a/ouroboros-consensus-cardano/src/shelley/Ouroboros/Consensus/Shelley/Protocol/Praos.hs#L196-L202
Refactor shelley BBODY transition to export reusable validations to future eras and update the BBODY signal. Move isOverlaySlot calculation into incrBlocks.
Refactor alonzo BBODY transition to export reusable validations to future eras and update the BBODY signal.
Refactor conway BBODY transition to export reusable validations to future eras and update the BBODY signal.
Introduce DijkstraEraBlockHeader to have a lens to the previous nonce, and the DijkstraBbodySignal GADT to constrain the header existantially, same as for BbodySignal. Move PerasCert, PerasKey and validatePerasCert to dijkstra's BlockBody. Remove PrevEpochNonceNotPresent because the field is implicitly present in the signal to the BBODY rule now. Refactor the dijkstra BBODY rule to reuse validations from shelley, alonzo, conway, inlined, because the change in the type of Signal makes calling earlier BBODY transitions impossible.
ab6354e to
5b35f0a
Compare
| blockHeaderIssuerL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> hashKey $ bheaderVk bhb) | ||
| (\(Block (BHeader bhb sig) body) _ -> Block (BHeader bhb sig) body) -- QUESTION: What does it mean to "unhash" and set this? |
| blockHeaderHSizeL = | ||
| lens | ||
| (\(Block bh _) -> originalBytesSize bh) | ||
| (\(Block bh body) _ -> Block bh body) -- QUESTION: What does it mean to do this here? |
| instance Crypto c => EraBlockHeader (BHeader c) BabbageEra where | ||
| blockHeaderIssuerL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> hashKey $ bheaderVk bhb) | ||
| (\(Block (BHeader bhb sig) body) _ -> Block (BHeader bhb sig) body) | ||
| blockHeaderBSizeL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bsize bhb) | ||
| (\(Block (BHeader bhb sig) body) newSize -> Block (BHeader (bhb {bsize = newSize}) sig) body) | ||
| blockHeaderHSizeL = | ||
| lens | ||
| (\(Block bh _) -> originalBytesSize bh) | ||
| (\(Block bh body) _ -> Block bh body) | ||
| blockHeaderBHashL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bhash bhb) | ||
| (\(Block (BHeader bhb sig) body) newHash -> Block (BHeader (bhb {bhash = newHash}) sig) body) | ||
| blockHeaderSlotL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bheaderSlotNo bhb) | ||
| (\(Block (BHeader bhb sig) body) newSlot -> Block (BHeader (bhb {bheaderSlotNo = newSlot}) sig) body) | ||
| blockHeaderProtVerL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bprotver bhb) | ||
| (\(Block (BHeader bhb sig) body) protVer -> Block (BHeader (bhb {bprotver = protVer}) sig) body) | ||
|
|
||
| instance Crypto c => EraBlockHeader (BHeader c) ConwayEra where | ||
| blockHeaderIssuerL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> hashKey $ bheaderVk bhb) | ||
| (\(Block (BHeader bhb sig) body) _ -> Block (BHeader bhb sig) body) | ||
| blockHeaderBSizeL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bsize bhb) | ||
| (\(Block (BHeader bhb sig) body) newSize -> Block (BHeader (bhb {bsize = newSize}) sig) body) | ||
| blockHeaderHSizeL = | ||
| lens | ||
| (\(Block bh _) -> originalBytesSize bh) | ||
| (\(Block bh body) _ -> Block bh body) | ||
| blockHeaderBHashL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bhash bhb) | ||
| (\(Block (BHeader bhb sig) body) newHash -> Block (BHeader (bhb {bhash = newHash}) sig) body) | ||
| blockHeaderSlotL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bheaderSlotNo bhb) | ||
| (\(Block (BHeader bhb sig) body) newSlot -> Block (BHeader (bhb {bheaderSlotNo = newSlot}) sig) body) | ||
| blockHeaderProtVerL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bprotver bhb) | ||
| (\(Block (BHeader bhb sig) body) protVer -> Block (BHeader (bhb {bprotver = protVer}) sig) body) | ||
|
|
||
| instance Crypto c => EraBlockHeader (BHeader c) DijkstraEra where | ||
| blockHeaderIssuerL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> hashKey $ bheaderVk bhb) | ||
| (\(Block (BHeader bhb sig) body) _ -> Block (BHeader bhb sig) body) | ||
| blockHeaderBSizeL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bsize bhb) | ||
| (\(Block (BHeader bhb sig) body) newSize -> Block (BHeader (bhb {bsize = newSize}) sig) body) | ||
| blockHeaderHSizeL = | ||
| lens | ||
| (\(Block bh _) -> originalBytesSize bh) | ||
| (\(Block bh body) _ -> Block bh body) | ||
| blockHeaderBHashL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bhash bhb) | ||
| (\(Block (BHeader bhb sig) body) newHash -> Block (BHeader (bhb {bhash = newHash}) sig) body) | ||
| blockHeaderSlotL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bheaderSlotNo bhb) | ||
| (\(Block (BHeader bhb sig) body) newSlot -> Block (BHeader (bhb {bheaderSlotNo = newSlot}) sig) body) | ||
| blockHeaderProtVerL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bprotver bhb) | ||
| (\(Block (BHeader bhb sig) body) protVer -> Block (BHeader (bhb {bprotver = protVer}) sig) body) |
There was a problem hiding this comment.
Should these instances exist for for Babbage, Conway and Dijkstra? I would think not! Because BHeader is only used until Alonzo, no?
| blockHeaderProtVerL = | ||
| lens | ||
| (\(Block (BHeader bhb _) _) -> bprotver bhb) | ||
| (\(Block (BHeader bhb sig) body) protVer -> Block (BHeader (bhb {bprotver = protVer}) sig) body) |
There was a problem hiding this comment.
Actually I don't even know why these instances are necessary for each era?
Wouldn't this suffice?
instance (Era era, Crypto c) => EraBlockHeader (BHeader c) era where
Description
QUESTIONs are addressed in code-reviewCloses #5541
Checklist
CHANGELOG.mdfiles updated for packages with externally visible changes.NOTE: New section is never added with the code changes. (See RELEASING.md).
.cabalandCHANGELOG.mdfiles when necessary, according to theversioning process.
.cabalfiles updated when necessary.NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
scripts/fourmolize.sh).scripts/cabal-format.sh).scripts/gen-cddl.sh)hie.yamlupdated (usescripts/gen-hie.sh).