Refactor gas constants

[cburgdorf]

What is wrong?

I noticed that our gas constants are a bit messy and should be reorganized.

Let's take GAS_BALANCE as an example which is defined as such:

py-evm/eth/constants.py

Line 43 in bf1d3c2

GAS_BALANCE = 20

For someone new to the code base this may seem as if retrieving the balance from some account has a fixed gas cost of 20 when in reality it is 400 as specified with the tangerine whistle fork.

py-evm/eth/vm/forks/tangerine_whistle/constants.py

Line 13 in bf1d3c2

GAS_BALANCE_EIP150 = 400

How can it be fixed

My gut feeling says that only true constants™ belong into eth.constants and that all fork specific constants should live in evm.vm.forks.<fork_name>.constants.

[pipermerriam]

I'm 👍 on this. I have been thinking similarly about our opcodes. That all opcodes in eth.vm.logic should be moved into the FrontierVM.

[cburgdorf]

I have been thinking similarly about our opcodes. That all opcodes in eth.vm.logic should be moved into the FrontierVM

Mmh...I'm not so sure about that one. After all, these are logic functions that even private chains may want to use. We already configure the opcodes per fork.

py-evm/eth/vm/forks/constantinople/opcodes.py

Lines 24 to 40 in bf1d3c2

	UPDATED_OPCODES = {
	opcode_values.SHL: as_opcode(
	logic_fn=arithmetic.shl,
	mnemonic=mnemonics.SHL,
	gas_cost=constants.GAS_VERYLOW,
	),
	opcode_values.SHR: as_opcode(
	logic_fn=arithmetic.shr,
	mnemonic=mnemonics.SHR,
	gas_cost=constants.GAS_VERYLOW,
	),
	}
	CONSTANTINOPLE_OPCODES = merge(
	copy.deepcopy(BYZANTIUM_OPCODES),
	UPDATED_OPCODES,
	)

But imagine you are implementing a private chain and want to use add, sub, mul etc and have to import it from eth.vm.forks.frontier. That would feel weird, no?

I can see the point though, chances are another fork wants to implement mul differently (unlikely but anyway) and so we end up with a different implementation of mul. But even then, I think this second mul implementation would just end up living in eth.vm.logic as long as it is generic enough to be used across different forks and chains.

[pipermerriam]

We could potentially anchor them in stub fork like eth.vm.forks.core which contains this common stuff and is more sane for 3rd parties to import and use.

[cburgdorf]

As in eth.vm.forks.core contains everything that forks may want to pull in or redefine. Sgtm.

[mratsim]

When porting py-evm to Nim, we first used the same gas costs structure of Py-EVM but then refactored to isolate all gas costs and computation in a single file. See https://github.com/status-im/nimbus/pull/49 and the final implementation.

In the end, we currently have on array per fork that maps the "name" to the gas cost from Yellow Paper Appendix G:

# Generate the fork-specific gas costs tables
const
  BaseGasFees: GasFeeSchedule = [
    # Fee Schedule at for the initial Ethereum forks
    GasZero:            0'i64,
    GasBase:            2,
    GasVeryLow:         3,
    GasLow:             5,
    GasMid:             8,
    GasHigh:            10,
    GasExtCode:         20,     # Changed to 700 in Tangerine (EIP150)
    GasBalance:         20,     # Changed to 400 in Tangerine (EIP150)
    GasSload:           50,     # Changed to 200 in Tangerine (EIP150)
    GasJumpDest:        1,
    GasSset:            20_000,
    GasSreset:          5_000,
    RefundSclear:       15_000,
    RefundSelfDestruct: 24_000,
    GasSelfDestruct:    0,      # Changed to 5000 in Tangerine (EIP150)
    GasCreate:          32000,
    GasCodeDeposit:     200,
    GasCall:            40,     # Changed to 700 in Tangerine (EIP150)
    GasCallValue:       9000,
    GasCallStipend:     2300,
    GasNewAccount:      25_000,
    GasExp:             10,
    GasExpByte:         10,     # Changed to 50 in Spurious Dragon (EIP160)
    GasMemory:          3,
    GasTXCreate:        32000,
    GasTXDataZero:      4,
    GasTXDataNonZero:   68,
    GasTransaction:     21000,
    GasLog:             375,
    GasLogData:         8,
    GasLogTopic:        375,
    GasSha3:            30,
    GasSha3Word:        6,
    GasCopy:            3,
    GasBlockhash:       20
    # GasQuadDivisor:     100     # Unused, do not confuse with the quadratic coefficient 512 for memory expansion
  ]

