simulators/eth2/engine: Fix SyncingWithChainHavingInvalidPostTransiti…

[zilm13]

I don't understand this test. I have changed it to be valid how I understand it, but maybe I'm not right, let's discuss

• both ELs are mining. I don't understand why second EL is mining, is it for SYNCING response only?
• first fCU call is on terminal block, before optimistic sync breaks in, currently forloop breaks on it with callback, it's too early, optimistic sync is not yet started

I've updated test to do all actions when optimistic sync is definitely started (and it's passing for me with these changes)

[marioevz]

Hi @zilm13, thanks for the changes, let me clarify:

• Yes, if both EL clients have each a different block with the same difficulty, neither of the two will re-org to the other, unless a fcU is received and points to the other. So the clients will remain on SYNCING or ACCEPTED when the newPayload is received.
• Maybe this is a more recent change on lighthouse, but when I coded this test originally, the importer called newPayload first and upon receiving SYNCING it skipped sending the fcU.

I thought the CL would only send the fcU on the terminal block if it was meant to build a payload, and this was the behavior up until a few weeks ago. It doesn't seem obvious to me why would the CL send fcU to a proof-of-work block if this CL is not the one building the transition payload.

Also @pawanjay176 mentioned to me that all optimistic sync tests were failing due to clients banning each other, and suggested a fix of making all peers trusted peers with each other. I'm not sure if this is the same issue, but I could implement it in the following days and see if it fixes this specific test also.

[mkalinin]

May we use EL mock to artificially return SYNCING when needed? I guess Teku is always making an fcU call with terminal block as the head once the block is observed

[zilm13]

It's a legit call by the spec, we shouldn't expect that it's a sign of optimistic sync breaks in

Note: Client software MUST NOT call this function until the transition conditions are met on the PoW network, i.e. there exists a block for which is_valid_terminal_pow_block function returns True.
Note: Client software MUST call this function to initiate the payload build process to produce the merge transition block; the head_block_hash parameter MUST be set to the hash of a terminal PoW block in this case.

So once consensus client observed terminal it could send fCU with it. And it just got this terminal from the EL, and EL answers with INVALID though it provided it itself a second before

[marioevz]

From that fragment I understand that the fcU should only be sent when the validator is proposing the transition block, but I understand that it is not directly prohibited in any other circumstance.

I'm currently revisiting the test and will try out the idea to force the SYNCING response by using the EL mock.

[zilm13]

It's not the MUST, but it's not prohibited, right

[marioevz]

I'm almost done with the changes, but the verification approach is giving me some trouble.

The test now returns SYNCING to every newPayload/forkchoiceUpdated until after we start getting execution_optimistic==true blocks on the chain, then it starts to return INVALID, lvh=0x00...000 for every payload.

The expectation is that, when we check the head of the importer's beacon chain, the head must be the block where the first execution payload was included minus one (the last beacon block without an execution payload).

However this is what I'm getting:

• First execution beacon block: Slot 64
• Builder's Head: Slot 84
• Importer's Head: Slot 72

I'm not really sure what to make out of these values, the importer node does wait SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY before importing these blocks, but the outcome seems weird.

More info to consider:

• Bellatrix happens on slot 64
• Importer's Head returns execution_optimistic==true
• Test waits 1 slot before querying the head of the importer, but the result is exactly the same if we wait 10 slots

cc @zilm13 @mkalinin

[marioevz]

Branch https://github.com/marioevz/hive/tree/fix-opt-sync-test has this test updated now:

hive --client go-ethereum,lighthouse-bn,lighthouse-vc --sim eth2/engine --sim.limit "/syncing-with-chain-having-invalid-transition-block"

[mkalinin]

Do we have any logs from what is happening on CL side? The result is weird, indeed.

[zilm13]

What I see with Teku:
[SYNCING] answer till Epoch 6
next you are sending [INVALID], Teku crashes, because Epoch 4 is optimistically finalized
Looks like test is not working like it should, but Teku is ok :)
Btw why don't you want to override Teku's SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY_CLIENT_OVERRIDE?

[marioevz]

Teku seems to now be passing the test (yay! 🥳), this behavior is only seen in Lighthouse, so I am now more inclined to think this is a client specific behavior rather than a test verification issue.

Hi @paulhauner, the behavior described above is still happening for Lighthouse, and it is only happening when the whole post-transition beacon chain is invalidated (INVALID + lvh==0x00...000).

We are currently spoofing newPayloadV1 and forkchoiceUpdatedV1, first with SYNCING until optimistic sync, then with INVALID, and I was wondering if Lighthouse forkchoice relies on any other call either to the engine_ or the eth_ namespaces that we need to intercept and switch. Thanks!

[marioevz]

What I see with Teku: [SYNCING] answer till Epoch 6 next you are sending [INVALID], Teku crashes, because Epoch 4 is optimistically finalized Looks like test is not working like it should, but Teku is ok :) Btw why don't you want to override Teku's SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY_CLIENT_OVERRIDE?

I reduced SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY_CLIENT_OVERRIDE in all clients to 8, and all optimistic sync tests are passing without issues for teku it seems.

[zilm13]

Yeah, it works for me correctly with "8" override too

[fjl]

Closing because this test was fixed in another way.