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

Some smaller cleanups (mostly regarding calls) #186

Merged
merged 7 commits into from
Aug 22, 2017
Merged

Some smaller cleanups (mostly regarding calls) #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f7f7c0e
Simplify some gas calculations
axic Aug 18, 2017
2477e66
Bubble up runCall errors via parseCallResults (actual EVM-level error…
axic Aug 18, 2017
948e459
Do not pass err in successful callbacks
axic Aug 18, 2017
339ecfe
Set compiled to false if not set
axic Aug 18, 2017
67f23a0
Load account for CALLCODE too (as for DELEGATECALL)
axic Aug 18, 2017
5b3593e
Handle errors in CALL
axic Aug 20, 2017
f4122b0
Handle errors in async calls
axic Aug 20, 2017
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
55 changes: 32 additions & 23 deletions lib/opFns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ module.exports = {

memStore(runState, memOffset, runState.callData, dataOffset, dataLength)
// sub the COPY fee
subGas(runState, new BN(Number(fees.copyGas.v) * Math.ceil(dataLength / 32)))
subGas(runState, new BN(fees.copyGas.v).imuln(Math.ceil(dataLength / 32)))
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

},
CODESIZE: function (runState) {
return utils.intToBuffer(runState.code.length)
Expand All @@ -270,14 +270,14 @@ module.exports = {

memStore(runState, memOffset, runState.code, codeOffset, length)
// sub the COPY fee
subGas(runState, new BN(fees.copyGas.v * Math.ceil(length / 32)))
subGas(runState, new BN(fees.copyGas.v).imuln(Math.ceil(length / 32)))
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

},
EXTCODESIZE: function (address, runState, cb) {
var stateManager = runState.stateManager
address = utils.setLengthLeft(address, 20)
stateManager.getContractCode(address, function (err, code) {
if (err) return cb(err)
cb(err, utils.intToBuffer(code.length))
cb(null, utils.intToBuffer(code.length))
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

})
},
EXTCODECOPY: function (address, memOffset, codeOffset, length, runState, cb) {
Expand All @@ -286,15 +286,16 @@ module.exports = {
memOffset = utils.bufferToInt(memOffset)
codeOffset = utils.bufferToInt(codeOffset)
length = utils.bufferToInt(length)
subMemUsage(runState, memOffset, length)

// FIXME: for some reason this must come before subGas
subMemUsage(runState, memOffset, length)
// copy fee
subGas(runState, new BN(fees.copyGas.v).imuln(Math.ceil(length / 32)))

stateManager.getContractCode(address, function (err, code) {
if (err) return cb(err)
memStore(runState, memOffset, code, codeOffset, length, false)
cb(err)
cb(null)
})
},
GASPRICE: function (runState) {
Expand Down Expand Up @@ -386,7 +387,7 @@ module.exports = {
stateManager.putContractStorage(address, key, value, function (err) {
if (err) return cb(err)
runState.contract = stateManager.cache.get(address)
cb()
cb(null)
Copy link
Member

Choose a reason for hiding this comment

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

Both ok.

})
})
},
Expand Down Expand Up @@ -473,7 +474,7 @@ module.exports = {
memLength = utils.bufferToInt(memLength)
const numOfTopics = runState.opCode - 0xa0
const mem = memLoad(runState, memOffset, memLength)
subGas(runState, new BN(numOfTopics * fees.logTopicGas.v + memLength * fees.logDataGas.v))
subGas(runState, new BN(fees.logTopicGas.v).imuln(numOfTopics).iadd(new BN(fees.logDataGas.v).imuln(memLength)))
Copy link
Member

Choose a reason for hiding this comment

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

You can optionally put some brackets around first multiplication like (new BN(fees.logTopicGas.v).imuln(numOfTopics)) for better readability/consistency, otherwise ok.


// add address
var log = [runState.address]
Expand Down Expand Up @@ -616,18 +617,22 @@ module.exports = {
options.gasLimit.iadd(new BN(fees.callStipend.v))
}

if (utils.isPrecompiled(toAddress)) {
options.compiled = true
options.code = runState._precompiled[toAddress.toString('hex')]
makeCall(runState, options, localOpts, done)
} else {
stateManager.getContractCode(toAddress, function (err, code, compiled) {
if (err) return done(err)
options.code = code
options.compiled = compiled
// load the code
stateManager.getAccount(toAddress, function (err, account) {
if (err) return done(err)
if (utils.isPrecompiled(toAddress)) {
options.compiled = true
options.code = runState._precompiled[toAddress.toString('hex')]
makeCall(runState, options, localOpts, done)
})
}
} else {
stateManager.getContractCode(toAddress, function (err, code, compiled) {
if (err) return done(err)
options.compiled = compiled || false
options.code = code
makeCall(runState, options, localOpts, done)
})
}
})
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, code looks correct, not completely understand the changing semantics here, so probably for someone else to review again.

Copy link
Member Author

Choose a reason for hiding this comment

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

The account must exists even for precompiles.

},
DELEGATECALL: function (gas, toAddress, inOffset, inLength, outOffset, outLength, runState, done) {
var stateManager = runState.stateManager
Expand Down Expand Up @@ -670,8 +675,8 @@ module.exports = {
} else {
stateManager.getContractCode(toAddress, function (err, code, compiled) {
if (err) return done(err)
options.compiled = compiled || false
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

options.code = code
options.compiled = compiled
makeCall(runState, options, localOpts, done)
})
}
Expand Down Expand Up @@ -874,7 +879,7 @@ function makeCall (runState, callOptions, localOpts, cb) {
// Note: in the case of delegatecall, the value is persisted and doesn't need to be deducted again
if (runState.depth >= fees.stackLimit.v || (callOptions.delegatecall !== true && new BN(runState.contract.balance).lt(callOptions.value))) {
runState.stack.push(Buffer.from([0]))
cb()
cb(null)
} else {
// if creating a new contract then increament the nonce
if (!callOptions.to) {
Expand All @@ -886,6 +891,8 @@ function makeCall (runState, callOptions, localOpts, cb) {
}

function parseCallResults (err, results) {
if (err) return cb(err)

// concat the runState.logs
if (results.vm.logs) {
runState.logs = runState.logs.concat(results.vm.logs)
Expand All @@ -907,12 +914,14 @@ function makeCall (runState, callOptions, localOpts, cb) {

// update stateRoot on current contract
runState.stateManager.getAccount(runState.address, function (err, account) {
if (err) return cb(err)

runState.contract = account
// push the created address to the stack
if (results.createdAddress) {
cb(err, results.createdAddress)
cb(null, results.createdAddress)
} else {
cb(err, Buffer.from([results.vm.exception]))
cb(null, Buffer.from([results.vm.exception]))
}
})
} else {
Expand All @@ -921,7 +930,7 @@ function makeCall (runState, callOptions, localOpts, cb) {
runState.contract.nonce = new BN(runState.contract.nonce).subn(1)
}

cb(err, Buffer.from([results.vm.exception]))
cb(null, Buffer.from([results.vm.exception]))
Copy link
Member

Choose a reason for hiding this comment

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

All above ok.

}
}
}
5 changes: 4 additions & 1 deletion lib/runCode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,14 +173,17 @@ module.exports = function (opts, cb) {
// create a callback for async opFunc
if (opInfo.async) {
args.push(function (err, result) {
if (err) return cb(err)

// save result to the stack
if (result) {
// NOTE: Ensure that every stack item is padded to 256 bits.
// This should be done at every opcode in the future.
result = utils.setLengthLeft(result, 32)
runState.stack.push(result)
}
cb(err)

cb()
Copy link
Member

Choose a reason for hiding this comment

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

Call with null here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, though that file has all the callbacks without null, need to tackle it at some point. Also some other methods (parseVmResults) there have an issue passing both err and results. Haven't reviewed it yet.

})
}

Expand Down