Implement EOF #972

gurukamath · 2024-06-27T06:00:48Z

What was wrong?

The current specs do not support EOF

Related to Issue #971

How was it fixed?

Implement the 11 EOF EIPs

Cute Animal Picture

SamWilsn

Partial review.

src/ethereum/prague/vm/__init__.py

SamWilsn · 2024-07-11T16:04:22Z

src/ethereum/prague/vm/__init__.py

@@ -95,6 +111,7 @@ class Evm:
    error: Optional[Exception]
    accessed_addresses: Set[Address]
    accessed_storage_keys: Set[Tuple[Address, Bytes32]]
+    eof: EOF


This is not a descriptive field name. How about:

Suggested change

eof: EOF

container_format: Eof

src/ethereum/prague/vm/__init__.py

SamWilsn · 2024-07-11T18:54:53Z

src/ethereum/prague/vm/eof.py

+    header_end_index: Uint
+
+
+def get_opcode(opcode: int, eof: EOF) -> Ops:


We should name this more descriptively. Some (bad) ideas:

integer_to_op

parse_op

validate_op

Updated as map_int_to_op

src/ethereum/prague/vm/eof.py

SamWilsn · 2024-07-11T21:36:33Z

src/ethereum/prague/vm/__init__.py

+    pc: Uint
+    stack: List[U256]
+    memory: bytearray
+    code: Bytes


Should we create NewType for code vs. container vs. whatever other types of sections there are?

I am interpreting this as the code being implemented. If it is legacy, then it's just the code. If EOF, it is the code in the section being currently implemented.

SamWilsn · 2024-07-11T21:37:41Z

src/ethereum/prague/vm/eof.py

+    )
+
+
+def validate_header(container: bytes) -> EOFMetadata:


Why is this separate from meta_from_valid_eof1_container?

Combined into one function with a flag to validate

SamWilsn · 2024-07-11T21:39:02Z

src/ethereum/prague/vm/eof.py

+            raise InvalidEOF("Invalid input/output for first section")
+
+
+def get_valid_instructions(code: bytes) -> Set[int]:


Should have a better name for this. From just the signature, it looks like it returns a uniqued list of all the opcodes used in code, but I think it returns offsets instead?

Has now been integrated into the validate_code_section function.

gurukamath · 2024-08-29T11:31:53Z

Rebased on latest forks/prague

SamWilsn

Partial review (5/22)

SamWilsn · 2024-08-31T16:19:38Z

src/ethereum/prague/fork.py

+    except InvalidEof:
+        output = MessageCallOutput(
+            gas, U256(0), tuple(), set(), set(), InvalidEof(), b""
+        )


I think we should preserve the original exception.

Suggested change

except InvalidEof:

output = MessageCallOutput(

gas, U256(0), tuple(), set(), set(), InvalidEof(), b""

)

except InvalidEof as error:

output = MessageCallOutput(

gas, U256(0), tuple(), set(), set(), error, b""

)

SamWilsn · 2024-08-31T16:22:29Z

src/ethereum/prague/utils/address.py

+    if data > MAX_ADDRESS_U256:
+        raise ValueError("Address is too large")
+    return Address(data.to_be_bytes32()[-20:])


Would this be more clear if we used to_be_bytes, left_pad_zero_bytes, and Address.LENGTH?

SamWilsn · 2024-08-31T16:23:00Z

src/ethereum/prague/utils/address.py

@@ -62,7 +90,7 @@ def compute_contract_address(address: Address, nonce: Uint) -> Address:
    return Address(padded_address)


