Skip to content

Fix: Blocktest validation bypass and exception handling logic#9439

Closed
bshastry wants to merge 6 commits into
NethermindEth:masterfrom
bshastry:fix/blocktest-validation-bypass
Closed

Fix: Blocktest validation bypass and exception handling logic#9439
bshastry wants to merge 6 commits into
NethermindEth:masterfrom
bshastry:fix/blocktest-validation-bypass

Conversation

@bshastry

@bshastry bshastry commented Oct 10, 2025

Copy link
Copy Markdown
Contributor

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.

@bshastry

Copy link
Copy Markdown
Contributor Author

Related: #9438

Just realized after making a PR that a similar PR fixes the issue. Please feel free to close this PR in favour of the linked PR 🙏

This commit addresses two critical issues in blockchain test validation:

1. Validation Bypass Vulnerability:
   - Removed the `!test.SealEngineUsed ||` condition that allowed blocks
     to skip validation entirely when using NoProof seal engine
   - Now all blocks undergo proper consensus rule validation regardless
     of seal engine type (seal validation itself remains conditional)

2. Inverted Exception Logic:
   - Fixed inverted null checks on `ExpectedException` in both validation
     failure and exception handling paths
   - Added explicit validation pass/fail checks to catch blocks that
     unexpectedly pass when they should fail
   - Improved error messages to include actual validation error details
   - Added explanatory comments documenting expected behavior

The validation framework now correctly:
- Validates all blocks through the consensus rule validator
- Fails tests when blocks unexpectedly pass validation
- Fails tests when blocks unexpectedly fail validation
- Properly handles expected failures via both validation and exceptions

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@bshastry bshastry force-pushed the fix/blocktest-validation-bypass branch from 1a78d3a to 3906966 Compare October 15, 2025 09:03
@bshastry bshastry changed the title Fix: Blocktest validation bypass for NoProof seal engine Fix: Blocktest validation bypass and exception handling logic Oct 15, 2025
@LukaszRozmej

Copy link
Copy Markdown
Member

@bshastry decided to merge the related PR. Your PR still has the ExpectedException part. Do you want to update it so we can merge it or we should close it?

@LukaszRozmej

Copy link
Copy Markdown
Member

I resolved conflicts and simplified asserts

Comment thread src/Nethermind/Ethereum.Test.Base/JsonToEthereumTest.cs
}

// Parse seal engine - fixes bug where NoProof tests bypassed validation
test.SealEngineUsed = testJson.SealEngine switch

@LukaszRozmej LukaszRozmej Oct 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@flcl42 the PR: https://github.com/NethermindEth/nethermind/pull/9438/files#diff-3817f3ceecf8e386796bb85e5a12d923d16d6bbe36e57cadd460a39fc7ed3cb7L197

removed the only usage of this flag. Should we remove it altogether or bring it back?

@flcl42 flcl42 Oct 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That usage was not working well, so we deleted it yes. But we inject in tests and always use null seal validator, so there are two ways: 1. we could inject proper seal validator depending on this flag, in proper way on per-test basis 2. we can drop it fully indeed 3. we keep it and don't do nothing, because it presents in test jsons, and we don't yet care, but it can change later

@LukaszRozmej

Copy link
Copy Markdown
Member

closing in favor of #9491

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.

3 participants