Skip to content
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

Proposer compares fees between local and builder blocks #11557

Closed
wants to merge 1 commit into from

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Oct 17, 2022

This is an experimental PR with some unknowns, which we'll document in the todos. This PR enables proposer to compare and choose between 1.) builder block fees and 2.) local block fees. The proposer will choose the higher fee between local_block_fees and builder_block_fees * N / 100 where N is a CLI flag but defaulted to 100

Todos

  • Simply adding fees to ExecutionPayload will not work!! ExecutionPayload is a consensus object. We can not modify that because it's part of the ssz spec. We need to decouple consensus protobuf with local JSON communication objects
  • The default fraction is 100%. We should understand and pick a better value. I wonder what model or heuristic method we can learn from

@rauljordan
Copy link
Contributor

To get a better feel for this, perhaps we can add logging / metrics for how the local vs. builder fees compare in practice to our dev nodes? This way we can evaluate the actual fraction we should use

@@ -145,6 +146,8 @@ func (e *ExecutionPayload) MarshalJSON() ([]byte, error) {
timeStamp := hexutil.Uint64(e.Timestamp)
recipient := common.BytesToAddress(e.FeeRecipient)
logsBloom := hexutil.Bytes(e.LogsBloom)
fee := new(big.Int).SetBytes(bytesutil.ReverseByteOrder(e.Fees))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically this will be a new JSON field of execution payloads that we receive via the engine API, right? This is gonna be hard unless we add the ability to skip fields for SSZ generation...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Or we don't use consensus protobuf object for engine API marshal/unmarshal. It's not clear to me why that's a must

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need to, only once we are preparing to put it inside a beacon block. The naming is weird though...perhaps we could have pb.ExecutionPayload which is used in consensus and ExecutionPayloadEngineResponse types

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming is weird though

Geth uses ExecutionData which I like

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see this feature being worked on! To help co-ordinate amongst teams could you comment on the execution API issue/pull request on how to best format this new data field. The current suggestion from @mkalinin is to create a new engine_getPayloadV2 method.

ethereum/execution-apis#314
ethereum/execution-apis#307

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants