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

Merged

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.

kevm-pyk/src/kevm_pyk/kproj/evm-semantics/evm.md Outdated Show resolved Hide resolved
kevm-pyk/src/kevm_pyk/kproj/evm-semantics/state-utils.md Outdated Show resolved Hide resolved
@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.

- Position of `BLOBHASH` on evm.md
- Deleting invalid side condition in `$effectiveGasPrice`
Copy link
Collaborator

@dwightguth dwightguth left a comment

Choose a reason for hiding this comment

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

Code looks good. I wonder why some of the blobhash related tests still fail. You may want to take a look, but obviously the first priority is getting the proofs passing. As I said before, please timebox that to a couple days at most and let me know if it seems like it will take longer than that.

kevm-pyk/src/kevm_pyk/kproj/evm-semantics/evm.md Outdated Show resolved Hide resolved
@@ -2,19 +2,35 @@ BlockchainTests/GeneralStateTests/Cancun/stEIP4844-blobtransactions/blobhashList
BlockchainTests/GeneralStateTests/Cancun/stEIP4844-blobtransactions/blobhashListBounds4.json,*
BlockchainTests/GeneralStateTests/Cancun/stEIP4844-blobtransactions/blobhashListBounds5.json,*
BlockchainTests/GeneralStateTests/Cancun/stEIP4844-blobtransactions/blobhashListBounds6.json,*
BlockchainTests/GeneralStateTests/Cancun/stEIP4844-blobtransactions/blobhashListBounds7.json,*
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see a lot of green in this file. Can you make sure you're explaining what is happening here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From what I saw, it's mostly because these tests were supposed to fail. However, they were failing for the wrong reasons and now, they're "passing" when a failure is expected.

The reasons vary a lot, from wrong balance (lack of implementation of gas accounting) to the block validation.

@Robertorosmaninho Robertorosmaninho merged commit 7c1de74 into master Jan 31, 2025
12 checks passed
@Robertorosmaninho Robertorosmaninho deleted the pi2/robertorosmaninho/eip-4844-blobhash-opcode branch January 31, 2025 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants