Firewood static sync #4361

alarso16 · 2025-09-29T19:38:15Z

Why this should be merged

This is the first of many PRs to enable state-syncing Firewood in the EVM. Although not in the perfect state to be used, this marks being able to create the xsync.Manager object with range proofs, and this functionality is tested.

How this works

Creates the interfaces needed to be a xsync.DB, with stubs for everything change-proof related. Additionally, the FFI's FindNextKey simply returns the last key in the range proof, so there's some hacky stuff to make that work

How this was tested

New UT

Need to be documented in RELEASES.md?

No

github-actions · 2025-11-02T00:00:39Z

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

…ange-proofs

x/firewood/sync_test.go

alarso16 · 2025-12-08T21:17:58Z

x/firewood/sync_test.go

+
+	require.NoError(syncer.Start(ctx))
+	err = syncer.Wait(ctx)
+	if errors.Is(err, xsync.ErrFinishedWithUnexpectedRoot) {


The most common error I encountered while trying to make this work as a timeout, so I wonder if making a context with a timeout to log differences would be more beneficial than letting it stall for 2 minutes without helpful logs

I'm not aware of a great way of doing this - if we use our own timeout logic we don't respect the runtime's testing timeout and although t.Deadline exists trying to fire something right before the timeout is race-y.

Interesting. I'll leave it for now, and see if we run into more issues like this in the future.

x/firewood/sync_db.go

alarso16 · 2025-12-08T21:22:15Z

x/firewood/sync_db.go

+}
+
+// Commit the range proof to the database.
+func (db *syncDB) CommitRangeProof(_ context.Context, start, end maybe.Maybe[[]byte], proof *RangeProof) (maybe.Maybe[[]byte], error) {


I wouldn't look to closely at this function. it's all a little janky until Firewood has a proper FindNextKey implementation, in which case we don't have to copy the byte slice, and we don't have to analyze the returned values.

Copilot

Pull request overview

This PR implements initial Firewood static sync functionality by integrating Firewood with the xsync.Manager. The implementation enables creating range proofs and syncing database state, though change proofs remain unimplemented stubs.

Key Changes:

Added xsync.DB interface implementation for Firewood with range proof support
Introduced EmptyRoot configuration parameter to handle empty state checks
Implemented comprehensive sync testing with various database sizes

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
x/sync/network_server.go	Added compile-time interface checks for handler types
x/sync/manager.go	Added EmptyRoot config field and updated empty state checks
x/firewood/sync_test.go	Added comprehensive sync tests for empty and populated databases
x/firewood/sync_db.go	Implemented xsync.DB interface with range proof support and change proof stubs
x/firewood/proof.go	Added proof marshaling/unmarshaling implementations
go.mod	Changed firewood-go-ethhash/ffi from indirect to direct dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

x/firewood/sync_test.go

x/firewood/proof.go

joshua-kim · 2025-12-09T19:03:01Z

x/sync/firewood/proof.go

+	return nil, errors.New("not implemented")
+}
+
+type ChangeProof struct{}


nit: We could remove this struct definition and satisfy the generic w/ struct{} in places where this is used to avoid prematurely introducing this type - but the intent of this type is pretty clear so it's also fine to leave this as a TODO.

I did consider that, but it felt more intuitive to provide the infrastructure here, since we need some dummy marshaller anyway

We do need the dummy marshaler but this type isn't needed - we don't implement change proofs so struct{}{} works as a sentinel value (and actually this type being exposed out of firewood/syncer suggests that they are implemented).

joshua-kim · 2025-12-09T19:20:06Z

x/firewood/proof.go

+type RangeProof struct {
+	ffi       *ffi.RangeProof


nit: I think we went with ffi as a name to make it clear that we're talking to the ffi - but although it makes clear the domain boundary we're communicating across it makes it more unclear what the type we're working with is (r.ffi.MarshalBinary() reads like we're serializing the ffi).

Some recommendations for other names would be:

rangeProof - verbose, defaulting to the type name is never bad

ffiRangeProof - if you want to highlight that we're working with the ffi's version of this domain type

inner - since this is a wrapper pattern

r or rp - Can be used for shorter scoped code and since this a wrapper pattern we can argue that the context is given by the wrapping type so r.r.MarshalBinary() is clearly us marshaling the type we wrap.

If I was the author I would probably have used r - but this comment is entering opinionated territory so don't feel bound to my preferences.

Ah that is unclear. I'll choose rp out of preference, but I don't mind any of those names

x/firewood/proof.go

x/firewood/sync_test.go

joshua-kim · 2025-12-09T20:42:09Z

x/sync/syncer.go

+		work.start,
+		work.end,


is this a separate diff/bugfix?

It's just unnecessary to convert from proto when we have it right there. So yes? I forgot this was in the diff, I can remove it

We should avoid unrelated changes where possible - I'm open to merging this in separately but we want to keep PRs scoped to a logical change. As an example reverting a feature by reverting the commit inadvertently also reverting unrelated cleanups means that we now need to manually revert a diff.

joshua-kim · 2025-12-09T20:42:35Z

x/sync/syncer.go

 	SimultaneousWorkLimit int
 	Log                   logging.Logger
 	TargetRoot            ids.ID
+	EmptyRoot             ids.ID


What is firewood's value for the empty root? Is it not just 32 zeroes?

Ethereum's empty root is not 32 zeros. An empty trie has the following hash:
https://github.com/ava-labs/libevm/blob/749b6cefda2894bab9bbdbe2d27086d57ca0d393/core/types/hashes.go#L27

joshua-kim · 2025-12-09T20:44:54Z

x/sync/firewood/sync_db.go

+	return nil, errors.New("change proofs are not implemented")
+}
+
+//nolint:revive // TODO: implement this method.


What linter are we breaking here?

There's unused variables. I left them for clarity, so you don't have to go look at the interface definition at change-proof time to know what ids.ID is

We should listen to the linter here - it doesn't matter what the parameters are/mean because they're not used and this code is clear/self-documenting enough to convey this to the reader

x/firewood/sync_test.go

joshua-kim · 2025-12-15T21:05:37Z

x/sync/firewood/sync_db.go

+type Config struct {
+	RangeProofClient      *p2p.Client
+	ChangeProofClient     *p2p.Client
+	SimultaneousWorkLimit int
+	Log                   logging.Logger
+	TargetRoot            ids.ID
+	StateSyncNodes        []ids.NodeID
+}


x/sync's Config type is abused as a DI container + a go config pattern - can we abstract over this to avoid leaking the bad implementation into this package? The proof clients are really dependencies of the syncer and the TargetRoot is really a parameter for which to start the syncer - I feel like we could probably do something like have Config know about stuff with safe zero values that have reasonable defaults to make tests easier to write + each VM integrating with this won't have to re-define their own defaults (which will all probably be the same). I think SimultaneousWorkLimit and StateSyncNodes have reasonable default values, and the logger can default to not logging. We could also pass the metrics instance through the config and the default can be a new registry - only prod cares to configure to use its own metrics instance.

TargetRoot i actually think is not a true config value because as part of state sync we need to define the point to sync to - so I think the caller should be explicitly providing this through a parameter. Similarly I think there aren't reasonable default values for the network clients so those should be defined explicitly as well.

joshua-kim · 2025-12-15T21:06:36Z

x/sync/firewood/sync_db.go

