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

Implementing BLOBHASH Opcode #2693

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Robertorosmaninho
Copy link
Collaborator

This PR implements the BLOBHASH Opcode as part of the implementation of EIP-4844.
To implement this new operation, the type of txVersionedHashes had to be changed to List.

@@ -479,6 +487,7 @@ The `"rlp"` key loads the block information.
=> #if ( notBool Ghasbasefee << SCHED >> )
orBool TXTYPE ==K Legacy
orBool TXTYPE ==K AccessList
orBool TXTYPE ==K Blob
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm just unsure about this part. Do you have any thoughts, @anvacaru? Please see this Gas Accounting section of EIP-4844 for context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems wrong. Blob transactions are newer than DynamicFee transactions and should have the fields of all prior types, including the DynamicFee fields.

rule <k> MLOAD INDEX => #asWord(#range(LM, INDEX, 32)) ~> #push ... </k>
<localMem> LM </localMem>

rule <k> BLOBHASH INDEX => 0 ~> #push ... </k>
Copy link
Collaborator

Choose a reason for hiding this comment

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

These rules seem fine but the placement of them, between MLOAD and MSTORE, seems off. I would place it next to BLOCKHASH, which it is much more similar to.

@@ -479,6 +487,7 @@ The `"rlp"` key loads the block information.
=> #if ( notBool Ghasbasefee << SCHED >> )
orBool TXTYPE ==K Legacy
orBool TXTYPE ==K AccessList
orBool TXTYPE ==K Blob
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems wrong. Blob transactions are newer than DynamicFee transactions and should have the fields of all prior types, including the DynamicFee fields.

@dwightguth
Copy link
Collaborator

dwightguth commented Jan 23, 2025

There is one more problem. You are matching directly on the txVersionedHashes field, but this field exists inside a cell Map and you are not specifying which transaction you are accessing the hashes of, which means that if the block contains multiple transactions, you might conceivably return the hashes associated with a past or future transaction within the block. You probably want to copy the field from the transaction into the <evm> cell when loadTx is executed. This will involve a change to the configuration and thus to the spec files in the test suite, but I don't see anything that is currently in the EVM cell that could be reliably used to tell the semantics which transaction you are interested in accessing.

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.

2 participants