-
Notifications
You must be signed in to change notification settings - Fork 457
Add initial FOCIL spec #609
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
base: main
Are you sure you want to change the base?
Conversation
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.
I think there once was an experimental folder for your purpose but was later removed because the folder is empty.
You can refer to #340 to see how eip6110.md is placed.
|
Moved EIP-7805 spec under the experimental folder. Thank you for your review and providing the reference! @ensi321 |
|
I think |
e518c7b to
572c54c
Compare
|
Will |
src/engine/experimental/eip7805.md
Outdated
|
|
||
| #### Response | ||
|
|
||
| * result: `inclusionList`: `Array of DATA` - Array of transaction objects, each object is a byte list (`DATA`) representing `TransactionType || TransactionPayload` or `LegacyTransaction` as defined in [EIP-2718](https://eips.ethereum.org/EIPS/eip-2718). |
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.
We can introduce a response object like it is done in the forkchoiceUpdated case. Should we extend inclusion list with some meta information in the future, it will be smoother with an object in response
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.
We had object for that purpose but removed it as it makes the caller's job easier. I want to ask @terencechain's opinion on this. Relevant discussion here.
|
There are some potential changes in queue and I'd like to share them here. They're queued because we want to make big changes altogether as we already have +5 clients implemented. Being SFI'd could be a good signal to make such changes.
|
|
I’d suggest we use a different name than |
|
Reflected the changes in the queue. Here is a change log:
Note: the fork check using timestamp is not specified. It should be specified once this merges into some canonical fork. |
|
|
||
| #### Response | ||
|
|
||
| * result: [`PayloadStatusV1`](./paris.md#payloadstatusv1), values of the `status` field are modified in the following way: |
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.
Why don't we define PayloadStatusV2?
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.
We add one enum value without changing its structure and it seems the spec doesn't add another version in such cases. It's not a strong opinion though. Should we define PayloadStatusV2?
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.
Currently, the spec doesn’t require to bump a version in this case, see here for all cases where the version change is required — we can definitely extend this list if we find it reasonable. But I am personally leaning towards not adding a new version in this particular case as the change doesn’t affect the layout of the PayloadStatus
This PR adds initial FOCIL spec. It mainly adds three new methods.
i)
engine_newPayloadV5engine_newPayloadV5is introduced asengine_newPayloadV4will be shipped in Prague.engine_newPayloadV5takes an inclusion list (IL) as a parameter and verifies if the payload satisfies the IL constraints. (For the IL constraints, please refer toExecution Layersection in EIP-7805.)As IL isn't recorded onchain, it cannot be enforced during syncing. Currently, this spec adopts a naive approach: CL passes IL only when not syncing and EL enforces the IL constraints only if the given IL is not null. We're looking for feedback on whether there is a better approach such as using a sync complete flag.
ii)
engine_getInclusionListV1EL must provide an IL when
engine_getInclusionListV1is called. FOCIL as in EIP-7805 does not dictate IL construction algorithm and expects that having diverse approaches would help foster censorship resistance.iii)
engine_updatePayloadWithInclusionListV1A proposer should listen to all ILs submitted by IL committee members and apply the aggregated IL to its payload before proposing a block. There are two ways to achieve this.
a) use engine_forkchoiceUpdated
We can add the IL field to
payloadAttributesand callforkchoiceUpdatedinitially with a null IL field at the start of a slot, then call it again with the actual IL once it’s ready.This will require modifying FCU to allow updates to an ongoing payload building process. If I’m not mistaken, geth and reth currently early return a valid response without updating the existing payload building process.
b) add a new Engine API
The second option is adding a new Engine API. We welcome feedback on better ways of applying IL.
We’re in the early stages of FOCIL implementation and would appreciate your feedback. Thank you.