Harden Orphaned Block validations #9389
Conversation
LukaszRozmej
changed the title
LukaszRozmej
requested review from
MarekM25,
ak88,
asdacap,
benaadams,
damian-orzechowski,
deffrian,
emlautarom1,
flcl42,
kamilchodola,
marcindsobczak,
smartprogrammer93 and
tanishqjasoria
Marchhill
requested review from
Marchhill and
asdacap
and removed request for
ak88
There was a problem hiding this comment.
Pull Request Overview
This PR hardens orphaned block validations by improving validation consistency between regular blocks and orphaned blocks. The main purpose is to ensure that orphaned block validation (used in operations like NewPayload and EraSync) receives proper validation coverage.
- Unified validation paths between regular and orphaned blocks to prevent missed validations
- Added a new
ValidateOrphanmethod toIHeaderValidatorinterface - Made parent parameters non-optional in various validation methods for consistency
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Nethermind/Nethermind.Consensus/Validators/HeaderValidator.cs | Core validation logic refactored to use generic type flags for orphaned vs regular blocks |
| src/Nethermind/Nethermind.Consensus/Validators/BlockValidator.cs | Block validation unified with orphaned validation using type flags |
| src/Nethermind/Nethermind.Consensus/Validators/IHeaderValidator.cs | Added ValidateOrphaned method to interface |
| src/Nethermind/Nethermind.Consensus/Validators/IBlockValidator.cs | Updated signatures to make parent parameter non-optional |
| src/Nethermind/Nethermind.Core/TypeFlags.cs | Fixed interface method order for virtual static properties |
| Various validator implementations | Updated to match new interface signatures and use type flags |
| Test files | Updated to use new non-optional parent parameters and simplified test code |
Marchhill
left a comment
Marchhill
left a comment
There was a problem hiding this comment.
Looks good, only thing is whether to use this template approach to specify an orphaned blocks. It's fine but seems a bit less clear to me than having another argument and overloading
| if (validateHashes) | ||
| private bool ValidateBlockSize(Block block, IReleaseSpec spec, ref string? errorMessage) | ||
| { | ||
| int encodedSize = block.EncodedSize ?? _blockDecoder.GetLength(block, RlpBehaviors.None); |
There was a problem hiding this comment.
small optimisation: could calculate the encoded size only if Eip7934 is enabled
| protected readonly ISpecProvider _specProvider = specProvider ?? throw new ArgumentNullException(nameof(specProvider)); | ||
| private readonly long? _daoBlockNumber = specProvider.DaoBlockNumber; | ||
| protected readonly ILogger _logger = logManager?.GetClassLogger() ?? throw new ArgumentNullException(nameof(logManager)); | ||
| private readonly IBlockTree _blockTree = blockTree ?? throw new ArgumentNullException(nameof(blockTree)); |
There was a problem hiding this comment.
Since we dont get parent blocks anymore, maybe we can just pass genesis difficulty instead, and remove this dependency
There was a problem hiding this comment.
We could, not necessarily in this PR. @asdacap could you help with how to set it up in DI?
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
Just one comment on OP stack.
|
ok this is enough approvals for me |
We have something like
ValidateOrphanedBlockused in some occasions inNewPayloadandEraSync.This validations were severely lacking. Updating them to have as much validations on BlockBody (basically all) and Header (the ones that don't need parent). Keep as much of the code reusable to force future changes to think of both cases.
Changes
IHeaderValidator.ValidateOrphanmethodHeaderValidatorandBlockValidatorwithOnFlagfor orphan to not miss orphan validation logic when adding new validations and in derived classes.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?