nimbus-eth1 icon indicating copy to clipboard operation
nimbus-eth1 copied to clipboard

Switch `GasInt` to `uint64`

Open arnetheduck opened this issue 1 year ago • 2 comments

The initial choice in https://github.com/status-im/nimbus-eth1/issues/35#issuecomment-391726518 is argued to simplify development by raising a Defect whenever an out-of-bounds computation happens.

While this might have been an appropriate choice in early-stages design, it is not a reliable choice for production code, also because types where GasInt is used are RLP-serialized and RLP doesn't support signed integers.

Since geth uses uint64, this will also ensure that our handling is similar to that of geth:

https://github.com/ethereum/go-ethereum/blob/682ae838b2312a4ec8e5581069039b567e33c7c2/core/types/block.go#L76C2-L76C10

As an alternative to plain uint64, we could also develop a special "checked uint64" in which all operations return an overflow flag that is checked where applicable.

arnetheduck avatar Jun 04 '24 11:06 arnetheduck

A note: Some of the gas calculation depends on the int signedness, e.g. in evm/interpreter/gas_costs.nim and evm/interpreter/gas_meter.nim and in calculateAndPossiblyRefundGas of nimbus/transaction/call_common.nim are using equations that will result in negative number as intermediate value. We need to rewrite the equations to avoid negative number when fixing this issue.

jangko avatar Jun 04 '24 12:06 jangko

Something to be careful about: code like https://github.com/status-im/nimbus-eth1/blob/32c51b14a4b574433d1cd0a48b3ebcb37022aa52/nimbus/core/validate.nim#L329 will allow negative values (!)

arnetheduck avatar Jun 07 '24 17:06 arnetheduck