Skip to content

EOFCREATE and RETURNCONTRACT implementation#553

Merged
gumb0 merged 11 commits intomasterfrom
eof-create3
Apr 16, 2024
Merged

EOFCREATE and RETURNCONTRACT implementation#553
gumb0 merged 11 commits intomasterfrom
eof-create3

Conversation

@gumb0
Copy link
Copy Markdown
Member

@gumb0 gumb0 commented Feb 6, 2023

TODO:

  • Validate CREATE3 to not allow it with truncated containers (currently implemented as run-time check, but should be done at validation, but requires parsing subcontainers before CREATE3 validation)
  • Fix some commented out tests
  • [ ] Optimization to replace validation recursion with iteration created separate issue EOF: Nested container validation without recursion #794
  • More transition tests and unit tests
  • Merge EVMC changes
  • Update state tests

@gumb0 gumb0 force-pushed the eof-create3 branch 2 times, most recently from 1e9a68a to 17dd8f8 Compare February 6, 2023 14:52
@gumb0 gumb0 changed the base branch from master to eof February 6, 2023 14:52
@gumb0 gumb0 force-pushed the eof-create3 branch 6 times, most recently from e5b5a78 to 8b25057 Compare February 6, 2023 15:41
table[EVMC_CANCUN][OP_DUPN] = 3;
table[EVMC_CANCUN][OP_SWAPN] = 3;
table[EVMC_CANCUN][OP_CREATE3] = 32000;
table[EVMC_CANCUN][OP_RETURNCONTRACT] = 0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To think about this: probably neither of these prices are good?

@gumb0 gumb0 force-pushed the eof-create3 branch 4 times, most recently from 82013aa to f8f095b Compare February 8, 2023 11:13
@rodiazet rodiazet force-pushed the eof branch 2 times, most recently from 219821b to 3987c3d Compare February 13, 2023 14:33
@gumb0 gumb0 force-pushed the eof-create3 branch 8 times, most recently from 860b84f to b4b4aa0 Compare February 15, 2023 18:31
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 16, 2023

Codecov Report

Attention: Patch coverage is 98.82729% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 98.25%. Comparing base (24a3214) to head (6e697e7).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #553      +/-   ##
==========================================
+ Coverage   98.17%   98.25%   +0.08%     
==========================================
  Files         129      129              
  Lines       13767    14644     +877     
==========================================
+ Hits        13516    14389     +873     
- Misses        251      255       +4     
Flag Coverage Δ
statetests 29.35% <7.70%> (-1.70%) ⬇️
statetests-silkpre 19.86% <3.61%> (-1.22%) ⬇️
unittests 93.79% <98.82%> (+0.36%) ⬆️

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

Files Coverage Δ
lib/evmone/advanced_instructions.cpp 100.00% <ø> (ø)
lib/evmone/baseline.cpp 100.00% <100.00%> (ø)
lib/evmone/baseline_instruction_table.cpp 100.00% <ø> (ø)
lib/evmone/eof.hpp 100.00% <100.00%> (ø)
lib/evmone/execution_state.hpp 96.22% <ø> (ø)
lib/evmone/instructions.hpp 100.00% <100.00%> (ø)
test/state/host.cpp 98.14% <100.00%> (+1.36%) ⬆️
test/state/host.hpp 100.00% <ø> (ø)
test/state/state.cpp 100.00% <100.00%> (ø)
test/unittests/eof_validation.cpp 94.28% <100.00%> (+0.34%) ⬆️
... and 8 more

@axic axic force-pushed the eof branch 2 times, most recently from 91b770f to ef94b4e Compare February 17, 2023 17:37
const auto& offset = stack[0];
const auto& size = stack[1];

if (state.msg->kind != EVMC_CREATE3)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In case we end up allowing EOF creation transactions with empty to (as opposed to relying only on transaction to Creator Contract), this will have to be adjusted to allow RETURNCONTRACT in the context of creation transaction.

@pdobacz
Copy link
Copy Markdown
Member

pdobacz commented Oct 26, 2023

[ ] Fix some commented out tests

They have been fixed and uncommented in eof-create4 (I think all of them relied on CREATE4), can tick that one off.

@gumb0
Copy link
Copy Markdown
Member Author

gumb0 commented Dec 4, 2023

Rebased.

@gumb0
Copy link
Copy Markdown
Member Author

gumb0 commented Dec 19, 2023

Rebased and reworked subcontainers and truncated data support in bytecode.hpp

@gumb0
Copy link
Copy Markdown
Member Author

gumb0 commented Jan 15, 2024

Rebased and squashed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved to eof_create4 branch

Copy link
Copy Markdown
Member

@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.

Very nice, I appreciate the very thorough testing. Just a few nitpicks left, but pls someone else review and give a stamp too.

Late thought: do we have a test that a CREATE3-RETURNCONTRACT-ed contract can be successfully called, for example DATALOAD ing from the dynamic aux data part. To ensure that the deployment process doesn't garble the EOF container in any way.


/// Size of the data section.
/// In case of deploy container it is the minimal data size of the container that will be
/// deployed, taking into account part of data appended at deploy-time (aux_data). In this case
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
/// deployed, taking into account part of data appended at deploy-time (aux_data). In this case
/// deployed, taking into account part of data appended at deploy-time (static_aux_data). In this case

EXPECT_STATUS(EVMC_UNDEFINED_INSTRUCTION);
}

TEST_P(evm, returncontract_not_int_initcode)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
TEST_P(evm, returncontract_not_int_initcode)
TEST_P(evm, returncontract_not_in_initcode)

EXPECT_EQ(std::get<std::error_code>(validate_transaction(acc, bi, tx, EVMC_PRAGUE, 60000,
BlockInfo::MAX_BLOB_GAS_PER_BLOCK))
.message(),
"EOF in creation transaction");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"EOF in creation transaction");
"EOF initcode in creation transaction");

to be more specific? Can also be "in creation transaction data"

@gumb0
Copy link
Copy Markdown
Member Author

gumb0 commented Feb 27, 2024

Late thought: do we have a test that a CREATE3-RETURNCONTRACT-ed contract can be successfully called, for example DATALOAD ing from the dynamic aux data part. To ensure that the deployment process doesn't garble the EOF container in any way.

Added such test.

@gumb0
Copy link
Copy Markdown
Member Author

gumb0 commented Mar 4, 2024

Renamed CREATE3 to EOFCREATE.

@gumb0
Copy link
Copy Markdown
Member Author

gumb0 commented Mar 27, 2024

Not sure yet what happened to the coverage now, it was almost everything covered some time ago 😞

@gumb0
Copy link
Copy Markdown
Member Author

gumb0 commented Mar 27, 2024

Not sure yet what happened to the coverage now, it was almost everything covered some time ago 😞

Seems to be beacause blockchain-tests job is segfaulting.

@gumb0
Copy link
Copy Markdown
Member Author

gumb0 commented Apr 12, 2024

Rebased.

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.

5 participants