Skip to content
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

Synchronously check all transactions to have non-zero length #573

Merged
merged 3 commits into from
Dec 5, 2024
Merged

Synchronously check all transactions to have non-zero length #573

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15bc86e
Synchronously check all `transactions` to have non-zero length
etan-status Aug 14, 2024
180eb4c
Allow `INVALID` on invalid transaction.
etan-status Aug 14, 2024
91069c2
Remove redundant validity check
etan-status Dec 4, 2024
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
16 changes: 10 additions & 6 deletions src/engine/paris.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,26 +161,30 @@ The payload build process is specified as follows:

#### Specification

1. Client software **MUST** validate `blockHash` value as being equivalent to `Keccak256(RLP(ExecutionBlockHeader))`, where `ExecutionBlockHeader` is the execution layer block header (the former PoW block header structure). Fields of this object are set to the corresponding payload values and constant values according to the Block structure section of [EIP-3675](https://eips.ethereum.org/EIPS/eip-3675#block-structure), extended with the corresponding section of [EIP-4399](https://eips.ethereum.org/EIPS/eip-4399#block-structure). Client software **MUST** run this validation in all cases even if this branch or any other branches of the block tree are in an active sync process.
1. Client software **MUST** validate that all `transactions` have non-zero length (at least 1 byte). Client software **MUST** run this validation in all cases even if this branch or any other branches of the block tree are in an active sync process. Client software **MAY** employ stricter checks and validate that all `transactions` are fully valid.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why MAY in the second case? Can we change it to MUST as likely ELs run this validation either before or during the block hash computations, or not?

Suggested change
1. Client software **MUST** validate that all `transactions` have non-zero length (at least 1 byte). Client software **MUST** run this validation in all cases even if this branch or any other branches of the block tree are in an active sync process. Client software **MAY** employ stricter checks and validate that all `transactions` are fully valid.
1. Client software **MUST** validate that all `transactions` are encoded correctly and have non-zero length (at least 1 byte). Client software **MUST** run this validation in all cases even if this branch or any other branches of the block tree are in an active sync process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the purpose of computing a unique block hash that's not affected by hash collisions, it's fine if a transaction is malformed as long as it is not empty.

MAY wording enables CL implementations to also perform the checks in situation where the EL is unavailable (maintenance etc); making this a MUST requires knowledge of individual transaction types.

Copy link
Collaborator

Choose a reason for hiding this comment

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

MAY introduces ambiguity as transactions encoding may be run synchronously or may be not depending on the state of the node and EL client implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same ambuity that's already present with steps (4) and (5). But arguably, the stricter transaction validation check is already covered by those. As in, if a transaction doesn't parse, it will fail in step (4). And step (5) already allows deferring validation until fcU. Will remove it from here. Only the empty-check has to be done always and must be synchronous, as in, there has to be an INVALID on newPayload if a tx is empty, so that there is no chance that allows the CL to optimistically import it and request an ambiguous fcu lateron.

  1. Client software MUST validate the payload if it extends the canonical chain and requisite data for the validation is locally available. The validation process is specified in the Payload validation section.

  2. Client software MAY NOT validate the payload if the payload doesn't belong to the canonical chain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This “ambiguity" is necessary to handle different EL client designs


2. Client software **MAY** initiate a sync process if requisite data for payload validation is missing. Sync process is specified in the [Sync](#sync) section.
2. Client software **MUST** validate `blockHash` value as being equivalent to `Keccak256(RLP(ExecutionBlockHeader))`, where `ExecutionBlockHeader` is the execution layer block header (the former PoW block header structure). Fields of this object are set to the corresponding payload values and constant values according to the Block structure section of [EIP-3675](https://eips.ethereum.org/EIPS/eip-3675#block-structure), extended with the corresponding section of [EIP-4399](https://eips.ethereum.org/EIPS/eip-4399#block-structure). Client software **MUST** run this validation in all cases even if this branch or any other branches of the block tree are in an active sync process.

3. Client software **MUST** validate the payload if it extends the canonical chain and requisite data for the validation is locally available. The validation process is specified in the [Payload validation](#payload-validation) section.
3. Client software **MAY** initiate a sync process if requisite data for payload validation is missing. Sync process is specified in the [Sync](#sync) section.

4. Client software **MAY NOT** validate the payload if the payload doesn't belong to the canonical chain.
4. Client software **MUST** validate the payload if it extends the canonical chain and requisite data for the validation is locally available. The validation process is specified in the [Payload validation](#payload-validation) section.

5. Client software **MUST** respond to this method call in the following way:
5. Client software **MAY NOT** validate the payload if the payload doesn't belong to the canonical chain.

6. Client software **MUST** respond to this method call in the following way:
* `{status: INVALID, latestValidHash: null, validationError: errorMessage | null}` if `transactions` contains zero length or invalid entries
* `{status: INVALID_BLOCK_HASH, latestValidHash: null, validationError: errorMessage | null}` if the `blockHash` validation has failed
* `{status: INVALID, latestValidHash: 0x0000000000000000000000000000000000000000000000000000000000000000, validationError: errorMessage | null}` if terminal block conditions are not satisfied
* `{status: SYNCING, latestValidHash: null, validationError: null}` if requisite data for the payload's acceptance or validation is missing
* with the payload status obtained from the [Payload validation](#payload-validation) process if the payload has been fully validated while processing the call
* `{status: ACCEPTED, latestValidHash: null, validationError: null}` if the following conditions are met:
- all `transactions` have non-zero length
mkalinin marked this conversation as resolved.
Show resolved Hide resolved
- the `blockHash` of the payload is valid
- the payload doesn't extend the canonical chain
- the payload hasn't been fully validated
- ancestors of a payload are known and comprise a well-formed chain.

6. If any of the above fails due to errors unrelated to the normal processing flow of the method, client software **MUST** respond with an error object.
7. If any of the above fails due to errors unrelated to the normal processing flow of the method, client software **MUST** respond with an error object.

### engine_forkchoiceUpdatedV1

Expand Down