-
Notifications
You must be signed in to change notification settings - Fork 23
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
Small tweaks to the LoE #1125
Small tweaks to the LoE #1125
Conversation
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Outdated
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Outdated
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Outdated
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Outdated
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Outdated
Show resolved
Hide resolved
followsLoEFrag (LoEEnabled loe) frag = | ||
AF.withinFragmentBounds (AF.headPoint loe) frag | ||
|| AF.withinFragmentBounds (AF.headPoint frag) loe | ||
-> ChainAndLedger blk |
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 don't actually need the ledger, so maybe just pass in the fragment?
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.
That is true, but that would actually be more verbose to use in addToCurrentChain
and switchToAFork
so I'd prefer keeping it like this. Alternatively, to make the use of arguments clear, we could have followsLoEFrag
(now trimToLoE
) getting only a fragment and then a variation followsLoEFrag'
that gets a ChainAndLedger
and a diff, turns it into a fragment, and passes it to followsLoEFrag
. Only the latter would actually be used in code.
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Outdated
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Outdated
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Outdated
Show resolved
Hide resolved
3860cb4
to
60af229
Compare
I went into the trouble of pruning the generation of candidates instead of trimming. I find the code easier to understand since there are no subtle interactions between generation and trimming. But Uniform tests are failing still. For May I propose that we deliver this PR more or less as is, or perhaps without the trimming. Then we can try to polish the code for July by tuning the generation of candidates. I'll see if the trimming is noticeable in benchmarks. I'm guessing that the intersections could be noticeable in the 1 peer case of syncing, when chain selection dominates execution time. |
I had the exact same experience when I tried this, although my changes to
I think that is wise. |
There is a small overhead to trimming. But it doesn't seem to delay syncing or not by much in any case.
|
241f00d
to
bbcd6e3
Compare
db11bc1
to
2acc4b3
Compare
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Outdated
Show resolved
Hide resolved
Bundled everything into one commit and rebased on top of |
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Show resolved
Hide resolved
...boros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/Impl/ChainSel.hs
Outdated
Show resolved
Hide resolved
78c05a2
to
80cf37d
Compare
This commit brings several LoE-related improvements, namely: - The precondition of `followsLoEFrag` stipulating that the given candidate fragment had to intersect with the LoE fragment was often violated in the code, because the candidate fragments were anchored at the tip of the selection. - `followsLoEFrag` used to accept or reject fragments depending on whether they were LoE-compliant or not. The new verson trims them to their longest LoE-compliant prefix. Following this change, `followsLoEFrag` has been renamed to `timToLoE`. It does not filter candidates out anymore. - This last change makes `computeLoEMaxExtra` redundant. We get rid of it entirely and clean up the functions `maximalCandidates` and `extendWithSuccessors` accordingly.
This PR is a cherry pick of the commits of #1119 that make changes to the LoE:
We relax the bound in
computeLoEMaxExtra
in d32239b. Previously, this function could never returnJust 0
. This made the LoE stricter than necessary. This is not a big problem but it does not match the modelled behaviour.Instead of rejecting candidates that fork off the LoE fragment, we now trim them in 3957238. This is again just relaxing the LoE a tiny bit.
We change the helper
followsLoEFrag
in c6b1d22. It previously had a pre-condition that the anchor of the candidate had to be somewhere on the LoE fragment, but this precondition was regularly violated. The new version does not have this shortcoming.