Skip to content

[WIP] Add pure Peras voting rules #1723

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.

Sign up for GitHub

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

Jump to bottom
Draft
Parent: Peras staging ground
wants to merge 7 commits into
base: peras-staging
Choose a base branch
from
Draft

[WIP] Add pure Peras voting rules #1723

wants to merge 7 commits into from
+1,154 −46

Conversation

@agustinmista
Copy link
Contributor

@agustinmista agustinmista requested a review from amesgen October 21, 2025 10:55
@agustinmista agustinmista self-assigned this Oct 21, 2025
agustinmista
agustinmista commented Oct 21, 2025
View reviewed changes
Copy link
Contributor Author

@agustinmista agustinmista left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some implementation details are yet to be defined, especially around PerasVotingView

@agustinmista agustinmista force-pushed the peras/pure-voting-rules branch from abde225 to 8f63b94 Compare October 21, 2025 11:15
amesgen
amesgen reviewed Oct 21, 2025
View reviewed changes
Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, some first thoughts

@agustinmista agustinmista force-pushed the peras/pure-voting-rules branch 8 times, most recently from 966b262 to 2bf3cfb Compare October 27, 2025 09:44
@agustinmista agustinmista force-pushed the peras/pure-voting-rules branch 5 times, most recently from 55feefc to 713f30f Compare October 27, 2025 11:25
@agustinmista agustinmista requested a review from amesgen October 27, 2025 11:32
@agustinmista agustinmista force-pushed the peras/pure-voting-rules branch from 713f30f to 564060c Compare October 27, 2025 11:36
@agustinmista agustinmista force-pushed the peras/pure-voting-rules branch from 564060c to aa884d7 Compare October 27, 2025 11:57
amesgen
amesgen reviewed Oct 27, 2025
View reviewed changes
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Peras/Voting.hs
Comment on lines +164 to +175
-- NOTE: in the case of an extremely old certificate boosting a block
-- beyond the immutable prefix, this could incorrectly return false even
-- if the voting candidate technically extends the certificate point.
-- However, this a boring case that we can safely ignore. Conversely,
-- the case of a certificate that's too new to be voted for is covered
-- by using the approriate prefix of our current chain.
candidateExtendsCert cert =
AF.withinFragmentBounds certBoostedBlockHeader chainAtCandidate
where
certBoostedBlockHeader = castPoint (getPerasCertBoostedBlock cert)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely fine for now, but we could also remove this caveat if we store one extra bit of information in the ChainDB/PerasCertDB, namely whether the most recent certificate boosts a block on our selected fragment or the immutable chain. Let's create an issue for this.

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Peras/Voting.hs
Comment on lines 84 to 89
, pvvLatestCertSeen :: !cert
-- ^ The most recent certificate seen by the voter.
, pvvLatestCertOnChain :: !cert
-- ^ The most recent certificate present in some chain.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, one real-world consideration here is what to do if there have been no certificates so far (eg because Peras just got enabled via a hard fork, or even after that if Peras immediately entered a cooldown). We of course need to make sure that we actually start voting once Peras is enabled 😅

The formal spec introduces a special cert₀ certificate for this purpose (for round 0 and boosting the genesis point). Maybe we could make this work, but it might be better to be a bit more explicit and use Maybe cert (or an isomorphic ADT) here. The round number of a Maybe cert could then be a WithOrigin PerasRoundNo.

@agustinmista
Add onPerasRoundNo helper and Num instance
b625685
@agustinmista
Add explainable boolean predicate evaluator
d4ca991
@agustinmista
Introduce O.C.Peras.Params
50bffb4
@agustinmista
Move geometric into Test.Util.QuickCheck
9703346
@agustinmista
Add pure Peras Voting rules
6aa4a58 
With the currently selected generation sizes, we hit the different
voting rules in a somewhat decent proportion, even against randomly
generated functions as part of the PerasVotingView interface.

