fix: selectively evict header or data from cache on block validation failure

Classify block validation errors as FaultHeader or FaultData so the syncer
can drop only the offending side of the (header, data) pair from the DA
cache. Previously a single mismatch removed both, discarding a potentially
legitimate counterpart.

Author: randygrok

block/internal/syncing/syncer.go

@@ -746,13 +746,23 @@ func (s *Syncer) trySyncNextBlockWithState(ctx context.Context, event *common.DA
 	// here only the previous block needs to be applied to proceed to the verification.
 	// The header validation must be done before applying the block to avoid executing gibberish
 	if err := s.ValidateBlock(ctx, currentState, data, header); err != nil {
-		// remove header as da included from cache
-		s.cache.RemoveHeaderDAIncluded(headerHash)
-		s.cache.RemoveDataDAIncluded(data.DACommitment().String())
-
-		if errors.Is(err, types.ErrUnexpectedProposer) {
-			return errors.Join(errInvalidBlock, err)
+		var vErr *BlockValidationError
+		switch {
+		case errors.As(err, &vErr):
+			switch vErr.Fault {
+			case FaultHeader:
+				s.cache.RemoveHeaderDAIncluded(headerHash)
+			case FaultData:
+				s.cache.RemoveDataDAIncluded(data.DACommitment().String())
+			}
+		case errors.Is(err, errInvalidState):
+			// State divergence does not point at a specific side of the pair;
+			// the cached entries stay so an honest counterpart can still pair up.
+		default:
+			s.cache.RemoveHeaderDAIncluded(headerHash)
+			s.cache.RemoveDataDAIncluded(data.DACommitment().String())
 		}
+
 		if !errors.Is(err, errInvalidState) && !errors.Is(err, errInvalidBlock) {
 			return errors.Join(errInvalidBlock, err)
 		}
@@ -891,38 +901,54 @@ func (s *Syncer) executeTxsWithRetry(ctx context.Context, rawTxs [][]byte, heade
 	return coreexecutor.ExecuteResult{}, nil
 }
 
-// ValidateBlock validates a synced block
-// NOTE: if the header was gibberish and somehow passed all validation prior but the data was correct
-// or if the data was gibberish and somehow passed all validation prior but the header was correct
-// we are still losing both in the pending event. This should never happen.
+// ValidateBlock validates a synced block. It runs header-only checks first
+// (signature, proposer, sequence) and only then the pair checks between the
+// header and the attached data. Failures are wrapped in BlockValidationError so
+// callers can drop the right side of the pair from caches without discarding a
+// potentially legitimate counterpart.
 func (s *Syncer) ValidateBlock(_ context.Context, currState types.State, data *types.Data, header *types.SignedHeader) error {
 	// Set custom verifier for aggregator node signature
 	header.SetCustomVerifierForSyncNode(s.options.SyncNodeSignatureBytesProvider)
 
 	if err := header.ValidateBasicWithData(data); err != nil { //nolint:contextcheck // validation API does not accept context
-		return fmt.Errorf("invalid header: %w", err)
+		return classifyValidationError(fmt.Errorf("invalid header: %w", err))
 	}
 
 	if err := currState.AssertExpectedProposer(header); err != nil {
-		return errors.Join(errInvalidBlock, err)
+		return errors.Join(errInvalidBlock, &BlockValidationError{Fault: FaultHeader, Err: err})
 	}
 
 	if err := currState.AssertValidForNextState(header, data); err != nil {
-		if isExternalBlockValidationError(err) {
-			return errors.Join(errInvalidBlock, err)
+		if vErr := classifyValidationError(err); vErr != nil {
+			return errors.Join(errInvalidBlock, vErr)
 		}
 		return errors.Join(errInvalidState, err)
 	}
 	return nil
 }
 
-func isExternalBlockValidationError(err error) bool {
-	return errors.Is(err, types.ErrUnexpectedProposer) ||
-		errors.Is(err, types.ErrInvalidChainID) ||
-		errors.Is(err, types.ErrInvalidBlockHeight) ||
-		errors.Is(err, types.ErrInvalidBlockTime) ||
-		errors.Is(err, types.ErrHeaderDataMismatch) ||
-		errors.Is(err, types.ErrDataHashMismatch)
+// classifyValidationError tags a known external validation error with the side
+// at fault. It returns nil for errors that signal state divergence (the caller
+// must then classify them as errInvalidState).
+func classifyValidationError(err error) *BlockValidationError {
+	switch {
+	case errors.Is(err, types.ErrHeaderDataMismatch),
+		errors.Is(err, types.ErrDataHashMismatch):
+		return &BlockValidationError{Fault: FaultData, Err: err}
+	case errors.Is(err, types.ErrUnexpectedProposer),
+		errors.Is(err, types.ErrInvalidChainID),
+		errors.Is(err, types.ErrInvalidBlockHeight),
+		errors.Is(err, types.ErrInvalidBlockTime),
+		errors.Is(err, types.ErrSignerPubKeyMissing),
+		errors.Is(err, types.ErrSignerAddressMismatch),
+		errors.Is(err, types.ErrSignatureEmpty),
+		errors.Is(err, types.ErrSignatureVerificationFailed),
+		errors.Is(err, types.ErrProposerAddressMismatch),
+		errors.Is(err, types.ErrNoProposerAddress):
+		return &BlockValidationError{Fault: FaultHeader, Err: err}
+	default:
+		return nil
+	}
 }
 
 var errMaliciousProposer = errors.New("malicious proposer detected")

block/internal/syncing/syncer_test.go

@@ -352,6 +352,166 @@ func TestSyncer_ValidateBlock_RejectsSignerAddressNotDerivedFromPubKey(t *testin
 	require.Contains(t, err.Error(), "signer address")
 }
 
+func TestSyncer_ValidateBlock_ClassifiesFault(t *testing.T) {
+	expectedAddr, expectedPub, expectedSigner := buildSyncTestSigner(t)
+	wrongAddr, wrongPub, wrongSigner := buildSyncTestSigner(t)
+
+	now := time.Now()
+	baseState := types.State{
+		ChainID:             "tchain",
+		InitialHeight:       1,
+		LastBlockHeight:     1,
+		LastBlockTime:       now,
+		LastHeaderHash:      []byte("last-header-hash"),
+		AppHash:             []byte("app0"),
+		NextProposerAddress: expectedAddr,
+	}
+
+	makeHeader := func(tb testing.TB, chainID string, proposer []byte, pub crypto.PubKey, signer signerpkg.Signer, appHash []byte, data *types.Data) *types.SignedHeader {
+		tb.Helper()
+		_, header := makeSignedHeaderBytes(tb, chainID, 2, proposer, pub, signer, appHash, data, baseState.LastHeaderHash)
+		return header
+	}
+
+	tests := map[string]struct {
+		setup     func(testing.TB) (*types.SignedHeader, *types.Data)
+		wantFault ValidationFault
+	}{
+		"wrong proposer -> header fault": {
+			setup: func(tb testing.TB) (*types.SignedHeader, *types.Data) {
+				data := makeData(baseState.ChainID, 2, 1)
+				data.Metadata.Time = uint64(now.Add(time.Second).UnixNano())
+				return makeHeader(tb, baseState.ChainID, wrongAddr, wrongPub, wrongSigner, baseState.AppHash, data), data
+			},
+			wantFault: FaultHeader,
+		},
+		"wrong chain id -> header fault": {
+			setup: func(tb testing.TB) (*types.SignedHeader, *types.Data) {
+				data := makeData("other-chain", 2, 1)
+				data.Metadata.Time = uint64(now.Add(time.Second).UnixNano())
+				return makeHeader(tb, "other-chain", expectedAddr, expectedPub, expectedSigner, baseState.AppHash, data), data
+			},
+			wantFault: FaultHeader,
+		},
+		"data hash mismatch -> data fault": {
+			setup: func(tb testing.TB) (*types.SignedHeader, *types.Data) {
+				headerData := makeData(baseState.ChainID, 2, 1)
+				headerData.Metadata.Time = uint64(now.Add(time.Second).UnixNano())
+				header := makeHeader(tb, baseState.ChainID, expectedAddr, expectedPub, expectedSigner, baseState.AppHash, headerData)
+				attached := makeData(baseState.ChainID, 2, 2)
+				attached.Metadata.Time = headerData.Metadata.Time
+				return header, attached
+			},
+			wantFault: FaultData,
+		},
+		"header data metadata mismatch -> data fault": {
+			setup: func(tb testing.TB) (*types.SignedHeader, *types.Data) {
+				headerData := makeData(baseState.ChainID, 2, 1)
+				headerData.Metadata.Time = uint64(now.Add(time.Second).UnixNano())
+				header := makeHeader(tb, baseState.ChainID, expectedAddr, expectedPub, expectedSigner, baseState.AppHash, headerData)
+				attached := &types.Data{
+					Metadata: &types.Metadata{
+						ChainID: baseState.ChainID,
+						Height:  3,
+						Time:    headerData.Metadata.Time,
+					},
+					Txs: headerData.Txs,
+				}
+				return header, attached
+			},
+			wantFault: FaultData,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			header, data := tc.setup(t)
+			s := &Syncer{logger: zerolog.Nop(), options: common.DefaultBlockOptions()}
+			err := s.ValidateBlock(t.Context(), baseState, data, header)
+			require.Error(t, err)
+
+			var vErr *BlockValidationError
+			require.ErrorAs(t, err, &vErr, "expected *BlockValidationError")
+			assert.Equal(t, tc.wantFault, vErr.Fault, "fault classification")
+		})
+	}
+}
+
+func TestSyncer_TrySyncNextBlock_SelectiveCacheCleanup(t *testing.T) {
+	expectedAddr, expectedPub, expectedSigner := buildSyncTestSigner(t)
+	wrongAddr, wrongPub, wrongSigner := buildSyncTestSigner(t)
+
+	now := time.Now()
+	baseState := types.State{
+		ChainID:             "tchain",
+		InitialHeight:       1,
+		LastBlockHeight:     1,
+		LastBlockTime:       now,
+		LastHeaderHash:      []byte("last-header-hash"),
+		AppHash:             []byte("app0"),
+		NextProposerAddress: expectedAddr,
+	}
+
+	makeSyncer := func(tb testing.TB) (*Syncer, cache.Manager) {
+		tb.Helper()
+		ds := dssync.MutexWrap(datastore.NewMapDatastore())
+		st := store.New(ds)
+		cm, err := cache.NewManager(config.DefaultConfig(), st, zerolog.Nop())
+		require.NoError(tb, err)
+		return &Syncer{cache: cm, logger: zerolog.Nop(), options: common.DefaultBlockOptions()}, cm
+	}
+
+	tests := map[string]struct {
+		event           func(testing.TB) common.DAHeightEvent
+		wantHeaderInCache bool
+		wantDataInCache   bool
+	}{
+		"header fault keeps data in cache": {
+			event: func(tb testing.TB) common.DAHeightEvent {
+				data := makeData(baseState.ChainID, 2, 1)
+				data.Metadata.Time = uint64(now.Add(time.Second).UnixNano())
+				_, header := makeSignedHeaderBytes(tb, baseState.ChainID, 2, wrongAddr, wrongPub, wrongSigner, baseState.AppHash, data, baseState.LastHeaderHash)
+				return common.DAHeightEvent{Header: header, Data: data, Source: common.SourceDA}
+			},
+			wantHeaderInCache: false,
+			wantDataInCache:   true,
+		},
+		"data fault keeps header in cache": {
+			event: func(tb testing.TB) common.DAHeightEvent {
+				headerData := makeData(baseState.ChainID, 2, 1)
+				headerData.Metadata.Time = uint64(now.Add(time.Second).UnixNano())
+				_, header := makeSignedHeaderBytes(tb, baseState.ChainID, 2, expectedAddr, expectedPub, expectedSigner, baseState.AppHash, headerData, baseState.LastHeaderHash)
+				attached := makeData(baseState.ChainID, 2, 2)
+				attached.Metadata.Time = headerData.Metadata.Time
+				return common.DAHeightEvent{Header: header, Data: attached, Source: common.SourceDA}
+			},
+			wantHeaderInCache: true,
+			wantDataInCache:   false,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			s, cm := makeSyncer(t)
+			event := tc.event(t)
+			headerHash := event.Header.Hash().String()
+			dataHash := event.Data.DACommitment().String()
+
+			// Seed the cache so we can observe what gets removed.
+			cm.SetHeaderDAIncluded(headerHash, 1, event.Header.Height())
+			cm.SetDataDAIncluded(dataHash, 1, event.Header.Height())
+
+			err := s.trySyncNextBlockWithState(t.Context(), &event, baseState)
+			require.Error(t, err)
+
+			_, headerStillIncluded := cm.GetHeaderDAIncludedByHash(headerHash)
+			_, dataStillIncluded := cm.GetDataDAIncludedByHash(dataHash)
+			assert.Equal(t, tc.wantHeaderInCache, headerStillIncluded, "header cache presence")
+			assert.Equal(t, tc.wantDataInCache, dataStillIncluded, "data cache presence")
+		})
+	}
+}
+
 func TestSyncer_ApplyBlockPersistsExecutionNextProposer(t *testing.T) {
 	addr, _, _ := buildSyncTestSigner(t)
 	execNext := []byte("execution-next-proposer")

block/internal/syncing/validation_errors.go

@@ -0,0 +1,43 @@
+package syncing
+
+import "fmt"
+
+// ValidationFault identifies which side of a (header, data) pair is responsible
+// for a block validation failure. Callers use it to decide what to evict from
+// caches so a legitimate counterpart is not discarded together with the bad one.
+type ValidationFault int
+
+const (
+	// FaultHeader marks the header as invalid on its own (signature, proposer,
+	// chain id, height, sequence). The data attached to the event may still be
+	// legitimate and pair with a different valid header.
+	FaultHeader ValidationFault = iota
+
+	// FaultData marks the data as the suspect. It is used when the header
+	// passed all header-only checks but the data does not match the header
+	// (mismatched DataHash or metadata).
+	FaultData
+)
+
+// BlockValidationError wraps the underlying validation error with a fault tag.
+type BlockValidationError struct {
+	Fault ValidationFault
+	Err   error
+}
+
+func (e *BlockValidationError) Error() string {
+	return fmt.Sprintf("block validation failed (%s): %v", e.Fault, e.Err)
+}
+
+func (e *BlockValidationError) Unwrap() error { return e.Err }
+
+func (f ValidationFault) String() string {
+	switch f {
+	case FaultHeader:
+		return "header"
+	case FaultData:
+		return "data"
+	default:
+		return "unknown"
+	}
+}