From 36bcc24fbe1b50f641564b5dfa730523a6b0bb94 Mon Sep 17 00:00:00 2001 From: rjl493456442 Date: Thu, 26 Jun 2025 23:19:02 +0800 Subject: [PATCH] triedb/pathdb: fix journal resolution in pathdb (#32097) This pull request fixes a flaw in the PBSS state iterator, which could return empty account or storage data. In PBSS, multiple in-memory diff layers and a write buffer are maintained. These layers are persisted to the database and reloaded after node restarts. However, since the state data is encoded using RLP, the distinction between nil and an empty byte slice is lost during the encode/decode process. As a result, invalid state values such as `[]byte{}` can appear in PBSS and ultimately be returned by the state iterator. Checkout https://github.com/ethereum/go-ethereum/blob/master/triedb/pathdb/iterator_fast.go#L270 for more iterator details. It's a long-term existent issue and now be activated since the snapshot integration. The error `err="range contains deletion"` will occur when Geth tries to serve other peers with SNAP protocol request. --------- Co-authored-by: Felix Lange --- triedb/pathdb/iterator_test.go | 93 +++++++++++++++++++++++++++------- triedb/pathdb/states.go | 23 ++++++--- 2 files changed, 91 insertions(+), 25 deletions(-) diff --git a/triedb/pathdb/iterator_test.go b/triedb/pathdb/iterator_test.go index b24575cb47..adb534f47d 100644 --- a/triedb/pathdb/iterator_test.go +++ b/triedb/pathdb/iterator_test.go @@ -816,7 +816,8 @@ func testAccountIteratorDeletions(t *testing.T, newIterator func(db *Database, r config := &Config{ NoAsyncGeneration: true, } - db := New(rawdb.NewMemoryDatabase(), config, false) + memoryDB := rawdb.NewMemoryDatabase() + db := New(memoryDB, config, false) // Stack three diff layers on top with various overlaps db.Update(common.HexToHash("0x02"), types.EmptyRootHash, 1, trienode.NewMergedNodeSet(), @@ -831,31 +832,55 @@ func testAccountIteratorDeletions(t *testing.T, newIterator func(db *Database, r db.Update(common.HexToHash("0x04"), common.HexToHash("0x03"), 3, trienode.NewMergedNodeSet(), NewStateSetWithOrigin(randomAccountSet("0x33", "0x44", "0x55"), nil, nil, nil, false)) - // The output should be 11,33,44,55 - it := newIterator(db, common.HexToHash("0x04"), common.Hash{}) - // Do a quick check - verifyIterator(t, 4, it, verifyAccount) - it.Release() + verify := func() { + // The output should be 11,33,44,55 + it := newIterator(db, common.HexToHash("0x04"), common.Hash{}) + // Do a quick check + verifyIterator(t, 4, it, verifyAccount) + it.Release() - // And a more detailed verification that we indeed do not see '0x22' - it = newIterator(db, common.HexToHash("0x04"), common.Hash{}) - defer it.Release() - for it.Next() { - hash := it.Hash() - if it.Account() == nil { - t.Errorf("iterator returned nil-value for hash %x", hash) - } - if hash == deleted { - t.Errorf("expected deleted elem %x to not be returned by iterator", deleted) + // And a more detailed verification that we indeed do not see '0x22' + it = newIterator(db, common.HexToHash("0x04"), common.Hash{}) + defer it.Release() + for it.Next() { + hash := it.Hash() + if it.Account() == nil { + t.Errorf("iterator returned nil-value for hash %x", hash) + } + if hash == deleted { + t.Errorf("expected deleted elem %x to not be returned by iterator", deleted) + } } } + verify() + + if err := db.Journal(common.HexToHash("0x04")); err != nil { + t.Fatalf("Failed to journal the database, %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Failed to close the database, %v", err) + } + db = New(memoryDB, config, false) + + verify() } func TestStorageIteratorDeletions(t *testing.T) { config := &Config{ NoAsyncGeneration: true, } - db := New(rawdb.NewMemoryDatabase(), config, false) + memoryDB := rawdb.NewMemoryDatabase() + db := New(memoryDB, config, false) + + restart := func(head common.Hash) { + if err := db.Journal(head); err != nil { + t.Fatalf("Failed to journal the database, %v", err) + } + if err := db.Close(); err != nil { + t.Fatalf("Failed to close the database, %v", err) + } + db = New(memoryDB, config, false) + } // Stack three diff layers on top with various overlaps db.Update(common.HexToHash("0x02"), types.EmptyRootHash, 1, trienode.NewMergedNodeSet(), @@ -874,6 +899,19 @@ func TestStorageIteratorDeletions(t *testing.T) { verifyIterator(t, 3, it, verifyStorage) it.Release() + // Ensure the iteration result aligns after the database restart + restart(common.HexToHash("0x03")) + + // The output should be 02,04,05,06 + it, _ = db.StorageIterator(common.HexToHash("0x03"), common.HexToHash("0xaa"), common.Hash{}) + verifyIterator(t, 4, it, verifyStorage) + it.Release() + + // The output should be 04,05,06 + it, _ = db.StorageIterator(common.HexToHash("0x03"), common.HexToHash("0xaa"), common.HexToHash("0x03")) + verifyIterator(t, 3, it, verifyStorage) + it.Release() + // Destruct the whole storage accounts := map[common.Hash][]byte{ common.HexToHash("0xaa"): nil, @@ -885,6 +923,12 @@ func TestStorageIteratorDeletions(t *testing.T) { verifyIterator(t, 0, it, verifyStorage) it.Release() + // Ensure the iteration result aligns after the database restart + restart(common.HexToHash("0x04")) + it, _ = db.StorageIterator(common.HexToHash("0x04"), common.HexToHash("0xaa"), common.Hash{}) + verifyIterator(t, 0, it, verifyStorage) + it.Release() + // Re-insert the slots of the same account db.Update(common.HexToHash("0x05"), common.HexToHash("0x04"), 4, trienode.NewMergedNodeSet(), NewStateSetWithOrigin(randomAccountSet("0xaa"), randomStorageSet([]string{"0xaa"}, [][]string{{"0x07", "0x08", "0x09"}}, nil), nil, nil, false)) @@ -894,6 +938,14 @@ func TestStorageIteratorDeletions(t *testing.T) { verifyIterator(t, 3, it, verifyStorage) it.Release() + // Ensure the iteration result aligns after the database restart + restart(common.HexToHash("0x05")) + + // The output should be 07,08,09 + it, _ = db.StorageIterator(common.HexToHash("0x05"), common.HexToHash("0xaa"), common.Hash{}) + verifyIterator(t, 3, it, verifyStorage) + it.Release() + // Destruct the whole storage but re-create the account in the same layer db.Update(common.HexToHash("0x06"), common.HexToHash("0x05"), 5, trienode.NewMergedNodeSet(), NewStateSetWithOrigin(randomAccountSet("0xaa"), randomStorageSet([]string{"0xaa"}, [][]string{{"0x11", "0x12"}}, [][]string{{"0x07", "0x08", "0x09"}}), nil, nil, false)) @@ -903,6 +955,13 @@ func TestStorageIteratorDeletions(t *testing.T) { it.Release() verifyIterator(t, 2, db.tree.get(common.HexToHash("0x06")).(*diffLayer).newBinaryStorageIterator(common.HexToHash("0xaa"), common.Hash{}), verifyStorage) + + // Ensure the iteration result aligns after the database restart + restart(common.HexToHash("0x06")) + it, _ = db.StorageIterator(common.HexToHash("0x06"), common.HexToHash("0xaa"), common.Hash{}) + verifyIterator(t, 2, it, verifyStorage) // The output should be 11,12 + it.Release() + verifyIterator(t, 2, db.tree.get(common.HexToHash("0x06")).(*diffLayer).newBinaryStorageIterator(common.HexToHash("0xaa"), common.Hash{}), verifyStorage) } // TestStaleIterator tests if the iterator could correctly terminate the iteration diff --git a/triedb/pathdb/states.go b/triedb/pathdb/states.go index 2a88d74f5f..bc638a569e 100644 --- a/triedb/pathdb/states.go +++ b/triedb/pathdb/states.go @@ -387,8 +387,8 @@ func (s *stateSet) decode(r *rlp.Stream) error { if err := r.Decode(&dec); err != nil { return fmt.Errorf("load diff accounts: %v", err) } - for i := 0; i < len(dec.AddrHashes); i++ { - accountSet[dec.AddrHashes[i]] = dec.Accounts[i] + for i := range dec.AddrHashes { + accountSet[dec.AddrHashes[i]] = empty2nil(dec.Accounts[i]) } s.accountData = accountSet @@ -407,8 +407,8 @@ func (s *stateSet) decode(r *rlp.Stream) error { } for _, entry := range storages { storageSet[entry.AddrHash] = make(map[common.Hash][]byte, len(entry.Keys)) - for i := 0; i < len(entry.Keys); i++ { - storageSet[entry.AddrHash][entry.Keys[i]] = entry.Vals[i] + for i := range entry.Keys { + storageSet[entry.AddrHash][entry.Keys[i]] = empty2nil(entry.Vals[i]) } } s.storageData = storageSet @@ -550,8 +550,8 @@ func (s *StateSetWithOrigin) decode(r *rlp.Stream) error { if err := r.Decode(&accounts); err != nil { return fmt.Errorf("load diff account origin set: %v", err) } - for i := 0; i < len(accounts.Accounts); i++ { - accountSet[accounts.Addresses[i]] = accounts.Accounts[i] + for i := range accounts.Accounts { + accountSet[accounts.Addresses[i]] = empty2nil(accounts.Accounts[i]) } s.accountOrigin = accountSet @@ -570,10 +570,17 @@ func (s *StateSetWithOrigin) decode(r *rlp.Stream) error { } for _, storage := range storages { storageSet[storage.Address] = make(map[common.Hash][]byte) - for i := 0; i < len(storage.Keys); i++ { - storageSet[storage.Address][storage.Keys[i]] = storage.Vals[i] + for i := range storage.Keys { + storageSet[storage.Address][storage.Keys[i]] = empty2nil(storage.Vals[i]) } } s.storageOrigin = storageSet return nil } + +func empty2nil(b []byte) []byte { + if len(b) == 0 { + return nil + } + return b +}