Conversation
bartfrenk
force-pushed
the
bartfrenk/private-16/no-pipelining-of-future-blocks
branch
2 times, most recently
from
845fdd9 to
b06b8f8
Compare
bartfrenk
force-pushed
the
bartfrenk/private-16/no-pipelining-of-future-blocks
branch
from
b06b8f8 to
a81b00e
Compare
| checkInFuture :: ValidatedFragment (Header blk) (LedgerState blk) | ||
| -- > checkInFuture _ af >>= \(af', fut) -> | ||
| -- > af == af' <=> null fut | ||
| checkInFuture :: LedgerState blk -> AnchoredFragment (Header blk) |
There was a problem hiding this comment.
Please add a comment explaining the assumptions about the given ledger state and the given fragment.
| (hardForkSummary cfg (VF.validatedLedger validated)) | ||
| now | ||
| (VF.validatedFragment validated) | ||
| checkInFuture = \ledgerState af -> do |
There was a problem hiding this comment.
Since checkFragment has an error case in it, please add a comment here explaining how the assumptions made by checkInFuture guard this call to checkFragment.
| -- Remove future blocks from the chain, and update the ChainDB state | ||
| -- that keeps track of those blocks, so we can take them into account | ||
| -- in the decision to make blocks available for pipelining. | ||
| candidate' <- futureCheckCandidate chainSelEnv candidate |
There was a problem hiding this comment.
Hmm. Unless you've figure out a more thorough argument, my understanding was that we would leave the existing future check as-is, and merely add a check to only enable pipelining when the translation is available and isn't in the future. At the moment, this diff seems to me to be removing/changing the existing future check instead of leaving it as-is.
| -- > checkInFuture vf >>= \(af, fut) -> | ||
| -- > validatedFragment vf == af <=> null fut | ||
| checkInFuture :: ValidatedFragment (Header blk) (LedgerState blk) | ||
| -- > checkInFuture _ af >>= \(af', fut) -> |
There was a problem hiding this comment.
Perhaps we could use this opportunity to add more comments to this function (especially the return type).
|
@bartfrenk This commit sketches what I think is the minimal possible change: 1bc9135 Does it look sound to you? cc @amesgen |
|
Closing in favor of #4334. |
3 participants
Description
Updates chain selection to first check for blocks from the future before setting the tentative header. This avoids pipelining blocks from the future.
Checklist