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

Allow RETURN/STOP in EOF creation tx code #119

Closed
wants to merge 1 commit into from

Conversation

pdobacz
Copy link
Member

@pdobacz pdobacz commented May 22, 2024

Sibling PR of #118. This time we drop the requirement for initcontainer s in EOF creation txs be actually validated to be... "initcontainers", in the sense of not containing RETURN/STOP.

The effect of this is that they can actually RETURN or STOP (without deploying any contract!), in addition to RETURNCONTRACT. So now the creation tx has access to the salt-based hashing scheme, and we also don't care about runtime/initcode mode of such containers.

Opening the PR just to assess the fresh idea in writing, does not mean yet we're changing the spec.

- load deploy-contract from EOF subcontainer at `deploy_container_index` in the container from which `RETURNCONTRACT` is executed
- concatenate data section with `(aux_data_offset, aux_data_offset + aux_data_size)` memory segment and update data size in the header
- let `deployed_code_size` be updated deploy container size
- if `deployed_code_size > MAX_CODE_SIZE` instruction exceptionally aborts
- set `state[new_address].code` to the updated deploy container
7. deduct `200 * deployed_code_size` gas
- deduct `200 * deployed_code_size` gas
3. If execution ends with `RETURN` or `STOP`, no code is deployed
Copy link
Member

Choose a reason for hiding this comment

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

Is the account created (e.g. have nonce 1)?

@gumb0
Copy link
Contributor

gumb0 commented May 29, 2024

This feels to me more hacky than other variants, because

  1. It requires yet another validation mode, because in other contexts only RETURNCONTRACT or STOP/RETURN are allowed, but not both
  2. RETURN (or STOP) from creation transaction does not result in deployment - this is not consistent with legacy creation transactions (so it's a special case in handling them in Host). Returned data would be just ignored.

@gumb0
Copy link
Contributor

gumb0 commented May 29, 2024

it is an error for a container to contain both RETURNCONTRACT and either of RETURN or STOP.

Is this rule still applied?

@gumb0
Copy link
Contributor

gumb0 commented May 29, 2024

If we would go with this, it seems kinda logical to also allow STOP/RETURN in EOFCREATE-referenced containers, but I don't think it's a good idea.

@gumb0
Copy link
Contributor

gumb0 commented May 29, 2024

To quote @pdobacz from discord discussions

Another way to put this. #119 makes the initcode mode validation a validation of the parent-child relation between containers, not that of the container per se. So top-levelness is irrelevant, containers which have no parent are implicitly not affected by the rule.

I think this is what my original idea was about how validation tests should work in regard to inicode mode validation, but then I realized it's pretty confusing, and better to explicitly validate either initcode mode or runtime mode for toplevel container of validation tests.

And I think similar idea applies to the spec: the spec is more clear and behaviour is better defined if we require specific mode of validation for toplevel container in all contexts where we do validation (so in the spec it is only in creation tx and later possibly TXCREATE).

containers which have no parent are implicitly not affected by the rule.

This exception makes it more complicated to figure out all the rules.

@pdobacz
Copy link
Member Author

pdobacz commented May 29, 2024

Idea dropped on the EOF impls call (reasons in comments)

@pdobacz pdobacz closed this May 29, 2024
@pdobacz pdobacz deleted the allow-stop-in-initcode branch May 29, 2024 15:41
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.

3 participants