Add engine_getPayloadV2 with locally built block value#314
Add engine_getPayloadV2 with locally built block value#314mkalinin merged 2 commits intoethereum:mainfrom
Conversation
mkalinin
left a comment
There was a problem hiding this comment.
If we expect to rollout block value before Shanghai which I think we do, then this PR should also bump the version of Shanghai's getPayload to V3
src/engine/specification.md
Outdated
|
|
||
| * result: `object` | ||
| - `executionPayloadV1`: [`ExecutionPayloadV1`](#ExecutionPayloadV1) | ||
| - `blockValue` : `QUANTITY`, 256 Bits - The expected value to be recieved by the `feeRecipient` in wei |
There was a problem hiding this comment.
AFAIK, we decided to use a sum of priority fees to calculate value of locally built block. If so, I would explicitly add this statement to the field definition. Or do it in another way, say something abstract in the field definition and specify particular calculation algorithm in the Specification part.
There was a problem hiding this comment.
did we decide to go this route? I lost the thread over a number of long-running conversations across the various issues...
There was a problem hiding this comment.
My notes from CL call #97 were that sum of fees (over balance diff) was the preferred approach, but there were a few voices for giving the EL flexibility to implement it however it wants.
For context, some ideas mentioned on why the EL might want to diverge from simply summing fees were to include coinbase payments in the block value (in case MEV searchers sent transactions to the mempool) or to add an artificial boost to local block value over the builder's blocks.
There was a problem hiding this comment.
I think the probability of searchers sending their transaction to the mempool is negligible, thus sum of the fees looks like the best approach to evaluate locally built block value as it's not affected by inbound and outbound transactions to and from the fee recipient address.
I actually like the idea of flexibility, we may specify that blockValue computation is implementation dependent.
There was a problem hiding this comment.
I think in some cases searchers will fall back to the mempool, and even play PGAs if the situation calls for it -- esp. with long-tail types of MEV just getting a transaction on-chain via the public mempool can be preferable to using a builder
that being said, I think we are fine to settle on a "good enough" solution here
the thing w/ this computation being client-specific is that you could get to a setting where one client team has spent more time optimizing this and that leads to better returns and we have a a centralization vector around that one client -- so I think better to standardize one approach
There was a problem hiding this comment.
I think in some cases searchers will fall back to the mempool, and even play PGAs if the situation calls for it -- esp. with long-tail types of MEV just getting a transaction on-chain via the public mempool can be preferable to using a builder
I guess this transaction will be quickly bundled in one of the builders blocks, if this transaction breaches sanctions there are non-censoring builders that can still include it into a block. Using public mempool in this case makes sense only if local builders can recognise this type of MEV, i.e. account for coinbase transfers, or if a searcher don't want to communicate with builders directly. So, if we want to optimise for this case then I do agree that we need more fancy logic for computing block value which accounts for transactions spending fee recipient's balance that can be supplied by a local mempool.
I think specification of exact algorithm if we have it unified or not (there can be a couple of preferred approaches) should be out of the scope of the Engine API spec. If we see an EL client with some very popular algo becoming a centralised force then we should consider this algo to be spread across all other clients. Though, I believe we don't have a lot of different approaches to compute this thing
There was a problem hiding this comment.
I added this to the spec as a compromise, does that work?
"4. Client software MAY use the sum of the block's priority fees to determine blockValue."
There was a problem hiding this comment.
yeah this works for me
I think we can let client teams decide how to implement this and there is enough alignment across them that if something weird happens after deployment we can lobby to tweak one way or another
|
I am wondering how urgent is this change. We have Capella/Shanghai devnets in progress and I believe now is the time to add this change to the Engine API spec, otherwise, there is a risk of it to be delay until after the Shanghai. |
|
This change has been considered for inclusion in Shanghai on ACD#151 with an option to be reconsidered if it delays Shanghai. |
c38ab2e to
f33432b
Compare
|
IMHO it doesn't make much sense to have a V3 already, V2 should contain Is there a reason why not to do it like this? |
Yeah, V3 doesn't make sense. This PR should basically add |
|
Ok, I had thought that some clients were planning to support this pre-Shanghai. Will remove V3 |
|
Thanks for the comments, I have committed the suggested changes. This should be ready to merge now pending the reorganization in #327. |
ralexstokes
left a comment
There was a problem hiding this comment.
this looks good to me
I'd even suggest merging before #327 so clients have something to target for testiest
src/engine/specification.md
Outdated
| * method: `engine_getPayloadV2` | ||
| * params: | ||
| 1. `payloadId`: `DATA`, 8 Bytes - Identifier of the payload build process | ||
| * timeout: 1s |
There was a problem hiding this comment.
yeah this should have probably been there in the first place
Sounds good, @lightclient can you take a look and merge this if no issues? Thanks. |
lightclient
left a comment
There was a problem hiding this comment.
Hi, this generally looks good to me. I left one comment that I think should be fixed, but after this PR is rebased and that is addressed I believe we can merge.
Apply suggestions from code review Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com> Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
710e2b7 to
7db4151
Compare
This is an initial draft for versioning
engine_getPayloadto support the EL client's estimate of the value of the block.This allows the CL to compare the value of locally built blocks to remotely build blocks received from relays/builders.
#307
This PR is pending consensus on the optimal path to handle changes to the execution API.
Comments and/or improvements are welcome.