revert config addition + simplify logging

Author: julienrbrt

block/internal/syncing/syncer.go

@@ -5,7 +5,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -261,22 +260,12 @@ func (s *Syncer) syncLoop() {
 		// Process pending events from cache on every iteration
 		s.processPendingEvents()
 
-		// Fetch events from DA layer first and optimistically p2p when necessary.
-		// This is the default behavior.
-		if !s.config.Sync.PreferP2P {
-			if s.tryFetchFromDA(nextDARequestAt) {
-				continue
-			}
-			if s.tryFetchFromP2P(lastHeaderHeight, lastDataHeight, blockTicker.C) {
-				continue
-			}
-		} else {
-			if s.tryFetchFromP2P(lastHeaderHeight, lastDataHeight, blockTicker.C) {
-				continue
-			}
-			if s.tryFetchFromDA(nextDARequestAt) {
-				continue
-			}
+		if s.tryFetchFromDA(nextDARequestAt) {
+			continue
+		}
+
+		if s.tryFetchFromP2P(lastHeaderHeight, lastDataHeight, blockTicker.C) {
+			continue
 		}
 
 		// Prevent busy-waiting when no events are available
@@ -309,17 +298,14 @@ func (s *Syncer) tryFetchFromDA(nextDARequestAt *time.Time) bool {
 		}
 
 		// Back off exactly by DA block time to avoid overloading
-		hffDelay := s.config.DA.BlockTime.Duration
-		if hffDelay <= 0 {
-			hffDelay = 2 * time.Second
+		backoffDelay := s.config.DA.BlockTime.Duration
+		if backoffDelay <= 0 {
+			backoffDelay = 2 * time.Second
 		}
-		*nextDARequestAt = now.Add(hffDelay)
+		*nextDARequestAt = now.Add(backoffDelay)
+
+		s.logger.Error().Err(err).Dur("delay", backoffDelay).Uint64("da_height", daHeight).Msg("failed to retrieve from DA; backing off DA requests")
 
-		if s.isHeightFromFutureError(err) {
-			s.logger.Debug().Dur("delay", hffDelay).Uint64("da_height", daHeight).Msg("height from future; backing off DA requests")
-		} else {
-			s.logger.Error().Err(err).Dur("delay", hffDelay).Uint64("da_height", daHeight).Msg("failed to retrieve from DA; backing off DA requests")
-		}
 		return false
 	}
 
@@ -583,24 +569,6 @@ func (s *Syncer) sendNonBlockingSignal(ch chan struct{}, name string) {
 	}
 }
 
-// isHeightFromFutureError checks if the error is a height from future error
-func (s *Syncer) isHeightFromFutureError(err error) bool {
-	if err == nil {
-		return false
-	}
-	if errors.Is(err, coreda.ErrHeightFromFuture) || errors.Is(err, common.ErrHeightFromFutureStr) {
-		return true
-	}
-	msg := err.Error()
-	if msg == "" {
-		return false
-	}
-	if strings.Contains(msg, coreda.ErrHeightFromFuture.Error()) || strings.Contains(msg, common.ErrHeightFromFutureStr.Error()) {
-		return true
-	}
-	return false
-}
-
 // processPendingEvents fetches and processes pending events from cache
 // optimistically fetches the next events from cache until no matching heights are found
 func (s *Syncer) processPendingEvents() {

block/internal/syncing/syncer_test.go

@@ -3,7 +3,6 @@ package syncing
 import (
 	"context"
 	crand "crypto/rand"
-	"errors"
 	"testing"
 	"time"
 
@@ -191,18 +190,6 @@ func TestSequentialBlockSync(t *testing.T) {
 	assert.True(t, ok)
 }
 
-func TestSyncer_isHeightFromFutureError(t *testing.T) {
-	s := &Syncer{}
-	// exact error
-	err := common.ErrHeightFromFutureStr
-	assert.True(t, s.isHeightFromFutureError(err))
-	// string-wrapped error
-	err = errors.New("some context: " + common.ErrHeightFromFutureStr.Error())
-	assert.True(t, s.isHeightFromFutureError(err))
-	// unrelated
-	assert.False(t, s.isHeightFromFutureError(errors.New("boom")))
-}
-
 func TestSyncer_sendNonBlockingSignal(t *testing.T) {
 	s := &Syncer{logger: zerolog.Nop()}
 	ch := make(chan struct{}, 1)

pkg/config/config.go

@@ -28,8 +28,6 @@ const (
 	FlagRootDir = "home"
 	// FlagDBPath is a flag for specifying the database path
 	FlagDBPath = FlagPrefixEvnode + "db_path"
-	// FlagClearCache is a flag for clearing the cache
-	FlagClearCache = FlagPrefixEvnode + "clear_cache"
 
 	// Node configuration flags
 
@@ -49,11 +47,8 @@ const (
 	FlagLazyBlockTime = FlagPrefixEvnode + "node.lazy_block_interval"
 	// FlagReadinessMaxBlocksBehind configures how many blocks behind best-known head is still considered ready
 	FlagReadinessMaxBlocksBehind = FlagPrefixEvnode + "node.readiness_max_blocks_behind"
-
-	// Sync configuration flags
-
-	// FlagSyncP2PPrefer is a flag to prioritize p2p over da fetching
-	FlagSyncP2PPrefer = FlagPrefixEvnode + "sync.prefer_p2p"
+	// FlagClearCache is a flag for clearing the cache
+	FlagClearCache = FlagPrefixEvnode + "clear_cache"
 
 	// Data Availability configuration flags
 
@@ -135,14 +130,10 @@ const (
 // Config stores Rollkit configuration.
 type Config struct {
 	RootDir    string `mapstructure:"-" yaml:"-" comment:"Root directory where rollkit files are located"`
-	ClearCache bool   `mapstructure:"clear_cache" yaml:"-" comment:"Clear the cache"`
+	ClearCache bool   `mapstructure:"-" yaml:"-" comment:"Clear the cache"`
 
 	// Base configuration
 	DBPath string `mapstructure:"db_path" yaml:"db_path" comment:"Path inside the root directory where the database is located"`
-
-	// Sync configuration (not in yaml)
-	Sync SyncConfig `mapstructure:"sync" yaml:"-"`
-
 	// P2P configuration
 	P2P P2PConfig `mapstructure:"p2p" yaml:"p2p"`
 
@@ -165,12 +156,6 @@ type Config struct {
 	Signer SignerConfig `mapstructure:"signer" yaml:"signer"`
 }
 
-// SyncConfig contains synchronization configuration parameters
-// Those are used in flags are not set in the ev-node config.
-type SyncConfig struct {
-	PreferP2P bool `mapstructure:"prefer_p2p" yaml:"-" comment:"Prefer P2P over DA fetching"`
-}
-
 // DAConfig contains all Data Availability configuration parameters
 type DAConfig struct {
 	Address           string          `mapstructure:"address" yaml:"address" comment:"Address of the data availability layer service (host:port). This is the endpoint where Rollkit will connect to submit and retrieve data."`
@@ -324,9 +309,6 @@ func AddFlags(cmd *cobra.Command) {
 	cmd.Flags().String(FlagDBPath, def.DBPath, "path for the node database")
 	cmd.Flags().Bool(FlagClearCache, def.ClearCache, "clear the cache")
 
-	// Sync configuration flags
-	cmd.Flags().Bool(FlagSyncP2PPrefer, def.Sync.PreferP2P, "prefer P2P over DA fetching")
-
 	// Node configuration flags
 	cmd.Flags().Bool(FlagAggregator, def.Node.Aggregator, "run node in aggregator mode")
 	cmd.Flags().Bool(FlagLight, def.Node.Light, "run light client")
@@ -372,9 +354,6 @@ func AddFlags(cmd *cobra.Command) {
 	cmd.Flags().String(FlagSignerType, def.Signer.SignerType, "type of signer to use (file, grpc)")
 	cmd.Flags().String(FlagSignerPath, def.Signer.SignerPath, "path to the signer file or address")
 	cmd.Flags().String(FlagSignerPassphrase, "", "passphrase for the signer (required for file signer and if aggregator is enabled)")
-
-	// FlagSyncP2PPrefer is only relevant for sync nodes.
-	cmd.MarkFlagsMutuallyExclusive(FlagSyncP2PPrefer, FlagAggregator)
 }
 
 // Load loads the node configuration in the following order of precedence:

pkg/config/config_test.go

@@ -54,7 +54,6 @@ func TestAddFlags(t *testing.T) {
 
 	// Test specific flags
 	assertFlagValue(t, flags, FlagDBPath, DefaultConfig().DBPath)
-	assertFlagValue(t, flags, FlagClearCache, false)
 
 	// Node flags
 	assertFlagValue(t, flags, FlagAggregator, DefaultConfig().Node.Aggregator)
@@ -105,8 +104,25 @@ func TestAddFlags(t *testing.T) {
 	// RPC flags
 	assertFlagValue(t, flags, FlagRPCAddress, DefaultConfig().RPC.Address)
 
-	// Sync flags
-	assertFlagValue(t, flags, FlagSyncP2PPrefer, false)
+	// Count the number of flags we're explicitly checking
+	expectedFlagCount := 39 // Update this number if you add more flag checks above
+
+	// Get the actual number of flags (both regular and persistent)
+	actualFlagCount := 0
+	flags.VisitAll(func(flag *pflag.Flag) {
+		actualFlagCount++
+	})
+	persistentFlags.VisitAll(func(flag *pflag.Flag) {
+		actualFlagCount++
+	})
+
+	// Verify that the counts match
+	assert.Equal(
+		t,
+		expectedFlagCount,
+		actualFlagCount,
+		"Number of flags doesn't match. If you added a new flag, please update the test.",
+	)
 }
 
 func TestLoad(t *testing.T) {