search

ethereum / py-evm

A Python implementation of the Ethereum Virtual Machine
https://py-evm.readthedocs.io/en/latest/
MIT License
2.27k stars 654 forks source link

Discussion : Remove support for multiple types in the EVM stack #1656

Open Bhargavasomu opened 5 years ago

Bhargavasomu commented 5 years ago

What is wrong?

As part of issue #1398 this new issue arised from this https://github.com/ethereum/py-evm/issues/1398#issuecomment-432303381. It was proposed to remove the different types of elements in the stack (bytes and int), and put only one element type in the Stack

How can it be fixed

Fill this in if you know how to fix it.

Bhargavasomu commented 5 years ago

@cburgdorf @pipermerriam @carver I see a problem with this proposed approach. Previously (current live version scenario) we are storing both bytes and int, and most of the time, it is expected that if the user requests the popped data to be in the bytes format, the popped data would be in the bytes format. But now we are entirely converting the data stored in the stack into a single type, and hence if the user asks for the other type, we would have to convert it into that type and present it to the user. I believe that this would take a lot more time. Please correct me if I am wrong.

carver commented 5 years ago

Silly question inbound: is there any time that leading 0s matter in bytes? Like is there ever a difference between b'\0' and b'\0' *32 on the stack? I assume not, if the EVM is simulating a fixed 32-byte instruction width.

But if so, converting all values to int would be problematic, of course. Having everything be bytes would be fine.

I believe that this would take a lot more time.

You're talking about the time to convert the value from int to bytes? Seems relatively small compared to other costs, but the only way to know for sure is to benchmark.

pipermerriam commented 5 years ago

and hence if the user asks for the other type, we would have to convert it into that type and present it to the user.

Part of this change would be removing this type of conversion from the Stack API and requiring call sites to 1) expect to get an int back and 2) do any necessary conversions to bytes types locally.