-
Notifications
You must be signed in to change notification settings - Fork 129
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
Beacon sync revisting fc automove base2 #2995
base: master
Are you sure you want to change the base?
Changes from all commits
dacba17
7ffbc4a
c9667f8
2c23511
dfaa358
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -462,6 +462,50 @@ proc updateHeadIfNecessary(c: ForkedChainRef, pvarc: PivotArc) = | |
|
||
c.setHead(pvarc) | ||
|
||
proc autoUpdateBase(c: ForkedChainRef): Result[void, string] = | ||
## To be called after`importBlock()` for implied `base` update so that | ||
## there is no need to know about a finalised block. Here the `base` is | ||
## kept at a certain distance from the current `latest` cursor head. | ||
## | ||
# This function code is a tweaked version of `importBlockBlindly()` | ||
# from draft PR #2845. | ||
# | ||
let | ||
distanceFromBase = c.cursorHeader.number - c.baseHeader.number | ||
hysteresis = max(1'u64, min(c.baseDistance div 4'u64, 32'u64)) | ||
# Finalizer threshold is baseDistance + 25% of baseDistancce capped at 32. | ||
if distanceFromBase < c.baseDistance + hysteresis: | ||
return ok() | ||
|
||
# Move the base forward and stay away `baseDistance` blocks from | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this necessarily valid? For example, a chain might never have finalized. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs to be discussed/re-visited, probably. Was ported from an earlier PR. |
||
# the top block. | ||
let | ||
target = c.cursorHeader.number - c.baseDistance | ||
pvarc = ?c.findCursorArc(c.cursorHash) | ||
newBase = c.calculateNewBase(target, pvarc) | ||
|
||
doAssert newBase.pvHash != c.baseHash | ||
|
||
# Write segment from base+1 to newBase into database | ||
c.stagingTx.rollback() | ||
c.stagingTx = c.db.ctx.txFrameBegin() | ||
c.replaySegment(newBase.pvHash) | ||
c.writeBaggage(newBase.pvHash) | ||
c.stagingTx.commit() | ||
c.stagingTx = nil | ||
|
||
# Update base forward to newBase | ||
c.updateBase(newBase) | ||
c.db.persistent(newBase.pvNumber).isOkOr: | ||
return err("Failed to save state: " & $$error) | ||
|
||
# Move chain state forward to current head | ||
c.stagingTx = c.db.ctx.txFrameBegin() | ||
c.replaySegment(pvarc.pvHash) | ||
c.setHead(pvarc) | ||
|
||
ok() | ||
|
||
# ------------------------------------------------------------------------------ | ||
# Public functions | ||
# ------------------------------------------------------------------------------ | ||
|
@@ -523,7 +567,11 @@ proc newForkedChain*(com: CommonRef, | |
com.syncStart = baseHeader.number | ||
chain | ||
|
||
proc importBlock*(c: ForkedChainRef, blk: Block): Result[void, string] = | ||
proc importBlock*( | ||
c: ForkedChainRef; | ||
blk: Block; | ||
autoRebase = false; | ||
): Result[void, string] = | ||
# Try to import block to canonical or side chain. | ||
# return error if the block is invalid | ||
if c.stagingTx.isNil: | ||
|
@@ -533,7 +581,10 @@ proc importBlock*(c: ForkedChainRef, blk: Block): Result[void, string] = | |
blk.header | ||
|
||
if header.parentHash == c.cursorHash: | ||
return c.validateBlock(c.cursorHeader, blk) | ||
?c.validateBlock(c.cursorHeader, blk) | ||
if autoRebase: | ||
return c.autoUpdateBase() | ||
return ok() | ||
|
||
if header.parentHash == c.baseHash: | ||
c.stagingTx.rollback() | ||
|
@@ -555,7 +606,12 @@ proc importBlock*(c: ForkedChainRef, blk: Block): Result[void, string] = | |
# `base` is the point of no return, we only update it on finality. | ||
|
||
c.replaySegment(header.parentHash) | ||
c.validateBlock(c.cursorHeader, blk) | ||
?c.validateBlock(c.cursorHeader, blk) | ||
if autoRebase: | ||
return c.autoUpdateBase() | ||
|
||
ok() | ||
|
||
|
||
proc forkChoice*(c: ForkedChainRef, | ||
headHash: Hash32, | ||
|
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.
btw, this function is a step in a good direction: the responsibility for updating the base should indeed rest firmly with FC.
However, there's nothing "special" with this flow at all, like this documentation makes it seem - instead, it should be considered a normal part of the importBlock flow and of the fCU flow.
There are two things that can cause the base to be updated:
Both of these should update base potentially and both flows should call the same function to do so, also so that the hysteresis computation below applies to both "sources" of base updates equally. This means that this same function should be called for all base updates that happen in FC.
Long-range syncing, ie syncing blocks that are older than finality, is a special case of the above logic because FC already knows that they form a linear history, but for the updateBase logic it does not matter: the execution head is what determines base in this case, that and the execution head in the case of long-range syncing is simply a block on the DAG branch that leads to finality and eventually the consensus head.