Skip to content

test(evm): un-ignore Trace_invalid_jump{,i}_exception#11799

Merged
LukaszRozmej merged 1 commit into
NethermindEth:masterfrom
0xDevNinja:test/unignore-trace-invalid-jump
May 29, 2026
Merged

test(evm): un-ignore Trace_invalid_jump{,i}_exception#11799
LukaszRozmej merged 1 commit into
NethermindEth:masterfrom
0xDevNinja:test/unignore-trace-invalid-jump

Conversation

@0xDevNinja

@0xDevNinja 0xDevNinja commented May 27, 2026

Copy link
Copy Markdown
Contributor

Changes

  • Trace_invalid_jump_exception and Trace_invalid_jumpi_exception in Nethermind.Evm.Test.VirtualMachineTests were [Ignore]d with the reason "// https://github.com/NethermindEth/nethermind/issues/140". Issue Review and refactor JUMP validation code #140 (Review and refactor JUMP validation code) has been closed for years and the refactor landed — but the tests stayed skipped, so we have had zero coverage of the Geth-trace error field on invalid JUMP / JUMPI instructions.
  • Drop the [Ignore] attribute so the tests run again.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Re-enable previously skipped tests

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

PR is the test re-enablement; no new test added. Verified locally with 15 consecutive standalone runs (all pass) and the full VirtualMachineTests fixture (35 tests) green. CI on this branch will exercise the same.

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

@0xDevNinja
test(evm): un-ignore Trace_invalid_jump{,i}_exception
c2554cb 
Both tests were `[Ignore]`d pointing at NethermindEth#140, which was closed long ago
("Review and refactor JUMP validation code"). The validation refactor
landed, so the tests pass cleanly today.

Verified locally: 15 consecutive standalone runs and the full
`VirtualMachineTests` fixture (35 tests) all green. Drop the
`[Ignore]` attribute so the trace-error coverage for invalid `JUMP` /
`JUMPI` runs in CI again.
@batrr

batrr commented May 29, 2026

Copy link
Copy Markdown
Contributor

@claude review

@claude

claude Bot commented May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @batrr's task in 1m 48s —— View job

Review: test(evm): un-ignore Trace_invalid_jump{,i}_exception

  • Gather context
  • Read changed file
  • Review test logic and correctness
  • Post findings

Clean and correct. The two removed [Ignore] attributes are the only change. Here's what I verified:

EVM implementation correctness — InstructionJump and InstructionJumpIf in EvmInstructions.ControlFlow.cs:84,123 both return EvmExceptionType.InvalidJumpDestination when the destination is not a JUMPDEST. VirtualMachine.HandleFailure calls txTracer.ReportOperationError(errorType), which sets CurrentTraceEntry.Error = "BadJumpDestination" via GethLikeTxTracer.GetErrorDescription (GethLikeTxTracer.cs:54). The pipeline from invalid jump → error entry is intact.

Test bytecodes are correct:

  • Trace_invalid_jump_exception: PUSH1 255; JUMP — jumps to offset 255, which is well past the end of the 3-byte program, so no JUMPDEST there → InvalidJumpDestination ✓
  • Trace_invalid_jumpi_exception: PUSH1 1; PUSH1 255; JUMPI — pops destination (255) then condition (1 ≠ 0), so the jump fires and hits the same invalid destination ✓

Assertion style — trace.Entries.Any(static e => e.Error is not null) is deliberately coarse, consistent with Trace_vm_errors and Trace_memory_out_of_gas_exception in the same class. The tests are named specifically enough that the intent is clear.

No findings. This restores long-missing CI coverage of the Geth error field for JUMP/JUMPI failures.

@LukaszRozmej LukaszRozmej merged commit 045c99c into NethermindEth:master May 29, 2026
822 of 826 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LukaszRozmej LukaszRozmej LukaszRozmej approved these changes

@batrr batrr batrr approved these changes

Assignees

No one assigned

Labels

cleanup evm test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@0xDevNinja @batrr @LukaszRozmej