Add support for engine_forkchoiceUpdatedV5 - #9935
Conversation
dca5c6a to
73bfb17
Compare
| #[derive(Clone, Debug, PartialEq)] | ||
| pub struct PayloadStatusV1 { | ||
| pub status: PayloadStatusV1Status, | ||
| pub latest_valid_hash: Option<ExecutionBlockHash>, | ||
| pub validation_error: Option<String>, | ||
| /// [New in Heze:EIP-7805] Only populated from `engine_forkchoiceUpdatedV5` onwards. | ||
| pub inclusion_list_satisfied: Option<bool>, | ||
| } |
There was a problem hiding this comment.
This struct is now compliant with PayloadStatusV2 (per spec), however, I still kept its initial V1 naming because V2 only appends a nullable field, so this type can represent both versions. Though, I'm a bit concerned that using the initial naming could be confusing.
Not sure what is the best/clearer option here:
- keeping it as is
- renaming it to
PayloadStatusV2 - having a versioned
PayloadStatussuperstruct
Open to suggestions here
There was a problem hiding this comment.
since its a optional field im not sure we need the superstruct.
maybe just to be a bit more explicit we can add a type alias type PayloadStatusV2 = PayloadStatusV1
|
This pull request has merge conflicts. Could you please resolve them @conache? 🙏 |
mergify
Bot
added
waiting-on-author
487c39b to
314ef99
Compare
…ix) into focil-bp-il-wiring-base
rahulbarmann
left a comment
rahulbarmann
left a comment
There was a problem hiding this comment.
LGTM!
the only overlap with #10017 is PayloadStatusV1, I've already switched mine onto JsonPayloadStatusV2 so it should be a trivial conflict for whoever merges second
| @@ -940,6 +942,7 @@ impl<E: EthSpec> MockBuilder<E> { | |||
| Some(head_block_root), | |||
| Some(slot.as_u64()), | |||
| None, // TODO(gloas): pass target_gas_limit | |||
| None, | |||
There was a problem hiding this comment.
we are still using PayloadAttributes V4 in the heze case here, since we are passing None for IL. we should add a TODO here at least
Or just pass in Some(ProgressiveTransactions::empty()) for the heze variant
There was a problem hiding this comment.
oh, right! I added a dedicated Heze arm passing Some(ProgressiveTransactions::empty()) (in aa26e96), but we still need to provide a value for target_gas_limit to use PayloadAttributes V5. I'm not sure what the best value to use here is, so I left a todo comment for it, as we do for the Gloas variant for now
| #[derive(Clone, Debug, PartialEq)] | ||
| pub struct PayloadStatusV1 { | ||
| pub status: PayloadStatusV1Status, | ||
| pub latest_valid_hash: Option<ExecutionBlockHash>, | ||
| pub validation_error: Option<String>, | ||
| /// [New in Heze:EIP-7805] Only populated from `engine_forkchoiceUpdatedV5` onwards. | ||
| pub inclusion_list_satisfied: Option<bool>, | ||
| } |
There was a problem hiding this comment.
since its a optional field im not sure we need the superstruct.
maybe just to be a bit more explicit we can add a type alias type PayloadStatusV2 = PayloadStatusV1
eserilev
added
ready-for-merge
Merge Queue Status
This pull request spent 33 minutes 28 seconds in the queue, including 28 minutes 59 seconds running CI. Required conditions to merge
|
3 participants
Issue Addressed
Part of the FOCIL implementation. This PR adds the
engine_forkchoiceUpdatedV5(execution api spec) engine API integration, which enables specifying an inclusion list to the execution client when requesting a payload to be built.Proposed Changes
PayloadAttributesV5, which extends V4 withinclusion_list_transactions(spec)engine_forkchoiceUpdatedV5support to the engine API module and expose the capability throughexchange_capabilitiesPayloadStatusV2, which appends theinclusionListSatisfiedfield (per spec)custody_columnsis sent as null, matching the consensus specsAdditional Info
inclusion_list_transactionsis sent empty for now; populating it from the inclusion list store will be added in a follow-up PR