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

EOF DATA* opcodes #586

Merged
merged 9 commits into from
May 26, 2023
Merged

EOF DATA* opcodes #586

merged 9 commits into from
May 26, 2023

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Mar 18, 2023

No description provided.

lib/evmone/instructions.hpp Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 18, 2023

Codecov Report

Merging #586 (f31a835) into master (5983bd7) will increase coverage by 0.04%.
The diff coverage is 99.10%.

❗ Current head f31a835 differs from pull request most recent head bb6001b. Consider uploading reports for the commit bb6001b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #586      +/-   ##
==========================================
+ Coverage   97.33%   97.38%   +0.04%     
==========================================
  Files          80       80              
  Lines        7739     7949     +210     
==========================================
+ Hits         7533     7741     +208     
- Misses        206      208       +2     
Flag Coverage Δ
blockchaintests 62.97% <30.23%> (-1.20%) ⬇️
statetests 73.89% <40.00%> (-0.67%) ⬇️
unittests 94.94% <99.10%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
lib/evmone/advanced_instructions.cpp 100.00% <ø> (ø)
lib/evmone/baseline_instruction_table.cpp 100.00% <ø> (ø)
test/unittests/instructions_test.cpp 89.58% <ø> (ø)
lib/evmone/eof.cpp 83.01% <80.00%> (-0.18%) ⬇️
lib/evmone/advanced_analysis.hpp 100.00% <100.00%> (ø)
lib/evmone/advanced_execution.cpp 100.00% <100.00%> (ø)
lib/evmone/baseline.cpp 100.00% <100.00%> (ø)
lib/evmone/eof.hpp 100.00% <100.00%> (ø)
lib/evmone/execution_state.hpp 96.22% <100.00%> (+0.39%) ⬆️
lib/evmone/instructions.hpp 100.00% <100.00%> (ø)
... and 5 more

for (size_t i = 0; i < 32; ++i)
data[i] = state.data[begin + i];

index = intx::be::load<uint256>(data);
Copy link
Member

Choose a reason for hiding this comment

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

Is this taking an array (and not iterators)? Otherwise could just use

Suggested change
index = intx::be::load<uint256>(data);
index = intx::be::load<uint256>(begin + index);

Copy link
Member Author

Choose a reason for hiding this comment

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

It takes array

Copy link
Member

Choose a reason for hiding this comment

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

Need an overload in intx 😅

Copy link
Member

Choose a reason for hiding this comment

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

Made an issue: chfast/intx#288

Copy link
Member

Choose a reason for hiding this comment

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

Use intx::be::unsafe::load(&state.data[begin]).

@gumb0 gumb0 force-pushed the eof-data-opcodes branch 2 times, most recently from 336a205 to 8b93f3f Compare March 18, 2023 18:52
@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
lib/evmone/instructions.hpp Outdated Show resolved Hide resolved
@axic axic added the EOF label Mar 22, 2023
@axic axic force-pushed the eof-data-opcodes branch 2 times, most recently from 72693a7 to ad36c18 Compare March 28, 2023 10:31
@gumb0 gumb0 force-pushed the eof-data-opcodes branch 11 times, most recently from 3fada02 to 5b2a4d8 Compare April 13, 2023 15:42
lib/evmone/eof.hpp Outdated Show resolved Hide resolved
lib/evmone/execution_state.hpp Show resolved Hide resolved
test/unittests/eof_validation_test.cpp Show resolved Hide resolved
test/unittests/evm_eof_test.cpp Outdated Show resolved Hide resolved
test/unittests/evm_eof_test.cpp Outdated Show resolved Hide resolved
test/unittests/evm_eof_test.cpp Outdated Show resolved Hide resolved
lib/evmone/instructions.hpp Outdated Show resolved Hide resolved
@gumb0
Copy link
Member Author

gumb0 commented May 15, 2023

@hugo-dc Any idea why EIP-3670 tests don't fail here? Don't we check that all undefined opcodes are rejected?

We change the validation to accept some new opcodes that tests consider invalid, so supposedly this change shouldn't pass current tests.

I investigated this a bit and I think that codes with new opcodes are most certainly still invalid for some other reason.
The tests I found check the code generated from a pattern 0xef000101000402000100020300000000000000<invalid_opcode>00 - this would be invalid either because of wrong stack for new instruction, or for wrong max stack height declared as 0.

So this makes those tests (stEIP3670/*InvalidOpcodes) kinda useless.

@gumb0 gumb0 force-pushed the eof-data-opcodes branch 3 times, most recently from b592f2e to 54bf309 Compare May 16, 2023 10:53
lib/evmone/execution_state.hpp Outdated Show resolved Hide resolved
lib/evmone/instructions.hpp Outdated Show resolved Hide resolved
lib/evmone/instructions.hpp Outdated Show resolved Hide resolved
lib/evmone/instructions.hpp Show resolved Hide resolved
test/unittests/evm_eof_test.cpp Show resolved Hide resolved
@axic
Copy link
Member

axic commented May 22, 2023

@gumb0 what is outstanding here?

@gumb0 gumb0 requested a review from chfast May 23, 2023 08:37
@gumb0
Copy link
Member Author

gumb0 commented May 23, 2023

@gumb0 what is outstanding here?

I think another round of review form @chfast

lib/evmone/instructions.hpp Show resolved Hide resolved
auto& index = stack.top();

if (state.data.size() < 32 || (state.data.size() - 32) < index)
return {EVMC_INVALID_MEMORY_ACCESS, gas_left}; // TODO: Introduce EVMC_INVALID_DATA_ACCESS
Copy link
Member

Choose a reason for hiding this comment

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

I mean general design: maybe padding with zero is better. But I think this is good staring point.

lib/evmone/instructions.hpp Show resolved Hide resolved
@gumb0 gumb0 force-pushed the eof-data-opcodes branch 2 times, most recently from bb6001b to 043b1b5 Compare May 24, 2023 10:58
@gumb0 gumb0 requested a review from chfast May 24, 2023 11:07
@gumb0 gumb0 merged commit 894fe66 into master May 26, 2023
@gumb0 gumb0 deleted the eof-data-opcodes branch May 26, 2023 16:13
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.

3 participants