ethereumjs-monorepo icon indicating copy to clipboard operation
ethereumjs-monorepo copied to clipboard

Ethers forked state provider

Open ScottyPoi opened this issue 2 years ago • 12 comments

Implementing idea from @acolytec3 in #2438

When we introduced the new ethersStateManager, it allows for essentially forking mainnet (or any chain) and running transactions locally against the state of that chain. One can chain transactions together and see the effect of multiple transactions on the state of the chain without actually submitting a real transaction. It would be nice to implement a new ethers provider interface that extends the JsonRPCProvider by wrapping it with the ethersStateManager and proxying getBalance/getCode/getStorageAt state related calls to the ethersStateManager so that developers can make simulated testing against forked chain state much easier since they will be able to just use ethers without directly having to interact with the stateManager or other lower level primitives in the @ethereumjs stack.

  1. Creats EthersForkedStateProvider class in /statemanager/ethersForkedStateProvider.ts
  • EthersForkedStateProvider extends ethers.providers.JsonRpcProvider
  • Constructor takes a provider or provider_url as the only parameter
  • Class has private element this.ethersStateManager
  • Implements interface for ethers.providers.JsonRpcProvider calls:
    • getBalance
    • getCode
    • getStorageAt
    • getTransactionCount
  1. Includes fix for ethersStateManager.putContractStorage
  • Details in comments.
    • Passing a buffer of zeros, or an empty buffer, should delete a value from the storage by replacing it with empty buffer.
  1. Adds tests for new EthersForkedStateProvider class

ScottyPoi avatar Dec 03 '22 01:12 ScottyPoi

Codecov Report

Merging #2442 (df9f341) into master (52dbfc2) will decrease coverage by 0.56%. The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 90.10% <ø> (ø)
blockchain 90.02% <ø> (ø)
client 87.59% <ø> (ø)
common 95.82% <ø> (ø)
devp2p ?
ethash ∅ <ø> (∅)
evm 83.35% <ø> (ø)
rlp ∅ <ø> (∅)
statemanager 89.63% <100.00%> (+0.01%) :arrow_up:
trie ?
tx 93.71% <ø> (ø)
util 84.48% <ø> (ø)
vm 83.68% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

codecov[bot] avatar Dec 03 '22 01:12 codecov[bot]

Great thanks for the feedback. Got rid of the fallback provider stuff, and the unnecessary getAccount.

In trying to write tests for the EthersStateManager fixes, i encountered some weirdness that was (i think) giving us some false positives in the test.

~~TLDR is that while i haven't added any tests, the first change broke some tests, and the second change fixed those tests.~~

TLDR 2 Only the one change was necessary to EthersStateManager. Writing a buffer full of zeros to storage should instead delete the storage value by writing an empty buffer.

 async putContractStorage(address: Address, key: Buffer, value: Buffer): Promise<void> {
    let accountStorage = this.storageCache.get(address.toString())
    if (accountStorage === undefined) {
      this.storageCache.set(address.toString(), new Map<string, Buffer>())
      accountStorage = this.storageCache.get(address.toString())
    }
    accountStorage?.set(key.toString('hex'), value)
  }

For unknown reasons, the conditional as written did not actually kick in if the first this.storageCache.get() returned undefined. So the empty Map() would not be set, and accountStorage would remain undefined. The ? at the end allowed this to happen without error. This method was also missing the function of deleting a value from the Map when a buffer full of zeros is passed as the value parameter.

  • If it is empty or filled with zeros, deletes the value.

By rewriting like this, we add the function of actually deleting the value, while also removing the potential for "nothing at all" to happen.

  async putContractStorage(address: Address, key: Buffer, value: Buffer): Promise<void> {
    const accountStorage = this.storageCache.get(address.toString()) ?? new Map<string, Buffer>()

    if (value.length === 0 || value.equals(Buffer.alloc(value.length).fill(0))) {
      accountStorage.set(key.toString('hex'), Buffer.from([]))
    } else {
      accountStorage.set(key.toString('hex'), value)
    }
    this.storageCache.set(address.toString(), accountStorage)
  }

~~Then because of an unknown fallback mechanism happening in EthersStateManager.getContractStorage(), we~~ ~~would end up with the correct return value (an empty buffer) anyway, further masking the issue. The way it was~~ ~~written (below), it should return nothing (undefined) unless both accountStorage and storage are found. But it~~ ~~would not return undefined... it would return an empty buffer. Which is what it should return. but where is~~ ~~that empty buffer coming from?~~

