Skip to content
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.

Sign up for GitHub

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

Jump to bottom

core/vm: avoid memory expansion check for trivial ops #24048

Merged
merged 1 commit into from
Dec 14, 2021
Merged

core/vm: avoid memory expansion check for trivial ops #24048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 25 additions & 31 deletions core/vm/interpreter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,62 +182,56 @@ func (in *EVMInterpreter) Run(contract *Contract, input []byte, readOnly bool) (
// Capture pre-execution values for tracing.
logged, pcCopy, gasCopy = false, pc, contract.Gas
}

// Get the operation from the jump table and validate the stack to ensure there are
// enough stack items available to perform the operation.
op = contract.GetOp(pc)
operation := in.cfg.JumpTable[op]
cost = operation.constantGas // For tracing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yesterday I started a branch to experiment with moving all the gas calculation into another function (next to the implementation): https://github.com/ipsilon/go-ethereum/tree/gas-redesign

The constant gas instructions would have the same simple function charging the constant gas, others would have the dynamic calculation + memory expansion.

I am not sure if this will help everything considered, because it will always mean an indirect call, but at the same time it reduces most of the branching in this loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked briefly at the last commit there -- I think it won't work that easily.

The way the dynamic gas has evolved, we need to first ensure that the caller can cover the static cost, because for some ops, actually calculating the dynamic cost is expensive: checking the existence of things in the trie.
So we can't just calculate everything and then charge.

We can get around it by either having the "chargeGas" function not only return the gas, but actually charge it via the contract.
Alternatively, we can pass the availableGas into the "chargeGas" function.

