common/vm: add EIP-3855, PUSH0 instruction #1616
Conversation
packages/vm/src/runCode.ts
Outdated
| @@ -83,7 +83,7 @@ export default function runCode(this: VM, opts: RunCodeOpts): Promise<ExecResult | |||
| new Message({ | |||
| code: opts.code, | |||
| data: opts.data, | |||
| gasLimit: opts.gasLimit, | |||
| gasLimit: opts.gasLimit ?? new BN(0), | |||
There was a problem hiding this comment.
Might look a bit out of place in this PR, but I noticed that gasLimit is optional, but if no gasLimit is provided it is thus undefined in the context, which causes the EVM to throw internally here. (cannot clone() the undefined gasLimit).
There was a problem hiding this comment.
Would it always go OOG if provided with gasLimit of 0?
If so maybe we should throw with an error, or make it default to e.g. the last block's gasLimit?
Should we consider making it non-optional in a next breaking change release? (We can add to our list)
There was a problem hiding this comment.
Ah right I did not give this a lot of thought. I think an error here is cleaner (non-breaking) but I think we should make gasLimit mandatory in the breaking release.
There are a few edge cases where it does not go OOG: either if you run no code, or if you run a code which uses 0 gas (STOP). All other opcodes which have 0 gas cannot be used AFAIK since these need stack items and you would thus need to push items on the stack, which costs gas.
There was a problem hiding this comment.
Actually thinking about this, if we make it error in case that we do not provide gasLimit, then that's breaking as well right?
There was a problem hiding this comment.
That's true, well if the behavior is correct then we can leave it as be and consider the error for a breaking change release.
There was a problem hiding this comment.
It didn't work without providing the gasLimit anyway.
We can check if the typedoc for Message has information about providing a gasLimit otherwise default: 0
There was a problem hiding this comment.
I have removed this change; if we default it to 0 then the behavior is slightly different than if we do not provide a gasLimit; in the case that we just send eth OR we run the 00 code, the code will run and finish if we default it to 0, while if we don't do this it would throw, so this is somewhat a breaking change. (I also should not have added this whole thing since this is out of scope of this PR). Will add this thing to the tiny fixes / breaking issue.
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Thanks @ryanio 😄 |
https://eips.ethereum.org/EIPS/eip-3855