Remove vm accesses to stateManager trie and cache #376

mattdean-digicatapult · 2018-10-31T13:09:43Z

This is part of the broader discussion in #268 and #309 so readers should probably start there.

This PR removes all direct accesses, from the VM functions, to the cache and trie properties of stateManager. This is essentially done by replacing calls to these properties to the equivalent stateManager functions.

There are potentially two more controversial change here. The first is the removal of the loadCompiled method on the VM. This method is unused internally and undocumented. It is problematic to keep this function as it interferes with checkpoint/commit structure used in stateManager. My assumption is that if someone desires this functionality they would interact with the stateManager directly prior to instantiating the vm. The second is the change of the step event which now exposes the stateManager instance rather than the cache. This is essentially a breaking change as the event object structure is documented.

holgerd77 · 2018-10-31T17:24:34Z

Hi Matthew, cool, I did a complex (or at least broadly scoped) API doc refactoring PR merge #377 in between and also merged the StateManager work of @danjm #375. Can you do a careful rebase on top of this?

Then I think we get your stuff merged relatively quickly.

Cheers, Holger (relaxing a bit at the hotel :-))

mattdean-digicatapult · 2018-11-01T10:53:11Z

I've rebased and updated the docs to include the change to the step event format. Hopefully I've done this right 🤞

coveralls · 2018-11-01T10:57:11Z

Coverage increased (+0.2%) to 90.62% when pulling d9cca33 on remove-vm-accesses-to-statemanager-trie-cache into 7390207 on master.

mattdean-digicatapult · 2018-11-01T11:51:59Z

Hold off reviewing this for now as it's causing extra failing blockchain tests. Will investigate

holgerd77 · 2018-11-02T17:28:19Z

Rebased this.

holgerd77 · 2018-11-02T17:33:54Z

lib/index.js

