Move atomic sync #723

ceyonur · 2024-12-28T17:21:08Z

Why this should be merged

Decouples atomic logic from syncervm server/client to atomic pkg.

How this works

This pull request introduces a new atomic sync mechanism for the EVM plugin. The changes include the creation of new types and interfaces for atomic sync summaries, along with the implementation of the sync process. The most important changes are summarized below:

Introduction of Atomic Sync Mechanism:

plugin/evm/atomic/atomic_sync_extender.go: Added the atomicSyncExtender type and its methods to handle the synchronization process, including Sync, OnFinishBeforeCommit, and OnFinishAfterCommit.
plugin/evm/atomic/atomic_sync_provider.go: Introduced the atomicSyncProvider type to provide state summaries at specific heights.
plugin/evm/atomic/syncable.go: Defined the AtomicBlockSyncSummary type and related functions to parse and create sync summaries.

Refactoring and Code Organization:

plugin/evm/message/codec.go: Updated the codec manager to register the new sync summary type and introduced a variable to hold the sync summary type. [1] [2]
plugin/evm/message/syncable.go: Refactored the BlockSyncSummary to implement the Syncable interface and added a parser for sync summaries.

File and Type Renaming:

plugin/evm/sync/syncervm_client.go: Renamed from plugin/evm/syncervm_client.go and updated to use the new Syncable interface and atomicSyncExtender for the sync process. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]

How this was tested

Existing UTs should cover

Need to be documented?

No

Need to update RELEASES.md?

No

ceyonur · 2024-12-28T17:22:03Z

plugin/evm/sync/syncervm_client.go

