-
Notifications
You must be signed in to change notification settings - Fork 20k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core/vm: improved EVM run loop & instruction calling #3378
Conversation
@obscuren, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Arachnid, @tgerring and @Gustav-Simonsson to be potential reviewers. |
eaf48f9
to
6bf9c0c
Compare
// Depth check execution. Fail if we're trying to execute above the | ||
// limit. | ||
if env.Depth() > int(params.CallCreateDepth.Int64()) { | ||
caller.ReturnGas(gas, gasPrice) | ||
if env.Depth > int(params.CallCreateDepth.Int64()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is not this limit deprecated nowadays, with EIP150? So this check should only be for pre-150, if I'm reading it right. And the same comment for Call
above and Create
below.
If it's left here because it's practically unreachable anyway, perhaps a comment about that would be in order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong PR. This was changed in the previous PR (which this one is based on). You're right though, but unfortunately there seem to be a few tests that still test this clause so I cannot remove this yet.
|
||
if eip158 { | ||
// if empty and transfers value | ||
if env.StateDB.Empty(address) && env.StateDB.GetBalance(contract.Address()).BitLen() > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The uglyness of env.StateDB.GetBalance(contract.Address()).BitLen() > 0
could be hidden within env.StateDB.HasBalance(contract.Address)
, instead of having it all over the place.
And if there is ever a 0-value-account-bloomfilter or whatever, you can swap it out in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would add an additional requirement on the already fat StateDB
so I'd rather not do this yet.
Your 0-value-account is a good argument to add this, however, I'll think about this a little bit
// non 0 => non 0 (or 0 => 0) | ||
g = params.SstoreResetGas | ||
var memorySize *big.Int | ||
// calucalte the new memory size and expand the memory to fit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/calucalte/calculate
08cfbfc
to
ffa5bcb
Compare
Preliminary benchmarks have given the following results: Old vs new (metered):
Old vs new (unmetered):
|
c2e20bc
to
c868275
Compare
09f8858
to
81a28a1
Compare
e59adce
to
e0b64a4
Compare
var jumpTable = [256]operation{ | ||
ADD: operation{ | ||
execute: opAdd, | ||
gasCost: makeGenericGasFunc(ADD), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like an additional indirection to have global gasTable and then create a function that looks up
in the table. Why not have constGasFunc(GasFastestStep)
here?
func constGasFunc(gas *big.Int) gasFunc {
return func(gt params.GasTable, env *Environment, contract *Contract, stack *Stack, mem *Memory, memorySize *big.Int) *big.Int {
return gas
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. It was laziness, actually.
@@ -35,6 +37,7 @@ type ( | |||
|
|||
// Context provides the EVM with auxilary information. Once provided it shouldn't be modified. | |||
type Context struct { | |||
context.Context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this. The context should be passed into the function as a separate parameter.
Making it part of a struct is usually wrong. See context documentation:
Programs that use Contexts should follow these rules to keep interfaces consistent across packages and enable static analysis tools to check context propagation:
Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx:
It's also misplaced in the VM Context struct it is supposed to contain info about the "blockchain context" in which execution is taking place.
If you want to enable quitting via context.Context, please pass it explicitly.
For recursive calls, you'll probably need to make additional unexported versions
of call
, create
, etc. because they don't need to take the context when recursing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm aware of that. But circumstances are different here as well since the context is fully optional. Please provide an alternative approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it can run without context, I'd suggest adding a Cancel
method on environment that flips the atomic stop value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's one way @karalabe and I thought about it. Will think about this some more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I don't like about the current EVM is that it may only be used once, this is completely opaque and only becomes apparent when one inspects the source code or reads the documentation. This becomes apparent whit something like a Cancel
method is added which sets an atomic field as it it will also introduce weird race conditions.
The main problem is that when the EVM is cancelled and the abort
flag is set you'll no longer we able to run any other program, which imo kind of sucks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I suggested it because you made EVM/Environment single use. Why is it a problem though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can always make the EVM struct itself smaller by moving more fields into Context and by making the Context field a pointer. That would resolve potential problems with allocation because Context can be reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it more, it feels like we are running in circles here. The Interpreter contained in EVM is supposed to be reusable for some reason, so all runtime state (including depth, etc) must be held in locals or in the EVM struct. Now you say that you're not happy with EVM being single use. Maybe we should make Interpreter single use and EVM reusable/thread safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that it had been since forever, but I've never liked the idea that it is single use, imo it's poorly designed and I'd like to change it. The Interpreter
currently can be re-used, the EVM
can not. The only reason the EVM
can not be reused is because of the abort
field that may be set.
Perhaps it is Context
that should be single use. Argument for this is that you'll never apply a transaction twice and a context is based on a transaction (given it has a field like Origin
).
@@ -66,29 +69,55 @@ type Environment struct { | |||
// Depth is the current call stack | |||
Depth int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why Depth is exported now that I look at this again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't need to be (anymore). Will unexport.
3140ced
to
248a93b
Compare
Previously we had a large "requirement" check loop which switched around every opcode and calculated the gas, checked stack, and resized the memory when needed. This has been refactored in to the EVM operation table witch acts as a large jump table for the EVM containing functions for stack and gas requirement and execution. In addition the EVM can now also be used unmetered by setting the `DisableGasMetering` flag on the EVM config. This will cause the EVM to run unmetered without gas. Preliminary benchmarks have given the following results: **Old vs new (metered):** benchmark old ns/op new ns/op delta BenchmarkVmFibonacci16Tests-4 37668307 21323060 -43.39% **Old vs new (unmetered):** benchmark old ns/op new ns/op delta BenchmarkVmFibonacci16Tests-4 37668307 14204094 -62.29%
Implemented RunWithContext which allows the execution of the EVM to be halted mid-flight. This is useful when using the EVM unmetered or when using the Light Client, which requires control over the EVM run loop.
Renamed Environment to EVM and EVM to Interpreter. The rational behind this that the Environment used to be an interface and had multiple implementations across the code base. All of the logic implemented in the Environment is part of the EVM. The old EVM is actually the interpreter, which interprets the byte code. The old EVM is part of the EVM as a whole.
Removed the context.Context cancellation approach from vm.Environment and added a Cancel method instead; this however makes the environment a one shot object due to the `evm.abort` being set to 1, disallowing any further operation to be done using that environment.
248a93b
to
f02b401
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM per review call
The run loop, which previously contained custom opcode executes have been removed and has been simplified to a few checks. Each operation consists of 4 elements: execution function, gas cost function, stack validation function and memory size function. The execution function implements the operation's runtime behaviour, the gas cost function implements the operation gas costs function and greatly depends on the memory and stack, the stack validation function validates the stack and makes sure that enough items can be popped off and pushed on and the memory size function calculates the memory required for the operation and returns it. This commit also allows the EVM to go unmetered. This is helpful for offline operations such as contract calls.
Review #3348 first
This PR implements a refactored EVM and EVM run loop.
Simplified run loop
The run loop, which previously contained custom opcode executes have been removed and has been simplified to a few checks; including:
Each operation consists out of 4 elements:
The execution function implements the operation's runtime behaviour, the gas cost function implements the operation gas costs function and greatly depends on the memory and stack, the stack validation function validates the stack and makes sure that enough items can be popped off and pushed on and the memory size function calculates the memory required for the operation and returns it.
Optional gas metering
This PR also allows the EVM to go unmetered. This is helpful for offline operations such as contract
call
's.