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

feat(pectra): Use EIP-7865 encoding for execution_requests #107

Conversation

ryanschneider
Copy link

@ryanschneider ryanschneider commented Dec 6, 2024

As discussed here on the Eth R&D Discord I think we should change to using the EIP-7865 encoding for execution_requests for the following reason:

The relays usually validate submissions by sending them to a custom EL implementing the flashbots_validateBuilderSubmissionVN RPC (aside: I'd love to work with everyone and get this RPC standardized!). Since this RPC uses the same payload format as the builder spec, this means that any ELs that want to implement this method (or hopefully the future standardized version) would need to consume the requests in the "full" non-opaque format as well.

Since there didn't seem to be any strong objections to this I was encouraged to proceed with drafting a specs change, so here it is.

A couple more todos/questions:

  • Currently I've included placeholder 0x_TODO values for the encoded requests lists, ideally these should actually be decodable values (so someone can try converting them to the consensus-spec types as a code test) I just haven't had time to do that yet.
  • @jtraglia just pointed me to get_execution_requests_list function in the consensus-specs which properly explains the encoding process, I'll link to that as well.
  • I think Bitvector is the correct type to use in the spec for each entry, but I'm not terribly familiar with the CL specs so and happy to be corrected!

}
}
},
"execution_requests": [ "0x00_TODO_DEPOSITS", "0x01_TODO_WITHDRAWLS", "0x02_TODO_CONSOLIDATIONS"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can use the example value in ethereum/execution-apis#607 .

Copy link
Author

Choose a reason for hiding this comment

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

Perfect, updated in 7955984 as well, thanks again!

type: array
items:
$ref: '../../beacon-apis/types/primitive.yaml#/Bitvector'
minItems: 3
Copy link
Contributor

Choose a reason for hiding this comment

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

minItems should be 0 in case all of the requests (deposit, withdrawal and consolidation) are empty

Copy link
Author

Choose a reason for hiding this comment

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

Oh right I was still thinking pre- ethereum/execution-apis#599, updated in 7955984 thanks!

@ryanschneider ryanschneider requested a review from ensi321 December 6, 2024 02:20
description: "`ExecutionRequests` to include in block proposal."
type: array
items:
$ref: '../../beacon-apis/types/primitive.yaml#/Bitvector'

Choose a reason for hiding this comment

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

I am not sure if the Bitvector is a good semantic representation for the opaque field, although technically, it could work.

I would consider adding a Bytes primitive type (similar to Bytes32) but unbounded in length.

Keen to have people weight in on this before committing to it.

Copy link
Author

Choose a reason for hiding this comment

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

Funny when writing this up I originally used Bytes only to go look up the actual primitives and realize it didn't exist. :). So ya I'd be in favor of that change as well. I assume that'd be a separate PR on https://github.com/ethereum/beacon-apis/ ?

Choose a reason for hiding this comment

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

I believe so. It looks like builder-specs is importing beacon-api as a git submodule and referring to the types defined there.

Copy link
Author

Choose a reason for hiding this comment

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

Ok cool I just sent ethereum/beacon-APIs#484 I guess if this is accepted I can update the submodule in this PR and point to that. Thanks for the suggestion!

Copy link
Contributor

Choose a reason for hiding this comment

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

I would consider adding a Bytes primitive type (similar to Bytes32) but unbounded in length.

you can't have unbounded ssz types

Copy link
Contributor

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

I don't understand why we want to adopt this format on the CL side, this would mean we also have update the other apis on the beacon-api side to match the format.

SSZ is a native concept to the CL and embedding SSZ serialized data (as hex) inside JSON quite frankly is a horrible pattern.

Would be good if we can keep that hacky approach contained to the engine api

### `OpaqueExecutionRequests`

```python
class OpaqueExecutionRequests(List[Bitvector]):
Copy link
Contributor

Choose a reason for hiding this comment

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

those are not really opaque, just ssz encoded and 1-byte prefixed, anyone can decode the requests and read the data

Copy link
Author

Choose a reason for hiding this comment

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

They're "semantically" opaque, EIP-7865 stipulates that each request type is allowed to decide its own encoding, it's just a coincidence that the current three are all SSZ in the same way that all EIP-2718 txs are currently RLP.

A requests object consists of a request_type prepended to an opaque byte array request_data. The request_data contains zero or more encoded request objects.

Each request type will defines its own requests object with its own request_data format.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if the same wording is used on the engine-api side this is fine

@ryanschneider
Copy link
Author

I don't understand why we want to adopt this format on the CL side, this would mean we also have update the other apis on the beacon-api side to match the format.

SSZ is a native concept to the CL and embedding SSZ serialized data (as hex) inside JSON quite frankly is a horrible pattern.

Would be good if we can keep that hacky approach contained to the engine api

When you say we "have to update the other apis" do you mean we'd be forced to because of some hard dependency, or just to maintain consistency? If there's a hard dependency then yes I agree we shouldn't do it, but since the builder spec sits in a grey zone between the EL and CL IMO it's ok for it to be less consistent with the rest of the CL APIs.

@nflaig
Copy link
Contributor

nflaig commented Dec 6, 2024

When you say we "have to update the other apis" do you mean we'd be forced to because of some hard dependency, or just to maintain consistency?

There are apis which pass a SignedBlindedBeaconBlock around, namely produceBlockV3 and publishBlindedBlockV2 (which just passes the block through to builder) and those follow the format used in the builder spec.

Further more, I think we would also have to update BeaconBlockBody on the CL spec as otherwise the hash tree root of full block and blinded block would be different.

but since the builder spec sits in a grey zone between the EL and CL IMO it's ok for it to be less consistent with the rest of the CL APIs.

is it a grey zone? imo the builder spec is a pure CL spec extension

@ryanschneider
Copy link
Author

There are apis which pass a SignedBlindedBeaconBlock around, namely produceBlockV3 and publishBlindedBlockV2 (which just passes the block through to builder) and those follow the format used in the builder spec.

Ah, thanks, I wasn't aware of these, and ya my reading is they would have to change as well. :( @lucassaldanha or @jtraglia can you confirm?

is it a grey zone? imo the builder spec is a pure CL spec extension

I guess what I meant by grey zone is that the block builders are usually custom ELs, for example rbuilder is built on reth/alloy and will now probably need to handle serde for the requests types (which is what I think we were trying to avoid with the engine api using the opaque types) as mentioned here: flashbots/rbuilder#267

@jtraglia
Copy link
Member

jtraglia commented Dec 6, 2024

There are apis which pass a SignedBlindedBeaconBlock around, namely produceBlockV3 and publishBlindedBlockV2 (which just passes the block through to builder) and those follow the format used in the builder spec.

Ah, thanks, I wasn't aware of these, and ya my reading is they would have to change as well. :( @lucassaldanha or @jtraglia can you confirm?

Correct. These would need to change too. And I would advise against that.

@ryanschneider
Copy link
Author

Ok, sounds like builders will have to go with the JSON encoding for their relay submissions afterall, thanks everyone for the feedback but I'll close this PR.

@nflaig
Copy link
Contributor

nflaig commented Dec 6, 2024

You don't have to use JSON if we start adopting SSZ on the builder api, see #104. This would address any concerns related to performance / latency overhead.

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.

5 participants