Fixes to the "initcode mode" validation spec #129
Conversation
| - `DATALOADN`'s `immediate + 32` must be within `pre_deploy_data_size` (see [Data Section Lifecycle](#data-section-lifecycle)) | ||
| - the part of the data section which exceeds these bounds (the `dynamic_aux_data` portion) needs to be accessed using `DATALOAD` or `DATACOPY` | ||
| - no unreachable code sections are allowed, i.e. every code section can be reached from the 0th code section with a series of CALLF / JUMPF instructions, and section 0 is implicitly reachable. | ||
| - it is an error for a container to contain both `RETURNCONTRACT` and either of `RETURN` or `STOP`. | ||
| - it is an error for a subcontainer to never be referenced in code sections of its parent container | ||
| - for terminology purposes, the following concepts are defined: |
There was a problem hiding this comment.
I'm leaning to keeping these definitions, I don't feel like they dictate particular implementation, and I expect these concepts to appear in tests (at least as "initcode container" flag to mark some tests, "runtime" should be default and might not be mentioned).
But it would make sense to move the definitions to above their first use.
There was a problem hiding this comment.
I expect these concepts to appear in tests (at least as "initcode container" flag to mark some tests
Just got an idea - this flag could just as well be "creation_tx_container", as it would be what it is in production. Then the default (former "runtime container"), would be much more natural. WDYT?
There was a problem hiding this comment.
However, actually, initcode container or initcontainer is fine as a testing flag, even if these definitions are not here. The rule is in there in cases where it applies (creation tx, txcreate, eofcreate), and all of these name this "thing" as initcontainer, so I think it is all clear enough.
| @@ -288,18 +288,15 @@ The following instructions are introduced in EOF code: | |||
| - the first code section must have a type signature `(0, 0x80, max_stack_height)` (0 inputs non-returning function) | |||
| - `EOFCREATE` `initcontainer_index` must be less than `num_container_sections` | |||
| - `EOFCREATE` the subcontainer pointed to by `initcontainer_index` must have its `len(data_section)` equal `data_size`, i.e. data section content is exactly as the size declared in the header (see [Data section lifecycle](#data-section-lifecycle)) | |||
| - `EOFCREATE` the subcontainer pointed to by `initcontainer_index` must be an "initcode" subcontainer, that is, it *must not* contain either a `RETURN` or `STOP` instruction. | |||
| - `EOFCREATE` the subcontainer pointed to by `initcontainer_index` *must not* contain either a `RETURN` or `STOP` instruction. | |||
There was a problem hiding this comment.
Rephrasing like this could hint that it is part of subcontainer's validation and not EOFCREATE's validaton, i.e. suggest outside-in view.
| - `EOFCREATE` the subcontainer pointed to by `initcontainer_index` *must not* contain either a `RETURN` or `STOP` instruction. | |
| - If the subcontainer is pointed to by `initcontainer_index` of some `EOFCREATE` instruction, it *must not* contain either a `RETURN` or `STOP` instruction. |
But I'm not very sure if it's worth to change.
|
OK, since the removal of definitions doesn't sound like a good idea, I'll close this and revisit in separate PR(s). BTW @gumb0 , do we still want to push the other change:
? |
The change in implementation for this: ethereum/evmone@da6dd49 Maybe not a huge simplification, but I think I'm still in favor of it, for the benefit of simpler reasoning about what is allowed in which container, and for reduced testing surface. |
Extracted this to the PR linked ☝️ |
This combines two changes:
initcontainer_indexanddeploy_container_indexof opcodes. NOTE this one is a rather subjective proposal, so do not hesitate to turn it down if you think these definitions are useful.The intention of this PR as a whole is to remove the "gray area" edge cases of EOF validation and also to not mix implementation details into the spec.