Make explicit that the child of LVH is INVALID

[potuz]

Make explicit that when returning INVALID from payload validation, the child of the latestValidHash in the invalid payload branch, is indeed INVALID, allowing thus the consumer to prune any block in this branch starting from said child.

[mkalinin]

Suggested change

	* If validation fails, the response **MUST** contain `{status: INVALID, latestValidHash: validHash}` where `validHash` is either `null`, or the block hash of the unique ancestor of the invalid payload satisfying the following two conditions:
	* If validation fails, the response **MUST** contain `{status: INVALID, latestValidHash: validHash}` where `validHash` is either `0x`, or the block hash of the unique ancestor of the invalid payload satisfying the following two conditions:

I suggest we confirm that this entire clarification goes into the spec, I just want to double check with teams on ACD. And then decide how to encode null -- either option is fine with me

[MicahZoltu]

Since we don't really care about compression, I would prefer a pre-defined string (defined in the spec) that describes the problem. While this won't change anything functionally, it makes it easier to understand what is happening for someone who may not be intimately familiar with the specification.

For example, 'syncing' or something for latestValidHash (instead of null).

[mkalinin]

The problem with 'syncing' is that latestValidHash field is of DATA type and it either should be a valid DATA value or null.

[yperbasis]

Please encode null as JSON null :)

[MicahZoltu]

Hmm, Is the issue that not all languages support union types?

[mkalinin]

This PR doesn't introduce new changes but rather clarifies the spec. The following two cases make an illustration to this PR:

Case 1

VALID_P0 <- VALID_P1 <- INVALID_P2 <- INVALID_P3 <- INVALID_P4

# Incorrect response A. LVH must always be parent of INVALID block
newPayload(INVALID_P4) ~> {status: INVALID, latestValidHash: VALID_P0.blockHash}
# Incorrect response B. LVH must always be VALID
newPayload(INVALID_P4) ~> {status: INVALID, latestValidHash: INVALID_P2.blockHash}

# Correct response
newPayload(INVALID_P4) ~> {status: INVALID, latestValidHash: VALID_P1.blockHash}

Case 2

VALID_P0 <- ACCEPTED_P1 <- INVALID_P2 <- INVALID_P3 <- INVALID_P4

# Incorrect response A. LVH must always be fully validated
newPayload(INVALID_P4) ~> {status: INVALID, latestValidHash: ACCEPTED_P1.blockHash}
# Incorrect response B.  LVH must always be parent of INVALID block
newPayload(INVALID_P4) ~> {status: INVALID, latestValidHash: VALID_P0.blockHash}

# Correct response. LVH must be null if parent of the INVALID block isn't fully validated and deemed VALID
newPayload(INVALID_P4) ~> {status: INVALID, latestValidHash: null}

[marioevz]

Regarding the tests, if the test provides all the information to the client enough to invalidate the chain up to the latest valid hash, will it be acceptable that the execution client still for some reason decides to return null ?

I.e.
The test would pass if the client returns

• {status: INVALID, latestValidHash: <Correct latest valid hash>}
• {status: INVALID, latestValidHash: null}

The test would fail if the client returns:

• {status: INVALID, latestValidHash: <Any other hash>}
• {status: VALID, latestValidHash: null}

Tests currently only pass with the first bullet point.

Also, if the client always keeps returning SYNCING will that still be a failure? -- Currently the test times out and it is indeed a failure.

[potuz]

Returning SYNCING when the client has all the information to invalidate the chain is the worst possible outcome (other than returning VALID that is) as it would induce the client to continue building on that chain. A timeout would be a better reply.

I would fail the test if the client has all the information to comply and returns null, but I'm aware this may cause disagreement

[mkalinin]

as it would induce the client to continue building on that chain.

How so? The node will become optimistic in this case which means do not attest and propose.

[mkalinin]

Regarding the tests, if the test provides all the information to the client enough to invalidate the chain up to the latest valid hash, will it be acceptable that the execution client still for some reason decides to return null ?

It's not acceptable in this case. The client must return correct LVH

[mkalinin]

is either null, or the block hash of the unique ancestor of the invalid payload satisfying the following two conditions

I read it as EL may decide to return null without evaluating the conditions. It should say, it MUST NOT return null if there is a block satisfying the following two conditions in addition to what we already have

[potuz]

I think that rules out null then because there is always such a block. Perhaps rephrasing along the lines of "where lvh is the hash... Or, the client MAY return null if it can ascertain the lvh in its current status"

[mkalinin]

This block is not always fully validated

[potuz]

as it would induce the client to continue building on that chain.

How so? The node will become optimistic in this case which means do not attest and propose.

Yes but will continue accepting blocks in that chain and keep it viable as head, thus remaining offline

[MicahZoltu]

As written, isn't valid JSON, needs quotes.

Suggested change

	* If validation fails, the response **MUST** contain `{status: INVALID, latestValidHash: validHash}` where `validHash` is the block hash of the most recent *valid* ancestor of the invalid payload. That is, the valid ancestor of the payload with the highest `blockNumber`
	* If validation fails, the response **MUST** contain `{status: INVALID, latestValidHash: validHash}` where `validHash` is either `null`, or the block hash of the unique ancestor of the invalid payload satisfying the following two conditions:
	* If validation fails, the response **MUST** contain `{"status": "INVALID", "latestValidHash": validHash}` where `validHash` is either `null`, or the block hash of the unique ancestor of the invalid payload satisfying the following two conditions:

[yperbasis]

Suggested change

	if such a block exists and is a PoS block. `validHash` is `0x0000000000000000000000000000000000000000000000000000000000000000` if such a block exists and it is a PoW block, and `null` otherwise.
	if such block is a PoS block. `validHash` is `0x0000000000000000000000000000000000000000000000000000000000000000` if such block is a PoW block. If the Execution Layer cannot determine such block precisely, it may return `null` instead of the block hash.

[mkalinin]

What about something like the following:

• If validation fails, the response MUST contain {status: INVALID, latestValidHash: validHash} where validHash MUST be:
  • The block hash of ancestor of the invalid payload if the following two conditions are satisfied:
    • it is fully validated and deemed VALID
    • any other ancestor of the invalid payload with a higher blockNumber is INVALID
  • 0x0000000000000000000000000000000000000000000000000000000000000000 if the above conditions are satisfied by a PoW block
  • null if client software can't determine ancestor of the invalid payload satisfying the above conditions

[djrtwo]

that looks good to me

[potuz]

pushed this, please let me know if this doesn't work.