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

Update EIP-7685: exclude empty requests data in commitment #8989

Merged
merged 4 commits into from
Nov 26, 2024

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Oct 22, 2024

Here I am proposing to change the requests_hash commitment so that empty items are excluded. The point of this is to ensure a stable empty requests hash which is independent of fork. Having such a hash makes testing a lot easier, and it mirrors how other execution-layer commitments behave.

There is also a corresponding engine API change: ethereum/execution-apis#599

Here I am proposing to change the `requests_hash` commitment so that empty items are
excluded. The point of this is to ensure a stable empty requests hash which is independent
of fork. Having such a hash makes testing a lot easier, and it mirrors how other
execution-layer commitments behave.
@fjl fjl requested a review from eth-bot as a code owner October 22, 2024 08:48
@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels Oct 22, 2024
@eth-bot
Copy link
Collaborator

eth-bot commented Oct 22, 2024

✅ All reviewers have approved.

@eth-bot eth-bot added the a-review Waiting on author to review label Oct 22, 2024
@eth-bot eth-bot changed the title EIP-7685: exclude empty requests data in commitment Update EIP-7685: exclude empty requests data in commitment Oct 22, 2024
@lucassaldanha
Copy link
Contributor

Having such a hash makes testing a lot easier

To me this is already a good reason for the change! :)

EIPS/eip-7685.md Outdated
m.update(sha256(r))
for r in block_requests:
if len(r) > 1:
m.update(sha256(r))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
m.update(sha256(r))
m.update(sha256(r).digest())

tersec pushed a commit to status-im/nimbus-eth2 that referenced this pull request Nov 24, 2024
* Adopt latest changes to request hash computation

The `requestType` of empty lists is no longer part of the requests hash.

- ethereum/EIPs#8989

* Avoid nested computeDigest scopes
g11tech
g11tech previously approved these changes Nov 25, 2024
@mkalinin
Copy link
Contributor

With this change the order of requests by type is only mentioned in the Rationale which seems to be wrong. The ordering should be either explicitly specified by saying that block_requests passed onto the compute_requests_hash call must be ordered by the type byte or implicitly done as a part of compute_requests_hash. I prefer the latter because written code is more likely to make it into implementation

EIPS/eip-7685.md Outdated Show resolved Hide resolved
@fjl
Copy link
Contributor Author

fjl commented Nov 26, 2024

I have pushed a more thorough rewrite of the description of the commitment.

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

LGTM

@eth-bot eth-bot enabled auto-merge (squash) November 26, 2024 13:42
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-review Waiting on author to review c-update Modifies an existing proposal s-review This EIP is in Review t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants