Fix forked transaction trace storage so it returns the correct data and doesn't modify the root trie #398

davidmurdoch · 2019-03-26T16:42:54Z

davidmurdoch · 2019-03-26T16:49:05Z

lib/utils/forkedblockchain.js

-
-      callback();
-    });
+    BlockchainDouble.prototype.initialize.call(self, accounts, callback);


Reason for change: we don't need to call patchVm since it already gets called within createVMFromStateTrie.

davidmurdoch · 2019-03-26T16:51:08Z

lib/utils/forkedblockchain.js

@@ -107,7 +101,7 @@ ForkedBlockchain.prototype.createGenesisBlock = function(callback) {

    // Update the relevant block numbers
    self.forkBlockNumber = self.options.fork_block_number = blockNumber;
-    self.stateTrie.forkBlockNumber = blockNumber;
+    self.stateTrie.forkBlockNumber = self.stateTrie.options.forkBlockNumber = blockNumber;


Reason for change: The stateTrie is now copyed instead of created via new ForkedStorageTrie (this aligns with how the merkle patricia trie does it internally). When copying we need to also copy the options object, so here I am making sure the forkBlockNumber is set on the object.

davidmurdoch · 2019-03-26T16:52:41Z

lib/utils/forkedblockchain.js

-    return callback(null, this.storageTrieCache[address]);
-  }
+ForkedBlockchain.prototype.getLookupStorageTrie = function(stateTrie, lookupAccount) {
+  lookupAccount = lookupAccount || this.getLookupAccount(stateTrie);


Reason for change: getLookupStorageTrie is usually called when the ephemeralVm is created; when that happens, we already have a lookupAccount fn generated, so we used that. There are other ways to call getLookupStorageTrie and those don't have (easy) access to a valid lookupAccount fn. So sometimes we have to create it.

davidmurdoch · 2019-03-26T16:53:07Z

lib/utils/forkedblockchain.js

+ForkedBlockchain.prototype.getLookupStorageTrie = function(stateTrie, lookupAccount) {
+  lookupAccount = lookupAccount || this.getLookupAccount(stateTrie);
+  return (address, callback) => {
+    const storageTrie = stateTrie.copy();


Reason for change: DRY and in-line with the way the MPT lib does it.

davidmurdoch · 2019-03-26T16:53:27Z

lib/utils/forkedblockchain.js

+  lookupAccount = lookupAccount || this.getLookupAccount(stateTrie);
+  return (address, callback) => {
+    const storageTrie = stateTrie.copy();
+    storageTrie.address = address;


Reason for change: ForkedStorageTrie needs the address.

davidmurdoch · 2019-03-26T16:54:46Z

lib/utils/forkedblockchain.js

@@ -161,7 +145,8 @@ ForkedBlockchain.prototype.isFallbackBlock = function(value, callback) {
 };

 ForkedBlockchain.prototype.isBlockHash = function(value) {
-  return typeof value === "string" && value.indexOf("0x") === 0 && value.length > 42;
+  const isHash = typeof value === "string" && value.indexOf("0x") === 0 && value.length > 42;
+  return isHash || (Buffer.isBuffer(value) && value.byteLength > 20);


Reason for change: Sometimes the value is a Buffer and we obviously should check the buffer to see if it big enough to be a blockhash.

davidmurdoch · 2019-03-26T16:55:06Z

lib/utils/forkedblockchain.js

@@ -171,6 +156,10 @@ ForkedBlockchain.prototype.isFallbackBlockHash = function(value, callback) {
    return callback(null, false);
  }

+  if (Buffer.isBuffer(value)) {


Reason for change: blockHashes.get, below, only allows hex strings.

davidmurdoch · 2019-03-26T16:56:02Z

lib/utils/forkedblockchain.js

-  var self = this;
-
-  this.isFallbackBlockHash(number, function(err, isFallbackBlockHash) {
+  let checkFn;


Reason for change: simplifying some of the code, which also eliminates from duplicate checks.

davidmurdoch · 2019-03-26T16:57:11Z

lib/utils/forkedblockchain.js

  });
 };

 ForkedBlockchain.prototype.getStorage = function(address, key, number, callback) {
-  this.lookupStorageTrie(address, function(err, trie) {
+  this.getLookupStorageTrie(this.stateTrie)(address, (err, trie) => {


Reason for change: Make sure we use the new and improved storage trie instead of the old and busted one (which has been removed)

davidmurdoch · 2019-03-26T16:58:21Z

lib/utils/forkedblockchain.js

-  return trie;
-};
-
-ForkedBlockchain.prototype.lookupStorageTrie = function(address, callback) {


Reason for change: we have a better one now. And the cache is mostly unnecessary as it complicates things (like when the stateTrie root is from a previous block but the cache contains data from a newer block) and the cache is already properly handled in the VM, which most of the calls to this fn come from.

davidmurdoch · 2019-03-26T16:59:10Z

lib/utils/forkedblockchain.js

@@ -327,6 +303,28 @@ ForkedBlockchain.prototype.getCode = function(address, number, callback) {
  });
 };

+ForkedBlockchain.prototype.getLookupAccount = function(trie) {


Reason for change: we needed to be able to lookup an account based on a trie, not a block number.

davidmurdoch · 2019-03-26T16:59:28Z

lib/utils/forkedblockchain.js

@@ -640,33 +638,4 @@ ForkedBlockchain.prototype.getBlockLogs = function(number, callback) {
  });
 };

-ForkedBlockchain.prototype._checkpointTrie = function() {


Reason for change: These don't get used.

davidmurdoch · 2019-03-26T17:00:56Z

lib/utils/forkedstoragetrie.js

@@ -70,12 +58,14 @@ ForkedStorageTrie.prototype.get = function(key, blockNumber, callback) {
          callback(null, account.serialize());
        });
      } else {
+        if (to.number(blockNumber) > to.number(self.forkBlockNumber)) {


Reason for change: Whenever we go to the main chain we shouldn't use a block number after we forked because that data is not valid for us.

davidmurdoch · 2019-03-26T17:01:12Z

lib/utils/forkedstoragetrie.js

-// Note: For some reason, naming this function checkpoint()
-// -- overriding the default function -- prevents it from
-// being called.
-ForkedStorageTrie.prototype.customCheckpoint = function() {


Reason for change: These were never used.

davidmurdoch · 2019-03-26T17:03:44Z

lib/utils/forkedstoragetrie.js

 var utils = require("ethereumjs-util");
 var inherits = require("util").inherits;
 var Web3 = require("web3");
 var to = require("./to.js");

-inherits(ForkedStorageTrie, MerklePatriciaTree);
+inherits(ForkedStorageBaseTrie, BaseTrie);


Reason for change:

Most of these changes are just code reorg. We now have a "Base" trie and our normal trie. This is just to align with the MPT implementation. I wanted to use the copy mechanism, but couldn't do so without this small refactor... because of reasons (things crashed).

We don't use to.hex since we check/know the type is buffer and so we can reduce the overhead by converting to hex string directly.

nicholasjpaterno

I haven't quite had the bandwidth to give this the type of review it may require, but I have read through a few times and everything seems to LGTM! Happy to approve for now, we can talk more if you're not feeling 💯 about any part of this otherwise !

davidmurdoch self-assigned this Mar 26, 2019

davidmurdoch added bug priority2 ⚠️ labels Mar 26, 2019

davidmurdoch commented Mar 26, 2019

View reviewed changes

nicholasjpaterno and others added 12 commits March 26, 2019 13:05

Fix forkedblockchain.js, correctly handle buffer encoded hash values

2c2814c

Add test for forked debugging

a874c78

Fix forkedblockchain.js, convert buffer to hex encoded string

f916070

We don't use to.hex since we check/know the type is buffer and so we can reduce the overhead by converting to hex string directly.

add valid tests and remove the other ones for now

e018b24

only 1 test failure now

99f11ed

fix the last failure

8e14303

dead code removal

6ef8f5b

.

ed84aa7

refactor some things

c1fccb4

cleanup

bcebac2

cleanup

55494b2

add todo

73e5982

davidmurdoch force-pushed the fix-forked-transaction-trace-storage branch from 3371016 to 73e5982 Compare March 26, 2019 17:07

davidmurdoch and others added 2 commits March 26, 2019 13:09

simplify

11a0945

Merge branch 'develop' into fix-forked-transaction-trace-storage

26e6d43

davidmurdoch requested a review from nicholasjpaterno March 26, 2019 18:16

nicholasjpaterno approved these changes Mar 27, 2019

View reviewed changes

davidmurdoch merged commit df0578a into develop Apr 5, 2019

davidmurdoch deleted the fix-forked-transaction-trace-storage branch April 9, 2019 19:02

This was referenced Apr 16, 2019

Bump ganache-cli from 6.1.6 to 6.4.3 0xProject/0x-starter-project#132

Closed

chore(deps-dev): bump ganache-cli from 6.2.3 to 6.4.3 marcoonroad/instrumental#102

Closed

Bump ganache-cli from 6.1.8 to 6.4.3 dekz/defi-test-stack#64

Closed

dependabot-preview bot mentioned this pull request Apr 26, 2019

build(deps-dev): bump ganache-cli from 6.4.2 to 6.4.3 AztecProtocol/aztec-v1#104

Merged

This was referenced May 28, 2019

Bump ganache-cli from 6.0.3 to 6.4.3 yezooz/ticket-contract#4

Closed

Bump ganache-cli from 6.2.5 to 6.4.3 OpenZeppelin/openzeppelin-test-helpers#52

Closed

Bump ganache-cli from 6.4.1 to 6.4.3 OpenZeppelin/openzeppelin-contracts#1778

Closed

dependabot-preview bot mentioned this pull request Jun 10, 2019

Bump ganache-cli from 6.4.1 to 6.4.3 SAMHEO/openzeppelin-solidity#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix forked transaction trace storage so it returns the correct data and doesn't modify the root trie #398

Fix forked transaction trace storage so it returns the correct data and doesn't modify the root trie #398

davidmurdoch commented Mar 26, 2019 •

edited

davidmurdoch Mar 26, 2019 •

edited

davidmurdoch Mar 26, 2019

davidmurdoch Mar 26, 2019

davidmurdoch Mar 26, 2019

davidmurdoch Mar 26, 2019 •

edited

davidmurdoch Mar 26, 2019

davidmurdoch Mar 26, 2019

davidmurdoch Mar 26, 2019

davidmurdoch Mar 26, 2019

davidmurdoch Mar 26, 2019

davidmurdoch Mar 26, 2019

davidmurdoch Mar 26, 2019

davidmurdoch Mar 26, 2019

davidmurdoch Mar 26, 2019 •

edited

davidmurdoch Mar 26, 2019

nicholasjpaterno left a comment

Fix forked transaction trace storage so it returns the correct data and doesn't modify the root trie #398

Fix forked transaction trace storage so it returns the correct data and doesn't modify the root trie #398

Conversation

davidmurdoch commented Mar 26, 2019 • edited

davidmurdoch Mar 26, 2019 • edited

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

davidmurdoch Mar 26, 2019 • edited

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

davidmurdoch Mar 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholasjpaterno left a comment

Choose a reason for hiding this comment

davidmurdoch commented Mar 26, 2019 •

edited

davidmurdoch Mar 26, 2019 •

edited

davidmurdoch Mar 26, 2019 •

edited

davidmurdoch Mar 26, 2019 •

edited