# Create the schedule for each forks
func tangerineGasFees(previous_fees: GasFeeSchedule): GasFeeSchedule =
  # https://github.com/ethereum/EIPs/blob/master/EIPS/eip-150.md
  result = previous_fees
  result[GasSload]        = 200
  result[GasSelfDestruct] = 5000
  result[GasBalance]      = 400
  result[GasCall]         = 40

func spuriousGasFees(previous_fees: GasFeeSchedule): GasFeeSchedule =
  # https://github.com/ethereum/EIPs/blob/master/EIPS/eip-160.md
  result = previous_fees
  result[GasExpByte]      = 50

const
  TangerineGasFees = BaseGasFees.tangerineGasFees
  SpuriousGasFees = TangerineGasFees.spuriousGasFees
  # Note that later forks are still WIP

# `gasCosts` is a macro that builds an array of gas cost functions
# It takes (Fee schedule, prefix of generated functions, result array of functions)
# as argument.
# The array is a property of vm forks.
gasCosts(BaseGasFees, base, BaseGasCosts)
gasCosts(TangerineGasFees, tangerine, TangerineGasCosts)
# Note that later forks are still WIP

proc forkToSchedule*(fork: Fork): GasCosts =
  if fork < FkTangerine:
    BaseGasCosts
  else:
    TangerineGasCosts
  # Note that later forks are still WIP

[cburgdorf]

@mratsim thanks for chiming in here and sharing your code from the nimbus client 👍
I had written up a big comment right before the internet had an outage (traveling) but I think it's well aligned with what you wrote. Here it goes:

As promised to @glaksmono in chat, here's me elaborating on the issue. Let's look at how the constants are used to day and the things that may be wrong with that.

Let's consider GAS_BALANCE as an example. This constant was introduced to be used as a gas price for the opcode that reads the balance from an account. It was set to 20. We can see it being used here to set the gas price of that opcode for the frontier code.

py-evm/eth/constants.py

Line 44 in 931e915

GAS_BALANCE = 20

However, retrieving the balance doesn't actually cost 20 gas anymore. It costs 400 as specified in the tangerine whistle fork. However, there was a new constant GAS_BALANCE_EIP150 being introduced for that which lives in the tangerine whistle specific constants.

py-evm/eth/vm/forks/tangerine_whistle/constants.py

Line 13 in 931e915

GAS_BALANCE_EIP150 = 400

My initial reaction was: Let's clean up eth.constants and move all things that are only used within a specific fork into the constants of that specific fork. But I think there's a problem with that approach as well.

There are more generic constants such as GAS_VERYLOW which are used by many different forks. But then GAS_LOW which falls into the same category is only used in the frontier fork. That's more or less random which means, just looking at the usage side doesn't give us a strong indicator of how to organize these constants.

And then there's another category. A constant such as GAS_SSET isn't used by any fork but inside eth.vm.logic instead.

With that in mind, let's try something else.

Let's create a new class BaseConstants in eth.vm.forks.core and define it as such:

class BaseConstants:

    GAS_LOW = 5
    ...
    GAS_BALANCE = 20
    ...

And then instead of the forks maintaining plain old constants in a separate file, let them have their inherited versions of the BaseConstants.

class TangerineWhistleConstants(BaseConstants):

    GAS_BALANCE = 400