+	StateSyncNodes        []ids.NodeID
+}
+
+func NewSyncer(fw *ffi.Database, config Config, register prometheus.Registerer) (*xsync.Syncer[*RangeProof, *ChangeProof], error) {


nit: fw is deviating from the abstractions of ffi and Database - would calling this something like db make more sense? Similarly register might want to be registerer or r.

Locality for NewSyncer and Config's definition might want to be after all of the function definitions on db or before db's definition - currently this locality reads like a constructor for db.

joshua-kim · 2025-12-15T21:07:56Z

x/sync/firewood/sync_db.go

Should we update the file name here? Maybe syncer?

joshua-kim · 2025-12-15T21:10:51Z

x/sync/firewood/sync_db.go

+		// No error indicates the range is complete.
+		return maybe.Nothing[[]byte](), err


Although semantically correct - this is not clear/deviates from Go's typical pattern of error handling where err typically implies a non-nil error. This can also cause unexpected behavior if the ffi returns a non-nil next key range and a non-nil error (we shouldn't because it breaks the aforementioned idiom of a meaningful error and a meaningful return value, but our repository breaks this idiom without good reason at times). At any rate - I would break this into two separate if's for clarity.

joshua-kim · 2025-12-15T21:15:08Z

x/sync/firewood/sync_db.go

+	return nil, errors.New("change proofs are not implemented")
+}
+
+//nolint:revive // TODO: implement this method.


We should listen to the linter here - it doesn't matter what the parameters are/mean because they're not used and this code is clear/self-documenting enough to convey this to the reader

joshua-kim · 2025-12-15T21:32:29Z

x/sync/firewood/sync_test.go

+	for _, serverSize := range []int{0, 1, 1_000, 10_000, 100_000} {
+		for _, clientSize := range []int{0, 1_000} {
+			t.Run(fmt.Sprintf("numKeys=%d_clientKeys=%d", serverSize, clientSize), func(t *testing.T) {


This works but it's deviating from the idiomatic table pattern - could we use a testing table instead? They tend to be easier to maintain over time and are more easily readable and don't require the string formatting that we're doing.

joshua-kim · 2025-12-15T21:33:02Z

x/sync/firewood/sync_test.go

+			SimultaneousWorkLimit: 5,
+			Log:                   logging.NoLog{},


Related to my earlier comment about defining some defaults - but it would simplify this test setup

joshua-kim · 2025-12-15T21:38:30Z

x/sync/firewood/sync_test.go

+		ctx := context.WithoutCancel(t.Context())
+		ctx, cancel := context.WithTimeout(ctx, 5*time.Second) // allow some time for garbage collection
+		defer cancel()


Isn't this racey? If Close takes more than 5 seconds we can get a false positive by canceling early. Isn't it safe for us to timeout while closing because these are temporary db instances for testing purposes? If we mess up closure subsequent runs of the tests should not be impacted.

If there is a memory management bug in firewood - it's also not something that the syncer tests should care about anyways - it's an implementation detail of firewood. If we need better ways of debugging it I think we would want to add tests on Close closer to the ffi/firewood implementation than adding complexity to the syncer tests.

joshua-kim · 2025-12-15T21:39:21Z

x/sync/firewood/sync_test.go

+		t.Logf("%d keys missing from DB1 starting with %x", missingCount, prevKey)
+	}
+
+	t.Logf("DB1 had %d keys, DB2 had %d keys", count1, count2)


Should we update some logs/naming of db1/db2 to be something like got and want to make it clear what the expected value was?

joshua-kim · 2025-12-15T21:42:26Z

x/sync/syncer.go

+		work.start,
+		work.end,


We should avoid unrelated changes where possible - I'm open to merging this in separately but we want to keep PRs scoped to a logical change. As an example reverting a feature by reverting the commit inadvertently also reverting unrelated cleanups means that we now need to manually revert a diff.

joshua-kim · 2025-12-15T22:19:50Z

x/sync/firewood/proof.go

+	return nil, errors.New("not implemented")
+}
+
+type ChangeProof struct{}


We do need the dummy marshaler but this type isn't needed - we don't implement change proofs so struct{}{} works as a sentinel value (and actually this type being exposed out of firewood/syncer suggests that they are implemented).

misclick

github-project-automation bot added this to avalanchego Sep 29, 2025

alarso16 moved this to In Progress 🏗️ in avalanchego Sep 29, 2025

alarso16 removed this from avalanchego Sep 29, 2025

alarso16 linked an issue Sep 29, 2025 that may be closed by this pull request

Firewood database in x/sync - Range Proof only #4324

Open

github-actions bot added the lifecycle/stale Inactive for 60 days label Nov 2, 2025

alarso16 added lifecycle/frozen and removed lifecycle/stale Inactive for 60 days labels Nov 3, 2025

alarso16 force-pushed the alarso16/firewood-range-proofs branch from 34c131b to 5bed3e3 Compare November 19, 2025 19:04

alarso16 added 2 commits November 25, 2025 16:54

feat: Add x/sync for Firewood

ed246fd

fix: FindNextKey

2375dd7

alarso16 force-pushed the alarso16/firewood-range-proofs branch from 5bed3e3 to 2375dd7 Compare November 25, 2025 21:54

alarso16 added testing This primarily focuses on testing go Pull requests that update Go code coreth labels Nov 25, 2025

github-project-automation bot added this to avalanchego Nov 25, 2025

alarso16 added 4 commits December 5, 2025 10:55

Fix findnextkey, add logging on error

b75a7bb

Fix logger

f9a679a

fix for review

3205a33

Merge remote-tracking branch 'origin/master' into alarso16/firewood-r…

b20c207

…ange-proofs

alarso16 added storage This involves storage primitives and removed testing This primarily focuses on testing lifecycle/frozen labels Dec 5, 2025

alarso16 self-assigned this Dec 5, 2025

alarso16 added 3 commits December 8, 2025 16:00

fix: key length

f9ea6d5

feat: Add Clear()

3dd1c19

style: lint

8a8dcf9

alarso16 commented Dec 8, 2025

View reviewed changes

alarso16 marked this pull request as ready for review December 8, 2025 21:24

alarso16 requested a review from joshua-kim as a code owner December 8, 2025 21:24

Copilot AI review requested due to automatic review settings December 8, 2025 21:24

alarso16 requested review from StephenButtolph and rrazvan1 as code owners December 8, 2025 21:24

Copilot AI reviewed Dec 8, 2025

View reviewed changes

x/firewood/sync_test.go Outdated Show resolved Hide resolved

alarso16 added 2 commits December 8, 2025 16:26

fix: spelling

af00ec7

Merge branch 'master' into alarso16/firewood-range-proofs

9e6690e

alarso16 moved this to In Progress 🏗️ in avalanchego Dec 9, 2025

joshua-kim reviewed Dec 9, 2025

View reviewed changes

x/firewood/sync_test.go Outdated Show resolved Hide resolved

alarso16 added 4 commits December 9, 2025 15:56

Merge branch 'master' into alarso16/firewood-range-proofs

99eded2

Updates from new version

0a9d8ef

fix: Address comments

731582f

style: marshal scope

c45e765

alarso16 requested review from joshua-kim and rkuris December 9, 2025 22:05

refactor: Move into x/sync

3577fb4

github-actions bot removed the coreth label Dec 12, 2025

JonathanOppenheimer added the coreth Related to the former coreth standalone repository label Dec 15, 2025

alarso16 removed the coreth Related to the former coreth standalone repository label Dec 15, 2025

joshua-kim reviewed Dec 15, 2025

View reviewed changes

joshua-kim previously approved these changes Dec 15, 2025

View reviewed changes

		// No error indicates the range is complete.
		return maybe.Nothing[[]byte](), err

Firewood static sync #4361

Are you sure you want to change the base?

Firewood static sync #4361

Conversation

alarso16 commented Sep 29, 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 in RELEASES.md?

Uh oh!

github-actions bot commented Nov 2, 2025

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

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!

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!

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

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!

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!

alarso16 commented Sep 29, 2025 •

edited

Loading

alarso16 Dec 9, 2025 •

edited

Loading