Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new(tests): Add EOF validation tests for stack underflow #1331

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chfast
Copy link
Member

@chfast chfast commented Mar 19, 2025

πŸ—’οΈ Description

Port ethereum/tests EOF validation tests for stack underflow.

πŸ”— Related Issues

βœ… Checklist

@chfast chfast requested review from hugo-dc, gumb0 and pdobacz March 19, 2025 12:05
@chfast chfast added the scope:tests Scope: Changes EL client test cases in `./tests` label Mar 19, 2025
@chfast chfast force-pushed the eof/stack_underflow branch from f49311a to c99f0b2 Compare March 19, 2025 12:06
Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

A few comments, thanks!

sections=[
Section.Code(
code=Op.CALLF[1] + Op.STOP,
max_stack_height=1,
Copy link
Member

Choose a reason for hiding this comment

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

I would add a duplicate to this case but with max_stack_height=2, just for it to match the code_outputs=2 of the second section.

sections=[
Section.Code(
code=Op.PUSH0
+ Op.PUSH1[0]
Copy link
Member

Choose a reason for hiding this comment

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

Just wanted to confirm that the purpose of this test is to have an underflow in both branches of the RJUMPI, in contrast to the one below where only one of the branches has the underflow.

+ Op.PUSH0
+ Op.CALLF[1]
+ Op.STOP,
max_stack_height=4,
Copy link
Member

Choose a reason for hiding this comment

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

Same comment from first test, we could add a max_stack_height=5 variant.

Comment on lines +620 to +626
code=Op.PUSH0
+ Op.PUSH1[0]
+ Op.RJUMPI[2]
+ Op.PUSH0
+ Op.PUSH0
+ Op.CALLF[1]
+ Op.STOP,
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth it to add another test case where the underflow happens in the no-jump branch:

code=Op.PUSH0
                    + Op.PUSH1[0]
                    + Op.PUSH1[0]
                    + Op.PUSH1[0]
                    + Op.RJUMPI[1]
                    + Op.POP
                    + Op.CALLF[1]
                    + Op.STOP,

name="underflow_0",
sections=[
Section.Code(
code=Op.ADD + Op.STOP,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
code=Op.ADD + Op.STOP,
code=Op.POP + Op.STOP,

We could also add another test that uses an opcode that barely produces the underflow vs one that underflows by two items (and pushes one).

max_stack_height could also use more variants here, e.g. 1 and 2 in this case, and 0 and 1 in the case of Op.POP for example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we just iterate the opcodes and use each and every one which requires non-zero stack? Can also boost this to do inputs - 1 PUSH0s preceding the offending opcode and test all

Copy link
Member Author

Choose a reason for hiding this comment

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

This is exactly what I wanted to do later. But as it was already noted, I will do it up front and then we can remove this test.

The exhaustive test should take all opcodes with static non-zero stack height required?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The exhaustive test should take all opcodes with static non-zero stack height required?

ah yes, zero-stack height required cannot overflow. I've meant to say you can either test underflow with 0 items or with < inputs items on stack

+ Op.PUSH0
+ Op.PUSH0
+ Op.ADD
+ Op.STOP,
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as in previous file, we could add a variant that underflows in the no-jump case.

name="underflow_variable_stack_7",
sections=[
Section.Code(
code=Op.PUSH0 + Op.PUSH1[0] + Op.RJUMPI[2] + Op.PUSH0 + Op.PUSH0 + Op.JUMPF[1],
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as previous files, we could add a variant where the underflow happens in the no-jump branch.

Copy link
Collaborator

@pdobacz pdobacz left a comment

Choose a reason for hiding this comment

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

I second Mario's comments + one suggestion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:tests Scope: Changes EL client test cases in `./tests`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants