VM: Add option to add custom opcodes

[jochem-brouwer]

This PR adds a new field to VMOpts to allow users to add/change/delete opcodes in the VM.

Please note: in gas.ts the linter has inserted whitespace in almost every line, the only change there is to also allow for synchronous gas functions.

[codecov]

Codecov Report

Merging #1705 (5df3e68) into master (f774042) will increase coverage by 0.01%.
The diff coverage is 90.25%.

Flag	Coverage Δ
block	85.57% <ø> (ø)
blockchain	83.82% <ø> (ø)
client	18.23% <ø> (ø)
common	94.19% <ø> (ø)
devp2p	82.56% <ø> (ø)
ethash	90.76% <ø> (ø)
trie	80.02% <ø> (ø)
tx	88.20% <ø> (ø)
util	92.62% <ø> (ø)
vm	81.22% <90.25%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[jochem-brouwer]

Oof I just realized we have a problem here.

I am writing to the constants of gas.ts and function.ts if I add custom opcodes. Thus that means that whatever I have changed (lets say I once deleted KECCAK in a new VM) then that also means I have deleted it in further VMs.

How should I approach solving this? It would imply that we now need to deep-copy gas.ts and function.ts exported constants..?

[holgerd77]

This looks already pretty cool, thanks a lot for addressing so quickly Jochem! 🙂

Let's keep this a bit in discussion for a couple of days - independently from if we find answers to the existing questions. This is such a profound change, I guess it would be good if we let this new API sink in a bit and shovel this back and forth to think about use cases and implications. 😋

(and no need to merge this in particularly early or fast)

[ryanio]

not sure if this new test suite belongs in runCode, i suggest we make a new file tests/api/evm/customOpcodes.spec.ts

[holgerd77]

Totally a side topic here but just want to mention on the occasion since we are already so much in these StateManager design questions: my dream ("I have a dream", lol) for the breaking releases is still that we also manage to separate the EVM itself (so basically: everything in the evm folder) and all this outer execution stuff (so roughly: everything - at least - from VM.runTx() upwards).

This mangling together of the pure EVM execution logic (opcodes in, result out, basically 😋) and all this outer environment stuff with running txs, creating receipts, running the block, assiging block rewards in this one big VM object is one of the biggest design flaws we have. One can see this over and over again, e.g. this customOpcodes option would - if we would have that - also rather only be a thing to be passed to a clean (inner) EVM interface (in the sense of: API) and nothing to be babbled to this monster object with all the additional logic.

