-
Notifications
You must be signed in to change notification settings - Fork 33
[Peras 3] Make ChainDB aware of PerasCertDB, and switch to weighted chain selection
#1678
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
42bd02b to
b084c4a
Compare
ChainDB aware of PerasCertDB, and switch to weighted chain selectionChainDB aware of PerasCertDB, and switch to weighted chain selection
0e67f26 to
91a270e
Compare
ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/ChainDB/StateMachine.hs
Outdated
Show resolved
Hide resolved
8f303af to
6649e71
Compare
d7c3c64 to
f1158c0
Compare
6649e71 to
e18c8d6
Compare
ChainDB aware of PerasCertDB, and switch to weighted chain selectionChainDB aware of PerasCertDB, and switch to weighted chain selection
e18c8d6 to
70d336f
Compare
f1158c0 to
90131b2
Compare
2d628de to
d0e0655
Compare
d0e0655 to
3f2b421
Compare
90131b2 to
3fe15d3
Compare
04b7745 to
8aba6fc
Compare
8aba6fc to
8b987f6
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.
I reviewed the following commits:
- ChainDB: expose PerasCertDB functionality
- SecurityParam: mention weighted nature
- O.C.Peras.Weight: add takeVolatileSuffix
- ChainDB: define getCurrentChain in terms of weight
- ChainDB.StateMachine: check immutable tip monotonicity
- GSM: allow candidateOverSelection to be stateful
- ChainSel: make rollbackExceedsSuffix weight-aware
- Peras.SelectView: initialize, expose WeightedSelectView
I'll try to continue tomorrow
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/API.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Config/SecurityParam.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Config/SecurityParam.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Config/SecurityParam.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Peras/Weight.hs
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/Peras/WeightSnapshot.hs
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/Peras/WeightSnapshot.hs
Show resolved
Hide resolved
ouroboros-consensus/test/consensus-test/Test/Consensus/Peras/WeightSnapshot.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Peras/SelectView.hs
Show resolved
Hide resolved
...ensus/src/ouroboros-consensus/Ouroboros/Consensus/MiniProtocol/BlockFetch/ClientInterface.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Util/AnchoredFragment.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Types.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Query.hs
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/Types.hs
Outdated
Show resolved
Hide resolved
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.
Reviewed:
[x] ChainDB: expose PerasCertDB functionalityy
LGTM
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.
Took the time to understand the takeVolatileSuffix functions and AnchoredFragment library. Left a few comments, curious what you think.
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Peras/Weight.hs
Outdated
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Show resolved
Hide resolved
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]>
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]>
This is in preparation for weighted chain comparisons. 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]>
Also remove the version for `ValidatedChainDiff` as it is unused. 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]>
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]>
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]>
328b50a to
1018f6e
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.
Fyi this is noop for you folks, just wanted to share my thoughts on the overall code structure. I'm a complete beginner in this codebase so consider this as merely a conversation starter (after all, you are much better positioned to structure this code).
While attempting to comprehend the overall changes, I try to reason about what's common and what's different between Vanilla Praos and Peras Praos. Thinkering with how to generalize the code structure such that the differences can be a matter of supplying a different argument to a function (ie. dependency injection).
From what I can tell, Peras is all about summarizing or measuring the chain suffix differently, which means it needs weights to calculate it. It's also a 'special' deal that Peras with empty weights is simply Vanilla Praos, and we explicitly differentiate between them such that we gain some efficiency (eg. vanilla suffix length rule) in several places.
Currently, we've seen a couple of things in this PR:
- Peras artifacts like
weightsand friends is threaded through multiple functions - Often we differentiate between Vanilla and Peras cases using "empty" weights to gain efficiency and dispatch accordingly
- The Vanilla and Peras code bits are often together side by side.
My question to you is whether you see a pragmatic refactoring that would further isolate Peras bits into separate modules (like you already did in some places), while generalizing the common parts to accommodate for what now is Vanila and Peras.
Ideally, I would like to see:
- Explicit dispatching
import qualified Peras
import qualified Vanila
doSmtn =
| isVanila = Vanila.doSmtn
| isPeras = Peras.doSmtn- Generalized functions
doSmtnWith : (AnchoredFragment -> s) -> AnchoredFragment -> ...
doSmtnWith summarizeSuffix frag = ...which would push the decision on the concrete version to the caller, hopefully further elucidating convenient code structures and minimizing the spread of Peras specific artifacts to only the necessary parts (where the wiring happens).
| -- NOTE: This talks about the number of /blocks/ we can roll back, not | ||
| -- the number of /slots/. | ||
| -- | ||
| -- In weightiest-chain protocols (such as Ouroboros Peras), we interpret this as |
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.
Somehow overloading the meaning of a SecurityParameter implicitly makes me feel a bit uncomfortable. We collate the measure of "meters" with that of "grams" so to speak.
k in Praos means "count", and k in Peras means "weight", and there's an explicit relationship between them.
Do you see a way to perhaps be more explicit about this? Separating into Praos and Peras sub modules etc.
| , wsvWeightBoost :: !PerasWeight | ||
| -- ^ The weight boost of a fragment (w.r.t. a particular anchor). | ||
| , wsvTiebreaker :: TiebreakerView proto | ||
| -- ^ Lazy because it is only needed when 'wsvTotalWeight' is inconclusive. |
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.
Consider using Praos.SelectView (is such thing exists) to explicitly denote the relationship between Paros and Peras select views.
| maxRollbackWeight :: SecurityParam -> PerasWeight | ||
| maxRollbackWeight = PerasWeight . unNonZero . maxRollbacks |
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.
Just seen it used in takeVolatileSuffix. Upon reflection I'm thinking that perhaps maxRollbackWeight to be in Peras/SecurityParameter.hs and the Peras spin on the documentation attached to the SecurityParameter here move to the new module and attached to maxRollbackWeight
| -- | Return 'True' iff applying the 'ChainDiff' to the given chain @C@ will | ||
| -- result in a chain with less weight than @C@, i.e., the suffix of @C@ to roll | ||
| -- back has more weight than suffix is adding. | ||
| rollbackExceedsSuffix :: |
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.
In terms keeping Peras/Praos line explicit, wdyt about having
forall b1 b2.
( HasHeader b1
, HasHeader b2
) =>
(AnchoredFragment b1 -> s) ->
AnchoredFragment b1 ->
ChainDiff b2 ->
Bool
rollbackExceedsSuffix summarize curChain (ChainDiff nbRollback suffix) =
summarize suffixToRollBack > summarize suffix
where
suffixToRollBack = AF.anchorNewest nbRollback curChainJust thinking how to move direct mentions of Peras closer to the call sites.
| -- Filter out candidates that have less weight than the current | ||
| -- chain. We don't want to needlessly read the headers from disk | ||
| -- for those candidates. | ||
| . NE.filter (not . Diff.rollbackExceedsSuffix weights curChain) |
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.
Not sure how committed you are to isEmpty weights optimization you used in different places, seems like it could be used here too.
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.
The only "expensive" part of rollbackExceedsSuffix is totalWeightOfFragment, and that one is already using the isEmpty weights optimization, so I opted to not use it here.
| preferAnchoredCandidate cfg weights ours cand | ||
| | isEmptyPerasWeightSnapshot weights = | ||
| assertWithMsg (precondition ours cand) $ | ||
| case (ours, cand) of | ||
| (_, Empty _) -> False | ||
| (Empty ourAnchor, _ :> theirTip) -> | ||
| blockPoint theirTip /= castPoint (AF.anchorToPoint ourAnchor) | ||
| (_ :> ourTip, _ :> theirTip) -> | ||
| preferCandidate | ||
| (projectChainOrderConfig cfg) | ||
| (selectView cfg (getHeader1 ourTip)) | ||
| (selectView cfg (getHeader1 theirTip)) | ||
| | otherwise = | ||
| case AF.intersect ours cand of | ||
| Nothing -> error "precondition violated: fragments must intersect" | ||
| Just (_oursPrefix, _candPrefix, oursSuffix, candSuffix) -> | ||
| preferCandidate | ||
| (projectChainOrderConfig cfg) | ||
| (weightedSelectView cfg weights oursSuffix) | ||
| (weightedSelectView cfg weights candSuffix) |
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.
A couple of questions here:
- Is there a version of this function that for both cases Vanilla/Peras uses the same body? Can the vanilla case be expressed using the AF.intersect as well? Not sure how the cases in vanilla body relates to the Peras cases.
- I suspect there's an opportunity to generalize this function over the 'type of select view'.
preferAnchoredCandidate selectView cfg ours cand =
...
preferCandidate
(projectChainOrderConfig cfg)
(selectView cfg (getHeader1 ourTip))
(selectView cfg (getHeader1 theirTip))
...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.
Is there a version of this function that for both cases Vanilla/Peras uses the same body? Can the vanilla case be expressed using the AF.intersect as well? Not sure how the cases in vanilla body relates to the Peras cases.
Yes, the long-term goal here is to just always use the Peras logic. We even did this at first while developing on a branch, but switched to this approach (where we transparently use the Praos logic when there are no certificates) to make it trivial to argue that our changes are not introducing any (performance) regressions.
I suspect there's an opportunity to generalize this function over the 'type of select view'.
Yes, though if sharing as much code as possible is the goal, then using the Peras logic would be the way to go.
| data TraceAddPerasCertEvent blk | ||
| = -- | The Peras certificate from the given round boosting the given block was |
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.
Consider placing these in the Peras subdirectory you have?
| (selectView cfg (getHeader1 tip')) | ||
| compareAnchoredFragments cfg weights frag1 frag2 |
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.
Is there a concise common body based on AF.intersect that can be used in both cases? If so, we can perhaps shorten this and generalize it?
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.
| -- If @ours@ and @cand@ do not intersect, this returns 'False'. If they do | ||
| -- intersect, then we check that the suffix of @ours@ after the intersection has | ||
| -- total weight at most @k@. | ||
| forksAtMostKWeight :: |
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.
Similar story, perhaps this could be decomposed into a generalized forksAtMost and then further provide the Vanilla version along with Peras version?
|
@bladyjoker Thanks a lot for your thoughtful comments!
A high-level remark here is that we currently only keep the previous Praos logic for explicitness and to avoid any possible (performance) regression when Peras is disabled. Looking ahead, the idea would be to remove the Praos/"Vanilla" logic and uniformly make use of the Peras logic, leveraging the property you mention that Peras degrades to Praos when there are no certificates. I think separating the Peras logic into its own module hierarchy more explicitly is an interesting idea to explore; I will definitely keep this in mind for future PRs. Eg if we make further tweaks to just the Peras logic in the future, it seems nice that this is made explicit just by looking at which directories/files are being modified. Relatedly: Other changes due to Peras, such as the new mini-protocols (see eg #1679) are put behind explicit an explicit Finally, one option in the very long term would be to make the abstract Consensus layer (ie the I discussed this with the team in the past, and we concluded that this would be premature at this point; building an abstractions at this level of generality is very hard of course. It is easy to end up with an abstraction that is too general, such that one can hardly reason about how code using it will behave (similar to how unconstrained/lawless type classes can cause trouble). |
Description
This PR introduces:
Commits
The commits are intended to be reviewed individually:
ChainDB: expose PerasCertDB functionality
The ChainDB maintains a PerasCertDB internally, and exposes most of its functionality through its public API. This is analogous to how the LedgerDB is managed by the ChainDB.
SecurityParam: mention weighted naturePurely a documentation update to mention the richer dynamics under Peras, plus a small helper function.
O.C.Peras.Weight: add
takeVolatileSuffixIn Praos, the volatile suffix of a chain is defined to be the
kmost recent blocks. Analogously, in Peras, the volatile suffix of a chain is defined to be the longest suffix with weight at mostk.This commits adds an appropriate function (via a binary search), as well as documentation and tests.
ChainDB: define
getCurrentChainin terms of weightThis makes use of the previous commit in the ChainDB. Note that Minimize exposure to exact immutability criterion #1619 guarantees that the LedgerDB automatically uses the same notion of immutability.
Note that this means that the immutable tip will be less than$U = 90$ slots and a Peras boost of $B = 15$ , the length of the volatile suffix decreases from $k=2160$ to
kblocks behind the tip when Peras is working well, ie every Peras round is giving rise to a certificate. Concretely, for plausible parameters, ie a round length ofon average.
ChainDB.StateMachine: check immutable tip monotonicity
To make sure that the more refined immutability criterion doesn't introduce any surprises, we add a postcondition to the ChainDB q-s-m test that the immutable tip never recedes.
GSM: allow
candidateOverSelectionto be stateful and ChainSel: makerollbackExceedsSuffixweight-awarePreparatory refactorings for when the chain comparison will become weighted and hence additionally depend on the
PerasWeightSnapshot.Peras.SelectView: initialize, expose
WeightedSelectViewThis introduces the notion of a weighted
SelectView, making use of Refactor:SelectView=BlockNo×TiebreakerView#1591ouroboros-consensus/ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Peras/SelectView.hs
Lines 35 to 48 in 90131b2
This will used in place of the "normal"
SelectView. Eventually, we could also remove the normalSelectViewand replace it withWeightedSelectView. However, to keep this already big PR more focused, we propose to do this in the future.Introduce weighted chain comparisons
The core change in this commit is in the
O.C.Util.AnchoredFragmentto thepreferAnchoredCandidateandcompareAnchoredFragmentsfunctions. These nowPerasWeightSnapshotas an additional argument.We keep the old implementation in case the
PerasWeightSnapshotis empty; which is semantically unnecessary, but is a trivial way to make sure that no performance regression is introduced.The rest of the diff of this commit is simply due to adapting the respective call sites in a rather straightforward fashion.
Integrate weighted BlockFetch decision logic
This commit plugs the weighted chain comparison logic into the BlockFetch decision logic. This relies on a (merged, but not yet released, hence the s-r-p) Network change, see Support weighted chain comparisons ouroboros-network#5161 for a detailed description.
ChainDB: implement chain selection for Peras certificates
We allow new Peras certificates to be added to the ChainDB (and therefore to the managed PerasCertDB). If the certificate is boosting a block that is not on the current selection, we perform chain selection for it, potentially switching to a fork containing it if it now is weightier than our selection.
MockChainSel: switch to weighted chain selection
This logic is for example used in tests.
ChainDB q-s-m: test weighted chain selection
We enrich the model ChainDB implementation with weighted chain selection, and add a new command with a simple generator for adding certificates. We also enrich labelling with the
TagSwitchedToShorterChaintag that shows that we sometimes do switch to a shorter chain; which would be a bug without Peras.A follow-up PR (Better certificate generation in ChainDB q-s-m test #1670) will further improve the generators of this test.
Regression
As of today, there is no way to generate certificates, so
PerasCertDBis always empty, and the change to the chain selection algorithm is therefore invisible: in the absence of certificates, the Peras weight of a chain is equal to its length. Therefore, this PR does not introduce any semantic changes.Furthermore, from a performance standpoint, care is taken to ensure that if there are no certificates, we use the unweighted Praos logic, meaning that there is no change in performance when Peras is disabled.