implement v1.2.0 optimistic sync tests #4174
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1898,6 +1898,45 @@ proc updateHead*( | |
| dag.finalizedHead.blck.root, stateRoot, dag.finalizedHead.slot.epoch) | ||
| dag.onFinHappened(dag, data) | ||
|
|
||
| proc getEarliestInvalidBlockRoot*( | ||
| dag: ChainDAGRef, initialSearchRoot: Eth2Digest, | ||
| latestValidHash: Eth2Digest, defaultEarliestInvalidBlockRoot: Eth2Digest): | ||
| Eth2Digest = | ||
| # Earliest within a chain/fork in question, per LVH definition. Intended to | ||
| # be called with `initialRoot` as the parent of the block regarding which a | ||
| # newPayload or forkchoiceUpdated execution_status has been received as the | ||
| # tests effectively require being able to access this before the BlockRef's | ||
| # made. Therefore, to accommodate the EF consensus spec sync tests, and the | ||
| # possibilities that the LVH might be an immediate parent or a more distant | ||
| # ancestor special-case handling of an earliest invalid root as potentially | ||
| # not being from this function's search, but being provided as a default by | ||
| # the caller with access to the block. | ||
| var curBlck = dag.getBlockRef(initialSearchRoot).valueOr: | ||
| # Being asked to traverse a chain which the DAG doesn't know about -- but | ||
| # that'd imply the block's otherwise invalid for CL as well as EL. | ||
| return static(default(Eth2Digest)) | ||
|
|
||
| # Only allow this special case outside loop; it's when the LVH is the direct | ||
| # parent of the reported invalid block | ||
| if curBlck.executionBlockRoot.isSome and | ||
| curBlck.executionBlockRoot.get == latestValidHash: | ||
| return defaultEarliestInvalidBlockRoot | ||
|
|
||
| while true: | ||
| # This was supposed to have been either caught by the pre-loop check or the | ||
| # parent check. | ||
| if curBlck.executionBlockRoot.isSome and | ||
| curBlck.executionBlockRoot.get == latestValidHash: | ||
| doAssert false, "getEarliestInvalidBlockRoot: unexpected LVH in loop body" | ||
|
|
||
| if (curBlck.parent.isNil) or | ||
| curBlck.parent.executionBlockRoot.get(latestValidHash) == | ||
| latestValidHash: | ||
| break | ||
| curBlck = curBlck.parent | ||
|
|
||
| curBlck.root | ||
|
There was a problem hiding this comment. This is not necessarily correct. When someone switches from a different CL to Nimbus but retains their EL, the EL's If There was a problem hiding this comment. Also, in extended periods of non-finality, the linear scan may become expensive if performed repeatedly. There was a problem hiding this comment. In theory, latest-known In particular, https://github.com/ethereum/execution-apis/blob/main/src/engine/specification.md#payloadstatusv1 defines:
This also addresses the concern about the EL's As far as cost in general, this should be a relatively one-time thing per invalid branch -- once invalidated, it shouldn't be doing that backwards search again, so potentially better to do it once than multiple times, along the lines of how pruning huge swaths of finalized blocks after a long-unfinalizing network finalizes is expensive, but one-time. Doing halfway versions introduces less-well-defined state and doesn't necessarily save time overall, in a throughput sense. There is an issue here where the quarantine unviables are sometimes checked only as direct parents rather than ancestors, which is an argument for not using the LVH-based search for adding to quarantine. The other aspect is that it should rediscover all of this by just asking the EL, if it misses something. So everything here should be considered a cache. For the moment, this is all only used in tests, so another approach is to move this out from While the initial intent, and still one that seems broadly ideal, is to maximally match test behavior with non-test behavior (otherwise, what is the test testing?), just the status quo here is that this LVH infrastructure is test-only, so it can/should reflect that. |
||
|
|
||
| proc isInitialized*(T: type ChainDAGRef, db: BeaconChainDB): Result[void, cstring] = | ||
| # Lightweight check to see if we have the minimal information needed to | ||
| # load up a database - we don't check head here - if something is wrong with | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -442,6 +442,7 @@ func mark_root_invalid*(self: var ForkChoice, root: Eth2Digest) = | |
| self.backend.proto_array.nodes.offset | ||
| if nodePhysicalIdx < self.backend.proto_array.nodes.buf.len: | ||
| self.backend.proto_array.nodes.buf[nodePhysicalIdx].invalid = true | ||
| self.backend.proto_array.propagateInvalidity(nodePhysicalIdx) | ||
|
There was a problem hiding this comment. Shouldn't this already be covered for purpose of scoring / selecting heads? There was a problem hiding this comment. The updating is done in nimbus-eth2/beacon_chain/fork_choice/proto_array.nim Lines 424 to 451 in ad286b9 Which is called by nimbus-eth2/beacon_chain/fork_choice/proto_array.nim Lines 270 to 282 in ad286b9 Which is correct for the scores as such -- it allows quick updates. However, invalidity propagates the other direction, from ancestors to descendants, unlike the scores, and there's no existing mechanism in fork choice to propagate information from ancestors to descendants. |
||
| # Best-effort; attempts to mark unknown roots invalid harmlessly ignored | ||
| except KeyError: | ||
| discard | ||
|
|
||
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.
Hmm, would it be correct to return
defaultEarliestInvalidRoothere as well?If yes, the return type could be changed to
Opt[Eth2Digest], and the caller could then dodag.getEarliestInvalidRoot(initialSearchRoot, lvh).get(defaultEarliestInvalidRoot). This would avoid polluting this function withdefaultEarliestInvalidRoot.