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

Feature/initcode size limit using transaction creations #2665

Conversation

fmacleal
Copy link
Contributor

@fmacleal fmacleal commented Aug 2, 2024

Adding the verification of initcode size limit for the contract creation during transaction execution

Description

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Tests for the changes have been added (for bug fixes / features)
  • Requires Activation Code (Hard Fork)
  • Other information:

@fmacleal fmacleal force-pushed the feature/introduce_initcode_size_limit branch from 3fd6e2d to 8b74286 Compare August 2, 2024 10:29
@fmacleal fmacleal force-pushed the feature/initcode_size_limit_using_transaction_creations branch 3 times, most recently from bcefff3 to 6ee3c68 Compare August 6, 2024 17:33
@fmacleal fmacleal marked this pull request as ready for review August 6, 2024 17:33
Copy link
Contributor

@asoto-iov asoto-iov left a comment

Choose a reason for hiding this comment

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

Maybe it would be nice to add the test cases suggested by the EIP-3860:

Tests should include the following cases:

Creation transaction with gas limit enough to cover initcode cost
Creation transaction with gas limit enough to cover intrinsic cost except initcode cost
CREATE/CREATE2/creation transaction with len(initcode) at MAX_INITCODE_SIZE
CREATE/CREATE2/creation transaction with len(initcode) at MAX_INITCODE_SIZE+1

@fmacleal fmacleal force-pushed the feature/initcode_size_limit_using_transaction_creations branch from e561020 to 5a1ed48 Compare August 8, 2024 15:53
@fmacleal fmacleal requested review from asoto-iov and Vovchyk August 9, 2024 12:01
@fmacleal fmacleal force-pushed the feature/introduce_initcode_size_limit branch from 8b74286 to 482204c Compare August 12, 2024 12:21
@fmacleal fmacleal force-pushed the feature/initcode_size_limit_using_transaction_creations branch from 5a1ed48 to f5dbbe0 Compare August 12, 2024 12:58
In order to implement the verification of initcode size, necessary for the RSKIP438,
we need the constant value with the max allowed value. So we can start to validate
and use it on the logic of the verification
In order to implement the verification of initcode size, necessary for the RSKIP438,
we need the constant value with the max allowed value. So we can start to validate
and use it on the logic of the verification

- Adding scenarios of test for the contract creation with DSL
- The validation for the initCode size test was added
- The validation is based on the RSKIP that enables it
- Validation added for the contractCreation during transaction execution

Finished the implementation of transactionCost and initcode size validation

- All the logic related with transaction cost was changed to meet the new cost criteria
- More tests were added to be sure that the flow is following the RSKIP438
- Tests were added to validate that this cost and validation doesn't impact before reach the activation height

Applied some suggestions from review

Adding tests to validate the failure if we don't have enough gas to cover initCode
@fmacleal fmacleal force-pushed the feature/initcode_size_limit_using_transaction_creations branch from f5dbbe0 to 25d1e0a Compare August 23, 2024 17:14
@fmacleal fmacleal requested a review from a team as a code owner August 23, 2024 17:14
RIT Release Bot and others added 2 commits August 23, 2024 19:17
In order to have the exact same logic for the initcode cost calculation,
we introduced some new classes that can be easily reused by the TransactionExecutor and VM classes. This way, if this logic change,
we won't have to change the code in multiple places.

It was also introduced an interface for it, so we can expand this idea and
in the future, be able to calculate total transactions just calling a method
to calculate it independent of the logic used.
In order to implement the verification of initcode size, necessary for the RSKIP438,
we need the constant value with the max allowed value. So we can start to validate
and use it on the logic of the verification
@fmacleal fmacleal force-pushed the feature/introduce_initcode_size_limit branch from 482204c to 59943d6 Compare August 23, 2024 17:19
fmacleal and others added 2 commits August 29, 2024 18:23
Co-authored-by: Nazaret García Revetria <[email protected]>
…tcode_size_limit_using_transaction_creations
@fmacleal fmacleal requested a review from rmoreliovlabs August 29, 2024 17:00
Copy link
Contributor

@nagarev nagarev left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit: Good documenting on the DSL files! 🚀

@rmoreliovlabs
Copy link
Contributor

Other than the comment I left, LGTM! Great work on handling this complex task

Applying suggestions from reviews

