perf(saexec): Remove TrieDB memory leak

[alarso16]

Currently, SAE follows this pattern:
If we call triedb.Commit() every N blocks:

State 0 will be on disk
States [1, N] will be generated in memory
State N will be committed, moving all dirty nodes at that root from memory onto disk
All outdated nodes in [1, N) will remain in the dirty cache

Although not a correctness bug, keeping these tries is generally unnecessary and leads to a memory leak.

The expected functionality should be:

• For validators: keep the minimum states around to guarantee that all consensus-necessary states are accessible
• For API nodes: keep all states on disk.

Now, all nodes will default to NOT storing all states. API nodes must set the necessary config

[alarso16]

This is the same logic as coreth's core/state_manager.go logic, but simpler. There is a "we're not ready to commit yet, but we have a huge amount of state in memory, let's offload some into the database" function, but I don't think it's necessary until proven so

[alarso16]

Is there a better pattern for this? I could avoid the defer, but we should report all these errors, right?

[ARR4N]

Thank you for digging deeply enough to figure out that this would be a problem, and for fixing it!

[ARR4N]

If you define numBlocks as a uint64 then the linter won't complain:

numBlocks := uint64(saedb.StateHistory) + 10

[alarso16]

Would it be better to just make StateHistory a uint64? the buffer takes an int for some reason...

[ARR4N]

Tests that share logic and only differ in the declaration of parameters are easier to reason about. It's also unnecessary to check that a non-nil StateDB is returned when there's a nil error because that's implied by all other usage.

	t.Run("access state", func(t *testing.T) {
		for _, b := range chain.AllBlocks() {
			var want testerr.Want

			switch num := b.NumberU64(); {
			case num > numBlocks-saedb.StateHistory:
				// Still referenced
			case saedb.ShouldCommitTrieDB(num):
				// On disk
			default:
				want = testerr.As(func(got *trie.MissingNodeError) string {
					if r := b.PostExecutionStateRoot(); got.NodeHash != r {
						return fmt.Sprintf("%T for hash %#x", got, r)
					}
					return ""
				})
			}

			_, err := e.StateDB(b.PostExecutionStateRoot())
			if diff := testerr.Diff(err, want); diff != "" {
				t.Errorf("%T.StateDB([post-execution root of block %d]) %s", e, b.NumberU64(), diff)
			}
		}
	})

[alarso16]

That does look cleaner. I also tried to share with the recover portion of the test, since it's all the same logic besides one line

stale

[ARR4N]

Primarily readability and structure but the approach LGTM in general.

[ARR4N]

It doesn't make sense for the saexec.Executor to construct a worstcase.State. Although the plumbing works, the abstraction is strange and is being governed by the former.

This refactoring allows the Executor to be injected into the worstcase.State constructor instead of calling it:

package worstcase

type StateDBOpener interface {
  StateDB(root common.Hash) (*state.StateDB, error)
}

func New(hook.Points, *params.CacheConfig, *types.Block, StateDBOpener)

[alarso16]

I agree, I didn't love the abstraction, but didn't see a better way. This does seem to be an improvement, but still feels a little unnatural (specifically for testing). I added it, and let me know if you agree and we can refactor this constructor some more

[ARR4N]

Pretty much there. The move of the interface and test helper is trivial and I just have one open question. Please DM to take another look and we can almost certainly merge this today.

[ARR4N]

Why log here when we're not doing it in the regular path?

[alarso16]

I thought that logging the final state root known to the database is helpful, specifically if there's issues restarting the VM after shutdown.

FWIW setting log = false still does log, but at a debug instead of info level

[alarso16]

I'm not sure this regression test is still necessary....

[alarso16]

Since I now had two optional fields, seemed better to just provide the infrastructure.

[ARR4N]

Haven't finished the review but I've noticed something that requires a bit of work (as a test) so thought I'd share it early.

[ARR4N]

If the last-settled block has no transactions (allowed by SAE) and len(settles) > 1 then b.LastSettled().ParentBlock() will have the same post-execution state root and we'll un-track it early.

	keep := b.LastSettled()
	for _, s := range settles {
		if s.Hash() != keep.Hash() {
			vm.blocks.Delete(s.Hash())
		}
		// If `s` is the parent of `keep` and the latter has no transactions
		// then we MUST NOT dereference the state root too early.
		if r := s.PostExecutionStateRoot(); r != keep.PostExecutionStateRoot() {
			vm.exec.Untrack(r)
		}
	}
	if h := parentLastSettled.Hash(); h != keep.Hash() { // i.e. `parentLastSettled` was the last block's `keep`
		vm.blocks.Delete(h)
		vm.exec.Untrack(parentLastSettled.PostExecutionStateRoot())
	}

I presume this would be catastrophic as block-building couldn't then open the settled state, so such a scenario requires a test.

[ARR4N]

I think the second half of my block is incorrect and should follow the same pattern as in the loop.

[alarso16]

Each time a state root is seen by the Tracker, a reference is added. Thus, it won't removed until Untrack is called as many times as Track was called. Since each block will call Track, even if the state root is not unique, everything works fine

[alarso16]

Added a comment at the location of use (here) to explain that we don't need to check for duplicate roots