OffchainLabs / go-ethereum

GNU Lesser General Public License v3.0
54 stars 94 forks source link

use StateAtBlock and reference states when recreating missing state #277

Closed magicxyyz closed 5 months ago

magicxyyz commented 9 months ago

This PR is addressing following issues:

  1. When getting state for RPCs (in the upstream code), the state getter is called first and after that the state root is referenced in hashdb dirties cache if at all. It means that the state could potentially be garbage collected in another thread and further operations on state would fail. That is fine for normal archive node that keeps all the state and doesn't use dirties cache. It is also ok for a full node as we don't require it to keep all the state but only some recent, so RPCs are expected to fail when requesting too old state. However, that is an issue for "hybrid" node that persists only some states and keeps recent states in dirties cache - we expect it to always be able to process RPC for any state (to emulate normal archive node functionality).
  2. Previous implementation of recreating missing state (AdvanceStateUpToBlock), which we use for some RPCs (e.g. eth_call, etc_getBalance), accumulated changes to state in one StateDB object, what may cause some issues i.a. excessive memory usage by caches inside StateDB.

This PR:

  1. reorders referencing and getting state, adds referencing and adds a release method to state.StateDB
  2. changes the recreation of state to use eth.StateAtBlock which is upstream implementation for recreation of state for tracers and should be a more robust solution
magicxyyz commented 8 months ago

Also - I'd like to have a test for our partial-archive-node config that will try at least some of these APIs.

There already is a system test on nitro side, that runs this partial-archive-node, stops it, restarts it and then checks if eth_getBalance can be called for some blocks. https://github.com/OffchainLabs/nitro/blob/a40f8f1e9ba0975452d3dc1da602f4e20142b608/system_tests/recreatestate_rpc_test.go#L429-L429

If needed, I can add some more specific tests for the changes, also in geth repo.

magicxyyz commented 8 months ago

Also - I'd like to have a test for our partial-archive-node config that will try at least some of these APIs.

There already is a system test on nitro side, that runs this partial-archive-node, stops it, restarts it and then checks if eth_getBalance can be called for some blocks. https://github.com/OffchainLabs/nitro/blob/a40f8f1e9ba0975452d3dc1da602f4e20142b608/system_tests/recreatestate_rpc_test.go#L429-L429

If needed, I can add some more specific tests for the changes, also in geth rep

As suggested, I added a specific test on nitro side for getting state for RPCs: https://github.com/OffchainLabs/nitro/blob/bb5c908d7c16e103130e2d80c7f5fc01e4dbef2e/system_tests/recreatestate_rpc_test.go#L514C1-L557

magicxyyz commented 7 months ago

I am still doing some testing with nitro-testnode. I have to yet confirm that, but it seems that finalizers are not good solution.