refactor(x/sync): remove need for passing proof nodes to completeWorkItem #4064

alarso16 · 2025-07-09T14:13:24Z

Why this should be merged

As a part of the refactor to allow Firewood to use x/sync, we should remove any proof logic from sync manager. This moves finding the next key to request to the proof commit implementation, as well as parsing the proof from bytes.

How this works

The completeWorkItem function does not need to know the largest handled key, but rather the next key that needs retrieved. This is handled through a hook, since even on some non-error returns, it shouldn't be called (see implementation). Instead of passing in the DB to the Sync Manager, it will take this ProofClient

How this was tested

Unit tests.

Need to be documented in RELEASES.md?

No.

Copilot

Pull Request Overview

This PR refactors the completeWorkItem function in the x/sync package to simplify work item completion by removing the need to pass proof nodes and calculating the next key to fetch within the proof response handlers instead.

Moves next key calculation logic from completeWorkItem to handleRangeProofResponse and handleChangeProofResponse
Simplifies completeWorkItem function signature by removing ctx and proofOfLargestKey parameters
Adds test coverage for the findNextKey function with identical keys edge case

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
x/sync/manager.go	Refactors proof response handlers to calculate next key before calling `completeWorkItem` and simplifies the `completeWorkItem` function signature
x/sync/sync_test.go	Adds new test case for `findNextKey` function when start and end keys are identical

x/sync/manager.go

x/sync/sync_test.go

alarso16 · 2025-07-15T18:41:07Z

x/sync/merkledb_client.go

