Merge branch 'master' into lint-enable-gosec

Author: JonathanOppenheimer

core/blockchain.go

@@ -176,7 +176,7 @@ const (
 	// clean cache's underlying fastcache.
 	trieCleanCacheStatsNamespace = "hashdb/memcache/clean/fastcache"
 
-	firewoodFileName = "firewood_state"
+	firewoodFileName = "firewood.db"
 )
 
 // CacheConfig contains the configuration values for the trie database

core/extstate/firewood_database.go

@@ -29,12 +29,14 @@ func (db *firewoodAccessorDB) OpenTrie(root common.Hash) (state.Trie, error) {
 }
 
 // OpenStorageTrie opens a wrapped version of the account trie.
-func (*firewoodAccessorDB) OpenStorageTrie(_ common.Hash, _ common.Address, accountRoot common.Hash, self state.Trie) (state.Trie, error) {
+//
+//nolint:revive // removing names loses context.
+func (*firewoodAccessorDB) OpenStorageTrie(stateRoot common.Hash, addr common.Address, accountRoot common.Hash, self state.Trie) (state.Trie, error) {
 	accountTrie, ok := self.(*firewood.AccountTrie)
 	if !ok {
 		return nil, fmt.Errorf("invalid account trie type: %T", self)
 	}
-	return firewood.NewStorageTrie(accountTrie, accountRoot)
+	return firewood.NewStorageTrie(accountTrie)
 }
 
 // CopyTrie returns a deep copy of the given trie.

triedb/firewood/account_trie.go

@@ -24,6 +24,8 @@ import (
 //  2. The `Hash` method actually creates the proposal, since Firewood cannot calculate
 //     the hash of the trie without committing it. It is immediately dropped, and this
 //     can likely be optimized.
+//
+// Note this is not concurrent safe.
 type AccountTrie struct {
 	fw           *Database
 	parentRoot   common.Hash
@@ -49,7 +51,10 @@ func NewAccountTrie(root common.Hash, db *Database) (*AccountTrie, error) {
 	}, nil
 }
 
-// GetAccount implements state.Trie.
+// GetAccount returns the state account associated with an address.
+// - If the account has been updated, the new value is returned.
+// - If the account has been deleted, (nil, nil) is returned.
+// - If the account does not exist, (nil, nil) is returned.
 func (a *AccountTrie) GetAccount(addr common.Address) (*types.StateAccount, error) {
 	key := crypto.Keccak256Hash(addr.Bytes()).Bytes()
 
@@ -82,7 +87,10 @@ func (a *AccountTrie) GetAccount(addr common.Address) (*types.StateAccount, erro
 	return account, err
 }
 
-// GetStorage implements state.Trie.
+// GetStorage returns the value associated with a storage key for a given account address.
+// - If the storage slot has been updated, the new value is returned.
+// - If the storage slot has been deleted, (nil, nil) is returned.
+// - If the storage slot does not exist, (nil, nil) is returned.
 func (a *AccountTrie) GetStorage(addr common.Address, key []byte) ([]byte, error) {
 	// If the account has been deleted, we should return nil
 	accountKey := crypto.Keccak256Hash(addr.Bytes()).Bytes()
@@ -117,7 +125,8 @@ func (a *AccountTrie) GetStorage(addr common.Address, key []byte) ([]byte, error
 	return decoded, err
 }
 
-// UpdateAccount implements state.Trie.
+// UpdateAccount replaces or creates the state account associated with an address.
+// This new value will be returned for subsequent `GetAccount` calls.
 func (a *AccountTrie) UpdateAccount(addr common.Address, account *types.StateAccount) error {
 	// Queue the keys and values for later commit
 	key := crypto.Keccak256Hash(addr.Bytes()).Bytes()
@@ -132,7 +141,8 @@ func (a *AccountTrie) UpdateAccount(addr common.Address, account *types.StateAcc
 	return nil
 }
 
-// UpdateStorage implements state.Trie.
+// UpdateStorage replaces or creates the value associated with a storage key for a given account address.
+// This new value will be returned for subsequent `GetStorage` calls.
 func (a *AccountTrie) UpdateStorage(addr common.Address, key []byte, value []byte) error {
 	var combinedKey [2 * common.HashLength]byte
 	accountKey := crypto.Keccak256Hash(addr.Bytes()).Bytes()
@@ -153,7 +163,7 @@ func (a *AccountTrie) UpdateStorage(addr common.Address, key []byte, value []byt
 	return nil
 }
 
-// DeleteAccount implements state.Trie.
+// DeleteAccount removes the state account associated with an address.
 func (a *AccountTrie) DeleteAccount(addr common.Address) error {
 	key := crypto.Keccak256Hash(addr.Bytes()).Bytes()
 	// Queue the key for deletion
@@ -164,7 +174,7 @@ func (a *AccountTrie) DeleteAccount(addr common.Address) error {
 	return nil
 }
 
-// DeleteStorage implements state.Trie.
+// DeleteStorage removes the value associated with a storage key for a given account address.
 func (a *AccountTrie) DeleteStorage(addr common.Address, key []byte) error {
 	var combinedKey [2 * common.HashLength]byte
 	accountKey := crypto.Keccak256Hash(addr.Bytes()).Bytes()
@@ -180,7 +190,9 @@ func (a *AccountTrie) DeleteStorage(addr common.Address, key []byte) error {
 	return nil
 }
 
-// Hash implements state.Trie.
+// Hash returns the current hash of the state trie.
+// This will create a proposal and drop it, so it is not efficient to call for each transaction.
+// If there are no changes since the last call, the cached root is returned.
 func (a *AccountTrie) Hash() common.Hash {
 	hash, err := a.hash()
 	if err != nil {
@@ -203,7 +215,9 @@ func (a *AccountTrie) hash() (common.Hash, error) {
 	return a.root, nil
 }
 
-// Commit implements state.Trie.
+// Commit returns the new root hash of the trie and a NodeSet containing all modified accounts and storage slots.
+// The format of the NodeSet is different than in go-ethereum's trie implementation due to Firewood's design.
+// This boolean is ignored, as it is a relic of the StateTrie implementation.
 func (a *AccountTrie) Commit(bool) (common.Hash, *trienode.NodeSet, error) {
 	// Get the hash of the trie.
 	hash, err := a.hash()
@@ -212,7 +226,7 @@ func (a *AccountTrie) Commit(bool) (common.Hash, *trienode.NodeSet, error) {
 	}
 
 	// Create the NodeSet. This will be sent to `triedb.Update` later.
-	nodeset := trienode.NewNodeSet(a.parentRoot)
+	nodeset := trienode.NewNodeSet(common.Hash{})
 	for i, key := range a.updateKeys {
 		nodeset.AddNode(key, &trienode.Node{
 			Blob: a.updateValues[i],
@@ -229,16 +243,19 @@ func (*AccountTrie) UpdateContractCode(common.Address, common.Hash, []byte) erro
 }
 
 // GetKey implements state.Trie.
+// This should not be used, since any user should not be accessing by raw key.
 func (*AccountTrie) GetKey([]byte) []byte {
-	return nil // Not implemented, as this is only used in APIs
+	return nil
 }
 
 // NodeIterator implements state.Trie.
+// Firewood does not support iterating over internal nodes.
 func (*AccountTrie) NodeIterator([]byte) (trie.NodeIterator, error) {
 	return nil, errors.New("NodeIterator not implemented for Firewood")
 }
 
 // Prove implements state.Trie.
+// Firewood does not yet support providing key proofs.
 func (*AccountTrie) Prove([]byte, ethdb.KeyValueWriter) error {
 	return errors.New("Prove not implemented for Firewood")
 }

triedb/firewood/database.go

@@ -76,8 +76,12 @@ var Defaults = Config{
 	ReadCacheStrategy:    ffi.CacheAllReads,
 }
 
-func (c Config) BackendConstructor(_ ethdb.Database) triedb.DBOverride {
-	return New(&c)
+func (c Config) BackendConstructor(ethdb.Database) triedb.DBOverride {
+	db, err := New(c)
+	if err != nil {
+		log.Crit("firewood: error creating database", "error", err)
+	}
+	return db
 }
 
 type Database struct {
@@ -94,32 +98,24 @@ type Database struct {
 
 // New creates a new Firewood database with the given disk database and configuration.
 // Any error during creation will cause the program to exit.
-func New(config *Config) *Database {
-	//nolint:staticcheck // this nolint is required for the config.* nolints to work.
-	if config == nil {
-		log.Crit("firewood: config must be provided")
-	}
-
-	//nolint:staticcheck // false positive, if config is nil then we will have exited.
-	err := validatePath(config.FilePath)
-	if err != nil {
-		log.Crit("firewood: error validating config", "error", err)
+func New(config Config) (*Database, error) {
+	if err := validatePath(config.FilePath); err != nil {
+		return nil, err
 	}
 
-	//nolint:staticcheck // false positive, if config is nil then we will have exited.
 	fw, err := ffi.New(config.FilePath, &ffi.Config{
 		NodeCacheEntries:     uint(config.CleanCacheSize) / 256, // TODO: estimate 256 bytes per node
 		FreeListCacheEntries: config.FreeListCacheEntries,
 		Revisions:            config.Revisions,
 		ReadCacheStrategy:    config.ReadCacheStrategy,
 	})
 	if err != nil {
-		log.Crit("firewood: error creating firewood database", "error", err)
+		return nil, err
 	}
 
 	currentRoot, err := fw.Root()
 	if err != nil {
-		log.Crit("firewood: error getting current root", "error", err)
+		return nil, err
 	}
 
 	return &Database{
@@ -128,7 +124,7 @@ func New(config *Config) *Database {
 		proposalTree: &ProposalContext{
 			Root: common.Hash(currentRoot),
 		},
-	}
+	}, nil
 }
 
 func validatePath(path string) error {
@@ -138,17 +134,28 @@ func validatePath(path string) error {
 
 	// Check that the directory exists
 	dir := filepath.Dir(path)
-	_, err := os.Stat(dir)
-	if err == nil {
-		return nil // Directory exists
-	}
-	if !os.IsNotExist(err) {
+	switch info, err := os.Stat(dir); {
+	case os.IsNotExist(err):
+		log.Info("Database directory not found, creating", "path", dir)
+		if err := os.MkdirAll(dir, 0o755); err != nil {
+			return fmt.Errorf("error creating database directory: %w", err)
+		}
+		return nil
+	case err != nil:
 		return fmt.Errorf("error checking database directory: %w", err)
+	case !info.IsDir():
+		return fmt.Errorf("database directory path is not a directory: %s", dir)
 	}
-	log.Info("Database directory not found, creating", "path", dir)
-	if err := os.MkdirAll(dir, 0o755); err != nil {
-		return fmt.Errorf("error creating database directory: %w", err)
+
+	// TODO(#1253): remove this after testing infrastructure is updated
+	oldPath := filepath.Join(dir, "firewood_state")
+	if _, err := os.Stat(oldPath); err == nil {
+		log.Warn("Found old database file, moving to new location", "old", oldPath, "new", path)
+		if err := os.Rename(oldPath, path); err != nil {
+			return fmt.Errorf("failed to move old database file: %w", err)
+		}
 	}
+
 	return nil
 }
 
@@ -164,7 +171,7 @@ func (*Database) Scheme() string {
 }
 
 // Initialized checks whether a non-empty genesis block has been written.
-func (db *Database) Initialized(_ common.Hash) bool {
+func (db *Database) Initialized(common.Hash) bool {
 	rootBytes, err := db.fwDisk.Root()
 	if err != nil {
 		log.Error("firewood: error getting current root", "error", err)

triedb/firewood/storage_trie.go

@@ -10,28 +10,28 @@ import (
 
 type StorageTrie struct {
 	*AccountTrie
-	storageRoot common.Hash
 }
 
 // `NewStorageTrie` returns a wrapper around an `AccountTrie` since Firewood
 // does not require a separate storage trie. All changes are managed by the account trie.
-func NewStorageTrie(accountTrie *AccountTrie, storageRoot common.Hash) (*StorageTrie, error) {
+func NewStorageTrie(accountTrie *AccountTrie) (*StorageTrie, error) {
 	return &StorageTrie{
 		AccountTrie: accountTrie,
-		storageRoot: storageRoot,
 	}, nil
 }
 
 // Actual commit is handled by the account trie.
-// Return the old storage root as if there was no change - we don't want to use the
-// actual account trie hash and nodeset here.
-func (s *StorageTrie) Commit(bool) (common.Hash, *trienode.NodeSet, error) {
-	return s.storageRoot, nil, nil
+// Return the old storage root as if there was no change since Firewood
+// will manage the hash calculations without it.
+// All changes are managed by the account trie.
+func (*StorageTrie) Commit(bool) (common.Hash, *trienode.NodeSet, error) {
+	return common.Hash{}, nil, nil
 }
 
 // Firewood doesn't require tracking storage roots inside of an account.
-func (s *StorageTrie) Hash() common.Hash {
-	return s.storageRoot // only used in statedb to populate a `StateAccount`
+// They will be updated in place when hashing of the proposal takes place.
+func (*StorageTrie) Hash() common.Hash {
+	return common.Hash{}
 }
 
 // Copy should never be called on a storage trie, as it is just a wrapper around the account trie.