core/state/snapshot: fix data race in layer flattening

[rjl493456442]

This PR fixes a data race in snapshot tree which is described with a unit test.

[holiman]

This seems correct

[holiman]

Thinking about this some more, doesn't this only fix half the problem?

Let's say you have this situation

     p1
      | 
     p2
  /      \
a          b

Now, from snapshot a, we do a cap, flattening p2 into p1. The existing guarantees for both a and b is that if you try to read data while this is happening, you might get an error response "stale data". And this PR fixes that to instead wait until the flattening is done.

However, won't the same problem still be present for the b snapshot? So the fix here works only for the happy path, but we're still in the same situation if we try to access the data through b instead of a, and thus still cannot really rely on this new guarantee...?

Summary: whatever problem you have which prompted this, are you sure this PR really makes it go away?

[rjl493456442]

@holiman

In the scenario you described

     p1
      | 
     p2
  /      \
a          b

after cap(a)

     p2'
  /      \
a          b

when we access state via b, we will visit p2'(with this PR), instead of stale p2. So from the PoV of layer b, the parent content is not changed, the mix of p1 and p2.

whatever problem you have which prompted this

The real problem is: previously when the flattening happens(p1+p2 => p2'), the layer p1 and p2 are marked as stale but the b.parent is still p2.

In this PR, the read will be blocked until the b.parent is replaced by p2'

[holiman]

In this PR, the read will be blocked until the b.parent is replaced by p2'

Will it? IIUC, only reads via a (which does the cap) will be blocked, not reads via b, which will not know anything about the ongoing cap operation ? I may have misunderstood

[rjl493456442]

@holiman ah, yes.

     p0
      | 
     p1
      | 
     p2
  /      \
a          b

But in this case, flatten p1->p0, the blocking will start from p2. This PR works in most of the cases since we have 128 layer depth.

[holiman]

Heh, you dodged the scenario by adding another level above, nice move :)

Ok, so I guess my concern was that this PR introduces a new "guarantee" which is correct in 99% of all cases, but not 100.

It might be fine, just sayin.

[fjl]

@holiman @rjl493456442 can this be merged?

[rjl493456442]

@fjl I think it's mergeable. Can @holiman give a approval again?

[karalabe]

Seems good to me