Introduce an ExecutionPayloadEnvelope to abstract away the fields needed in the EL but not the CL
potuz opened this issue · 20 comments
Up until Electra, the CL clients would not save the full execution payload as this is not needed on the beacon state transition function. However, Electra includes new fields like withdrawal requests, deposit requests and consolidation requests that the consensus client requires in order to perform the state transition function. With the current design the consensus client would need to either
- save locally the requests or
- requests the payload from the EL for each time they want to execute a state transition
I propose to move the BeaconBlockBody to have the following structure on the CL
class BeaconBlockBody(Container):
randao_reveal: BLSSignature
...
execution_payload_envelope ExecutionPayloadEnvelope
...
class ExecutionPayloadEnvelope(Container)
deposit_requests: List[DepositRequest, MAX_DEPOSIT_REQUESTS_PER_PAYLOAD] # [New in Electra:EIP6110]
withdrawal_requests: List[WithdrawalRequest, MAX_WITHDRAWAL_REQUESTS_PER_PAYLOAD]
consolidation_requests: List[ConsolidationRequest, MAX_CONSOLIDATION_REQUESTS_PER_PAYLOAD]
execution_payload: ExecutionPayload
class ExecutionPayload(Container):
parent_hash: Hash32
...This way the CL can simply replace execution_payload by it's hash tree root and keep in DB a binded_execution_payload_envelope instead of the full envelope and still be able to perform the state transition function.
The exchange with the EL over the Engine API would also reflect this structure. The EL would need to take the requests from the envelope, add them to the ExecutionPayload object in order to compute the block hash.
Makes sense to me! (with execution_payload moved to the beginning so that it does not get re-indexed as new requests types get added).
The actual DB optimization could also be achieved by replacing the transactions list with its root. Feels more janky though.
How much of this is required for ePBS?
in BeaconState, we only need the execution_payload block_hash?
How much of this is required for ePBS?
In ePBS we already have an envelope (and it's even signed) for a different reason, the payload reveal stage also sends information that is needed for the CL: https://github.com/ethereum/consensus-specs/blob/b082140312aad8248ffb7dafe1bf9c5d0c66c221/specs/_features/eip7732/beacon-chain.md#executionpayloadenvelope
The actual DB optimization could also be achieved by replacing the transactions list with its root. Feels more janky though.
This may require adding a new payload type, which would be nice to avoid.
Alternatively, the structures can be as the following:
class BeaconBlockBody(Container):
randao_reveal: BLSSignature
...
execution_payload: ExecutionPayload
bls_to_execution_changes: List[SignedBLSToExecutionChange, MAX_BLS_TO_EXECUTION_CHANGES]
blob_kzg_commitments: List[KZGCommitment, MAX_BLOB_COMMITMENTS_PER_BLOCK]
deposit_requests: List[DepositRequest, MAX_DEPOSIT_REQUESTS_PER_PAYLOAD] # [New in Electra:EIP6110]
withdrawal_requests: List[WithdrawalRequest, MAX_WITHDRAWAL_REQUESTS_PER_PAYLOAD] # [New in Electra:EIP7002]
consolidation_requests: List[ConsolidationRequest, MAX_CONSOLIDATION_REQUESTS_PER_PAYLOAD] # [New in Electra:EIP7251]In this case new fields are appended to the end instead of switching the type of the existing execution_payload field. In this case it won’t affect gindex of the payload (at least by Electra).
In this case Engine API has two options assuming all requests are put into a single list: ExecutionPayloadEnvelope or pass requests as param to newPayloadV4 and include requests into response of the getPayloadV4.
I am not sure if this would be future compatible with ePBS and if we even want to keep such compatibility in mind (sorry @potuz).
Alternatively, the structures can be as the following:
class BeaconBlockBody(Container):
I know the builder API shouldn't affect any decision-making on this, but one thing to note is that if we do this (which I don't see any blockers for), we will have to add all the request types to the builder's bid. I'm not against this but wanted to point it out.
Alternatively, the structures can be as the following:
class BeaconBlockBody(Container):I know the builder API shouldn't affect any decision-making on this, but one thing to note is that if we do this (which I don't see any blockers for), we will have to add all the request types to the builder's bid. I'm not against this but wanted to point it out.
AFAIU, this would require an update to the BuilderBid and BlindedBeaconBlockBody structures in the API:
class BuilderBid(Container):
header: ExecutionPayloadHeader
blob_kzg_commitments: List[KZGCommitment, MAX_BLOB_COMMITMENTS_PER_BLOCK]
deposit_requests: List[DepositRequest, MAX_DEPOSIT_REQUESTS_PER_PAYLOAD] # [New in Electra]
withdrawal_requests: List[WithdrawalRequest, MAX_WITHDRAWAL_REQUESTS_PER_PAYLOAD] # [New in Electra]
consolidation_requests: List[ConsolidationRequest, MAX_CONSOLIDATION_REQUESTS_PER_PAYLOAD] # [New in Electra]
value: uint256
pubkey: BLSPubkey
class BlindedBeaconBlockBody(Container):
randao_reveal: BLSSignature
eth1_data: Eth1Data
graffiti: Bytes32
proposer_slashings: List[ProposerSlashing, MAX_PROPOSER_SLASHINGS]
attester_slashings: List[AttesterSlashing, MAX_ATTESTER_SLASHINGS]
attestations: List[Attestation, MAX_ATTESTATIONS]
deposits: List[Deposit, MAX_DEPOSITS]
voluntary_exits: List[SignedVoluntaryExit, MAX_VOLUNTARY_EXITS]
sync_aggregate: SyncAggregate
execution_payload_header: ExecutionPayloadHeader
bls_to_execution_changes: List[SignedBLSToExecutionChange, MAX_BLS_TO_EXECUTION_CHANGES]
blob_kzg_commitments: List[KZGCommitment, MAX_BLOB_COMMITMENTS_PER_BLOCK]
deposit_requests: List[DepositRequest, MAX_DEPOSIT_REQUESTS_PER_PAYLOAD] # [New in Electra]
withdrawal_requests: List[WithdrawalRequest, MAX_WITHDRAWAL_REQUESTS_PER_PAYLOAD] # [New in Electra]
consolidation_requests: List[ConsolidationRequest, MAX_CONSOLIDATION_REQUESTS_PER_PAYLOAD] # [New in Electra]The above changes look neat to me. Am I missing anything or the changes could be simpler with the ExecutionPayloadEnvelope to your view?
@mkalinin the changes you propose look fine to me.
I am not sure if this would be future compatible with ePBS and if we even want to keep such compatibility in mind (sorry @potuz).
It's not compatible with EIP-7732, but anyway that EIP will change the block type and it would necessarily introduce a signed envelope and change the execution payload type so it doesn't really make it worse. However, if we did change to an envelope today we avoid changing the beacon block body type in the future for any new requests that appear in the Payload, with or without ePBS. I prefer the envelope design as it's more robust and encapsulated, instead of leaking data that is introduced from the EL directly in the CL structure, it stays within the envelope.
My only argument that is not in favour of the envelope approach is that it changes gindex of the payload which may affect some consumers while appending requests to the end does not. Once we have stable container such changes should become easier to apply. Other than that, the envelope approach looks good to me.
I don’t get the argument of leaking data because those requests are CL operations emitted by EL and since we move them out of the ExecutionPayload the place where they sit shouldn’t be necessarily tight to the payload, IMO. Each of those request type have a separate handler in the block processing routine and their processing does not depend on the payload. The only dependency is to check whether the given requests are the ones that the EL block is committed to which can be handled on the Engine API level. But I do admit that this will change with ePBS as there are more things introduced that are related to the payload but aren't part of it.
One thing to keep in mind is that if EIP-7688 SSZ StableContainer gets adopted, generalized indices will get reindexed anyway, so we will have an opportunity to optimize the affected data structures in a subsequent Pectra devnet.
Should we group the various validator request fields within a sub-Container to match the EL design with a requests_root more closely, and also to be able to group the various requests if there actually is a new one at some future time? see ethereum/execution-apis#565 (comment)
e.g.
class ValidatorRequests(Conainer):
deposit_requests: List[DepositRequest, MAX_DEPOSIT_REQUESTS_PER_PAYLOAD] # [New in Electra:EIP6110]
withdrawal_requests: List[WithdrawalRequest, MAX_WITHDRAWAL_REQUESTS_PER_PAYLOAD]
consolidation_requests: List[ConsolidationRequest, MAX_CONSOLIDATION_REQUESTS_PER_PAYLOAD]
class ExecutionPayloadEnvelope(Container)
execution_payload: ExecutionPayload
requests: ValidatorRequestsI am generally fine with any/either of changes proposed but everything EL responded in envelop sounds good to me for the fact that envelop will be modified in epbs as @potuz mentioned.
it would be nice to also have requests abstracted away in requests field as @etan-status mentioned
I agree with @g11tech , generally fine with the envelop approach and that it would be modified, and requests field described by @etan-status seems logical.
+1 for Etan's subtree... Now we need volunteers to write the PR :)
@potuz I started a PR here: #3875. Still WIP as I review some interactions. Hopefully it has everything you and @etan-status suggested!
One thing I am cautious about is the process_withdrawals function. One of its parameters is the execution_payload, and we get the withdrawals from it to compare with expected_withdrawals (read from the state).
In your example where the CL replaces execution_payload by it's hash tree root (and keep a binded_execution_payload_envelope in DB), does it impact this at all? Sorry if this is a silly question but I am not super familiar with the blinded block flow/spec.
I am curious because in Teku, we did not implement a element-to-element comparison to check the expected withdrawals, we actually compare the withdrawals_root from the execution_payload_header (that we can get from a blinded block body anyway).
TLDR: should we consider updating process_withdrawals to compare the hash root of the expected list of withdrawals with the withdrawal_root from the execution_payload_header in the spec?
Thanks @lucassaldanha for the change, it will not be an easy PR, fixing the pyspec tests will be really painful I suspect. I'll review the PR when it's ready (left only a comment now). For process_withdrawals there won't be a problem. you can keep it exactly as is in the spec, logic will be the same, you take the execution_payload_envelope as parameter instead of an execution_payload (you can leave it exactly as-is if you want without touching it and still passing an execution_payload if you want, as requests are not processed there). Clients that are syncing a block over the wire have the full payload_envelope so no problem there, clients that are executing an old state transition and only have a blinded_envelope are fine because they do not need the actual withdrawals, they already have verified that the withdrawals in the payload match the ones in the beacon state, they can skip this check and use the withdrawals from the state. We already do this in Prysm and I suspect every client does this as well.
I am curious because in Teku, we did not implement a element-to-element comparison to check the expected withdrawals, we actually compare the
withdrawals_rootfrom theexecution_payload_header(that we can get from a blinded block body anyway).TLDR: should we consider updating
process_withdrawalsto compare the hash root of the expected list of withdrawals with thewithdrawal_rootfrom theexecution_payload_headerin the spec?
Comparing root to root is just faster, we don't need to compare element by element. It's validating the same thing without doing any work... Basically we've loaded the list into our structures, got the resulting root and compared that.
I really like @etan-status idea on introducing a container for the requests introduced in Electra, but I still think that swapping ExecutionPayload with ExecutionPayloadEnvelope is unnecessary for now. So, I would propose the following design:
class BeaconBlockBody(Container):
randao_reveal: BLSSignature
eth1_data: Eth1Data
graffiti: Bytes32
proposer_slashings: List[ProposerSlashing, MAX_PROPOSER_SLASHINGS]
attester_slashings: List[AttesterSlashing, MAX_ATTESTER_SLASHINGS]
attestations: List[Attestation, MAX_ATTESTATIONS]
deposits: List[Deposit, MAX_DEPOSITS]
voluntary_exits: List[SignedVoluntaryExit, MAX_VOLUNTARY_EXITS]
sync_aggregate: SyncAggregate
execution_payload: ExecutionPayload
bls_to_execution_changes: List[SignedBLSToExecutionChange, MAX_BLS_TO_EXECUTION_CHANGES]
blob_kzg_commitments: List[KZGCommitment, MAX_BLOB_COMMITMENTS_PER_BLOCK]
validator_requests: ValidatorRequests # [New in Electra]It benefits from @etan-status sub container proposal and does not introduce any changes to the payload in the Electra spec and implementations.
This works as well, with the advantage that it makes the Python spec easier to change.
I implemented the updated changes as suggested by @mkalinin. It simplified a lot of the design and we have the benefit of not changing the gindex of Execution Payload.