Skip to content

Remove all the debug traces from the vm #1198

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

Merged
holgerd77 merged 1 commit into from
Apr 12, 2021
Merged

Remove all the debug traces from the vm #1198

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
600 changes: 426 additions & 174 deletions package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

25 changes: 11 additions & 14 deletions packages/vm/lib/evm/eei.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { debug as createDebugLogger } from 'debug'
import { Account, Address, BN } from 'ethereumjs-util'
import { Block } from '@ethereumjs/block'
import Blockchain from '@ethereumjs/blockchain'
Expand All @@ -9,8 +8,6 @@ import Message from './message'
import EVM, { EVMResult } from './evm'
import { Log } from './types'

const debugGas = createDebugLogger('vm:eei:gas')

function trap(err: ERROR) {
throw new VmError(err)
}
Expand Down Expand Up @@ -87,12 +84,12 @@ export default class EEI {
/**
* Subtracts an amount from the gas counter.
* @param amount - Amount of gas to consume
* @param context - Usage context for debugging
* @param _context - Deprecated: this param doesn't do anything now.
* @throws if out of gas
*/
useGas(amount: BN, context?: string): void {
// eslint-disable-next-line no-unused-vars
useGas(amount: BN, _context?: string): void {
this._gasLeft.isub(amount)
debugGas(`${context ? context + ': ' : ''}used ${amount} gas (-> ${this._gasLeft})`)
if (this._gasLeft.ltn(0)) {
this._gasLeft = new BN(0)
trap(ERROR.OUT_OF_GAS)
Expand All @@ -102,20 +99,20 @@ export default class EEI {
/**
* Adds a positive amount to the gas counter.
* @param amount - Amount of gas refunded
* @param context - Usage context for debugging
* @param _context - Deprecated: this param doesn't do anything now.
*/
refundGas(amount: BN, context?: string): void {
debugGas(`${context ? context + ': ' : ''}refund ${amount} gas (-> ${this._evm._refund})`)
// eslint-disable-next-line no-unused-vars
refundGas(amount: BN, _context?: string): void {
this._evm._refund.iadd(amount)
}

/**
* Reduces amount of gas to be refunded by a positive value.
* @param amount - Amount to subtract from gas refunds
* @param context - Usage context for debugging
* @param _context - Deprecated: this param doesn't do anything now.
*/
subRefund(amount: BN, context?: string): void {
debugGas(`${context ? context + ': ' : ''}sub gas refund ${amount} (-> ${this._evm._refund})`)
// eslint-disable-next-line no-unused-vars
subRefund(amount: BN, _context?: string): void {
this._evm._refund.isub(amount)
if (this._evm._refund.ltn(0)) {
this._evm._refund = new BN(0)
Expand Down Expand Up @@ -508,7 +505,7 @@ export default class EEI {
}

// this should always be safe
this.useGas(results.gasUsed, 'CALL, STATICCALL, DELEGATECALL, CALLCODE')
this.useGas(results.gasUsed)

// Set return value
if (
Expand Down Expand Up @@ -565,7 +562,7 @@ export default class EEI {
}

// this should always be safe
this.useGas(results.gasUsed, 'CREATE')
this.useGas(results.gasUsed)

// Set return buffer in case revert happened
if (
Expand Down
46 changes: 1 addition & 45 deletions packages/vm/lib/evm/evm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { debug as createDebugLogger } from 'debug'
import {
Account,
Address,
Expand All @@ -15,13 +14,9 @@ import { getPrecompile, PrecompileFunc } from './precompiles'
import TxContext from './txContext'
import Message from './message'
import EEI from './eei'
import { short } from './opcodes/util'
import { Log } from './types'
import { default as Interpreter, InterpreterOpts, RunState } from './interpreter'

const debug = createDebugLogger('vm:evm')
const debugGas = createDebugLogger('vm:evm:gas')

/**
* Result of executing a message via the [[EVM]].
*/
Expand Down Expand Up @@ -143,31 +138,14 @@ export default class EVM {
}

await this._state.checkpoint()
debug('-'.repeat(100))
debug(`message checkpoint`)

let result
debug(
`New message caller=${message.caller.toString()} gasLimit=${message.gasLimit.toString()} to=${
message.to ? message.to.toString() : ''
} value=${message.value.toString()} delegatecall=${message.delegatecall ? 'yes' : 'no'}`
)
if (message.to) {
debug(`Message CALL execution (to: ${message.to.toString()})`)
result = await this._executeCall(message)
} else {
debug(`Message CREATE execution (to undefined)`)
result = await this._executeCreate(message)
}
debug(
`Received message results gasUsed=${result.gasUsed} execResult: [ gasUsed=${
result.gasUsed
} exceptionError=${
result.execResult.exceptionError ? result.execResult.exceptionError.toString() : ''
} returnValue=${short(
result.execResult.returnValue
)} gasRefund=${result.execResult.gasRefund?.toString()} ]`
)

// TODO: Move `gasRefund` to a tx-level result object
// instead of `ExecResult`.
result.execResult.gasRefund = this._refund.clone()
Expand All @@ -177,16 +155,13 @@ export default class EVM {
if (this._vm._common.gteHardfork('homestead') || err.error != ERROR.CODESTORE_OUT_OF_GAS) {
result.execResult.logs = []
await this._state.revert()
debug(`message checkpoint reverted`)
} else {
// we are in chainstart and the error was the code deposit error
// we do like nothing happened.
await this._state.commit()
debug(`message checkpoint committed`)
}
} else {
await this._state.commit()
debug(`message checkpoint committed`)
}

await this._vm._emit('afterMessage', result)
Expand Down Expand Up @@ -217,11 +192,9 @@ export default class EVM {
let exit = false
if (!message.code || message.code.length === 0) {
exit = true
debug(`Exit early on no code`)
}
if (errorMessage) {
exit = true
debug(`Exit early on value tranfer overflowed`)
}
if (exit) {
return {
Expand All @@ -236,14 +209,12 @@ export default class EVM {

let result: ExecResult
if (message.isCompiled) {
debug(`Run precompile`)
result = await this.runPrecompile(
message.code as PrecompileFunc,
message.data,
message.gasLimit
)
} else {
debug(`Start bytecode processing...`)
result = await this.runInterpreter(message)
}

Expand All @@ -261,12 +232,10 @@ export default class EVM {
message.code = message.data
message.data = Buffer.alloc(0)
message.to = await this._generateAddress(message)
debug(`Generated CREATE contract address ${message.to.toString()}`)
let toAccount = await this._state.getAccount(message.to)

// Check for collision
if ((toAccount.nonce && toAccount.nonce.gtn(0)) || !toAccount.codeHash.equals(KECCAK256_NULL)) {
debug(`Returning on address collision`)
return {
gasUsed: message.gasLimit,
createdAddress: message.to,
Expand Down Expand Up @@ -304,11 +273,9 @@ export default class EVM {
let exit = false
if (!message.code || message.code.length === 0) {
exit = true
debug(`Exit early on no code`)
}
if (errorMessage) {
exit = true
debug(`Exit early on value tranfer overflowed`)
}
if (exit) {
return {
Expand All @@ -322,7 +289,6 @@ export default class EVM {
}
}

debug(`Start bytecode processing...`)
let result = await this.runInterpreter(message)

// fee for size of the return value
Expand All @@ -333,9 +299,6 @@ export default class EVM {
this._vm._common.param('gasPrices', 'createData')
)
totalGas = totalGas.add(returnFee)
debugGas(
`Add return value size fee (${returnFee.toString()} to gas used (-> ${totalGas.toString()}))`
)
}

// Check for SpuriousDragon EIP-170 code size limit
Expand All @@ -355,11 +318,9 @@ export default class EVM {
result.gasUsed = totalGas
} else {
if (this._vm._common.gteHardfork('homestead')) {
debug(`Not enough gas or code size not allowed (>= Homestead)`)
result = { ...result, ...OOGResult(message.gasLimit) }
} else {
// we are in Frontier
debug(`Not enough gas or code size not allowed (Frontier)`)
if (totalGas.sub(returnFee).lte(message.gasLimit)) {
// we cannot pay the code deposit fee (but the deposit code actually did run)
result = { ...result, ...COOGResult(totalGas.sub(returnFee)) }
Expand All @@ -373,7 +334,6 @@ export default class EVM {
// Save code if a new contract was created
if (!result.exceptionError && result.returnValue && result.returnValue.toString() !== '') {
await this._state.putContractCode(message.to, result.returnValue)
debug(`Code saved on new contract creation`)
} else if (CodestoreOOG) {
// This only happens at Frontier. But, let's do a sanity check;
if (!this._vm._common.gteHardfork('homestead')) {
Expand Down Expand Up @@ -512,9 +472,6 @@ export default class EVM {
async _reduceSenderBalance(account: Account, message: Message): Promise<void> {
account.balance.isub(message.value)
const result = this._state.putAccount(message.caller, account)
debug(
`Reduced sender (${message.caller.toString()}) balance (-> ${account.balance.toString()})`
)
return result
}

Expand All @@ -526,7 +483,6 @@ export default class EVM {
toAccount.balance = newBalance
// putAccount as the nonce may have changed for contract creation
const result = this._state.putAccount(message.to, toAccount)
debug(`Added toAccount (${message.to.toString()}) balance (-> ${toAccount.balance.toString()})`)
return result
}

Expand Down
27 changes: 1 addition & 26 deletions packages/vm/lib/evm/interpreter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { debug as createDebugLogger } from 'debug'
import { Account, Address, BN } from 'ethereumjs-util'
import Common from '@ethereumjs/common'
import { StateManager } from '../state/index'
Expand Down Expand Up @@ -66,9 +65,6 @@ export default class Interpreter {
_runState: RunState
_eei: EEI

// Opcode debuggers (e.g. { 'push': [debug Object], 'sstore': [debug Object], ...})
private opDebuggers: any = {}

constructor(vm: any, eei: EEI) {
this._vm = vm // TODO: remove when not needed
this._state = vm.stateManager
Expand Down Expand Up @@ -142,7 +138,7 @@ export default class Interpreter {
}

// Reduce opcode's base fee
this._eei.useGas(new BN(opInfo.fee), `${opInfo.name} (base fee)`)
this._eei.useGas(new BN(opInfo.fee))
// Advance program counter
this._runState.programCounter++

Expand Down Expand Up @@ -191,27 +187,6 @@ export default class Interpreter {
codeAddress: this._eei._env.codeAddress,
}

// Create opTrace for debug functionality
let hexStack = []
hexStack = eventObj.stack.map((item: any) => {
return '0x' + new BN(item).toString(16, 0)
})

const name = eventObj.opcode.name

@jochem-brouwer jochem-brouwer Apr 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it possible to regenerate this data in the step event? We have access to this eventObj in the event right? Then it is OK if all this is removed in this context.

@alcuadrado alcuadrado Apr 12, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, you can generate the same data.

@jochem-brouwer jochem-brouwer Apr 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change here changes the structure of the step event as mentioned in #1199

@alcuadrado alcuadrado Apr 13, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Where's the change? I really don't get it.

@jochem-brouwer jochem-brouwer Apr 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If one would listen to step like this;

vm.on('step', (obj: any) => {
// do stuff
let stack = obj.hexStack
// do stuff with stack (is now undefined)
})

Then this code now does not work anymore, since hexStack is not available anymore (and this opTrace thing is also not available anymore).

@alcuadrado alcuadrado Apr 13, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That object is not passed to the step handler. Take a look at line 232:

return this._vm._emit('step', eventObj)

Only eventObj is passed, which I didn't modify.

const opTrace = {
pc: eventObj.pc,
op: name,
gas: '0x' + eventObj.gasLeft.toString('hex'),
gasCost: '0x' + eventObj.opcode.fee.toString(16),
stack: hexStack,
depth: eventObj.depth,
}

if (!(name in this.opDebuggers)) {
this.opDebuggers[name] = createDebugLogger(`vm:ops:${name}`)
}
this.opDebuggers[name](JSON.stringify(opTrace))

/**
* The `step` event for trace output
*
Expand Down
43 changes: 9 additions & 34 deletions packages/vm/lib/evm/opcodes/EIP1283.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,70 +12,45 @@ export function updateSstoreGasEIP1283(runState: RunState, found: any, value: Bu
const { original, current } = found
if (current.equals(value)) {
// If current value equals new value (this is a no-op), 200 gas is deducted.
runState.eei.useGas(
new BN(runState._common.param('gasPrices', 'netSstoreNoopGas')),
'EIP-1283 -> netSstoreNoopGas'
)
runState.eei.useGas(new BN(runState._common.param('gasPrices', 'netSstoreNoopGas')))
return
}
// If current value does not equal new value
if (original.equals(current)) {
// If original value equals current value (this storage slot has not been changed by the current execution context)
if (original.length === 0) {
// If original value is 0, 20000 gas is deducted.
return runState.eei.useGas(
new BN(runState._common.param('gasPrices', 'netSstoreInitGas')),
'EIP-1283 -> netSstoreInitGas'
)
return runState.eei.useGas(new BN(runState._common.param('gasPrices', 'netSstoreInitGas')))
}
if (value.length === 0) {
// If new value is 0, add 15000 gas to refund counter.
runState.eei.refundGas(
new BN(runState._common.param('gasPrices', 'netSstoreClearRefund')),
'EIP-1283 -> netSstoreClearRefund'
)
runState.eei.refundGas(new BN(runState._common.param('gasPrices', 'netSstoreClearRefund')))
}
// Otherwise, 5000 gas is deducted.
return runState.eei.useGas(
new BN(runState._common.param('gasPrices', 'netSstoreCleanGas')),
'EIP-1283 -> netSstoreCleanGas'
)
return runState.eei.useGas(new BN(runState._common.param('gasPrices', 'netSstoreCleanGas')))
}
// If original value does not equal current value (this storage slot is dirty), 200 gas is deducted. Apply both of the following clauses.
if (original.length !== 0) {
// If original value is not 0
if (current.length === 0) {
// If current value is 0 (also means that new value is not 0), remove 15000 gas from refund counter. We can prove that refund counter will never go below 0.
runState.eei.subRefund(
new BN(runState._common.param('gasPrices', 'netSstoreClearRefund')),
'EIP-1283 -> netSstoreClearRefund'
)
runState.eei.subRefund(new BN(runState._common.param('gasPrices', 'netSstoreClearRefund')))
} else if (value.length === 0) {
// If new value is 0 (also means that current value is not 0), add 15000 gas to refund counter.
runState.eei.refundGas(
new BN(runState._common.param('gasPrices', 'netSstoreClearRefund')),
'EIP-1283 -> netSstoreClearRefund'
)
runState.eei.refundGas(new BN(runState._common.param('gasPrices', 'netSstoreClearRefund')))
}
}
if (original.equals(value)) {
// If original value equals new value (this storage slot is reset)
if (original.length === 0) {
// If original value is 0, add 19800 gas to refund counter.
runState.eei.refundGas(
new BN(runState._common.param('gasPrices', 'netSstoreResetClearRefund')),
'EIP-1283 -> netSstoreResetClearRefund'
new BN(runState._common.param('gasPrices', 'netSstoreResetClearRefund'))
)
} else {
// Otherwise, add 4800 gas to refund counter.
runState.eei.refundGas(
new BN(runState._common.param('gasPrices', 'netSstoreResetRefund')),
'EIP-1283 -> netSstoreResetRefund'
)
runState.eei.refundGas(new BN(runState._common.param('gasPrices', 'netSstoreResetRefund')))
}
}
return runState.eei.useGas(
new BN(runState._common.param('gasPrices', 'netSstoreDirtyGas')),
'EIP-1283 -> netSstoreDirtyGas'
)
return runState.eei.useGas(new BN(runState._common.param('gasPrices', 'netSstoreDirtyGas')))
}
Loading