refactor(syncing): centralize double-sign detection, drop reportDoubleSign callback

Author: CaelRowley

block/internal/syncing/da_retriever.go

@@ -15,7 +15,6 @@ import (
 	"github.com/evstack/ev-node/block/internal/da"
 	datypes "github.com/evstack/ev-node/pkg/da/types"
 	"github.com/evstack/ev-node/pkg/genesis"
-	"github.com/evstack/ev-node/pkg/store"
 	"github.com/evstack/ev-node/types"
 	pb "github.com/evstack/ev-node/types/pb/evnode/v1"
 )
@@ -35,8 +34,8 @@ type daRetriever struct {
 	cache   cache.CacheManager
 	genesis genesis.Genesis
 	logger  zerolog.Logger
-	store        store.Store
-	onDoubleSign doubleSignHandler // nil disables detection; the retriever aborts the batch on a positive
+	// detectDoubleSign aborts the batch when it returns true. Nil disables.
+	detectDoubleSign doubleSignDetector
 
 	mu sync.Mutex
 	// transient cache, only full event need to be passed to the syncer
@@ -49,26 +48,23 @@ type daRetriever struct {
 	strictMode bool
 }
 
-// NewDARetriever creates a new DA retriever. Double-sign detection is disabled
-// when st or onDoubleSign is nil.
+// NewDARetriever creates a new DA retriever.
 func NewDARetriever(
 	client da.Client,
 	cache cache.CacheManager,
 	genesis genesis.Genesis,
 	logger zerolog.Logger,
-	st store.Store,
-	onDoubleSign doubleSignHandler,
+	detectDoubleSign doubleSignDetector,
 ) *daRetriever {
 	return &daRetriever{
-		client:         client,
-		cache:          cache,
-		genesis:        genesis,
-		logger:         logger.With().Str("component", "da_retriever").Logger(),
-		store:          st,
-		onDoubleSign:   onDoubleSign,
-		pendingHeaders: make(map[uint64]*types.SignedHeader),
-		pendingData:    make(map[uint64]*types.Data),
-		strictMode:     false,
+		client:           client,
+		cache:            cache,
+		genesis:          genesis,
+		logger:           logger.With().Str("component", "da_retriever").Logger(),
+		detectDoubleSign: detectDoubleSign,
+		pendingHeaders:   make(map[uint64]*types.SignedHeader),
+		pendingData:      make(map[uint64]*types.Data),
+		strictMode:       false,
 	}
 }
 
@@ -181,14 +177,8 @@ func (r *daRetriever) processBlobs(ctx context.Context, blobs [][]byte, daHeight
 
 		if header := r.tryDecodeHeader(bz, daHeight); header != nil {
 			// Catches both in-batch alternates and alternates of already-persisted heights.
-			if r.store != nil && r.onDoubleSign != nil {
-				if ev, err := detectDoubleSign(ctx, r.store, r.cache, header, types.EvidenceSourceDA); err == nil && ev != nil {
-					r.onDoubleSign(ctx, ev)
-					return nil
-				} else if err != nil {
-					r.logger.Warn().Err(err).Uint64("height", header.Height()).Msg("double-sign detection error")
-				}
-				r.cache.SetPendingSignedHeader(header, types.EvidenceSourceDA)
+			if r.detectDoubleSign != nil && r.detectDoubleSign(ctx, header, types.EvidenceSourceDA) {
+				return nil
 			}
 
 			if _, ok := r.pendingHeaders[header.Height()]; ok {

block/internal/syncing/da_retriever_test.go

@@ -52,7 +52,7 @@ func newTestDARetriever(t *testing.T, mockClient *mocks.MockClient, cfg config.C
 	mockClient.On("GetForcedInclusionNamespace").Return([]byte(nil)).Maybe()
 	mockClient.On("HasForcedInclusionNamespace").Return(false).Maybe()
 
-	return NewDARetriever(mockClient, cm, gen, zerolog.Nop(), nil, nil)
+	return NewDARetriever(mockClient, cm, gen, zerolog.Nop(), nil)
 }
 
 // makeSignedDataBytes builds SignedData containing the provided Data and returns its binary encoding

block/internal/syncing/doublesign.go

@@ -19,42 +19,33 @@ import (
 // ErrDoubleSign is returned when two validly-signed SignedHeaders are observed at the same height.
 var ErrDoubleSign = errors.New("double-sign detected")
 
-// doubleSignHandler is fired when an equivocation is confirmed. It persists
-// evidence, bumps metrics, and halts the syncer.
-type doubleSignHandler func(ctx context.Context, evidence *types.DoubleSignEvidence)
-
-// detectDoubleSign compares incoming against the first-seen SignedHeader at
-// the same height (cache then store) and returns non-nil evidence when their
-// hashes differ. Caller must verify proposer + signature first.
-func detectDoubleSign(
+// doubleSignDetector reports an observed header for equivocation detection.
+// Returns true on a confirmed double-sign so the caller can abort.
+type doubleSignDetector func(ctx context.Context, header *types.SignedHeader, source string) bool
+
+// firstObservation returns the first-seen SignedHeader at this height,
+// preferring the cache over the store. Returns (nil, "", nil) when none.
+func firstObservation(
 	ctx context.Context,
 	st store.Store,
 	cm cache.CacheManager,
-	incoming *types.SignedHeader,
-	incomingSource string,
-) (*types.DoubleSignEvidence, error) {
-	if incoming == nil {
-		return nil, errors.New("incoming header is nil")
-	}
-	height := incoming.Height()
-
-	// Cache wins over store: the cached entry is the literal first observation
-	// and carries the original FirstSource.
+	height uint64,
+) (*types.SignedHeader, string, error) {
 	if cached, source, ok := cm.GetPendingSignedHeader(height); ok {
-		return buildEvidenceFromPair(cached, incoming, source, incomingSource), nil
+		return cached, source, nil
 	}
 
-	storedHeader, storeErr := st.GetHeader(ctx, height)
-	if storeErr != nil {
-		if store.IsNotFound(storeErr) {
-			return nil, nil
+	storedHeader, err := st.GetHeader(ctx, height)
+	if err != nil {
+		if store.IsNotFound(err) {
+			return nil, "", nil
 		}
-		return nil, fmt.Errorf("lookup stored header at %d: %w", height, storeErr)
+		return nil, "", fmt.Errorf("lookup stored header at %d: %w", height, err)
 	}
 	if storedHeader == nil {
-		return nil, nil
+		return nil, "", nil
 	}
-	return buildEvidenceFromPair(storedHeader, incoming, types.EvidenceSourceStored, incomingSource), nil
+	return storedHeader, types.EvidenceSourceStored, nil
 }
 
 // buildEvidenceFromPair returns evidence for two SignedHeaders at the same
@@ -99,16 +90,15 @@ func persistEvidence(ctx context.Context, st store.Store, ev *types.DoubleSignEv
 	return nil
 }
 
-// reportDoubleSign persists evidence, logs, bumps the metric (once per
-// distinct alternate hash via seen), fires criticalErr, and returns the
-// wrapped ErrDoubleSign for the caller to propagate as the halt cause.
+// reportDoubleSign persists evidence and bumps the metric, deduping by
+// (height, altHash). Returns a wrapped ErrDoubleSign on first sighting,
+// nil when already seen.
 func reportDoubleSign(
 	ctx context.Context,
 	st store.Store,
 	metrics *common.Metrics,
 	logger zerolog.Logger,
 	seen *doubleSignDedup,
-	criticalErr func(error),
 	ev *types.DoubleSignEvidence,
 ) error {
 	altHashStr := ev.AlternateHeader.Hash().String()
@@ -144,15 +134,11 @@ func reportDoubleSign(
 		Str("evidence_key", key).
 		Msg("DOUBLE-SIGN DETECTED — sequencer equivocation; halting syncer")
 
-	halt := fmt.Errorf(
+	return fmt.Errorf(
 		"double-sign detected at height %d: sequencer signed conflicting headers %s and %s. "+
 			"Evidence persisted at metadata key %s. Manual intervention required: %w",
 		ev.Height, firstHashStr, altHashStr, key, ErrDoubleSign,
 	)
-	if criticalErr != nil {
-		criticalErr(halt)
-	}
-	return halt
 }
 
 // doubleSignDedup collapses (height, altHash) duplicates so the same

block/internal/syncing/doublesign_branches_test.go

@@ -56,48 +56,40 @@ func (nilHeaderStore) GetHeader(context.Context, uint64) (*types.SignedHeader, e
 	return nil, nil
 }
 
-func TestDetectDoubleSign_NilIncomingReturnsError(t *testing.T) {
-	env := newDSTestEnv(t)
-	ev, err := detectDoubleSign(context.Background(), env.store, env.cache, nil, types.EvidenceSourceP2P)
-	require.Error(t, err)
-	require.Nil(t, ev)
-}
-
 // A non-NotFound store failure must be surfaced, not swallowed.
-func TestDetectDoubleSign_StoreErrorWrapped(t *testing.T) {
+func TestFirstObservation_StoreErrorWrapped(t *testing.T) {
 	env := newDSTestEnv(t)
 	wrapped := &errStore{Store: env.store, getHeaderErr: errors.New("backend down")}
 
-	alt := env.signHeaderAtHeight(5, 0x01)
-	ev, err := detectDoubleSign(context.Background(), wrapped, env.cache, alt, types.EvidenceSourceP2P)
+	prior, priorSource, err := firstObservation(context.Background(), wrapped, env.cache, 5)
 	require.Error(t, err)
-	require.Nil(t, ev)
+	require.Nil(t, prior)
+	require.Empty(t, priorSource)
 	require.Contains(t, err.Error(), "lookup stored header")
 	require.ErrorContains(t, err, "backend down")
 }
 
-func TestDetectDoubleSign_StoredHeaderNilDefensive(t *testing.T) {
+func TestFirstObservation_StoredHeaderNilDefensive(t *testing.T) {
 	env := newDSTestEnv(t)
 	wrapped := nilHeaderStore{Store: env.store}
 
-	alt := env.signHeaderAtHeight(5, 0x01)
-	ev, err := detectDoubleSign(context.Background(), wrapped, env.cache, alt, types.EvidenceSourceP2P)
+	prior, priorSource, err := firstObservation(context.Background(), wrapped, env.cache, 5)
 	require.NoError(t, err)
-	require.Nil(t, ev)
+	require.Nil(t, prior)
+	require.Empty(t, priorSource)
 }
 
-// Store-path detections must use the "stored" sentinel as FirstSource so
-// downstream consumers can disambiguate it from in-flight observations.
-func TestDetectDoubleSign_FirstSourceStoredSentinel(t *testing.T) {
+// Store-path observations must use the "stored" sentinel as the source so
+// downstream consumers can disambiguate them from in-flight observations.
+func TestFirstObservation_StoredSourceSentinel(t *testing.T) {
 	env := newDSTestEnv(t)
 	first := env.signHeaderAtHeight(5, 0x01)
 	env.saveHeader(first)
 
-	alt := env.signHeaderAtHeight(5, 0x02)
-	ev, err := detectDoubleSign(context.Background(), env.store, env.cache, alt, types.EvidenceSourceP2P)
+	prior, priorSource, err := firstObservation(context.Background(), env.store, env.cache, first.Height())
 	require.NoError(t, err)
-	require.NotNil(t, ev)
-	require.Equal(t, types.EvidenceSourceStored, ev.FirstSource)
+	require.NotNil(t, prior)
+	require.Equal(t, types.EvidenceSourceStored, priorSource)
 }
 
 // SetMetadata failures must include the canonical key so an operator can
@@ -118,7 +110,7 @@ func TestPersistEvidence_StoreError(t *testing.T) {
 }
 
 // Persistence failure must not break the halt contract: metric still
-// increments, criticalErr still fires, returned error still wraps ErrDoubleSign.
+// increments and the returned error still wraps ErrDoubleSign.
 func TestReportDoubleSign_PersistFailureLoggedNotBlocking(t *testing.T) {
 	env := newDSTestEnv(t)
 	wrapped := &errStore{Store: env.store, setMetadataErr: errors.New("disk full")}
@@ -132,16 +124,12 @@ func TestReportDoubleSign_PersistFailureLoggedNotBlocking(t *testing.T) {
 	metrics := common.NopMetrics()
 	metrics.DoubleSignsDetected = &counterCtr{n: &dsCount}
 
-	var fired atomic.Pointer[error]
-	crit := func(err error) { fired.Store(&err) }
-
 	halt := reportDoubleSign(context.Background(), wrapped, metrics, zerolog.Nop(),
-		newDoubleSignDedup(), crit, ev)
+		newDoubleSignDedup(), ev)
 	require.Error(t, halt)
 	require.ErrorIs(t, halt, ErrDoubleSign)
 
 	require.Equal(t, int64(1), dsCount.Load())
-	require.NotNil(t, fired.Load())
 }
 
 // Dedup is keyed on (height, altHash), so two distinct alts at the same
@@ -163,12 +151,11 @@ func TestReportDoubleSign_TwoDistinctAltsAtSameHeight(t *testing.T) {
 	metrics.DoubleSignsDetected = &counterCtr{n: &dsCount}
 
 	seen := newDoubleSignDedup()
-	noopCrit := func(error) {}
 
 	require.Error(t, reportDoubleSign(context.Background(), env.store, metrics,
-		zerolog.Nop(), seen, noopCrit, ev1))
+		zerolog.Nop(), seen, ev1))
 	require.Error(t, reportDoubleSign(context.Background(), env.store, metrics,
-		zerolog.Nop(), seen, noopCrit, ev2))
+		zerolog.Nop(), seen, ev2))
 
 	require.Equal(t, int64(2), dsCount.Load())
 
@@ -189,14 +176,14 @@ func TestReportDoubleSign_NilSeenAndNilGuards(t *testing.T) {
 
 	t.Run("nil seen still halts", func(t *testing.T) {
 		halt := reportDoubleSign(context.Background(), env.store, common.NopMetrics(),
-			zerolog.Nop(), nil, func(error) {}, ev)
+			zerolog.Nop(), nil, ev)
 		require.Error(t, halt)
 		require.ErrorIs(t, halt, ErrDoubleSign)
 	})
 
 	t.Run("nil metrics still halts", func(t *testing.T) {
 		halt := reportDoubleSign(context.Background(), env.store, nil,
-			zerolog.Nop(), newDoubleSignDedup(), func(error) {}, ev)
+			zerolog.Nop(), newDoubleSignDedup(), ev)
 		require.Error(t, halt)
 		require.ErrorIs(t, halt, ErrDoubleSign)
 	})
@@ -205,14 +192,7 @@ func TestReportDoubleSign_NilSeenAndNilGuards(t *testing.T) {
 		m := common.NopMetrics()
 		m.DoubleSignsDetected = nil
 		halt := reportDoubleSign(context.Background(), env.store, m,
-			zerolog.Nop(), newDoubleSignDedup(), func(error) {}, ev)
-		require.Error(t, halt)
-		require.ErrorIs(t, halt, ErrDoubleSign)
-	})
-
-	t.Run("nil criticalErr still halts", func(t *testing.T) {
-		halt := reportDoubleSign(context.Background(), env.store, common.NopMetrics(),
-			zerolog.Nop(), newDoubleSignDedup(), nil, ev)
+			zerolog.Nop(), newDoubleSignDedup(), ev)
 		require.Error(t, halt)
 		require.ErrorIs(t, halt, ErrDoubleSign)
 	})
@@ -270,7 +250,7 @@ func TestDARetriever_AbortsBatchOnDetection(t *testing.T) {
 
 	mockClient := newMockDAClient(t)
 
-	r := NewDARetriever(mockClient, env.cache, env.gen, zerolog.Nop(), env.store, env.onDouble)
+	r := NewDARetriever(mockClient, env.cache, env.gen, zerolog.Nop(), env.detectDoubleSign)
 
 	events := r.ProcessBlobs(context.Background(),
 		[][]byte{firstBin, altBin, nextBin}, 100)
@@ -290,7 +270,7 @@ func TestDARetriever_DetectorErrorWarnAndContinue(t *testing.T) {
 
 	mockClient := newMockDAClient(t)
 
-	r := NewDARetriever(mockClient, env.cache, env.gen, zerolog.Nop(), wrapped, env.onDouble)
+	r := NewDARetriever(mockClient, env.cache, env.gen, zerolog.Nop(), env.makeDetectDoubleSign(wrapped))
 
 	_ = r.ProcessBlobs(context.Background(), [][]byte{bin}, 100)
 
@@ -324,7 +304,7 @@ func TestDARetriever_StrictModeEnvelopeDoubleSign(t *testing.T) {
 
 	mockClient := newMockDAClient(t)
 
-	r := NewDARetriever(mockClient, env.cache, env.gen, zerolog.Nop(), env.store, env.onDouble)
+	r := NewDARetriever(mockClient, env.cache, env.gen, zerolog.Nop(), env.detectDoubleSign)
 
 	events := r.ProcessBlobs(context.Background(), [][]byte{firstBin, altBin}, 100)
 	require.Empty(t, events)
@@ -348,12 +328,11 @@ func TestDARetriever_SetsPendingSignedHeaderOnFirstObservation(t *testing.T) {
 
 	mockClient := newMockDAClient(t)
 
-	r := NewDARetriever(mockClient, env.cache, env.gen, zerolog.Nop(), env.store, env.onDouble)
+	r := NewDARetriever(mockClient, env.cache, env.gen, zerolog.Nop(), env.detectDoubleSign)
 	_ = r.ProcessBlobs(context.Background(), [][]byte{bin}, 100)
 
 	got, src, ok := env.cache.GetPendingSignedHeader(5)
 	require.True(t, ok)
 	require.Equal(t, first.Hash().String(), got.Hash().String())
 	require.Equal(t, types.EvidenceSourceDA, src)
 }
-