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

Creation transaction instead of the irregular state change #78

Merged
merged 5 commits into from
Apr 30, 2024
Merged

Conversation

pdobacz
Copy link
Member

@pdobacz pdobacz commented Mar 28, 2024

A limited #76, where we keep the TXCREATE, but discuss the necessity of having the Createor Contract introduced via an irregular state change.

Instead of the "ISC", we keep the possibility of "legacy-like" creation transaction, where the entire calldata is treated as an initcontainer (and wiped, so that it behaves as empty calldata within the execution frame of the initcontainer).

Can be informally thought of how sending EOF to an "empty address" would have the same effect as an "implicit txcreate" operating on the calldata.

The lack of ability to put data in the calldata in this first iteration is substituted by putting the data into the data_section of the initcontainer. There are alternative ideas to this where "stray data" beyond the end of the declared data_section becomes calldata for the initcode execution frame.

EDIT: I've changed the proposal to the alternative mentioned above, see comments for discussion

Another alternative is to reuse the type 4 InitcodeTransaction zeroth field for fetching the initcontainer in such transactions and keep calldata behaving normally.

@pdobacz pdobacz self-assigned this Mar 28, 2024
spec/eof.md Outdated Show resolved Hide resolved
spec/eof.md Outdated

- intrinsic gas cost rules and limits defined in EIP-3860 for legacy creation transaction apply. The entire `initcontainer` is treated as initcode
- **the initcontainer and all its subcontainers are validated recursively**
- 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))
Copy link
Contributor

@gumb0 gumb0 Apr 3, 2024

Choose a reason for hiding this comment

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

This should be relaxed. The goal is to allow existing tooling to append constructor arguments as they do now, without any other change. I.e. somehow it should be allowed for data section in the initcontainer to be longer than declared.

Alternative is to consider this constructor arguments "stray data" out of initcontainer, and transaction processing would pass it as calldata to initcontainer execution.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is what the:

There are alternative ideas to this where "stray data" beyond the end of the declared data_section becomes calldata for the initcode execution frame.

bit in the gh description says. That is your second paragraph:

Alternative is to consider this constructor arguments "stray data" out of initcontainer, and transaction processing would pass it as calldata to initcontainer execution.

I think that is cleaner than rewriting the data section of the initcontainer to include the stray data.

That being said, the PR diff itself right now takes on the model where the tooling would need to include the constructor args in the "proper" data section. I don't have a strong opinion on whether that is better. However, I think Charles mentioned somewhere on discord that tooling will not have a problem with either version, so we should just keep the one which is the cleanest and most consistent from our (EVM/EOF) standpoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think asking the tooling to adapt defeats the entire purpose of using old transaction types for this. If the tooling needs to change, we could just make this a new type 5 transaction without all the hacks we need to fit EOF into old transaction types (empty to, input in data section, having to live with CREATE address scheme)

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, good point, then lets make

Alternative is to consider this constructor arguments "stray data" out of initcontainer, and transaction processing would pass it as calldata to initcontainer execution.

the baseline. I'll make adjustments to the PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

@pdobacz pdobacz Apr 11, 2024

Choose a reason for hiding this comment

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

@gumb0 @shemnon When implementing in evmone, I noticed this alternative has an ugly consequence in EOF validation. The initcontainer sent with empty to with some extra bytes (to become calldata / constructor args) doesn't pass a validation rule we currently have, that there are no bytes in the container beyond what the data_size would indicate (there can be less bytes for deployed container, but not more!). This rule is of course very useful and desired for all other cases of EOF validation.

Making an exception from this rule for legacy-eof-creation-txs is not ideal (I am experimenting with this rn). Did you consider the 2nd alternative I posted earlier? This:

reuse the type 4 InitcodeTransaction zeroth field for fetching the initcontainer in such transactions and keep calldata behaving normally.

So here an InitcodeTransaction sent with empty to causes the initcontainer to be fetched from initcodes[0] and tx' calldata functions normally. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

