Fix: Blocktest exception handling logic

[LukaszRozmej]

Based on and replaces #9439

Fix: Blocktest validation bypass and exception handling logic

Description

This PR fixes two critical bugs in blockchain test validation that allowed invalid blocks to pass tests and caused incorrect test failure detection.

Problems

1. Validation Bypass Vulnerability

The bug in BlockchainTestBase.cs line 190 had this logic:

if (!test.SealEngineUsed || blockValidator.ValidateSuggestedBlock(...))

When SealEngineUsed was false (NoProof seal engine), all validation was completely bypassed, allowing blocks with invalid consensus parameters (baseFee, gasLimit, etc.) to be accepted. This violates Ethereum consensus rules because:

• NoProof should only skip seal/PoW validation
• Consensus rules like baseFee, gas limits, and other header validations MUST still be enforced
• The seal validator framework already handles conditional seal validation internally

2. Inverted Exception Handling Logic

The exception handling logic had inverted null checks on ExpectedException:

Validation failure path (line 205):

if (correctRlp[i].ExpectedException is not null)  // WRONG: inverted logic
{
    Assert.Fail($"Unexpected invalid block {correctRlp[i].Block.Hash}");
}

This caused tests to fail when blocks were expected to be invalid but correctly failed validation.

Exception handler path (line 213):

if (correctRlp[i].ExpectedException is not null)  // WRONG: inverted logic
{
    Assert.Fail($"Unexpected invalid block {correctRlp[i].Block.Hash}: {e}");
}

This caused tests to fail when blocks were expected to throw exceptions.

Missing validation pass check:
There was no check to detect when a block unexpectedly passes validation when it should have failed.

Solution

1. Removed Validation Bypass

// Before (vulnerable):
if (!test.SealEngineUsed || blockValidator.ValidateSuggestedBlock(...))

// After (secure):
if (blockValidator.ValidateSuggestedBlock(correctRlp[i].Block, parentHeader, out var _error))

Now all blocks undergo proper consensus rule validation regardless of seal engine type. The seal validation itself remains conditional within the validator framework.

2. Fixed Exception Handling Logic

Added validation pass check:

if (blockValidator.ValidateSuggestedBlock(...))
{
    // Validation PASSED
    if (correctRlp[i].ExpectedException is not null)
    {
        Assert.Fail($"Expected block to fail with '{ExpectedException}', but it passed");
    }
    blockTree.SuggestBlock(correctRlp[i].Block);
}

Fixed validation failure check:

else
{
    // Validation FAILED
    if (correctRlp[i].ExpectedException is null)  // FIXED: now correct
    {
        Assert.Fail($"Unexpected invalid block: {_error}");
    }
    // else: Expected to fail and did fail → correct behavior
}

Fixed exception handler check:

catch (InvalidBlockException e)
{
    // Exception thrown during block processing
    if (correctRlp[i].ExpectedException is null)  // FIXED: now correct
    {
        Assert.Fail($"Unexpected invalid block: {e}");
    }
    // else: Expected to fail and did fail via exception → correct behavior
}

3. Enhanced Error Reporting

• Validation errors now include actual error details from the validator
• Clear comments document expected behavior in each code path
• Better diagnostic messages for debugging test failures

4. Parse sealEngine Field Correctly

JsonToEthereumTest.cs now properly parses the sealEngine field from test JSON to set the SealEngineUsed flag.

5. Added Comprehensive Regression Tests

HeaderValidatorTests.cs includes tests that verify:

• Invalid baseFee is rejected even with NoProof seal engine
• Valid blocks still pass with NoProof seal engine
• Expected test failures are handled correctly

Type of change

• Bug fix (non-breaking change which fixes critical security and correctness issues)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Impact

Before This Fix:

❌ Blocks with invalid consensus parameters were accepted when using NoProof seal engine
❌ Tests incorrectly failed when blocks were expected to be invalid
❌ No detection of blocks that unexpectedly pass validation
❌ Poor error messages without validation details

After This Fix:

✅ All blocks properly validate consensus rules regardless of seal engine
✅ Tests correctly handle expected failures via validation and exceptions
✅ Tests fail when blocks unexpectedly pass or fail validation
✅ Detailed error messages aid in debugging
✅ Better alignment with Ethereum consensus requirements
✅ Improved test reliability and correctness

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

Testing

Unit Tests Added

• BaseFee_validation_must_not_be_bypassed_for_NoProof_seal_engine - Verifies invalid baseFee is rejected even with NoProof
• Valid_baseFee_with_NoProof_seal_engine_should_pass - Verifies valid blocks still pass with NoProof

Manual Testing

• Ran Ethereum Foundation blockchain tests with NoProof seal engine
• Verified tests with expected failures now correctly pass
• Verified invalid blocks are correctly rejected
• Confirmed no regressions in existing test suites

Impact on Test Results

• Previously passing invalid tests will now correctly fail (security improvement)
• Previously incorrectly failing tests will now correctly pass (correctness improvement)
• Better alignment with reference implementations (Geth, Besu, etc.)

Files Changed

• src/Nethermind/Ethereum.Test.Base/BlockchainTestBase.cs - Fix validation bypass and exception handling logic (+19 lines, -4 lines)
• src/Nethermind/Ethereum.Test.Base/JsonToEthereumTest.cs - Parse sealEngine field (+9 lines)
• src/Nethermind/Nethermind.Blockchain.Test/Validators/HeaderValidatorTests.cs - Add comprehensive regression tests (+116 lines)

Total Changes: 3 files changed, 140 insertions(+), 4 deletions(-)

Security Considerations

This PR addresses a critical security vulnerability where consensus rules could be bypassed. The validation bypass allowed:

• Invalid baseFee values to be accepted
• Invalid gas limits to be accepted
• Other consensus rule violations to go undetected

This fix ensures all consensus rules are properly enforced during blockchain testing, improving the reliability of Nethermind's validation logic.

Related Issues

Fixes validation bypass bug and exception handling logic errors discovered during fuzzing where:

1. Blocks with invalid baseFee were incorrectly accepted when sealEngine was set to "NoProof"
2. Tests with expected failures were incorrectly reporting failures
3. Blocks that unexpectedly passed validation were not being caught

Additional Notes

The seal validation framework already has internal logic to conditionally validate seals based on the seal engine type. The outer bypass condition was redundant and created a security hole. This fix removes that bypass while maintaining the correct seal validation behavior through the validator framework's internal logic.

[Copilot]

Pull Request Overview

This PR fixes critical bugs in blockchain test validation that allowed invalid blocks to bypass consensus rule validation and caused incorrect test failure detection. The fix addresses security vulnerabilities where blocks with invalid baseFee, gasLimit, and other consensus parameters were being accepted when using NoProof seal engines.

Key changes:

• Removed validation bypass logic that completely skipped consensus rule validation for NoProof seal engines
• Fixed inverted exception handling logic that caused tests to fail when blocks were expected to be invalid
• Enhanced error reporting with detailed validation messages and comprehensive test coverage

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/Nethermind/Nethermind.Core/BlockHeader.cs	Removed unused SealEngineType property from BlockHeader class
src/Nethermind/Nethermind.Blockchain.Test/Validators/HeaderValidatorTests.cs	Removed SealEngineType assignments from existing tests and added comprehensive regression tests for baseFee validation with NoProof seal engines
src/Nethermind/Ethereum.Test.Base/BlockchainTestBase.cs	Fixed critical validation bypass bug and corrected inverted exception handling logic with improved error reporting
src/Nethermind/Ethereum.Test.Base/BlockchainTest.cs	Removed unused SealEngineUsed property from BlockchainTest class