+//
+// Invariant: [lastReceivedKey] < [rangeEnd].
+// If [rangeEnd] is Nothing it's considered > [lastReceivedKey].
+func (c *xSyncClient) findNextKey(


Everything this function and below was completely unmodified.

alarso16 · 2025-07-15T18:41:58Z

x/sync/proof_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func Test_Sync_FindNextKey_InSync(t *testing.T) {


These tests are unmodified besides initialization

These comments significantly reduce review time, ty for doing that!

alarso16 · 2025-07-15T18:42:31Z

x/sync/db.go

@@ -12,3 +19,18 @@ type DB interface {
 	merkledb.ChangeProofer
 	merkledb.RangeProofer
 }
+
+type ProofClient interface {


Any ideas for a better name?

SyncClient?

What do we define this interface for?

The x/sync manager should be able to work with other database implementations besides MerkleDB (specifically, Firewood). This is the smallest reasonable interface to allow other databases to hook in.

rkuris

This looks fine to me, not sure why we are removing BranchFactor16. We support this in firewood as well, although it is not a runtime option, it's compile time. We could check and refuse to open if the feature isn't enabled in a future release. For now, I think we can just implement it without BranchFactor16 working.

rkuris · 2025-07-15T20:51:26Z

x/sync/db.go

@@ -12,3 +19,18 @@ type DB interface {
 	merkledb.ChangeProofer
 	merkledb.RangeProofer
 }
+
+type ProofClient interface {


SyncClient?

rkuris · 2025-07-15T20:52:47Z

x/sync/db.go

+	merkledb.Clearer
+	merkledb.MerkleRootGetter


nit: naming: These are no longer merkledb interfaces, correct? Sure, they are implemented by merkledb but IIRC golang just lets you declare interfaces. If so, interfaces used by ProofClient (or SyncClient) should be maybe in a package named xsync or something.

rkuris · 2025-07-15T20:53:57Z

x/sync/manager.go


 	stateSyncNodeIdx uint32
 	metrics          SyncMetrics
 }

 // TODO remove non-config values out of this struct
 type ManagerConfig struct {
-	DB                    DB
+	ProofClient           ProofClient


nit: Does it make sense to just use ProofClient here so you aren't stuttering? Not sure if that makes sense.

rkuris · 2025-07-15T20:54:20Z

x/sync/manager.go

 	RangeProofClient      *p2p.Client
 	ChangeProofClient     *p2p.Client
 	SimultaneousWorkLimit int
 	Log                   logging.Logger
 	TargetRoot            ids.ID
-	BranchFactor          merkledb.BranchFactor


What happened to this?

Moved to ProofClient

rkuris · 2025-07-15T20:55:21Z

x/sync/manager.go

@@ -327,7 +315,7 @@ func (m *Manager) requestChangeProof(ctx context.Context, work *workItem) {

 	if work.localRootID == targetRootID {
 		// Start root is the same as the end root, so we're done.
-		m.completeWorkItem(ctx, work, work.end, targetRootID, nil)
+		m.completeWorkItem(work, work.end, targetRootID)


🏆 seems like not having context means we aren't doing any database work down there, which I think is good!

rkuris · 2025-07-15T20:57:19Z

x/sync/proof_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func Test_Sync_FindNextKey_InSync(t *testing.T) {


These comments significantly reduce review time, ty for doing that!

joshua-kim · 2025-07-16T17:22:12Z

x/sync/db.go

@@ -12,3 +19,18 @@ type DB interface {
 	merkledb.ChangeProofer
 	merkledb.RangeProofer
 }
+
+type ProofClient interface {


What do we define this interface for?

joshua-kim · 2025-07-16T17:24:22Z

x/sync/merkledb_client.go

+	BranchFactor merkledb.BranchFactor
+}
+
+type xSyncClient struct {


This abstraction feels very difficult to reason about and there doesn't seem to be a clear abstraction boundary defined - what is this type intended to represent and what is it's responsibility? It's not really a client - it doesn't make any requests, but seems to define handlers for how to interpret responses + you can interact with the underlying database it's coupled to.

It isn't really a client, but I didn't know what to call it. The goal was to remove the dependence on database and proof implementation (specifically merkledb) from the sync manager. The x/sync manager in particular shouldn't depend on any specific database/proof implementation

github-project-automation bot added this to avalanchego Jul 9, 2025

alarso16 added merkledb go Pull requests that update Go code labels Jul 9, 2025

alarso16 force-pushed the alarso16/move-next-key branch 2 times, most recently from 6936bbb to 4a3dae0 Compare July 9, 2025 16:41

alarso16 marked this pull request as ready for review July 9, 2025 16:42

Copilot AI review requested due to automatic review settings July 9, 2025 16:42

alarso16 requested review from joshua-kim and rrazvan1 as code owners July 9, 2025 16:42

Copilot AI reviewed Jul 9, 2025

View reviewed changes

x/sync/manager.go Outdated Show resolved Hide resolved

x/sync/manager.go Outdated Show resolved Hide resolved

joshua-kim reviewed Jul 9, 2025

View reviewed changes

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

alarso16 requested a review from StephenButtolph as a code owner July 11, 2025 15:08

alarso16 force-pushed the alarso16/move-next-key branch 8 times, most recently from 86765a8 to ab1bd24 Compare July 15, 2025 18:51

alarso16 added 2 commits July 15, 2025 14:51

refactor: decouple work enqueuing from proofs

a1bc0cb

refactor: abstract client-side proof logic

c8e2fd9

alarso16 force-pushed the alarso16/move-next-key branch from ab1bd24 to c8e2fd9 Compare July 15, 2025 18:52

alarso16 commented Jul 15, 2025

View reviewed changes

rkuris approved these changes Jul 15, 2025

View reviewed changes

joshua-kim reviewed Jul 16, 2025

View reviewed changes

fix: remove callback

e25ace4

refactor(x/sync): remove need for passing proof nodes to completeWorkItem #4064

Are you sure you want to change the base?

refactor(x/sync): remove need for passing proof nodes to completeWorkItem #4064

Uh oh!

Conversation

alarso16 commented Jul 9, 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

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!

joshua-kim Jul 16, 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!

rkuris 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!

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!

joshua-kim Jul 16, 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!

joshua-kim Jul 16, 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!

Uh oh!

alarso16 commented Jul 9, 2025 •

edited

Loading

joshua-kim Jul 16, 2025 •

edited

Loading

joshua-kim Jul 16, 2025 •

edited

Loading

joshua-kim Jul 16, 2025 •

edited

Loading