Instead of introducing a new, different constant to adjust the gas price of the balance opcode, the fork would just redefine GAS_BALANCE with a different value.

With that in mind, let's derive some rules on how to use organize constants under that model.

1. Every constant that has been used in the very first version (Frontier) already, goes into BaseConstants

2. Constants that just redefine existing constants get dropped (e.g. GAS_BALANCE_EIP150) and instead, in the inherited ForknameConstants, the value of the original constant is overwritten.

3. New constants may be introduced in ForknameConstants if they describe things that would not make sense in BaseConstants as they aren't generic and rather describe fork specific things.

4. The ForknameConstants family of classes must never be imported outside of eth.vm.forks. E.g. eth.vm.logic can only import BaseConstants but not TangerineWhistleConstants

5. If a function inside eth.vm.logic wants to import from a fork specific constant, then that is a clear signal that the function either needs to be moved into a fork specific place (which is what @pipermerriam was suggesting) or that it needs to be refactored to accept the right constants to get injected.

@pipermerriam does that feel alright to you?

[pipermerriam]

My initial thought is 👎 with respect to the status-quo of just leaving things as they are. I already dislike how much inheritance we have in our current implementation and I'd like to move away from that. Using a class inheritance based approach feels like it just further contributes to the tangle of inherited class mechanics we have going.

I'm inclined to shelve this idea but I'm open to alternatives. I'm currently in love with the concepts of composition but it isn't clear exactly how that would apply here.

[cburgdorf]

I'm a big fan of composition as well but this applies more to functionality. This case is more about maintaining a extensible table of static constants. We wouldn't ever have instances of these classes anyway.

…

On Mon, Aug 20, 2018, 21:25 Piper Merriam ***@***.***> wrote: My initial thought is 👎 with respect to the status-quo of just leaving things as they are. I already dislike how much inheritance we have in our current implementation and I'd like to move away from that. I'm inclined to shelve this idea but I'm open to alternatives. I'm currently in love with the concepts of composition but it isn't clear exactly how that would apply here. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1146 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAfzlcaGNTw1WuOkCzXNlXGUEbhQzbq5ks5uSw1GgaJpZM4Vwha9> .

[pipermerriam]

instances and inheritance aside, I still don't see this as an improvement to the status quo. I think the only thing this cleans up is forks which have new opcodes which still use things like GAS_VERYLOW as well as some new constant for an updated gas cost by simplifying it to a single import rather than two constants imports. But then, trying to read what the actual value of one of those constants gets tricky because you'll have to trace your way backwards through the inheritance chain (6-7 classes deep) to find what the actual value is..

I'm not seeing the benefit.

[cburgdorf]

It basically boils down to the question: Do we prefer one fixed name per gas constant e.g. GAS_BALANCE vs introducing new ones (or alternative import pathes) for each change (e.g GAS_BALANCE_EIP4711, GAS_BALANCE_EIP_8757 etc.)

As I understand it Nimbus and I believe Geth also follow this "one table to rule them all" approach.

But I'm fine to just stick to what we do if you aren't sharing the love 💕

[pipermerriam]

While reading the nimbus issues they mentioned something about fully decoupling gas consumption from opcode logic. I think that our architecture fits that, except that we then re-couple them together with the as_opcode stuff which bundles up the gas logic with the vm logic. They mentioned something about the decoupling allowing for things like caching but it's not clear to me in practice how that would work. If you can show me a clear-ish win/benefit from using a table based approach, lets talk, but I'm not really seeing it.

As for the weird names, I actually like them as they are expressive in making it clear why the value is different, i.e. it's the EIP4711 value.

[cburgdorf]

To me, having one mapping table with fixed gas constants that just change in their value from fork to fork, sounds like a more maintainable thing compared to a potential explosion of different constants (depending on how often values change per constant).

But I don't wanna be stubborn about it. We can close this issue for now and maybe revisit it in the future if we feel things are starting to hurt.