From f139cc22adfd9c260f400b461bd23624749ca97c Mon Sep 17 00:00:00 2001 From: fedejinich Date: Tue, 11 Aug 2026 23:34:31 -0300 Subject: [PATCH 1/3] fix(rsk): run installed BlockVerifier regardless of TrustRPC --- op-service/sources/eth_client.go | 16 +- op-service/sources/eth_client_rsk_test.go | 206 +++++++++++++++++++++- 2 files changed, 219 insertions(+), 3 deletions(-) diff --git a/op-service/sources/eth_client.go b/op-service/sources/eth_client.go index d30a5901224..12235233f54 100644 --- a/op-service/sources/eth_client.go +++ b/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 } diff --git a/op-service/sources/eth_client_rsk_test.go b/op-service/sources/eth_client_rsk_test.go index 606bf8ba3ba..23914639fc3 100644 --- a/op-service/sources/eth_client_rsk_test.go +++ b/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") +} From c304876a83cc8eba92fce02c37201479087eb512 Mon Sep 17 00:00:00 2001 From: fedejinich Date: Wed, 12 Aug 2026 21:50:56 -0300 Subject: [PATCH 2/3] docs(rsk): record EthClient trust divergence --- AGENTS_rsk.md | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/AGENTS_rsk.md b/AGENTS_rsk.md index 31ef2a9a98c..8f90a5a69b0 100644 --- a/AGENTS_rsk.md +++ b/AGENTS_rsk.md @@ -76,6 +76,36 @@ wherever they conflict.** Read this before acting on anything in those files. RSK-only backoff to a tiny adapter, a ~3-line diff from upstream instead of a full rewrite (PAYROLLUP-87). +### Recorded divergence: `op-service/sources` block gates (PAYROLLUP-117) + +`op-service/sources/eth_client.go` intentionally differs from upstream at +exactly two verification gates: + +- `blockCall`: upstream `if !s.trustRPC` is + `if !s.trustRPC || s.blockVerifier != nil` in this fork. +- `payloadCall`: the same upstream gate has the same fork change. + +The purpose is narrow: an explicitly installed `BlockVerifier` must validate +the transaction root before a full block or execution payload can be cached or +returned when RSK runs with `TrustRPC=true`. A nil hook preserves upstream +behavior exactly: trusted-RPC calls skip block verification. The patch does not +change `headerCall`, `GetProof`, receipts, RPC decoding, cache placement, or the +default verification path used with `TrustRPC=false`. + +Do not simplify either gate back to `if !s.trustRPC`: that silently disables +RSK transaction-root validation. Conversely, do not broaden this exception to +`HeaderVerifier`. Rootstack deliberately leaves that hook nil because the +decoded `RPCHeader` has already discarded the RSK-specific fields needed to +recompute an RSKIP-92 block hash. + +The hermetic regression contract is in `eth_client_rsk_test.go`: +`TestRSK_BlockVerifierRunsWithTrustRPC` covers block and payload execution, +`TestRSK_BlockVerifierMismatchHaltsWithTrustRPC` covers error propagation and +no-cache/re-fetch behavior, `TestRSK_NilBlockVerifierKeepsUpstreamSemantics` +pins the nil-hook default, and +`TestRSK_HeaderVerifyEthereumFallbackRejectsRSKIP92Hash` records why RSK cannot +use the `TrustRPC=false` Ethereum header fallback. + ## Testing RSK changes Upstream tests only guard untouched behavior; some RSK branches even make an @@ -115,7 +145,7 @@ Run a package's RSK tests with, e.g.: | `op-service/txmgr/metrics/tx_metrics.go` | nil-guarded fee gauges (nil base/blob fee) | `tx_metrics_rsk_test.go` | | `op-node/rollup/derive/l1_traversal{,_managed}.go` | reorg-aware receipt-fetch `NotFound` handling | `l1_traversal_rsk_test.go` | | `op-node/rollup/derive/l1_block_info.go` | nil L1 `BaseFee` ⇒ 0 (no panic) | `l1_block_info_rsk_test.go` | -| `op-service/sources/{eth_client,receipts_rpc,types}.go` | pluggable block / header / receipts / tx-hash hooks | `eth_client_rsk_test.go` | +| `op-service/sources/{eth_client,receipts_rpc,types}.go` | pluggable block / header / receipts / tx-hash hooks; `blockCall` and `payloadCall` run an installed `BlockVerifier` with `TrustRPC=true` | `eth_client_rsk_test.go` (`TestRSK_BlockVerifierRunsWithTrustRPC`, `TestRSK_BlockVerifierMismatchHaltsWithTrustRPC`, `TestRSK_NilBlockVerifierKeepsUpstreamSemantics`, `TestRSK_HeaderVerifyEthereumFallbackRejectsRSKIP92Hash`) | | `op-deployer/pkg/deployer/broadcaster` | `TxMgrConfigHook` on `KeyedBroadcasterOpts` | `keyed_rsk_test.go` | | `op-deployer/pkg/deployer/forge` | `ExtraScriptOpts` on `Client` | `client_rsk_test.go` | | `op-proposer/contracts/disputegamefactory.go` | skip un-loadable games; surface ctx errors | `disputegamefactory_rsk_test.go` | From f3360d4f189b9cdaa64ae64ef5fc19fde0b7a8b4 Mon Sep 17 00:00:00 2001 From: fedejinich Date: Wed, 12 Aug 2026 21:57:57 -0300 Subject: [PATCH 3/3] Revert "docs(rsk): record EthClient trust divergence" This reverts commit c304876a83cc8eba92fce02c37201479087eb512. --- AGENTS_rsk.md | 32 +------------------------------- 1 file changed, 1 insertion(+), 31 deletions(-) diff --git a/AGENTS_rsk.md b/AGENTS_rsk.md index 8f90a5a69b0..31ef2a9a98c 100644 --- a/AGENTS_rsk.md +++ b/AGENTS_rsk.md @@ -76,36 +76,6 @@ wherever they conflict.** Read this before acting on anything in those files. RSK-only backoff to a tiny adapter, a ~3-line diff from upstream instead of a full rewrite (PAYROLLUP-87). -### Recorded divergence: `op-service/sources` block gates (PAYROLLUP-117) - -`op-service/sources/eth_client.go` intentionally differs from upstream at -exactly two verification gates: - -- `blockCall`: upstream `if !s.trustRPC` is - `if !s.trustRPC || s.blockVerifier != nil` in this fork. -- `payloadCall`: the same upstream gate has the same fork change. - -The purpose is narrow: an explicitly installed `BlockVerifier` must validate -the transaction root before a full block or execution payload can be cached or -returned when RSK runs with `TrustRPC=true`. A nil hook preserves upstream -behavior exactly: trusted-RPC calls skip block verification. The patch does not -change `headerCall`, `GetProof`, receipts, RPC decoding, cache placement, or the -default verification path used with `TrustRPC=false`. - -Do not simplify either gate back to `if !s.trustRPC`: that silently disables -RSK transaction-root validation. Conversely, do not broaden this exception to -`HeaderVerifier`. Rootstack deliberately leaves that hook nil because the -decoded `RPCHeader` has already discarded the RSK-specific fields needed to -recompute an RSKIP-92 block hash. - -The hermetic regression contract is in `eth_client_rsk_test.go`: -`TestRSK_BlockVerifierRunsWithTrustRPC` covers block and payload execution, -`TestRSK_BlockVerifierMismatchHaltsWithTrustRPC` covers error propagation and -no-cache/re-fetch behavior, `TestRSK_NilBlockVerifierKeepsUpstreamSemantics` -pins the nil-hook default, and -`TestRSK_HeaderVerifyEthereumFallbackRejectsRSKIP92Hash` records why RSK cannot -use the `TrustRPC=false` Ethereum header fallback. - ## Testing RSK changes Upstream tests only guard untouched behavior; some RSK branches even make an @@ -145,7 +115,7 @@ Run a package's RSK tests with, e.g.: | `op-service/txmgr/metrics/tx_metrics.go` | nil-guarded fee gauges (nil base/blob fee) | `tx_metrics_rsk_test.go` | | `op-node/rollup/derive/l1_traversal{,_managed}.go` | reorg-aware receipt-fetch `NotFound` handling | `l1_traversal_rsk_test.go` | | `op-node/rollup/derive/l1_block_info.go` | nil L1 `BaseFee` ⇒ 0 (no panic) | `l1_block_info_rsk_test.go` | -| `op-service/sources/{eth_client,receipts_rpc,types}.go` | pluggable block / header / receipts / tx-hash hooks; `blockCall` and `payloadCall` run an installed `BlockVerifier` with `TrustRPC=true` | `eth_client_rsk_test.go` (`TestRSK_BlockVerifierRunsWithTrustRPC`, `TestRSK_BlockVerifierMismatchHaltsWithTrustRPC`, `TestRSK_NilBlockVerifierKeepsUpstreamSemantics`, `TestRSK_HeaderVerifyEthereumFallbackRejectsRSKIP92Hash`) | +| `op-service/sources/{eth_client,receipts_rpc,types}.go` | pluggable block / header / receipts / tx-hash hooks | `eth_client_rsk_test.go` | | `op-deployer/pkg/deployer/broadcaster` | `TxMgrConfigHook` on `KeyedBroadcasterOpts` | `keyed_rsk_test.go` | | `op-deployer/pkg/deployer/forge` | `ExtraScriptOpts` on `Client` | `client_rsk_test.go` | | `op-proposer/contracts/disputegamefactory.go` | skip un-loadable games; surface ctx errors | `disputegamefactory_rsk_test.go` |