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(primitives): move header validation methods from consensus to primitive #9207

Open
tcoratger opened this issue Jun 30, 2024 · 3 comments
Labels
C-enhancement New feature or request S-needs-triage This issue needs to be labelled S-stale This issue/PR is stale and will close with no further activity

Comments

@tcoratger
Copy link
Contributor

Describe the feature

At the moment, in this file:

https://github.com/paradigmxyz/reth/blob/2a9fa4869e658ca89e050d0d985a7ccbfa360bfa/crates/consensus/common/src/validation.rs

we have a lot of header validation methods like this one:

/// Gas used needs to be less than gas limit. Gas used is going to be checked after execution.
#[inline]
pub fn validate_header_gas(header: &SealedHeader) -> Result<(), ConsensusError> {
if header.gas_used > header.gas_limit {
return Err(ConsensusError::HeaderGasUsedExceedsGasLimit {
gas_used: header.gas_used,
gas_limit: header.gas_limit,
})
}
Ok(())
}

I have the impression that rather than keeping these methods here, it would make more sense to integrate them into the SealedHeader implementation with a specific error type HeaderConsensusError which would group together all possible errors when validating the sealed headers and which would then be passed transparently to the ConsensusError.

This would make it possible to make the implementation more structural and to use:

header.validate_gas();

for example.

Additional context

No response

@tcoratger tcoratger added C-enhancement New feature or request S-needs-triage This issue needs to be labelled labels Jun 30, 2024
@Villegas2003
Copy link

Hi @tcoratger , Are you still in need of support for this issue? I'd be happy to help.

@tcoratger
Copy link
Contributor Author

Hi @tcoratger , Are you still in need of support for this issue? I'd be happy to help.

@Villegas2003 Before you start working on it, I don't know if this proposal will be accepted, wdyt @mattsse ?

Copy link
Contributor

This issue is stale because it has been open for 21 days with no activity.

@github-actions github-actions bot added the S-stale This issue/PR is stale and will close with no further activity label Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request S-needs-triage This issue needs to be labelled S-stale This issue/PR is stale and will close with no further activity
Projects
Status: Todo
Development

No branches or pull requests

2 participants