EOF code validation (EIP-3670) #366

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%> (ø)`

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

 #include <gtest/gtest.h>
 #include <test/utils/utils.hpp>

 using namespace evmone;

 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

 #include <gtest/gtest.h>
 #include <test/utils/utils.hpp>

 using namespace evmone;

 namespace
 {
+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

+{
+    return ::validate_eof(rev, cont);
+}
+
 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 2 times, most recently from 47fc7d9 to 5e9154c Compare March 22, 2022 18:47

gumb0 force-pushed the eof-code-validation branch 2 times, most recently from 1f16ee5 to 7fefde0 Compare March 22, 2022 19:00

gumb0 force-pushed the eof-execution branch from 5e9154c to 52912ab Compare March 23, 2022 13:03

axic reviewed Mar 23, 2022

View reviewed changes

gumb0 force-pushed the eof-execution branch from 52912ab to 631b430 Compare March 30, 2022 09:37

gumb0 force-pushed the eof-code-validation branch 5 times, most recently from fc7e218 to 58c98b9 Compare March 30, 2022 16:19

gumb0 marked this pull request as ready for review March 30, 2022 16:28

gumb0 requested review from axic and chfast March 30, 2022 16:28

chfast reviewed Mar 30, 2022

View reviewed changes

lib/evmone/eof.cpp Show resolved Hide resolved

gumb0 force-pushed the eof-code-validation branch 2 times, most recently from 6c563f4 to 7ac440c Compare March 31, 2022 11:18

gumb0 force-pushed the eof-execution branch from 631b430 to a57245a Compare April 1, 2022 13:01

Base automatically changed from eof-execution to master April 1, 2022 14:13

Validate code section

c0ad5d0

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

gumb0 force-pushed the eof-code-validation branch from 7ac440c to 3fdc2c5 Compare April 1, 2022 16:46

chfast approved these changes Apr 4, 2022

View reviewed changes

gumb0 force-pushed the eof-code-validation branch from 3fdc2c5 to b328d7f Compare April 4, 2022 10:01

Tests for EOF code validation

08aab70

gumb0 force-pushed the eof-code-validation branch from b328d7f to 08aab70 Compare April 4, 2022 10:36

chfast approved these changes Apr 4, 2022

View reviewed changes

gumb0 merged commit 27941b7 into master Apr 4, 2022

gumb0 deleted the eof-code-validation branch April 4, 2022 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EOF code validation (EIP-3670) #366

EOF code validation (EIP-3670) #366

gumb0 commented Jul 28, 2021 •

edited

Loading

codecov bot commented Jul 28, 2021 •

edited

Loading

axic Mar 23, 2022

gumb0 Mar 30, 2022

axic Mar 23, 2022

gumb0 Mar 30, 2022

chfast Apr 4, 2022

chfast Apr 4, 2022

chfast Apr 4, 2022

chfast Apr 4, 2022

gumb0 Apr 4, 2022

chfast Apr 4, 2022

chfast Apr 4, 2022

chfast Apr 4, 2022

	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

EOF code validation (EIP-3670) #366

EOF code validation (EIP-3670) #366

Conversation

gumb0 commented Jul 28, 2021 • edited Loading

codecov bot commented Jul 28, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Jul 28, 2021 •

edited

Loading

codecov bot commented Jul 28, 2021 •

edited

Loading