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.

Sign up for GitHub

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

Jump to bottom

cmd/evm: fix some issues with the evm run command #28109

Merged
merged 4 commits into from
Sep 19, 2023
Merged

cmd/evm: fix some issues with the evm run command #28109

merged 4 commits into from
Sep 19, 2023

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Sep 13, 2023 •
edited
Loading

I was messing around with the EVM last two weeks, and found some issues with evm run that
really make it a bit unpleasant to use.

  • The default configuration had no forks enabled, so modern contracts won't run without a custom genesis file.
  • Handling of flags was a bit broken and required specifying the flags before the command, e.g. evm --debug run. We fixed that a long time ago in geth, but not in the evm command.
  • Every command set up logging, etc. by itself, but we have package internal/debug for that.
  • The assembler had some minor bugs. I started fixing those and but then ended up turning the assembler into a standalone tool outside of go-ethereum. We could probably drop the assembler from go-ethereum, but while it's still here, might as well fix the issues.

@fjl
cmd/evm: improve flags handling
bf5682a 
This fixes some issues with flags in cmd/evm. The supported flags did not
actually show up in help output because they weren't categorized. I'm also
adding the VM-related flags to the run command here so they can be given
after the subcommand name. So it can be run like this now:

   ./evm run --code 6001 --debug
@fjl
cmd/evm: enable all forks by default in run command
1155612 
The default genesis was just empty with no forks at all, which is annoying because
contracts will be relying on opcodes introduced in a fork. So this changes the default to
have all forks enabled.
@fjl
core/asm: fix some issues in the assembler
06ccb10 
This fixes minor bugs in the old assembler:

- It is now possible to have comments on the same line as an instruction.
- Errors for invalid numbers in the jump instruction are reported better
- Line numbers in errors were off by one
holiman
holiman reviewed Sep 14, 2023
View reviewed changes
cmd/evm/runner.go
@@ -236,19 +226,6 @@ func runCmd(ctx *cli.Context) error {
},
}

if cpuProfilePath := ctx.String(CPUProfileFlag.Name); cpuProfilePath != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

How come you decided to remove this? I know it's not "generally useful", but from time to time it's pretty interesting to take a profile of a state-test execution, or e.g when running the snailtracer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The feature is still there, this is just handled by internal/debug now (through debug.Setup in the App.Before hook).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty good you made this comment though, because I noticed we were missing a call to the debug.Exit hook that actually writes the profile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works like this btw:

./evm --pprof.cpuprofile cpu.out run --codefile code.bin

@holiman
Copy link
Contributor

holiman commented Sep 14, 2023 •
edited
Loading

The assembler had some minor bugs. I started fixing those and but then ended up turning the assembler into a standalone tool outside of go-ethereum. We could probably drop the assembler from go-ethereum, but while it's still here, might as well fix the issues.

That geas looks nice, IMO we should drop the assembler from go-ethereum. We haven't updated it in years, and it must be outdated enough that it's safe to drop without breaking anything for anyone (..?)

holiman
holiman approved these changes Sep 14, 2023
View reviewed changes
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM, but: I'd rather we remove the assembler

@fjl
Copy link
Contributor Author

fjl commented Sep 14, 2023

We can do it. Question will be if we should keep evm disasm or also drop it. Probably it should be removed also.

@fjl
Copy link
Contributor Author

fjl commented Sep 14, 2023

Btw the assembler is not outdated at all, because it uses package core/vm to look up instructions. So all the recent opcodes are supported. It is perfectly usable as is, just, the output code is slightly suboptimal.

