Group opcodes based on HF

[s1na]

Currently HF validation happens inside the logic of an opcode handler. @yann300 mentioned it'd be helpful if one could get the list of opcodes for a particular HF. We could add a validSince attribute to OpInfo which specifies since which HF this opcode is valid, and perform the check in Loop instead of logic of the ophandler.

[holgerd77]

I already once thought about grouping opcodes statically into hardfork named files (opFns/byzantium.js or similar) which goes in a similar direction. But this solution is more dynamic and machine-wise extractable, like the suggestion.

[alcuadrado]

I like this idea. Should it be based on -common? Or should we add that data to opcodes.ts?

[holgerd77]

I would have a tendency to add this to opcodes.ts and not overload the common library.

[s1na]

Yeah, I wouldn't move this to Common for now. But even within the VM there can be various approaches that we should consider. What I suggested is one of the simplest (I thought). I'm open to other suggestions however.

Another one is how py-evm does things, e.g. having an initial set of opcodes (from genesis, or the first supported HF), and a new set for each subsequent HF which takes the previous set and updates it.

[alcuadrado]

Another one is how py-evm does things, e.g. having an initial set of opcodes (from genesis, or the first supported HF), and a new set for each subsequent HF which takes the previous set and updates it.

I like this. It shouldn't be that hard to implement in js/ts. Right?

[s1na]

It shouldn't be that hard to implement in js/ts. Right?

I don't see any difficulty. Maybe we could develop both approaches in two branches and compare them. I think the second one is more flexible, but not sure if it's an overkill

[jochem-brouwer]

Hey guys! My implementation is probably going to clash a bit with Spurious Dragon HF support PR (@holgerd77).

I have two points I'd like to raise here:

In istanbul the gas cost of SLOAD changed. This is reflected in opcodes.ts by "overwriting" the original opcode with a new fee. I suggest we move this fee to common. I think that hardfork opcodes are OK to be in opcodes.ts, but to code the gas fee there does not seem right to me.

The easiest way to implement this is to just edit getOpcodesForHF and now also implement other hardforks than istanbul. This is also what's being done at the above PR.

Thoughts? @alcuadrado @s1na

[jochem-brouwer]

Actually, for opcodes like SLOAD which have changed gas cost sometimes, it seems that for tangerineWhistle the gas cost is actually in common, but it is never used 🤔 .

[holgerd77]

@jochem-brouwer No worry, will rebase on top.

Have already answered here on the gas cost consolidation suggestion, think this makes a lot of sense.

@jochem-brouwer yes, it is likely that there are some historical inconsistencies (like tangerineWhistle gas costs from Common not read in the VM, likely due to it just stayed the "old" way, since Common was introduced on a later stage). Would be good if these kind of things gets cleaned up during a rework on this.