From f4a8cdb52809ebd4cae1f0fb1ded5ed10d526592 Mon Sep 17 00:00:00 2001 From: minh-bq <97180373+minh-bq@users.noreply.github.com> Date: Thu, 23 Nov 2023 13:05:06 +0700 Subject: [PATCH] core/state/snapshot: check difflayer staleness early (#27255) (#383) This PR adds a staleness-check to AccountRLP, before checking the bloom-filter and potentially going directly into the disklayer. --------- Co-authored-by: Martin Holst Swende Co-authored-by: rjl493456442 --- core/state/snapshot/difflayer.go | 12 +++++++++++- core/state/snapshot/snapshot_test.go | 4 ++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/core/state/snapshot/difflayer.go b/core/state/snapshot/difflayer.go index ee88938b77..2d69c33355 100644 --- a/core/state/snapshot/difflayer.go +++ b/core/state/snapshot/difflayer.go @@ -289,9 +289,14 @@ func (dl *diffLayer) Account(hash common.Hash) (*Account, error) { // // Note the returned account is not a copy, please don't modify it. func (dl *diffLayer) AccountRLP(hash common.Hash) ([]byte, error) { + dl.lock.RLock() + // Check staleness before reaching further. + if dl.Stale() { + dl.lock.RUnlock() + return nil, ErrSnapshotStale + } // 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(accountBloomHasher(hash)) if !hit { hit = dl.diffed.Contains(destructBloomHasher(hash)) @@ -358,6 +363,11 @@ func (dl *diffLayer) Storage(accountHash, storageHash common.Hash) ([]byte, erro // 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() + // Check staleness before reaching further. + if dl.Stale() { + dl.lock.RUnlock() + return nil, ErrSnapshotStale + } hit := dl.diffed.Contains(storageBloomHasher{accountHash, storageHash}) if !hit { hit = dl.diffed.Contains(destructBloomHasher(accountHash)) diff --git a/core/state/snapshot/snapshot_test.go b/core/state/snapshot/snapshot_test.go index 12f2765b3b..87c46629d9 100644 --- a/core/state/snapshot/snapshot_test.go +++ b/core/state/snapshot/snapshot_test.go @@ -183,6 +183,10 @@ func TestDiskLayerExternalInvalidationPartialFlatten(t *testing.T) { // be returned with junk data. This version of the test retains the bottom diff // layer to check the usual mode of operation where the accumulator is retained. func TestDiffLayerExternalInvalidationPartialFlatten(t *testing.T) { + // Un-commenting this triggers the bloom set to be deterministic. The values below + // were used to trigger the flaw described in https://github.com/ethereum/go-ethereum/issues/27254. + // bloomDestructHasherOffset, bloomAccountHasherOffset, bloomStorageHasherOffset = 14, 24, 5 + // Create an empty base layer and a snapshot tree out of it base := &diskLayer{ diskdb: rawdb.NewMemoryDatabase(),