Clean-up state-manager.touched #287

mattdean-digicatapult · 2018-03-12T09:25:22Z

Fixes #267

I'm looking to contribute on this project a bit, especially with the aim of cleaning up the StateManager interface, and thought I'd start here.

I've taken a different approach to that in the issue description so feel free to criticise if there was a particular reason for suggesting turning the StateManager into an EventEmitter. I just felt that would be externalising state that should instead be encapsulated in StateManager itself.

Instead the approach I've taken is to move all mutations of StateManager.touched into StateManager such that touched can be considered private. This involved adding a new method to the StateManager interface in order to tidy up touched accounts. I would have preferred to include this as part of a commit phase but couldn't find somewhere sensible to do so without changing a lot of code.

I've also had to include one usage of the private method StateManager._putAccount which I feel should probably be public in the long run anyway.

Finally some methods have been made asynchronous in order to allow using StateManager methods instead of directly manipulating StateManager.cache. This makes the diff look a little worse than it really is.

axic · 2018-03-12T10:09:39Z

lib/runCall.js

+    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)


The functions with an underscore would be internal functions of the StateManager and should not be called from outside. (It may be the case at the moment in some places but that should be fixed.)

Agreed. The only way of achieving this write in the current codebase is to either manipulate the cache directly (which I feel should also be a no-no) or to call _putAccount. My preference would be to make _putAccount public (rename to putAccount) but I didn't want to change the interface in this way without checking first. Would you be ok with this?

coveralls · 2018-03-14T11:53:53Z

Coverage increased (+0.07%) to 85.505% when pulling 5327e6f on mattdean-digicatapult:clean-up-statemanager-touched into 01b7f37 on ethereumjs:master.

jwasinger · 2018-04-09T03:18:12Z

@mattdean-digicatapult thank you for the PR and I am sorry that this has languished without a response (we are very busy!). I like the changes here and think they improve code readability.

I don't fully understand the solution described in #267 :

This should be done in a much cleaner manner, perhaps with an event emitted from StateManager for touching an account and subscribing to that event in runTx.

Otherwise, I feel like this PR addresses the issue well. Would like to here @axic and @holgerd77 's opinions before we merge this.

P.s. @mattdean-digicatapult please rebase this branch.

mattdean-digicatapult · 2018-04-09T08:35:22Z

Thanks. Rebased as requested

jwasinger · 2018-04-11T06:02:28Z

@mattdean-digicatapult Thanks. Looks like we are having some issues with the CircleCI job (I think it might have to do with your branch being located on your personal fork of ethereumjs-vm). We have another PR that updates the Circle config to the latest format. I would like to see if that helps with the issue at all.

If you could also please squash the two commits in this PR into one.

Thanks

jwasinger · 2018-04-11T20:55:07Z

Please rebase this again.

mattdean-digicatapult · 2018-04-16T12:34:15Z

Rebased, squashed and circle is now happy 😃

axic · 2018-04-18T11:38:20Z

@holgerd77 do you also want to have a look at this? Probably it didn't break anything, just to be sure before a release.

holgerd77

Generally really beautiful and clean PR, just have two or three questions for my own understanding.

holgerd77 · 2018-04-26T09:15:25Z

lib/runCode.js

@@ -248,7 +248,6 @@ module.exports = function (opts, cb) {
    if (results.exceptionError) {
      delete results.gasRefund
      delete results.selfdestruct
-      self.stateManager.touched = []


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?

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

Ok, thanks.

holgerd77 · 2018-04-26T09:15:47Z

lib/runTx.js

@@ -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)


Same for this?

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.

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.

holgerd77 · 2018-04-26T09:16:57Z

lib/runCall.js

+    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)


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

holgerd77 · 2018-04-26T09:18:57Z

lib/runTx.js

+          .add(new BN(fromAccount.balance))
+        fromAccount.balance = finalFromBalance
+
+        self.stateManager.putAccountBalance(utils.toBuffer(tx.from), finalFromBalance, next)


holgerd77 · 2018-04-26T09:21:37Z

lib/runTx.js

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


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

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

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

holgerd77 · 2018-04-26T09:24:59Z

lib/stateManager.js

+    self._touched.clear()
+    cb()
+  })
+}


Ok, this looks good.

Handle signatures for chainIds greater than 110

axic reviewed Mar 12, 2018

View reviewed changes

jwasinger requested review from holgerd77 and jwasinger April 9, 2018 03:02

jwasinger previously approved these changes Apr 9, 2018

View reviewed changes

mattdean-digicatapult dismissed jwasinger’s stale review via 66af9d3 April 9, 2018 08:34

mattdean-digicatapult force-pushed the clean-up-statemanager-touched branch from 62f0d19 to 66af9d3 Compare April 9, 2018 08:34

Clean-up state-manager.touched

5327e6f

mattdean-digicatapult force-pushed the clean-up-statemanager-touched branch from 66af9d3 to 5327e6f Compare April 16, 2018 08:08

jwasinger self-requested a review April 16, 2018 17:17

jwasinger approved these changes Apr 16, 2018

View reviewed changes

jwasinger merged commit 1cbba0a into ethereumjs:master Apr 16, 2018

holgerd77 mentioned this pull request Apr 25, 2018

New release v2.3.5 #296

Merged

holgerd77 reviewed Apr 26, 2018

View reviewed changes

evertonfraga added the package: vm label Feb 27, 2020

holgerd77 added a commit that referenced this pull request Mar 11, 2021

Merge pull request #287 from spalladino/handle-large-chainid-on-rpcsig

4ca587e

Handle signatures for chainIds greater than 110

This was referenced Aug 22, 2023

[Snyk] Upgrade sort-package-json from 1.57.0 to 2.5.1 Woodpile37/ethereumjs-monorepo#13

Merged

[Snyk] Upgrade sort-package-json from 1.57.0 to 2.5.1 Woodpile37/ethereumjs-monorepo#43

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean-up state-manager.touched #287

Clean-up state-manager.touched #287

mattdean-digicatapult commented Mar 12, 2018

axic Mar 12, 2018

mattdean-digicatapult Mar 12, 2018

coveralls commented Mar 14, 2018 •

edited

Loading

jwasinger commented Apr 9, 2018 •

edited

Loading

mattdean-digicatapult commented Apr 9, 2018

jwasinger commented Apr 11, 2018 •

edited

Loading

jwasinger commented Apr 11, 2018

mattdean-digicatapult commented Apr 16, 2018

axic commented Apr 18, 2018

holgerd77 left a comment

holgerd77 Apr 26, 2018

mattdean-digicatapult Apr 26, 2018

holgerd77 Apr 26, 2018

holgerd77 Apr 26, 2018

mattdean-digicatapult Apr 26, 2018

holgerd77 Apr 26, 2018

holgerd77 Apr 26, 2018

holgerd77 Apr 26, 2018

holgerd77 Apr 26, 2018

mattdean-digicatapult Apr 26, 2018

holgerd77 Apr 26, 2018

holgerd77 Apr 26, 2018

Clean-up state-manager.touched #287

Clean-up state-manager.touched #287

Conversation

mattdean-digicatapult commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 14, 2018 • edited Loading

jwasinger commented Apr 9, 2018 • edited Loading

mattdean-digicatapult commented Apr 9, 2018

jwasinger commented Apr 11, 2018 • edited Loading

jwasinger commented Apr 11, 2018

mattdean-digicatapult commented Apr 16, 2018

axic commented Apr 18, 2018

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 14, 2018 •

edited

Loading

jwasinger commented Apr 9, 2018 •

edited

Loading

jwasinger commented Apr 11, 2018 •

edited

Loading