Update forkchoice using latestValidHash updates from execution engine #4182
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Performance Report✔️ no performance regression detected Full benchmark results
|
g11tech
force-pushed
the
g11tech/latest-valid-hash-update
branch
2 times, most recently
from
ff89835
to
b5d6d0d
Compare
g11tech
force-pushed
the
g11tech/latest-valid-hash-update
branch
2 times, most recently
from
2e6beb1
to
c9a28af
Compare
twoeths
reviewed
Jul 6, 2022
twoeths
reviewed
Jul 6, 2022
twoeths
reviewed
Jul 6, 2022
| } | ||
| } | ||
|
|
||
| propagateInValidExecutionStatusByIndex(invalidateFromIndex: number, latestValidHashIndex: number): void { |
There was a problem hiding this comment.
can you make the 1st param name consistent to the consumer? this is called invalidateFromIndex while the consumer calls invalidateTillBlockIndex
also it's nice to have some descriptions in the method header, or draw some ascii texts representing forkchoice branches
twoeths
reviewed
Jul 6, 2022
| /** invalidateAll is a flag which indicates detection of consensus failure */ | ||
| let invalidateAll = false; | ||
| let invalidateIndex: number | undefined = invalidateFromIndex; | ||
| while (invalidateIndex !== undefined && invalidateFromIndex > latestValidHashIndex) { |
There was a problem hiding this comment.
should it be
while (invalidateIndex !== undefined && invalidateIndex > latestValidHashIndex) {There was a problem hiding this comment.
I suppose adding some unit tests for those new methods should help
There was a problem hiding this comment.
fixed! thanks for spotting it @tuyennhv ! yes will add a test case around this as well.
g11tech
force-pushed
the
g11tech/latest-valid-hash-update
branch
from
7373342
to
adedd33
Compare
twoeths
reviewed
Jul 11, 2022
|
|
||
| if (execResponse.executionStatus === ExecutionStatus.Valid) { | ||
| const {latestValidExecHash} = execResponse; | ||
| let latestValidHashIndex = this.nodes.length - 1; |
There was a problem hiding this comment.
the default latest valid hash index should not be the last node, maybe make it undefined or null?
There was a problem hiding this comment.
thanks for the input, i have initialized to -1, keeping in line with "not found" assignments of the indices. if you still feel strongly for undefined or null to represent not found, do respond 🙂
twoeths
reviewed
Jul 11, 2022
| const {latestValidExecHash} = execResponse; | ||
| let latestValidHashIndex = this.nodes.length - 1; | ||
|
|
||
| for (; latestValidHashIndex >= 0; latestValidHashIndex--) { |
There was a problem hiding this comment.
perhaps extract this to an array util so that it's reusable and testable
There was a problem hiding this comment.
its not really generic, and is covered by the validation test cases.
twoeths
reviewed
Jul 11, 2022
| @@ -193,17 +206,23 @@ export async function verifyBlockStateTransition( | |||
| switch (execResult.status) { | |||
| case ExecutePayloadStatus.VALID: | |||
| executionStatus = ExecutionStatus.Valid; | |||
| chain.forkChoice.validateLatestHash(execResult.latestValidHash, null); | |||
| chain.forkChoice.validateLatestHash({ | |||
There was a problem hiding this comment.
@g11tech my understanding is latestValidHash is the current execution payload which we have not imported into forkchoice, is it an issue?
There was a problem hiding this comment.
@tuyennhv its not an issue here, since the latestValidHash should be the parent of the current payload, as current payload should not have become part of the chain as it has not been fcU-ed yet. But I will validate this.
There was a problem hiding this comment.
@g11tech what I see is go-ethereum returns the current execution payload as latestValidHash most of the time so we should handle it https://github.com/ethereum/go-ethereum/blob/e108d36575fd93e6088259cdabeed6dddf76458c/eth/catalyst/api.go#L324
There was a problem hiding this comment.
modified, just directly passing the parent's exechash now, so no matter what EL responds as LVH (block or its parent), we pass the correct one
g11tech
force-pushed
the
g11tech/latest-valid-hash-update
branch
2 times, most recently
from
1f5bbb5
to
e893be4
Compare
wemeetagain
reviewed
Jul 18, 2022
| latestValidExecHash: parentBlock.executionPayloadBlockHash, | ||
| invalidateTillBlockHash: null, | ||
| }); | ||
| } | ||
| break; // OK | ||
|
|
||
| case ExecutePayloadStatus.INVALID: { |
There was a problem hiding this comment.
should there be executionStatus = ExecutionStatus.Invalid here?
There was a problem hiding this comment.
we are just throwing below after validateLatestHash invalid status propagation, so its not getting used.
There was a problem hiding this comment.
also added some typesafety to it:
- let executionStatus: ExecutionStatus;
+ let executionStatus: MaybeValidExecutionStatus;
|
@g11tech take a look at ethereum/consensus-specs#2954 |
yes, the PR is the result of the discussions we already had on discord interop channel. |
dapplion
reviewed
Aug 1, 2022
| node.executionStatus = ExecutionStatus.Valid; | ||
| break; | ||
|
|
||
| default: |
dapplion
reviewed
Aug 1, 2022
| @@ -482,6 +747,7 @@ export class ProtoArray { | |||
| * head. | |||
| */ | |||
| nodeIsViableForHead(node: ProtoNode): boolean { | |||
| if (node.executionStatus === ExecutionStatus.Invalid) return false; | |||
There was a problem hiding this comment.
Suggested change
| if (node.executionStatus === ExecutionStatus.Invalid) return false; | |
| if (node.executionStatus === ExecutionStatus.Invalid) { | |
| return false; | |
| } | |
dapplion
reviewed
Aug 1, 2022
| // Only invalidate if this is post merge, and either parent is invalid or the | ||
| // concensus has failed | ||
| if (node.executionStatus !== ExecutionStatus.PreMerge && parent?.executionStatus === ExecutionStatus.Invalid) { | ||
| if (node.executionStatus === ExecutionStatus.Valid) { |
There was a problem hiding this comment.
Should this two if statements be merged?
There was a problem hiding this comment.
simplified the conditions, hopefully its more clear now
g11tech
force-pushed
the
g11tech/latest-valid-hash-update
branch
from
f7a1937
to
e40a529
Compare
dapplion
reviewed
Aug 13, 2022
| @@ -17,6 +21,7 @@ export class ProtoArray { | |||
| finalizedRoot: RootHex; | |||
| nodes: ProtoNode[] = []; | |||
| indices = new Map<RootHex, number>(); | |||
| lvhError?: LVHExecError; | |||
There was a problem hiding this comment.
This PR should include the mechanism to kill beacon process from the beacon package. Just check every slot or at some predicatable spot
packages/fork-choice/test/unit/protoArray/executionStatusUpdates.test.ts
Outdated
Show resolved
Hide resolved
dapplion
reviewed
Aug 13, 2022
dapplion
reviewed
Aug 13, 2022
wemeetagain
requested changes
Sep 1, 2022
| private propagateValidExecutionStatusByIndex(validNodeIndex: number): void { | ||
| let nodeIndex: number | undefined = validNodeIndex; | ||
| // propagate till we keep encountering syncing status | ||
| while (nodeIndex !== undefined) { |
There was a problem hiding this comment.
This loop should also stop when the parent is already valid as a cases in addition to going until invalid, premerge, or thru all ancestors.
There was a problem hiding this comment.
updated the loop break condition to !== syncing 👍
* extend forkchoice execution type * handle the invalid status in protoarray * add the pre execution status validation testcase * relocate validateLatestHash to protoarray for better testing * add case for invalidating a branch * add invalidations and validations * add a bit more desc to test case title * fix the invalidate index bug * refac the reverse lookup and throw if forkchoice becomes invalidated * relax the LVH conditions in invalid responses * only invalidate post merge * modify the invalidations to allow for various LVH scenarios * add invalidation case * simplyfy lvh search and be more forgiving when not found * add a fail hard case * correct the lvh passed in valid response * refactor how to extract lvh in valid scenario * replace invalidate all with proper error handling to be more verbose and swallow less * add test for invalid to valid * add type safety to executionStatus * linting if for better visual clarity * fix tests * refac the invalidation condition in propagate invalid lvh * remove empty default clause * simplify segment exec response * error logging * always throw if there is segment error
Co-authored-by: Lion - dapplion <35266934+dapplion@users.noreply.github.com>
Co-authored-by: Lion - dapplion <35266934+dapplion@users.noreply.github.com>
g11tech
force-pushed
the
g11tech/latest-valid-hash-update
branch
from
e4d6d3f
to
6b75d90
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Post merge, engine can give optimistic
syncingstatus on fcU heads which can (should be) eventually resolved intovalid/invalid.This need to be propagated to forkchoice and remove the consideration of invalids and their descendant from the forkchoice,
This PR implements the fcU updates coming from from the execution via latestValidHash
TODO testcases:
for propagate valid
- [ ] verify is_merge_block if the mergeblock has not yet been validatedmoved to a separate issue as doesn't addressing this may be a bit more evolved, though EL already should have run terminal validations and hence de duping work isn't really necessary.for propagate invalid
real life optimistic sync testing against ELs
Closes #3508
Testcases summary:
executionStatus / normal updates ✔ preValidation forkchoice setup should be correct ✔ correcly invalidate 3C and validate 2C only ✔ Validate 3B, 2B, 1A ✔ Invalidate 3A, 2A with 2A loosing its bestChild, bestDescendant executionStatus / invalidate all postmerge chain ✔ preValidation forkchoice setup should be correct ✔ all post merge blocks should be invalidated except Cs ✔ pre merge block should be the FC head executionStatus / poision forkchoice if we invalidate previous valid ✔ preValidation forkchoice setup should be correct ✔ Validate 3B, 2B, 1A ✔ protoarray should be poisioned with a buggy LVH response executionStatus / poision forkchoice if we validate previous invalid ✔ preValidation forkchoice setup should be correct ✔ Inalidate 3B, 2B, 1A ✔ protoarray should be poisioned with a buggy LVH response 13 passing (8ms)