Make API backwards compatible

[msieczko]

Context

Hi, together with @sz-piotr we've been working on measuring the performance of MPT with native Map used as its underlying storage as opposed to level. It is clear that MPT performs better when Map is used which can be seen in both our benchmark results and @ryanio's which were discussed in #113. Next we wanted to measure performance gains on the level of packages using MPT as a dependency.

Problem

We tried hooking up our fork of MPT (modified to use Map for storage) to Buidler EVM. Our fork is similar to what @ryanio did in #113, but we left the MPT interface async.

Since MPT's interface has been rewritten to use promises and is not backwards compatible with v3.0.0 we created wrappers that translate the old callback style to the new promise style. We then made appropriate changes in Buidler EVM and its dependencies relying on MPT to work with the new wrapped MPT.

Although all MPT's tests pass when ran using our backported interface, many Buidler EVM tests fail on this version of MPT. We rerun the Builder EVM tests against current version of MPT wrapped in the compatible interface (see changes in this PR) to check if they would work. The same tests failed, which may indicate that MPT v4 introduces braking changes other than the change of interface. The other option is that we messed sth up with the wrappers themselves.

Steps to reproduce

In order to test Buidler EVM on wrapped MPT v4 we needed to adjust package.json files and imports in:

• buidler-core package itself
• its @nomiclabs/ethereumjs-vm dependency
• its ethereumjs-block subdependency

We then published the updated dependency packages to a private Verdaccio npm registry and used it when installing dependencies in buidler monorepo. This way we were able to have wrapped MPT v4 installed in node_modules.

To reproduce this setup:

1. Install Verdaccio globally

npm install --global verdaccio

2. Clone ethereumjs-vm-benchmarks and checkout measure-buidler branch.

git clone https://github.com/EthWorks/ethereumjs-vm-benchmarks.git
cd ethereumjs-vm-benchmarks
git fetch
git checkout measure-buidler

3. Prepare and run Verdaccio

yarn verdaccio:prepare
yarn verdaccio:run

4. Clone our buidler fork and checkout mpt-v4

git clone https://github.com/EthWorks/buidler.git
cd buidler
git fetch
git checkout mpt-v4

5. Install dependencies using Verdaccio (see postinstall script)

npm install

6. Run buidler-core tests

cd packages/buidler-core
npm run test

Tests output: buidler-core tests on wrapped MPT v4 (level).txt

CC: @alcuadrado

[msieczko]

Following @alcuadrado advice, I run the wrapped MPT v4 (version from this PR) against v3.0.0 tests. This helped discover a bug in getRaw and putRaw wrapper implementation. These methods should always use the main DB class instance (stored in _mainDB field) regardless of checkpointing state. The methods were removed in v4 so a polyfill in wrappers was necessary. Along with the methods the tests for them were removed so we didn't notice the problem earlier.

I also added wrappers for some other methods to make the v3 tests pass. They were not wrapped before as they are not used by the packages of interest to us. These steps helped to reduce the number of errors in Buidler EVM tests from 237 down to 12, but there still is some compatibility problem.

Tests output: buidler-core tests on wrapped MPT v4 (level, fixed "raw" methods).txt

CC: @ryanio @holgerd77

[alcuadrado]

I'm glad you found this @msieczko!

Failures 1, 2, 3, 4, 9, 10, 11, and 12 are unrelated. To avoid them, you have to run npm run build before npm run test. This is not very documented, sorry about that.

The other failing tests are related to eth_call, which runs a tx and immediately reverts the state. This is implemented here.

My suspicion is that one of these methods broke:

• StateManager#getStateRoot
• StateManager#setStateRoot
• VM#runTx -- Lower suspicion, as eth_sendTransaction test aren't failing.

[ryanio]

@msieczko thanks so much for your debugging and investigation.

I'll add some more "breaking change" release notes based on your insights in this PR, and I will continue to update with any new merges.

[msieczko]

The remaining failing tests that @alcuadrado mentioned are in fact 2 tests that fail for the same reason. They both test a simple behaviour and fail at step 3:

1. Deploy a simple storage contract (see it's solidity code below).
2. Send eth_call to get the stored value and confirm that it's all zeros.
3. Send eth_sendTransaction with transaction modifying the stored value.
4. Send eth_call to get the stored value again and check that it has been changed.

pragma solidity 0.5.10;

contract Example {
    event StateModified(uint256 indexed _oldI, uint256 _newI);
    
    
    uint256 public i = 0;
    uint8 public j = 1;
    bytes32 h = "1234567890123456789012345678901234567890123456789012345678901234";
    
    function modifiesState(uint256 _i) payable public {
        emit StateModified(i, _i);
        i = _i;
    }
    
}

What's interesting is that if you omit the 2nd step, both tests pass. This means that eth_call somehow changes the state of the vm or the trie. We're trying to get to the bottom of this at the moment. @ryanio maybe you recognise any changes in the tree that may have caused this or have some idea that would help us debugging?

[msieczko]

Ok, I found another deviation in the wrappers which caused these 2 last tests to fail. It turns out that SecureTrie.copy disregards changes introduced during checkpoint whereas CheckpointTrie.copy by default does not. Is this inconsistency intentional?

I've also found that MPT v3 copy methods don't work properly in checkpoint phase, so I assume that dependant packages don't make use of them during checkpoint.

[ryanio]

@msieczko thanks for finding these inconsistencies. i'm not sure if they are deliberate or not. maybe we can add some failing test cases and resolve them based on the behavior we should expect to be right?

[msieczko]

@ryanio I didn't identify any bug in MPT v4 itself other than this one inconsistency which would cause problems during my integration attempts. I'd say that SecureTrie.copy should behave in the same way as CheckpointTrie.copy (by default) and include checkpoints. However this was not the case in v3 (see this test which is failing) so I guess that's why it's left this way in v4. On the other hand copy method in v3 did not work during checkpoint at all which is demonstrated by this test.

Anyway, packages depending on MPT assume that SecureTrie.copy does not include checkpoints, because this last update to the wrappers fixed the remaining Buidler EVM tests.