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

Implement Blob transactions (type-3) #50

Merged
merged 9 commits into from
Feb 21, 2024
Merged

Implement Blob transactions (type-3) #50

merged 9 commits into from
Feb 21, 2024

Conversation

Nashtare
Copy link
Collaborator

@Nashtare Nashtare commented Feb 18, 2024

First part of dealing with #41

  • Implements blob transactions (type-3)
  • Adds new block header fields related to blob gas market
  • Implements BLOBHASH opcode

It doesn't deal with the actual blob gas market, this is low priority, though could be tackled in the future if judged necessary.

Once #46 is done, we could add a similar test than evm_arithmetization/src/cpu/kernel/tests/transaction_parsing/parse_type_0_txn.rs but for type-3 easily.

@Nashtare Nashtare added this to the Cancun - Q1 2024 milestone Feb 18, 2024
@Nashtare Nashtare self-assigned this Feb 18, 2024
Comment on lines +308 to +311
(
GlobalMetadata::BlockBlobBaseFee,
metadata.block_blob_base_fee,
),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated, but another tiny oversight of 0xPolygonZero/plonky2#1313

Copy link
Contributor

@hratoanina hratoanina left a comment

Choose a reason for hiding this comment

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

Partial review.

Copy link
Contributor

@hratoanina hratoanina left a comment

Choose a reason for hiding this comment

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

Finishing my review.

I think some part of the block circuit logic is missing, namely the updating of the blob excess gas in connect_block_proof.
Apart from that, looks good, thanks!

@Nashtare
Copy link
Collaborator Author

I think some part of the block circuit logic is missing, namely the updating of the blob excess gas

Do you mean the enforcement that the value is consistent with the parent block excess gas + blob_gas_used? I'm not sure this is really relevant, as we don't enforce anything on the blob gas market logic anyway (at least now). Or did you mean something else?

@@ -281,6 +281,39 @@ global sys_basefee:
SWAP1
EXIT_KERNEL

global sys_blobhash:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can add an interpreter test for this function?

@4l0n50
Copy link
Contributor

4l0n50 commented Feb 21, 2024

Once #46 is done, we could add a similar test than evm_arithmetization/src/cpu/kernel/tests/transaction_parsing/parse_type_0_txn.rs but for type-3 easily.

I don't understand why would you need the evm tests for cancun for the type 3 parsing test. Is it for avoiding using py-evm?

@Nashtare
Copy link
Collaborator Author

Once #46 is done, we could add a similar test than evm_arithmetization/src/cpu/kernel/tests/transaction_parsing/parse_type_0_txn.rs but for type-3 easily.

I don't understand why would you need the evm tests for cancun for the type 3 parsing test. Is it for avoiding using py-evm?

I meant to easily test. Ideally (probably not worth doing it on the current main branch), with the Cancun branch, we should have more hardcoded tests from the official Ethereum test suite that we just deserialize and run in our CI. It's cleaner and easier than crafting our own tests that are unofficial.

@Nashtare Nashtare merged commit 8926efd into feat/cancun Feb 21, 2024
5 checks passed
@Nashtare Nashtare deleted the blob_txn branch February 21, 2024 18:03
BGluth pushed a commit that referenced this pull request Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants