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

beacon/engine,eth/catalyst: hex marshal requests in engine api #30603

Merged
merged 4 commits into from
Oct 17, 2024

Conversation

lightclient
Copy link
Member

In #30584 I was mostly trying to sort out the issues in t8n, now I've run into similar unmarshaling issues in the engine API. Currently the methods like NewPayloadV4 define requests as [][]byte, but this doesn't handle hex correctly.

The two solutions to this seems to be:

  • define a type like RequestsList with the decoding methods
  • create a list of hexutil.Bytes

I went with the second option in this PR. It seems overkill to create a fully new type. However, what gives me pause is the interface for BlockToExecutableData now takes in []hexutil.Bytes. We could keep it as [][]byte, but that would require some data shuffling to convert from the []hexutil.Bytes. Not sure if people have a preference on that parameter type.

@lightclient lightclient force-pushed the eth-catalyst-hex-requests branch 2 times, most recently from 9bb3c22 to 689248e Compare October 15, 2024 16:43
@karalabe
Copy link
Member

I'd like to push back against hexutil types being passed around as method params. The purpose of those is to be encoding stuff. I'd ideally prefer to convert from hexutil to []byte and pass that to the block makers. Otherwise you open up a can of worms where hexutil will start to worm itself into our internal typesystem.

@fjl
Copy link
Contributor

fjl commented Oct 16, 2024

I have pushed another small update. Not sure if using unsafe is really warranted, but it feels silly to allocate a new slice just to convert to the same underlying type.

@fjl
Copy link
Contributor

fjl commented Oct 16, 2024

Actually changed my mind, it's better without unsafe.

@lightclient
Copy link
Member Author

I feel like if we're going to have requests as [][]byte in ExecutionPayloadEnvelope we should also update the witness to be []byte?

@fjl fjl merged commit e26468f into ethereum:master Oct 17, 2024
3 checks passed
@fjl fjl added this to the 1.14.12 milestone Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants