-
Notifications
You must be signed in to change notification settings - Fork 33
[Peras 2] Introduce PerasCertDB and related tests
#1674
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
Conversation
d3cd2fc to
8df7902
Compare
8a7ab97 to
0e67f26
Compare
PerasCertDB and related testsPerasCertDB and related tests
f3fbd4f to
01e1c69
Compare
8f303af to
6649e71
Compare
2fdba58 to
3db7a62
Compare
6649e71 to
e18c8d6
Compare
PerasCertDB and related testsPerasCertDB and related tests
ouroboros-consensus/changelog.d/20250917_101832_thomas.bagrel_peras_cert_db.md
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/PerasCertDB/API.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/PerasCertDB/API.hs
Outdated
Show resolved
Hide resolved
|
|
||
| -- * 'PerasCertSnapshot' | ||
| , PerasCertSnapshot (..) | ||
| , PerasCertTicketNo |
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 do we need ticket numbers for Peras certificates?
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.
We allocate PerasCertTicketNos in monotonically increasing fashion as new certificates arrive, just as we do for transactions in the mempool (we should add this in a Haddock comment). This way, anyone can subscribe to certificates as they arrive, by asking for more certificates after the last ticket number they know of, using getCertsAfter.
One thing to note here is that Peras round numbers almost could play the role of ticket numbers: Apart from edge cases, we will never receive a Peras certificate for round r after we received one for round s if r < s. However, because of these edge cases and a general concern for robustness, we are not using round numbers as "almost ticket numbers".
Lastly, we could also hide the ticket numbers by exposing a more monadic API (essentially a dequeue interface), where the PerasCertNo is maintained "internally". (This would be possible both here and in the Mempool API)
Status quo (simplified) in this PR:
getCertSnapshot :: STM m (PerasCertSnapshot blk)
data PerasCertSnapshot blk = PerasCertSnapshot
{ getCertsAfter :: PerasCertTicketNo -> [(ValidatedPerasCert blk, PerasCertTicketNo)]
}Here, the caller is responsible for maintaining the PerasCertTicketNo across calls to getCertsAfter/getCertSnapshot.
More monadic option without ticket numbers:
getCertFollower :: STM m (PerasCertFollower blk)
data PerasCertFollower blk = PerasCertFollower
{ getNextPerasCert :: STM m (ValidatedPerasCert blk)]
}Here, getNextPerasCert would internally maintain the PerasCertTicketNo in a TVar (or we could change the implementation to be based on a TChan 🤔 ).
Let us know if you find this appealing, we can explore that.
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.
However, because of these edge cases and a general concern for robustness, we are not using round numbers as "almost ticket numbers".
That makes sense. Should we document this properly (if it's not documented already). Thanks!
Let us know if you find this appealing, we can explore that.
I can't say I have a preference, but the current design seems simpler.
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.
Should we document this properly (if it's not documented already). Thanks!
I enriched the Haddocks of PerasCertTicketNo appropriately 👍
|
|
||
| -- | Volatile Peras certificate state, i.e. certificates that could influence | ||
| -- chain selection by boosting a volatile block. | ||
| data PerasVolatileCertState blk = PerasVolatileCertState |
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.
Perhaps we should add some invariant checking here. For instance:
- Should the different maps have all the same size?
- For every certificate in
pvcsCerts, is there a corresponding weight inpvcsWeightByPoint? - Should the range of
pvcsCertsandpvcsCertsByTicketbe the same? - etc.
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.
It might also be worth explaining why we're splitting the information of Peras certificates into multiple maps.
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 believe the answer to the last question is that these are particular indices that make call site queries execute optimally, otherwise we'd be scanning the entire pvcsCerts looking for a ticketNo.
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.
@bladyjoker is exactly right, we want to efficiently support different use cases using different representations, so that's why we have some redundancy.
In fact, we opened #1649 and submitted #1651 precisely to easily attach an invariant for this particular case, but somehow forgot to do this before opening this PR. Thanks for catching that!
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.
Done, see invariantForPerasVolatileCertState
ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/PerasCertDB/StateMachine.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/PerasCertDB/Impl.hs
Show resolved
Hide resolved
ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/PerasCertDB/StateMachine.hs
Show resolved
Hide resolved
ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/PerasCertDB/StateMachine.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/PerasCertDB/StateMachine.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/PerasCertDB/API.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/PerasCertDB/Impl.hs
Show resolved
Hide resolved
2d628de to
d0e0655
Compare
Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
Co-authored-by: Agustin Mista <[email protected]> Co-authored-by: Alexander Esgen <[email protected]> Co-authored-by: Georgy Lukyanov <[email protected]> Co-authored-by: Thomas BAGREL <[email protected]> Co-authored-by: Nicolas BACQUEY <[email protected]> Co-authored-by: Nicolas "Niols" Jeannerod <[email protected]>
d0e0655 to
3f2b421
Compare
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.
This was a lovely read! Looks good :)
This pull request introduces the
PerasCertDBdatatype, used to hold and access allPerasCerts known by a node that could affect chain selection.In the mid/near-term future, this will be enriched to persist certificates across restarts, and probably also to store historical certificates.
Peras types
Ouroboros.Consensus.Storage.PerasCertDB{,.API,.Impl}, notably defining the typesPerasCertDB,PerasCertSnapshot(read-only snapshot of certs contained in the DB), andAddPerasCertResult; alongside their respectives methodsTests
Test.Ouroboros.Storage.PerasCertDB{,.StateMachine,.Model}for q-s-m testing of thePerasCertDBdatatype. The corresponding tests have been included in the test suite defined byTest.Ouroboros.StorageRegression
As the previous PR, all the components introduced by this PR are not used at the moment by the rest of the code, so no regression is expected.