`LogsDBChainIngester` informational findings

[tothtamas28]

Consider reordering check and function call in processBlockLogs

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L660-L663

diff --git a/op-interop-filter/filter/logsdb_chain_ingester.go b/op-interop-filter/filter/logsdb_chain_ingester.go
index 7db2d9a00a..43d2639556 100644
--- a/op-interop-filter/filter/logsdb_chain_ingester.go
+++ b/op-interop-filter/filter/logsdb_chain_ingester.go
@@ -657,9 +657,9 @@ func (c *LogsDBChainIngester) processBlockLogs(blockInfo eth.BlockInfo, blockID

 	var logIndex uint32

-	parentBlock := eth.BlockID{Hash: blockInfo.ParentHash(), Number: blockNum - 1}
-	if blockNum == 0 {
-		parentBlock = eth.BlockID{}
+	parentBlock := eth.BlockID{}
+	if blockNum != 0 {
+		parentBlock = eth.BlockID{Hash: blockInfo.ParentHash(), Number: blockNum - 1}
 	}

 	for _, receipt := range receipts {

Missing nil check for rollupCfg in NewLogsDBChainIngester

In NewLogsDBChainIngester, rollupCfg is stored without a nil check:

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L126

When calling Start, this results in a nil pointer dereference in calculateStartingBlock:

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L342

ethClient and rpcClient are not closed if logsDB.Close() returns an error

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L158-L169

SetError discards IsUint64 result on conversion

Therefore, if chainID does not fit into an uint64, the error to be set is recorded under the wrong chain ID.

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L192-L195

Temporary directory is not cleaned up if LogsDB creation fails

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L358-L370

Consider swapping condition order

For readability and a minor performance benefit.

diff --git a/op-interop-filter/filter/logsdb_chain_ingester.go b/op-interop-filter/filter/logsdb_chain_ingester.go
index 7db2d9a00a..a82c662b61 100644
--- a/op-interop-filter/filter/logsdb_chain_ingester.go
+++ b/op-interop-filter/filter/logsdb_chain_ingester.go
@@ -279,7 +279,7 @@ func (c *LogsDBChainIngester) GetExecMsgsAtTimestamp(timestamp uint64) ([]Includ
        }

        latestBlock, ok := c.logsDB.LatestSealedBlock()
-       if !c.earliestIngestedBlockSet.Load() || !ok {
+       if !ok || !c.earliestIngestedBlockSet.Load() {
                return nil, nil
        }
        earliest := c.earliestIngestedBlock.Load()

Consider using a write lock in findAndSetEarliestBlock

The function starts by acquiring a read lock.

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L313-L315

However, it sets two fields, which in theory might interleave.

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L332-L333

(Note that earliestIngestedBlockSet.Store(true) is idempotent, therefore this is not a bug.)

Consider refactoring sealParentBlock

The function seals the block whose number it's passed to. Its naming on the other hand suggests that it's the parent of the block that gets sealed.

To make the function's purpose clear, consider renaming it's argument and local variables:

diff --git a/op-interop-filter/filter/logsdb_chain_ingester.go b/op-interop-filter/filter/logsdb_chain_ingester.go
index 7db2d9a00a..c2522381d9 100644
--- a/op-interop-filter/filter/logsdb_chain_ingester.go
+++ b/op-interop-filter/filter/logsdb_chain_ingester.go
@@ -553,21 +553,21 @@ func (c *LogsDBChainIngester) checkReorg(head eth.BlockInfo) error {
 	return fmt.Errorf("reorg detected")
 }

-func (c *LogsDBChainIngester) sealParentBlock(blockNum uint64) error {
-	c.log.Info("Sealing parent block as starting point", "block", blockNum)
+func (c *LogsDBChainIngester) sealParentBlock(parentNum uint64) error {
+	c.log.Info("Sealing parent block as starting point", "block", parentNum)

-	blockInfo, err := c.ethClient.InfoByNumber(c.ctx, blockNum)
+	parentInfo, err := c.ethClient.InfoByNumber(c.ctx, parentNum)
 	if err != nil {
 		return fmt.Errorf("failed to get block info: %w", err)
 	}

-	blockID := eth.BlockID{Hash: blockInfo.Hash(), Number: blockInfo.NumberU64()}
+	parentID := eth.BlockID{Hash: parentInfo.Hash(), Number: parentInfo.NumberU64()}

 	c.mu.Lock()
 	defer c.mu.Unlock()

-	parentHash := blockInfo.ParentHash()
-	if err := c.logsDB.SealBlock(parentHash, blockID, blockInfo.Time()); err != nil {
+	grandParentHash := parentInfo.ParentHash()
+	if err := c.logsDB.SealBlock(grandParentHash, parentID, parentInfo.Time()); err != nil {
 		return fmt.Errorf("failed to seal block: %w", err)
 	}

@@ -575,7 +575,7 @@ func (c *LogsDBChainIngester) sealParentBlock(blockNum uint64) error {
 	// an anchor checkpoint. earliestIngestedBlock will be set in ingestBlock when
 	// the first block with actual log data is ingested.

-	c.log.Info("Sealed parent block", "block", blockNum, "hash", blockID.Hash)
+	c.log.Info("Sealed parent block", "block", parentNum, "hash", parentID.Hash)
 	return nil
 }

or pulling the parent-specific log messages to the call site and generalizing the function:

diff --git a/op-interop-filter/filter/logsdb_chain_ingester.go b/op-interop-filter/filter/logsdb_chain_ingester.go
index 7db2d9a00a..8e8618894d 100644
--- a/op-interop-filter/filter/logsdb_chain_ingester.go
+++ b/op-interop-filter/filter/logsdb_chain_ingester.go
@@ -510,10 +510,18 @@ func (c *LogsDBChainIngester) initIngestion() (uint64, error) {
 	}

 	// Fresh start: seal parent block as anchor
-	if err := c.sealParentBlock(startingBlock - 1); err != nil {
+	parentNum := startingBlock - 1
+	c.log.Info("Sealing parent block as starting point", "block", parentNum)
+
+	if err := c.sealBlockByNum(parentNum); err != nil {
 		return 0, fmt.Errorf("failed to seal parent block: %w", err)
 	}

+	// Note: We don't set earliestIngestedBlock here because the parent block is just
+	// an anchor checkpoint. earliestIngestedBlock will be set in ingestBlock when
+	// the first block with actual log data is ingested.
+	c.log.Info("Sealed parent block", "block", parentNum)
+
 	c.log.Info("Starting fresh ingestion",
 		"from", startingBlock,
 		"to", head.NumberU64(),
@@ -553,9 +561,7 @@ func (c *LogsDBChainIngester) checkReorg(head eth.BlockInfo) error {
 	return fmt.Errorf("reorg detected")
 }

-func (c *LogsDBChainIngester) sealParentBlock(blockNum uint64) error {
-	c.log.Info("Sealing parent block as starting point", "block", blockNum)
-
+func (c *LogsDBChainIngester) sealBlockByNum(blockNum uint64) error {
 	blockInfo, err := c.ethClient.InfoByNumber(c.ctx, blockNum)
 	if err != nil {
 		return fmt.Errorf("failed to get block info: %w", err)
@@ -571,11 +577,6 @@ func (c *LogsDBChainIngester) sealParentBlock(blockNum uint64) error {
 		return fmt.Errorf("failed to seal block: %w", err)
 	}

-	// Note: We don't set earliestIngestedBlock here because the parent block is just
-	// an anchor checkpoint. earliestIngestedBlock will be set in ingestBlock when
-	// the first block with actual log data is ingested.
-
-	c.log.Info("Sealed parent block", "block", blockNum, "hash", blockID.Hash)
 	return nil
 }

Consider sharing the LogsDB interface between op-supernode and op-interop-filter

Both op-supernode and op-interop-filter are backed by logs.DB from op-supervisor, but they handle the dependency inconsistently:

• op-supernode (to be precise, the interop activity) defines and depends on a LogsDB interface
• op-interop-filter (LogsDBChainIngester) depends directly on the concrete implementation

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L49

Since op-interop-filter (aside from LogsDBChainIngester.initLogsDB) only depends on methods covered by the interface, the interface could be moved to a shared location (e.g. op-supervisor) so both packages can depend on the abstraction rather than the implementation. This would improve testability and make the dependency boundary explicit.

Consider extracting a function for the DB resume logic in initIngestion

diff --git a/op-interop-filter/filter/logsdb_chain_ingester.go b/op-interop-filter/filter/logsdb_chain_ingester.go
index 7db2d9a00a..7b5736d5ea 100644
--- a/op-interop-filter/filter/logsdb_chain_ingester.go
+++ b/op-interop-filter/filter/logsdb_chain_ingester.go
@@ -471,6 +471,11 @@ func (c *LogsDBChainIngester) runIngestion() {

 // initIngestion performs one-time setup and returns the first block to ingest.
 func (c *LogsDBChainIngester) initIngestion() (uint64, error) {
+	nextBlock, resumed := c.tryResumeFromExistingDB()
+	if resumed {
+		return nextBlock, nil
+	}
+
 	head, err := c.ethClient.InfoByLabel(c.ctx, eth.Unsafe)
 	if err != nil {
 		return 0, fmt.Errorf("failed to get current head: %w", err)
@@ -492,23 +497,6 @@ func (c *LogsDBChainIngester) initIngestion() (uint64, error) {

 	c.log.Info("Determined starting block", "block", startingBlock, "startTimestamp", c.startTimestamp)

-	// Check if we have existing data to resume from
-	c.mu.RLock()
-	latestSealed, hasSealed := c.logsDB.LatestSealedBlock()
-	c.mu.RUnlock()
-
-	if hasSealed {
-		// Resume from existing DB
-		nextBlock := latestSealed.Number + 1
-		c.log.Info("Resuming from existing DB", "lastSealed", latestSealed.Number, "resumeFrom", nextBlock)
-
-		if !c.earliestIngestedBlockSet.Load() {
-			c.findAndSetEarliestBlock(latestSealed.Number)
-		}
-
-		return nextBlock, nil
-	}
-
 	// Fresh start: seal parent block as anchor
 	if err := c.sealParentBlock(startingBlock - 1); err != nil {
 		return 0, fmt.Errorf("failed to seal parent block: %w", err)
@@ -522,6 +510,27 @@ func (c *LogsDBChainIngester) initIngestion() (uint64, error) {
 	return startingBlock, nil
 }

+func (c *LogsDBChainIngester) tryResumeFromExistingDB() (uint64, bool) {
+	// Check if we have existing data to resume from
+	c.mu.RLock()
+	latestSealed, hasSealed := c.logsDB.LatestSealedBlock()
+	c.mu.RUnlock()
+
+	if !hasSealed {
+		return 0, false
+	}
+
+	// Resume from existing DB
+	nextBlock := latestSealed.Number + 1
+	c.log.Info("Resuming from existing DB", "lastSealed", latestSealed.Number, "resumeFrom", nextBlock)
+
+	if !c.earliestIngestedBlockSet.Load() {
+		c.findAndSetEarliestBlock(latestSealed.Number)
+	}
+
+	return nextBlock, true
+}
+
 // checkReorg checks if a reorg occurred when head moves behind our progress.
 func (c *LogsDBChainIngester) checkReorg(head eth.BlockInfo) error {
 	headNum := head.NumberU64()

Inconsistent computation of starting block in initIngestion

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L480-L491

If head.NumberU64() == 0 or c.calculateStartingBlock() == 0, the result of the function is 1, i.e. min(head.NumberU64(), c.calculateStartingBlock()) + 1. Otherwise, it's min(head.NumberU64(), c.calculateStartingBlock()).

Consider making the two cases consistent:

diff --git a/op-interop-filter/filter/logsdb_chain_ingester.go b/op-interop-filter/filter/logsdb_chain_ingester.go
index 7db2d9a00a..5b793e8db5 100644
--- a/op-interop-filter/filter/logsdb_chain_ingester.go
+++ b/op-interop-filter/filter/logsdb_chain_ingester.go
@@ -477,19 +477,7 @@ func (c *LogsDBChainIngester) initIngestion() (uint64, error) {
 	}
 	c.log.Info("Current chain head", "block", head.NumberU64(), "hash", head.Hash())

-	startingBlock := c.calculateStartingBlock()
-
-	// Clamp to head if needed
-	if startingBlock > head.NumberU64() {
-		startingBlock = head.NumberU64()
-	}
-
-	// Guard against underflow: genesis block has no parent to seal
-	if startingBlock == 0 {
-		startingBlock = 1
-		c.log.Info("Starting from block 1 (genesis has no parent to seal)")
-	}
-
+	startingBlock := min(head.NumberU64(), c.calculateStartingBlock()) + 1
 	c.log.Info("Determined starting block", "block", startingBlock, "startTimestamp", c.startTimestamp)

 	// Check if we have existing data to resume from

This also makes it obvious that there is a previous block for sealParentBlock to seal in the database. The function can even be refactored to avoid the subtraction startingBlock - 1 altogether, and instead do

rootBlock := min(head.NumberU64(), c.calculateStartingBlock())
// ...
if err := c.sealParentBlock(rootBlock); err != nil {
   // ...
}
// ...
return rootBlock + 1, nil

hasLatest should be asserted instead of branched on in ingestBlock

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L599-L604

Reason:

1. After a successful call to initIngestion, there should be at least one sealed block in the LogsDB.
2. ingestBlock is only called if initIngestion succeeds.
3. The LogsDB is only appended to, in particular, Rewind is never called.

Consider implementing findAndSetEarliestBlock in terms of LogsDB.FirstSealedBlock

The current implementation is based on an iteration and multiple database queries.

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L313-L335

But logs.DB might already implement the same or a similar functionality:

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-supernode/supernode/activity/interop/logdb.go#L25-L26

Avoid subtraction in runIngestion

This is not a bug, since nextBlock should be at least 1:

• It initialized from initIngestion, which should return at least 1.
• It is only ever incremented in the function.

Nevertheless, it is easy to make the absence of underflow immediately obvious:

diff --git a/op-interop-filter/filter/logsdb_chain_ingester.go b/op-interop-filter/filter/logsdb_chain_ingester.go
index 7db2d9a00a..f003840b24 100644
--- a/op-interop-filter/filter/logsdb_chain_ingester.go
+++ b/op-interop-filter/filter/logsdb_chain_ingester.go
@@ -423,7 +423,7 @@ func (c *LogsDBChainIngester) runIngestion() {
 		}

 		// Reorg detection: if head moved behind our progress, check hash
-		if head.NumberU64() < nextBlock-1 {
+		if head.NumberU64()+1 < nextBlock {
 			if err := c.checkReorg(head); err != nil {
 				continue
 			}

checkReorg logs spurious error if earliestIngestedBlock is unset

If !c.earliestIngestedBlockSet.Load(), checkReorg performs c.BlockHashAt(headNum).

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L527-L541

Since on the call site headNum < nextBlock - 1, where nextBlock - 1 is the root of the LogsDB in this case, c.BlockHashAt(headNum) fails and logs an error.

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L425-L430

Instead, c.earliestIngestedBlockSet.Load() should be checked separately.

diff --git a/op-interop-filter/filter/logsdb_chain_ingester.go b/op-interop-filter/filter/logsdb_chain_ingester.go
index 7db2d9a00a..15a47e7574 100644
--- a/op-interop-filter/filter/logsdb_chain_ingester.go
+++ b/op-interop-filter/filter/logsdb_chain_ingester.go
@@ -526,8 +526,13 @@ func (c *LogsDBChainIngester) initIngestion() (uint64, error) {
 func (c *LogsDBChainIngester) checkReorg(head eth.BlockInfo) error {
 	headNum := head.NumberU64()

+	if !c.earliestIngestedBlockSet.Load() {
+		c.log.Debug("Earliest block has not been set yet, can't verify", "head", headNum)
+		return nil
+	}
+
 	// If head is before our earliest block, we can't verify - this is expected
-	if c.earliestIngestedBlockSet.Load() && headNum < c.earliestIngestedBlock.Load() {
+	if headNum < c.earliestIngestedBlock.Load() {
 		c.log.Debug("Head before our earliest block, can't verify",
 			"head", headNum, "earliest", c.earliestIngestedBlock.Load())
 		return nil

After the fix, ealiestIngestedBlock.Load() <= headNum < nextBlock - 1 and thus headNum should be present in the database. Therefore, c.BlockHashAt(headNum) should pass, so the failure case should be modified to set the error state as it indicates a DB inconsistency that has to be resolved manually.

No output if the error is set

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L410-L413

Consider periodically logging a warning message to indicate that the ingester is still operational.

Consider adding a Rewind() method

Currently, if the ingester enters an error state, LogsDB consistency must be restored manually and the ingester restarted.

Consider adding a Rewind() method that rolls the DB back to a known-good state and optionally clears the error, enabling recovery without a full restart. This also moves the ingester closer to a model where it owns the DB, preventing inconsistent external changes.

Document in processBlockLogs that execMsg == nil is a valid case

In processBlockLogs, the result of processors.DecodeExecutingMessageLog is checked for errors.

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L669-L672

However, there are cases when execMsg == nil, even though err == nil.

https://github.com/ethereum-optimism/optimism/blob/53d3861a03f5a1b9c23b23096d209523679dcba7/op-supervisor/supervisor/backend/processors/executing_message.go#L31-L35

https://github.com/ethereum-optimism/optimism/blob/53d3861a03f5a1b9c23b23096d209523679dcba7/op-supervisor/supervisor/backend/processors/executing_message.go#L14-L23

In such a case, AddLog is called with nil as execMsg.

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/logsdb_chain_ingester.go#L674-L676

Add a comment to indicate that this is a valid case.