fix(forking): fix snapshots for forking

[mikeseese]

This issue fixes #624 and fixes #611 with the following approach:

• Track what keys we've touched (put/delete) rather than what keys we've deleted (not sure if this was necessary, but it is a cleaner approach)
  • Track when we first touched these keys by block number and vice versa (block number -> which keys)
• Handle popBlock in ForkedBlockchain to forget that we touched respective keys in that block

A couple of side effects:

• ForkedStorageBaseTrie no longer overloads get with a block number (we use this.blockchain.blocks.last() to get the current block now)
• This also fixes Ensure ForkedBlockchain.prototype.getAccount works #623. I can separate this into another PR if you want

[coveralls]

Coverage decreased (-0.6%) to 82.133% when pulling e7e50b8 on fix/forking-snapshots into 95101d7 on develop.

[davidmurdoch]

Ganache currently accepts non-standard input as a "feature". For instance, you can pass in a Buffer in place of a hex address in many RPC methods and ganache will happily go about using it. The toLowerCase() introduced here changes that behavior.

This also causes other interesting changes, for example, passing something like null, or other non-string types, to eth_getBalance currently returns "0x", but with these changes it returns an error.

Also note that in any case where toLowerCase() is used on a hex string that will be converted to a Buffer, the casing doesn't matter, as Buffer.from("AA", "hex").equals(Buffer.from("aa", "hex")) === true because node's Buffer treats hex as case-insensitive.

I'm leaning towards reverting the broad toLowerCase() changes here, and only changing what is necessary.

[mikeseese]

The whole reason to doing the toLowerCase was I was under the impression Buffer.from("AA", "hex").equals(Buffer.from("aa", "hex")) was false. This is because I didn't add hex as the radix, so it's pretty cool that Buffer handles that properly

I can do what's only necessary then which is at the function level rather than at the interface level and only do it for the forked storage/code/account functions

[davidmurdoch]

I can do what's only necessary then which is at the function level rather than at the interface level and only do it for the forked storage/code/account functions

Sounds good!

[davidmurdoch]

I like the approach where the storage trie isn't directly block-number aware. Hopefully any issues I've highlighted won't necessitate bringing the blockNumber back here.

I had some questions and possible changes, but other than these, things are looking good!

[mikeseese]

@davidmurdoch jk, i still need to fix the toLowerCase thing

[mikeseese]

alright @davidmurdoch, I think this is ready to review now

[mikeseese]

@davidmurdoch since this one hasn't been finished merging, i pushed a small commit (and accompanying test) that fixes an issue where I also need to check if the key exists on the trie. I wouldn't quite say it was a regression for the case I was solving, but it could be a regression for some past-block lookups

[mikeseese]

I'll check into why these 2 forking tests are now failing, but this should still be good to review

[mikeseese]

alright @davidmurdoch, tests pass locally for me with the latest commit. i didnt need to rearchitect anything; I just changed ForkedStorageBaseTrie.delete to do a put(key, 0). This should be ready to review now

[BlinkyStitt]

Thank you!

[mikeseese]

@wysenynja I'll keep you up to date in this thread (and in the individual issue threads) when a release goes out with these fixes (probably a beta release)

[mikeseese]

This was released in https://github.com/trufflesuite/ganache-core/releases/tag/v2.12.2-beta.0

[mrice32]

@davidmurdoch is it possible that this PR re-broke #571 and caused #797.

[mikeseese]

FWIW forking (especially in it's current state) is a beast, so I wouldn't be surprised if this PR caused more issues.

However, I don't think it rebroke #571 as I believe I implemented specific test scenarios to pinpoint the causes of that issue (and others with similar symptoms). It's likely a different issue with the same symptom. It's very possible that @0xTimepunk's issue is even another