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

host: Make EOF opaque for EXTCODE* instructions #587

Merged
merged 5 commits into from
Mar 13, 2024
Merged

Conversation

axic
Copy link
Member

@axic axic commented Mar 18, 2023

No description provided.

@chfast chfast force-pushed the eof branch 2 times, most recently from cf08fe5 to 8891cbc Compare March 21, 2023 08:09
Base automatically changed from eof to master March 21, 2023 12:02
@axic
Copy link
Member Author

axic commented Mar 21, 2023

@chfast @gumb0 any feedback on the direction (before I start adding tests / more code)?

test/state/host.cpp Outdated Show resolved Hide resolved
}

bytes32 Host::get_code_hash(const address& addr) const noexcept
{
// TODO: Cache code hash. It will be needed also to compute the MPT hash.
const auto* const acc = m_state.find(addr);
return (acc != nullptr && !acc->is_empty()) ? keccak256(acc->code) : bytes32{};
return (acc != nullptr && !acc->is_empty()) ? keccak256(extcode(acc->code)) : bytes32{};
Copy link
Member

Choose a reason for hiding this comment

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

Patching EXTCODEHASH will be a headache for real implementations. The code hash is available directly in the account node. Now you need to additionally load the code to obtain a possibly different hash.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now you need to additionally load the code to obtain a possibly different hash.

This was always the case with this approach. Need to figure out if it is an EOF account or not (by loading the account).

Copy link
Member

Choose a reason for hiding this comment

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

keccak256("ef00") should better be memoized or hard-coded

Copy link
Member

Choose a reason for hiding this comment

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

I think this is generally fine for now although it conflicts with the TODO comment above.

test/state/host.cpp Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.05%. Comparing base (05ee922) to head (b67c8bb).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #587   +/-   ##
=======================================
  Coverage   98.05%   98.05%           
=======================================
  Files         116      117    +1     
  Lines       11455    11487   +32     
=======================================
+ Hits        11232    11264   +32     
  Misses        223      223           
Flag Coverage Δ
blockchaintests 59.75% <ø> (ø)
statetests 62.14% <100.00%> (+0.02%) ⬆️
statetests-silkpre 24.02% <16.12%> (-0.05%) ⬇️
unittests 96.51% <100.00%> (+0.12%) ⬆️

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

Files Coverage Δ
test/state/host.cpp 95.95% <100.00%> (+0.03%) ⬆️
test/unittests/state_transition_extcode_test.cpp 100.00% <100.00%> (ø)

test/state/host.cpp Outdated Show resolved Hide resolved
@chfast chfast marked this pull request as ready for review February 12, 2024 16:02
@pdobacz
Copy link
Collaborator

pdobacz commented Mar 11, 2024

Uses tests from ethereum/tests#1353, which contains some changes on top of the currently used tag for testing EOF in CI state-tests.

@pdobacz pdobacz requested review from gumb0 and chfast March 11, 2024 15:13
@pdobacz pdobacz merged commit c65d75e into master Mar 13, 2024
25 checks passed
@pdobacz pdobacz deleted the eof2-extcode branch March 13, 2024 11:33
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.

4 participants