EOF code validation (EIP-3670) by gumb0 · Pull Request #366 · ipsilon/evmone

gumb0 · 2021-07-28T13:48:32Z

No description provided.

codecov · 2021-07-28T14:23:36Z

Codecov Report

Merging #366 (6c563f4) into eof-execution (631b430) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 6c563f4 differs from pull request most recent head 7ac440c. Consider uploading reports for the commit 7ac440c to get more accurate results

@@              Coverage Diff               @@
##           eof-execution     #366   +/-   ##
==============================================
  Coverage          99.57%   99.58%           
==============================================
  Files                 37       37           
  Lines               4487     4551   +64     
==============================================
+ Hits                4468     4532   +64     
  Misses                19       19

Flag	Coverage Δ
consensus	`78.94% <0.00%> (-1.22%)`	⬇️
unittests	`99.62% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
lib/evmone/eof.cpp	`98.03% <100.00%> (+0.39%)`	⬆️
test/unittests/eof_validation_test.cpp	`100.00% <100.00%> (ø)`

lib/evmone/eof.cpp

chfast · 2022-03-22T17:13:55Z

lib/evmone/instructions_traits.hpp

    int8_t stack_height_change = 0;

+    /// Size of the immediate argument in bytes.
+    uint8_t immediate_size = 0;


Probably better to place it as the second field. The whole traits change should be submitted separately.

Why the second? What about is_terminating?

Do you want a separate PR? (these fields are used only in this PR) I put it into a separate commit so far.

axic · 2022-03-23T16:20:04Z

lib/evmone/eof.cpp

-        return {{}, error};
+    assert(code.size() > 0);  // guaranteed by EOF headers validation
+
+    size_t i = 0;


Why isn't this an iterator instead?

Because with an iterator loop end condition can be only it != code.end(), but loop can overrun end in case of truncated immediate.

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

lib/evmone/eof.cpp

+    {
+        op = code[i];
+        const auto& since = instr::traits[op].since;
+        if (!since.has_value() || *since > rev)


I think this doesn't cover the case when we want to remove an opcode (e.g.calldata) from EOF contracts?

Yes, when that happens I guess we will need to add final revision to traits.

Yes. This looks like a good solution if needed.

lib/evmone/eof.cpp

Validate that all used instructions are defined and code ends with a terminating instruction.

lib/evmone/eof.cpp

chfast · 2022-04-04T08:49:54Z

test/unittests/eof_validation_test.cpp


 namespace
 {
+inline EOFValidationError validate_eof(bytes_view cont, evmc_revision rev = EVMC_SHANGHAI) noexcept


Suggested change

inline EOFValidationError validate_eof(bytes_view cont, evmc_revision rev = EVMC_SHANGHAI) noexcept

inline EOFValidationError validate_eof(bytes_view container, evmc_revision rev = EVMC_SHANGHAI) noexcept

chfast · 2022-04-04T08:50:24Z

test/unittests/eof_validation_test.cpp

 {
+inline EOFValidationError validate_eof(bytes_view cont, evmc_revision rev = EVMC_SHANGHAI) noexcept
+{
+    return ::validate_eof(rev, cont);


Does it call evmone::validate_eof()? I'm confused.

chfast · 2022-04-04T08:55:36Z

test/unittests/eof_validation_test.cpp

+}
+
 inline EOFValidationError validate_eof(
    std::string_view code_hex, evmc_revision rev = EVMC_SHANGHAI) noexcept


It seems this overload is used once and there is another use case as validate_eof(from_hex(...)). I'm rather for removing this overload. It sometimes become confusing when a unit tests multiple levels of wrappers. Using from_hex() or bytecode{} is fine or you can use bytecode as the type of the parameter.

Changed it to a single helper with bytecode parameter, can be called with either hex string or bytes container.

clang-tidy want it as const bytecode&. I think changing it like this should not be a problem.

chfast · 2022-04-04T08:58:07Z

lib/evmone/eof.cpp

+    while (i < code.size())
+    {
+        op = code[i];
+        const auto& since = instr::traits[op].since;


I think it is fine for now, but instr::traits has sub-optimal layout for runtime use.

chfast · 2022-04-04T09:00:48Z

lib/evmone/eof.cpp

+    {
+        op = code[i];
+        const auto& since = instr::traits[op].since;
+        if (!since.has_value() || *since > rev)


Yes. This looks like a good solution if needed.

gumb0 force-pushed the eof-code-validation branch from 7765c0b to d27046d Compare July 28, 2021 14:05

gumb0 force-pushed the eof-execution branch 9 times, most recently from b166534 to 850d7b8 Compare August 12, 2021 13:45

gumb0 force-pushed the eof-code-validation branch from d27046d to cac511b Compare August 12, 2021 14:24

gumb0 force-pushed the eof-code-validation branch 2 times, most recently from 1ccc8aa to f82ac27 Compare August 30, 2021 13:56

gumb0 force-pushed the eof-execution branch 2 times, most recently from e07ffef to de74fc2 Compare September 29, 2021 14:51

gumb0 force-pushed the eof-code-validation branch 3 times, most recently from 3b42a85 to 2411d77 Compare October 4, 2021 11:27

gumb0 force-pushed the eof-execution branch 2 times, most recently from 8127c9b to 6181dd1 Compare October 6, 2021 15:26

gumb0 force-pushed the eof-code-validation branch 4 times, most recently from a73f066 to eb773aa Compare October 11, 2021 16:43

gumb0 force-pushed the eof-execution branch 5 times, most recently from d387060 to 38461f6 Compare March 2, 2022 15:06

gumb0 force-pushed the eof-execution branch 15 times, most recently from a574ec8 to 6eb5cc5 Compare March 15, 2022 14:21

gumb0 force-pushed the eof-code-validation branch 2 times, most recently from 1210ca1 to e017bda Compare March 16, 2022 11:47

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

gumb0 force-pushed the eof-code-validation branch from e017bda to c09f8cc Compare March 22, 2022 11:36

chfast reviewed Mar 22, 2022

View reviewed changes

axic reviewed Mar 23, 2022

View reviewed changes

chfast reviewed Mar 30, 2022

View reviewed changes

lib/evmone/eof.cpp Show resolved Hide resolved

Validate code section

c0ad5d0

Validate that all used instructions are defined and code ends with a terminating instruction.

chfast approved these changes Apr 4, 2022

View reviewed changes

Tests for EOF code validation

08aab70

chfast approved these changes Apr 4, 2022

View reviewed changes

	inline EOFValidationError validate_eof(bytes_view cont, evmc_revision rev = EVMC_SHANGHAI) noexcept
	inline EOFValidationError validate_eof(bytes_view container, evmc_revision rev = EVMC_SHANGHAI) noexcept

Conversation

gumb0 commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gumb0 Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gumb0 commented Jul 28, 2021 •

edited

Loading

codecov bot commented Jul 28, 2021 •

edited

Loading

gumb0 Mar 30, 2022 •

edited

Loading