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

Enable EOF validation before execution in evmc run #768

Merged
merged 1 commit into from
Jan 4, 2024
Merged

Conversation

pdobacz
Copy link
Collaborator

@pdobacz pdobacz commented Jan 3, 2024

Closes #585

Not sure about placement and API. I assumed it should be off by default, and not concern evmc code, this is what I've got as a result.

@pdobacz pdobacz requested a review from chfast January 3, 2024 13:12
@chfast chfast requested review from hugo-dc and gumb0 January 3, 2024 13:16
@chfast chfast added the EOF label Jan 3, 2024
Copy link

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c21b12d) 97.90% compared to head (5a17f73) 97.90%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #768   +/-   ##
=======================================
  Coverage   97.90%   97.90%           
=======================================
  Files         110      110           
  Lines       10536    10542    +6     
=======================================
+ Hits        10315    10321    +6     
  Misses        221      221           
Flag Coverage Δ
blockchaintests 60.23% <20.00%> (-0.15%) ⬇️
statetests 62.31% <40.00%> (-0.04%) ⬇️
statetests-silkpre 26.23% <40.00%> (+<0.01%) ⬆️
unittests 95.89% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
lib/evmone/baseline.cpp 100.00% <100.00%> (ø)
lib/evmone/vm.cpp 100.00% <100.00%> (ø)
lib/evmone/vm.hpp 100.00% <ø> (ø)

@pdobacz pdobacz force-pushed the eof-evmc-run branch 2 times, most recently from ee2dd7a to d905ee9 Compare January 3, 2024 13:58
@pdobacz
Copy link
Collaborator Author

pdobacz commented Jan 3, 2024

I initially missed the possibilities to cover the new lines with tests, but codecov assisted me. It is now done. I'll squash the 2 commits before merge

@chfast
Copy link
Member

chfast commented Jan 3, 2024

I think this is relatively good approach. Any other comments how useful it is in practice?

@pdobacz
Copy link
Collaborator Author

pdobacz commented Jan 4, 2024

I wish there was a natural way to have it "on" by default when running from the CLI, but there is no natural way to set it as default in evmc run, as it treats options opaquely.

One thought I got today is we can have the option "on" be default, but always disable it when running in a controlled environment, when we know revalidation isn't necessary. Setting the option would however be an overhead on each vm.execute and all the clients would need to be made aware of whether or not they're using evmone (e.g. host.cpp). Not good either...

On more thought, the validate_eof=yes option isn't any better or safer than requiring users to run evmone-eofparse beforehand. So maybe the best way to proceed here is just document EOF support with evmone-eofparse?

@pdobacz
Copy link
Collaborator Author

pdobacz commented Jan 4, 2024

Of course, ultimate solution is to add evmc_validate_eof to EVMC and fire it in evmc run whenever an EOF revision is targeted.

We have a bunch of options, WDYT?

@chfast
Copy link
Member

chfast commented Jan 4, 2024

We have a bunch of options, WDYT?

I think EVMC is unnecessary overhead because there are no EVM implementations other than evmone. At some point we should create a evmone run and drop tooling from EVMC.

@pdobacz
Copy link
Collaborator Author

pdobacz commented Jan 4, 2024

We have a bunch of options, WDYT?

I think EVMC is unnecessary overhead because there are no EVM implementations other than evmone. At some point we should create a evmone run and drop tooling from EVMC.

hold on then, if this is the long-term plan, it sounds like a case for the "won't fix" option, let's just document the eofparse.

@chfast
Copy link
Member

chfast commented Jan 4, 2024

hold on then, if this is the long-term plan, it sounds like a case for the "won't fix" option, let's just document the eofparse.

I think this is fine for now. Solidity is using EVMC/evmone so this may be handy for them.

@pdobacz pdobacz merged commit 79eb777 into master Jan 4, 2024
25 checks passed
@pdobacz pdobacz deleted the eof-evmc-run branch January 4, 2024 10:25
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.

EOF validation in tools / tests
2 participants