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.

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

Improve chain and fork support #304

Merged
merged 3 commits into from
Jul 20, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ Creates a new VM object
- `stateManager` - A state manager instance (**EXPERIMENTAL** - unstable API)
- `state` - A merkle-patricia-tree instance for the state tree (ignored if `stateManager` is passed)
- `blockchain` - A blockchain object for storing/retrieving blocks (ignored if `stateManager` is passed)
- `chain` - The chain the VM operates on [default: 'mainnet']
- `hardfork` - Hardfork rules to be used [default: 'byzantium', supported: 'byzantium' (will throw on unsupported)]
- `activatePrecompiles` - Create entries in the state tree for the precompiled contracts
- `allowUnlimitedContractSize` - Allows unlimited contract sizes while debugging. By setting this to `true`, the check for contract size limit of 2KB (see [EIP-170](https://git.io/vxZkK)) is bypassed. (default: `false`; **ONLY** set to `true` during debugging).

Expand Down
11 changes: 10 additions & 1 deletion lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const Buffer = require('safe-buffer').Buffer
const util = require('util')
const ethUtil = require('ethereumjs-util')
const StateManager = require('./stateManager.js')
const Common = require('ethereumjs-common')
const Account = require('ethereumjs-account')
const AsyncEventEmitter = require('async-eventemitter')
const BN = ethUtil.BN
Expand Down Expand Up @@ -31,18 +32,26 @@ VM.deps = {
* @param {StateManager} [opts.stateManager] A state manager instance (EXPERIMENTAL - unstable API)
* @param {Trie} [opts.state] A merkle-patricia-tree instance for the state tree (ignored if stateManager is passed)
* @param {Blockchain} [opts.blockchain] A blockchain object for storing/retrieving blocks (ignored if stateManager is passed)
* @param {String|Number} opts.chain The chain the VM operates on [default: 'mainnet']
* @param {String} opts.hardfork Hardfork rules to be used [default: 'byzantium', supported: 'byzantium' (will throw on unsupported)]
* @param {Boolean} [opts.activatePrecompiles] Create entries in the state tree for the precompiled contracts
* @param {Boolean} [opts.allowUnlimitedContractSize] Allows unlimited contract sizes while debugging (default: false; ONLY use during debugging)
*/
function VM (opts = {}) {
this.opts = opts

let chain = opts.chain ? opts.chain : 'mainnet'
let hardfork = opts.hardfork ? opts.hardfork : 'byzantium'
let supportedHardforks = [ 'byzantium' ]
this._common = new Common(chain, hardfork, supportedHardforks)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should just be a static map from hard fork -> costs. It doesn't seem to make sense to instantiate it as an object and then attach it to the current runState since it is external to the call state and static for the lifetime of a transaction.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is deeply rooted in the new ethereumjs-common library and there are various reasons for why this is designed to be created as an object, please have a closer look into the library if you want to understand what it does and the reasons for the design choices.

Copy link
Member Author

Choose a reason for hiding this comment

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

One main thing it does is to encapsulate the hardfork choice switch, otherwise you would have to do in the using library (here: the VM) every time you want to access an parameter value, like:

if (vm.hardfork === 'byzantium')  {
  // read param from byzantium.js file
} if (vm.hardfork === 'constantinople') {
  // ...
}

With instantiating the Common library with network and the hardfork, this is further done internally and the library makes the correct choice with:

c.param('pow', 'minerReward')

Copy link
Contributor

Choose a reason for hiding this comment

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

It is much easier to have the costs structured like this:

const ethereumjsCommon = {
  'Byzantium': {
    ...
  },
  'Constantinople': {
    'blockReward': ...,
     .... 
  },
  ...
}

At the very least, if this object could be instantiated once when it is imported and not attached to the runState, I think it would be cleaner and more intuitive.

Copy link
Contributor

Choose a reason for hiding this comment

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

One main thing it does is to encapsulate the hardfork choice switch, otherwise you would have to do in the using library (here: the VM) every time you want to access an parameter value, like:

if (vm.hardfork === 'byzantium')  {
  // read param from byzantium.js file
} if (vm.hardfork === 'constantinople') {
  // ...
}

Or you have all HF parameters in one file with the JSON structure that I pasted up above. Then you don't need to do any if statement like this.

But... I digress. It would be great to have the common parameters as a global const variable. Not attached to runState. Then I think this can be merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jwasinger Could you move on with this, I would like to move on to Constantinople work and would need this as a basis. I really can't abstain attaching this to the run state, otherwise I would have to re-create the whole commons library and would loose much of the benefits from there, and this was in development for weeks.

The commons library has been reviewed thoroughly by Vinay and the API constructors used here are already in production on the latest block library release, so I would like to introduce an analogue constructor instantiation in other libraries so that things stay consistent.

I actually thought about this more to be a you-guys-should-at-least-perceive-once-whats-happening here review and not as a tear-everything-apart review, hehe. 😄 Nevertheless you are of course free to bring on further criticism if you stumble on things not thought through thoroughly.

Copy link
Member

Choose a reason for hiding this comment

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

If this has hardfork and suppertedHardforks as an argument, I'd assume it returns nil if hardfork is not part of the supported ones. Should this code here check if the call failed or returned nil meaning an unsupported fork was chosen?

Copy link
Member

Choose a reason for hiding this comment

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

Answer is here:

This will e.g. throw an error when a param is requested for an unsupported hardfork and like this prevents unpredicted behaviour.

Though perhaps a comment here may be good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will make this more clear in the constructor documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@axic Added the comment you suggested to the JSDoc and README. Do you also want to have a broader look on this? I would otherwise merge within 24h as announced above.


if (opts.stateManager) {
this.stateManager = opts.stateManager
} else {
this.stateManager = new StateManager({
trie: opts.state,
blockchain: opts.blockchain
blockchain: opts.blockchain,
common: this._common
})
}

Expand Down
49 changes: 24 additions & 25 deletions lib/opFns.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
const Buffer = require('safe-buffer').Buffer
const async = require('async')
const fees = require('ethereum-common')
const utils = require('ethereumjs-util')
const BN = utils.BN
const exceptions = require('./exceptions.js')
Expand Down Expand Up @@ -95,7 +94,7 @@ module.exports = {

if (!exponent.isZero()) {
var bytes = 1 + logTable(exponent)
subGas(runState, new BN(bytes).muln(fees.expByteGas.v))
subGas(runState, new BN(bytes).muln(runState._common.param('gasPrices', 'expByte')))
return base.redPow(exponent)
} else {
return new BN(1)
Expand Down Expand Up @@ -162,7 +161,7 @@ module.exports = {
SHA3: function (offset, length, runState) {
var data = memLoad(runState, offset, length)
// copy fee
subGas(runState, new BN(fees.sha3WordGas.v).imul(length.divCeil(new BN(32))))
subGas(runState, new BN(runState._common.param('gasPrices', 'sha3Word')).imul(length.divCeil(new BN(32))))
return new BN(utils.sha3(data))
},
// 0x30 range - closure state
Expand Down Expand Up @@ -217,15 +216,15 @@ module.exports = {
CALLDATACOPY: function (memOffset, dataOffset, dataLength, runState) {
memStore(runState, memOffset, runState.callData, dataOffset, dataLength)
// sub the COPY fee
subGas(runState, new BN(fees.copyGas.v).imul(dataLength.divCeil(new BN(32))))
subGas(runState, new BN(runState._common.param('gasPrices', 'copy')).imul(dataLength.divCeil(new BN(32))))
},
CODESIZE: function (runState) {
return new BN(runState.code.length)
},
CODECOPY: function (memOffset, codeOffset, length, runState) {
memStore(runState, memOffset, runState.code, codeOffset, length)
// sub the COPY fee
subGas(runState, new BN(fees.copyGas.v).imul(length.divCeil(new BN(32))))
subGas(runState, new BN(runState._common.param('gasPrices', 'copy')).imul(length.divCeil(new BN(32))))
},
EXTCODESIZE: function (address, runState, cb) {
var stateManager = runState.stateManager
Expand All @@ -242,7 +241,7 @@ module.exports = {
// FIXME: for some reason this must come before subGas
subMemUsage(runState, memOffset, length)
// copy fee
subGas(runState, new BN(fees.copyGas.v).imul(length.divCeil(new BN(32))))
subGas(runState, new BN(runState._common.param('gasPrices', 'copy')).imul(length.divCeil(new BN(32))))

stateManager.getContractCode(address, function (err, code) {
if (err) return cb(err)
Expand All @@ -260,7 +259,7 @@ module.exports = {

memStore(runState, memOffset, utils.toBuffer(runState.lastReturned), returnDataOffset, length, false)
// sub the COPY fee
subGas(runState, new BN(fees.copyGas.v).mul(length.divCeil(new BN(32))))
subGas(runState, new BN(runState._common.param('gasPrices', 'copy')).mul(length.divCeil(new BN(32))))
},
GASPRICE: function (runState) {
return new BN(runState.gasPrice)
Expand Down Expand Up @@ -339,14 +338,14 @@ module.exports = {
if (err) return cb(err)
try {
if (value.length === 0 && !found.length) {
subGas(runState, new BN(fees.sstoreResetGas.v))
subGas(runState, new BN(runState._common.param('gasPrices', 'sstoreReset')))
} else if (value.length === 0 && found.length) {
subGas(runState, new BN(fees.sstoreResetGas.v))
runState.gasRefund.iaddn(fees.sstoreRefundGas.v)
subGas(runState, new BN(runState._common.param('gasPrices', 'sstoreReset')))
runState.gasRefund.iaddn(runState._common.param('gasPrices', 'sstoreRefund'))
} else if (value.length !== 0 && !found.length) {
subGas(runState, new BN(fees.sstoreSetGas.v))
subGas(runState, new BN(runState._common.param('gasPrices', 'sstoreSet')))
} else if (value.length !== 0 && found.length) {
subGas(runState, new BN(fees.sstoreResetGas.v))
subGas(runState, new BN(runState._common.param('gasPrices', 'sstoreReset')))
}
} catch (e) {
cb(e.error)
Expand Down Expand Up @@ -445,7 +444,7 @@ module.exports = {

const numOfTopics = runState.opCode - 0xa0
const mem = memLoad(runState, memOffset, memLength)
subGas(runState, new BN(fees.logTopicGas.v).imuln(numOfTopics).iadd(memLength.muln(fees.logDataGas.v)))
subGas(runState, new BN(runState._common.param('gasPrices', 'logTopic')).imuln(numOfTopics).iadd(memLength.muln(runState._common.param('gasPrices', 'logData'))))

// add address
var log = [runState.address]
Expand Down Expand Up @@ -507,7 +506,7 @@ module.exports = {
}

if (!value.isZero()) {
subGas(runState, new BN(fees.callValueTransferGas.v))
subGas(runState, new BN(runState._common.param('gasPrices', 'callValueTransfer')))
}

stateManager.exists(toAddress, function (err, exists) {
Expand All @@ -525,7 +524,7 @@ module.exports = {
if (!exists || empty) {
if (!value.isZero()) {
try {
subGas(runState, new BN(fees.callNewAccountGas.v))
subGas(runState, new BN(runState._common.param('gasPrices', 'callNewAccount')))
} catch (e) {
done(e.error)
return
Expand All @@ -542,8 +541,8 @@ module.exports = {
}

if (!value.isZero()) {
runState.gasLeft.iaddn(fees.callStipend.v)
options.gasLimit.iaddn(fees.callStipend.v)
runState.gasLeft.iaddn(runState._common.param('gasPrices', 'callStipend'))
options.gasLimit.iaddn(runState._common.param('gasPrices', 'callStipend'))
}

makeCall(runState, options, localOpts, done)
Expand Down Expand Up @@ -572,15 +571,15 @@ module.exports = {
}

if (!value.isZero()) {
subGas(runState, new BN(fees.callValueTransferGas.v))
subGas(runState, new BN(runState._common.param('gasPrices', 'callValueTransfer')))
}

checkCallMemCost(runState, options, localOpts)
checkOutOfGas(runState, options)

if (!value.isZero()) {
runState.gasLeft.iaddn(fees.callStipend.v)
options.gasLimit.iaddn(fees.callStipend.v)
runState.gasLeft.iaddn(runState._common.param('gasPrices', 'callStipend'))
options.gasLimit.iaddn(runState._common.param('gasPrices', 'callStipend'))
}

// load the code
Expand Down Expand Up @@ -724,7 +723,7 @@ module.exports = {
if ((new BN(contract.balance)).gtn(0)) {
if (!toAccount.exists || empty) {
try {
subGas(runState, new BN(fees.callNewAccountGas.v))
subGas(runState, new BN(runState._common.param('gasPrices', 'callNewAccount')))
} catch (e) {
cb(e.error)
return
Expand All @@ -734,7 +733,7 @@ module.exports = {

// only add to refund if this is the first selfdestruct for the address
if (!runState.selfdestruct[contractAddress.toString('hex')]) {
runState.gasRefund = runState.gasRefund.addn(fees.suicideRefundGas.v)
runState.gasRefund = runState.gasRefund.addn(runState._common.param('gasPrices', 'selfdestructRefund'))
}
runState.selfdestruct[contractAddress.toString('hex')] = selfdestructToAddress
runState.stopped = true
Expand Down Expand Up @@ -786,8 +785,8 @@ function subMemUsage (runState, offset, length) {
if (newMemoryWordCount.lte(runState.memoryWordCount)) return

const words = newMemoryWordCount
const fee = new BN(fees.memoryGas.v)
const quadCoeff = new BN(fees.quadCoeffDiv.v)
const fee = new BN(runState._common.param('gasPrices', 'memory'))
const quadCoeff = new BN(runState._common.param('gasPrices', 'quadCoeffDiv'))
// words * 3 + words ^2 / 512
const cost = words.mul(fee).add(words.mul(words).div(quadCoeff))

Expand Down Expand Up @@ -927,7 +926,7 @@ function makeCall (runState, callOptions, localOpts, cb) {

// check if account has enough ether
// 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))) {
if (runState.depth >= runState._common.param('vm', 'stackLimit') || (callOptions.delegatecall !== true && new BN(runState.contract.balance).lt(callOptions.value))) {
cb(null, new BN(0))
} else {
// if creating a new contract then increament the nonce
Expand Down
3 changes: 1 addition & 2 deletions lib/precompiled/01-ecrecover.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
const utils = require('ethereumjs-util')
const BN = utils.BN
const error = require('../exceptions.js').ERROR
const fees = require('ethereum-common')
const assert = require('assert')

module.exports = function (opts) {
assert(opts.data)

var results = {}

results.gasUsed = new BN(fees.ecrecoverGas.v)
results.gasUsed = new BN(opts._common.param('gasPrices', 'ecRecover'))

if (opts.gasLimit.lt(results.gasUsed)) {
results.gasUsed = opts.gasLimit
Expand Down
5 changes: 2 additions & 3 deletions lib/precompiled/02-sha256.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const utils = require('ethereumjs-util')
const BN = utils.BN
const error = require('../exceptions.js').ERROR
const fees = require('ethereum-common')
const assert = require('assert')

module.exports = function (opts) {
Expand All @@ -10,8 +9,8 @@ module.exports = function (opts) {
var results = {}
var data = opts.data

results.gasUsed = new BN(fees.sha256Gas.v)
results.gasUsed.iadd(new BN(fees.sha256WordGas.v).imuln(Math.ceil(data.length / 32)))
results.gasUsed = new BN(opts._common.param('gasPrices', 'sha256'))
results.gasUsed.iadd(new BN(opts._common.param('gasPrices', 'sha256Word')).imuln(Math.ceil(data.length / 32)))

if (opts.gasLimit.lt(results.gasUsed)) {
results.gasUsed = opts.gasLimit
Expand Down
5 changes: 2 additions & 3 deletions lib/precompiled/03-ripemd160.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const utils = require('ethereumjs-util')
const BN = utils.BN
const error = require('../exceptions.js').ERROR
const fees = require('ethereum-common')
const assert = require('assert')

module.exports = function (opts) {
Expand All @@ -10,8 +9,8 @@ module.exports = function (opts) {
var results = {}
var data = opts.data

results.gasUsed = new BN(fees.ripemd160Gas.v)
results.gasUsed.iadd(new BN(fees.ripemd160WordGas.v).imuln(Math.ceil(data.length / 32)))
results.gasUsed = new BN(opts._common.param('gasPrices', 'ripemd160'))
results.gasUsed.iadd(new BN(opts._common.param('gasPrices', 'ripemd160Word')).imuln(Math.ceil(data.length / 32)))

if (opts.gasLimit.lt(results.gasUsed)) {
results.gasUsed = opts.gasLimit
Expand Down
5 changes: 2 additions & 3 deletions lib/precompiled/04-identity.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
const utils = require('ethereumjs-util')
const BN = utils.BN
const fees = require('ethereum-common')
const error = require('../exceptions.js').ERROR
const assert = require('assert')

Expand All @@ -10,8 +9,8 @@ module.exports = function (opts) {
var results = {}
var data = opts.data

results.gasUsed = new BN(fees.identityGas.v)
results.gasUsed.iadd(new BN(fees.identityWordGas.v).imuln(Math.ceil(data.length / 32)))
results.gasUsed = new BN(opts._common.param('gasPrices', 'identity'))
results.gasUsed.iadd(new BN(opts._common.param('gasPrices', 'identityWord')).imuln(Math.ceil(data.length / 32)))

if (opts.gasLimit.lt(results.gasUsed)) {
results.gasUsed = opts.gasLimit
Expand Down
4 changes: 1 addition & 3 deletions lib/precompiled/05-modexp.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
const utils = require('ethereumjs-util')
const BN = utils.BN
const error = require('../exceptions.js').ERROR
const fees = require('ethereum-common')
const assert = require('assert')

const Gquaddivisor = fees.modexpGquaddivisor.v

function multComplexity (x) {
var fac1 = new BN(0)
var fac2 = new BN(0)
Expand Down Expand Up @@ -95,6 +92,7 @@ module.exports = function (opts) {
if (maxLen.lt(mLen)) {
maxLen = mLen
}
const Gquaddivisor = opts._common.param('gasPrices', 'modexpGquaddivisor')
var gasUsed = adjustedELen.mul(multComplexity(maxLen)).divn(Gquaddivisor)

if (opts.gasLimit.lt(gasUsed)) {
Expand Down
3 changes: 1 addition & 2 deletions lib/precompiled/06-ecadd.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const utils = require('ethereumjs-util')
const BN = utils.BN
const error = require('../exceptions.js').ERROR
const fees = require('ethereum-common')
const assert = require('assert')

const bn128Module = require('rustbn.js')
Expand All @@ -14,7 +13,7 @@ module.exports = function (opts) {
let data = opts.data
let inputHexStr = data.toString('hex')

results.gasUsed = new BN(fees.ecAddGas.v)
results.gasUsed = new BN(opts._common.param('gasPrices', 'ecAdd'))
if (opts.gasLimit.lt(results.gasUsed)) {
results.return = Buffer.alloc(0)
results.exception = 0
Expand Down
3 changes: 1 addition & 2 deletions lib/precompiled/07-ecmul.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const utils = require('ethereumjs-util')
const ERROR = require('../exceptions.js').ERROR
const BN = utils.BN
const fees = require('ethereum-common')
const assert = require('assert')

const bn128Module = require('rustbn.js')
Expand All @@ -14,7 +13,7 @@ module.exports = function (opts) {
let data = opts.data

let inputHexStr = data.toString('hex')
results.gasUsed = new BN(fees.ecMulGas.v)
results.gasUsed = new BN(opts._common.param('gasPrices', 'ecMul'))

if (opts.gasLimit.lt(results.gasUsed)) {
results.return = Buffer.alloc(0)
Expand Down
3 changes: 1 addition & 2 deletions lib/precompiled/08-ecpairing.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const utils = require('ethereumjs-util')
const BN = utils.BN
const error = require('../exceptions.js').ERROR
const fees = require('ethereum-common')
const assert = require('assert')

const bn128Module = require('rustbn.js')
Expand All @@ -17,7 +16,7 @@ module.exports = function (opts) {
let inputData = Buffer.from(inputHexStr, 'hex')
let inputDataSize = Math.floor(inputData.length / 192)

const gascost = fees.ecPairingGas.v + (inputDataSize * fees.ecPairingWordGas.v)
const gascost = opts._common.param('gasPrices', 'ecPairing') + (inputDataSize * opts._common.param('gasPrices', 'ecPairingWord'))
results.gasUsed = new BN(gascost)

if (opts.gasLimit.ltn(gascost)) {
Expand Down
5 changes: 2 additions & 3 deletions lib/runBlock.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,10 @@ const Buffer = require('safe-buffer').Buffer
const async = require('async')
const ethUtil = require('ethereumjs-util')
const Bloom = require('./bloom.js')
const common = require('ethereum-common')
const rlp = ethUtil.rlp
const Trie = require('merkle-patricia-tree')
const BN = ethUtil.BN

const minerReward = new BN(common.minerReward.v)

/**
* process the transaction in a block and pays the miners
* @param opts
Expand Down Expand Up @@ -189,6 +186,7 @@ module.exports = function (opts, cb) {
ommers.forEach(rewardOmmer)

// calculate nibling reward
var minerReward = new BN(self._common.param('pow', 'minerReward'))
var niblingReward = minerReward.divn(32)
var totalNiblingReward = niblingReward.muln(ommers.length)
minerAccount = self.stateManager.cache.get(block.header.coinbase)
Expand All @@ -202,6 +200,7 @@ module.exports = function (opts, cb) {
// credit ommer
function rewardOmmer (ommer) {
// calculate reward
var minerReward = new BN(self._common.param('pow', 'minerReward'))
var heightDiff = new BN(block.header.number).sub(new BN(ommer.number))
var reward = ((new BN(8)).sub(heightDiff)).mul(minerReward.divn(8))

Expand Down
Loading