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

EIP 1283 SSTORE #367

Merged
merged 1 commit into from
Oct 11, 2018
Merged

EIP 1283 SSTORE #367

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
83 changes: 72 additions & 11 deletions lib/opFns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,19 +374,10 @@ module.exports = {
value = val.toArrayLike(Buffer, 'be')
}

stateManager.getContractStorage(runState.address, key, function (err, found) {
getContractStorage(runState, address, key, function (err, found) {
if (err) return cb(err)
try {
if (value.length === 0 && !found.length) {
subGas(runState, new BN(runState._common.param('gasPrices', 'sstoreReset')))
} else if (value.length === 0 && found.length) {
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(runState._common.param('gasPrices', 'sstoreSet')))
} else if (value.length !== 0 && found.length) {
subGas(runState, new BN(runState._common.param('gasPrices', 'sstoreReset')))
}
updateSstoreGas(runState, found, value)
} catch (e) {
cb(e.error)
return
Expand Down Expand Up @@ -1024,3 +1015,73 @@ function makeCall (runState, callOptions, localOpts, cb) {
}
}
}

function getContractStorage (runState, address, key, cb) {
if (runState._common.gteHardfork('constantinople')) {
async.parallel({
original: function (cb) { runState.originalState.getContractStorage(address, key, cb) },
current: function (cb) { runState.stateManager.getContractStorage(address, key, cb) }
}, cb)
} else {
runState.stateManager.getContractStorage(address, key, cb)
Copy link
Member

Choose a reason for hiding this comment

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

Pre-constantinople logic seems to be cleanly separated, ok.

}
}

function updateSstoreGas (runState, found, value) {
if (runState._common.gteHardfork('constantinople')) {
var original = found.original
var current = found.current
if (current.equals(value)) {
// If current value equals new value (this is a no-op), 200 gas is deducted.
subGas(runState, new BN(runState._common.param('gasPrices', 'netSstoreNoopGas')))
return
}
Copy link
Member

Choose a reason for hiding this comment

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

First case of EIP, with return, ok.

Copy link
Member

Choose a reason for hiding this comment

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

Second look, as far as I see it these both versions 1) first subGas() call, then blank return and 2) return subGas() on other occasions doesn't make any difference I assume (?). Won't make this a review blocker, but unless there are reasons for it I am not seeing right now this might be updated to be more consistent if there is the occasion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! This should be return subGas... like in the rest of the code. This is stylistic and can be fixed later... not critical.

// 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 subGas(runState, new BN(runState._common.param('gasPrices', 'netSstoreInitGas')))
}
if (value.length === 0) {
// If new value is 0, add 15000 gas to refund counter.
runState.gasRefund = runState.gasRefund.addn(runState._common.param('gasPrices', 'netSstoreClearRefund'))
}
// Otherwise, 5000 gas is deducted.
return subGas(runState, 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.gasRefund = runState.gasRefund.subn(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.gasRefund = runState.gasRefund.addn(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.gasRefund = runState.gasRefund.addn(runState._common.param('gasPrices', 'netSstoreResetClearRefund'))
} else {
// Otherwise, add 4800 gas to refund counter.
runState.gasRefund = runState.gasRefund.addn(runState._common.param('gasPrices', 'netSstoreResetRefund'))
}
}
return subGas(runState, new BN(runState._common.param('gasPrices', 'netSstoreDirtyGas')))
Copy link
Member

Choose a reason for hiding this comment

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

This is working down the various cases from the EIP and adding to the existing runState.gasRefund variable. I assume this is correct since tests are passing here on a broad basis.

If there are smaller caveats or edge cases we can do subsequent PRs to correct once we have a clearer picture on the test runs and tests have stabilized more.

} else {
if (value.length === 0 && !found.length) {
subGas(runState, new BN(runState._common.param('gasPrices', 'sstoreReset')))
} else if (value.length === 0 && found.length) {
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(runState._common.param('gasPrices', 'sstoreSet')))
} else if (value.length !== 0 && found.length) {
subGas(runState, new BN(runState._common.param('gasPrices', 'sstoreReset')))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Same here, clean separation of pre-constantinople SSTORE gas calculations.

}
1 change: 1 addition & 0 deletions lib/runCode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ module.exports = function (opts, cb) {
var runState = {
blockchain: self.blockchain,
stateManager: stateManager,
originalState: stateManager.copy(),
returnValue: false,
stopped: false,
vmError: false,
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
"async-eventemitter": "^0.2.2",
"ethereumjs-account": "^2.0.3",
"ethereumjs-block": "~2.0.1",
"ethereumjs-common": "~0.4.0",
"ethereumjs-common": "^0.6.0",
"ethereumjs-util": "^6.0.0",
"fake-merkle-patricia-tree": "^1.0.1",
"functional-red-black-tree": "^1.0.1",
Expand Down
113 changes: 113 additions & 0 deletions tests/constantinopleSstoreTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
const tape = require('tape')
const async = require('async')
const VM = require('../')
const Account = require('ethereumjs-account')
const testUtil = require('./util')
const Trie = require('merkle-patricia-tree/secure')
const ethUtil = require('ethereumjs-util')
const BN = ethUtil.BN

var testCases = [
{ code: '0x60006000556000600055', usedGas: 412, refund: 0, original: '0x' },
{ code: '0x60006000556001600055', usedGas: 20212, refund: 0, original: '0x' },
{ code: '0x60016000556000600055', usedGas: 20212, refund: 19800, original: '0x' },
{ code: '0x60016000556002600055', usedGas: 20212, refund: 0, original: '0x' },
{ code: '0x60016000556001600055', usedGas: 20212, refund: 0, original: '0x' },
{ code: '0x60006000556000600055', usedGas: 5212, refund: 15000, original: '0x01' },
{ code: '0x60006000556001600055', usedGas: 5212, refund: 4800, original: '0x01' },
{ code: '0x60006000556002600055', usedGas: 5212, refund: 0, original: '0x01' },
{ code: '0x60026000556000600055', usedGas: 5212, refund: 15000, original: '0x01' },
{ code: '0x60026000556003600055', usedGas: 5212, refund: 0, original: '0x01' },
{ code: '0x60026000556001600055', usedGas: 5212, refund: 4800, original: '0x01' },
{ code: '0x60026000556002600055', usedGas: 5212, refund: 0, original: '0x01' },
{ code: '0x60016000556000600055', usedGas: 5212, refund: 15000, original: '0x01' },
{ code: '0x60016000556002600055', usedGas: 5212, refund: 0, original: '0x01' },
{ code: '0x60016000556001600055', usedGas: 412, refund: 0, original: '0x01' },
{ code: '0x600160005560006000556001600055', usedGas: 40218, refund: 19800, original: '0x' },
{ code: '0x600060005560016000556000600055', usedGas: 10218, refund: 19800, original: '0x01' }
]

var testData = {
'env': {
'currentCoinbase': '0x2adc25665018aa1fe0e6bc666dac8fc2697ff9ba',
'currentDifficulty': '0x0100',
'currentGasLimit': '0x0f4240',
'currentNumber': '0x00',
'currentTimestamp': '0x01'
},
'exec': {
'address': '0x01',
'caller': '0xcd1722f3947def4cf144679da39c4c32bdc35681',
'code': '0x60006000556000600055',
'data': '0x',
'gas': '0',
'gasPrice': '0x5af3107a4000',
'origin': '0xcd1722f3947def4cf144679da39c4c32bdc35681',
'value': '0x0de0b6b3a7640000'
},
'gas': '0',
'pre': {
'0x01': {
'balance': '0x152d02c7e14af6800000',
'code': '0x',
'nonce': '0x00',
'storage': {
'0x': '0'
}
}
}
}

tape('test constantinople SSTORE (eip-1283)', function (t) {
testCases.forEach(function (params, i) {
t.test('should correctly run eip-1283 test #' + i, function (st) {
let state = new Trie()
let results
let account

testData.exec.code = params.code
testData.exec.gas = params.usedGas
testData.pre['0x01'].storage['0x'] = params.original

async.series([
function (done) {
let acctData = testData.pre[testData.exec.address]
account = new Account()
account.nonce = testUtil.format(acctData.nonce)
account.balance = testUtil.format(acctData.balance)
testUtil.setupPreConditions(state, testData, done)
},
function (done) {
state.get(Buffer.from(testData.exec.address, 'hex'), function (err, data) {
let a = new Account(data)
account.stateRoot = a.stateRoot
done(err)
})
},
function (done) {
let block = testUtil.makeBlockFromEnv(testData.env)
let vm = new VM({state: state, hardfork: 'constantinople'})
let runCodeData = testUtil.makeRunCodeData(testData.exec, account, block)
vm.runCode(runCodeData, function (err, r) {
if (r) {
results = r
}
done(err)
})
},
function (done) {
if (testData.gas) {
let actualGas = results.gas.toString()
let expectedGas = new BN(testUtil.format(testData.gas)).toString()
t.equal(actualGas, expectedGas, 'valid gas usage')
t.equals(results.gasRefund.toNumber(), params.refund, 'valid gas refund')
}
done()
}
], function (err) {
t.assert(!err)
st.end()
})
})
})
})
1 change: 1 addition & 0 deletions tests/tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ function runAll () {
require('./tester.js')
require('./cacheTest.js')
require('./genesishashes.js')
require('./constantinopleSstoreTest.js')
async.series([
// runTests.bind(this, 'VMTests', {}), // VM tests disabled since we don't support Frontier gas costs
runTests.bind(this, 'GeneralStateTests', {}),
Expand Down