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

add minimum check in check_gas_limit with unit test #6157

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

tcoratger
Copy link
Contributor

Description

This pull request introduces a minimum gas limit check within the check_gas_limit function, accompanied by a corresponding unit test.

Changes Made

  • Added a check to ensure that the child gas limit does not fall below the minimum allowed limit (MINIMUM_GAS_LIMIT) in the check_gas_limit function.
  • Included a unit test (test_gas_limit_below_limit) to verify that the minimum gas limit check is functioning correctly.

Motivation

Ensuring that the child gas limit does not go below a specified minimum is crucial for maintaining network integrity. This addition helps prevent gas limit values that could potentially cause issues within the Ethereum consensus.

@tcoratger tcoratger requested a review from gakonst as a code owner January 22, 2024 14:57
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm!

pending @Rjected

@mattsse mattsse added C-bug An unexpected or incorrect behavior A-consensus Related to the consensus engine labels Jan 22, 2024
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mattsse mattsse added this pull request to the merge queue Jan 22, 2024
@tcoratger
Copy link
Contributor Author

@mattsse Small question here: as the validate_header_regarding_parent function is made to validate a header in relation to its sealed parent, why not implement this method directly on the Header structure rather than here? Because of the consensus errors that emerge?

Merged via the queue into paradigmxyz:main with commit 3cf0e3a Jan 22, 2024
28 checks passed
@mattsse
Copy link
Collaborator

mattsse commented Jan 22, 2024

good point, technically this doesn't require the parent block and could/should be performed when we validate the header standalone

I also see that we have a few functions that we don't use like full_validation which we could remove.

and honestly, I'm not super happy with the function name inconsistencies like

pub fn validate_4844_header_with_parent(

pub fn validate_header_regarding_parent(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Related to the consensus engine C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants