Skip to content

Make explicit that the child of LVH is INVALID #254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 14, 2022
Merged

Make explicit that the child of LVH is INVALID #254

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0799788
Make explicit that the child of LVH is INVALID
potuz Jul 4, 2022
7b0ac66
Update src/engine/specification.md
potuz Jul 7, 2022
7b35853
add null option for validhash
potuz Jul 7, 2022
0cbe394
specify that null is only allowed when the block does not exist
potuz Jul 9, 2022
5a6b5e8
Andrew+Misha's comment
potuz Jul 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/engine/specification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,9 @@ Payload validation process consists of validating a payload with respect to the

3. Client software **MUST** validate a payload according to the block header and execution environment rule set with modifications to these rule sets defined in the [Block Validity](https://eips.ethereum.org/EIPS/eip-3675#block-validity) section of [EIP-3675](https://eips.ethereum.org/EIPS/eip-3675#specification):
* If validation succeeds, the response **MUST** contain `{status: VALID, latestValidHash: payload.blockHash}`
* 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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@MicahZoltu MicahZoltu Jul 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please encode null as JSON null :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block is not always fully validated

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

- It is fully validated and deemed `VALID`
- Any other ancestor of the invalid payload with a higher `blockNumber` is `INVALID`
* If the most recent valid ancestor is a PoW block, `latestValidHash` **MUST** be set to `0x0000000000000000000000000000000000000000000000000000000000000000`
* Client software **MUST NOT** surface an `INVALID` payload over any API endpoint and p2p interface.

Expand Down