I guess this outer thing is something which rather would belong to the client then to the VM (we for sure doesn't want to integrate though) and historically likely was bapped on to the VM because there just wasn't a wrapping client or something and people needed this functionality.

Maybe we can come up with a separate package or so - VMExecutionEnv (I have totally no idea on a fitting name yet) - and then create a stable interface (again: API) to the EVM package.

This would lead to a lot of new use cases. The EVM package could be swapped out, extended by others, used in a standalone-context, etc.. I am also pretty sure that our overall VM architecture would greatly benefit by refactoring questions which come up along the way.

This would need some serious design thinking though, not sure if we would make it with this into this round of breaking releases. I guess one main thing would be where to place and how to put in all this EEI stuff, since the inner EVM needs to have some environment information finally. Maybe you also want to give this some thinking already. 🙂

P.S.: I know, comment is totally misplaced here. 😛

[jochem-brouwer]

I think the general point you are making here is that we should take a deep look at our internals in the VM to see if we should refactor there. I'm also getting the feeling that some things are "all over the place" and maybe a bit misplaced in some cases as well. We might do an internal refactor soon?

[holgerd77]

I would like to do one along the breaking releases, with the optimal outcome that we can separate EVM and VM and have a clean (and somewhat refactored) API between them (what also comes into play here is this EVMC interface https://github.com/ethereum/evmc eventually).

Not sure if we'll make it though to full extend in this round, this is particularly hard.

[holgerd77]

This looks already pretty great, cool, thanks for tackling this so quickly! 🚀

Some things for discussion and comments with suggestions for some additions/modifications.

[holgerd77]

I love this so much to see how these things play so nicely together close to everytime we do a profound refactoring which was initially meant to solve something completely different! 🥳

Thanks again for this great work! ❤️

[holgerd77]

Love these tests, great! 🙂

[holgerd77]

@jochem-brouwer do you have some time to pick this up this week? Would be great if we can move this forward (merge). 🙂

[jochem-brouwer]

I'm gonna need to give this a little bit more thought. The problem is that the handlers (gas handler + logic handler) are used everywhere from using the exported values from their respective files (e.g. here:

ethereumjs-monorepo/packages/vm/src/evm/interpreter.ts

Line 182 in c842a97

return opHandlers.get(opInfo.code)!

). We somehow need to integrate the "copies" of these (only necessary when using custom opcodes to prevent editing the original) in (probably) EVM. So instead of getting the opcodes from their opcode files, we would get them from the respective Map which resides inside EVM. So, getOpcodesForHF here

ethereumjs-monorepo/packages/vm/src/evm/opcodes/codes.ts

Line 302 in c842a97

export function getOpcodesForHF(common: Common): OpcodeList {

should also get the EVM object and edit the gas+logic maps in there.

Does that make sense? So we (1) need to create copies of the gas/logic maps (https://github.com/ethereumjs/ethereumjs-monorepo/blob/c842a973bc8058df71cda140fe0720292e1aafe1/packages/vm/src/evm/opcodes/gas.ts, https://github.com/ethereumjs/ethereumjs-monorepo/blob/c842a973bc8058df71cda140fe0720292e1aafe1/packages/vm/src/evm/opcodes/functions.ts) and (2) we need to somehow integrate it in the VM such that these values are read internally (probably EVM).

However, maybe EVM is also not the right place because each Tx would create new EVM

ethereumjs-monorepo/packages/vm/src/runTx.ts

Line 392 in c842a97

const evm = new EVM(this, txContext, block)

. I'm not sure how expensive copying these maps are (I don't think super expensive). However, on the other hand, these copies would only work if we have custom opcodes so that would not slow down anything related to most practical implementations where we need speed (mainnet). If we want to prevent these copies then the right place would probably be VM itself.

Thoughts?

[jochem-brouwer]

I think at this point EVM.ts would be the right place to put these maps.

[holgerd77]

I think I am not getting the problem yet.

So lets take e.g. the dynamicGasHandlers. These are only imported once in interpreter.ts and then also only accessed in one single location in Interpreter, so here.

Istn't this just a one-liner to expand dynamicGasHandlers at this specific point and add/modfiy/remove the custom opcode dynamic gas handlers and then just work on this modified Map? 🤔

[holgerd77]

Update: ah, I think I am slowly getting what you mean.

I have the impression you are thinking the wrong way around. getOpcodesForHF() doesn't need to be passed anything additionally in if I am not mistaken. Instead the outer VM already has this created dynamic (custom) opcode list in the _opcodes property. The outer VM is also passed to the inner EVM in the constructor constructor(vm: any, txContext: TxContext, block: Block), so you can access the opcodes from there with this._vm._opcodes and use the dynamically created list in places like you mentioned (

ethereumjs-monorepo/packages/vm/src/evm/interpreter.ts

Line 182 in c842a97

return opHandlers.get(opInfo.code)!

) instead of taking the static maps from the import (and use this in other places as well).

Does this make sense?

[holgerd77]

Update 2: ah, just seeing that this VM._opcodes property only contains the opcode information, and not the function and gas logic itself. So I would just add similar properties to the EVM - so EVM._handlers and EVM._dynamicGasHandlers and assign these properties along the getOpcodesForHF run and then do all VM executions from there and replace all static usages of ophandlers?

Wouldn't think that this would draw in some performance penalty.

[jochem-brouwer]

For some reason I cannot reply to this; #1705 (comment)

There is no check if the opcode is present; it does not throw if it is not present and user wants to delete (logicFunction set to undefined). There are also no checks for "opcode twice", you are right, users are responsible for their own inputs we have no reason to validate their input.

[jochem-brouwer]

I have updated, tests should pass, this should be ready for review now.

Note: I added the maps to VM, not to EVM. The reason is that EVM is instantiated many times and we do not want to copy the opcode map each time. (Only once and only if it is really necessary, if opcodes have changed or upon initializing). Otherwise, each time on runTx we copy these maps, which is not what we want. (Not sure if it is deep or shallow copy, but it is unnecessary anyways)

[jochem-brouwer]

Addressed the review comments as well.

[holgerd77]

One API thing and one small question, otherwise looks good. 😄

[holgerd77]

Short note: have just merged #1757, so the PR here would need another update to be ready for review.

[jochem-brouwer]

I took a look at this today but the merge conflicts are very weird 🤔 Need to give this a new look later because I messed up my local merge 😞

[jochem-brouwer]

Ok, this should work, ready for re-review.

[acolytec3]

Generally looks good. A couple of comments on documentation that we could take or leave.

I also verified the delete/add opcode logic by inspecting the opcode lists directly in the tests so can vouch that code works great.