-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 payload bodies rpc endpoints #6644
Conversation
00d05f0
to
624cd5c
Compare
ef541bd
to
25a7201
Compare
cmd/rpcdaemon/commands/engine_api.go
Outdated
} | ||
|
||
type BeaconBlocksByRangeRequest struct { | ||
StartSlot uint64 `json:"start_slot" gencodec:"required"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slot numbers in CL are different from block numbers in the EL. This field refers to the block rather than the slot number (see Item 6 of the spec). Please rename to StartBlockNumber
or simply Start
. And it's "start" in json per the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where I got the name for the json from, it mentions not to confuse this with the block number which we're OK on. I think we need to keep the start_slot
, but can rename the field name no problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The p2p spec is different from the Engine API spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out we don't need this type at all anyway, the last comment here negates the need for it
|
||
var i uint64 | ||
for i = 0; i < request.Count; i++ { | ||
block, err := rawdb.ReadBlockByNumber(tx, request.Start+i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing null blocks (blocks after the latest available) should be truncated (see Item 3 of the spec). In our database we don't allow any gaps in canonical blocks, so it's OK to truncate starting from the first missing block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK good to know. There were some comments on this when I initially looked at the PR, looks like that has been finalised now so will update our code.
cmd/rpcdaemon/commands/engine_api.go
Outdated
@@ -78,6 +88,8 @@ type EngineAPI interface { | |||
GetPayloadV1(ctx context.Context, payloadID hexutil.Bytes) (*ExecutionPayload, error) | |||
GetPayloadV2(ctx context.Context, payloadID hexutil.Bytes) (*GetPayloadV2Response, error) | |||
ExchangeTransitionConfigurationV1(ctx context.Context, transitionConfiguration *TransitionConfiguration) (*TransitionConfiguration, error) | |||
GetPayloadBodiesByHashV1(ctx context.Context, hashes []libcommon.Hash) ([]*ExecutionPayloadBodyV1, error) | |||
GetPayloadBodiesByRangeV1(ctx context.Context, payload BeaconBlocksByRangeRequest) ([]*ExecutionPayloadBodyV1, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should have 2 parameters (start
, count
) per the spec, not one, similar to how ForkchoiceUpdatedV1
has 2 parameters.
25a7201
to
e00409d
Compare
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.