Skip to content

Conversation

@pdobacz
Copy link
Member

@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 gumb0 and hugo-dc January 3, 2024 13:16
@chfast chfast added the EOF label Jan 3, 2024
@codecov
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
Member 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
Member 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
Member 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
Member 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.

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

3 participants