+    var trie = opts.state || new Trie()
+    if (opts.activatePrecompiles) {
+      trie = new Trie()
+      for (var i = 1; i <= 8; i++) {


Totally irritated, was this one precompile forgotten before and this is an implicit fix (you are iterating up to 8 and not to 7)?

looks that way...

holgerd77 · 2018-11-02T17:51:00Z

Additionally failing tests:

OOGStateCopyContainingDeletedContract, 542, 544
suicideCoinbase, 562, 564
suicideStorageCheck, 567, 568
suicideStorageCheckVCreate, 571, 572
RecallSuicidedContractInOneBlock, 775, 776

holgerd77 · 2018-11-02T18:21:13Z

lib/runBlock.js

@@ -217,13 +227,11 @@ module.exports = function (opts, cb) {
          if (ethUtil.bufferToInt(block.header.gasUsed) !== Number(gasUsed)) {
            err = new Error((err || '') + 'invalid gasUsed ')
          }
-          if (self.stateManager.trie.root.toString('hex') !== block.header.stateRoot.toString('hex')) {
+          if (stateRoot.toString('hex') !== block.header.stateRoot.toString('hex')) {


Extra failures are happening here, not sure why yet, probably you'll get this faster than me.

When I run node ./tests/tester.js -b --file='suicideStorageCheck', looking at the first test and and add console output for the first and second state root value, I get:

211aca80b1d1084fd9a18fda2afbc5bdf3866d06c5321660e105546e79fded8d 334d8d12b70ab372703f37bf6c9fed76c7c61ba1f131517123d5d440776d825a

On the old version I get both:

334d8d12b70ab372703f37bf6c9fed76c7c61ba1f131517123d5d440776d825a 334d8d12b70ab372703f37bf6c9fed76c7c61ba1f131517123d5d440776d825a

mattdean-digicatapult · 2018-11-05T09:25:02Z

Right we're back to 50 failing blockchain tests. I'm not quite happy that I understand quite why what I've changed works, but I have an inkling. I'd recommend we hold off of merging until I've done a bit more digging

holgerd77 · 2018-11-05T09:31:19Z

Ok, no rush here.

mattdean-digicatapult · 2018-11-05T14:20:10Z

So after a bit of investigating, I found that the changes introduced in #375 were not particularly compatible with the work I've been doing on #309. Essentially the issue is that the cache doesn't handle checkpoints, in my mind, correctly when it comes to deletes. My expectation is that the cache should, in the case of a checkpoint revert, restore the state of deletes at the time of the corresponding checkpoint. This was not happening, but by assuming that deletes only occur at particular times things worked out. What broke is that the cache needs to flush at the end of the transaction in order to clear touched accounts correctly. These changes prevented that.

My additional changes make it so that the cache will, on a revert, restore the original delete set. This is done in much the same way that the modified accounts set is maintained, i.e. on the cache value. This removes the _getAccountPure method on stateManager as it no longer matters if the empty account is added to the cache during the accountIsEmpty call. I've also made clearer the logic in stateManager to flush the cache when the checkpoint stack reaches zero. If anyone doesn't like this logic my alternative would be to add a flush method to stateManager but I don't feel this is necessary as we only need to flush when leaving a user initiated vm method.

This has made this PR, once again, larger than I would like. The alternative would be to split this up to first change the cache behaviour and then complete this PR with it's original intention.

holgerd77 · 2018-11-06T15:01:49Z

I now merged the CREATE2 work from @rmeissner, if you want to do one last rebase I think we can get this merged fairly quickly.

mattdean-digicatapult · 2018-11-06T16:52:11Z

Rebased as requested

holgerd77

Ok, looks good, thanks again Matthew!

holgerd77 · 2018-11-06T22:34:21Z

lib/cache.js

@@ -6,13 +6,12 @@ const async = require('async')
 var Cache = module.exports = function (trie) {
  this._cache = Tree()
  this._checkpoints = []
-  this._deletes = []


This refactoring seems to make very much sense, to remove this separate delete stack and put it in the values itself.

holgerd77 · 2018-11-06T22:35:47Z

lib/opFns.js

+    runState.stateManager.putAccount(runState.address, runState.contract, function (err) {
+      if (err) return cb(err)
+      runState._vm.runCall(callOptions, parseCallResults)
+    })


holgerd77 · 2018-11-06T22:37:09Z

lib/runBlock.js

+        // credit all block rewards
+        if (generateStateRoot) {
+          block.header.stateRoot = stateRoot
+        }


holgerd77 · 2018-11-06T22:39:49Z

lib/runCode.js

       * @property {Account} account the [`Account`](https://github.com/ethereum/ethereumjs-account) which owns the code running
       * @property {Buffer} address the address of the `account`
       * @property {Number} depth the current number of calls deep the contract is
       * @property {Buffer} memory the memory of the VM as a `buffer`
-       * @property {FunctionalRedBlackTree} cache the account cache. Contains all the accounts loaded from the trie. It is an instance of [functional red black tree](https://www.npmjs.com/package/functional-red-black-tree)
+       * @property {StateManager} storageManager a state manager instance (EXPERIMENTAL - unstable API)


Good that this aligned documentation is already used and further evolved and corrected, really glad that I once went through this hazzle, even if there might be still some glizzles every here and there to be corrected.

holgerd77 · 2018-11-06T22:40:12Z

lib/runTx.js

+      self.stateManager.commit(function (err) {
+        cb(err, results)
+      })
+    }


holgerd77 · 2018-11-06T22:40:22Z

lib/runTx.js

-          self.stateManager.cache.put(block.header.coinbase, minerAccount)
+          self.stateManager.putAccount(block.header.coinbase, minerAccount, next)
+        } else {
+          next()


holgerd77 · 2018-11-06T22:41:15Z

lib/runTx.js

+
+        function cleanTouched (done) {
+          self.stateManager.cleanupTouchedAccounts(done)
+        }


holgerd77 · 2018-11-06T22:44:28Z

lib/stateManager.js

+    })
+  })
+}
+


This is evolving into a really clean and easy-to-grasp well-named API, nice! 👍

holgerd77 · 2018-11-06T22:51:26Z

Have re-read your initial comments, think changes to loadCompiled and step event should be unproblematic.

mattdean-digicatapult requested a review from holgerd77 October 31, 2018 13:09

mattdean-digicatapult force-pushed the remove-vm-accesses-to-statemanager-trie-cache branch from b08d5cb to 26e6541 Compare November 1, 2018 10:51

holgerd77 force-pushed the remove-vm-accesses-to-statemanager-trie-cache branch from 26e6541 to d759d35 Compare November 2, 2018 17:26

holgerd77 reviewed Nov 2, 2018

View reviewed changes

mattdean-digicatapult force-pushed the remove-vm-accesses-to-statemanager-trie-cache branch from d759d35 to 80c1c34 Compare November 5, 2018 09:09

holgerd77 mentioned this pull request Nov 5, 2018

Typescript rewrite ethereumjs/rlp#37

Merged

10 tasks

mattdean-digicatapult force-pushed the remove-vm-accesses-to-statemanager-trie-cache branch 2 times, most recently from f3eef12 to 86d2b05 Compare November 5, 2018 14:05

mattdean-digicatapult mentioned this pull request Nov 5, 2018

Fix failing blockchain test GasLimitHigherThan2p63m1 #380

Merged

mattdean-digicatapult force-pushed the remove-vm-accesses-to-statemanager-trie-cache branch from 86d2b05 to c399832 Compare November 6, 2018 12:50

Remove vm accesses to stateManager trie and cache

d9cca33

mattdean-digicatapult force-pushed the remove-vm-accesses-to-statemanager-trie-cache branch from c399832 to d9cca33 Compare November 6, 2018 16:34

holgerd77 approved these changes Nov 6, 2018

View reviewed changes

holgerd77 merged commit 84db45e into master Nov 6, 2018

holgerd77 deleted the remove-vm-accesses-to-statemanager-trie-cache branch November 6, 2018 22:46

mattdean-digicatapult mentioned this pull request Nov 7, 2018

Make stateManager cache and trie private #385

Merged

davidmurdoch mentioned this pull request Dec 21, 2018

opts.state and opts.activatePrecompiles are not compatible #411

Closed

evertonfraga added the package: vm label Feb 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove vm accesses to stateManager trie and cache #376

Remove vm accesses to stateManager trie and cache #376

mattdean-digicatapult commented Oct 31, 2018

holgerd77 commented Oct 31, 2018

mattdean-digicatapult commented Nov 1, 2018

coveralls commented Nov 1, 2018 •

edited

Loading

mattdean-digicatapult commented Nov 1, 2018

holgerd77 commented Nov 2, 2018

holgerd77 Nov 2, 2018

mattdean-digicatapult Nov 5, 2018

holgerd77 commented Nov 2, 2018

holgerd77 Nov 2, 2018

mattdean-digicatapult commented Nov 5, 2018

holgerd77 commented Nov 5, 2018

mattdean-digicatapult commented Nov 5, 2018

holgerd77 commented Nov 6, 2018

mattdean-digicatapult commented Nov 6, 2018

holgerd77 left a comment

holgerd77 Nov 6, 2018

holgerd77 Nov 6, 2018

holgerd77 Nov 6, 2018

holgerd77 Nov 6, 2018

holgerd77 Nov 6, 2018

holgerd77 Nov 6, 2018

holgerd77 Nov 6, 2018

holgerd77 Nov 6, 2018

holgerd77 commented Nov 6, 2018

Remove vm accesses to stateManager trie and cache #376

Remove vm accesses to stateManager trie and cache #376

Conversation

mattdean-digicatapult commented Oct 31, 2018

holgerd77 commented Oct 31, 2018

mattdean-digicatapult commented Nov 1, 2018

coveralls commented Nov 1, 2018 • edited Loading

mattdean-digicatapult commented Nov 1, 2018

holgerd77 commented Nov 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Nov 2, 2018

Choose a reason for hiding this comment

mattdean-digicatapult commented Nov 5, 2018

holgerd77 commented Nov 5, 2018

mattdean-digicatapult commented Nov 5, 2018

holgerd77 commented Nov 6, 2018

mattdean-digicatapult commented Nov 6, 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

holgerd77 commented Nov 6, 2018

coveralls commented Nov 1, 2018 •

edited

Loading