Add classes each era must satisfy for consensus integration #1944
Conversation
There was a problem hiding this comment.
The goal of this PR is to have something very concrete that needs to be completed before consensus can proceed with CAD-1679, i.e., the instances in Allegra.hs and Mary.hs.
Ideally, these instances are provided ASAP, maybe not necessarily with complete working implementations (error?). The actual implementations can follow later, in the meantime, the integration in downstream repos can proceed and we can at least propagate the types.
| ApplyBlock era, | ||
| ApplyTx era | ||
| ) => | ||
| ShelleyBasedEra era |
There was a problem hiding this comment.
I'm open to suggestions for a better name and/or a better place for it.
There was a problem hiding this comment.
Is this top level of bundling especially useful? I feel like it'll be potentially confusing to have so many different variants on this...
There was a problem hiding this comment.
IMO, very. It is the constraint that consensus uses on era, as it implies everything we need. See for example: https://github.com/input-output-hk/ouroboros-network/blob/master/ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Block.hs.
Also, this gives us a single check to verify whether an era satisfies everything that is needed. (If you have to provide 3-4 instances, it easier to forget to provide one of them, e.g., ApplyTx.)
I could see that the name is confusing (compared to ShelleyBased), so I'm certainly open to a better name.
There was a problem hiding this comment.
OK. I'm also strugging to come up with a good name...
There was a problem hiding this comment.
ShelleyMinimum? I struggle as well...
| KESignable c (BHBody c), | ||
| VRFSignable c Seed | ||
| ) => | ||
| TPraosCrypto c |
There was a problem hiding this comment.
I'm open to suggestions for a better name and/or a better place for it.
There was a problem hiding this comment.
What does the T stand for?
There was a problem hiding this comment.
Transitional Praos (transition from PBFT to Praos). PraosCrypto would also be fine for me. Or ShelleyCrypto, or ShelleyBasedCrypto, ...
There was a problem hiding this comment.
ah! thanks :) I like PraosCrypto or ShelleyCrypto. I don't have strong opinions here.
There was a problem hiding this comment.
I think I prefer ShelleyBasedCrypto over ShelleyCrypto because the latter might give the impression that it is related to the Shelley era while it is in fact not specific to the Shelley era? 🤔
PraosCrypto is shorter and avoids the confusion?
Up to you.
|
I'll rebase this PR once #1931 has been merged. I'll do the renaming/moves that have been decided by then at the same time. UPDATE: done |
This class exists in consensus and bundles all *signable* constraints that the Shelley(-based) ledger needs. Since these constraints all originate from the ledger, it makes sense to just put them in the ledger. Using this class (which implies `Crypto crypto`) also simplifies the API slightly. Why not make this a constraint synonym? Because with a class, one can easily check whether a given crypto type satisfies all the required constraints by providing an instance declaration: ```haskell instance PraosCrypto C_Crypto ``` If one of the constraints required for `PraosCrypto` is not satisfied by `C_Crypto`, then there will be an error at the instance declaration. When using a constraint synonym, there is no instance to define. An error about a missing constraint will be only be visible where `PraosCrypto crypto` is instantiated to `C_Crypto`, e.g., somewhere in consensus (way too late) or in the testsuite.
This is *the* constraint that an era must satisfy in order to be used by consensus. As discussed in the previous commit, this is a class and not a constraint synonym. This means that instances of it can (and should!) be defined in the ledger, which requires all constraints to be satisfied at the site of the instance definitions. With a constraint synonym, the constraints are only checked when the synonym is instantiated and used, which would be *in the consensus layer*. This would mean that the ledger won't be alerted when the required constraints are accidentally removed. Instead, the consensus layer would detect it when updating its dependency on the ledger, which is *too late*. By having the instance in the ledger, it is up to the ledger to make sure all required constraints keep being satisfied.
Now that `AllegraEra` implements all the needed constraints, `ShelleyBased (AllegraEra c)` should be trivially true.
mrBliss
force-pushed
the
mrBliss/consensus-requirements
branch
from
13fdd59
to
cc9995f
Compare
|
I have rebased this PR on master and renamed @JaredCorduan @nc6 Note that the last commit is unrelated to this PR, but it is needed to compile warning-free. I believe this warning snuck in because CI is not properly configured: |
This pulls in IntersectMBO/cardano-ledger#1944 which moves `ShelleyBasedEra` and `TPraosCrypto` (renamed to `PraosCrypto`) to `cardano-ledger-specs`.
This pulls in IntersectMBO/cardano-ledger#1944 which moves `ShelleyBasedEra` and `TPraosCrypto` (renamed to `PraosCrypto`) to `cardano-ledger-specs`.
See the individual commits for more details.