Refactor subcontainer validation #888

gumb0 · 2024-05-10T15:17:56Z

Refactoring pulled out of #876

Main idea:

Instruction validation returns the list of references to subcontainers (instruction, subcontainer_idx)
1.1. (Not directly related, but I moved returning accessed section list into returned InstructionValidationResult struct instead of non-const reference argument)
The check for EOFCREATE with truncated data is moved out of instruction validation into being done once for a referenced container, after it is already validated.
This way we don't need to split subcontainer header validation and instructions validation and the loop over containers gets simpler.

Then it gets handy for additional container kind validation in #876.

codecov · 2024-05-10T15:20:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.50%. Comparing base (af17b74) to head (dc1d74c).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #888      +/-   ##
==========================================
- Coverage   98.50%   98.50%   -0.01%     
==========================================
  Files         130      130              
  Lines       15621    15625       +4     
==========================================
+ Hits        15388    15391       +3     
- Misses        233      234       +1

Flag	Coverage Δ
ethereum-tests	`27.96% <100.00%> (+<0.01%)`	⬆️
ethereum-tests-silkpre	`19.70% <0.00%> (+<0.01%)`	⬆️
execution-spec-tests	`19.06% <0.00%> (-0.01%)`	⬇️
unittests	`94.35% <100.00%> (+<0.01%)`	⬆️

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

Files	Coverage Δ
lib/evmone/eof.cpp	`87.31% <100.00%> (-0.10%)`	⬇️

gumb0 · 2024-05-10T15:42:45Z

lib/evmone/eof.cpp

+    /// Pairs of (container_index, opcode) of all opcodes referencing subcontainers in this section.
+    std::vector<std::pair<uint8_t, Opcode>> subcontainer_references;
+    /// Set of accessed code section indices.
+    std::unordered_set<uint16_t> accessed_code_sections;


this can be a vector, if we're cautios of sets

This may be a performance issue. But it is fine for now, just leave an issue or TODO comment.

gumb0 · 2024-05-13T06:47:45Z

I think we could optimize out "list of references to subcontainers" if we rely more on non-const reference arguments to instruction validation, but I think this version looks cleaner, I would start with it at least.

pdobacz

Looks great. I was wondering whether it impacts #878 but it seems it doesn't. And you're correct it supersedes #887

pdobacz · 2024-05-13T08:15:17Z

lib/evmone/eof.cpp

-    // Queue of (container, header) pairs left to process
-    std::queue<std::pair<bytes_view, EOF1Header>> container_queue;
-    container_queue.emplace(main_container, main_container_header);
+    struct Container


maybe a more specific name here, like "ValidatedContainer" or "ContainerValidationInfo"?

chfast · 2024-05-14T07:36:42Z

lib/evmone/eof.cpp

+    /// Pairs of (container_index, opcode) of all opcodes referencing subcontainers in this section.
+    std::vector<std::pair<uint8_t, Opcode>> subcontainer_references;
+    /// Set of accessed code section indices.
+    std::unordered_set<uint16_t> accessed_code_sections;


This may be a performance issue. But it is fine for now, just leave an issue or TODO comment.

chfast · 2024-05-14T07:51:24Z

lib/evmone/eof.cpp

+            const auto& [accessed_code_sections] =
+                std::get<InstructionValidationResult>(instr_validation_result_or_error);
+            for (const auto section_id : accessed_code_sections)
+                code_sections_queue.push(section_id);


C++23 has .push_range(). You can add TODO(C++23) comment.

chfast · 2024-05-14T07:52:14Z

lib/evmone/eof.cpp

@@ -310,8 +310,12 @@ std::variant<EOF1Header, EOFValidationError> validate_header(
    };
 }

+/// Result of validating instructions in a code section.


pedantic: should be in the first commit, but fine to leave it as it.

chfast · 2024-05-14T08:03:41Z

lib/evmone/eof.cpp

                std::get<InstructionValidationResult>(instr_validation_result_or_error);
+
+            // Mark what instructions referenced which subcontainers.
+            for (const auto& [index, opcode] : subcontainer_references)


Can be std::ranges::any_of()?

Not really, can be std::for_each() or std::ranges::for_each(), but maybe not much point.

chfast · 2024-05-14T08:09:53Z

lib/evmone/eof.cpp

        bool referenced_by_eofcreate = false;
    };
    // Queue of containers left to process
    std::queue<ContainerValidation> container_queue;
-    container_queue.emplace(main_container, main_container_header, false);
+    container_queue.push({main_container, false});


Why emplace → push is needed?

chfast · 2024-05-14T08:12:26Z

lib/evmone/eof.cpp

-            container_queue.emplace(std::move(subcontainer));
+            container_queue.emplace(subcontainer, std::move(subcont_header),
+                subcontainer_referenced_by_eofcreate[subcont_idx],
+                subcontainer_referenced_by_returncontract[subcont_idx]);


This fields doesn't exist in ContainerValidation.

gumb0 added the EOF label May 10, 2024

gumb0 force-pushed the validate-instr-refactor branch from 80df8c9 to 0e48680 Compare May 10, 2024 15:20

gumb0 mentioned this pull request May 10, 2024

Use can_init for EOFCREATE validation #887

Closed

gumb0 commented May 10, 2024

View reviewed changes

gumb0 requested review from hugo-dc, chfast, pdobacz and rodiazet May 13, 2024 06:48

pdobacz approved these changes May 13, 2024

View reviewed changes

gumb0 mentioned this pull request May 13, 2024

EOF validation of subcontainer kinds #876

Merged

2 tasks

gumb0 force-pushed the validate-instr-refactor branch from 0e48680 to e406e19 Compare May 13, 2024 12:56

chfast reviewed May 14, 2024

View reviewed changes

gumb0 added 5 commits May 14, 2024 11:33

Refactor returning accessed code sections from validate_instuctions()

93e8150

Collect subcontainer references during instruction validation

6ab1b54

Refactor: move EOFCREATE truncated container check to level above

64cfef6

Refactor: validate subcontainer headers in the end of validation

d7d2936

Refactor: don't validate header before enqueing container

dc1d74c

gumb0 force-pushed the validate-instr-refactor branch from e406e19 to dc1d74c Compare May 14, 2024 08:55

chfast approved these changes May 14, 2024

View reviewed changes

gumb0 merged commit 7ad882c into master May 14, 2024
23 of 24 checks passed

gumb0 deleted the validate-instr-refactor branch May 14, 2024 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor subcontainer validation #888

Refactor subcontainer validation #888

gumb0 commented May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading

gumb0 May 10, 2024

chfast May 14, 2024

gumb0 commented May 13, 2024

pdobacz left a comment

pdobacz May 13, 2024

chfast May 14, 2024

chfast May 14, 2024

chfast May 14, 2024

chfast May 14, 2024

gumb0 May 14, 2024 •

edited

Loading

chfast May 14, 2024

chfast May 14, 2024

gumb0 May 14, 2024

Refactor subcontainer validation #888

Refactor subcontainer validation #888

Conversation

gumb0 commented May 10, 2024 • edited Loading

codecov bot commented May 10, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented May 13, 2024

pdobacz left a 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 May 14, 2024 • edited Loading

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 May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading

gumb0 May 14, 2024 •

edited

Loading