Skip to content

Comments

fix(bytecode): improve analyze_legacy and stack tests#3335

Merged
rakita merged 5 commits intomainfrom
jumpa
Jan 20, 2026
Merged

fix(bytecode): improve analyze_legacy and stack tests#3335
rakita merged 5 commits intomainfrom
jumpa

Conversation

@rakita
Copy link
Member

@rakita rakita commented Jan 20, 2026

Summary

  • Improve analyze_legacy padding logic for SWAPN/DUPN/EXCHANGE edge cases using prev_opcode tracking
  • Simplify the main while loop to only handle PUSH opcodes
  • Update stack.rs tests for new Interpreter API

Test plan

  • Existing tests pass including new edge case tests for SWAPN/DUPN/EXCHANGE with STOP

Track both previous and current opcode in the analysis loop to handle
edge cases where opcodes with immediates (SWAPN, DUPN, EXCHANGE) have
their immediate byte appear as STOP. For example, [SWAPN, STOP] needs
padding because the STOP is consumed as the immediate operand, not as
an actual instruction.

Add test to verify [SWAPN, STOP] is padded to [SWAPN, STOP, STOP].
Remove obsolete GasParams import and argument, use DummyHost::new()
constructor instead of unit struct syntax.
Remove dupn_offset calculation from the main loop, keeping only PUSH
handling. The SWAPN/DUPN/EXCHANGE padding logic via prev_opcode is
retained for edge case handling.
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 20, 2026

Merging this PR will not alter performance

✅ 173 untouched benchmarks


Comparing jumpa (f15ff0e) with main (9016fd0)

Open in CodSpeed

These opcodes have 1-byte immediates that aren't handled by the main
analysis loop, so we need extra padding to ensure safe execution
regardless of whether the bytecode ends with STOP.
@rakita
Copy link
Member Author

rakita commented Jan 20, 2026

This fixed the inconsistency in padding that immediates would do

@rakita rakita merged commit 63ad58e into main Jan 20, 2026
30 checks passed
Comment on lines +206 to +210
let bytecode = vec![op];
let (_, padded_bytecode) = analyze_legacy(bytecode.into());
assert_eq!(padded_bytecode.len(), 2);
assert_eq!(padded_bytecode[0], op);
assert_eq!(padded_bytecode[1], opcode::STOP);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this right? This corresponds to a single instruction. My understanding was you always want a final STOP instruction. In that case, you'd expect 2 bytes of padding where the first is an immediate and the second is an instruction.

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.

2 participants