`LockstepCrossValidator` informational findings

[tothtamas28]

Consider enforcing inclusionTimestamp <= execTimestamp in validateMessageTiming if timeout > 0

As a result, all the events become ordered for the timeout > 0 case:

   initTimestamp
<  inclusionTimestamp
<= execTimestamp
<  execTimestamp + timeout              // maxExecTimestamp
<= initTimestamp + messageExpiryWindow  // expiresAt

Alternatively, note that there's a single call site where timeout > 0, and there execTimestamp == inclusionTimestamp:

_audits_Ethereum-optimism_optimism_interopv2/op-interop-filter/filter/lockstep_cross_validator.go

Lines 196 to 205 in be00aaa

	// Validate timing constraints (including timeout if set)
	if err := validateMessageTiming(
	access.Timestamp,
	execDescriptor.Timestamp,
	v.messageExpiryWindow,
	execDescriptor.Timeout,
	execDescriptor.Timestamp,
	); err != nil {
	return err
	}

By eliminating execTimestamp, the checks can be simplified, resulting in the following ordering for all timeout values:

   initTimestamp
<  inclusionTimestamp
<= inclusionTimestamp + timeout         // maxExecTimestamp
<= initTimestamp + messageExpiryWindow  // expiresAt

diff --git a/op-interop-filter/filter/lockstep_cross_validator.go b/op-interop-filter/filter/lockstep_cross_validator.go
index 9a9539c105..415d11f992 100644
--- a/op-interop-filter/filter/lockstep_cross_validator.go
+++ b/op-interop-filter/filter/lockstep_cross_validator.go
@@ -127,7 +127,7 @@ func (v *LockstepCrossValidator) CrossValidatedTimestamp() (uint64, bool) {
 //   - timeout: optional max execution delay (0 = disabled)
 //   - execTimestamp: execution timestamp (only used if timeout > 0)
 func validateMessageTiming(
-       initTimestamp, inclusionTimestamp, messageExpiryWindow, timeout, execTimestamp uint64,
+       initTimestamp, inclusionTimestamp, messageExpiryWindow, timeout uint64,
 ) error {
        // Rule 1: init must be strictly before inclusion
        if initTimestamp >= inclusionTimestamp {
@@ -142,26 +142,18 @@ func validateMessageTiming(
                        initTimestamp, messageExpiryWindow, types.ErrConflict)
        }

-       // Rule 3: message must not be expired at inclusion
-       if expiresAt < inclusionTimestamp {
-               return fmt.Errorf("initiating message expired: init %d + expiry window %d = %d < inclusion %d: %w",
-                       initTimestamp, messageExpiryWindow, expiresAt, inclusionTimestamp, types.ErrConflict)
+       // Rule 3: message must not expire before timeout deadline
+       maxExecTimestamp := inclusionTimestamp + timeout
+       if maxExecTimestamp < inclusionTimestamp {
+               return fmt.Errorf("overflow in max exec timestamp calculation: timestamp %d + timeout %d: %w",
+                       inclusionTimestamp, timeout, types.ErrConflict)
        }
-
-       // Rule 4: if timeout set, message must not expire before timeout deadline
-       if timeout > 0 {
-               maxExecTimestamp := execTimestamp + timeout
-               if maxExecTimestamp < execTimestamp {
-                       return fmt.Errorf("overflow in max exec timestamp calculation: timestamp %d + timeout %d: %w",
-                               execTimestamp, timeout, types.ErrConflict)
-               }
-               if expiresAt < maxExecTimestamp {
-                       return fmt.Errorf("initiating message will expire before timeout: "+
-                               "init %d + expiry %d = %d < exec %d + timeout %d = %d: %w",
-                               initTimestamp, messageExpiryWindow, expiresAt,
-                               execTimestamp, timeout, maxExecTimestamp,
-                               types.ErrConflict)
-               }
+       if expiresAt < maxExecTimestamp {
+               return fmt.Errorf("initiating message will expire before timeout: "+
+                       "init %d + expiry %d = %d < exec %d + timeout %d = %d: %w",
+                       initTimestamp, messageExpiryWindow, expiresAt,
+                       inclusionTimestamp, timeout, maxExecTimestamp,
+                       types.ErrConflict)
        }

        return nil

Consider checking for ingester errors in getMinIngestedTimestamp

Then errors will be checked in ValidateAccessEntry as well and not only in advanceValidation.

Also note that the Ready() check can be safely removed from advanceValidation, as it's also performed in getMinIngesterTimestamp.

diff --git a/op-interop-filter/filter/lockstep_cross_validator.go b/op-interop-filter/filter/lockstep_cross_validator.go
index 9a9539c105..fb0b3d4c7d 100644
--- a/op-interop-filter/filter/lockstep_cross_validator.go
+++ b/op-interop-filter/filter/lockstep_cross_validator.go
@@ -272,16 +272,6 @@ func (v *LockstepCrossValidator) advanceValidation() {
                return
        }

-       // All chains must be ready and error-free
-       for _, ingester := range v.chains {
-               if ingester.Error() != nil {
-                       return
-               }
-               if !ingester.Ready() {
-                       return
-               }
-       }
-
        minIngestedTs, ok := v.getMinIngestedTimestamp()
        if !ok {
                return
@@ -349,6 +339,10 @@ func (v *LockstepCrossValidator) getMinIngestedTimestamp() (uint64, bool) {
        var minTs uint64
        first := true
        for _, ingester := range v.chains {
+               // All chains must be ready and error-free
+               if ingester.Error() != nil {
+                       return 0, false
+               }
                if !ingester.Ready() {
                        return 0, false
                }

startTimestamp is assumed cross-valid at initialization

At initialization, crossValidatedTs is set from startTimestamp without checking its cross-validity.

https://github.com/ethereum-optimism/optimism/blob/be00aaa2cc3c8bea80e916d17c30a2b5d8d87696/op-interop-filter/filter/lockstep_cross_validator.go#L292-L297

Therefore, in this case, crossValidatedTs is not actually cross-validated.

Check cross-validity for startTimestamp, or document that the range of validated timestamps is (startTimestamp, crossValidatedTs], i.e. the lower bound is exclusive.