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.

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

Engine API: define payload bodies requests #352

Merged
merged 9 commits into from
Jan 20, 2023

Conversation

mkalinin
Copy link
Collaborator

The steal of #218 (I hope @arnetheduck doesn't mind) as we need this change in Shanghai ASAP.

Differences from the original PR:

  • withdrawals are added to ExecutionBlockBodyV1 struct
  • engine_getPayloadBodiesByRangeV1 isn't optional and must be supported by all EL clients
  • Request timeouts are set to 10s which is the RESP_TIMEOUT param value from the p2p-interface spec
  • nil is replaced by null to follow JSON format

src/engine/shanghai.md Outdated Show resolved Hide resolved
src/engine/shanghai.md Outdated Show resolved Hide resolved
Copy link
Contributor

@arnetheduck arnetheduck left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the rebase!

src/engine/shanghai.md Outdated Show resolved Hide resolved
Co-authored-by: Jacek Sieka <arnetheduck@gmail.com>
@mkalinin
Copy link
Collaborator Author

I am wondering how we can make these methods future compatible with ExecutionPayloadBodyV2 and other more advanced versions:

  • either return null when V1 method faces blocks requiring V2 version
  • or make V2 method V1 aware, i.e. mix V1 | V2 struct versions in the response

1. Client software **MUST** support `count` values of at least 32 blocks.

1. Client software **MUST** place `null` in the response array for any blocks that have been pruned or where the request extends past the current latest known block.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be determinable by the number of responses? Is there ever a case where there would be a gap? Right now, I expect every request over a boundary to be like:

->  engine_getPayloadBodiesByRangeV1(100, 5)
<- [block_101, block_102, null, null, null]

Basically I think the trailing nulls are redundant info.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. @arnetheduck why would CL need trailing nulls?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it matters greatly either way - we have to consider that a syncing client cannot reliably tell if it should be a null or not past its own head, so we can just pick one "style" and go with it.

Arguably, not sending "null" carries the "I don't know" distinction vs "I know there's nothing past that block" as happens with a historical request over a boundary where the EL "knows" there are blocks after the gap, so it might indeed be better to not include the null for any request past head.

@arnetheduck
Copy link
Contributor

future compatible

I'm not convinced there's much to do on this front - ie a client that doesn't understand v2 will not be able to do anything (significantly) better - ie maybe show an informational log to the user that the fork that they're on no longer is supported by the connected EL.. a v2 client on the other hand will already know what to do.

mkalinin and others added 2 commits January 13, 2023 13:10
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
@mkalinin
Copy link
Collaborator Author

I have added a requirement to set withdrawals to null if pre-Shanghai bodies are returned which explicitly makes the new structure backwards compatible.

src/engine/shanghai.md Outdated Show resolved Hide resolved
src/engine/shanghai.md Outdated Show resolved Hide resolved
@mkalinin mkalinin merged commit 8058687 into ethereum:main Jan 20, 2023
hexoscott added a commit to ledgerwatch/erigon that referenced this pull request Jan 24, 2023
Very basic implementation for get payload bodies rpc calls. Once we have
Hive tests for these calls I can pick this back up and work through any
issues.

Implementation of ethereum/execution-apis#352.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants