feat: Add firewood as a database #1029

alarso16 · 2025-07-02T17:28:35Z

Why this should be merged

This adds much of the Firewood state database implementation, without adding it as a configurable option in coreth.

How this works

This reimplements everything with core/state/database.go by adding the Firewood backend for both reading and hashing from the tries (the Trie implementation) and for writing changes (the triedb.Backend implementation).

Note: starting this PR, Windows can no longer be built on coreth.

How this was tested

This is broken off from #959, which is currently bootstrapping to mainnet and Fuji. Additionally, there is a small amount of fuzz testing to ensure that hashing, reading, and committing works as expected.

Need to be documented?

No.

Need to update RELEASES.md?

No - this code is not yet available to consumers.

core/state/database.go

ceyonur · 2025-07-03T11:05:47Z

core/state/database.go

+	case *firewood.AccountTrie:
+		return t.Copy()
+	case *firewood.StorageTrie:
+		return nil // The storage trie just wraps the account trie, so we don't need to copy it separately.


should not we still call t.Copy (or t.AccountTrie.Copy) though? (bc AccountTrie is not an interface but struct)

We can't - all storage tries just point to the same account trie. If we copy the account trie (all the real information) and then copy the storage trie (just the pointer), then how will they know what account trie to use? They really should be re-allocated.

The only reason there's a storage trie at all in the first place is this interface - because we don't need to explicitly hash the storage tries (firewood does that internally), we only need the 1-tier trie

I see but I sense there is an invariant that this depends on:
an AccountTrie for the related StorageTrie will always be copied. If that so we should comment it somewhere.

I added some comments (and an explicit method).

This could still use the Copy method and return nil, but I think this is more clear.

core/state/database_test.go

triedb/firewood/account_trie.go

triedb/firewood/database.go

rkuris · 2025-07-03T19:43:21Z

core/state/firewood_database.go

+)
+
+var (
+	_ Database = (*firewoodAccessorDb)(nil)


I'm not sure why this check is needed. Since the first member of FirewoodAccessorDb is Database already, isn't it 100% guaranteed to be true?

Is this just a guard to prevent someone from removing Database from firewoodAccessorDb`?

Pretty much. My understanding is that this is good practice in general.

rkuris · 2025-07-03T19:47:19Z

plugin/evm/customrawdb/database_ext.go

+// It checks the rawdb package for HashDB or PathDB schemes.
+// If it's neither of those, it checks to see if the string is set to FirewoodScheme.


These comments don't seem to match the code, which checks for FirewoodScheme first, and then just delegates to ParseStateScheme, which might have it's own restrictions, possibly obsoleting this comment.

Suggest renaming to CheckForMismatchedStorageScheme or explaining why this is a bad name.

It's a perfectly reasonable name, but I think it should attempt to match the eth-specific name, like our other overloaded methods from libevm in these custom* packages. It does exactly the same thing as ParseStateScheme, except also checks for Firewood

plugin/evm/customrawdb/database_ext.go

rkuris · 2025-07-03T19:53:37Z

triedb/firewood/account_trie.go

+	// If the account has been deleted, we should return nil
+	if val, exists := a.dirtyKeys[string(accountKey)]; exists && len(val) == 0 {
+		return nil, nil
+	}


Seems like we can do this once we have the accountKey on line 88

rkuris · 2025-07-03T19:59:46Z

triedb/firewood/account_trie.go

+			return nil, nil
+		}
+		// Decode and return the updated storage value
+		_, decoded, _, err := rlp.Split(updateValue)


We don't check if rlp.Split returns a list and always assume it's a byte array. Not sure if this is okay or not, but it does seem strange.

The byte array is provided. This is following the example in libevm's trie/secure_trie.go

rkuris · 2025-07-03T20:08:19Z

triedb/firewood/account_trie.go

+	a.dirtyKeys[string(combinedKey[:])] = data
+	a.updateKeys = append(a.updateKeys, combinedKey[:])
+	a.updateValues = append(a.updateValues, data)
+	a.hasChanges = true // Mark that there are changes to commit


why can't this be inferred from a.DirtyKeys.len()?

This was a "small" optimization to avoid rehashing if we haven't made any changes. This is called 2-3 times every block without any additional changes, so this is just to avoid the call. As Cey noted, the dirty keys are never reset, and instead pile on top of each other. If we ever implement the AddToProposal API (which creates a proposal and commits on top of it in memory, tracking to ensure we drop properly), then we could just use the length

triedb/firewood/account_trie.go

triedb/firewood/database.go

rkuris · 2025-07-03T20:13:28Z

triedb/firewood/database.go

+			// We already have this proposal, but should create a new context with the correct hash.
+			// This solves the case of a unique block hash, but the same underlying proposal.
+			if _, exists := existing.Parent.Hashes[parentHash]; exists {
+				log.Debug("firewood: proposal already exists, updating hash", "root", root.Hex(), "parent", parentRoot.Hex(), "block", block, "hash", hash.Hex())


might want a metric here

We could, but it really shouldn't ever happen. This is just technically permitted, so it should be enforced

rkuris · 2025-07-03T20:14:20Z

triedb/firewood/database.go

+	db.proposalLock.Lock()
+	defer db.proposalLock.Unlock()


Is this too early? Can't we wait until we called ExtractTrieDBUpdatePayload?

Also when is this released? Can't we release it while firewood is processing the proposal?

I could move it a couple lines later.
However, we have to wait to release this until the end of the function. If we are proposing from a proposal during a commit, we have a race, so until we address that deeper in Firewood, we need to exclusively guarantee that only one of these operations is happening

triedb/firewood/database.go

Co-authored-by: Ceyhun Onur <[email protected]> Signed-off-by: Austin Larson <[email protected]>

plugin/evm/customrawdb/schema_ext.go

ceyonur

few nits and more cleanups + Initialized seems always returning true even for "uninitialized" dbs.

ceyonur · 2025-07-07T20:45:53Z

triedb/firewood/database.go

+
+type Config struct {
+	FilePath          string // Path to the directory where the Firewood database will be stored
+	FileName          string


can we use filepath as full filepath rather than taking filename+filepath?

ceyonur · 2025-07-07T20:55:42Z

triedb/firewood/database.go

+
+type Database struct {
+	fwDisk *ffi.Database  // The underlying Firewood database, used for storing proposals and revisions.
+	ethdb  ethdb.Database // The underlying disk database, used for storing genesis and the path.


seems this can be removed

ceyonur · 2025-07-07T20:56:02Z

triedb/firewood/database.go

+		config = Defaults
+	}
+
+	fwConfig, path, err := validateConfig(config)


this more looks like a getPath

ceyonur · 2025-07-07T21:05:03Z

triedb/firewood/database.go

+func (db *Database) Initialized(root common.Hash) bool {
+	// If the database is open, then the file is necessarily initialized.
+	// TODO: This would be more informative to have a flag to check that we've state synced later.
+	return db.fwDisk != nil


why don't we check fw.Root() != common.Hash?, this looks like we actually always return Initialized=true since fwDisk is never nil after New but if we haven't commited anything fw.Root still can be empty (and actually not initialized)

alarso16 added 2 commits July 2, 2025 13:51

feat: Add firewood as a database

fb74a97

fix: include customrawdb changes

b9ac3fa

alarso16 force-pushed the alarso16/firewood-trie branch from 0506448 to b9ac3fa Compare July 2, 2025 17:51

alarso16 marked this pull request as ready for review July 2, 2025 17:54

alarso16 requested a review from a team as a code owner July 2, 2025 17:54

alarso16 added enhancement New feature or request go Pull requests that update go code labels Jul 2, 2025

ceyonur reviewed Jul 3, 2025

View reviewed changes

alarso16 added 3 commits July 3, 2025 14:45

fix: comments

6cef181

fix: missing file

da16d49

chore: lint

b1c6f4c

rkuris reviewed Jul 3, 2025

View reviewed changes

alarso16 added 6 commits July 7, 2025 13:37

fix: address comments

15818d6

fix: remove unnecessary calls, clarify comments

808481f

fix: test

ce520fc

fix: use file initialization

ba977ef

fix: comment

fb7bd37

chore: merge

f5778d1

alarso16 requested review from rkuris and ceyonur July 7, 2025 20:31

ceyonur reviewed Jul 7, 2025

View reviewed changes

triedb/firewood/database.go Outdated Show resolved Hide resolved

Update triedb/firewood/database.go

a29ca82

Co-authored-by: Ceyhun Onur <[email protected]> Signed-off-by: Austin Larson <[email protected]>

ceyonur reviewed Jul 7, 2025

View reviewed changes

plugin/evm/customrawdb/schema_ext.go Outdated Show resolved Hide resolved

fix: lint

afafed3

ceyonur reviewed Jul 7, 2025

View reviewed changes

		// It checks the rawdb package for HashDB or PathDB schemes.
		// If it's neither of those, it checks to see if the string is set to FirewoodScheme.

feat: Add firewood as a database #1029

Are you sure you want to change the base?

feat: Add firewood as a database #1029

Uh oh!

Conversation

alarso16 commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Need to be documented?

Need to update RELEASES.md?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ceyonur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alarso16 commented Jul 2, 2025 •

edited

Loading