-def compute_create2_contract_address(
+def compute_contract_address_2(


Perhaps?

Suggested change

def compute_contract_address_2(

def compute_contract_address_with_salt(

SamWilsn · 2024-08-31T16:23:29Z

src/ethereum/prague/utils/address.py

+    return Address(data.to_be_bytes32()[-20:])
+
+
+def compute_contract_address_1(address: Address, nonce: Uint) -> Address:


Suggested change

def compute_contract_address_1(address: Address, nonce: Uint) -> Address:

def compute_contract_address_with_nonce(address: Address, nonce: Uint) -> Address:

Perhaps?

SamWilsn · 2024-08-31T16:24:50Z

src/ethereum/prague/utils/message.py

    elif isinstance(target, Address):
        current_target = target
        msg_data = data
        code = get_account(env.state, target).code
        if code_address is None:
            code_address = target
+
+        if get_eof_version(code) == EofVersion.LEGACY:


This is getting really nested. Is there a good way to refactor this?

SamWilsn · 2024-08-31T16:28:41Z

src/ethereum/prague/utils/message.py

+
+        if get_eof_version(code) == EofVersion.LEGACY:
+            eof = None
+        else:


Using a catch-all here will make this more difficult to catch if we add another EofVersion variant.

Is mypy smart enough to let us do something like:

eof_version = get_eof_version(code) if eof_version == EofVersion.LEGACY: ... elif eof_version == EofVersion.EOF1: ... else: assert_never(eof_version)

IIRC, assert_never is available in typing_extensions.

SamWilsn

More review (14 / 22)

SamWilsn · 2024-09-03T15:56:48Z

src/ethereum/prague/vm/eof/__init__.py

+    stack_height: Optional[OperandStackHeight]
+
+
+SectionMetadata = Dict[Uint, InstructionMetadata]


Mostly because I'm just curious, but why Dict[Uint, InstructionMetadata] over List[Optional[InstructionMetadata]]? Is this very hole-y data?

SamWilsn · 2024-09-03T15:57:23Z

src/ethereum/prague/vm/eof/__init__.py

+    from ..instructions import Ops
+
+    eof: Eof
+    sections: Dict[Uint, SectionMetadata]


Suggested change

sections: Dict[Uint, SectionMetadata]

sections: SectionMetadata

Maybe?

SamWilsn · 2024-09-03T16:04:30Z

src/ethereum/prague/vm/eof/instructions_check.py

+        The current validator instance.
+    """
+    code = validator.current_code
+    position = Uint(validator.current_pc)


Isn't current_pc already a Uint?

SamWilsn · 2024-09-03T16:06:04Z

src/ethereum/prague/vm/eof/instructions_check.py

+    target_type = eof_meta.type_section_contents[target_index]
+    target_outputs = target_type[1]
+
+    if target_outputs == 0x80:


Where does 0x80 come from? Should it be a constant?

SamWilsn · 2024-09-03T16:07:26Z

src/ethereum/prague/vm/eof/instructions_check.py

+    current_outputs = current_section_type[1]
+    target_outputs = target_section_type[1]
+
+    if target_outputs != 0x80 and target_outputs > current_outputs:


I assume this is the same 0x80, and so should be in a constant.

SamWilsn · 2024-09-03T16:15:20Z

src/ethereum/prague/vm/eof/utils.py

+        raise InvalidEof("Kind code not specified in header")
+    kind_code = container[counter]
+    counter += 1
+    if validate and kind_code != 2:


Is it possible to disentangle validation from the other logic? Perhaps in two functions, maybe with a dataclass to store intermediate values?

Just seems like this function is a bit complex.

SamWilsn · 2024-09-03T16:18:06Z

src/ethereum/prague/vm/eof/validation.py

+        if op_metadata.stack_height.max > computed_maximum_stack_height:
+            computed_maximum_stack_height = op_metadata.stack_height.max
+
+    if computed_maximum_stack_height > 1023:


Should we make an EOF max stack height constant?

SamWilsn · 2024-09-03T16:22:21Z

src/ethereum/prague/vm/eof/validation.py

+        ]
+        for index in range(len(eof_meta.container_section_contents)):
+            if (
+                index in eofcreate_references


Can we make some intermediate variables to clean up these conditions?

Maybe:

eofcreate_seen = index in eofcreate_references returncontract_seen = index in returncontract_references if eofcreate_seen and returncontract_seen: ...

SamWilsn · 2024-09-03T16:24:49Z

src/ethereum/prague/vm/eof/validation.py

+    if (
+        len(container) < EOF_MAGIC_LENGTH
+        or container[:EOF_MAGIC_LENGTH] != EOF_MAGIC
+    ):
+        raise InvalidEof("Invalid magic")


Similarly, maybe:

Suggested change

if (

len(container) < EOF_MAGIC_LENGTH

or container[:EOF_MAGIC_LENGTH] != EOF_MAGIC

):

raise InvalidEof("Invalid magic")

if len(container) < EOF_MAGIC_LENGTH:

raise InvalidEof("Too short for magic")

if container[:EOF_MAGIC_LENGTH] != EOF_MAGIC:

raise InvalidEof("Invalid magic")

Personally I find separate if blocks easier to read than a single long if condition.

SamWilsn · 2024-09-03T16:26:47Z

src/ethereum/prague/vm/instructions/control_flow.py

+
+    # PROGRAM COUNTER
+    relative_offset = int.from_bytes(
+        evm.code[evm.pc + 1 : evm.pc + 3], "big", signed=True


Is it ever possible to get an IndexError here, or is this safe because of the EOF validation?

SamWilsn

The last bit

SamWilsn · 2024-09-03T18:58:09Z

src/ethereum/prague/vm/instructions/environment.py

+    if evm.eof is None and Uint(return_data_start_position) + Uint(size) > len(
+        evm.return_data
+    ):
        raise OutOfBoundsRead


Maybe?

Suggested change

if evm.eof is None and Uint(return_data_start_position) + Uint(size) > len(

evm.return_data

):

raise OutOfBoundsRead

if evm.eof is None:

if Uint(return_data_start_position) + Uint(size) > len(evm.return_data):

raise OutOfBoundsRead

SamWilsn · 2024-09-03T19:03:04Z

src/ethereum/prague/vm/instructions/environment.py

+    charge_gas(evm, copy_gas_cost + extend_memory.cost)
+
+    # OPERATION
+    assert evm.eof is not None


I assume there's a check somewhere else that enforces datacopy is only used in EOF contracts? Should we leave a comment explaining that?

gurukamath · 2024-09-24T08:25:15Z

Rebased on current forks/prague and updated tests to the latest release

This commit updates to the latest test releases and fixes some minor bugs exposed by the increased coverage. In particular, the bugs related to faulty return flag in the type section are fixed. Any code section that has RETF instruction or has a JUMPF to a returning section is considered a returning section. Otherwise, the code section is considered non-returning. The output in the type section should reflect this behaviour. If not, the container is invalid

gurukamath force-pushed the eips/prague/eof branch from c5318cc to ab31dd7 Compare June 27, 2024 07:39

gurukamath changed the base branch from forks/prague to master July 6, 2024 10:46

gurukamath force-pushed the eips/prague/eof branch from ab31dd7 to f672f5d Compare July 11, 2024 20:29

SamWilsn reviewed Jul 11, 2024

View reviewed changes

gurukamath force-pushed the eips/prague/eof branch from f672f5d to 47180a1 Compare August 5, 2024 06:49

gurukamath force-pushed the eips/prague/eof branch from 47180a1 to 5869072 Compare August 13, 2024 09:07

gurukamath marked this pull request as ready for review August 13, 2024 09:07

gurukamath changed the base branch from master to forks/prague August 15, 2024 14:13

gurukamath force-pushed the eips/prague/eof branch from 999635f to 6a7de73 Compare August 29, 2024 11:30

gurukamath force-pushed the eips/prague/eof branch from 6a7de73 to 0157e72 Compare August 31, 2024 05:16

SamWilsn reviewed Aug 31, 2024

View reviewed changes

SamWilsn reviewed Sep 3, 2024

View reviewed changes

gurukamath mentioned this pull request Sep 6, 2024

eels eof implementation update 2 ipsilon/eof#163

Merged

gurukamath force-pushed the eips/prague/eof branch from b234ede to 0dbb07f Compare September 24, 2024 08:23

gurukamath added 13 commits September 25, 2024 15:06

Implement EIP-3540

7eac383

Implement EIP-3670

bc79c7d

Implement EIP-4200

2119bdf

Implement EIP-4750

c4ecc5b

Implement EIP-7069

f11a4c4

Implement EIP-7480

65e6741

post review updates 1

6b4ae75

Implement EIP-5450

efd5ae1

refactor EIP-5450

4d4489c

Implement EIP-663

a9477c1

Implement EIP-7620

f5e46ba

Implement EIP-7698

3dad6be

Implement EIP-6206

c672b8f

gurukamath added 19 commits September 25, 2024 15:06

Run tests

b94673e

start eof code re-factor

469c04a

address mypy nits

0a3d8ce

re-factor utility functions

83cab5e

add eof to message

154b4c1

refactor eof validation

cdabb39

refactor stack validation

dd0a67f

create eof package

3eafdc1

re-organise EOF types

a3d43a6

run evm tools tests

8859183

clean-up

d8dec66

doc fix

a6ddda6

minor fixes

0957721

refactor eof-call opcodes

6c73497

run latest tests

2199950

minor fixes + more eof tests

255d0e9

update to EEST 1.0.9

9d8c80e

Update to EEST EOF release 1.1.1

9c12043

gurukamath force-pushed the eips/prague/eof branch from 0dbb07f to 9c12043 Compare September 25, 2024 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement EOF #972

Implement EOF #972

gurukamath commented Jun 27, 2024

SamWilsn left a comment

SamWilsn Jul 11, 2024

SamWilsn Jul 11, 2024

gurukamath Aug 13, 2024

SamWilsn Jul 11, 2024

gurukamath Aug 13, 2024

SamWilsn Jul 11, 2024

gurukamath Aug 13, 2024

SamWilsn Jul 11, 2024

gurukamath Aug 13, 2024

gurukamath commented Aug 29, 2024

SamWilsn left a comment

SamWilsn Aug 31, 2024

SamWilsn Aug 31, 2024

SamWilsn Aug 31, 2024

SamWilsn Aug 31, 2024

SamWilsn Aug 31, 2024

SamWilsn Aug 31, 2024

SamWilsn left a comment

SamWilsn Sep 3, 2024

SamWilsn Sep 3, 2024

SamWilsn Sep 3, 2024

SamWilsn Sep 3, 2024

SamWilsn Sep 3, 2024

SamWilsn Sep 3, 2024

SamWilsn Sep 3, 2024

SamWilsn Sep 3, 2024

SamWilsn Sep 3, 2024

SamWilsn Sep 3, 2024

SamWilsn left a comment

SamWilsn Sep 3, 2024

SamWilsn Sep 3, 2024

gurukamath commented Sep 24, 2024

		header_end_index: Uint


		def get_opcode(opcode: int, eof: EOF) -> Ops:

		raise InvalidEOF("Invalid input/output for first section")


		def get_valid_instructions(code: bytes) -> Set[int]:

	def compute_contract_address_2(
	def compute_contract_address_with_salt(

		return Address(data.to_be_bytes32()[-20:])


		def compute_contract_address_1(address: Address, nonce: Uint) -> Address:

	def compute_contract_address_1(address: Address, nonce: Uint) -> Address:
	def compute_contract_address_with_nonce(address: Address, nonce: Uint) -> Address:

		stack_height: Optional[OperandStackHeight]


		SectionMetadata = Dict[Uint, InstructionMetadata]

	sections: Dict[Uint, SectionMetadata]
	sections: SectionMetadata

Implement EOF #972

Are you sure you want to change the base?

Implement EOF #972

Conversation

gurukamath commented Jun 27, 2024

What was wrong?

How was it fixed?

Cute Animal Picture

SamWilsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurukamath commented Aug 29, 2024

SamWilsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamWilsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamWilsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurukamath commented Sep 24, 2024