Add engine_getPayloadV2 with locally built block value #314

allboxes · 2022-10-16T21:57:47Z

This is an initial draft for versioning engine_getPayload to 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.

mkalinin

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

mkalinin · 2022-11-22T05:44:32Z

src/engine/specification.md

+
+* result: `object`
+  - `executionPayloadV1`: [`ExecutionPayloadV1`](#ExecutionPayloadV1)
+  - `blockValue` : `QUANTITY`, 256 Bits - The expected value to be recieved by the `feeRecipient` in wei


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.

did we decide to go this route? I lost the thread over a number of long-running conversations across the various issues...

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.

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.

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

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

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."

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

mkalinin · 2022-12-08T09:38:15Z

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.

src/engine/specification.md

mkalinin · 2022-12-08T15:49:04Z

This change has been considered for inclusion in Shanghai on ACD#151 with an option to be reconsidered if it delays Shanghai.

marioevz · 2022-12-08T18:50:12Z

IMHO it doesn't make much sense to have a V3 already, V2 should contain executionPayloadV2 from the start, and if you need to get the block value now (not having to wait for the withdrawals fork), put null withdrawals in executionPayloadV2.

Is there a reason why not to do it like this?

@mkalinin

mkalinin · 2022-12-09T09:01:10Z

IMHO it doesn't make much sense to have a V3 already, V2 should contain executionPayloadV2 from the start, and if you need to get the block value now (not having to wait for the withdrawals fork), put null withdrawals in executionPayloadV2.

Is there a reason why not to do it like this?

@mkalinin

Yeah, V3 doesn't make sense. This PR should basically add blockValue to already existing getPayloadV2 as the desire is to have this change in the Shanghai's scope.

allboxes · 2022-12-09T15:30:48Z

Ok, I had thought that some clients were planning to support this pre-Shanghai. Will remove V3

mkalinin

I would wait for this PR to get merged #327, then we can clean up and merge this change.

src/engine/specification.md

allboxes · 2022-12-12T16:19:15Z

Thanks for the comments, I have committed the suggested changes. This should be ready to merge now pending the reorganization in #327.

ralexstokes

this looks good to me

I'd even suggest merging before #327 so clients have something to target for testiest

ralexstokes · 2022-12-15T20:59:35Z

src/engine/specification.md

@@ -470,15 +470,20 @@ This method follows the same specification as [`engine_forkchoiceUpdatedV1`](#en
 * method: `engine_getPayloadV2`
 * params:
  1. `payloadId`: `DATA`, 8 Bytes - Identifier of the payload build process
+* timeout: 1s


yeah this should have probably been there in the first place

mkalinin · 2022-12-16T08:16:53Z

I'd even suggest merging before #327 so clients have something to target for testiest

Agreed, #327 needs some debates before merging

allboxes · 2022-12-17T04:28:32Z

I'd even suggest merging before #327 so clients have something to target for testiest

Agreed, #327 needs some debates before merging

Sounds good, @lightclient can you take a look and merge this if no issues? Thanks.

lightclient

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.

src/engine/specification.md

Apply suggestions from code review Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com> Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>

allboxes marked this pull request as draft October 16, 2022 21:57

allboxes mentioned this pull request Oct 18, 2022

Proposer compares fees between local and builder blocks prysmaticlabs/prysm#11557

Closed

pawanjay176 mentioned this pull request Oct 19, 2022

Add engine_getPayloadV2 sigp/lighthouse#3646

Closed

MariusVanDerWijden mentioned this pull request Nov 4, 2022

miner: compute tx fees for blocks ethereum/go-ethereum#25998

Closed

deffrian mentioned this pull request Nov 9, 2022

Add block value to getPayload result NethermindEth/nethermind#4884

Merged

9 tasks

mkalinin reviewed Nov 22, 2022

View reviewed changes

This was referenced Dec 8, 2022

Engine API spec improvement proposal #321

Closed

Ethereum Core Devs Meeting 151 Agenda ethereum/pm#675

Closed

marioevz reviewed Dec 8, 2022

View reviewed changes

src/engine/specification.md Outdated Show resolved Hide resolved

allboxes closed this Dec 8, 2022

allboxes force-pushed the getPayloadV2 branch from c38ab2e to f33432b Compare December 8, 2022 18:14

allboxes reopened this Dec 8, 2022

allboxes marked this pull request as ready for review December 8, 2022 18:30

allboxes requested a review from mkalinin December 9, 2022 19:07

mkalinin reviewed Dec 12, 2022

View reviewed changes

src/engine/specification.md Outdated Show resolved Hide resolved

src/engine/specification.md Outdated Show resolved Hide resolved

src/engine/specification.md Outdated Show resolved Hide resolved

ralexstokes approved these changes Dec 15, 2022

View reviewed changes

This was referenced Dec 15, 2022

Support locally built block value field Consensys/teku#6612

Closed

Add blockValue to get payload V2 response Consensys/teku#6614

Merged

mkalinin approved these changes Dec 16, 2022

View reviewed changes

ethDreamer mentioned this pull request Dec 16, 2022

Make engine_getPayloadV2 accept local block value sigp/lighthouse#3817

Merged

siladu mentioned this pull request Dec 20, 2022

Throwaway Withdrawals Testnet Branch (withdrawal-devnet-3) hyperledger/besu#4818

Closed

lightclient requested changes Dec 20, 2022

View reviewed changes

src/engine/specification.md Outdated Show resolved Hide resolved

mkalinin reviewed Dec 21, 2022

View reviewed changes

src/engine/specification.md Outdated Show resolved Hide resolved

Add blockValue to engine_getPayloadV2

7db4151

Apply suggestions from code review Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com> Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>

allboxes force-pushed the getPayloadV2 branch from 710e2b7 to 7db4151 Compare December 24, 2022 17:00

Fix reference path

86c03a0

allboxes requested a review from lightclient December 24, 2022 17:07

mkalinin merged commit fb72652 into ethereum:main Dec 26, 2022

allboxes mentioned this pull request Dec 26, 2022

[RFC] Engine API: Add block value in response to engine_getPayload #307

Closed

terencechain mentioned this pull request Dec 27, 2022

Add block value response to blob-extension spec #341

Merged

yperbasis mentioned this pull request Jan 12, 2023

GetPayloadV2 change ledgerwatch/erigon#6554

Merged

siladu mentioned this pull request Jan 14, 2023

withdrawals-devnet-2 hyperledger/besu#4929

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add engine_getPayloadV2 with locally built block value #314

Add engine_getPayloadV2 with locally built block value #314

allboxes commented Oct 16, 2022 •

edited

Loading

mkalinin left a comment

mkalinin Nov 22, 2022

ralexstokes Nov 22, 2022

allboxes Nov 22, 2022

mkalinin Nov 23, 2022

ralexstokes Nov 23, 2022

mkalinin Nov 24, 2022

allboxes Dec 8, 2022

ralexstokes Dec 15, 2022

mkalinin commented Dec 8, 2022

mkalinin commented Dec 8, 2022

marioevz commented Dec 8, 2022

mkalinin commented Dec 9, 2022

allboxes commented Dec 9, 2022

mkalinin left a comment

allboxes commented Dec 12, 2022

ralexstokes left a comment

ralexstokes Dec 15, 2022

mkalinin commented Dec 16, 2022

allboxes commented Dec 17, 2022

lightclient left a comment

Add engine_getPayloadV2 with locally built block value #314

Add engine_getPayloadV2 with locally built block value #314

Conversation

allboxes commented Oct 16, 2022 • edited Loading

mkalinin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkalinin commented Dec 8, 2022

mkalinin commented Dec 8, 2022

marioevz commented Dec 8, 2022

mkalinin commented Dec 9, 2022

allboxes commented Dec 9, 2022

mkalinin left a comment

Choose a reason for hiding this comment

allboxes commented Dec 12, 2022

ralexstokes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkalinin commented Dec 16, 2022

allboxes commented Dec 17, 2022

lightclient left a comment

Choose a reason for hiding this comment

allboxes commented Oct 16, 2022 •

edited

Loading