@@ -74,13 +79,13 @@ type StateSyncClientConfig struct {
 type stateSyncerClient struct {
 	*StateSyncClientConfig

-	resumableSummary message.SyncSummary
+	resumableSummary message.Syncable


this can be nil and we do check it

ceyonur · 2024-12-28T17:22:08Z

plugin/evm/sync/syncervm_client.go


 	cancel context.CancelFunc
 	wg     sync.WaitGroup

 	// State Sync results
-	syncSummary  message.SyncSummary
+	syncSummary  message.Syncable


this can be nil, but we never use it when it's nil.

ceyonur · 2024-12-30T08:24:08Z

plugin/evm/message/codec.go

-var Codec codec.Manager
+var (
+	Codec codec.Manager
+	// TODO: Remove this once we have a better way to register types (i.e use a different codec version or use build flags)


we can move init to a function like (Initialize) and call in vm.Initialize like Codec.Initialize(SyncSummaryType), but then tests should be doing the same thing everytime. But it might be the only way once we merge subnet-evm since two VMs should be using two different codecs since subnet-evm do not have atomic types (and thus codec has different order of type IDs).

plugin/evm/message/syncable.go

plugin/evm/vm.go

Signed-off-by: Ceyhun Onur <[email protected]>

…ve-atomic-sync

qdm12

Nice tidying up 😉 And thanks for that thick detailed PR body 👍

I just left a few comments, mostly non-important things to be fair. Most of them you can apply as suggestions directly or discard them in case we should keep similar code with subnet-evm at this time.

plugin/evm/atomic/atomic_sync_extender.go

qdm12 · 2024-12-30T13:20:36Z

plugin/evm/atomic/atomic_sync_extender.go

+	log.Info("atomic tx: sync starting", "root", atomicSyncSummary)
+	atomicSyncer, err := a.backend.Syncer(client, atomicSyncSummary.AtomicRoot, atomicSyncSummary.BlockNumber, a.stateSyncRequestSize)
+	if err != nil {
+		return err


Suggested change

return err

return fmt.Error("creating syncer: %s", err)

PS: I also used to (like you I think?) wrap all the errors with %w so the callers can use errors.Is if they want to, until very recently. The problem is you then expose API that might not even come from your own package (Arran pointed me to this), so having them as dumb strings %s is better, unless you actually need to check the errors programmatically.

but isn't that the case if we use %s in any of chained (wrapped) errs, then we would lose all the ability to check with errors.Is?

IMO:

There's no need to wrap all errors. It seems quite cumbersome to wrap every single error and in shared codebase with geth is doesn't seem feasible. I'm not sold on the advantages.

Seems best to wrap the error (%w) for consistency with the case that doesn't wrap the error (return err)

Given shared code, I suggest we follow the current convention of adding a wrapped error when the context has proven useful for debugging.

Additionally the geth codebase use a mix of panics, returning wrapped & unwrapped errs, and log.Crit, so I don't think this is an issue we need to address now.

There's no need to wrap all errors. It seems quite cumbersome to wrap every single error
Additionally the geth codebase use a mix of panics, returning wrapped & unwrapped errs, and log.Crit, so I don't think this is an issue we need to address now.

I disagree, wrapping errors helps debugging or the user understand an error message. For example, I saw recently avalanche-cli complain about some obscure error, which, as a user, was very confusing, and, as someone who looked in the code, I couldn't really figure out where it was coming from either. Of course we don't want to modify geth code, but I don't think we should not make our part of the code better (unless geth checks the error with ==, which really it shouldn't - I might PR this to geth)

Seems best to wrap the error (%w) for consistency with the case that doesn't wrap the error (return err)

Actually I think geth do check errors directly with == whereas wrapping errors with %w only works with errors.Is, not ==, so two different operations, although errors.Is works with unwrapped (sentinel) errors. Really geth should always use errors.Is (works for both cases) and not == to check errors.

plugin/evm/message/syncable.go

plugin/evm/vm.go

darioush · 2025-01-06T19:52:22Z

plugin/evm/atomic/atomic_sync_provider.go

+
+	"github.com/ava-labs/avalanchego/snow/engine/snowman/block"
+
+	"github.com/ava-labs/coreth/core"


We shouldn't import core in the atomic package this will couple it to libevm bindings again.
IMO we can have a pacakge like atomic/sync.

See #723 (comment)

qdm12 · 2025-01-14T14:21:08Z

plugin/evm/atomic/sync/atomic_sync_extender.go

+	stateSyncRequestSize uint16
+}
+
+func NewAtomicSyncExtender(backend state.AtomicBackend, stateSyncRequestSize uint16) *AtomicSyncExtender {


nit you could define an interface AtomicBackend in this package instead of using state.AtomicBackend so that:

reduces dependency on state package

narrower interface: you only need the Root method here

I moved them to interfaces pkg.

qdm12 · 2025-01-14T14:22:55Z

plugin/evm/atomic/sync/atomic_sync_provider.go

+var _ sync.SummaryProvider = &AtomicSyncProvider{}
+
+type AtomicSyncProvider struct {
+	chain      *core.BlockChain


you could define an interface Chain for this chain field with only GetBlockByNumber and HasState, so that it removes the dependency on the core package.

unfortunately it goes deeper than that as GetBlockByNumber returns types.Block so we end-up importing the core package anyway. We can create an adapter and wrap the passed blockchain here, but I think having a separate pkg for sync should be good enough.

qdm12 · 2025-01-14T14:30:28Z

plugin/evm/atomic/sync/atomic_sync_extender.go

+	}
+}
+
+func (a *AtomicSyncExtender) Sync(ctx context.Context, client syncclient.Client, verDB *versiondb.Database, syncSummary message.Syncable) error {


client syncclient.Client can be the narrower interface client syncclient.LeafClient (Extender interface needs that update as well).

qdm12 · 2025-01-14T14:31:46Z

plugin/evm/atomic/sync/atomic_sync_extender.go

+		a.stateSyncRequestSize,
+	)
+	if err != nil {
+		return err


nit

Suggested change

return err

return fmt.Errorf("failed to create atomic syncer: %w", err)

qdm12 · 2025-01-14T14:33:47Z

plugin/evm/atomic/sync/atomic_sync_provider.go

+		return nil, fmt.Errorf("atomic trie root not found for height (%d)", height)
+	}
+
+	blk := a.chain.GetBlockByNumber(height)


nit perhaps (makes more sense to me I'd say)

alias block to snowblock

then rename blk to block

seems weird to rename a package import to avoid namespace collision with a local variable. (seems one would usually rename a local variable to avoid aliasing an imported package)

let's keep this as it is.

qdm12 · 2025-01-14T14:39:13Z

plugin/evm/message/syncable.go

+// BlockSyncSummary provides the information necessary to sync a node starting
 // at the given block.
-type SyncSummary struct {
+type BlockSyncSummary struct {


did we need to rename SyncSummary to BlockSyncSummary? 🤔
It seems like unneeded changes, unless there is a reason?

This is to distinguish between AtomicBlockSyncSummary

darioush · 2025-01-14T21:19:58Z

plugin/evm/atomic/sync/atomic_sync_provider.go

+		return nil, fmt.Errorf("atomic trie root not found for height (%d)", height)
+	}
+
+	blk := a.chain.GetBlockByNumber(height)


seems weird to rename a package import to avoid namespace collision with a local variable. (seems one would usually rename a local variable to avoid aliasing an imported package)

darioush · 2025-01-14T21:28:59Z

plugin/evm/syncervm_test.go

@@ -56,7 +57,7 @@ func TestSkipStateSync(t *testing.T) {
 		stateSyncMinBlocks: 300, // must be greater than [syncableInterval] to skip sync
 		syncMode:           block.StateSyncSkipped,
 	}
-	vmSetup := createSyncServerAndClientVMs(t, test, StateSyncParentsToFetch)
+	vmSetup := createSyncServerAndClientVMs(t, test, vmsync.StateSyncParentsToFetch)


seems we can rename these to drop the StateSync prefix with this refactor

darioush · 2025-01-14T21:30:04Z

plugin/evm/vm.go

+	vmsync.StateSyncServer
+	vmsync.StateSyncClient


seems these can also be renamed to vmsync.Server / vmsync.Client

darioush · 2025-01-14T21:33:59Z

plugin/evm/atomic/sync/syncable.go

+}
+
+func init() {
+	message.SyncSummaryType = &AtomicBlockSyncSummary{}


how does this work in conjunction with the call to .RegisterType(SyncSummaryType)?

this is indeed an ugly assumption that assumes var is called before init blocks.

darioush · 2025-01-14T21:39:42Z

plugin/evm/sync/syncervm_client.go

+	if err := client.ExtraSyncer.OnFinishBeforeCommit(client.LastAcceptedHeight, client.syncSummary); err != nil {
 		return err
 	}


optional: maybe move this out of updateVMMarkers so OnFinishBeforeCommit/OnFinishAfterCommit are closer to each other.

ceyonur added 3 commits December 28, 2024 20:19

decouple atomic logic from statesyncers

f17d97c

remove unused provider

b5777f4

Merge branch 'move-atomic-state' into move-atomic-sync

d45cd2e

ceyonur commented Dec 30, 2024

View reviewed changes

ceyonur added 4 commits December 30, 2024 11:29

Update plugin/evm/vm.go

4ef96e8

Signed-off-by: Ceyhun Onur <[email protected]>

Update plugin/evm/vm.go

caa305d

Signed-off-by: Ceyhun Onur <[email protected]>

Merge branch 'move-atomic-state' into move-atomic-sync

104bab7

Merge branch 'move-atomic-sync' of github.com:ava-labs/coreth into mo…

62479e3

…ve-atomic-sync

ceyonur marked this pull request as ready for review December 30, 2024 10:19

ceyonur requested review from darioush and a team as code owners December 30, 2024 10:19

qdm12 reviewed Dec 30, 2024

View reviewed changes

ceyonur added 3 commits January 6, 2025 15:52

Reviews

f651bad

Merge branch 'move-atomic-state' into move-atomic-sync

39da24e

Merge branch 'move-atomic-state' into move-atomic-sync

6d54408

ceyonur self-assigned this Jan 6, 2025

ceyonur added the atomic refactor label Jan 6, 2025

darioush requested changes Jan 6, 2025

View reviewed changes

darioush mentioned this pull request Jan 6, 2025

atomic syncer: simplify interface #735

Merged

Base automatically changed from move-atomic-state to seperate-atomic-pkg-base January 7, 2025 13:45

Merge branch 'seperate-atomic-pkg-base' into move-atomic-sync

22591f3

qdm12 reviewed Jan 14, 2025

View reviewed changes

darioush reviewed Jan 14, 2025

View reviewed changes

Reviews

7a3b1b7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move atomic sync #723

Move atomic sync #723

ceyonur commented Dec 28, 2024 •

edited

Loading

ceyonur Dec 28, 2024

ceyonur Dec 28, 2024

ceyonur Dec 30, 2024

qdm12 left a comment

qdm12 Dec 30, 2024 •

edited

Loading

ceyonur Jan 3, 2025

darioush Jan 6, 2025 •

edited

Loading

qdm12 Jan 14, 2025

darioush Jan 6, 2025

qdm12 Jan 14, 2025

qdm12 Jan 14, 2025

ceyonur Jan 15, 2025

qdm12 Jan 14, 2025

ceyonur Jan 15, 2025

qdm12 Jan 14, 2025 •

edited

Loading

ceyonur Jan 15, 2025

qdm12 Jan 14, 2025

ceyonur Jan 15, 2025

qdm12 Jan 14, 2025

darioush Jan 14, 2025

ceyonur Jan 15, 2025

qdm12 Jan 14, 2025 •

edited

Loading

ceyonur Jan 15, 2025

darioush Jan 14, 2025

darioush Jan 14, 2025

darioush Jan 14, 2025

darioush Jan 14, 2025

ceyonur Jan 15, 2025

darioush Jan 14, 2025


		"github.com/ava-labs/avalanchego/snow/engine/snowman/block"

		"github.com/ava-labs/coreth/core"

	return err
	return fmt.Errorf("failed to create atomic syncer: %w", err)

Move atomic sync #723

Are you sure you want to change the base?

Move atomic sync #723

Conversation

ceyonur commented Dec 28, 2024 • edited Loading

Why this should be merged

How this works

Introduction of Atomic Sync Mechanism:

Refactoring and Code Organization:

File and Type Renaming:

How this was tested

Need to be documented?

Need to update RELEASES.md?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 left a comment

Choose a reason for hiding this comment

qdm12 Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceyonur commented Dec 28, 2024 •

edited

Loading

qdm12 Dec 30, 2024 •

edited

Loading

darioush Jan 6, 2025 •

edited

Loading

qdm12 Jan 14, 2025 •

edited

Loading

qdm12 Jan 14, 2025 •

edited

Loading