fix(rsk): run installed BlockVerifier regardless of TrustRPC

op-service/sources/eth_client.go

@@ -243,6 +243,10 @@ func (s *EthClient) runHeaderVerify(ctx context.Context, hdr *RPCHeader) error {
 // runBlockVerify runs the configured BlockVerifier hook if non-nil,
 // otherwise falls back to the default RPCBlock.Verify() (block hash +
 // DeriveSha tx-trie root + L1/L2 withdrawals).
+//
+// Callers gate the default path on !trustRPC, but an installed hook runs
+// regardless of trustRPC: trusting the RPC only waives the default Ethereum
+// checks, not a verifier installed explicitly to replace them (PAYROLLUP-117).
 func (s *EthClient) runBlockVerify(ctx context.Context, b *RPCBlock) error {
 	if s.blockVerifier != nil {
 		return s.blockVerifier(ctx, b.CreateGethHeader(), types.Transactions(b.Transactions))
@@ -256,6 +260,10 @@ func (s *EthClient) runBlockVerify(ctx context.Context, b *RPCBlock) error {
 // hash reported by the RPC. It is the single source of truth for both
 // HeaderBy* and InfoBy*.
 //
+// Unlike blockCall/payloadCall, header verification stays gated on !trustRPC
+// even when a HeaderVerifier hook is set: with trustRPC=true no header
+// verification runs at all (hybrid policy, PAYROLLUP-117).
+//
 // The trusted hash matters on non-Ethereum L1s (RSK / RSKIP-92) where
 // go-ethereum's header.Hash() recomputation does not match the canonical
 // chain hash; callers wrapping the result as eth.BlockInfo must use
@@ -301,7 +309,9 @@ func (s *EthClient) blockCall(ctx context.Context, method string, id rpcBlockID)
 	if block == nil {
 		return nil, nil, common.Hash{}, ethereum.NotFound
 	}
-	if !s.trustRPC {
+	// Hybrid policy (PAYROLLUP-117): trustRPC only skips the default Ethereum
+	// verification; an explicitly installed BlockVerifier hook always runs.
+	if !s.trustRPC || s.blockVerifier != nil {
 		if err := s.runBlockVerify(ctx, block); err != nil {
 			return nil, nil, common.Hash{}, err
 		}
@@ -329,7 +339,9 @@ func (s *EthClient) payloadCall(ctx context.Context, method string, id rpcBlockI
 	if block == nil {
 		return nil, ethereum.NotFound
 	}
-	if !s.trustRPC {
+	// Hybrid policy (PAYROLLUP-117): trustRPC only skips the default Ethereum
+	// verification; an explicitly installed BlockVerifier hook always runs.
+	if !s.trustRPC || s.blockVerifier != nil {
 		if err := s.runBlockVerify(ctx, block); err != nil {
 			return nil, err
 		}

op-service/sources/eth_client_rsk_test.go

@@ -17,6 +17,7 @@ package sources
 import (
 	"context"
 	"errors"
+	"fmt"
 	"math/rand"
 	"testing"
 
@@ -26,6 +27,7 @@ import (
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core/types"
 
+	"github.com/ethereum-optimism/optimism/op-service/bigs"
 	"github.com/ethereum-optimism/optimism/op-service/eth"
 )
 
@@ -69,7 +71,7 @@ func TestRSK_EthClient_HeaderVerifierHook(t *testing.T) {
 		err := s.runHeaderVerify(ctx, good)
 		require.ErrorIs(t, err, wantErr)
 		require.NotNil(t, gotHeader)
-		require.Equal(t, uint64(good.Number), gotHeader.Number.Uint64())
+		require.Equal(t, uint64(good.Number), bigs.Uint64Strict(gotHeader.Number))
 	})
 
 	t.Run("non-nil hook overrides the default (accepts a tampered header)", func(t *testing.T) {
@@ -264,3 +266,205 @@ func TestRSK_RPCReceiptsFetcher_ReceiptsValidatorHook(t *testing.T) {
 		require.NotContains(t, err.Error(), "unexpected nil block number")
 	})
 }
+
+// rskEthClientConfig returns a copy of testEthClientConfig with the given
+// TrustRPC value and BlockVerifier hook.
+func rskEthClientConfig(trustRPC bool, hook BlockVerifierFn) *EthClientConfig {
+	cfg := *testEthClientConfig
+	cfg.TrustRPC = trustRPC
+	cfg.BlockVerifier = hook
+	return &cfg
+}
+
+// rskMockBlockRPC returns a mockRPC serving the given block for the fullTx
+// eth_getBlockByHash / eth_getBlockByNumber calls, counting every hit so
+// tests can assert whether a result was served from cache or re-fetched.
+func rskMockBlockRPC(ctx context.Context, block *RPCBlock, rpcCalls *int) *mockRPC {
+	m := new(mockRPC)
+	serve := func(args mock.Arguments) {
+		*rpcCalls++
+		*(args[1].(**RPCBlock)) = block
+	}
+	m.On("CallContext", ctx, new(*RPCBlock),
+		"eth_getBlockByHash", []any{block.Hash, true}).Run(serve).Return([]error{nil})
+	m.On("CallContext", ctx, new(*RPCBlock),
+		"eth_getBlockByNumber", []any{block.Number.String(), true}).Run(serve).Return([]error{nil})
+	return m
+}
+
+// TestRSK_BlockVerifierRunsWithTrustRPC (PAYROLLUP-117 T1) pins the hybrid
+// validation policy: an installed BlockVerifier hook must run on the blockCall
+// and payloadCall paths even with TrustRPC=true. TrustRPC only disables the
+// default Ethereum verification, never a non-Ethereum hook installed
+// explicitly to replace it.
+func TestRSK_BlockVerifierRunsWithTrustRPC(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("blockCall by hash", func(t *testing.T) {
+		block, _ := randomRpcBlockAndReceipts(rand.New(rand.NewSource(1)), 2)
+		hookCalls := 0
+		var gotTxs types.Transactions
+		hook := func(_ context.Context, _ *types.Header, txs types.Transactions) error {
+			hookCalls++
+			gotTxs = txs
+			return nil
+		}
+		s, err := NewEthClient(rskMockBlockRPC(ctx, block, new(int)), nil, nil, rskEthClientConfig(true, hook))
+		require.NoError(t, err)
+
+		_, txs, err := s.InfoAndTxsByHash(ctx, block.Hash)
+		require.NoError(t, err)
+		require.Len(t, txs, len(block.Transactions))
+		require.Equal(t, 1, hookCalls, "BlockVerifier must run on blockCall despite TrustRPC=true")
+		require.Len(t, gotTxs, len(block.Transactions), "hook must receive the block transactions")
+	})
+
+	t.Run("blockCall by number", func(t *testing.T) {
+		block, _ := randomRpcBlockAndReceipts(rand.New(rand.NewSource(2)), 2)
+		hookCalls := 0
+		hook := func(_ context.Context, _ *types.Header, _ types.Transactions) error {
+			hookCalls++
+			return nil
+		}
+		s, err := NewEthClient(rskMockBlockRPC(ctx, block, new(int)), nil, nil, rskEthClientConfig(true, hook))
+		require.NoError(t, err)
+
+		_, _, err = s.InfoAndTxsByNumber(ctx, uint64(block.Number))
+		require.NoError(t, err)
+		require.Equal(t, 1, hookCalls, "BlockVerifier must run on blockCall despite TrustRPC=true")
+	})
+
+	t.Run("payloadCall", func(t *testing.T) {
+		block, _ := randomRpcBlockAndReceipts(rand.New(rand.NewSource(3)), 2)
+		hookCalls := 0
+		hook := func(_ context.Context, _ *types.Header, _ types.Transactions) error {
+			hookCalls++
+			return nil
+		}
+		s, err := NewEthClient(rskMockBlockRPC(ctx, block, new(int)), nil, nil, rskEthClientConfig(true, hook))
+		require.NoError(t, err)
+
+		envelope, err := s.PayloadByHash(ctx, block.Hash)
+		require.NoError(t, err)
+		require.Equal(t, block.Hash, envelope.ExecutionPayload.BlockHash)
+		require.Equal(t, 1, hookCalls, "BlockVerifier must run on payloadCall despite TrustRPC=true")
+	})
+}
+
+// TestRSK_BlockVerifierMismatchHaltsWithTrustRPC (PAYROLLUP-117 T2) pins the
+// mismatch behavior with TrustRPC=true: the hook's error (carrying both the
+// header root and the computed root, as rsk/l1source VerifyRSKBlock produces)
+// halts the call, keeps both roots in the message, and the failed result is
+// never cached — a later call re-queries the RPC.
+func TestRSK_BlockVerifierMismatchHaltsWithTrustRPC(t *testing.T) {
+	ctx := context.Background()
+	headerRoot := randHash()
+	computedRoot := randHash()
+	wantErr := fmt.Errorf("tx root mismatch: header %s, computed %s", headerRoot, computedRoot)
+
+	t.Run("blockCall", func(t *testing.T) {
+		block, _ := randomRpcBlockAndReceipts(rand.New(rand.NewSource(4)), 2)
+		hookCalls := 0
+		hook := func(_ context.Context, _ *types.Header, _ types.Transactions) error {
+			hookCalls++
+			return wantErr
+		}
+		rpcCalls := 0
+		s, err := NewEthClient(rskMockBlockRPC(ctx, block, &rpcCalls), nil, nil, rskEthClientConfig(true, hook))
+		require.NoError(t, err)
+
+		_, _, err = s.InfoAndTxsByHash(ctx, block.Hash)
+		require.ErrorIs(t, err, wantErr, "mismatch must halt blockCall despite TrustRPC=true")
+		require.ErrorContains(t, err, headerRoot.String(), "error must keep the header root")
+		require.ErrorContains(t, err, computedRoot.String(), "error must keep the computed root")
+
+		_, _, err = s.InfoAndTxsByHash(ctx, block.Hash)
+		require.ErrorIs(t, err, wantErr)
+		require.Equal(t, 2, rpcCalls, "failed result must not be cached: second call re-queries the RPC")
+		require.Equal(t, 2, hookCalls)
+	})
+
+	t.Run("payloadCall", func(t *testing.T) {
+		block, _ := randomRpcBlockAndReceipts(rand.New(rand.NewSource(5)), 2)
+		hookCalls := 0
+		hook := func(_ context.Context, _ *types.Header, _ types.Transactions) error {
+			hookCalls++
+			return wantErr
+		}
+		rpcCalls := 0
+		s, err := NewEthClient(rskMockBlockRPC(ctx, block, &rpcCalls), nil, nil, rskEthClientConfig(true, hook))
+		require.NoError(t, err)
+
+		_, err = s.PayloadByHash(ctx, block.Hash)
+		require.ErrorIs(t, err, wantErr, "mismatch must halt payloadCall despite TrustRPC=true")
+		require.ErrorContains(t, err, headerRoot.String(), "error must keep the header root")
+		require.ErrorContains(t, err, computedRoot.String(), "error must keep the computed root")
+
+		_, err = s.PayloadByHash(ctx, block.Hash)
+		require.ErrorIs(t, err, wantErr)
+		require.Equal(t, 2, rpcCalls, "failed result must not be cached: second call re-queries the RPC")
+		require.Equal(t, 2, hookCalls)
+	})
+}
+
+// TestRSK_NilBlockVerifierKeepsUpstreamSemantics (PAYROLLUP-117 T3,
+// characterization) protects the upstream semantics the hybrid policy must
+// not change: with TrustRPC=true and no hook installed, no verification runs
+// at all — a block whose hash would fail the default Verify is accepted.
+func TestRSK_NilBlockVerifierKeepsUpstreamSemantics(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("blockCall", func(t *testing.T) {
+		good, _ := randomRpcBlockAndReceipts(rand.New(rand.NewSource(6)), 2)
+		bad := *good
+		bad.Hash = rskTamperHash(bad.Hash) // default Verify would reject this block
+
+		s, err := NewEthClient(rskMockBlockRPC(ctx, &bad, new(int)), nil, nil, rskEthClientConfig(true, nil))
+		require.NoError(t, err)
+
+		_, txs, err := s.InfoAndTxsByHash(ctx, bad.Hash)
+		require.NoError(t, err, "TrustRPC=true with nil hook must skip all verification (upstream semantics)")
+		require.Len(t, txs, len(bad.Transactions))
+	})
+
+	t.Run("payloadCall", func(t *testing.T) {
+		good, _ := randomRpcBlockAndReceipts(rand.New(rand.NewSource(7)), 2)
+		bad := *good
+		bad.Hash = rskTamperHash(bad.Hash)
+
+		s, err := NewEthClient(rskMockBlockRPC(ctx, &bad, new(int)), nil, nil, rskEthClientConfig(true, nil))
+		require.NoError(t, err)
+
+		envelope, err := s.PayloadByHash(ctx, bad.Hash)
+		require.NoError(t, err, "TrustRPC=true with nil hook must skip all verification (upstream semantics)")
+		require.Equal(t, bad.Hash, envelope.ExecutionPayload.BlockHash)
+	})
+}
+
+// TestRSK_HeaderVerifyEthereumFallbackRejectsRSKIP92Hash (PAYROLLUP-117 T6,
+// characterization) pins the known limit of the hybrid policy: with
+// TrustRPC=false and no HeaderVerifier installed, headerCall falls back to the
+// default Ethereum hash recomputation (hdr.VerifyHash), which cannot match an
+// RSKIP-92-style header hash. Running against RSK therefore requires
+// TrustRPC=true; this test documents the boundary, it is not a bug to fix.
+func TestRSK_HeaderVerifyEthereumFallbackRejectsRSKIP92Hash(t *testing.T) {
+	ctx := context.Background()
+	_, rhdr := randHeader()
+	rskip92 := *rhdr
+	// An RSKIP-92 hash is not the keccak of the geth-style header; a tampered
+	// hash stands in for it.
+	rskip92.Hash = rskTamperHash(rhdr.Hash)
+
+	m := new(mockRPC)
+	m.On("CallContext", ctx, new(*RPCHeader),
+		"eth_getBlockByHash", []any{rskip92.Hash, false}).Run(func(args mock.Arguments) {
+		*args[1].(**RPCHeader) = &rskip92
+	}).Return([]error{nil})
+
+	s, err := NewEthClient(m, nil, nil, rskEthClientConfig(false, nil))
+	require.NoError(t, err)
+
+	_, err = s.InfoByHash(ctx, rskip92.Hash)
+	require.ErrorContains(t, err, "failed to verify block hash",
+		"Ethereum VerifyHash fallback must reject an RSKIP-92-style header hash")
+}