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

WIP mega eof #29518

Draft
wants to merge 69 commits into
base: master
Choose a base branch
from
Draft

Conversation

MariusVanDerWijden
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden commented Apr 12, 2024

@@ -161,3 +161,24 @@ func parseAndValidate(s string) (*vm.Container, error) {
}
return &c, nil
}

func eofDump(ctx *cli.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

we have cmd/rlpdump and cmd/abidump. How about a cmd/eofdump ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, will move

Copy link
Member Author

Choose a reason for hiding this comment

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

ah @fjl actually said today that he would prefer to have it a subcommand of evm

case op == DUPN:
case op == SWAPN:
arg := int(code[pos+1]) + 1
if want, have := arg, height; want >= have {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it unintuitive that want == have is an error.

@MariusVanDerWijden MariusVanDerWijden force-pushed the mega-eof branch 4 times, most recently from 801d48d to cdd4d1a Compare June 24, 2024 15:31
@holiman
Copy link
Contributor

holiman commented Aug 26, 2024

Is this the latest and greatest eof branch/pr ?

@MariusVanDerWijden MariusVanDerWijden force-pushed the mega-eof branch 2 times, most recently from b516f42 to c8a1db2 Compare September 4, 2024 13:50
core/vm/eips.go Outdated Show resolved Hide resolved
core/vm/eips.go Outdated
if idx, overflow := idx.Uint64WithOverflow(); overflow || idx >= count {
// Index out-of-bounds, don't branch, just skip over immediate
// argument.
*pc += 1 + count*2
Copy link
Contributor

Choose a reason for hiding this comment

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

What is count is 0 -- won't this start executing on the immediate ? The spec says to pc += (max_index + 1) * 2, that is something like

		*pc += 1 + (count+1)*2

???

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'm doing the right thing here, because we still have the +1 from the interpreter loop

core/vm/eips.go Outdated Show resolved Hide resolved
core/vm/eips.go Outdated Show resolved Hide resolved
backwards = worklist[idx].backwards
)
worklist = worklist[:idx]
outer:
Copy link
Contributor

Choose a reason for hiding this comment

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

Whenever we have these loops and labels and break outer, it's IMO usually better to branch off a method. So instead of

for ....
  outer: 
    for .. 
      switch 
         break outer
         ....
      break outer
 bazonk()

we do

for ....
  inner()
 bazonk()


func inner(){
    for .. 
      switch 
         return //break outer
         ....
      return // break outer
}

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, this is old unused code. Please just nuke it, if we're not using it.

holiman and others added 4 commits September 8, 2024 12:39
Fixes a few EOF validation issues in EEST and found by fuzzing.
* subcontainers header with zero subcontainers is invalid
* returning code sections require at least one RETF or qualified JUMPF
* RETF stack must have stack enough for outputs
* JUMPF must not jump into a section with more outputs
* Check stack minimums on RJIMPI and RJUMPV
Comment on lines +1117 to +1128
INVALID: {
execute: opUndefined,
minStack: minStack(0, 0),
maxStack: maxStack(0, 0),
terminal: true,
},
}

// Fill all unassigned slots with opUndefined.
for i, entry := range tbl {
if entry == nil {
tbl[i] = &operation{execute: opUndefined, maxStack: maxStack(0, 0)}
tbl[i] = &operation{execute: opUndefined, maxStack: maxStack(0, 0), undefined: true}
Copy link
Contributor

Choose a reason for hiding this comment

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

The way this is defined, makes it so that a code-container containing nothing but the FE (invalid) opcode is a valid container.

$ go run ./cmd/eofdump/  eof --hex 0xef000101000402000100010400000000800000fe
OK

That is, it behaves just like STOP:

[user@work go-ethereum]$ go run ./cmd/eofdump/  eof --hex 0xef00010100040200010001040000000080000000
OK

But different from an actual undefined opcode, like EF:

[user@work go-ethereum]$ go run ./cmd/eofdump/  eof --hex 0xef000101000402000100010400000000800000ef
err(15): undefined instruction
exit status 1

This seems wrong to me. @shemnon what do you say ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It appears that this is covered by at least one of the common consensus-tests for eof. It seems very weird that we have one "blessed" undefined which acts differently from the actual undefined. Afaik, the idea with the designated undefined was that it will be left undefined, forever -- as opposed to non-designated undefineds, which may become used in the future.

Copy link
Contributor

@holiman holiman Sep 11, 2024

Choose a reason for hiding this comment

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

The spec does mention this:https://github.com/ipsilon/eof/blob/main/spec/eof.md#stack-validation .

  • Terminating instructions:

    • ending function execution: RETF, JUMPF,
    • ending whole EVM execution: STOP, RETURN, RETURNCONTRACT, REVERT, INVALID.

I think this behaviour is utterly incomprehensible. Would love to know a bit more about the thoughts behind it. Why is it useful to allow INVALID?

IMO doing so violates the agreement/idea behind INVALID, by turning it into a unique opcode with a special purpose, as opposed to "one in the general soup of invalid/undefined operations".

Copy link
Contributor

Choose a reason for hiding this comment

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

The space that INVALID occupies is REVERT with penalties. We could easily replace it with PUSH0/PUSH0/REVERT but then all gas is not consumed. I think it is important that EOF have access to a "halt and catch fire" instruction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for the explanation. Re-reading what I wrote -- sorry if I sounded overly aggressive !

... 🍰

shemnon and others added 12 commits September 11, 2024 17:30
Fixes fill of opcode validiiy tests
* use corret stack heights for value and address in EXTCALL
* New extCallGas function that handles min retained and min callee gas
* Handle min gas failures via tempCallGas == 0
* remove stipend refund for EXT*CALL
* Call Stack too deep should return 1
Handle Aux Data in EOF Create w/o validation failures
Allow Create Transactions
Some corner cases for create
* Don't consume all gas with an invalid EOF contract
* don't allow CREATE/CREATE2 opcodes to create EOF
Co-authored-by: Marius van der Wijden <[email protected]>
holiman added a commit that referenced this pull request Oct 2, 2024
The bulk of this PR is authored by @lightclient , in the original
EOF-work. More recently, the code has been picked up and reworked for the new EOF
specification, by @MariusVanDerWijden , in #29518, and also @shemnon has contributed with fixes.

This PR is an attempt to start eating the elephant one small bite at a
time, by selecting only the eof-validation as a standalone piece which
can be merged without interfering too much in the core stuff.

In this PR: 

- [x] Validation of eof containers, lifted from #29518, along with
test-vectors from consensus-tests and fuzzing, to ensure that the move
did not lose any functionality.
- [x] Definition of eof opcodes, which is a prerequisite for validation
- [x] Addition of `undefined` to a jumptable entry item. I'm not
super-happy with this, but for the moment it seems the least invasive
way to do it. A better way might be to go back and allowing nil-items or
nil execute-functions to denote "undefined".
- [x] benchmarks of eof validation speed 


---------

Co-authored-by: lightclient <[email protected]>
Co-authored-by: Marius van der Wijden <[email protected]>
Co-authored-by: Danno Ferrin <[email protected]>
islishude pushed a commit to islishude/go-ethereum that referenced this pull request Oct 6, 2024
The bulk of this PR is authored by @lightclient , in the original
EOF-work. More recently, the code has been picked up and reworked for the new EOF
specification, by @MariusVanDerWijden , in ethereum#29518, and also @shemnon has contributed with fixes.

This PR is an attempt to start eating the elephant one small bite at a
time, by selecting only the eof-validation as a standalone piece which
can be merged without interfering too much in the core stuff.

In this PR: 

- [x] Validation of eof containers, lifted from ethereum#29518, along with
test-vectors from consensus-tests and fuzzing, to ensure that the move
did not lose any functionality.
- [x] Definition of eof opcodes, which is a prerequisite for validation
- [x] Addition of `undefined` to a jumptable entry item. I'm not
super-happy with this, but for the moment it seems the least invasive
way to do it. A better way might be to go back and allowing nil-items or
nil execute-functions to denote "undefined".
- [x] benchmarks of eof validation speed 


---------

Co-authored-by: lightclient <[email protected]>
Co-authored-by: Marius van der Wijden <[email protected]>
Co-authored-by: Danno Ferrin <[email protected]>
holiman added a commit that referenced this pull request Nov 19, 2024
The bulk of this PR is authored by @lightclient , in the original
EOF-work. More recently, the code has been picked up and reworked for the new EOF
specification, by @MariusVanDerWijden , in #29518, and also @shemnon has contributed with fixes.

This PR is an attempt to start eating the elephant one small bite at a
time, by selecting only the eof-validation as a standalone piece which
can be merged without interfering too much in the core stuff.

In this PR: 

- [x] Validation of eof containers, lifted from #29518, along with
test-vectors from consensus-tests and fuzzing, to ensure that the move
did not lose any functionality.
- [x] Definition of eof opcodes, which is a prerequisite for validation
- [x] Addition of `undefined` to a jumptable entry item. I'm not
super-happy with this, but for the moment it seems the least invasive
way to do it. A better way might be to go back and allowing nil-items or
nil execute-functions to denote "undefined".
- [x] benchmarks of eof validation speed 


---------

Co-authored-by: lightclient <[email protected]>
Co-authored-by: Marius van der Wijden <[email protected]>
Co-authored-by: Danno Ferrin <[email protected]>
zfy0701 pushed a commit to sentioxyz/go-ethereum that referenced this pull request Dec 3, 2024
The bulk of this PR is authored by @lightclient , in the original
EOF-work. More recently, the code has been picked up and reworked for the new EOF
specification, by @MariusVanDerWijden , in ethereum#29518, and also @shemnon has contributed with fixes.

This PR is an attempt to start eating the elephant one small bite at a
time, by selecting only the eof-validation as a standalone piece which
can be merged without interfering too much in the core stuff.

In this PR: 

- [x] Validation of eof containers, lifted from ethereum#29518, along with
test-vectors from consensus-tests and fuzzing, to ensure that the move
did not lose any functionality.
- [x] Definition of eof opcodes, which is a prerequisite for validation
- [x] Addition of `undefined` to a jumptable entry item. I'm not
super-happy with this, but for the moment it seems the least invasive
way to do it. A better way might be to go back and allowing nil-items or
nil execute-functions to denote "undefined".
- [x] benchmarks of eof validation speed 


---------

Co-authored-by: lightclient <[email protected]>
Co-authored-by: Marius van der Wijden <[email protected]>
Co-authored-by: Danno Ferrin <[email protected]>
vikinatora pushed a commit to PufferFinance/unifi-geth that referenced this pull request Jan 13, 2025
The bulk of this PR is authored by @lightclient , in the original
EOF-work. More recently, the code has been picked up and reworked for the new EOF
specification, by @MariusVanDerWijden , in ethereum#29518, and also @shemnon has contributed with fixes.

This PR is an attempt to start eating the elephant one small bite at a
time, by selecting only the eof-validation as a standalone piece which
can be merged without interfering too much in the core stuff.

In this PR:

- [x] Validation of eof containers, lifted from ethereum#29518, along with
test-vectors from consensus-tests and fuzzing, to ensure that the move
did not lose any functionality.
- [x] Definition of eof opcodes, which is a prerequisite for validation
- [x] Addition of `undefined` to a jumptable entry item. I'm not
super-happy with this, but for the moment it seems the least invasive
way to do it. A better way might be to go back and allowing nil-items or
nil execute-functions to denote "undefined".
- [x] benchmarks of eof validation speed

---------

Co-authored-by: lightclient <[email protected]>
Co-authored-by: Marius van der Wijden <[email protected]>
Co-authored-by: Danno Ferrin <[email protected]>
vikinatora pushed a commit to PufferFinance/unifi-geth that referenced this pull request Jan 13, 2025
The bulk of this PR is authored by @lightclient , in the original
EOF-work. More recently, the code has been picked up and reworked for the new EOF
specification, by @MariusVanDerWijden , in ethereum#29518, and also @shemnon has contributed with fixes.

This PR is an attempt to start eating the elephant one small bite at a
time, by selecting only the eof-validation as a standalone piece which
can be merged without interfering too much in the core stuff.

In this PR: 

- [x] Validation of eof containers, lifted from ethereum#29518, along with
test-vectors from consensus-tests and fuzzing, to ensure that the move
did not lose any functionality.
- [x] Definition of eof opcodes, which is a prerequisite for validation
- [x] Addition of `undefined` to a jumptable entry item. I'm not
super-happy with this, but for the moment it seems the least invasive
way to do it. A better way might be to go back and allowing nil-items or
nil execute-functions to denote "undefined".
- [x] benchmarks of eof validation speed 


---------

Co-authored-by: lightclient <[email protected]>
Co-authored-by: Marius van der Wijden <[email protected]>
Co-authored-by: Danno Ferrin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants