EOF execution and validation (EIP-3540) #334

gumb0 · 2021-05-28T17:36:57Z

Make EOF header validation more generic (supporting any sections) commit is an optional refactoring, than will make adding new sections easier in the future.

lib/evmone/eof.cpp

chfast

Looks good so far.

codecov · 2021-05-31T13:03:54Z

Codecov Report

Merging #334 (a57245a) into master (0c052a2) will decrease coverage by 0.03%.
The diff coverage is 99.29%.

@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
- Coverage   99.60%   99.57%   -0.04%     
==========================================
  Files          33       37       +4     
  Lines        4057     4477     +420     
==========================================
+ Hits         4041     4458     +417     
- Misses         16       19       +3

Flag	Coverage Δ
consensus	`80.06% <19.54%> (-6.51%)`	⬇️
unittests	`99.61% <99.29%> (-0.04%)`	⬇️

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

Impacted Files	Coverage Δ
test/utils/bytecode.hpp	`97.63% <94.11%> (-0.40%)`	⬇️
lib/evmone/eof.cpp	`97.70% <97.70%> (ø)`
lib/evmone/advanced_execution.cpp	`100.00% <100.00%> (ø)`
lib/evmone/baseline.cpp	`100.00% <100.00%> (ø)`
lib/evmone/execution_state.hpp	`94.64% <100.00%> (+0.19%)`	⬆️
lib/evmone/instructions.hpp	`99.80% <100.00%> (ø)`
test/unittests/analysis_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/eof_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/eof_validation_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/evm_eof_test.cpp	`100.00% <100.00%> (ø)`

axic · 2021-06-02T11:38:01Z

test/utils/bytecode.hpp

@@ -62,6 +62,25 @@ inline bytecode operator*(int n, evmc_opcode op)
    return n * bytecode{op};
 }

+inline bytecode eof_header(uint16_t code_size, uint16_t data_size = 0)


Nice how non-complex this is.

axic · 2021-06-02T11:39:44Z

lib/evmone/eof.hpp