// Validate stack
if sLen := stack.len(); sLen < operation.minStack {
return nil, &ErrStackUnderflow{stackLen: sLen, required: operation.minStack}
} else if sLen > operation.maxStack {
return nil, &ErrStackOverflow{stackLen: sLen, limit: operation.maxStack}
}
// Static portion of gas
cost = operation.constantGas // For tracing
if !contract.UseGas(operation.constantGas) {
if !contract.UseGas(cost) {
return nil, ErrOutOfGas
}

var memorySize uint64
// calculate the new memory size and expand the memory to fit
// the operation
// Memory check needs to be done prior to evaluating the dynamic gas portion,
// to detect calculation overflows
if operation.memorySize != nil {
memSize, overflow := operation.memorySize(stack)
if overflow {
return nil, ErrGasUintOverflow
}
// memory is expanded in words of 32 bytes. Gas
// is also calculated in words.
if memorySize, overflow = math.SafeMul(toWordSize(memSize), 32); overflow {
return nil, ErrGasUintOverflow
}
}
// Dynamic portion of gas
// consume the gas and return an error if not enough gas is available.
// cost is explicitly set so that the capture state defer method can get the proper cost
if operation.dynamicGas != nil {
// All ops with a dynamic memory usage also has a dynamic gas cost.
var memorySize uint64
// calculate the new memory size and expand the memory to fit
// the operation
// Memory check needs to be done prior to evaluating the dynamic gas portion,
// to detect calculation overflows
if operation.memorySize != nil {
memSize, overflow := operation.memorySize(stack)
if overflow {
return nil, ErrGasUintOverflow
}
// memory is expanded in words of 32 bytes. Gas
// is also calculated in words.
if memorySize, overflow = math.SafeMul(toWordSize(memSize), 32); overflow {
return nil, ErrGasUintOverflow
}
}
// Consume the gas and return an error if not enough gas is available.
// cost is explicitly set so that the capture state defer method can get the proper cost
var dynamicCost uint64
dynamicCost, err = operation.dynamicGas(in.evm, contract, stack, mem, memorySize)
cost += dynamicCost // total cost, for debug tracing
cost += dynamicCost // for tracing
if err != nil || !contract.UseGas(dynamicCost) {
return nil, ErrOutOfGas
}
if memorySize > 0 {
mem.Resize(memorySize)
}
}
if memorySize > 0 {
mem.Resize(memorySize)
}

if in.cfg.Debug {
in.cfg.Tracer.CaptureState(pc, op, gasCopy, cost, callContext, in.returnData, in.evm.depth, err)
logged = true
}

// execute the operation
res, err = operation.execute(&pc, in, callContext)

if err != nil {
break
}
Expand Down
38 changes: 29 additions & 9 deletions core/vm/jump_table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package vm

import (
"fmt"

"github.com/ethereum/go-ethereum/params"
)

Expand Down Expand Up @@ -57,21 +59,39 @@ var (
// JumpTable contains the EVM opcodes supported at a given fork.
type JumpTable [256]*operation

func validate(jt JumpTable) JumpTable {
s1na marked this conversation as resolved.
Show resolved Hide resolved
for i, op := range jt {
if op == nil {
panic(fmt.Sprintf("op 0x%x is not set", i))
}
// The interpreter has an assumption that if the memorySize function is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering they always go together, it would be better to merge these into single function that outputs new memory size and error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: they don't "always go together". If dynamic memory, then dynamic gas, but not always the other way around. For example the EXP or SSTORE and SELFDESTRUCT doesn't expand memory but have a dynamic cost.

But yes, maybe something could be changed in the strucutre at a higher level.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, what I meant in this case they should return unchanged memory size or 0.
But in total this reduces helper calls from 2 to 1 and eliminates the need for validate().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eliminates the need for validate().

Afaik, the validate is only performed during geth bootup, when the global vars are instantiated. Unless I misunderstood something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it can be reworked, but I don't want to do it in this PR. I think merging memory funcs and gas funcs is going to be a very large change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik, the validate is only performed during geth bootup, when the global vars are instantiated. Unless I misunderstood something.

Correct. But my comment was about the design. Not needing validate() is better design.

I agree that it can be reworked, but I don't want to do it in this PR.

Make sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am doing this refactoring in #24113

// set, then the dynamicGas function is also set. This is a somewhat
// arbitrary assumption, and can be removed if we need to -- but it
// allows us to avoid a condition check. As long as we have that assumption
// in there, this little sanity check prevents us from merging in a
// change which violates it.
if op.memorySize != nil && op.dynamicGas == nil {
panic(fmt.Sprintf("op %v has dynamic memory but not dynamic gas", OpCode(i).String()))
}
}
return jt
}

// newLondonInstructionSet returns the frontier, homestead, byzantium,
// contantinople, istanbul, petersburg, berlin and london instructions.
func newLondonInstructionSet() JumpTable {
instructionSet := newBerlinInstructionSet()
enable3529(&instructionSet) // EIP-3529: Reduction in refunds https://eips.ethereum.org/EIPS/eip-3529
enable3198(&instructionSet) // Base fee opcode https://eips.ethereum.org/EIPS/eip-3198
return instructionSet
return validate(instructionSet)
}

// newBerlinInstructionSet returns the frontier, homestead, byzantium,
// contantinople, istanbul, petersburg and berlin instructions.
func newBerlinInstructionSet() JumpTable {
instructionSet := newIstanbulInstructionSet()
enable2929(&instructionSet) // Access lists for trie accesses https://eips.ethereum.org/EIPS/eip-2929
return instructionSet
return validate(instructionSet)
}

// newIstanbulInstructionSet returns the frontier, homestead, byzantium,
Expand All @@ -83,7 +103,7 @@ func newIstanbulInstructionSet() JumpTable {
enable1884(&instructionSet) // Reprice reader opcodes - https://eips.ethereum.org/EIPS/eip-1884
enable2200(&instructionSet) // Net metered SSTORE - https://eips.ethereum.org/EIPS/eip-2200

return instructionSet
return validate(instructionSet)
}

// newConstantinopleInstructionSet returns the frontier, homestead,
Expand Down Expand Up @@ -122,7 +142,7 @@ func newConstantinopleInstructionSet() JumpTable {
maxStack: maxStack(4, 1),
memorySize: memoryCreate2,
}
return instructionSet
return validate(instructionSet)
}

// newByzantiumInstructionSet returns the frontier, homestead and
Expand Down Expand Up @@ -158,14 +178,14 @@ func newByzantiumInstructionSet() JumpTable {
maxStack: maxStack(2, 0),
memorySize: memoryRevert,
}
return instructionSet
return validate(instructionSet)
}

// EIP 158 a.k.a Spurious Dragon
func newSpuriousDragonInstructionSet() JumpTable {
instructionSet := newTangerineWhistleInstructionSet()
instructionSet[EXP].dynamicGas = gasExpEIP158
return instructionSet
return validate(instructionSet)

}

Expand All @@ -179,7 +199,7 @@ func newTangerineWhistleInstructionSet() JumpTable {
instructionSet[CALL].constantGas = params.CallGasEIP150
instructionSet[CALLCODE].constantGas = params.CallGasEIP150
instructionSet[DELEGATECALL].constantGas = params.CallGasEIP150
return instructionSet
return validate(instructionSet)
}

// newHomesteadInstructionSet returns the frontier and homestead
Expand All @@ -194,7 +214,7 @@ func newHomesteadInstructionSet() JumpTable {
maxStack: maxStack(6, 1),
memorySize: memoryDelegateCall,
}
return instructionSet
return validate(instructionSet)
}

// newFrontierInstructionSet returns the frontier instructions
Expand Down Expand Up @@ -1010,5 +1030,5 @@ func newFrontierInstructionSet() JumpTable {
}
}

return tbl
return validate(tbl)
}