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 <[email protected]>

Author: rjl493456442

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()
-
-	// 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 := 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)
+			}
 		}
 	}
+	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

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
+}