+struct EOF1Header
+{
+    int code_size = 0;
+    int data_size = 0;


Do we need to expose data_size now? It is not used for anything in the VM. Perhaps just leave a comment that this only cares about code?

Currently it's used (only checking != 0) to find code section offset (i.e. total header size).

lib/evmone/baseline.cpp

gumb0 · 2022-03-18T17:57:50Z

Rebased, please review @chfast @axic

chfast · 2022-03-21T15:44:36Z

lib/evmone/execution_state.hpp

@@ -121,6 +121,7 @@ class Memory


 /// Generic execution state for generic instructions implementations.
+// NOLINTNEXTLINE(clang-analyzer-optin.performance.Padding)


This is expected, because stack space is 32-byte aligned and the struct padding changes depending on what fields you have before the stack space.

chfast · 2022-03-21T15:46:45Z

lib/evmone/execution_state.hpp

@@ -165,7 +172,8 @@ class ExecutionState
        msg{&message},
        host{host_interface, host_ctx},
        rev{revision},
-        code{_code}
+        code{_code},


Is the code section ever needed here? The interpreter runs on the padded code from CodeAnalysis.

It needs at least the size of it, at least to report correct code to tracing...

But also the field itself is set in baseline to refenence the padded code, JUMPs and PC use it.

chfast · 2022-03-22T14:40:45Z

test/utils/bytecode.hpp

+inline bytecode eof_header(uint8_t version, uint16_t code_size, uint16_t data_size)
+{
+    bytecode out{"ef00"};
+    out += bytes{version};


I guess .push_back() should work here.

Or bytecode out{0xEF, 0x00, version}

This works as bytecode{bytes{0xEF, 0x00, version}}, because bytecode doesn't have initializer_list constructor. And adding such constructor would break bytecode{int_const} expressions, which currently translates to PUSH.

test/utils/bytecode.hpp

chfast · 2022-03-22T15:55:42Z

lib/evmone/eof.hpp

+
+struct EOF1Header
+{
+    size_t code_size = 0;


I'm not sure size_t is good type for it.

Me neither, is uint16_t better?

Yes, I think uint16_t is better.

I changed the fields to uint16_t (but left code_begin() to return size_t)

lib/evmone/eof.cpp

axic · 2022-03-23T12:08:31Z

lib/evmone/eof.cpp

+    const auto code_size_offset = 4;  // FORMAT + MAGIC + VERSION + CODE_SECTION_ID
+    header.code_size =
+        static_cast<uint16_t>((code[code_size_offset] << 8) | code[code_size_offset + 1]);
+    if (code[code_size_offset + 2] == 2)  // is data section present


This doesn't check for truncated EOF headers, i.e. does this expect validation ensures it?

Yes, this is a function to be used for validated deployed code.

Can you add this as a comment on the function?

evmone/lib/evmone/eof.hpp

Lines 28 to 29 in 52912ab

/// Reads the section sizes assuming that code has valid format.

/// (must be true for all EOF contracts on-chain)

axic · 2022-03-23T15:42:53Z

lib/evmone/eof.cpp

+
+    auto state = State::section_id;
+    uint8_t section_id = 0;
+    size_t section_sizes[3] = {0, 0, 0};


What are the 3 sizes?

Sizes for each possible value of section_id, 0th one is always 0 for terminator.

In later commit this is replaced with std::array<uint16_t, MAX_SECTION + 1>

lib/evmone/eof.cpp

axic · 2022-03-23T16:01:48Z

lib/evmone/eof.cpp

+    size_t section_sizes[3] = {0, 0, 0};
+    const auto code_end = code.end();
+    auto it = code.begin() + sizeof(MAGIC) + 2;  // FORMAT + MAGIC + VERSION
+    while (it != code_end && state != State::terminated)


Why did you decide to implement the generic loop as opposed the short two checks we have in the EIP? Did we ever measure any speed difference?

~~It became handy at one point when implementing RJUMPTABLE with additional jump table section.~~

~~That idea is abandoned now, but it can become handy again when we add type section for EOF Functions.~~

~~But I don't mind dropping this commit for now.~~

I was answering wrong question. To "why loop at all": I just liked that it's generic, it's less repetition in code I guess. No measurements done.

Actually it was written originaly by @chfast in #303, I decided not to write another one.

axic · 2022-03-23T16:03:14Z

lib/evmone/eof.cpp

+            return {{}, EOFValidationErrror::impossible};
+        }
+
+        ++it;


I think this may be better moved to the cases, ie. size_hi = *it++ ?

axic · 2022-03-23T16:05:33Z

lib/evmone/eof.cpp

+        return {{}, EOFValidationErrror::section_headers_not_terminated};
+
+    const auto section_bodies_size = section_sizes[CODE_SECTION] + section_sizes[DATA_SECTION];
+    const auto remaining_code_size = static_cast<size_t>(code_end - it);


Hmm, shouldn't we call this container?

axic · 2022-03-23T16:05:43Z

lib/evmone/eof.cpp

+constexpr uint8_t CODE_SECTION = 0x01;
+constexpr uint8_t DATA_SECTION = 0x02;
+
+std::pair<EOF1Header, EOFValidationErrror> validate_eof_headers(bytes_view code) noexcept


Same here, how about calling this container?

chfast · 2022-04-01T11:09:30Z

lib/evmone/eof.cpp

+    return header;
+}
+
+uint8_t get_eof_version(bytes_view cont) noexcept


Suggested change

uint8_t get_eof_version(bytes_view cont) noexcept

uint8_t get_eof_version(bytes_view container) noexcept

Renamed. (I originally wanted to avoid line wrapping.)

Co-authored-by: Paweł Bylica <chfast@gmail.com>

axic reviewed May 28, 2021

View reviewed changes

lib/evmone/eof.cpp Outdated Show resolved Hide resolved

gumb0 force-pushed the eof-execution branch 2 times, most recently from 114f86f to 913d23c Compare May 31, 2021 11:18

chfast reviewed May 31, 2021

View reviewed changes

gumb0 force-pushed the eof-execution branch 2 times, most recently from 5de5588 to c8b0852 Compare May 31, 2021 12:51

gumb0 changed the title ~~WIP EOF exection~~ EOF execution May 31, 2021

gumb0 force-pushed the eof-execution branch from c8b0852 to 05eb729 Compare May 31, 2021 12:55

gumb0 force-pushed the eof-execution branch 11 times, most recently from 9233479 to 8f83384 Compare June 1, 2021 16:30

axic reviewed Jun 2, 2021

View reviewed changes

gumb0 force-pushed the eof-execution branch 5 times, most recently from 101eaf4 to 052f65e Compare June 22, 2021 18:28

gumb0 commented Jun 22, 2021

View reviewed changes

lib/evmone/baseline.cpp Outdated Show resolved Hide resolved

gumb0 force-pushed the eof-execution branch from 052f65e to faaf770 Compare June 22, 2021 18:30

gumb0 commented Jun 22, 2021

View reviewed changes

lib/evmone/baseline.cpp Outdated Show resolved Hide resolved

chfast reviewed Mar 21, 2022

View reviewed changes

gumb0 force-pushed the eof-execution branch from 6826cc4 to 453c9ca Compare March 22, 2022 11:28

chfast approved these changes Mar 22, 2022

View reviewed changes

gumb0 force-pushed the eof-execution branch 2 times, most recently from 47fc7d9 to 5e9154c Compare March 22, 2022 18:47