@fjl fjl merged commit e9f78db into ethereum:master Sep 19, 2023
1 check passed
@fjl fjl added this to the 1.13.2 milestone Sep 19, 2023
phenix3443 pushed a commit to phenix3443/go-ethereum that referenced this pull request Sep 19, 2023
@fjl @phenix3443
cmd/evm: fix some issues with the evm run command (ethereum#28109)
4f64c16 
* cmd/evm: improve flags handling

This fixes some issues with flags in cmd/evm. The supported flags did not
actually show up in help output because they weren't categorized. I'm also
adding the VM-related flags to the run command here so they can be given
after the subcommand name. So it can be run like this now:

   ./evm run --code 6001 --debug

* cmd/evm: enable all forks by default in run command

The default genesis was just empty with no forks at all, which is annoying because
contracts will be relying on opcodes introduced in a fork. So this changes the default to
have all forks enabled.

* core/asm: fix some issues in the assembler

This fixes minor bugs in the old assembler:

- It is now possible to have comments on the same line as an instruction.
- Errors for invalid numbers in the jump instruction are reported better
- Line numbers in errors were off by one
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 12, 2023
@fjl @tyler-smith
cmd/evm: fix some issues with the evm run command (ethereum#28109)
d4f2cff 
* cmd/evm: improve flags handling

This fixes some issues with flags in cmd/evm. The supported flags did not
actually show up in help output because they weren't categorized. I'm also
adding the VM-related flags to the run command here so they can be given
after the subcommand name. So it can be run like this now:

   ./evm run --code 6001 --debug

* cmd/evm: enable all forks by default in run command

The default genesis was just empty with no forks at all, which is annoying because
contracts will be relying on opcodes introduced in a fork. So this changes the default to
have all forks enabled.

* core/asm: fix some issues in the assembler

This fixes minor bugs in the old assembler:

- It is now possible to have comments on the same line as an instruction.
- Errors for invalid numbers in the jump instruction are reported better
- Line numbers in errors were off by one
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 12, 2023
@fjl @tyler-smith
cmd/evm: fix some issues with the evm run command (ethereum#28109)
a16a36b 
* cmd/evm: improve flags handling

This fixes some issues with flags in cmd/evm. The supported flags did not
actually show up in help output because they weren't categorized. I'm also
adding the VM-related flags to the run command here so they can be given
after the subcommand name. So it can be run like this now:

   ./evm run --code 6001 --debug

* cmd/evm: enable all forks by default in run command

The default genesis was just empty with no forks at all, which is annoying because
contracts will be relying on opcodes introduced in a fork. So this changes the default to
have all forks enabled.

* core/asm: fix some issues in the assembler

This fixes minor bugs in the old assembler:

- It is now possible to have comments on the same line as an instruction.
- Errors for invalid numbers in the jump instruction are reported better
- Line numbers in errors were off by one
siosw pushed a commit to fabriqnetwork/go-ethereum that referenced this pull request Oct 16, 2023
@fjl
cmd/evm: fix some issues with the evm run command (ethereum#28109)
ec1e4be 
* cmd/evm: improve flags handling

This fixes some issues with flags in cmd/evm. The supported flags did not
actually show up in help output because they weren't categorized. I'm also
adding the VM-related flags to the run command here so they can be given
after the subcommand name. So it can be run like this now:

   ./evm run --code 6001 --debug

* cmd/evm: enable all forks by default in run command

The default genesis was just empty with no forks at all, which is annoying because
contracts will be relying on opcodes introduced in a fork. So this changes the default to
have all forks enabled.

* core/asm: fix some issues in the assembler

This fixes minor bugs in the old assembler:

- It is now possible to have comments on the same line as an instruction.
- Errors for invalid numbers in the jump instruction are reported better
- Line numbers in errors were off by one
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
@fjl @devopsbo3
cmd/evm: fix some issues with the evm run command (ethereum#28109)
0264a9a 
* cmd/evm: improve flags handling

This fixes some issues with flags in cmd/evm. The supported flags did not
actually show up in help output because they weren't categorized. I'm also
adding the VM-related flags to the run command here so they can be given
after the subcommand name. So it can be run like this now:

   ./evm run --code 6001 --debug

* cmd/evm: enable all forks by default in run command

The default genesis was just empty with no forks at all, which is annoying because
contracts will be relying on opcodes introduced in a fork. So this changes the default to
have all forks enabled.

* core/asm: fix some issues in the assembler

This fixes minor bugs in the old assembler:

- It is now possible to have comments on the same line as an instruction.
- Errors for invalid numbers in the jump instruction are reported better
- Line numbers in errors were off by one
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
@devopsbo3
Revert "cmd/evm: fix some issues with the evm run command (ethereum#2…
2755593 
…8109)"

This reverts commit 0264a9a.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
@devopsbo3
Revert "cmd/evm: fix some issues with the evm run command (ethereum#2…
a41329a 
…8109)"

This reverts commit 0264a9a.
@lightclient lightclient mentioned this pull request Oct 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holiman holiman holiman approved these changes

@karalabe karalabe Awaiting requested review from karalabe karalabe is a code owner

@rjl493456442 rjl493456442 Awaiting requested review from rjl493456442 rjl493456442 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.13.2
Development

Successfully merging this pull request may close these issues.
2 participants
@fjl @holiman