refactor(block): log error on data verification failed (#2624)

<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.

NOTE: PR titles should follow semantic commits:
https://www.conventionalcommits.org/en/v1.0.0/
-->

## Overview

Looks like we we never logging the error when data verification failed.
This changes isValidSignedData to return an error with the cause instead
of true/false

<!-- 
Please provide an explanation of the PR, including the appropriate
context,
background, goal, and rationale. If there is an issue with this
information,
please provide a tl;dr and link the issue. 

Ex: Closes #<issue number>
-->

Author: julienrbrt

block/manager.go

@@ -1127,22 +1127,3 @@ func (m *Manager) SaveCache() error {
 
 	return nil
 }
-
-// isValidSignedData returns true if the data signature is valid for the expected sequencer.
-func (m *Manager) isValidSignedData(signedData *types.SignedData) bool {
-	if signedData == nil || signedData.Txs == nil {
-		return false
-	}
-
-	if err := m.assertUsingExpectedSingleSequencer(signedData.Signer.Address); err != nil {
-		return false
-	}
-
-	dataBytes, err := signedData.Data.MarshalBinary()
-	if err != nil {
-		return false
-	}
-
-	valid, err := signedData.Signer.PubKey.Verify(dataBytes, signedData.Signature)
-	return err == nil && valid
-}

block/manager_test.go

@@ -410,106 +410,6 @@ func TestGetDataSignature_NilSigner(t *testing.T) {
 	require.ErrorContains(err, "signer is nil; cannot sign data")
 }
 
-// TestIsValidSignedData covers valid, nil, wrong proposer, and invalid signature cases for isValidSignedData.
-func TestIsValidSignedData(t *testing.T) {
-	require := require.New(t)
-	privKey, _, err := crypto.GenerateKeyPair(crypto.Ed25519, 256)
-	require.NoError(err)
-	testSigner, err := noopsigner.NewNoopSigner(privKey)
-	require.NoError(err)
-	proposerAddr, err := testSigner.GetAddress()
-	require.NoError(err)
-	gen := genesispkg.NewGenesis(
-		"testchain",
-		1,
-		time.Now(),
-		proposerAddr,
-	)
-	m := &Manager{
-		signer:  testSigner,
-		genesis: gen,
-	}
-
-	t.Run("valid signed data", func(t *testing.T) {
-		batch := &types.Data{
-			Txs: types.Txs{types.Tx("tx1"), types.Tx("tx2")},
-		}
-		sig, err := m.getDataSignature(batch)
-		require.NoError(err)
-		pubKey, err := m.signer.GetPublic()
-		require.NoError(err)
-		signedData := &types.SignedData{
-			Data:      *batch,
-			Signature: sig,
-			Signer: types.Signer{
-				PubKey:  pubKey,
-				Address: proposerAddr,
-			},
-		}
-		assert.True(t, m.isValidSignedData(signedData))
-	})
-
-	t.Run("nil signed data", func(t *testing.T) {
-		assert.False(t, m.isValidSignedData(nil))
-	})
-
-	t.Run("nil Txs", func(t *testing.T) {
-		signedData := &types.SignedData{
-			Data: types.Data{},
-			Signer: types.Signer{
-				Address: proposerAddr,
-			},
-		}
-		signedData.Txs = nil
-		assert.False(t, m.isValidSignedData(signedData))
-	})
-
-	t.Run("wrong proposer address", func(t *testing.T) {
-		batch := &types.Data{
-			Txs: types.Txs{types.Tx("tx1")},
-		}
-		sig, err := m.getDataSignature(batch)
-		require.NoError(err)
-		pubKey, err := m.signer.GetPublic()
-		require.NoError(err)
-		wrongAddr := make([]byte, len(proposerAddr))
-		copy(wrongAddr, proposerAddr)
-		wrongAddr[0] ^= 0xFF // flip a bit
-		signedData := &types.SignedData{
-			Data:      *batch,
-			Signature: sig,
-			Signer: types.Signer{
-				PubKey:  pubKey,
-				Address: wrongAddr,
-			},
-		}
-		assert.False(t, m.isValidSignedData(signedData))
-	})
-
-	t.Run("invalid signature", func(t *testing.T) {
-		batch := &types.Data{
-			Txs: types.Txs{types.Tx("tx1")},
-		}
-		sig, err := m.getDataSignature(batch)
-		require.NoError(err)
-		pubKey, err := m.signer.GetPublic()
-		require.NoError(err)
-		// Corrupt the signature
-		badSig := make([]byte, len(sig))
-		copy(badSig, sig)
-		badSig[0] ^= 0xFF
-		signedData := &types.SignedData{
-			Data:      *batch,
-			Signature: badSig,
-			Signer: types.Signer{
-				PubKey:  pubKey,
-				Address: proposerAddr,
-			},
-		}
-		assert.False(t, m.isValidSignedData(signedData))
-	})
-}
-
 // TestManager_execValidate tests the execValidate method for various header/data/state conditions.
 func TestManager_execValidate(t *testing.T) {
 	require := require.New(t)

block/retriever_da.go

@@ -255,8 +255,8 @@ func (m *Manager) tryDecodeData(bz []byte, daHeight uint64) *types.Data {
 	}
 
 	// Early validation to reject junk data
-	if !m.isValidSignedData(&signedData) {
-		m.logger.Debug().Uint64("daHeight", daHeight).Msg("invalid data signature")
+	if err := m.assertValidSignedData(&signedData); err != nil {
+		m.logger.Debug().Uint64("daHeight", daHeight).Err(err).Msg("invalid data signature")
 		return nil
 	}
 
@@ -268,6 +268,33 @@ func (m *Manager) tryDecodeData(bz []byte, daHeight uint64) *types.Data {
 	return &signedData.Data
 }
 
+// assertValidSignedData validates the data signature and returns an error if it's invalid.
+func (m *Manager) assertValidSignedData(signedData *types.SignedData) error {
+	if signedData == nil || signedData.Txs == nil {
+		return errors.New("empty signed data")
+	}
+
+	if err := m.assertUsingExpectedSingleSequencer(signedData.Signer.Address); err != nil {
+		return err
+	}
+
+	dataBytes, err := signedData.Data.MarshalBinary()
+	if err != nil {
+		return fmt.Errorf("failed to marshal data: %w", err)
+	}
+
+	valid, err := signedData.Signer.PubKey.Verify(dataBytes, signedData.Signature)
+	if err != nil {
+		return fmt.Errorf("failed to verify signature: %w", err)
+	}
+
+	if !valid {
+		return fmt.Errorf("invalid signature")
+	}
+
+	return nil
+}
+
 // isAtHeight checks if a height is available.
 func (m *Manager) isAtHeight(ctx context.Context, height uint64) error {
 	currentHeight, err := m.GetStoreHeight(ctx)

block/retriever_da_test.go

@@ -23,7 +23,7 @@ import (
 	"github.com/evstack/ev-node/pkg/cache"
 	"github.com/evstack/ev-node/pkg/config"
 	"github.com/evstack/ev-node/pkg/genesis"
-	"github.com/evstack/ev-node/pkg/signer/noop"
+	noopsigner "github.com/evstack/ev-node/pkg/signer/noop"
 	storepkg "github.com/evstack/ev-node/pkg/store"
 	rollmocks "github.com/evstack/ev-node/test/mocks"
 	"github.com/evstack/ev-node/types"
@@ -52,7 +52,7 @@ func setupManagerForRetrieverTest(t *testing.T, initialDAHeight uint64) (*daRetr
 	src := rand.Reader
 	pk, _, err := crypto.GenerateEd25519Key(src)
 	require.NoError(t, err)
-	noopSigner, err := noop.NewNoopSigner(pk)
+	noopSigner, err := noopsigner.NewNoopSigner(pk)
 	require.NoError(t, err)
 
 	addr, err := noopSigner.GetAddress()
@@ -361,7 +361,7 @@ func TestProcessNextDAHeader_UnexpectedSequencer(t *testing.T) {
 	src := rand.Reader
 	pk, _, err := crypto.GenerateEd25519Key(src)
 	require.NoError(t, err)
-	signerNoop, err := noop.NewNoopSigner(pk)
+	signerNoop, err := noopsigner.NewNoopSigner(pk)
 	require.NoError(t, err)
 	hc := types.HeaderConfig{
 		Height: blockHeight,
@@ -732,3 +732,103 @@ func TestRetrieveLoop_DAHeightIncrementsOnlyOnSuccess(t *testing.T) {
 
 	mockDAClient.AssertExpectations(t)
 }
+
+// TestAssertValidSignedData covers valid, nil, wrong proposer, and invalid signature cases for assertValidSignedData.
+func TestAssertValidSignedData(t *testing.T) {
+	require := require.New(t)
+	privKey, _, err := crypto.GenerateKeyPair(crypto.Ed25519, 256)
+	require.NoError(err)
+	testSigner, err := noopsigner.NewNoopSigner(privKey)
+	require.NoError(err)
+	proposerAddr, err := testSigner.GetAddress()
+	require.NoError(err)
+	gen := genesis.NewGenesis(
+		"testchain",
+		1,
+		time.Now(),
+		proposerAddr,
+	)
+	m := &Manager{
+		signer:  testSigner,
+		genesis: gen,
+	}
+
+	t.Run("valid signed data", func(t *testing.T) {
+		batch := &types.Data{
+			Txs: types.Txs{types.Tx("tx1"), types.Tx("tx2")},
+		}
+		sig, err := m.getDataSignature(batch)
+		require.NoError(err)
+		pubKey, err := m.signer.GetPublic()
+		require.NoError(err)
+		signedData := &types.SignedData{
+			Data:      *batch,
+			Signature: sig,
+			Signer: types.Signer{
+				PubKey:  pubKey,
+				Address: proposerAddr,
+			},
+		}
+		assert.NoError(t, m.assertValidSignedData(signedData))
+	})
+
+	t.Run("nil signed data", func(t *testing.T) {
+		assert.Error(t, m.assertValidSignedData(nil))
+	})
+
+	t.Run("nil Txs", func(t *testing.T) {
+		signedData := &types.SignedData{
+			Data: types.Data{},
+			Signer: types.Signer{
+				Address: proposerAddr,
+			},
+		}
+		signedData.Txs = nil
+		assert.Error(t, m.assertValidSignedData(signedData))
+	})
+
+	t.Run("wrong proposer address", func(t *testing.T) {
+		batch := &types.Data{
+			Txs: types.Txs{types.Tx("tx1")},
+		}
+		sig, err := m.getDataSignature(batch)
+		require.NoError(err)
+		pubKey, err := m.signer.GetPublic()
+		require.NoError(err)
+		wrongAddr := make([]byte, len(proposerAddr))
+		copy(wrongAddr, proposerAddr)
+		wrongAddr[0] ^= 0xFF // flip a bit
+		signedData := &types.SignedData{
+			Data:      *batch,
+			Signature: sig,
+			Signer: types.Signer{
+				PubKey:  pubKey,
+				Address: wrongAddr,
+			},
+		}
+		assert.Error(t, m.assertValidSignedData(signedData))
+	})
+
+	t.Run("invalid signature", func(t *testing.T) {
+		batch := &types.Data{
+			Txs: types.Txs{types.Tx("tx1")},
+		}
+		sig, err := m.getDataSignature(batch)
+		require.NoError(err)
+		pubKey, err := m.signer.GetPublic()
+		require.NoError(err)
+		// Corrupt the signature
+		badSig := make([]byte, len(sig))
+		copy(badSig, sig)
+		badSig[0] ^= 0xFF
+		signedData := &types.SignedData{
+			Data:      *batch,
+			Signature: badSig,
+			Signer: types.Signer{
+				PubKey:  pubKey,
+				Address: proposerAddr,
+			},
+		}
+		assert.Error(t, m.assertValidSignedData(signedData))
+	})
+}

docs/learn/specs/block-manager.md

@@ -456,7 +456,7 @@ The `DataStoreRetrieveLoop`:
 - Operates at `BlockTime` intervals via `dataStoreCh` signals
 - Tracks `dataStoreHeight` for the last retrieved data
 - Retrieves all data blocks between last height and current store height
-- Validates data signatures using `isValidSignedData`
+- Validates data signatures using `assertValidSignedData`
 - Marks data as "seen" in the data cache
 - Sends data to sync goroutine via `dataInCh`
 
@@ -568,30 +568,30 @@ The communication with DA layer:
 
 ## Assumptions and Considerations
 
-* The block manager loads the initial state from the local store and uses genesis if not found in the local store, when the node (re)starts.
-* The default mode for sequencer nodes is normal (not lazy).
-* The sequencer can produce empty blocks.
-* In lazy aggregation mode, the block manager maintains consistency with the DA layer by producing empty blocks at regular intervals, ensuring a 1:1 mapping between DA layer blocks and execution layer blocks.
-* The lazy aggregation mechanism uses a dual timer approach:
-  * A `blockTimer` that triggers block production when transactions are available
-  * A `lazyTimer` that ensures blocks are produced even during periods of inactivity
-* Empty batches are handled differently in lazy mode - instead of discarding them, they are returned with the `ErrNoBatch` error, allowing the caller to create empty blocks with proper timestamps.
-* Transaction notifications from the `Reaper` to the `Manager` are handled via a non-blocking notification channel (`txNotifyCh`) to prevent backpressure.
-* The block manager enforces `MaxPendingHeadersAndData` limit to prevent unbounded growth of pending queues during DA submission issues.
-* Headers and data are submitted separately to the DA layer using different namespaces, supporting the header/data separation architecture.
-* The block manager uses persistent caches for headers and data to track seen items and DA inclusion status.
-* Namespace migration is handled transparently, with automatic detection and state persistence to optimize future operations.
-* The system supports backward compatibility with legacy single-namespace deployments while transitioning to separate namespaces.
-* Gas price management includes automatic adjustment with `GasMultiplier` on DA submission retries.
-* The block manager uses persistent storage (disk) when the `root_dir` and `db_path` configuration parameters are specified in `config.yaml` file under the app directory. If these configuration parameters are not specified, the in-memory storage is used, which will not be persistent if the node stops.
-* The block manager does not re-apply blocks when they transition from soft confirmed to DA included status. The block is only marked DA included in the caches.
-* Header and data stores use separate prefixes for isolation in the underlying database.
-* The genesis `ChainID` is used to create separate `PubSubTopID`s for headers and data in go-header.
-* Block sync over the P2P network works only when a full node is connected to the P2P network by specifying the initial seeds to connect to via `P2PConfig.Seeds` configuration parameter when starting the full node.
-* Node's context is passed down to all components to support graceful shutdown and cancellation.
-* The block manager supports custom signature payload providers for headers, enabling flexible signing schemes.
-* The block manager supports the separation of header and data structures in Evolve. This allows for expanding the sequencing scheme beyond single sequencing and enables the use of a decentralized sequencer mode. For detailed information on this architecture, see the [Header and Data Separation ADR](../../adr/adr-014-header-and-data-separation.md).
-* The block manager processes blocks with a minimal header format, which is designed to eliminate dependency on CometBFT's header format and can be used to produce an execution layer tailored header if needed. For details on this header structure, see the [Evolve Minimal Header](../../adr/adr-015-rollkit-minimal-header.md) specification.
+- The block manager loads the initial state from the local store and uses genesis if not found in the local store, when the node (re)starts.
+- The default mode for sequencer nodes is normal (not lazy).
+- The sequencer can produce empty blocks.
+- In lazy aggregation mode, the block manager maintains consistency with the DA layer by producing empty blocks at regular intervals, ensuring a 1:1 mapping between DA layer blocks and execution layer blocks.
+- The lazy aggregation mechanism uses a dual timer approach:
+  - A `blockTimer` that triggers block production when transactions are available
+  - A `lazyTimer` that ensures blocks are produced even during periods of inactivity
+- Empty batches are handled differently in lazy mode - instead of discarding them, they are returned with the `ErrNoBatch` error, allowing the caller to create empty blocks with proper timestamps.
+- Transaction notifications from the `Reaper` to the `Manager` are handled via a non-blocking notification channel (`txNotifyCh`) to prevent backpressure.
+- The block manager enforces `MaxPendingHeadersAndData` limit to prevent unbounded growth of pending queues during DA submission issues.
+- Headers and data are submitted separately to the DA layer using different namespaces, supporting the header/data separation architecture.
+- The block manager uses persistent caches for headers and data to track seen items and DA inclusion status.
+- Namespace migration is handled transparently, with automatic detection and state persistence to optimize future operations.
+- The system supports backward compatibility with legacy single-namespace deployments while transitioning to separate namespaces.
+- Gas price management includes automatic adjustment with `GasMultiplier` on DA submission retries.
+- The block manager uses persistent storage (disk) when the `root_dir` and `db_path` configuration parameters are specified in `config.yaml` file under the app directory. If these configuration parameters are not specified, the in-memory storage is used, which will not be persistent if the node stops.
+- The block manager does not re-apply blocks when they transition from soft confirmed to DA included status. The block is only marked DA included in the caches.
+- Header and data stores use separate prefixes for isolation in the underlying database.
+- The genesis `ChainID` is used to create separate `PubSubTopID`s for headers and data in go-header.
+- Block sync over the P2P network works only when a full node is connected to the P2P network by specifying the initial seeds to connect to via `P2PConfig.Seeds` configuration parameter when starting the full node.
+- Node's context is passed down to all components to support graceful shutdown and cancellation.
+- The block manager supports custom signature payload providers for headers, enabling flexible signing schemes.
+- The block manager supports the separation of header and data structures in Evolve. This allows for expanding the sequencing scheme beyond single sequencing and enables the use of a decentralized sequencer mode. For detailed information on this architecture, see the [Header and Data Separation ADR](../../adr/adr-014-header-and-data-separation.md).
+- The block manager processes blocks with a minimal header format, which is designed to eliminate dependency on CometBFT's header format and can be used to produce an execution layer tailored header if needed. For details on this header structure, see the [Evolve Minimal Header](../../adr/adr-015-rollkit-minimal-header.md) specification.
 
 ## Metrics