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-4844: change op to BLOBHASH #7001

Merged
merged 2 commits into from
May 16, 2023

Conversation

lightclient
Copy link
Member

Right now DATAHASH sort of conflicts with the EOF data section concept (which will want to have things like DATALOAD, DATACOPY, etc for). So I propose we change the mnemonic for blob hashes to BLOBHASH instead.

@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels May 9, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented May 9, 2023

✅ All reviewers have approved.

@eth-bot eth-bot changed the title 4844: change op to BLOBHASH Update EIP-4844: change op to BLOBHASH May 9, 2023
EIPS/eip-4844.md Outdated
@@ -246,7 +246,7 @@ The `ethereum/consensus-specs` repository defines the following beacon-node chan

### Opcode to get versioned hashes

We add an opcode `DATAHASH` (with byte value `HASH_OPCODE_BYTE`) which reads `index` from the top of the stack
We add an opcode `BLOBHASH` (with byte value `HASH_OPCODE_BYTE`) which reads `index` from the top of the stack
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
We add an opcode `BLOBHASH` (with byte value `HASH_OPCODE_BYTE`) which reads `index` from the top of the stack
We add an instruction `BLOBHASH` (with opcode `HASH_OPCODE_BYTE`) which reads `index` from the top of the stack

Copy link
Member

Choose a reason for hiding this comment

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

Some other name suggestions were:

  • TXBLOBHASH
  • TXDATAHASH

@lightclient
Copy link
Member Author

If we go forward with this, we should consider changing the nomenclature in the EIP generally to use "blob" instead of "data".

@Inphi
Copy link
Contributor

Inphi commented May 9, 2023

Makes sense as we already call the hashes in the blob tx blob_versioned_hashes. Though I don't think we need to replace "data" with "blob" everywhere else. The context is clear enough.

g11tech
g11tech previously approved these changes May 15, 2023
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

looks good to me

dankrad
dankrad previously approved these changes May 16, 2023
@lightclient lightclient dismissed stale reviews from dankrad and g11tech via c5e7e67 May 16, 2023 10:59
lightclient and others added 2 commits May 16, 2023 13:00
Co-authored-by: Paweł Bylica <chfast@gmail.com>
@lightclient
Copy link
Member Author

@dankrad also already approved this but the review was dismissed due to a rebase. The change is still the exact same, so I will go ahead and merge.

@lightclient lightclient marked this pull request as ready for review May 16, 2023 11:01
@lightclient lightclient requested a review from eth-bot as a code owner May 16, 2023 11:01
@lightclient lightclient reopened this May 16, 2023
@eth-bot eth-bot enabled auto-merge (squash) May 16, 2023 11:12
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...

AskAlexSharov pushed a commit to erigontech/erigon that referenced this pull request Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants