core/state/snapshot: address peter and martin's comment

core/state/snapshot/account.go

@@ -24,8 +24,10 @@ import (
 	"github.com/ethereum/go-ethereum/rlp"
 )
 
-// Account is a slim version of a state.Account, where the root and code hash
-// are replaced with a nil byte slice for empty accounts.
+// Account is a modified version of a state.Account, where the root is replaced
+// with a byte slice. This format can be used to represent full-consensus format
+// or slim-snapshot format which replaces the empty root and code hash as nil
+// byte slice.
 type Account struct {
 	Nonce    uint64
 	Balance  *big.Int
@@ -33,9 +35,8 @@ type Account struct {
 	CodeHash []byte
 }
 
-// AccountRLP converts a state.Account content into a slim snapshot version RLP
-// encoded.
-func AccountRLP(nonce uint64, balance *big.Int, root common.Hash, codehash []byte) []byte {
+// SlimAccount converts a state.Account content into a slim snapshot account
+func SlimAccount(nonce uint64, balance *big.Int, root common.Hash, codehash []byte) Account {
 	slim := Account{
 		Nonce:   nonce,
 		Balance: balance,
@@ -46,9 +47,40 @@ func AccountRLP(nonce uint64, balance *big.Int, root common.Hash, codehash []byt
 	if !bytes.Equal(codehash, emptyCode[:]) {
 		slim.CodeHash = codehash
 	}
-	data, err := rlp.EncodeToBytes(slim)
+	return slim
+}
+
+// SlimAccountRLP converts a state.Account content into a slim snapshot
+// version RLP encoded.
+func SlimAccountRLP(nonce uint64, balance *big.Int, root common.Hash, codehash []byte) []byte {
+	data, err := rlp.EncodeToBytes(SlimAccount(nonce, balance, root, codehash))
 	if err != nil {
 		panic(err)
 	}
 	return data
 }
+
+// FullAccount decodes the data on the 'slim RLP' format and return
+// the consensus format account.
+func FullAccount(data []byte) (Account, error) {
+	var account Account
+	if err := rlp.DecodeBytes(data, &account); err != nil {
+		return Account{}, err
+	}
+	if len(account.Root) == 0 {
+		account.Root = emptyRoot[:]
+	}
+	if len(account.CodeHash) == 0 {
+		account.CodeHash = emptyCode[:]
+	}
+	return account, nil
+}
+
+// FullAccountRLP converts data on the 'slim RLP' format into the full RLP-format.
+func FullAccountRLP(data []byte) ([]byte, error) {
+	account, err := FullAccount(data)
+	if err != nil {
+		return nil, err
+	}
+	return rlp.EncodeToBytes(account)
+}

core/state/snapshot/conversion.go

@@ -17,6 +17,7 @@
 package snapshot
 
 import (
+	"bytes"
 	"fmt"
 	"sync"
 	"time"
@@ -28,41 +29,6 @@ import (
 	"github.com/ethereum/go-ethereum/trie"
 )
 
-// conversionAccount is used for converting between full and slim format. When
-// doing this, we can consider 'balance' as a byte array, as it has already
-// been converted from big.Int into an rlp-byteslice.
-type conversionAccount struct {
-	Nonce    uint64
-	Balance  []byte
-	Root     []byte
-	CodeHash []byte
-}
-
-// SlimToFull converts data on the 'slim RLP' format into the full RLP-format.
-// Besides, this function accepts another parameter "subRoot". If the root is
-// not empty, apply it to account. Usually the subRoot is specified if we want
-// to verify the whole state or re-generate state root with different trie algo.
-func SlimToFull(data []byte, subRoot common.Hash) ([]byte, error) {
-	acc := &conversionAccount{}
-	if err := rlp.DecodeBytes(data, acc); err != nil {
-		return nil, err
-	}
-	if len(acc.Root) == 0 {
-		acc.Root = emptyRoot[:]
-	}
-	if subRoot != (common.Hash{}) {
-		acc.Root = subRoot.Bytes()
-	}
-	if len(acc.CodeHash) == 0 {
-		acc.CodeHash = emptyCode[:]
-	}
-	fullData, err := rlp.EncodeToBytes(acc)
-	if err != nil {
-		return nil, err
-	}
-	return fullData, nil
-}
-
 // trieKV represents a trie key-value pair
 type trieKV struct {
 	key   common.Hash
@@ -76,17 +42,17 @@ type (
 
 	// leafCallbackFn is the callback invoked at the leaves of the trie,
 	// returns the subtrie root with the specified subtrie identifier.
-	leafCallbackFn func(hash common.Hash) common.Hash
+	leafCallbackFn func(hash common.Hash, stat *generateStats) common.Hash
 )
 
 // GenerateAccountTrieRoot takes an account iterator and reproduces the root hash.
-func GenerateAccountTrieRoot(it AccountIterator) common.Hash {
-	return generateTrieRoot(it, true, stdGenerate, nil, true)
+func GenerateAccountTrieRoot(it AccountIterator) (common.Hash, error) {
+	return generateTrieRoot(it, common.Hash{}, stdGenerate, nil, &generateStats{start: time.Now()}, true)
 }
 
 // GenerateStorageTrieRoot takes a storage iterator and reproduces the root hash.
-func GenerateStorageTrieRoot(it StorageIterator) common.Hash {
-	return generateTrieRoot(it, false, stdGenerate, nil, true)
+func GenerateStorageTrieRoot(account common.Hash, it StorageIterator) (common.Hash, error) {
+	return generateTrieRoot(it, account, stdGenerate, nil, &generateStats{start: time.Now()}, true)
 }
 
 // VerifyState takes the whole snapshot tree as the input, traverses all the accounts
@@ -97,69 +63,198 @@ func VerifyState(snaptree *Tree, root common.Hash) error {
 	if err != nil {
 		return err
 	}
-	got := generateTrieRoot(acctIt, true, stdGenerate, func(account common.Hash) common.Hash {
+	got, err := generateTrieRoot(acctIt, common.Hash{}, stdGenerate, func(account common.Hash, stat *generateStats) common.Hash {
 		storageIt, err := snaptree.StorageIterator(root, account, common.Hash{})
 		if err != nil {
 			return common.Hash{}
 		}
-		return generateTrieRoot(storageIt, false, stdGenerate, nil, false)
-	}, true)
+		hash, err := generateTrieRoot(storageIt, account, stdGenerate, nil, stat, false)
+		if err != nil {
+			return common.Hash{}
+		}
+		return hash
+	}, &generateStats{start: time.Now()}, true)
 
+	if err != nil {
+		return err
+	}
 	if got != root {
 		return fmt.Errorf("State root hash mismatch, got %x, want %x", got, root)
 	}
 	return nil
 }
 
-func generateTrieRoot(it Iterator, accountIterator bool, generatorFn trieGeneratorFn, leafCallback leafCallbackFn, report bool) common.Hash {
+// generateStats is a collection of statistics gathered by the trie generator
+// for logging purposes.
+type generateStats struct {
+	accounts   uint64
+	slots      uint64
+	curAccount common.Hash
+	curSlot    common.Hash
+	start      time.Time
+	lock       sync.RWMutex
+}
+
+// progress records the progress trie generator made recently.
+func (stat *generateStats) progress(accounts, slots uint64, curAccount common.Hash, curSlot common.Hash) {
+	stat.lock.Lock()
+	defer stat.lock.Unlock()
+
+	stat.accounts += accounts
+	stat.slots += slots
+	if curAccount != (common.Hash{}) {
+		stat.curAccount = curAccount
+	}
+	if curSlot != (common.Hash{}) {
+		stat.curSlot = curSlot
+	}
+}
+
+// report prints the cumulative progress statistic smartly.
+func (stat *generateStats) report() {
+	stat.lock.RLock()
+	defer stat.lock.RUnlock()
+
+	var ctx []interface{}
+	if stat.curSlot != (common.Hash{}) {
+		ctx = append(ctx, []interface{}{
+			"in", stat.curAccount,
+			"at", stat.curSlot,
+		}...)
+	} else {
+		ctx = append(ctx, []interface{}{"at", stat.curAccount}...)
+	}
+	// Add the usual measurements
+	ctx = append(ctx, []interface{}{"accounts", stat.accounts}...)
+	if stat.slots != 0 {
+		ctx = append(ctx, []interface{}{"slots", stat.slots}...)
+	}
+	ctx = append(ctx, []interface{}{"elapsed", common.PrettyDuration(time.Since(stat.start))}...)
+	log.Info("Generating trie hash from snapshot", ctx)
+}
+
+// reportDone prints the last log when the whole generation is finished.
+func (stat *generateStats) reportDone() {
+	stat.lock.RLock()
+	defer stat.lock.RUnlock()
+
+	var ctx []interface{}
+	ctx = append(ctx, []interface{}{"accounts", stat.accounts}...)
+	if stat.slots != 0 {
+		ctx = append(ctx, []interface{}{"slots", stat.slots}...)
+	}
+	ctx = append(ctx, []interface{}{"elapsed", common.PrettyDuration(time.Since(stat.start))}...)
+	log.Info("Generated trie hash from snapshot", ctx)
+}
+
+// generateTrieRoot generates the trie hash based on the snapshot iterator.
+// It can be used for generating account trie, storage trie or even the
+// whole state which connects the accounts and the corresponding storages.
+func generateTrieRoot(it Iterator, account common.Hash, generatorFn trieGeneratorFn, leafCallback leafCallbackFn, stats *generateStats, report bool) (common.Hash, error) {
 	var (
 		in  = make(chan trieKV)         // chan to pass leaves
-		out = make(chan common.Hash) // chan to collect result
+		out = make(chan common.Hash, 1) // chan to collect result
 		wg  sync.WaitGroup
 	)
+	// Spin up a go-routine for trie hash re-generation
 	wg.Add(1)
 	go func() {
+		defer wg.Done()
 		generatorFn(in, out)
-		wg.Done()
 	}()
 
+	// Spin up a go-routine for progress logging
+	if report && stats != nil {
+		stopLogging := make(chan struct{})
+		defer close(stopLogging)
+
+		go func() {
+			defer wg.Done()
+
+			timer := time.NewTimer(0)
+			defer timer.Stop()
+			<-timer.C // discard the initial tick
+
+			for {
+				select {
+				case <-timer.C:
+					stats.report()
+					timer.Reset(time.Second * 8)
+				case <-stopLogging:
+					stats.reportDone()
+					return
+				}
+			}
+		}()
+	}
 	var (
-		start   = time.Now()
 		logged    = time.Now()
-		entries = 0
+		processed = uint64(0)
+		leaf      trieKV
+		last      common.Hash
 	)
 	// Start to feed leaves
 	for it.Next() {
-		// Apply the leaf callback first. Normally the callback is used
-		// to traverse the storage trie and re-generate the subtrie root.
-		// If the callback is specified, then replace the original storage
-		// root hash with new one.
-		var subRoot common.Hash
-		if leafCallback != nil {
-			subRoot = leafCallback(it.Hash())
+		if account == (common.Hash{}) {
+			var (
+				err      error
+				fullData []byte
+			)
+			if leafCallback == nil {
+				fullData, err = FullAccountRLP(it.(AccountIterator).Account())
+				if err != nil {
+					close(in)
+					return common.Hash{}, err
 				}
-		var l trieKV
-		if accountIterator {
-			fullData, _ := SlimToFull(it.(AccountIterator).Account(), subRoot)
-			l = trieKV{it.Hash(), fullData}
 			} else {
-			l = trieKV{it.Hash(), it.(StorageIterator).Slot()}
+				account, err := FullAccount(it.(AccountIterator).Account())
+				if err != nil {
+					close(in)
+					return common.Hash{}, err
 				}
-		in <- l
-		if time.Since(logged) > 8*time.Second && report {
-			log.Info("Generating trie hash from snapshot", "at", l.key, "entries", entries, "elapsed", time.Since(start))
-			logged = time.Now()
+				// Apply the leaf callback. Normally the callback is used to traverse
+				// the storage trie and re-generate the subtrie root.
+				subroot := leafCallback(it.Hash(), stats)
+				if !bytes.Equal(account.Root, subroot.Bytes()) {
+					close(in)
+					return common.Hash{}, fmt.Errorf("invalid subroot(%x), want %x, got %x", it.Hash(), account.Root, subroot)
+				}
+				fullData, err = rlp.EncodeToBytes(account)
+				if err != nil {
+					close(in)
+					return common.Hash{}, err
+				}
+			}
+			leaf = trieKV{it.Hash(), fullData}
+		} else {
+			leaf = trieKV{it.Hash(), it.(StorageIterator).Slot()}
+		}
+		in <- leaf
+
+		// Accumulate the generaation statistic if it's required.
+		processed++
+		if time.Since(logged) > 3*time.Second && stats != nil {
+			if account == (common.Hash{}) {
+				stats.progress(processed, 0, it.Hash(), common.Hash{})
+			} else {
+				stats.progress(0, processed, account, it.Hash())
+			}
+			logged, processed = time.Now(), 0
+		}
+		last = it.Hash()
+	}
+	// Commit the last part statistic.
+	if processed > 0 && stats != nil {
+		if account == (common.Hash{}) {
+			stats.progress(processed, 0, last, common.Hash{})
+		} else {
+			stats.progress(0, processed, account, last)
 		}
-		entries++
 	}
 	close(in)
 	result := <-out
 	wg.Wait()
-
-	if report {
-		log.Info("Generated trie hash from snapshot", "entries", entries, "elapsed", time.Since(start))
-	}
-	return result
+	return result, nil
 }
 
 // stdGenerate is a very basic hexary trie builder which uses the same Trie

core/state/snapshot/generate.go

@@ -167,7 +167,7 @@ func (dl *diskLayer) generate(stats *generatorStats) {
 		if err := rlp.DecodeBytes(accIt.Value, &acc); err != nil {
 			log.Crit("Invalid account encountered during snapshot creation", "err", err)
 		}
-		data := AccountRLP(acc.Nonce, acc.Balance, acc.Root, acc.CodeHash)
+		data := SlimAccountRLP(acc.Nonce, acc.Balance, acc.Root, acc.CodeHash)
 
 		// If the account is not yet in-progress, write it out
 		if accMarker == nil || !bytes.Equal(accountHash[:], accMarker) {

core/state/snapshot/iterator.go

@@ -137,6 +137,8 @@ func (it *diffAccountIterator) Hash() common.Hash {
 // This method assumes that flattening does not delete elements from
 // the accountdata mapping (writing nil into it is fine though), and will panic
 // if elements have been deleted.
+//
+// Note the returned account is not a copy, please don't modify it.
 func (it *diffAccountIterator) Account() []byte {
 	it.layer.lock.RLock()
 	blob, ok := it.layer.accountData[it.curHash]
@@ -151,7 +153,7 @@ func (it *diffAccountIterator) Account() []byte {
 	if it.layer.Stale() {
 		it.fail, it.keys = ErrSnapshotStale, nil
 	}
-	return common.CopyBytes(blob)
+	return blob
 }
 
 // Release is a noop for diff account iterators as there are no held resources.
@@ -181,7 +183,7 @@ func (it *diskAccountIterator) Next() bool {
 	}
 	// Try to advance the iterator and release it if we reached the end
 	for {
-		if !it.it.Next() || !bytes.HasPrefix(it.it.Key(), rawdb.SnapshotAccountPrefix) {
+		if !it.it.Next() {
 			it.it.Release()
 			it.it = nil
 			return false
@@ -212,7 +214,7 @@ func (it *diskAccountIterator) Hash() common.Hash {
 
 // Account returns the RLP encoded slim account the iterator is currently at.
 func (it *diskAccountIterator) Account() []byte {
-	return common.CopyBytes(it.it.Value())
+	return it.it.Value()
 }
 
 // Release releases the database snapshot held during iteration.
@@ -302,6 +304,8 @@ func (it *diffStorageIterator) Hash() common.Hash {
 // This method assumes that flattening does not delete elements from
 // the storage mapping (writing nil into it is fine though), and will panic
 // if elements have been deleted.
+//
+// Note the returned slot is not a copy, please don't modify it.
 func (it *diffStorageIterator) Slot() []byte {
 	it.layer.lock.RLock()
 	storage, ok := it.layer.storageData[it.account]
@@ -317,7 +321,7 @@ func (it *diffStorageIterator) Slot() []byte {
 	if it.layer.Stale() {
 		it.fail, it.keys = ErrSnapshotStale, nil
 	}
-	return common.CopyBytes(blob)
+	return blob
 }
 
 // Release is a noop for diff account iterators as there are no held resources.
@@ -351,9 +355,8 @@ func (it *diskStorageIterator) Next() bool {
 		return false
 	}
 	// Try to advance the iterator and release it if we reached the end
-	prefix := append(rawdb.SnapshotStoragePrefix, it.account.Bytes()...)
 	for {
-		if !it.it.Next() || !bytes.HasPrefix(it.it.Key(), prefix) {
+		if !it.it.Next() {
 			it.it.Release()
 			it.it = nil
 			return false
@@ -384,7 +387,7 @@ func (it *diskStorageIterator) Hash() common.Hash {
 
 // Slot returns the raw strorage slot content the iterator is currently at.
 func (it *diskStorageIterator) Slot() []byte {
-	return common.CopyBytes(it.it.Value())
+	return it.it.Value()
 }
 
 // Release releases the database snapshot held during iteration.

core/state/snapshot/iterator_binary.go

@@ -160,6 +160,8 @@ func (it *binaryIterator) Hash() common.Hash {
 // Account returns the RLP encoded slim account the iterator is currently at, or
 // nil if the iterated snapshot stack became stale (you can check Error after
 // to see if it failed or not).
+//
+// Note the returned account is not a copy, please don't modify it.
 func (it *binaryIterator) Account() []byte {
 	if !it.accountIterator {
 		return nil
@@ -170,12 +172,14 @@ func (it *binaryIterator) Account() []byte {
 		it.fail = err
 		return nil
 	}
-	return common.CopyBytes(blob)
+	return blob
 }
 
 // Slot returns the raw storage slot data the iterator is currently at, or
 // nil if the iterated snapshot stack became stale (you can check Error after
 // to see if it failed or not).
+//
+// Note the returned slot is not a copy, please don't modify it.
 func (it *binaryIterator) Slot() []byte {
 	if it.accountIterator {
 		return nil
@@ -185,7 +189,7 @@ func (it *binaryIterator) Slot() []byte {
 		it.fail = err
 		return nil
 	}
-	return common.CopyBytes(blob)
+	return blob
 }
 
 // Release recursively releases all the iterators in the stack.

core/state/snapshot/iterator_fast.go

@@ -307,11 +307,13 @@ func (fi *fastIterator) Hash() common.Hash {
 }
 
 // Account returns the current account blob.
+// Note the returned account is not a copy, please don't modify it.
 func (fi *fastIterator) Account() []byte {
 	return fi.curAccount
 }
 
 // Slot returns the current storage slot.
+// Note the returned slot is not a copy, please don't modify it.
 func (fi *fastIterator) Slot() []byte {
 	return fi.curSlot
 }

core/state/snapshot/iterator_test.go

@@ -31,7 +31,6 @@ import (
 // TestAccountIteratorBasics tests some simple single-layer(diff and disk) iteration
 func TestAccountIteratorBasics(t *testing.T) {
 	var (
-		nilAccount int
 		destructs = make(map[common.Hash]struct{})
 		accounts  = make(map[common.Hash][]byte)
 		storage   = make(map[common.Hash]map[common.Hash][]byte)
@@ -44,8 +43,6 @@ func TestAccountIteratorBasics(t *testing.T) {
 		accounts[h] = data
 		if rand.Intn(4) == 0 {
 			destructs[h] = struct{}{}
-			delete(accounts, h)
-			nilAccount += 1
 		}
 		if rand.Intn(2) == 0 {
 			accStorage := make(map[common.Hash][]byte)
@@ -62,7 +59,7 @@ func TestAccountIteratorBasics(t *testing.T) {
 
 	diskLayer := diffToDisk(diffLayer)
 	it = diskLayer.AccountIterator(common.Hash{})
-	verifyIterator(t, 100-nilAccount, it, verifyNothing) // Nil is allowed for single layer iterator
+	verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator
 }
 
 // TestStorageIteratorBasics tests some simple single-layer(diff and disk) iteration for storage
@@ -195,9 +192,9 @@ func verifyIterator(t *testing.T, expCount int, it Iterator, verify verifyConten
 			t.Errorf("wrong order: %x >= %x", last, hash)
 		}
 		count++
-		if verify == verifyAccount && it.(AccountIterator).Account() == nil {
+		if verify == verifyAccount && len(it.(AccountIterator).Account()) == 0 {
 			t.Errorf("iterator returned nil-value for hash %x", hash)
-		} else if verify == verifyStorage && it.(StorageIterator).Slot() == nil {
+		} else if verify == verifyStorage && len(it.(StorageIterator).Slot()) == 0 {
 			t.Errorf("iterator returned nil-value for hash %x", hash)
 		}
 	}

core/state/statedb.go

@@ -472,7 +472,7 @@ func (s *StateDB) updateStateObject(obj *stateObject) {
 	// enough to track account updates at commit time, deletions need tracking
 	// at transaction boundary level to ensure we capture state clearing.
 	if s.snap != nil {
-		s.snapAccounts[obj.addrHash] = snapshot.AccountRLP(obj.data.Nonce, obj.data.Balance, obj.data.Root, obj.data.CodeHash)
+		s.snapAccounts[obj.addrHash] = snapshot.SlimAccountRLP(obj.data.Nonce, obj.data.Balance, obj.data.Root, obj.data.CodeHash)
 	}
 }