[core] Race condition on diffLayer

[fxfactorial]

I encountered this race condition and it happens at difflayer.go:223 wrt dl.origin = origin but .Storage uses dl.origin at return dl.origin.Storage(accountHash, storageHash) in difflayer.go.

[holiman]

I encountered this race condition

Could you provide some more info on how you encountered it? Do you have a stack trace? Was it during a test?

Your change does two things, turning an RLock to a Lock, and changing the scope. I don't see the need for either of them, since both 1) storage and Storage internally obtains the Rlock, and 2) the internals are not modified, so readlock should suffice.

So any more info about how you encountered this would likely clear this up for me

[fxfactorial]

@holiman Sorry - was late and I was too brief.

I found it the usual way, with -race turned on and one thread hit a call to rebloom https://github.com/ethereum/go-ethereum/blob/70a8d2cbacae6378e0da73097035c27a8114672f/core/state/snapshot/difflayer.go#L223 which set the .origin field, but another thread hit the the call to .Storage which uses the .origin field.

So

1. the lock doesn't protect the usage of the field .origin
2. a read lock isn't enough since rebloom will set it

I'll try to look through tmux history for the -race stack traces

[karalabe]

The PR is definitely problematic because it serializes reads in the spanshots, and even keeps it locked for disk access.

If the underlying issue is the .origin, we can work around that by extracting it while still in the read lock:

	// Check the bloom filter first whether there's even a point in reaching into
	// all the maps in all the layers below
	dl.lock.RLock()
	hit := dl.diffed.Contains(storageBloomHasher{accountHash, storageHash})
	if !hit {
		hit = dl.diffed.Contains(destructBloomHasher(accountHash))
	}
	var origin *diskLayer
	if !hit {
		origin = dl.origin // extract origin while holding the lock
	}
	dl.lock.RUnlock()

	// If the bloom filter misses, don't even bother with traversing the memory
	// diff layers, reach straight into the bottom persistent disk layer
	if origin != nil {
		snapshotBloomStorageMissMeter.Mark(1)
		return origin.Storage(accountHash, storageHash)
	}
	// The bloom filter hit, start poking in the internal maps
	return dl.storage(accountHash, storageHash, 0)

Would this solve the issue @fxfactorial?

[karalabe]

Though I guess we'd need to look through the code now, because account and whatnot accessors will use the same patterns as the faulty storage above.

We definitely need the same fix in AccountRLP too.

[karalabe]

I think that would suffice. There are 1-2 more accesses into .origin via parent.origin paths, but those are snapshot mutation operations, and I think we only ever mutate serialized.

[fxfactorial]

The PR is definitely problematic because it serializes reads in the spanshots, and even keeps it locked for disk access.

If the underlying issue is the .origin, we can work around that by extracting it while still in the read lock:

	// Check the bloom filter first whether there's even a point in reaching into
	// all the maps in all the layers below
	dl.lock.RLock()
	hit := dl.diffed.Contains(storageBloomHasher{accountHash, storageHash})
	if !hit {
		hit = dl.diffed.Contains(destructBloomHasher(accountHash))
	}
	var origin *diskLayer
	if !hit {
		origin = dl.origin // extract origin while holding the lock
	}
	dl.lock.RUnlock()

	// If the bloom filter misses, don't even bother with traversing the memory
	// diff layers, reach straight into the bottom persistent disk layer
	if origin != nil {
		snapshotBloomStorageMissMeter.Mark(1)
		return origin.Storage(accountHash, storageHash)
	}
	// The bloom filter hit, start poking in the internal maps
	return dl.storage(accountHash, storageHash, 0)

Would this solve the issue @fxfactorial?

yes - i think so - I can clean up the other spots if you like as well (lmk where to look, i see you mentioned some places)

[holiman]

@fxfactorial Do you want to fix this? Would be nice to get it merged.
I think you can leave the other spots out, unless you find something that looks suspicious to you

[fxfactorial]

@holiman force pushed - covered the AccountRLP method as well -

[holiman]

LGTM, but please remove the iterate.sh, probably an accidental addition :)

[holiman]

LGTM, thanks!

[fjl]

@karalabe Please merge if you think this fix is OK.

[fxfactorial]

@karalabe ping - anything else for merge?

[karalabe]

SGTM