The initcontainer sent to 0x0 with some extra bytes (to become calldata / constructor args) doesn't pass a validation rule we currently have, that there are no bytes in the container beyond what the data_size would indicate (there can be less bytes for deployed container, but not more!).

Did you mean "sent with empty to" instead of sent to 0x0 here? (otherwise I don't see what's the significance of address 0x0)

This conflict with a validation rule feels like a technicality to me, because entire data field of transaction can be seen as concatenation of EOF container + calldata, where EOF container is valid without stray data.

In evmone implementation I think I see how this can cause some problems, maybe some refactoring needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you mean "sent with empty to" instead of sent to 0x0 here? (otherwise I don't see what's the significance of address 0x0)

yeah, brainfart on my part, fixed

This conflict with a validation rule feels like a technicality to me, because entire data field of transaction can be seen as concatenation of EOF container + calldata, where EOF container is valid without stray data.

But "EOF container + calldata" is what we know when we concatenate bytes, from the standpoint of EOF validating a sequence of bytes in data it is indistinguishable from an invalid EOF container (if we include the said rule to apply for all containers).

And what do you think about the "InitcodeTransaction -> empty address" alternative per se?

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean "sent with empty to" instead of sent to 0x0 here? (otherwise I don't see what's the significance of address 0x0)

yeah, brainfart on my part, fixed

This conflict with a validation rule feels like a technicality to me, because entire data field of transaction can be seen as concatenation of EOF container + calldata, where EOF container is valid without stray data.

But "EOF container + calldata" is what we know when we concatenate bytes, from the standpoint of EOF validating a sequence of bytes in data it is indistinguishable from an invalid EOF container (if we include the said rule to apply for all containers).

The idea I had how so far to address this in implementation:

  • Validate only the header first,
  • get all section sizes and calculate full container size,
  • Get tx.data[0:container_size] and validate it as initcode container,
  • tx.data[container_size:] is calldata.

But I don't think we need to change the spec much to reflect these implementation details.

And what do you think about the "InitcodeTransaction -> empty address" alternative per se?

I like this idea, it's cleaner in many ways, but I think decoupling from TXCREATE is valuable.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea I had how so far to address this in implementation:
* Validate only the header first,
* get all section sizes and calculate full container size,
* Get tx.data[0:container_size] and validate it as initcode container,
* tx.data[container_size:] is calldata.

I know we've moved on, and this will work, but I'm still having many reservations. On some level it's just like adding a flag to the EOF validation (whether we're in a creation tx or not). The additions to the spec you've proposed make this evident, and also the algo you've listed boils down to this.

Now we have some additional inputs since our discussion started ☝️

One is re this:

If the tooling needs to change, we could just make this a new type 5 transaction without all the hacks we need to fit EOF into old transaction types (empty to, input in data section, having to live with CREATE address scheme)

we now know type 5 transactions is in practice almost surely not happening (type 4 transaction is disputed), so that is not an option we're trading for.

Second is another declaration from last EOF impl call that tooling adapting is not a problem. And now when you take this apart:

  • option with calldata appended and EOF validation flag - you get constructor args with CALLDATA* instructions
  • option without that - you get constructor args with DATA* instructions

it seems like it's literally just replacing 3 opcode bytes with other 3 opcodes bytes, even offsets are the same! (if zealous, one can also use DATALOADN, but that's optional). Well this and put data_size into the header, which you need to have tooling build anyway, that seems like it.

It would be ideal if we got more input from tooling builders.

spec/eof.md Outdated Show resolved Hide resolved
spec/eof.md Outdated Show resolved Hide resolved
Co-authored-by: Andrei Maiboroda <andrei@ethereum.org>
@gumb0 gumb0 changed the title Discuss the version without the irregular state change Creation transaction instead of the irregular state change Apr 30, 2024
@pdobacz pdobacz merged commit 47aa8ae into main Apr 30, 2024
2 checks passed
@pdobacz pdobacz deleted the no-isc branch April 30, 2024 10:51
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.

None yet

2 participants