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

Remove TXCREATE and InitcodeTransaction #76

Merged
merged 2 commits into from
May 8, 2024
Merged

Remove TXCREATE and InitcodeTransaction #76

merged 2 commits into from
May 8, 2024

Conversation

pdobacz
Copy link
Member

@pdobacz pdobacz commented Mar 25, 2024

This PR for now is just to visualize the impact of substituting TXCREATE/InitcodeTransaction/Deployer Contract with some application of "legacy-like" creation tx adapted to EOF.

Even if late, the amount of simplification this enables warrants considering it, at least for the sake of establishing the motivation for having TXCREATE and friends in EOF.

Advantages so far:

  • no new transaction type, or the hash-based initcode lookup
  • no need for two new creation opcodes, just one
  • no need for the Deployer Contract introduced via an irregular state change

Disadvantages:

  • less ability to deploy multiple contracts in one tx (new in EOF) - this requires using EOFCREATE and is more constrained by MAX_INITCODE_SIZE, where all contracts must fit in.
  • implementations need to flag execution as "initcode-mode" (in evmone msg.kind == EVMC_EOFCREATE), so that RETURNCONTRACT is interpreted correctly in EOF-creation txs
  • smart contract wallets cannot deploy arbitrary EOF code - a feature which exists in legacy (main original reason for TXCREATE to exist)

@pdobacz pdobacz self-assigned this Mar 25, 2024
@shemnon
Copy link
Contributor

shemnon commented Mar 25, 2024

lose ability to deploy multiple contracts in one tx (new in EOF)

You can do that by calling EOFCREATE in the initcode.

@pdobacz
Copy link
Member Author

pdobacz commented Mar 25, 2024

lose ability to deploy multiple contracts in one tx (new in EOF)

You can do that by calling EOFCREATE in the initcode.

Yes, but it's a tiny bit hacky, and constrained by max_initcode_size (unless the code is the same for each instance), which was not the case with InitcodeTx+TXCREATE.

I'll amend the bullet point to make it clearer.

spec/eof.md Outdated Show resolved Hide resolved
spec/eof.md Outdated
- concatenate data section with `(aux_data_offset, aux_data_offset + aux_data_size)` memory segment and update data size in the header
- if updated deploy container size exceeds `MAX_CODE_SIZE` instruction exceptionally aborts
- set `state[new_address].code` to the updated deploy container
- push `new_address` onto the stack
Copy link
Member Author

Choose a reason for hiding this comment

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

self-note: this line needs fixing in case this is ever considered.

spec/eof.md Outdated
- it is checked if the initcode container has its `len(data_section)` equal to `data_size`, i.e. data section content is exactly as the size declared in the header (see [Data section lifecycle](#data-section-lifecycle))
- transaction fails if any of those checks were invalid (gas for initcode execution is not consumed. Only intrinsic creation transaction costs were consumed)
- execute the container in "initcode-mode" and deduct gas for execution
- calculate `new_address` as `keccak256(sender || sender_nonce)[12:]`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can address be calculated with CREATE2-like scheme here instead? as we're anyway applying special EOF-rules to this kind of transaction.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't have an idea how to supply salt to here, so I punted it for now. Unless this approach is partially revived to do away with the Creator Contract, this is probably not useful (TXCREATE has good reasons to exist). Salt would need to be crammed into some non-standard place or given as argument to RETURNCONTRACT, I don't see an elegant solution, any ideas?

Copy link
Contributor

@gumb0 gumb0 Mar 27, 2024

Choose a reason for hiding this comment

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

I guess we could cram salt into data section, next to calldata 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

But then how do you fetch it and apply? Note that salt is an argument to TXCREATE, which is implicit here, we start execution just after the moment where it "would-have-been".

You either need to move it to RETURNCONTRACT or have a ugly special rule, that it is first bytes of the data_section, and the creation logic peeks there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah seems even uglier than I thought, probably not worth it, let's stick with the nonce-based scheme.

@pdobacz
Copy link
Member Author

pdobacz commented Mar 27, 2024

I'll close this PR to demonstrate we're not actively considering a design without TXCREATE (I think we aren't at this point). I'll add the motivation for TXCREATE, which we unearthed here, to the EIP-7620.

Meanwhile, we can still continue any discussions we have here ofc.

@pdobacz
Copy link
Member Author

pdobacz commented Apr 18, 2024

Cherry-picked improvements from #78 and reopened

@gumb0
Copy link
Contributor

gumb0 commented Apr 29, 2024

I would move TXCREATE / InitcodeTransaction spec into another file in a separate PR, then probably merge #78

@pdobacz
Copy link
Member Author

pdobacz commented Apr 29, 2024

I would move TXCREATE / InitcodeTransaction spec into another file in a separate PR, then probably merge #78

OK, I've marked #78 as ready to review. I will do the next steps after we merge that

@pdobacz
Copy link
Member Author

pdobacz commented Apr 30, 2024

I decided to use this PR as the "separate PR". Rebased on top of main this now only removes TXCREATE and InitcodeTransaction cleanly.

@pdobacz pdobacz marked this pull request as ready for review April 30, 2024 11:35
@pdobacz pdobacz requested a review from gumb0 April 30, 2024 11:35
@pdobacz pdobacz changed the title Discuss the version without TXCREATE Remove TXCREATE and InitcodeTransaction Apr 30, 2024
spec/eof_freezer.md Outdated Show resolved Hide resolved
spec/eof_freezer.md Outdated Show resolved Hide resolved
@pdobacz
Copy link
Member Author

pdobacz commented May 8, 2024

Decided to move ahead with this during the EOF impl call:

  • the EOF Creation transactions design is forward compatible with txcreate/initcode tx, in a tooling-friendly manner as well
  • the feature we're losing (arbitrary contract deployed via smart contract wallet) can be at worst performed in 2-steps if necessary (deploy code first, then sc deploys using EOFCREATE)
  • reduces testing vectors and complexity quite a bit
  • adding txcreate/initcode txs can be done easily in a future upgrade and we can validate the design and motivations thoroughly before that.

@pdobacz pdobacz merged commit 22e56b0 into main May 8, 2024
2 checks passed
@pdobacz pdobacz deleted the no-txcreate branch May 8, 2024 16:12
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.

4 participants