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

Clean-up state-manager.touched #287

Merged
merged 1 commit into from
Apr 16, 2018
Merged

Clean-up state-manager.touched #287

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
23 changes: 13 additions & 10 deletions lib/runCall.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,10 @@ module.exports = function (opts, cb) {
stateManager.checkpoint()

// run and parse
subTxValue()

async.series([
subTxValue,
loadToAccount,
addTxValue,
loadCode,
runCode,
saveCode
Expand Down Expand Up @@ -83,26 +83,29 @@ module.exports = function (opts, cb) {
}
}

function subTxValue () {
function subTxValue (cb) {
if (delegatecall) {
cb()
return
}
account.balance = new BN(account.balance).sub(txValue)
stateManager.cache.put(caller, account)
var newBalance = new BN(account.balance).sub(txValue)
account.balance = newBalance
stateManager.putAccountBalance(ethUtil.toBuffer(caller), newBalance, cb)
}

function addTxValue () {
function addTxValue (cb) {
if (delegatecall) {
cb()
return
}
// add the amount sent to the `to` account
toAccount.balance = new BN(toAccount.balance).add(txValue)
stateManager.cache.put(toAddress, toAccount)
stateManager.touched.push(toAddress)
var newBalance = new BN(toAccount.balance).add(txValue)
toAccount.balance = newBalance
// putAccount as the nonce may have changed for contract creation
stateManager.putAccount(ethUtil.toBuffer(toAddress), toAccount, cb)
Copy link
Member

Choose a reason for hiding this comment

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

Up to here, I think changes are functionally equivalent to the old version.

}

function loadCode (cb) {
addTxValue()
// loads the contract's code if the account is a contract
if (code || !(toAccount.isContract() || self._precompiled[toAddress.toString('hex')])) {
cb()
Expand Down
1 change: 0 additions & 1 deletion lib/runCode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,6 @@ module.exports = function (opts, cb) {
if (results.exceptionError) {
delete results.gasRefund
delete results.selfdestruct
self.stateManager.touched = []
Copy link
Member

Choose a reason for hiding this comment

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

Hi Matthew, I am just doing a second review on this before we put this out on npm as a release. Just for my understanding: why is this here not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only way exceptionError can be truthy is if err is truthy. In this case stateManager.revertContracts() will have been called on ln 233. That method clears _touched

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks.

}

if (err && err.error !== ERROR.REVERT) {
Expand Down
82 changes: 39 additions & 43 deletions lib/runTx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ module.exports = function (opts, cb) {

if (tx.to.toString('hex') !== '') {
accounts.add(tx.to.toString('hex'))
self.stateManager.touched.push(tx.to)
Copy link
Member

Choose a reason for hiding this comment

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

Same for this?

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'm trying to remember; I think this one is just logically superfluous. Any operation that modifies the empty-ness of the to address will necessarily add that address to the touched set. Therefore there's no need to do it here as part of cache population.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, just seeing that the toAccount (also the newly created one from loadToAccount) is added in runCall.addTxValue() through stateManager.putAccount(). So this should be fine.

}

if (opts.populateCache === false) {
Expand Down Expand Up @@ -148,53 +147,50 @@ module.exports = function (opts, cb) {
}

results.amountSpent = results.gasUsed.mul(new BN(tx.gasPrice))
// refund the leftover gas amount
fromAccount.balance = new BN(tx.gasLimit).sub(results.gasUsed)
.mul(new BN(tx.gasPrice))
.add(new BN(fromAccount.balance))

self.stateManager.cache.put(tx.from, fromAccount)
self.stateManager.touched.push(tx.from)

var minerAccount = self.stateManager.cache.get(block.header.coinbase)
// add the amount spent on gas to the miner's account
minerAccount.balance = new BN(minerAccount.balance)
.add(results.amountSpent)

// save the miner's account
if (!(new BN(minerAccount.balance).isZero())) {
self.stateManager.cache.put(block.header.coinbase, minerAccount)

async.series([
updateFromAccount,
updateMinerAccount,
cleanupAccounts
], cb)

function updateFromAccount (next) {
// refund the leftover gas amount
var finalFromBalance = new BN(tx.gasLimit).sub(results.gasUsed)
.mul(new BN(tx.gasPrice))
.add(new BN(fromAccount.balance))
fromAccount.balance = finalFromBalance

self.stateManager.putAccountBalance(utils.toBuffer(tx.from), finalFromBalance, next)
Copy link
Member

Choose a reason for hiding this comment

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

Yes.

}

if (!results.vm.selfdestruct) {
results.vm.selfdestruct = {}
function updateMinerAccount (next) {
var minerAccount = self.stateManager.cache.get(block.header.coinbase)
// add the amount spent on gas to the miner's account
minerAccount.balance = new BN(minerAccount.balance)
.add(results.amountSpent)

// save the miner's account
if (!(new BN(minerAccount.balance).isZero())) {
self.stateManager.cache.put(block.header.coinbase, minerAccount)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason that this stays on acting on the cache directly and not switch to putAccount or putAccountBalance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I can think of. I was just refactoring and didn't change it. We should probably have a separate ticket to clean up all these direct cache writes

Copy link
Member

Choose a reason for hiding this comment

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

Yes, probably, have added a note to #268.

}

next(null)
}

var keys = Object.keys(results.vm.selfdestruct)

keys.forEach(function (s) {
self.stateManager.cache.del(Buffer.from(s, 'hex'))
})

// delete all touched accounts
var touched = self.stateManager.touched
async.forEach(touched, function (address, next) {
self.stateManager.accountIsEmpty(address, function (err, empty) {
if (err) {
next(err)
return
}

if (empty) {
self.stateManager.cache.del(address)
}
next(null)
function cleanupAccounts (next) {
if (!results.vm.selfdestruct) {
results.vm.selfdestruct = {}
}

var keys = Object.keys(results.vm.selfdestruct)

keys.forEach(function (s) {
self.stateManager.cache.del(Buffer.from(s, 'hex'))
})
},
function () {
self.stateManager.touched = []
cb()
})

self.stateManager.cleanupTouchedAccounts(next)
}
}
}

Expand Down
41 changes: 32 additions & 9 deletions lib/stateManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ function StateManager (opts) {
self.trie = trie
self._storageTries = {} // the storage trie cache
self.cache = new Cache(trie)
self.touched = []
self._touched = new Set()
}

var proto = StateManager.prototype
Expand All @@ -47,14 +47,13 @@ proto.exists = function (address, cb) {
}

// saves the account
proto._putAccount = function (address, account, cb) {
proto.putAccount = function (address, account, cb) {
var self = this
var addressHex = Buffer.from(address, 'hex')
// TODO: dont save newly created accounts that have no balance
// if (toAccount.balance.toString('hex') === '00') {
// if they have money or a non-zero nonce or code, then write to tree
self.cache.put(addressHex, account)
self.touched.push(address)
self.cache.put(address, account)
self._touched.add(address.toString('hex'))
// self.trie.put(addressHex, account.serialize(), cb)
cb()
}
Expand Down Expand Up @@ -82,7 +81,7 @@ proto.putAccountBalance = function (address, balance, cb) {
}

account.balance = balance
self._putAccount(address, account, cb)
self.putAccount(address, account, cb)
})
}

Expand All @@ -98,7 +97,7 @@ proto.putContractCode = function (address, value, cb) {
if (err) {
return cb(err)
}
self._putAccount(address, account, cb)
self.putAccount(address, account, cb)
})
})
}
Expand Down Expand Up @@ -180,8 +179,8 @@ proto.putContractStorage = function (address, key, value, cb) {
// update contract stateRoot
var contract = self.cache.get(address)
contract.stateRoot = storageTrie.root
self._putAccount(address, contract, cb)
self.touched.push(address)
self.putAccount(address, contract, cb)
self._touched.add(address.toString('hex'))
}
})
}
Expand All @@ -204,6 +203,7 @@ proto.commitContracts = function (cb) {
proto.revertContracts = function () {
var self = this
self._storageTries = {}
self._touched.clear()
}

//
Expand Down Expand Up @@ -325,3 +325,26 @@ proto.accountIsEmpty = function (address, cb) {
cb(null, account.nonce.toString('hex') === '' && account.balance.toString('hex') === '' && account.codeHash.toString('hex') === utils.SHA3_NULL_S)
})
}

proto.cleanupTouchedAccounts = function (cb) {
var self = this
var touchedArray = Array.from(self._touched)
async.forEach(touchedArray, function (addressHex, next) {
var address = Buffer.from(addressHex, 'hex')
self.accountIsEmpty(address, function (err, empty) {
if (err) {
next(err)
return
}

if (empty) {
self.cache.del(address)
}
next(null)
})
},
function () {
self._touched.clear()
cb()
})
}
Copy link
Member

Choose a reason for hiding this comment

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

Ok, this looks good.