Repository navigation
Conversation
8270249 to
84df011
Compare
705bf5a to
575d05d
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Public API compatibility and the new on-disk Leios state schema must be corrected before approval.
Review effort: Balanced
Findings: 5
Open (6)
Add default implementation for ShelleyBasedEra method · New Document or defer the additional certificate body validation · New Add default implementation for ProtocolHeaderSupportsEnvelope method · New Update disk CDDL for the new Leios state encoding · New Renaming HeaderView constructor breaks downstream compatibility · New Correct the changelog symbol name · New
What changed in this PR
Introduces Leios as the Dijkstra-era consensus protocol, establishing header validation and future EB-forging foundations for linked Leios work.
Changes:
- Adds Leios protocol state, validation, serialization, and forging plumbing.
- Switches Dijkstra from Praos to Leios headers.
- Validates Leios certificate claims against block bodies and adds tests.
| File | Description |
|---|---|
ouroboros-consensus.cabal |
Exposes Leios modules and test dependencies. |
.../Serialisation/Generators.hs |
Adds Leios state generators. |
.../Protocol/Praos/Views.hs |
Generalizes Praos header views. |
.../Protocol/Praos.hs |
Shares Praos validation with Leios. |
.../Protocol/Leios.hs |
Implements the Leios consensus protocol. |
.../Shelley/Integrity.hs |
Tests certificate-claim integrity. |
shelley-test/Main.hs |
Registers integrity tests. |
.../Cardano/Translation.hs |
Updates Dijkstra translation types. |
.../Cardano/Capacity.hs |
Updates Dijkstra capacity tests. |
.../Shelley/Examples.hs |
Produces Leios Dijkstra examples. |
.../Cardano/MockCrypto.hs |
Adds mock Leios crypto support. |
.../Shelley/ShelleyHFC.hs |
Adds Leios HFC configuration. |
.../Shelley/Protocol/TPraos.hs |
Supplies the non-Leios header claim. |
.../Shelley/Protocol/Praos.hs |
Refactors shared Praos behavior. |
.../Shelley/Protocol/Leios.hs |
Integrates Leios headers with Shelley. |
.../Protocol/EnvelopeChecks.hs |
Extracts shared Praos envelope checks. |
.../Shelley/Protocol/Abstract.hs |
Adds the header certificate-claim API. |
.../Shelley/Node/Serialisation.hs |
Adds Leios state disk serialization. |
.../Shelley/Node/Leios.hs |
Adds Leios block-forging plumbing. |
.../Ledger/SupportsProtocol.hs |
Adds Leios ledger-view support. |
.../Shelley/Ledger/Mempool.hs |
Retypes Dijkstra measures for Leios. |
.../Shelley/Ledger/Block.hs |
Validates certificate/body consistency. |
.../Shelley/HFEras.hs |
Switches standard Dijkstra to Leios. |
.../Shelley/Eras.hs |
Detects Dijkstra body certificates. |
.../Cardano/Node.hs |
Wires Leios into the Cardano node. |
.../Cardano/Ledger.hs |
Retypes Dijkstra ledger outputs. |
.../Cardano/CanHardFork.hs |
Updates Dijkstra hard-fork translations. |
.../Cardano/Block.hs |
Retypes Cardano’s Dijkstra era. |
.../Result_Dijkstra_LedgerTip |
Updates the Dijkstra golden hash. |
changelog.d/...leios_cert_bit_matches_body.md |
Documents certificate-claim validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fromCBOR = decode | ||
|
|
||
| -- | Versioned independently of the 'PraosState' encoding it nests. | ||
| instance Serialise LeiosState where |
There was a problem hiding this comment.
@jasagredo offered to this on top of this PR 🫶
| , Leios.hbBodyHash = bbHash | ||
| , Leios.hbOCert = praosToSignOCert | ||
| , Leios.hbVersionInfo = versionInfo | ||
| , Leios.hbBlockBodyContainsLeiosCert = False -- FIXME: Fill this in when forging |
There was a problem hiding this comment.
To address these FIXMEs, we'll need to add an argument to mkHeader so that the correct values can be in scope here. And they must be here (it can't be done after the fact on whatever mkHeader retruns) because mkHeader returns a signature.
What will the type of those arguments to mkHeader be? My intuition is that they'll have to branch on the proto type: if it's Leios then you have to provide these arguments and if it's Praos then you must not provide these arguments.
But I guess you can just always provide them and Praos will ignore them? And the Praos caller is free to pass in False and SNothing since the Praos method impl will ignore the values anyway? 😬
I would prefer to use more precise types for that new proto-dependent argument. (Which is one step towards what's on my PR 2282 branch, but maybe it's just a small one?)
There was a problem hiding this comment.
Claude and I worked through that "small step" on my PR 2282 yesterday. Hope to discuss in today's Consensus Office Hours.
| mkHeader hk cbl il slotNo blockNo prevHash bbHash sz protVer = do | ||
| PraosFields{praosSignature, praosToSign} <- forgePraosFields hk cbl il mkLeiosHeaderBody | ||
| -- TODO: update mkHeader to take a protVer | ||
| pure $ mkMemoized (pvMajor protVer) $ Leios.HeaderRaw praosToSign praosSignature |
There was a problem hiding this comment.
Can this be LeiosCodec.Header praosToSign praosSignature instead? (That's what it is on my branch, but I don't remember the details.)
There was a problem hiding this comment.
This is incorrect! protVer should not be used!!!! That is a self reported value which has nothing to do with the actual protocol version.
You need to use mkHeader instead: https://github.com/IntersectMBO/cardano-ledger/blob/0a72d43f5996c23eae1f6164d7d6a4dfeabe4c7f/libs/cardano-protocol/src/Cardano/Protocol/Leios/BlockHeader.hs#L214-L220
Which this mkHeader will need to accept a new argument Proxy era, which should work, since forgeShelleyBlock does have the era parameter available, eg.
mkHeader ::
(Crypto crypto, Monad m, crypto ~ ProtoCrypto proto, Era era) =>
proxy era ->
HotKey crypto m ->
CanBeLeader proto ->
IsLeader proto ->
-- | Slot no
SlotNo ->
-- | Block no
BlockNo ->
-- | Hash of the previous block
PrevHash ->
-- | Hash of the block body to include in the header
Hash.Hash HASH EraIndependentBlockBody ->
-- | Size of the block body
Int ->
-- | Protocol version
ProtVer ->
m (ShelleyProtocolHeader proto)There was a problem hiding this comment.
Sure, gladly do. That code did not exist when I first wrote this code :)
There was a problem hiding this comment.
The alternative to this PR got merged into main. It uses Cardano.Protocol.Leios.BlockHeader.mkHeader era. I think that's what Alexey suggested here.
|
@ch1bo a heads-up: the endorser-block forging work (input-output-hk/ouroboros-leios#1107) will change the type of The forge interface does not follow the "How" in #1107. |
|
|
||
| mkHeader hk cbl il slotNo blockNo prevHash bbHash sz protVer = do | ||
| PraosFields{praosSignature, praosToSign} <- forgePraosFields hk cbl il mkLeiosHeaderBody | ||
| -- TODO: update mkHeader to take a protVer |
There was a problem hiding this comment.
Incorrect TODO
| -- TODO: update mkHeader to take a protVer |
Thanks for the heads-up. I do object, but happy to have the conversation there: #2377 (review) |
| -- varies by one. | ||
| instance ToCBOR AnnouncedBy where | ||
| toCBOR (AnnouncedBy issuer ann) = | ||
| CBOR.encodeListLen 2 <> toCBOR issuer <> toEraCBOR @ShelleyEra ann |
There was a problem hiding this comment.
Weird that this exists only in Dijkstra but we use toEraCBOR @ShelleyEra, no?
There was a problem hiding this comment.
Any would be fine I guess. The PraosState is also using ShelleyEra so I think its fine?
| -- | An endorser-block announcement, and who announced it. | ||
| -- | ||
| -- The issuer and the announcing header's slot ('praosStateLastSlot') together | ||
| -- give the election, which is what lets a certificate name what it certifies. | ||
| data AnnouncedBy = AnnouncedBy | ||
| { announcedByIssuer :: !(KeyHash SL.BlockIssuer) | ||
| , announcedEbReferences :: !EbReferencesAnnouncement | ||
| } |
There was a problem hiding this comment.
Wouldn't it make sense to add the slot here? Why do we need to keep them separate?
There was a problem hiding this comment.
I.e. to get a complete picture one needs a PraosState and an AnnouncedBy, but we could keep the relevant information entirely in AnnouncedBy.
There was a problem hiding this comment.
I followed / anticipated what @nfrisby did in the prototype (omnibus PR and soon merged)
| -- The body hash does not cover this claim, so it is checked separately. | ||
| && bodyContainsCert == pHeaderContainsLeiosCert shelleyHdr | ||
| -- A CertRB carries a certificate instead of transactions, never both. | ||
| && not (bodyContainsCert && bodyContainsTxs) |
There was a problem hiding this comment.
Aren't these body checks? Perhaps they should move to the ledger BBODY rule?
There was a problem hiding this comment.
Yeah they will definitely be in the ledger. Do we want to do as little as possible or as much as possible here?
There was a problem hiding this comment.
In the ledger formal spec, there's a PR that puts both checks in the BBODY rule. fls PR #1339 adds leiosBodyChecks, by cases on the body's certificate: without one the header's certified bit must be false; with one the bit must be true and the transaction list empty (BlockBody.lagda.md, lines 109 to 111 and 167).
The bit is required in both directions: header validation sees the bit but not the body, so a header-level check gated on the bit, such as the delay check in the consensus spec's certChecks (2278), is sound only if something that sees the body verifies the bit. The no-transactions rule is the CIP's own (Step 5: "When a certificate is included, no further transactions are allowed in the RB").
So the ledger rule is authoritative for both. Keeping them in blockMatchesHeader as well costs nothing beyond recording the division; it would provide an early reject of a block the ledger would reject anyway. Whether cardano-ledger's BBODY will carry the same two checks is a question we should ask the ledger implementation team (cc: @lehins ).
There was a problem hiding this comment.
Let's reorder these checks to make sure the hashBlockBody blockBody expression is evaluated only when needed (all previous tests failed). Don't recall if that already has some hash memoized or not?
Also, I don't recall exactly, but why are we not checking the size here too? Perhaps let's just add it also.
The thing is, we do need these checks in the Ledger, but we do a non trivial amount of work before calling Ledger, and if we can establish that a block is corrupt sooner we can avoid doing any unnecessary work for it.
There was a problem hiding this comment.
Alternatively, we can just issue a call to Ledger here that performs only these sorts of checks for us.
There was a problem hiding this comment.
blockMatchesHeader are cheap checks we can do immediately upon receiving the block from a peer. If those checks fail, then we can instantly disconnect from that peer. In particular, we can disconnect even if ChainSel can't/chooses to not validate the newly arrived block right now. And when it doesn't, it forgets who provide the block, so we lose our chance to punish the peer.
So: it's a cheap way to catch a buggy/evil peer, even for blocks that aren't (immediately) worth invoking the full ledger rules on.
If we want to factor these checks out so that they exist in a function that both the blockMatchesHeader and the ledger rules proper share, that's fine; that'd be upstreaming it into Ledger.
| pHeaderIssuer = hbVk . headerBody | ||
| pHeaderIssueNo = SL.ocertN . hbOCert . headerBody | ||
| pTieBreakVRFValue = certifiedOutput . hbVrfRes . headerBody |
There was a problem hiding this comment.
Why not use the view:
| pHeaderIssuer = hbVk . headerBody | |
| pHeaderIssueNo = SL.ocertN . hbOCert . headerBody | |
| pTieBreakVRFValue = certifiedOutput . hbVrfRes . headerBody | |
| pHeaderIssuer = hvVK . leiosHeaderToView | |
| pHeaderIssueNo = SL.ocertN . hvOCert . leiosHeaderToView | |
| pTieBreakVRFValue = certifiedOutput . hvVrfRes . leiosHeaderToView |
There was a problem hiding this comment.
If we do the same with Praos that would be also good. If we don't do this change, the commit 9008611 probably doesn't make sense as it stands because it would only change configSlotsPerKESPeriod which is not what the message says.
There was a problem hiding this comment.
What difference does it make? Should I do it?
An overlay on Praos signs a header body of its own. With the Praos body hard-coded into HeaderView, the shared signature checks would have to summon a Signable dictionary per extension; as a type parameter they simply ask for one. reupdatePraosState comes out of the instance for a related reason: an overlay cannot delegate to a method that takes the ValidateView, but nothing in that body reads the header's body, so it can share the function. Praos is otherwise untouched.
Leios runs on top of Praos rather than replacing it: the ranking blocks are Praos blocks, same leader schedule, same KES keys, same nonce-carrying chain-dep state. So nearly everything here is Praos's, called directly, and only the two methods that take a ValidateView are written out --- a Leios header signs the Leios body. LeiosCrypto states what that body needs on top of PraosCrypto, rather than widening PraosCrypto, so the base protocol stays unaware the overlay exists. Enough to decode a Leios header and check its signature and envelope, which is what a node following the chain needs.
A certificate names the endorser block its predecessor announced, so the chain-dep state has to remember that announcement. LeiosState is PraosState plus it, overwritten by every header, so one that announces nothing clears it. Using the ledger's EbReferencesAnnouncement rather than a consensus copy: the only difference would be a raw-bytes hash in place of its SafeHash, which buys nothing on this path and makes the inverse conversion partial. Also wires Leios into the Shelley block layer --- the partial config, the ledger view, ShelleyCompatible for Dijkstra and the chain-dep-state codec --- so a Leios ShelleyBlock can exist. The two LedgerSupportsProtocol instances differ only in their head, so the methods are shared functions.
Dijkstra is the era that carries endorser blocks, so it is the era that should use the Leios header. It was paired with Praos only because consensus had no Leios protocol to give it --- see the adapter this removes, which taught the Praos header to answer the ledger's Leios questions with False and SNothing. Also adds the block forging for it, so a Leios block has a forge to produce it; that forge announces nothing yet. Dijkstra is not released, so the header and chain-dep-state formats are free to change: only the four Dijkstra goldens move, and no earlier era's does. One golden does not pass: the node-to-node CDDL still says `dijkstraPraosHeader = conway.header`. The blueprint already describes the real `dijkstra.header`, so that is a one-line change in cardano-blueprint, which is not a submodule of this checkout.
The body hash does not cover the claim, so a Dijkstra header could mark itself a certifying ranking block whose body carries no certificate. That bit is what out-of-order endorser-block fetching keys off, and it is read from the header alone, ahead of the body. It is also the only Leios claim checkable with the block in hand --- no committee, no ledger view, no endorser-block store --- so it belongs in `blockMatchesHeader`, which `verifyBlockIntegrity` and hence the forge's own assertion both reach.
575d05d to
2c44906
Compare
Each method that could re-implement a Praos projection now names the Praos code it reuses instead: the config method calls Praos's own class method, and the tie-break VRF value is one function both protocols apply to their header view. What is left written out is what genuinely differs. The header-keyed methods cannot go through Praos's class methods: those take a `Praos.Header`, and building one from a Leios header would need the KES signature coerced across body types and `hbProtVer` invented from `bhviHighestSupportedMajorVersion`. The signature covers the Leios body, so such a header would verify against the wrong bytes.
2c44906 to
bac0162
Compare
This is done upstream now in injectIntoTestState, which sets mark, set and go snapshots, along with selecting the leios committee.
be19f5c to
ad2e9b3
Compare
This finally makes us not use protVer for serializing the header bytes.
The fragment only described the certificate-claim check, misnamed pHeaderContainsLeiosCert, claimed both new methods default to False and filed everything as non-breaking. Rewrite it to cover the whole PR: Dijkstra moving to Leios, the HeaderView generalisation, the new modules and exports, and both new rules in blockMatchesHeader.
ad2e9b3 to
433cd75
Compare
afdd6ce to
57556fd
Compare
| serialisedShelleyHeader<babbage.header>, | ||
| serialisedShelleyHeader<conway.header>, | ||
| serialisedShelleyHeader<dijkstraPraosHeader>> | ||
| serialisedShelleyHeader<dijkstraLeiosHeader>> |
There was a problem hiding this comment.
let's do dijkstra.header here?
There was a problem hiding this comment.
That's what Javier did on the PR that just got merged into main, FYI.
| CardanoTriggerHardForkAtEpoch epochNo -> | ||
| TriggerHardForkAtEpoch epochNo | ||
|
|
||
| -- | Warm the initial stake snapshots for early-bootstrap (test) networks, so the |
There was a problem hiding this comment.
Javier told me it got upstreamed into ledger, so this is redundant now.
| -- The body hash does not cover this claim, so it is checked separately. | ||
| && bodyContainsCert == pHeaderContainsLeiosCert shelleyHdr | ||
| -- A CertRB carries a certificate instead of transactions, never both. | ||
| && not (bodyContainsCert && bodyContainsTxs) |
There was a problem hiding this comment.
Let's reorder these checks to make sure the hashBlockBody blockBody expression is evaluated only when needed (all previous tests failed). Don't recall if that already has some hash memoized or not?
Also, I don't recall exactly, but why are we not checking the size here too? Perhaps let's just add it also.
The thing is, we do need these checks in the Ledger, but we do a non trivial amount of work before calling Ledger, and if we can establish that a block is corrupt sooner we can avoid doing any unnecessary work for it.
| -- The body hash does not cover this claim, so it is checked separately. | ||
| && bodyContainsCert == pHeaderContainsLeiosCert shelleyHdr | ||
| -- A CertRB carries a certificate instead of transactions, never both. | ||
| && not (bodyContainsCert && bodyContainsTxs) |
There was a problem hiding this comment.
Alternatively, we can just issue a call to Ledger here that performs only these sorts of checks for us.
This is an alternative to PR #2354 The key differences are that we don't define the new protocol by composing various Praos pieces with their various Leios addenda. Instead, we have one holistic definition of "PolyPraos" and then the Praos and Praos2 (ie Praos plus the Leios extension) variants are defined in terms of that one. Notably, and by design, Praos2 is essentially PolyPraos: PolyPraos will always be the _richest_/"latest and greatest"/fully-extended Praos variant running on Cardano. But it's defined in such a way that the previous Praos variants (starting _after_ TPraos) can also be defined in terms of it. The first commit is best read using `diff -w` (or the equivalent setting the PR's settings cog): it most directly shows that the old Praos methods have been floated out to become PolyPraos definitions. The subsequent commits then merely reorg around that idea.


First pull request in course of input-output-hk/ouroboros-leios#1090, that should also help build a foundation for other work items (e.g. input-output-hk/ouroboros-leios#1107, CC @dnadales)
I opted for the simplest possible way to type this. Most code for
Leiosinstances is directly re-used fromPraos(it's an overlay after all), sometimes extended and only in 1-2 places, even deliberately, duplicated.This contains quite some plumbing, but the only semantical change is that PV12 headers are decoded into
Leios.BlockHeaderand the certified bit is checked inblockMatchesHeader. More to follow