ouroboros-consensus
  Peras
    Peras voting rules
      isPerasVotingAllowed: OK (1.55s)
        +++ OK, passed 10000 tests.

        Actual result (10000 in total):
        60.29% NoVoteReason(VR-1A or VR-2A)
        20.90% NoVoteReason(VR-1A or VR-2B)
         9.60% VoteReason(VR-2A and VR-2B)
         4.93% VoteReason(VR-1A and VR-1B)
         2.67% NoVoteReason(VR-1B or VR-2A)
         1.61% NoVoteReason(VR-1B or VR-2B)

        Should vote according to model (10000 in total):
        85.47% False
        14.53% True

        VR-(1A|1B|2A|2B) (10000 in total):
        21.30% (False,True,False,False)
        21.24% (False,False,False,False)
        10.63% (False,False,True,False)
        10.27% (False,True,True,False)
         8.99% (False,False,False,True)
         8.76% (False,True,False,True)
         4.67% (False,True,True,True)
         4.26% (False,False,True,True)
         1.93% (True,False,False,False)
         1.77% (True,True,False,False)
         1.71% (True,True,True,False)
         1.61% (True,False,True,False)
         0.75% (True,True,True,True)
         0.74% (True,False,False,True)
         0.70% (True,True,False,True)
         0.67% (True,False,True,True)

        VR-1A (10000 in total):
        90.12% False
         9.88% True

        VR-1B (10000 in total):
        50.07% False
        49.93% True

        VR-2A (10000 in total):
        65.43% False
        34.57% True

        VR-2B (10000 in total):
        70.46% False
        29.54% True
@agustinmista agustinmista force-pushed the peras/pure-voting-rules branch from aa884d7 to b46df86 Compare October 28, 2025 09:27
@agustinmista agustinmista force-pushed the peras/pure-voting-rules branch from b46df86 to 9869f7a Compare October 28, 2025 10:36
@agustinmista agustinmista mentioned this pull request Oct 28, 2025
Open
amesgen
amesgen reviewed Oct 28, 2025
View reviewed changes
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsPeras.hs
HasPerasCertBoostedBlock cert =>
HasPerasCertBoostedBlock (WithOriginCert cert)
where
type BoostedBlock (WithOriginCert cert) = WithOrigin (BoostedBlock cert)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this feels a bit weird, as we have two levels of WithOrigin here, eg this will be WithOrigin (Point blk) in the end, which has three kinds of inhabitants, where the first two seem weird to both exist:

  • Origin
  • NotOrigin GenesisPoint
  • NotOrigin (BlockPoint slot hash)

I think we can collapse this.

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsPeras.hs
Comment on lines +219 to +221
getPerasCertRound = \case
Cert0 -> Origin
Cert cert -> At (getPerasCertRound cert)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels a bit surprising to have both getPerasCertRound and withOriginCertRoundNo for WithOriginCert. AFAICT, we don't actually use this instance at the moment at all, and instead only use withOriginCertRoundNo.

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Peras/Voting.hs
Comment on lines 86 to 87
, pvvLatestCertSeen :: !cert
-- ^ The most recent certificate seen by the voter.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to be WithOriginCert.

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsPeras.hs
Comment on lines +186 to +192
-- | Eliminate the origin certificate case for a round number
--
-- NOTE: this assumes that the origin certificate corresponds to round number 0
withOriginCertRoundNo ::
(HasPerasCertRound cert, Num (RoundNo cert)) =>
WithOriginCert cert -> RoundNo cert
withOriginCertRoundNo = withOriginCert 0 getPerasCertRound
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the previous comment: I think it would be a bit more in line to treat the round number of Cert0 specially, ie assume that it is Origin which is the slot before slot 0.

This is similar to how slot numbers work in the current code base: The pointSlot of a Point blk has type WithOrigin SlotNo, which is Origin when the point is the GenesisPoint. This makes arithmetic involving slots a bit annoying sometimes, but we have some utility functions like succWithOrigin :: WithOrigin SlotNo -> SlotNo that make it bearable.

For example, checking VR-1A would look very roughly like this:

  VR1A
    := (pvvCurrRoundNo pvv :==: succWithOrigin latestCertSeenRoundNo)
    :/\: case pvvLatestCertSeen pvv of
           Cert0 -> Bool True
           Cert c -> latestCertSeenArrivalSlot c :<=: latestCertSeenRoundStart c + _X

Does this make sense, given that you might have already tried sth like this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amesgen amesgen amesgen left review comments

@nfrisby nfrisby Awaiting requested review from nfrisby nfrisby will be requested when the pull request is marked ready for review nfrisby is a code owner

@jasagredo jasagredo Awaiting requested review from jasagredo jasagredo will be requested when the pull request is marked ready for review jasagredo is a code owner

@fraser-iohk fraser-iohk Awaiting requested review from fraser-iohk fraser-iohk will be requested when the pull request is marked ready for review fraser-iohk is a code owner

@dnadales dnadales Awaiting requested review from dnadales dnadales will be requested when the pull request is marked ready for review dnadales is a code owner

@geo2a geo2a Awaiting requested review from geo2a geo2a will be requested when the pull request is marked ready for review geo2a is a code owner

Assignees

@agustinmista agustinmista

Labels

Peras

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@agustinmista @amesgen