~~Making that change caused this test to fail:~~ ~~```ts~~ ~~// Verify that provider is not called~~ ~~;(state as any).getAccountFromProvider = function () {~~ ~~throw new Error('should not have called this!')~~ ~~}~~

~~Which was then fixed by adding the missing return value (...} else {return Buffer.from([])) to getContractStorage:~~ ~~async getContractStorage(address: Address, key: Buffer): Promise<Buffer> {~~ ~~// Check storage slot in cache~~ ~~const accountStorage: Map<string, Buffer> | undefined = this.storageCache.get(~~ ~~address.toString()~~ ~~)~~ ~~let storage: Buffer | string | undefined~~ ~~if (accountStorage !== undefined) {~~ ~~storage = accountStorage.get(key.toString('hex'))~~ ~~if (storage !== undefined) {~~ ~~return storage~~ ~~} else {~~ ~~return Buffer.from([])~~ ~~}~~ ~~}~~

ScottyPoi avatar Dec 08 '22 21:12 ScottyPoi

So I think I misread your original comment and I think the short answer to your question is that the reason the correct empty buffer is being returned is that if a call to getContractStorage doesn't find the storage value in the local cache, it always calls out to the provider and retrieves a value from there. If the provider returns undefined, we create an empty buffer at that point and return it. So, I think we need to revert your change in getContractStorage.

acolytec3 avatar Dec 08 '22 21:12 acolytec3

If the provider returns undefined, we create an empty buffer at that point and return it. So, I think we need to revert your change in getContractStorage.

~~Where does that happen?~~ wow...nevermind... :facepalm:

ScottyPoi avatar Dec 08 '22 23:12 ScottyPoi

The tests I've written do not test the "blockTag" parameter for getBalance, getStorageAt, and getTransactionCount, and I'm having wondering what you think the best approach to that might be?

ScottyPoi avatar Dec 13 '22 01:12 ScottyPoi

The tests I've written do not test the "blockTag" parameter for getBalance, getStorageAt, and getTransactionCount, and I'm having wondering what you think the best approach to that might be?

I wouldn't lose any sleep over that. I covered all of that in the original ethersStateManager tests and I don't see anything new you're doing with that parameter that should require us to test it again.

acolytec3 avatar Dec 13 '22 01:12 acolytec3

You know what just occurred to me that may be missing from this whole implementation is that we don't have a way to to actually simulate the transactions without the dapp developer actually having to implement and run our VM. The whole idea here is to be able to simulate and run transactions just from this provider and I totally missed that when I was sketching out the initial solution. As such, we're also going to have to override provider.sendTransaction.

In thinking about it, the implementation should be something like this:

  1. We need to add a private vm property to the provider and instantiate it using the ethersStateManager property.
  2. Override sendTransaction as follows:
  • Deserialize the signed transaction parameter
  • Call this.vm.runTx(decodedTransaction)
  • Return the result in the format provided

If I'm thinking about things correctly, since the private vm will run the transactions using the ethersStateManager, the state updated applied by running the transaction should magically be updated so if you call the provider.getBalance method after running a transaction, you should see the expected balance net of transaction fees. There's probably some edge case I'm not contemplating but that seems like it should work as roughly described above.

A fruitful set of tests would be to run a couple of transactions and then verify that the post account balances match up. I can probably help devise a contract with some storage updates as well if needed to verify that everything really works properly.

acolytec3 avatar Dec 13 '22 02:12 acolytec3

A fruitful set of tests would be to run a couple of transactions and then verify that the post account balances match up. I can probably help devise a contract with some storage updates as well if needed to verify that everything really works properly.

Thanks. I'll get it going and let you know where I need help.

ScottyPoi avatar Dec 14 '22 20:12 ScottyPoi

We need to add a private vm property to the provider and instantiate it using the ethersStateManager property.

Is there a good way to instantiate a VM in a constructor? since VM.create( ) is async.

client/vmexecution does it like this:

    this.vm = new (VM as any)({
      stateManager: this.ethersStateManager,
    }) as VM

I think that should be fine. The VM.create is async because it initializes an internal blockchain that uses a DB and is therefore async to start up but we don't need the blockchain or DB for this.

ScottyPoi avatar Dec 14 '22 21:12 ScottyPoi

If I'm thinking about things correctly, since the private vm will run the transactions using the ethersStateManager, the state updated applied by running the transaction should magically be updated so if you call the provider.getBalance method after running a transaction, you should see the expected balance net of transaction fees

Yes, this is working as expected.
I have to figure out how to turn our TxResult from vm.runTx() into the TransactionResponse interface that the ethers provider expects to return

ScottyPoi avatar Dec 14 '22 23:12 ScottyPoi

This is super useful! I need this as well gonna implement this for my viem state manager

roninjin10 avatar Nov 21 '23 18:11 roninjin10

Oh, this got a bit forgotten. 😳

I have to admit that I only now (better late than never) getting the whole idea of this. Yes, this is really great!

So how do we want to proceed?

My biggest fear here is the "yet another package to maintain" thing. Would it be a way that we integrate this into StateManager instead and then live with an optional Ethers dependency, as we do for EthersStateManager?

Then we can do everything one level smaller, do not need to setup an own README, tests, CI, and all this stuff.

WDYGT?

holgerd77 avatar Nov 23 '23 11:11 holgerd77