Co-authored-by: Vovchyk <[email protected]>
@fmacleal fmacleal force-pushed the feature/initcode_size_limit_using_transaction_creations branch from f607c0f to 91ea7d1 Compare September 5, 2024 08:34
@fmacleal fmacleal requested a review from Vovchyk September 5, 2024 08:36
@fmacleal fmacleal force-pushed the feature/initcode_size_limit_using_transaction_creations branch from 91ea7d1 to 3df1686 Compare September 5, 2024 08:52
@fmacleal fmacleal force-pushed the feature/initcode_size_limit_using_transaction_creations branch from 3df1686 to dd3cee9 Compare September 5, 2024 11:00
@fmacleal fmacleal force-pushed the feature/introduce_initcode_size_limit branch 2 times, most recently from 319d678 to f5da98a Compare September 5, 2024 14:13
* Initcode validation for opcodes contract creation

- Adding scenarios of test for the contract creation with DSL
- The validation for the initCode size test was added
- The validation is based on the RSKIP that enables it
- Validation added for the contractCreation during transaction execution
- In order to have the exact same logic for the initcode cost calculation,
we introduced some new classes that can be easily reused by the TransactionExecutor and VM classes. This way, if this logic change,
we won't have to change the code in multiple places.

It was also introduced an interface for it, so we can expand this idea and
in the future, be able to calculate total transactions just calling a method
to calculate it independent of the logic used.

- Adding scenarios of test for the contract creation with DSL
- The validation is based on the RSKIP that enables it

* Added the remaining test scenarios for contracts with opcode creation

* Applying suggestions from review

* Applying some more suggestions from review
---------
@fmacleal fmacleal force-pushed the feature/initcode_size_limit_using_transaction_creations branch 2 times, most recently from 2a0a10b to 5c25f3a Compare September 6, 2024 12:21
…tcode_size_limit_using_transaction_creations
@fmacleal fmacleal force-pushed the feature/initcode_size_limit_using_transaction_creations branch from 5c25f3a to 035b7d2 Compare September 6, 2024 13:39
Copy link

sonarqubecloud bot commented Sep 6, 2024

@fmacleal fmacleal merged commit 236401e into feature/introduce_initcode_size_limit Sep 8, 2024
6 checks passed
fmacleal added a commit that referenced this pull request Sep 9, 2024
* Initial constants and config for the max initcode size

In order to implement the verification of initcode size, necessary for the RSKIP438,
we need the constant value with the max allowed value. So we can start to validate
and use it on the logic of the verification

- Adding scenarios of test for the contract creation with DSL
- The validation for the initCode size test was added
- The validation is based on the RSKIP that enables it
- Validation added for the contractCreation during transaction execution

Finished the implementation of transactionCost and initcode size validation

- All the logic related with transaction cost was changed to meet the new cost criteria
- More tests were added to be sure that the flow is following the RSKIP438
- Tests were added to validate that this cost and validation doesn't impact before reach the activation height

Applied some suggestions from review

Adding tests to validate the failure if we don't have enough gas to cover initCode

* Refactor from the Initcode cost calculation

In order to have the exact same logic for the initcode cost calculation,
we introduced some new classes that can be easily reused by the TransactionExecutor and VM classes. This way, if this logic change, we won't have to change the code in multiple places.

It was also introduced an interface for it, so we can expand this idea and
in the future, be able to calculate total transactions just calling a method
to calculate it independent of the logic used.

* Applying some more suggestions from review

Co-Authored-By: Nazaret García Revetria <[email protected]>
Co-Authored-By: Vovchyk <[email protected]>
fmacleal added a commit that referenced this pull request Sep 9, 2024
* Initial constants and config for the max initcode size

In order to implement the verification of initcode size, necessary for the RSKIP438,
we need the constant value with the max allowed value. So we can start to validate
and use it on the logic of the verification

- Adding scenarios of test for the contract creation with DSL
- The validation for the initCode size test was added
- The validation is based on the RSKIP that enables it
- Validation added for the contractCreation during transaction execution

Finished the implementation of transactionCost and initcode size validation

- All the logic related with transaction cost was changed to meet the new cost criteria
- More tests were added to be sure that the flow is following the RSKIP438
- Tests were added to validate that this cost and validation doesn't impact before reach the activation height

Applied some suggestions from review

Adding tests to validate the failure if we don't have enough gas to cover initCode

* Refactor from the Initcode cost calculation

In order to have the exact same logic for the initcode cost calculation,
we introduced some new classes that can be easily reused by the TransactionExecutor and VM classes. This way, if this logic change, we won't have to change the code in multiple places.

It was also introduced an interface for it, so we can expand this idea and
in the future, be able to calculate total transactions just calling a method
to calculate it independent of the logic used.

* Applying some more suggestions from review

Co-Authored-By: Nazaret García Revetria <[email protected]>
Co-Authored-By: Vovchyk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants