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

EIP-4750: EOF - Functions #497

Closed
wants to merge 74 commits into from
Closed

EIP-4750: EOF - Functions #497

wants to merge 74 commits into from

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Aug 23, 2022

No description provided.

section_count * section_header_size + sizeof(TERMINATOR);
}

std::pair<EOFSectionHeaders, EOFValidationError> validate_eof_headers(bytes_view container)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not noexcept anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Number of sections is arbitrary now and so function does dynamic allocation (to return all sections' sizes)

case DATA_SECTION:
if (section_headers[CODE_SECTION] == 0)
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 you can't remove this from here?

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's replaced by another check in case CODE_SECTION:

if (!section_headers[DATA_SECTION].empty())
    return {{}, EOFValidationError::data_section_before_code_section};

(because now invalid case can be like code-data-code)

plus the old check in case TERMINATOR guarantees that at least one code section is present.

@gumb0 gumb0 force-pushed the eof-functions branch 3 times, most recently from ef30ed0 to 41acfad Compare September 8, 2022 14:05
@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #497 (75a38b2) into eof-static-jumps2 (6d03f5d) will decrease coverage by 0.65%.
The diff coverage is 92.82%.

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

Additional details and impacted files
@@                  Coverage Diff                  @@
##           eof-static-jumps2     #497      +/-   ##
=====================================================
- Coverage              98.03%   97.37%   -0.66%     
=====================================================
  Files                     59       64       +5     
  Lines                   5687     6390     +703     
=====================================================
+ Hits                    5575     6222     +647     
- Misses                   112      168      +56     
Flag Coverage Δ
blockchaintests 69.50% <6.21%> (-8.66%) ⬇️
statetests 77.26% <73.70%> (+4.49%) ⬆️
unittests 89.80% <76.14%> (-1.92%) ⬇️

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

Impacted Files Coverage Δ
lib/evmone/eof.hpp 100.00% <ø> (ø)
lib/evmone/execution_state.hpp 94.54% <ø> (ø)
test/state/precompiles.hpp 0.00% <0.00%> (ø)
test/unittests/evm_fixture.hpp 100.00% <ø> (ø)
test/state/precompiles_cache.cpp 50.00% <50.00%> (ø)
test/statetest/statetest_loader.cpp 76.47% <52.38%> (-4.40%) ⬇️
lib/evmone/instructions_calls.cpp 98.14% <75.00%> (-1.86%) ⬇️
test/state/host.cpp 80.31% <75.00%> (-0.23%) ⬇️
lib/evmone/eips.hpp 83.33% <83.33%> (ø)
test/state/precompiles.cpp 83.15% <86.20%> (+16.49%) ⬆️
... and 22 more

@gumb0 gumb0 force-pushed the eof-functions branch 2 times, most recently from a2f706b to 72c70b4 Compare September 13, 2022 10:45
@chfast chfast force-pushed the eof-static-jumps2 branch 4 times, most recently from 3da226a to ceb4032 Compare September 28, 2022 16:05
@gumb0 gumb0 force-pushed the eof-static-jumps2 branch 4 times, most recently from 3e89dde to 1ab5b19 Compare December 6, 2022 14:10
@gumb0
Copy link
Member Author

gumb0 commented Dec 6, 2022

Rebased, updated opcodes and gas prices for CALLF/RETF and made it activated in Shanghai.

@gumb0 gumb0 force-pushed the eof-static-jumps2 branch 2 times, most recently from 9c49c47 to 875d04e Compare December 7, 2022 16:51
Comment on lines 733 to 734
const auto rel_offset_hi = pc[1 + static_cast<uint16_t>(case_) * REL_OFFSET_SIZE];
const auto rel_offset_lo = pc[2 + static_cast<uint16_t>(case_) * REL_OFFSET_SIZE];
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this off-by-one error?

Suggested change
const auto rel_offset_hi = pc[1 + static_cast<uint16_t>(case_) * REL_OFFSET_SIZE];
const auto rel_offset_lo = pc[2 + static_cast<uint16_t>(case_) * REL_OFFSET_SIZE];
const auto rel_offset_hi = pc[2 + static_cast<uint16_t>(case_) * REL_OFFSET_SIZE];
const auto rel_offset_lo = pc[3 + static_cast<uint16_t>(case_) * REL_OFFSET_SIZE];

pc[1] is count, for case == 0, it should take pc[2] and pc[3].

return condition + OP_RJUMPI + bytecode{big_endian(offset)};
}

inline bytecode rjumpv(const std::initializer_list<uint16_t> offsets, bytecode condition)
Copy link
Member Author

Choose a reason for hiding this comment

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

sorry this was wrong

Suggested change
inline bytecode rjumpv(const std::initializer_list<uint16_t> offsets, bytecode condition)
inline bytecode rjumpv(const std::initializer_list<int16_t> offsets, bytecode condition)


for (const auto& t : types)
{
if (t.max_stack_height > MAX_STACK_HEIGHT)
Copy link
Member Author

Choose a reason for hiding this comment

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

According to EIP-5450 max stack height doesn't exceed 1023

Suggested change
if (t.max_stack_height > MAX_STACK_HEIGHT)
if (t.max_stack_height >= MAX_STACK_HEIGHT)

@axic
Copy link
Member

axic commented Feb 14, 2023

Is this deprecated by #563?

@gumb0 gumb0 closed this Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants