Fix: Blocktest validation bypass and exception handling logic#9439
Fix: Blocktest validation bypass and exception handling logic#9439bshastry wants to merge 6 commits into
Conversation
|
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>
1a78d3a to
3906966
Compare
|
@bshastry decided to merge the related PR. Your PR still has the |
|
I resolved conflicts and simplified asserts |
| } | ||
|
|
||
| // Parse seal engine - fixes bug where NoProof tests bypassed validation | ||
| test.SealEngineUsed = testJson.SealEngine switch |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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
|
closing in favor of #9491 |
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.csline 190 had this logic:When
SealEngineUsedwas 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:2. Inverted Exception Handling Logic
The exception handling logic had inverted null checks on
ExpectedException:Validation failure path (line 205):
This caused tests to fail when blocks were expected to be invalid but correctly failed validation.
Exception handler path (line 213):
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
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:
Fixed validation failure check:
Fixed exception handler check:
3. Enhanced Error Reporting
4. Parse sealEngine Field Correctly
JsonToEthereumTest.csnow properly parses the sealEngine field from test JSON to set theSealEngineUsedflag.5. Added Comprehensive Regression Tests
HeaderValidatorTests.csincludes tests that verify:Type of change
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
Testing
Unit Tests Added
BaseFee_validation_must_not_be_bypassed_for_NoProof_seal_engine- Verifies invalid baseFee is rejected even with NoProofValid_baseFee_with_NoProof_seal_engine_should_pass- Verifies valid blocks still pass with NoProofManual Testing
Impact on Test Results
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:
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:
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.