Moving requests out of ExecutionPayload #8600
Conversation
…ests from execution payload.
lucassaldanha
changed the title
|
|
||
| public class BeaconBlockBodyElectra extends BeaconBlockBodyAltair { | ||
|
|
||
| @JsonProperty("execution_payload") | ||
| public final ExecutionPayloadElectra executionPayload; | ||
| public final ExecutionPayloadDeneb executionPayload; |
There was a problem hiding this comment.
is it worth a comment here explaining that we use ExecutionPayloadDeneb because no changes have been introduced in Electra? It's is clear now but it might be confusing in the future.
There was a problem hiding this comment.
its really common, i'd prefer no comment as its just noise.
| @@ -88,7 +91,8 @@ class BlindedBeaconBlockBodyElectraImpl | |||
| syncAggregate, | |||
| executionPayloadHeader, | |||
| blsToExecutionChanges, | |||
| blobKzgCommitments); | |||
| blobKzgCommitments, | |||
| executionRequests); | |||
There was a problem hiding this comment.
is this missing its correspondent get method?
There was a problem hiding this comment.
Maybe, but I did the bare minimum on blinded block. This is likely going to be addressed on #8624
lucassaldanha
force-pushed
the
new-requests-2
branch
from
77b8215
to
908f6b7
Compare
| spec.getBlockProcessor(state.getSlot()) | ||
| .processConsolidationRequests(state, consolidationRequests); | ||
| .processConsolidationRequests(state, List.of(consolidationRequest)); |
There was a problem hiding this comment.
it seems like we've hard coded the configuration of needing 1 consolidation request half way through, this could be fun when we need to now handle 2 consolidations, i wonder why we bothered going half way with a list?
If we're hard coding 1 anyway, why do we bother taking a list into the spec function?
There was a problem hiding this comment.
All EL request operation tests in the reference-tests have two parameters: a pre state and a single operation (ssz encoded) as input.
With the new ExecutionRequests container, the alternative would be to update the reference-tests to use that containers ssz as input (instead of a single EL request). This approach is similar to how some operation tests have the ExecutionPayload container ssz as an input. However the current reference-tests are still using the individual operations.
TLDR: reference-tests need to change first, before we can change anything here.
There was a problem hiding this comment.
If we're hard coding 1 anyway, why do we bother taking a list into the spec function?
On the spec, each block can have a list of each EL request type, so the spec function does require a list. It is only on the reference-tests that the input is a single operation.
There was a problem hiding this comment.
I'll follow up with the ref-tests to see if we need to change anything there.
There was a problem hiding this comment.
the list does make sense it just seems weird to have a single up to a point hten a list, so it'd almost be a better idea to have the list all the way through imo so that when we decide 1 consolidation wasnt enough it's less crazy to fix
but either or, just looked weird.
There was a problem hiding this comment.
Yeah, did the change it is better :)
PR Description
withdrawal_requests,deposit_requestsandconsolidation_requestsfrom Execution Payload (execution payload is exactly like Deneb, removed all Electra classes for it);execution_requestsinto Beacon Block Body (Electra)Also added some TODOs that are going to be addressed on:
Reference spec PR: https://github.com/ethereum/consensus-specs/pull/3875/files
Fixed Issue(s)
fixes #8593
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog