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

feat(ethereum) <- add eip4844 block header verification #19

Merged
merged 9 commits into from
Feb 19, 2024

Conversation

Oghma
Copy link
Contributor

@Oghma Oghma commented Feb 9, 2024

This PR continues the work of #15 by adding block header verification for the new Dencun fork

@Oghma Oghma requested a review from gskapka February 9, 2024 14:48
@Oghma Oghma force-pushed the eip4844-block-verification branch from 133e9eb to 340824d Compare February 9, 2024 15:03
@Oghma Oghma self-assigned this Feb 9, 2024
@Oghma Oghma force-pushed the eip4844-block-verification branch from 340824d to db43b1a Compare February 9, 2024 15:27
common/ethereum/src/eip_4844.rs Outdated Show resolved Hide resolved
…hether eip4844 is active

The block number of when Dencun will be realeased on mainnet is not
known, only the beacon slot. Reverse the commit after Dencun is released
@Oghma Oghma force-pushed the eip4844-block-verification branch from 70989b3 to 66193ff Compare February 16, 2024 18:34
Copy link
Contributor

@gskapka gskapka left a comment

Choose a reason for hiding this comment

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

Couple of tiny tweaks, but otherwise LGTM.

Comment on lines 21 to 23
pub fn is_active(&self, block: &EthBlock) -> Result<bool> {
Ok(block.blob_gas_used.is_some())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need for this to return a Result.

Suggested change
pub fn is_active(&self, block: &EthBlock) -> Result<bool> {
Ok(block.blob_gas_used.is_some())
}
pub fn is_active(&self, block: &EthBlock) -> bool {
block.blob_gas_used.is_some()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the same signature of Eip1559::is_active:

pub fn is_active(&self, eth_chain_id: &EthChainId, block_number: U256) -> Result<bool> {

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but that one is fallible since activation numbers aren't known for all EthChainIds. Your version has no such constraint.

pub struct Eip4844 {}

impl Eip4844 {
#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well make this:

Suggested change
#[allow(dead_code)]
#[cfg(test)]

instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That function should be removed because it is not used and it will be not used until 66193ff is reverted.

Replacing with #[cfg(test)], clippy warns us with unused import: ethereum_types::U256 therefore the changes should be

Suggested change
#[allow(dead_code)]
#[cfg(test)]
pub fn get_activation_block_number(&self, eth_chain_id: &EthChainId) -> Result<U256> {
use ethereum_types::U256;

I would prefer to remove the function instead

Copy link
Contributor

@gskapka gskapka Feb 19, 2024

Choose a reason for hiding this comment

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

Once we know the activation block number, we may already be past it so it becomes pointless.

Your way of checking the block's contents is sufficient for us to be ready in advance without having to do anything further.

In fact, it's so sufficient we might as well ditch the activation number thing altogether.

The worse case scenario is an attacker trying to submit a block that's not EIP4844 compliant, but with those fields added, at which point it'd fail validation anyway so I see no danger there.

common/ethereum/src/eip_4844.rs Show resolved Hide resolved
common/ethereum/src/eth_block.rs Show resolved Hide resolved
common/ethereum/src/eth_block.rs Show resolved Hide resolved
common/ethereum/src/eth_block.rs Show resolved Hide resolved
Copy link
Contributor

@gskapka gskapka left a comment

Choose a reason for hiding this comment

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

LGTM!

@Oghma Oghma merged commit 1c8a1ea into master Feb 19, 2024
5 checks passed
@gskapka gskapka deleted the eip4844-block-verification branch February 19, 2024 18:53
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