Skip to content

eof: Add caching of TXCREATE validation results #1193

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

Merged
merged 3 commits into from
Apr 16, 2025

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented Apr 11, 2025

No description provided.

@gumb0 gumb0 added the EOF label Apr 11, 2025
Copy link

codecov bot commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 99.10714% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.84%. Comparing base (e3ff3b9) to head (d1bc275).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
lib/evmone/instructions_calls.cpp 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1193      +/-   ##
==========================================
+ Coverage   94.82%   94.84%   +0.02%     
==========================================
  Files         171      171              
  Lines       19406    19501      +95     
==========================================
+ Hits        18401    18496      +95     
  Misses       1005     1005              
Flag Coverage Δ
eof_execution_spec_tests 2.56% <0.00%> (-0.02%) ⬇️
ethereum_tests 21.07% <0.00%> (-0.11%) ⬇️
ethereum_tests_silkpre 18.15% <0.00%> (-0.10%) ⬇️
execution_spec_tests 18.24% <0.00%> (-0.09%) ⬇️
unittests 92.52% <99.10%> (+0.03%) ⬆️

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

Files with missing lines Coverage Δ
lib/evmone/execution_state.hpp 95.58% <100.00%> (+0.06%) ⬆️
test/unittests/state_transition_txcreate_test.cpp 100.00% <100.00%> (ø)
lib/evmone/instructions_calls.cpp 98.38% <94.11%> (+0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gumb0 gumb0 force-pushed the eof/cache-txcreate-validation branch 3 times, most recently from eb46837 to c7c3809 Compare April 14, 2025 12:45
@gumb0 gumb0 marked this pull request as ready for review April 14, 2025 12:48
@gumb0 gumb0 force-pushed the eof/cache-txcreate-validation branch from c7c3809 to b23a729 Compare April 14, 2025 13:04
@gumb0 gumb0 requested review from chfast and pdobacz April 14, 2025 13:05
Copy link
Contributor

@pdobacz pdobacz left a comment

Choose a reason for hiding this comment

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

LGTM. Just some test ideas, but better to implement these in EEST

tx.type = Transaction::Type::initcodes;
tx.initcodes.push_back(init_container);

constexpr auto iterations = 500;
Copy link
Contributor

Choose a reason for hiding this comment

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

as mentioned on the call, I'd also make a version with invalid EOF, and with a mix valid/invalid. But maybe enough to leave it for EEST.

There is also a twisted variant where the loop is handled by recursion, where an (erroneous) cache mechanism could clear the cache on every call depth level. Also possible better in EEST.

Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

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

So

@@ -216,6 +217,7 @@ class ExecutionState
deploy_container = {};
m_tx = {};
m_initcodes.reset();
m_initcodes_validity.clear();
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 it is better to combine this with m_initcodes: to have value as struct {bytes_view, std::optional<bool>} or struct {bytes_view, bool}.

Copy link
Member Author

@gumb0 gumb0 Apr 15, 2025

Choose a reason for hiding this comment

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

will have to have optional, I mostly decided against this originally because of this cumbersome structure

Copy link
Member Author

Choose a reason for hiding this comment

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

Combined now.

@@ -435,8 +435,18 @@ Result create_eof_impl(

if constexpr (Op == OP_TXCREATE)
{
const auto error_subcont = validate_eof(state.rev, ContainerKind::initcode, initcontainer);
if (error_subcont != EOFValidationError::success)
const auto validity = state.get_initcode_validity(initcode_hash);
Copy link
Member

Choose a reason for hiding this comment

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

This section is unnecessarily separated from the other TXCREATE processing section above. I think we should reorder them and place the endowment and depth checks before the get_tx_initcode_by_hash() section.

Then we will be able to combine the get_tx_initcode_by_hash() and set_initcode_validity, potentially with having single unordered_map.

Copy link
Member Author

Choose a reason for hiding this comment

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

Problem is pos has to be updated before light failures
(and differently for EOFCREATE and TXCREATE)

I pushed one possible version, without unifying initcode and validity query yet.

@gumb0 gumb0 force-pushed the eof/cache-txcreate-validation branch 2 times, most recently from 1265290 to 3a4be96 Compare April 15, 2025 13:15
@gumb0 gumb0 marked this pull request as draft April 15, 2025 13:16
@gumb0 gumb0 force-pushed the eof/cache-txcreate-validation branch 2 times, most recently from 44863a1 to aec7428 Compare April 15, 2025 16:43
@gumb0 gumb0 marked this pull request as ready for review April 15, 2025 16:52
@gumb0 gumb0 requested a review from chfast April 16, 2025 09:24
@gumb0 gumb0 force-pushed the eof/cache-txcreate-validation branch from 2880d00 to d1bc275 Compare April 16, 2025 12:30
@gumb0 gumb0 merged commit 48c2d63 into master Apr 16, 2025
24 checks passed
@gumb0 gumb0 deleted the eof/cache-txcreate-validation branch April 16, 2025 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants