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

chore: Block type migrated to BlockResponse from Network trait (alloy) #492

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

Dhruv-2003
Copy link
Contributor

This PR migrates the custom Block type in core code to alloy BlockResponse to make implementation generic over Block type, and enable integrations with L2s with custom fields to the block.

Changes:

  • custom Block and Transactions removed
  • corresponding code updated to support BlockResponse in core crate
  • Transactions replaced with BlockTransactions from alloy
  • also introduced a new method called hash_block on network trait for the networks to implement it
  • implements the network crates ethereum & op-stack to work with the new type and the method

@Dhruv-2003
Copy link
Contributor Author

In certain functions, alloy return type was different from current implementation, so I did a temporary workaround, to convert the type from alloy, as changing our implementation includes further changes across the code
https://github.com/Dhruv-2003/helios/tree/migrate-block-type/core/src/client/node.rs#L196-L207

If we want to be close to alloy, then we can add these changes as well

core/src/execution/state.rs Outdated Show resolved Hide resolved
@@ -36,6 +36,10 @@ impl NetworkSpec for Ethereum {
}
}

fn hash_block(block: &Self::BlockResponse) -> revm::primitives::B256 {
block.header.hash_slow()
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this step we need to be a bit more careful than this to ensure that the header is consistent with the other values in the block. For example, are the transactions consistent with the transactions root? I think there may be some other things as well that could be out of sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added the txs & withdrawal root verification now for both ethereum & op-stack

@@ -67,6 +69,10 @@ impl NetworkSpec for OpStack {
}
}

fn hash_block(block: &Self::BlockResponse) -> revm::primitives::B256 {
block.header.hash_slow()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue here on the consistency of the header with the rest of the block

@ncitron ncitron marked this pull request as ready for review January 22, 2025 18:22
Copy link
Collaborator

@ncitron ncitron left a comment

Choose a reason for hiding this comment

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

LGTM

@ncitron ncitron merged commit 552bc95 into a16z:master Jan 22, 2025
4 of 6 checks passed
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