engine: skip fcU only when the ancestor of the canonical head is VALID

[mkalinin]

Clarifies EL’s behaviour in the following case mentioned by @potuz:

1. NewPayload 6 -> SYNCING
2. FCU 6 -> INVALID, LVH:5
   Some CL MAY send FCU: 5 in this case. Edit: Prysm will send here in most cases too

EL must update its forkchoice state when FCU: 5 is sent.

The skip was originally introduced to handle a scenario when CL is syncing from scratch while EL was fully synced up to some moment in the past. This fix is related to the online mode when a node is fully sync and doesn’t affect the original scenario.

cc @marioevz it would be great to have a test covering this scenario some time after the PR gets merged

[mkalinin]

After the Discord conversation with @lightclient we figured out that the scenario provided in the PR description is handled properly dues to point (4) in the fcU spec, i.e.:

1. NewPayload 6 -> SYNCING
2. FCU 6 -> INVALID, LVH:5
   Point (4) of the fcU spec ensures that no update happens as the head isn’t VALID
   Thus FCU 5 will either be the current head or its descendant which doesn’t fall into the skip case anyhow

Therefore, this PR has an information flavour rather than being a fix into the spec. Although, I still think that this scenario worth being implemented in Hive, perhaps with a lower priority.

[spencer-tb]

This scenario seems to be covered within the invalid_payload.go tests parameterized here: https://github.com/ethereum/hive/blob/master/simulators/ethereum/engine/suites/engine/invalid_payload.go#L66-L307

[mkalinin]

This scenario seems to be covered within the invalid_payload.go tests parameterized here: https://github.com/ethereum/hive/blob/master/simulators/ethereum/engine/suites/engine/invalid_payload.go#L66-L307

This is great, thank you! So EL clients already maintain the expected behavior. @lightclient do you have any opposition to get this PR merged?