Add initial FOCIL implementation #30914
Conversation
jihoonsong
requested review from
MariusVanDerWijden,
holiman,
fjl,
rjl493456442,
lightclient and
jwasinger
as code owners
jihoonsong
force-pushed
the
focil
branch
3 times, most recently
from
33efbee to
78f8ad9
Compare
jihoonsong
force-pushed
the
focil
branch
2 times, most recently
from
9f5db6f to
bbdaa45
Compare
There was a problem hiding this comment.
Pointing out Engine API has incorrect type for inclusion list. Current implementation expects base64 strings whereas the convention and the specs demand hex strings.
I suggest either define inclusion list type and its marshal type in beacon/engine/types.go and let gencodec handles the conversion similar to how ExecutableData is handled. Or manually marshal/unmarshal like how convertRequests handles incoming executionRequests in NewPayloadV4
|
I've addressed comments. Thank you very much for spotting out the issue and suggesting solutions! @ensi321 |
jihoonsong
force-pushed
the
focil
branch
2 times, most recently
from
eb46040 to
d5c724b
Compare
jihoonsong
force-pushed
the
focil
branch
3 times, most recently
from
9e1f361 to
29b805d
Compare
| pendingInclusionListTxs = append(pendingInclusionListTxs, tx) | ||
| } | ||
|
|
||
| // Append any valid inclusion list transactions at the end of the block. |
There was a problem hiding this comment.
We should sort IL+pool transactions by how many fees they pay and use this to prioritize inclusion. It's possible there could be high-paying txs in the IL that aren't in our pool.
There was a problem hiding this comment.
Absolutely. Current implementation is a naive one, prioritized easier implementation and testing. Do you think we need to adapt the inclusion strategy in this PR?
There was a problem hiding this comment.
It's not absolutely vital, but we will want to make the change before this goes into production.
Is there anything else that you omitted in this PR to keep the diff easier to read/test?
There was a problem hiding this comment.
Nothing comes to my mind at the moment. Whether to use engine_updatePayloadWithInclusionListV1 or engine_forkchoiceUpdatedV4 could be changed in future as it's not decided yet.
There was a problem hiding this comment.
I've come up with another one. It takes a naive approach when building an IL, too. It doesn't use any heuristic algorithm such as taking the time spent in the mempool into account, and so on.
There was a problem hiding this comment.
Like I wrote on the call: because it's unlikely that we will simultaneously be an IL committee member and the proposer for the same block, maybe we can afford to have a more altruistic default strategy than just building the most profitable IL.
There was a problem hiding this comment.
and also to build the most profitable IL, we would have to do full transaction execution here. and even then, it's still speculative because we are not building the IL on top of the state that it will be applied.
eth/catalyst/api.go
Outdated
| @@ -769,7 +865,7 @@ func (api *ConsensusAPI) ExecuteStatelessPayloadV4(params engine.ExecutableData, | |||
| return api.executeStatelessPayload(params, versionedHashes, beaconRoot, requests, opaqueWitness) | |||
| } | |||
|
|
|||
| func (api *ConsensusAPI) newPayload(params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash, requests [][]byte, witness bool) (engine.PayloadStatusV1, error) { | |||
| func (api *ConsensusAPI) newPayload(params engine.ExecutableData, versionedHashes []common.Hash, beaconRoot *common.Hash, requests [][]byte, inclusionList engine.InclusionList, witness bool) (engine.PayloadStatusV1, error) { | |||
There was a problem hiding this comment.
The parameters here become quite clunky. Maybe we should create a struct here to pass the data, makes it easier to add fields in the future
There was a problem hiding this comment.
Fair point. But until we get into this implementation seriously, I'd like to keep it simple and naive as master can be refactored. I believe applying these changes into master when the time comes would be cleaner.
|
At the call with Geth team, there was feedback that I haven't given much thought about where is the best location from the Geth codebase perspective, but from the spec perspective, the IL constraints should be checked after executing all transactions and before processing withdrawal requests. This is to prevent such situations that some withdrawal address become availble to cover some IL transaction that they couldn't. That missing IL transaction can reorg the block, which it shouldn't. In current implementation, it would be between L106-L107 of As this would change the core part of Geth, I would wait until I can get more feedback from the Geth team before proceeding. |
… on eip7805 fork
…clusion list constraints
|
This PR has been updated to validate inclusion list inside |
This PR implements initial FOCIL specifications. For more information, please refer to EIP-7805 and this PR.