-
Notifications
You must be signed in to change notification settings - Fork 39
[Peras 13] Introduce votes and certificate forging API #1801
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
base: peras/main-pr/new-defs-and-plumbing
Are you sure you want to change the base?
[Peras 13] Introduce votes and certificate forging API #1801
Conversation
bf3a52c to
4063507
Compare
4063507 to
06f4e61
Compare
tbagrel1
left a comment
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.
Looks perfect to me! 😃
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsPeras.hs
Outdated
Show resolved
Hide resolved
06f4e61 to
42bb662
Compare
c314f45 to
bb97f3f
Compare
42bb662 to
4f2f0cc
Compare
| pcCertBoostedBlock <- decode | ||
| pure $ PerasCert{pcCertRound, pcCertBoostedBlock} | ||
|
|
||
| instance Serialise (HeaderHash blk) => Serialise (PerasVote blk) 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.
Why chose Serialise instead of ToCBOR/FromCBOR?
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.
@tbagrel1 maybe you have a better idea?
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.
Ouroboros.Consensus.Network.NodeToNode uses SerialiseNodeToNode class to serialise stuff. For types whose serialisation doesn't depend on a particular NodeToNodeVersion, the instances often fallback to encode/decode from Serialise class. For Peras types we've just followed the same existing pattern :)
| -- Consistent with the 'Serialise' instance for 'PerasVote' defined in Ouroboros.Consensus.Block.SupportsPeras | ||
| encodeNodeToNode ccfg version PerasVote{..} = | ||
| encodeListLen 3 | ||
| <> encodeNodeToNode ccfg version pvVoteRound | ||
| <> encodeNodeToNode ccfg version pvVoteBlock | ||
| <> encodeNodeToNode ccfg version pvVoteVoterId | ||
| decodeNodeToNode ccfg version = do | ||
| decodeListLenOf 3 | ||
| pvVoteRound <- decodeNodeToNode ccfg version | ||
| pvVoteBlock <- decodeNodeToNode ccfg version | ||
| pvVoteVoterId <- decodeNodeToNode ccfg version | ||
| pure $ PerasVote pvVoteRound pvVoteBlock pvVoteVoterId | ||
|
|
||
| instance SerialiseNodeToNode blk PerasVoterId where | ||
| encodeNodeToNode _ccfg _version = KeyHash.toCBOR . unPerasVoterId | ||
| decodeNodeToNode _ccfg _version = PerasVoterId <$> KeyHash.fromCBOR |
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 guess the Serialise instance could change as it is used for storing on disk (confirm?), while this one would require a new NTN version etc etc. So if any I would put the comment in the Serialise instance saying that it matches the NTN one.
Also I think I would expose a function
-- Ouroboros.Consensus.Block.SupportsPeras
encodeVoteNodeToNode :: CodecConfig blk -> NodeToNodeVersion -> PerasVote blk -> Encoding
encodeVoteNodeToNode _ccfg _version = encodemaybe? I should check how we do this in other places but I think this is the pattern we usually use.
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.
Bystander comment: I'm not super familiar with how these two type classes interact with each other but, in any case, we should remember to also apply any transformation to the instance for PerasCert defined above.
bb97f3f to
43c1fdc
Compare
4f2f0cc to
fd401fc
Compare
43c1fdc to
e7eadb3
Compare
fd401fc to
0006522
Compare
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsPeras.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsPeras.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsPeras.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Peras/Params.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsPeras.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsPeras.hs
Show resolved
Hide resolved
e7eadb3 to
ecff70d
Compare
0006522 to
d94393a
Compare
d94393a to
dff3d9b
Compare
This commit introduces a couple of new types to represent Peras votes and their corresponding certificate forging API. Notably, this requires an initial representation of notions like vote targets, vote stakes and stake distributions over multiple stake pools. Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io> Co-authored-by: Georgy Lukyanov <georgy.lukyanov@iohk.io> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io>
Co-authored-by: Agustin Mista <agustin.mista@moduscreate.com> Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io> Co-authored-by: Georgy Lukyanov <georgy.lukyanov@iohk.io> Co-authored-by: Thomas BAGREL <thomas.bagrel@tweag.io> Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io>
dff3d9b to
4b0c1af
Compare
|
This PR is now approved — merge pending integration of changes in upstream |
bladyjoker
left a comment
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.
Heyoo, this all looks nice and aligned with how things are done elsewhere!
I do have a couple of questions that probably require some historical forensics.
Why do we do things like this?
class
( Show (PerasCfg blk)
, NoThunks (PerasCert blk)
) =>
BlockSupportsPeras blk
where
data PerasCert blk
instance StandardHash blk => BlockSupportsPeras blk where
type PerasCfg blk = PerasParams
data PerasCert blk = PerasCert
{ pcCertRound :: PerasRoundNo
, pcCertBoostedBlock :: Point blk
}
class HasPerasCertRound cert where
getPerasCertRound :: cert -> PerasRoundNo
instance HasPerasCertRound (PerasCert blk) where
getPerasCertRound = pcCertRound
instance HasPerasCertRound (ValidatedPerasCert blk) where
getPerasCertRound = getPerasCertRound . vpcCert
instance
HasPerasCertRound cert =>
HasPerasCertRound (WithArrivalTime cert)
where
getPerasCertRound = getPerasCertRound . forgetArrivalTime
-- | Extract the boosted block point from a Peras certificate container
class HasPerasCertBoostedBlock cert blk | cert -> blk where
getPerasCertBoostedBlock :: cert -> Point blk
instance HasPerasCertBoostedBlock (PerasCert blk) blk where
getPerasCertBoostedBlock = pcCertBoostedBlock
instance HasPerasCertBoostedBlock (ValidatedPerasCert blk) blk where
getPerasCertBoostedBlock = getPerasCertBoostedBlock . vpcCert
instance
HasPerasCertBoostedBlock cert blk =>
HasPerasCertBoostedBlock (WithArrivalTime cert) blk
where
getPerasCertBoostedBlock = getPerasCertBoostedBlock . forgetArrivalTime
-- | Extract the certificate boost from a Peras certificate container
class HasPerasCertBoost cert where
getPerasCertBoost :: cert -> PerasWeight
instance HasPerasCertBoost (ValidatedPerasCert blk) where
getPerasCertBoost = vpcCertBoost
instance
HasPerasCertBoost cert =>
HasPerasCertBoost (WithArrivalTime cert)
where
getPerasCertBoost = getPerasCertBoost . forgetArrivalTime
Instead why not commit to the fact that PerasCert has BoostedBlock and Round and simply use
data PerasCert blk = PerasCert
{ pcCertRound :: PerasRoundNo
, pcCertBoostedBlock :: Point blk
}I understand some parts will perhaps only want to work with the 'round' or 'boosted block' separately, so HasFoo classes allow for that, perhaps it saves you from implementing a HasBar that you don't need in a particular test. But honestly, that's a lot of plumbing for something that doesn't look so needed.
I guess my take is that associated type/data families are overused sometimes, and it looks like we're avoiding committing to a core structure of Peras. I say, why not commit? Why not say in Peras a certificate is a thing that has a round and boosted block, rather than 'type classing' every possible cert getters which we'll end up using together anyway?
Curious to hear your thoughts!
|
@bladyjoker Good question! Actually it's a hot topic atm in our team, cf. #1829 for which we are still iterating the design. The idea is that in the future, the actual type of
But in general, I agree, at the moment indeed, we only have a |
Perfect! Curious what you'll come up with because this pattern has proliferated and we should re-evaluate it in some places. I suspect we can have much less wiring overhead in many places (for example Mempool etc). I'm also curious about HFC aspects of Peras. Is there ever a moment in which one deals with certs/votes ... basically Objects that are in different That's for example what we have with UTxOs. |
This PR comes in preparation to the implementation of the PerasVoteDB, and introduces a couple of new data types related to Peras votes and the certificate forging API:
PerasVotetype, and its correspondingValidatedPerasVote.PerasVoteTarget,PerasVoterId,PerasVoteStake, PerasVoteStakeDistrPerasQuorumStakeThreshold(corresponding toperasQuorumin the design document, section 2.1)