eth/tracers: avoid unsyncronized mutations on trie database

[holiman]

This might fix #23559.
The current code has several goroutine groups, for different purposes.

1. Executors which trace chains. Even if those are disabled (replaced with a random timout), the error(s) still happen. So they're not the problem.
2. One state-maker loop, which readies the state for the executors to trace on. This one also references/dereferences state.
3. One result-catcher loop. This one spews data to the caller, and also dereferences nodes.

I think the root cause is the fact that a statedb.Commit, which is done on 2 inside the StateAt call writes to the trie.Database. While doing the actual insert, it does hold the trie mutex, but I suspect that the locking is too granular: if we are in the middle of a large commit when all of a sudden a dereference happens, then things might become br0ken.

So this PR moves the final dereferencing into 2, so all Reference/Dereference/Commit operations are done from one routine only.

With this fix, I haven't been able to repro the issues any more.

Further testing/verification would be appreciated, to find out if it's a correct fix or not.

[rjl493456442]

if we are in the middle of a large commit when all of a sudden a dereference happens, then things might become br0ken.

But the dereference operation should be blocked until the lock is released. And the delayed dereference shouldn't lead to missing trie node error.

EDIT: It indeed has data race. The issue is valid.

[holiman]

But the dereference operation should be blocked until the lock is released.

The Commit doesn't hold the lock, it's aqcuired only when it needs to do an insert or add a storage root reference.

Take a look at https://github.com/ethereum/go-ethereum/blob/master/trie/database.go#L325 . When we do the commit, and do an insert of a node n.

• If the node is already there, we do nothing else.
• Later on during the commit operation, we will add a new parent p (at least the state root)
• which will increase the parent count of it's children (including the parent count of node n). But before we've done so, the parent count is untouched. Therefore, it's ripe for removal if a previous parent is dereffed

state 1
              R1
            /  \
           B     C
          /       \
         leaf1   leaf2
         
state 2:
         R2
         |
         C2
         |
         leaf2
         
state 3:

             R3
            /  \
           B     C
          /       \
         leaf1   leaf2

While committing `R3`, we do

insert(leaf1)
insert(b)
// Now, deref R1 happens
  - remove leaf1
  - remove B
  - remove R1
insert R3

So IIUC, it's only when we insert a new node that we recurse down the tree and update the refcounts. And until we've added a new node (e.g. the stateroot), the internal state (parent counts) of the db is kind of inconsistent.

[rjl493456442]

Perhaps rename the _statedb to s?

[rjl493456442]

Wait, is it necessary? if error is returned we will exit the for loop anyway. The statedb will not be used even it's set to nil.

[holiman]

You're right, it isn't necessary. While investigating, I did use the statedb for outputting some more info, but that code is gone now, so I can revert that change

[rjl493456442]

LGTM, just some nitpicks.