Skip to content
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

Use can_init for EOFCREATE validation #887

Closed
wants to merge 2 commits into from
Closed

Conversation

pdobacz
Copy link
Collaborator

@pdobacz pdobacz commented May 8, 2024

This was bugging me for a while.

We can leverage the same path in all cases where data section trunctaion is checked.

Not ideal, I tried a bunch of options, this seems least obtrusive. WDYT?

@pdobacz pdobacz requested a review from gumb0 May 8, 2024 12:51
@pdobacz pdobacz self-assigned this May 8, 2024
Copy link

codecov bot commented May 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #887      +/-   ##
==========================================
- Coverage   98.50%   98.50%   -0.01%     
==========================================
  Files         130      130              
  Lines       15621    15619       -2     
==========================================
- Hits        15388    15386       -2     
  Misses        233      233              
Flag Coverage Δ
ethereum-tests 27.95% <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.36% <100.00%> (-0.05%) ⬇️

@pdobacz pdobacz changed the title Use can_init in EOFCREATE Use can_init for EOFCREATE validation May 8, 2024
lib/evmone/eof.cpp Outdated Show resolved Hide resolved
lib/evmone/eof.cpp Outdated Show resolved Hide resolved
@gumb0
Copy link
Member

gumb0 commented May 8, 2024

In #876 I'm considering moving this check out of validate_instructions() to a level above in validate_eof1() where related checks for EOFCREATE/container kind compatibility are done, then validate_instructions() won't have subcontainer_can_init argument at all.

But we can merge this first.

@pdobacz
Copy link
Collaborator Author

pdobacz commented May 8, 2024

In #876 I'm considering moving this check out of validate_instructions() to a level above in validate_eof1() where related checks for EOFCREATE/container kind compatibility are done, then validate_instructions() won't have subcontainer_can_init argument at all.

But we can merge this first.

Ah yes, and there's also the refactors at #878. In case you have a feeling this will produce unnecessary conflicts, please let me know, I'll not merge until after dust settled

@gumb0
Copy link
Member

gumb0 commented May 10, 2024

I made another refactoring PR #888. If that one is merged, this one would be obsolete I think.
(This check is done in a different place and I used can_init() there right away)

@gumb0
Copy link
Member

gumb0 commented May 15, 2024

With #888 merged I think this can be closed

@pdobacz pdobacz closed this May 15, 2024
@pdobacz pdobacz deleted the more-can-init branch May 15, 2024 13:18
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