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

fillers/eips/eip4844/blobhash_opcode.py: Pytest port of Blobhash Opcode Tests #15

Conversation

spencer-tb
Copy link
Collaborator

@spencer-tb spencer-tb commented May 19, 2023

Within this PR:

  • Ports of the following test cases:
    • ✔️ test_blobhash_opcode_contexts to pytest -> blobhash_opcode_contexts.py
    • ✔️ test_blobhash_gas_cost to pytest -> blobhash_opcode.py
    • ✔️ test_blobhash_blob_versioned_hash to pytest -> blobhash_opcode.py
    • ✔️ test_blobhash_invalid_blob_index to pytest ->blobhash_opcode.py
    • ✔️ test_blobhash_multiple_txs_in_block to pytest -> blobhash_opcode.py
  • Additional function to for adding the blob commitment version kzg to blob hashes: blobhash_util.py

@danceratopz danceratopz force-pushed the feature/use-pytest-to-collect-and-hydrate-test-fillers-v2 branch from 536a25d to 0c8ee04 Compare May 31, 2023 11:55
@spencer-tb spencer-tb marked this pull request as ready for review June 2, 2023 17:50
@spencer-tb spencer-tb force-pushed the pytest-eip-4844-blobhash-tests branch from 4e2c870 to ba40cb4 Compare June 6, 2023 16:29
@spencer-tb spencer-tb force-pushed the pytest-eip-4844-blobhash-tests branch from 37be660 to fb0c18b Compare June 6, 2023 17:28
spencer-tb and others added 23 commits June 6, 2023 11:28
Copy link
Collaborator

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Some comments! overall it looks really good :)

if tx_type >= 3
else None,
)
for i in range(TARGET_BLOB_PER_BLOCK)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we will only see the gas cost in the storage of the last transaction.

Maybe having the same code in multiple addresses (same amount as the number of transactions sent), and finally verifying each of the addresses' storage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh yes! Damn nice find - I messed up the logic here in the port :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

471f464 removes the parameterization of the blobhash_index_values such that we have an individual blockchain test for each tx type 😄

fillers/eips/eip4844/util_blobhash.py Show resolved Hide resolved
"""
Returns an BLOBHASH sstore to the given index.
"""
return Op.SSTORE(index, Op.BLOBHASH(index))
Copy link
Collaborator

Choose a reason for hiding this comment

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

One issue I see with this method is that it overwrites the previous read of BLOBHASH.
E.g. if we do:

BLOBHASH(1)
BLOBHASH(1)
BLOBHASH(1)

And for some reason the middle one was incorrect, we will never know because the last BLOBHASH(1) overwrote the middle one.

I propose using a list of blobhash indexes like so:
[1, 2, 3], which translates in the following bytecode:

SSTORE(0, BLOBHASH(1))
SSTORE(1, BLOBHASH(2))
SSTORE(2, BLOBHASH(3))

Then check use this same list of blob indexes to generate the expected storage based on the blobs contained in the transaction.

We can modify blobhash_sstore to use memory to keep track of the last index used:

Op.SSTORE(Op.MLOAD(0), Op.BLOBHASH(blob_index)) + Op.MSTORE(0, Op.ADD(1, Op.MLOAD(0)))

If we make this modification, generate_blobhash_calls could also take the list of blobs contained by the transaction as a parameter to generate the appropriate expected post for the address containing the bytecode (and called by the tx).

On test_blobhash_multiple_txs_in_block we would indeed overwrite on the second tx, but I don't think it would matter there?

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 think I have resolved this in 84428dd

@danceratopz danceratopz merged commit dad5837 into danceratopz:feature/use-pytest-to-collect-and-hydrate-test-fillers-v2 Jun 7, 2023
@spencer-tb spencer-tb deleted the pytest-eip-4844-blobhash-tests branch July 7, 2023 03:52
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.

3 participants