Merge branch 'develop' into fix/l2oo-constructor

op-program/host/cmd/main.go

@@ -21,6 +21,7 @@ import (
 	"github.com/ethereum-optimism/optimism/op-program/host/version"
 	"github.com/ethereum-optimism/optimism/op-program/preimage"
 	oplog "github.com/ethereum-optimism/optimism/op-service/log"
+	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/log"
 	"github.com/urfave/cli"
 )
@@ -106,45 +107,66 @@ type L2Source struct {
 
 // FaultProofProgram is the programmatic entry-point for the fault proof program
 func FaultProofProgram(logger log.Logger, cfg *config.Config) error {
-	cfg.Rollup.LogDescription(logger, chaincfg.L2ChainIDToNetworkName)
-	if !cfg.FetchingEnabled() {
-		return errors.New("offline mode not supported")
+	if err := cfg.Check(); err != nil {
+		return fmt.Errorf("invalid config: %w", err)
 	}
+	cfg.Rollup.LogDescription(logger, chaincfg.L2ChainIDToNetworkName)
 
 	ctx := context.Background()
-	kv := kvstore.NewMemKV()
-
-	logger.Info("Connecting to L1 node", "l1", cfg.L1URL)
-	l1RPC, err := client.NewRPC(ctx, logger, cfg.L1URL)
-	if err != nil {
-		return fmt.Errorf("failed to setup L1 RPC: %w", err)
+	var kv kvstore.KV
+	if cfg.DataDir == "" {
+		logger.Info("Using in-memory storage")
+		kv = kvstore.NewMemKV()
+	} else {
+		logger.Info("Creating disk storage", "datadir", cfg.DataDir)
+		if err := os.MkdirAll(cfg.DataDir, 0755); err != nil {
+			return fmt.Errorf("creating datadir: %w", err)
+		}
+		kv = kvstore.NewDiskKV(cfg.DataDir)
 	}
 
-	logger.Info("Connecting to L2 node", "l2", cfg.L2URL)
-	l2RPC, err := client.NewRPC(ctx, logger, cfg.L2URL)
-	if err != nil {
-		return fmt.Errorf("failed to setup L2 RPC: %w", err)
-	}
+	var preimageOracle preimage.OracleFn
+	var hinter preimage.HinterFn
+	if cfg.FetchingEnabled() {
+		logger.Info("Connecting to L1 node", "l1", cfg.L1URL)
+		l1RPC, err := client.NewRPC(ctx, logger, cfg.L1URL)
+		if err != nil {
+			return fmt.Errorf("failed to setup L1 RPC: %w", err)
+		}
 
-	l1ClCfg := sources.L1ClientDefaultConfig(cfg.Rollup, cfg.L1TrustRPC, cfg.L1RPCKind)
-	l2ClCfg := sources.L2ClientDefaultConfig(cfg.Rollup, true)
-	l1Cl, err := sources.NewL1Client(l1RPC, logger, nil, l1ClCfg)
-	if err != nil {
-		return fmt.Errorf("failed to create L1 client: %w", err)
-	}
-	l2Cl, err := sources.NewL2Client(l2RPC, logger, nil, l2ClCfg)
-	if err != nil {
-		return fmt.Errorf("failed to create L2 client: %w", err)
-	}
-	l2DebugCl := &L2Source{L2Client: l2Cl, DebugClient: sources.NewDebugClient(l2RPC.CallContext)}
+		logger.Info("Connecting to L2 node", "l2", cfg.L2URL)
+		l2RPC, err := client.NewRPC(ctx, logger, cfg.L2URL)
+		if err != nil {
+			return fmt.Errorf("failed to setup L2 RPC: %w", err)
+		}
 
-	logger.Info("Setting up pre-fetcher")
-	prefetch := prefetcher.NewPrefetcher(l1Cl, l2DebugCl, kv)
-	preimageOracle := asOracleFn(ctx, prefetch)
-	hinter := asHinter(prefetch)
+		l1ClCfg := sources.L1ClientDefaultConfig(cfg.Rollup, cfg.L1TrustRPC, cfg.L1RPCKind)
+		l2ClCfg := sources.L2ClientDefaultConfig(cfg.Rollup, true)
+		l1Cl, err := sources.NewL1Client(l1RPC, logger, nil, l1ClCfg)
+		if err != nil {
+			return fmt.Errorf("failed to create L1 client: %w", err)
+		}
+		l2Cl, err := sources.NewL2Client(l2RPC, logger, nil, l2ClCfg)
+		if err != nil {
+			return fmt.Errorf("failed to create L2 client: %w", err)
+		}
+		l2DebugCl := &L2Source{L2Client: l2Cl, DebugClient: sources.NewDebugClient(l2RPC.CallContext)}
+
+		logger.Info("Setting up pre-fetcher")
+		prefetch := prefetcher.NewPrefetcher(l1Cl, l2DebugCl, kv)
+		preimageOracle = asOracleFn(func(key common.Hash) ([]byte, error) {
+			return prefetch.GetPreimage(ctx, key)
+		})
+		hinter = asHinter(prefetch.Hint)
+	} else {
+		logger.Info("Using offline mode. All required pre-images must be pre-populated.")
+		preimageOracle = asOracleFn(kv.Get)
+		hinter = func(v preimage.Hint) {
+			logger.Debug("ignoring prefetch hint", "hint", v)
+		}
+	}
 	l1Source := l1.NewSource(logger, preimageOracle, hinter, cfg.L1Head)
 
-	logger.Info("Connecting to L2 node", "l2", cfg.L2URL)
 	l2Source, err := l2.NewEngine(logger, preimageOracle, hinter, cfg)
 	if err != nil {
 		return fmt.Errorf("connect l2 oracle: %w", err)
@@ -166,9 +188,9 @@ func FaultProofProgram(logger log.Logger, cfg *config.Config) error {
 	return nil
 }
 
-func asOracleFn(ctx context.Context, prefetcher *prefetcher.Prefetcher) preimage.OracleFn {
+func asOracleFn(getter func(key common.Hash) ([]byte, error)) preimage.OracleFn {
 	return func(key preimage.Key) []byte {
-		pre, err := prefetcher.GetPreimage(ctx, key.PreimageKey())
+		pre, err := getter(key.PreimageKey())
 		if err != nil {
 			panic(fmt.Errorf("preimage unavailable for key %v: %w", key, err))
 		}
@@ -176,9 +198,9 @@ func asOracleFn(ctx context.Context, prefetcher *prefetcher.Prefetcher) preimage
 	}
 }
 
-func asHinter(prefetcher *prefetcher.Prefetcher) preimage.HinterFn {
+func asHinter(hint func(hint string) error) preimage.HinterFn {
 	return func(v preimage.Hint) {
-		err := prefetcher.Hint(v.Hint())
+		err := hint(v.Hint())
 		if err != nil {
 			panic(fmt.Errorf("hint rejected %v: %w", v, err))
 		}

op-program/host/config/config.go

 package config
 
 import (
+	"encoding/json"
 	"errors"
+	"fmt"
+	"os"
 
 	opnode "github.com/ethereum-optimism/optimism/op-node"
 	"github.com/ethereum-optimism/optimism/op-node/rollup"
 	"github.com/ethereum-optimism/optimism/op-node/sources"
 	"github.com/ethereum-optimism/optimism/op-program/host/flags"
 	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/core"
+	"github.com/ethereum/go-ethereum/params"
 	"github.com/urfave/cli"
 )
 
@@ -18,18 +23,28 @@ var (
 	ErrInvalidL2Head       = errors.New("invalid l2 head")
 	ErrL1AndL2Inconsistent = errors.New("l1 and l2 options must be specified together or both omitted")
 	ErrInvalidL2Claim      = errors.New("invalid l2 claim")
+	ErrDataDirRequired     = errors.New("datadir must be specified when in non-fetching mode")
 )
 
 type Config struct {
-	Rollup        *rollup.Config
-	L2URL         string
-	L2GenesisPath string
-	L1Head        common.Hash
-	L2Head        common.Hash
-	L2Claim       common.Hash
-	L1URL         string
-	L1TrustRPC    bool
-	L1RPCKind     sources.RPCProviderKind
+	Rollup *rollup.Config
+	// DataDir is the directory to read/write pre-image data from/to.
+	//If not set, an in-memory key-value store is used and fetching data must be enabled
+	DataDir string
+
+	// L1Head is the block has of the L1 chain head block
+	L1Head     common.Hash
+	L1URL      string
+	L1TrustRPC bool
+	L1RPCKind  sources.RPCProviderKind
+
+	// L2Head is the agreed L2 block to start derivation from
+	L2Head common.Hash
+	L2URL  string
+	// L2Claim is the claimed L2 output root to verify
+	L2Claim common.Hash
+	// L2ChainConfig is the op-geth chain config for the L2 execution engine
+	L2ChainConfig *params.ChainConfig
 }
 
 func (c *Config) Check() error {
@@ -48,12 +63,15 @@ func (c *Config) Check() error {
 	if c.L2Claim == (common.Hash{}) {
 		return ErrInvalidL2Claim
 	}
-	if c.L2GenesisPath == "" {
+	if c.L2ChainConfig == nil {
 		return ErrMissingL2Genesis
 	}
 	if (c.L1URL != "") != (c.L2URL != "") {
 		return ErrL1AndL2Inconsistent
 	}
+	if !c.FetchingEnabled() && c.DataDir == "" {
+		return ErrDataDirRequired
+	}
 	return nil
 }
 
@@ -62,10 +80,10 @@ func (c *Config) FetchingEnabled() bool {
 }
 
 // NewConfig creates a Config with all optional values set to the CLI default value
-func NewConfig(rollupCfg *rollup.Config, l2GenesisPath string, l1Head common.Hash, l2Head common.Hash, l2Claim common.Hash) *Config {
+func NewConfig(rollupCfg *rollup.Config, l2Genesis *params.ChainConfig, l1Head common.Hash, l2Head common.Hash, l2Claim common.Hash) *Config {
 	return &Config{
 		Rollup:        rollupCfg,
-		L2GenesisPath: l2GenesisPath,
+		L2ChainConfig: l2Genesis,
 		L1Head:        l1Head,
 		L2Head:        l2Head,
 		L2Claim:       l2Claim,
@@ -93,10 +111,16 @@ func NewConfigFromCLI(ctx *cli.Context) (*Config, error) {
 	if l1Head == (common.Hash{}) {
 		return nil, ErrInvalidL1Head
 	}
+	l2GenesisPath := ctx.GlobalString(flags.L2GenesisPath.Name)
+	l2ChainConfig, err := loadChainConfigFromGenesis(l2GenesisPath)
+	if err != nil {
+		return nil, fmt.Errorf("invalid genesis: %w", err)
+	}
 	return &Config{
 		Rollup:        rollupCfg,
+		DataDir:       ctx.GlobalString(flags.DataDir.Name),
 		L2URL:         ctx.GlobalString(flags.L2NodeAddr.Name),
-		L2GenesisPath: ctx.GlobalString(flags.L2GenesisPath.Name),
+		L2ChainConfig: l2ChainConfig,
 		L2Head:        l2Head,
 		L2Claim:       l2Claim,
 		L1Head:        l1Head,
@@ -105,3 +129,16 @@ func NewConfigFromCLI(ctx *cli.Context) (*Config, error) {
 		L1RPCKind:     sources.RPCProviderKind(ctx.GlobalString(flags.L1RPCProviderKind.Name)),
 	}, nil
 }
+
+func loadChainConfigFromGenesis(path string) (*params.ChainConfig, error) {
+	data, err := os.ReadFile(path)
+	if err != nil {
+		return nil, fmt.Errorf("read l2 genesis file: %w", err)
+	}
+	var genesis core.Genesis
+	err = json.Unmarshal(data, &genesis)
+	if err != nil {
+		return nil, fmt.Errorf("parse l2 genesis file: %w", err)
+	}
+	return genesis.Config, nil
+}

op-program/host/config/config_test.go

@@ -6,16 +6,18 @@ import (
 	"github.com/ethereum-optimism/optimism/op-node/chaincfg"
 	"github.com/ethereum-optimism/optimism/op-node/rollup"
 	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/params"
 	"github.com/stretchr/testify/require"
 )
 
 var validRollupConfig = &chaincfg.Goerli
-var validL2GenesisPath = "genesis.json"
+var validL2Genesis = params.GoerliChainConfig
 var validL1Head = common.Hash{0xaa}
 var validL2Head = common.Hash{0xbb}
 var validL2Claim = common.Hash{0xcc}
 
-func TestDefaultConfigIsValid(t *testing.T) {
+// TestValidConfigIsValid checks that the config provided by validConfig is actually valid
+func TestValidConfigIsValid(t *testing.T) {
 	err := validConfig().Check()
 	require.NoError(t, err)
 }
@@ -59,7 +61,7 @@ func TestL2ClaimRequired(t *testing.T) {
 
 func TestL2GenesisRequired(t *testing.T) {
 	config := validConfig()
-	config.L2GenesisPath = ""
+	config.L2ChainConfig = nil
 	err := config.Check()
 	require.ErrorIs(t, err, ErrMissingL2Genesis)
 }
@@ -121,6 +123,17 @@ func TestFetchingEnabled(t *testing.T) {
 	})
 }
 
+func TestRequireDataDirInNonFetchingMode(t *testing.T) {
+	cfg := validConfig()
+	cfg.DataDir = ""
+	cfg.L1URL = ""
+	cfg.L2URL = ""
+	err := cfg.Check()
+	require.ErrorIs(t, err, ErrDataDirRequired)
+}
+
 func validConfig() *Config {
-	return NewConfig(validRollupConfig, validL2GenesisPath, validL1Head, validL2Head, validL2Claim)
+	cfg := NewConfig(validRollupConfig, validL2Genesis, validL1Head, validL2Head, validL2Claim)
+	cfg.DataDir = "/tmp/configTest"
+	return cfg
 }

op-program/host/flags/flags.go

@@ -26,6 +26,11 @@ var (
 		Usage:  fmt.Sprintf("Predefined network selection. Available networks: %s", strings.Join(chaincfg.AvailableNetworks(), ", ")),
 		EnvVar: service.PrefixEnvVar(envVarPrefix, "NETWORK"),
 	}
+	DataDir = cli.StringFlag{
+		Name:   "datadir",
+		Usage:  "Directory to use for preimage data storage. Default uses in-memory storage",
+		EnvVar: service.PrefixEnvVar(envVarPrefix, "DATADIR"),
+	}
 	L2NodeAddr = cli.StringFlag{
 		Name:   "l2",
 		Usage:  "Address of L2 JSON-RPC endpoint to use (eth and debug namespace required)",
@@ -85,6 +90,7 @@ var requiredFlags = []cli.Flag{
 var programFlags = []cli.Flag{
 	RollupConfig,
 	Network,
+	DataDir,
 	L2NodeAddr,
 	L1NodeAddr,
 	L1TrustRPC,

op-program/host/l2/l2.go

@@ -2,25 +2,17 @@ package l2
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
-	"os"
 
 	cll2 "github.com/ethereum-optimism/optimism/op-program/client/l2"
 	"github.com/ethereum-optimism/optimism/op-program/host/config"
 	"github.com/ethereum-optimism/optimism/op-program/preimage"
-	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/log"
-	"github.com/ethereum/go-ethereum/params"
 )
 
 func NewEngine(logger log.Logger, pre preimage.Oracle, hint preimage.Hinter, cfg *config.Config) (*cll2.OracleEngine, error) {
 	oracle := cll2.NewCachingOracle(cll2.NewPreimageOracle(pre, hint))
-	genesis, err := loadL2Genesis(cfg)
-	if err != nil {
-		return nil, err
-	}
-	engineBackend, err := cll2.NewOracleBackedL2Chain(logger, oracle, genesis, cfg.L2Head)
+	engineBackend, err := cll2.NewOracleBackedL2Chain(logger, oracle, cfg.L2ChainConfig, cfg.L2Head)
 	if err != nil {
 		return nil, fmt.Errorf("create l2 chain: %w", err)
 	}
@@ -34,16 +26,3 @@ func NewFetchingOracle(ctx context.Context, logger log.Logger, cfg *config.Confi
 	}
 	return oracle, nil
 }
-
-func loadL2Genesis(cfg *config.Config) (*params.ChainConfig, error) {
-	data, err := os.ReadFile(cfg.L2GenesisPath)
-	if err != nil {
-		return nil, fmt.Errorf("read l2 genesis file: %w", err)
-	}
-	var genesis core.Genesis
-	err = json.Unmarshal(data, &genesis)
-	if err != nil {
-		return nil, fmt.Errorf("parse l2 genesis file: %w", err)
-	}
-	return genesis.Config, nil
-}

packages/contracts-bedrock/.gas-snapshot

@@ -7,14 +7,14 @@ Bytes_toNibbles_Test:test_toNibbles_expectedResult5Bytes_works() (gas: 6132)
 Bytes_toNibbles_Test:test_toNibbles_zeroLengthInput_works() (gas: 944)
 CrossDomainMessenger_BaseGas_Test:test_baseGas_succeeds() (gas: 20097)
 CrossDomainOwnable2_Test:test_onlyOwner_notMessenger_reverts() (gas: 8416)
-CrossDomainOwnable2_Test:test_onlyOwner_notOwner2_reverts() (gas: 61872)
+CrossDomainOwnable2_Test:test_onlyOwner_notOwner2_reverts() (gas: 57254)
 CrossDomainOwnable2_Test:test_onlyOwner_notOwner_reverts() (gas: 16566)
-CrossDomainOwnable2_Test:test_onlyOwner_succeeds() (gas: 75933)
+CrossDomainOwnable2_Test:test_onlyOwner_succeeds() (gas: 73282)
 CrossDomainOwnable3_Test:test_constructor_succeeds() (gas: 10554)
 CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notMessenger_reverts() (gas: 28334)
-CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner2_reverts() (gas: 76381)
+CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner2_reverts() (gas: 73730)
 CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner_reverts() (gas: 31978)
-CrossDomainOwnable3_Test:test_crossDomainTransferOwnership_succeeds() (gas: 93938)
+CrossDomainOwnable3_Test:test_crossDomainTransferOwnership_succeeds() (gas: 91287)
 CrossDomainOwnable3_Test:test_localOnlyOwner_notOwner_reverts() (gas: 13193)
 CrossDomainOwnable3_Test:test_localOnlyOwner_succeeds() (gas: 35220)
 CrossDomainOwnable3_Test:test_localTransferOwnership_succeeds() (gas: 52128)
@@ -70,20 +70,18 @@ L1BlockTest:test_timestamp_succeeds() (gas: 7640)
 L1BlockTest:test_updateValues_succeeds() (gas: 60482)
 L1CrossDomainMessenger_Test:test_messageVersion_succeeds() (gas: 24715)
 L1CrossDomainMessenger_Test:test_relayMessage_legacyOldReplay_reverts() (gas: 49394)
-L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailureThenSuccess_reverts() (gas: 232952)
-L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailure_succeeds() (gas: 206446)
-L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterSuccess_reverts() (gas: 146819)
-L1CrossDomainMessenger_Test:test_relayMessage_legacy_succeeds() (gas: 79729)
-L1CrossDomainMessenger_Test:test_relayMessage_reentrancyDiffMessage_succeeds() (gas: 725335)
-L1CrossDomainMessenger_Test:test_relayMessage_reentrancySameMessage_reverts() (gas: 662298)
-L1CrossDomainMessenger_Test:test_relayMessage_retryAfterFailure_succeeds() (gas: 200353)
-L1CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 76665)
-L1CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 101282)
+L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailureThenSuccess_reverts() (gas: 209286)
+L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailure_succeeds() (gas: 203184)
+L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterSuccess_reverts() (gas: 123784)
+L1CrossDomainMessenger_Test:test_relayMessage_legacy_succeeds() (gas: 77098)
+L1CrossDomainMessenger_Test:test_relayMessage_retryAfterFailure_succeeds() (gas: 197091)
+L1CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 74034)
+L1CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 56540)
 L1CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 12365)
-L1CrossDomainMessenger_Test:test_replayMessage_withValue_reverts() (gas: 53445)
+L1CrossDomainMessenger_Test:test_replayMessage_withValue_reverts() (gas: 31063)
 L1CrossDomainMessenger_Test:test_sendMessage_succeeds() (gas: 304740)
 L1CrossDomainMessenger_Test:test_sendMessage_twice_succeeds() (gas: 1496124)
-L1CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 87194)
+L1CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 84563)
 L1CrossDomainMessenger_Test:test_xDomainSender_notSet_reverts() (gas: 24296)
 L1ERC721Bridge_Test:test_bridgeERC721To_localTokenZeroAddress_reverts() (gas: 52707)
 L1ERC721Bridge_Test:test_bridgeERC721To_remoteTokenZeroAddress_reverts() (gas: 27310)
@@ -120,15 +118,13 @@ L1StandardBridge_Getter_Test:test_getters_succeeds() (gas: 32173)
 L1StandardBridge_Initialize_Test:test_initialize_succeeds() (gas: 22050)
 L1StandardBridge_Receive_Test:test_receive_succeeds() (gas: 525438)
 L2CrossDomainMessenger_Test:test_messageVersion_succeeds() (gas: 8411)
-L2CrossDomainMessenger_Test:test_relayMessage_reentrancyDiffMessage_succeeds() (gas: 680395)
-L2CrossDomainMessenger_Test:test_relayMessage_reentrancySameMessage_reverts() (gas: 626456)
-L2CrossDomainMessenger_Test:test_relayMessage_retry_succeeds() (gas: 166461)
-L2CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 54980)
-L2CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 51448)
+L2CrossDomainMessenger_Test:test_relayMessage_retry_succeeds() (gas: 163159)
+L2CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 48640)
+L2CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 29021)
 L2CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 11711)
 L2CrossDomainMessenger_Test:test_sendMessage_succeeds() (gas: 122508)
 L2CrossDomainMessenger_Test:test_sendMessage_twice_succeeds() (gas: 134826)
-L2CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 54580)
+L2CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 48139)
 L2CrossDomainMessenger_Test:test_xDomainSender_senderNotSet_reverts() (gas: 10590)
 L2ERC721Bridge_Test:test_bridgeERC721To_localTokenZeroAddress_reverts() (gas: 26431)
 L2ERC721Bridge_Test:test_bridgeERC721To_remoteTokenZeroAddress_reverts() (gas: 21814)

packages/contracts-bedrock/.storage-layout

@@ -24,8 +24,7 @@
 | xDomainMsgSender       | address                  | 204  | 0      | 20    | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
 | msgNonce               | uint240                  | 205  | 0      | 30    | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
 | failedMessages         | mapping(bytes32 => bool) | 206  | 0      | 32    | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
-| reentrancyLocks        | mapping(bytes32 => bool) | 207  | 0      | 32    | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
-| __gap                  | uint256[41]              | 208  | 0      | 1312  | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
+| __gap                  | uint256[42]              | 207  | 0      | 1344  | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
 
 =======================
 ➡ contracts/L1/L1StandardBridge.sol:L1StandardBridge
@@ -135,8 +134,7 @@
 | xDomainMsgSender       | address                  | 204  | 0      | 20    | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
 | msgNonce               | uint240                  | 205  | 0      | 30    | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
 | failedMessages         | mapping(bytes32 => bool) | 206  | 0      | 32    | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
-| reentrancyLocks        | mapping(bytes32 => bool) | 207  | 0      | 32    | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
-| __gap                  | uint256[41]              | 208  | 0      | 1312  | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
+| __gap                  | uint256[42]              | 207  | 0      | 1344  | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
 
 =======================
 ➡ contracts/L2/L2StandardBridge.sol:L2StandardBridge

packages/contracts-bedrock/contracts/test/CrossDomainMessenger.t.sol

 // SPDX-License-Identifier: MIT
 pragma solidity 0.8.15;
 
-import { Messenger_Initializer, Reverter, CallerCaller } from "./CommonTest.t.sol";
+import { Messenger_Initializer, Reverter, CallerCaller, CommonTest } from "./CommonTest.t.sol";
+import { L1CrossDomainMessenger } from "../L1/L1CrossDomainMessenger.sol";
+
+// Libraries
+import { Predeploys } from "../libraries/Predeploys.sol";
+import { Hashing } from "../libraries/Hashing.sol";
+import { Encoding } from "../libraries/Encoding.sol";
 
 // CrossDomainMessenger_Test is for testing functionality which is common to both the L1 and L2
 // CrossDomainMessenger contracts. For simplicity, we use the L1 Messenger as the test contract.
@@ -17,3 +23,149 @@ contract CrossDomainMessenger_BaseGas_Test is Messenger_Initializer {
         L1Messenger.baseGas(hex"ff", _minGasLimit);
     }
 }
+
+/**
+ * @title ExternalRelay
+ * @notice A mock external contract called via the SafeCall inside
+ *         the CrossDomainMessenger's `relayMessage` function.
+ */
+contract ExternalRelay is CommonTest {
+    address internal op;
+    address internal fuzzedSender;
+    L1CrossDomainMessenger internal L1Messenger;
+
+    event FailedRelayedMessage(bytes32 indexed msgHash);
+
+    constructor(L1CrossDomainMessenger _l1Messenger, address _op) {
+        L1Messenger = _l1Messenger;
+        op = _op;
+    }
+
+    /**
+     * @notice Internal helper function to relay a message and perform assertions.
+     */
+    function _internalRelay(address _innerSender) internal {
+        address initialSender = L1Messenger.xDomainMessageSender();
+
+        bytes memory callMessage = getCallData();
+
+        bytes32 hash = Hashing.hashCrossDomainMessage({
+            _nonce: Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
+            _sender: _innerSender,
+            _target: address(this),
+            _value: 0,
+            _gasLimit: 0,
+            _data: callMessage
+        });
+
+        vm.expectEmit(true, true, true, true);
+        emit FailedRelayedMessage(hash);
+
+        vm.prank(address(op));
+        L1Messenger.relayMessage({
+            _nonce: Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
+            _sender: _innerSender,
+            _target: address(this),
+            _value: 0,
+            _minGasLimit: 0,
+            _message: callMessage
+        });
+
+        assertTrue(L1Messenger.failedMessages(hash));
+        assertFalse(L1Messenger.successfulMessages(hash));
+        assertEq(initialSender, L1Messenger.xDomainMessageSender());
+    }
+
+    /**
+     * @notice externalCallWithMinGas is called by the CrossDomainMessenger.
+     */
+    function externalCallWithMinGas() external payable {
+        for (uint256 i = 0; i < 10; i++) {
+            address _innerSender;
+            unchecked {
+                _innerSender = address(uint160(uint256(uint160(fuzzedSender)) + i));
+            }
+            _internalRelay(_innerSender);
+        }
+    }
+
+    /**
+     * @notice Helper function to get the callData for an `externalCallWithMinGas
+     */
+    function getCallData() public returns (bytes memory) {
+        return abi.encodeWithSelector(ExternalRelay.externalCallWithMinGas.selector);
+    }
+
+    /**
+     * @notice Helper function to set the fuzzed sender
+     */
+    function setFuzzedSender(address _fuzzedSender) public {
+        fuzzedSender = _fuzzedSender;
+    }
+}
+
+/**
+ * @title CrossDomainMessenger_RelayMessage_Test
+ * @notice Fuzz tests re-entrancy into the CrossDomainMessenger relayMessage function.
+ */
+contract CrossDomainMessenger_RelayMessage_Test is Messenger_Initializer {
+    // Storage slot of the l2Sender
+    uint256 constant senderSlotIndex = 50;
+
+    ExternalRelay public er;
+
+    function setUp() public override {
+        super.setUp();
+        er = new ExternalRelay(L1Messenger, address(op));
+    }
+
+    /**
+     * @dev This test mocks an OptimismPortal call to the L1CrossDomainMessenger via
+     *      the relayMessage function. The relayMessage function will then use SafeCall's
+     *      callWithMinGas to call the target with call data packed in the callMessage.
+     *      For this test, the callWithMinGas will call the mock ExternalRelay test contract
+     *      defined above, executing the externalCallWithMinGas function which will try to
+     *      re-enter the CrossDomainMessenger's relayMessage function, resulting in that message
+     *      being recorded as failed.
+     */
+    function testFuzz_relayMessageReenter_succeeds(address _sender, uint256 _gasLimit) external {
+        vm.assume(_sender != Predeploys.L2_CROSS_DOMAIN_MESSENGER);
+        address sender = Predeploys.L2_CROSS_DOMAIN_MESSENGER;
+
+        er.setFuzzedSender(_sender);
+        address target = address(er);
+        bytes memory callMessage = er.getCallData();
+
+        vm.expectCall(target, callMessage);
+
+        uint64 gasLimit = uint64(bound(_gasLimit, 0, 30_000_000));
+
+        bytes32 hash = Hashing.hashCrossDomainMessage({
+            _nonce: Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
+            _sender: sender,
+            _target: target,
+            _value: 0,
+            _gasLimit: gasLimit,
+            _data: callMessage
+        });
+
+        // set the value of op.l2Sender() to be the L2 Cross Domain Messenger.
+        vm.store(address(op), bytes32(senderSlotIndex), bytes32(abi.encode(sender)));
+        vm.prank(address(op));
+        L1Messenger.relayMessage({
+            _nonce: Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
+            _sender: sender,
+            _target: target,
+            _value: 0,
+            _minGasLimit: gasLimit,
+            _message: callMessage
+        });
+
+        assertTrue(L1Messenger.successfulMessages(hash));
+        assertEq(L1Messenger.failedMessages(hash), false);
+
+        // Ensures that the `xDomainMsgSender` is set back to `Predeploys.L2_CROSS_DOMAIN_MESSENGER`
+        vm.expectRevert("CrossDomainMessenger: xDomainMessageSender is not set");
+        L1Messenger.xDomainMessageSender();
+    }
+}

packages/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol

@@ -100,10 +100,6 @@ contract L1CrossDomainMessenger_Test is Messenger_Initializer {
         L1Messenger.xDomainMessageSender();
     }
 
-    // xDomainMessageSender: should return the xDomainMsgSender address
-    // TODO: might need a test contract
-    // function test_xDomainSenderSetCorrectly() external {}
-
     function test_relayMessage_v2_reverts() external {
         address target = address(0xabcd);
         address sender = Predeploys.L2_CROSS_DOMAIN_MESSENGER;
@@ -295,173 +291,6 @@ contract L1CrossDomainMessenger_Test is Messenger_Initializer {
         assertEq(L1Messenger.failedMessages(hash), true);
     }
 
-    // relayMessage: Should revert if the recipient is trying to reenter with the
-    // same message.
-    function test_relayMessage_reentrancySameMessage_reverts() external {
-        ConfigurableCaller caller = new ConfigurableCaller();
-        address target = address(caller);
-        address sender = Predeploys.L2_CROSS_DOMAIN_MESSENGER;
-        bytes memory callMessage = abi.encodeWithSelector(caller.call.selector);
-
-        bytes32 hash = Hashing.hashCrossDomainMessage(
-            Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
-            sender,
-            target,
-            0,
-            0,
-            callMessage
-        );
-
-        // Set the portal's `l2Sender` to the `sender`.
-        vm.store(address(op), bytes32(senderSlotIndex), bytes32(uint256(uint160(sender))));
-
-        // Act as the portal and call the `relayMessage` function with the `innerMessage`.
-        vm.prank(address(op));
-        vm.expectCall(target, callMessage);
-        L1Messenger.relayMessage(
-            Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
-            sender,
-            target,
-            0,
-            0,
-            callMessage
-        );
-
-        // Assert that the message failed to be relayed
-        assertFalse(L1Messenger.successfulMessages(hash));
-        assertTrue(L1Messenger.failedMessages(hash));
-
-        // Set the configurable caller's target to `L1Messenger` and set the payload to `relayMessage(...)`.
-        caller.setDoRevert(false);
-        caller.setTarget(address(L1Messenger));
-        caller.setPayload(
-            abi.encodeWithSelector(
-                L1Messenger.relayMessage.selector,
-                Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
-                sender,
-                target,
-                0,
-                0,
-                callMessage
-            )
-        );
-
-        // Attempt to replay the failed message, which will *not* immediately revert this time around,
-        // but attempt to reenter `relayMessage` with the same message hash. The reentrancy attempt should
-        // revert.
-        vm.expectEmit(true, true, true, true, target);
-        emit WhatHappened(
-            false,
-            abi.encodeWithSignature("Error(string)", "ReentrancyGuard: reentrant call")
-        );
-        L1Messenger.relayMessage(
-            Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }), // nonce
-            sender,
-            target,
-            0,
-            0,
-            callMessage
-        );
-
-        // Assert that the message still failed to be relayed.
-        assertFalse(L1Messenger.successfulMessages(hash));
-        assertTrue(L1Messenger.failedMessages(hash));
-    }
-
-    // relayMessage: should not revert if the recipient reenters `relayMessage` with a different
-    // message hash.
-    function test_relayMessage_reentrancyDiffMessage_succeeds() external {
-        ConfigurableCaller caller = new ConfigurableCaller();
-        address target = address(caller);
-        address sender = Predeploys.L2_CROSS_DOMAIN_MESSENGER;
-        bytes memory messageA = abi.encodeWithSelector(caller.call.selector);
-        bytes memory messageB = hex"";
-
-        bytes32 hashA = Hashing.hashCrossDomainMessage(
-            Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
-            sender,
-            target,
-            0,
-            0,
-            messageA
-        );
-        bytes32 hashB = Hashing.hashCrossDomainMessage(
-            Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
-            sender,
-            target,
-            0,
-            0,
-            messageB
-        );
-
-        // Set the portal's `l2Sender` to the `sender`.
-        vm.store(address(op), bytes32(senderSlotIndex), bytes32(uint256(uint160(sender))));
-
-        // Act as the portal and call the `relayMessage` function with both `messageA` and `messageB`.
-        vm.startPrank(address(op));
-
-        vm.expectCall(target, messageA);
-        L1Messenger.relayMessage(
-            Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
-            sender,
-            target,
-            0,
-            0,
-            messageA
-        );
-        vm.expectCall(target, messageB);
-        L1Messenger.relayMessage(
-            Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
-            sender,
-            target,
-            0,
-            0,
-            messageB
-        );
-
-        // Stop acting as the portal
-        vm.stopPrank();
-
-        // Assert that both messages failed to be relayed
-        assertFalse(L1Messenger.successfulMessages(hashA));
-        assertFalse(L1Messenger.successfulMessages(hashB));
-        assertTrue(L1Messenger.failedMessages(hashA));
-        assertTrue(L1Messenger.failedMessages(hashB));
-
-        // Set the configurable caller's target to `L1Messenger` and set the payload to `relayMessage(...)`.
-        caller.setDoRevert(false);
-        caller.setTarget(address(L1Messenger));
-        caller.setPayload(
-            abi.encodeWithSelector(
-                L1Messenger.relayMessage.selector,
-                Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
-                sender,
-                target,
-                0,
-                0,
-                messageB
-            )
-        );
-
-        // Attempt to replay the failed message, which will *not* immediately revert this time around,
-        // but attempt to reenter `relayMessage` with messageB. The reentrancy attempt should succeed
-        // because the message hashes are different.
-        vm.expectEmit(true, true, true, true, target);
-        emit WhatHappened(true, hex"");
-        L1Messenger.relayMessage(
-            Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
-            sender,
-            target,
-            0,
-            0,
-            messageA
-        );
-
-        // Assert that both messages are now in the `successfulMessages` mapping.
-        assertTrue(L1Messenger.successfulMessages(hashA));
-        assertTrue(L1Messenger.successfulMessages(hashB));
-    }
-
     function test_relayMessage_legacy_succeeds() external {
         address target = address(0xabcd);
         address sender = Predeploys.L2_CROSS_DOMAIN_MESSENGER;

packages/contracts-bedrock/contracts/test/L2CrossDomainMessenger.t.sol

@@ -230,168 +230,4 @@ contract L2CrossDomainMessenger_Test is Messenger_Initializer {
         assertEq(L2Messenger.successfulMessages(hash), true);
         assertEq(L2Messenger.failedMessages(hash), true);
     }
-
-    // relayMessage: Should revert if the recipient is trying to reenter with the
-    // same message.
-    function test_relayMessage_reentrancySameMessage_reverts() external {
-        ConfigurableCaller caller = new ConfigurableCaller();
-        address target = address(caller);
-        address sender = address(L1Messenger);
-        address l1XDMAlias = AddressAliasHelper.applyL1ToL2Alias(address(L1Messenger));
-        bytes memory callMessage = abi.encodeWithSelector(caller.call.selector);
-
-        bytes32 hash = Hashing.hashCrossDomainMessage(
-            Encoding.encodeVersionedNonce(0, 1),
-            sender,
-            target,
-            0,
-            0,
-            callMessage
-        );
-
-        // Act as the L1XDM and call the `relayMessage` function with the `innerMessage`.
-        vm.prank(l1XDMAlias);
-        vm.expectCall(target, callMessage);
-        L2Messenger.relayMessage(
-            Encoding.encodeVersionedNonce(0, 1),
-            sender,
-            target,
-            0,
-            0,
-            callMessage
-        );
-
-        // Assert that the message failed to be relayed
-        assertFalse(L2Messenger.successfulMessages(hash));
-        assertTrue(L2Messenger.failedMessages(hash));
-
-        // Set the configurable caller's target to `L2Messenger` and set the payload to `relayMessage(...)`.
-        caller.setDoRevert(false);
-        caller.setTarget(address(L2Messenger));
-        caller.setPayload(
-            abi.encodeWithSelector(
-                L2Messenger.relayMessage.selector,
-                Encoding.encodeVersionedNonce(0, 1),
-                sender,
-                target,
-                0,
-                0,
-                callMessage
-            )
-        );
-
-        // Attempt to replay the failed message, which will *not* immediately revert this time around,
-        // but attempt to reenter `relayMessage` with the same message hash. The reentrancy attempt should
-        // revert.
-        vm.expectEmit(true, true, true, true, target);
-        emit WhatHappened(
-            false,
-            abi.encodeWithSignature("Error(string)", "ReentrancyGuard: reentrant call")
-        );
-        L2Messenger.relayMessage(
-            Encoding.encodeVersionedNonce(0, 1),
-            sender,
-            target,
-            0,
-            0,
-            callMessage
-        );
-
-        // Assert that the message still failed to be relayed.
-        assertFalse(L2Messenger.successfulMessages(hash));
-        assertTrue(L2Messenger.failedMessages(hash));
-    }
-
-    // relayMessage: should not revert if the recipient reenters `relayMessage` with a different
-    // message hash.
-    function test_relayMessage_reentrancyDiffMessage_succeeds() external {
-        ConfigurableCaller caller = new ConfigurableCaller();
-        address target = address(caller);
-        address sender = address(L1Messenger);
-        address l1XDMAlias = AddressAliasHelper.applyL1ToL2Alias(address(L1Messenger));
-
-        bytes memory messageA = abi.encodeWithSelector(caller.call.selector);
-        bytes memory messageB = hex"";
-
-        bytes32 hashA = Hashing.hashCrossDomainMessage(
-            Encoding.encodeVersionedNonce(0, 1),
-            sender,
-            target,
-            0,
-            0,
-            messageA
-        );
-        bytes32 hashB = Hashing.hashCrossDomainMessage(
-            Encoding.encodeVersionedNonce(0, 1),
-            sender,
-            target,
-            0,
-            0,
-            messageB
-        );
-
-        // Act as the L1XDM and call the `relayMessage` function with both `messageA` and `messageB`.
-        vm.startPrank(l1XDMAlias);
-
-        vm.expectCall(target, messageA);
-        L2Messenger.relayMessage(
-            Encoding.encodeVersionedNonce(0, 1),
-            sender,
-            target,
-            0,
-            0,
-            messageA
-        );
-        vm.expectCall(target, messageB);
-        L2Messenger.relayMessage(
-            Encoding.encodeVersionedNonce(0, 1),
-            sender,
-            target,
-            0,
-            0,
-            messageB
-        );
-
-        // Stop acting as the L1XDM
-        vm.stopPrank();
-
-        // Assert that both messages failed to be relayed
-        assertFalse(L2Messenger.successfulMessages(hashA));
-        assertFalse(L2Messenger.successfulMessages(hashB));
-        assertTrue(L2Messenger.failedMessages(hashA));
-        assertTrue(L2Messenger.failedMessages(hashB));
-
-        // Set the configurable caller's target to `L2Messenger` and set the payload to `relayMessage(...)`.
-        caller.setDoRevert(false);
-        caller.setTarget(address(L2Messenger));
-        caller.setPayload(
-            abi.encodeWithSelector(
-                L2Messenger.relayMessage.selector,
-                Encoding.encodeVersionedNonce(0, 1),
-                sender,
-                target,
-                0,
-                0,
-                messageB
-            )
-        );
-
-        // Attempt to replay the failed message, which will *not* immediately revert this time around,
-        // but attempt to reenter `relayMessage` with messageB. The reentrancy attempt should succeed
-        // because the message hashes are different.
-        vm.expectEmit(true, true, true, true, target);
-        emit WhatHappened(true, hex"");
-        L2Messenger.relayMessage(
-            Encoding.encodeVersionedNonce(0, 1),
-            sender,
-            target,
-            0,
-            0,
-            messageA
-        );
-
-        // Assert that both messages are now in the `successfulMessages` mapping.
-        assertTrue(L2Messenger.successfulMessages(hashA));
-        assertTrue(L2Messenger.successfulMessages(hashB));
-    }
 }

packages/contracts-bedrock/contracts/test/OptimismPortal.t.sol

@@ -1019,7 +1019,7 @@ contract OptimismPortal_FinalizeWithdrawal_Test is Portal_Initializer {
         assertEq(outputRoot, Hashing.hashOutputRootProof(proof));
         assertEq(withdrawalHash, Hashing.hashWithdrawal(_tx));
 
-        // Mock the call to the oracle
+        // Setup the Oracle to return the outputRoot
         vm.mockCall(
             address(oracle),
             abi.encodeWithSelector(oracle.getL2Output.selector),
@@ -1039,8 +1039,6 @@ contract OptimismPortal_FinalizeWithdrawal_Test is Portal_Initializer {
         // Warp past the finalization period
         vm.warp(block.timestamp + oracle.FINALIZATION_PERIOD_SECONDS() + 1);
 
-        uint256 targetBalanceBefore = _target.balance;
-
         // Finalize the withdrawal transaction
         vm.expectCallMinGas(_tx.target, _tx.value, uint64(_tx.gasLimit), _tx.data);
         op.finalizeWithdrawalTransaction(_tx);

packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol

@@ -175,17 +175,12 @@ abstract contract CrossDomainMessenger is
      */
     mapping(bytes32 => bool) public failedMessages;
 
-    /**
-     * @notice A mapping of hashes to reentrancy locks.
-     */
-    mapping(bytes32 => bool) internal reentrancyLocks;
-
     /**
      * @notice Reserve extra slots in the storage layout for future upgrades.
      *         A gap size of 41 was chosen here, so that the first slot used in a child contract
      *         would be a multiple of 50.
      */
-    uint256[41] private __gap;
+    uint256[42] private __gap;
 
     /**
      * @notice Emitted whenever a message is sent to the other chain.
@@ -323,13 +318,6 @@ abstract contract CrossDomainMessenger is
             _message
         );
 
-        // Check if the reentrancy lock for the `versionedHash` is already set.
-        if (reentrancyLocks[versionedHash]) {
-            revert("ReentrancyGuard: reentrant call");
-        }
-        // Trigger the reentrancy lock for `versionedHash`
-        reentrancyLocks[versionedHash] = true;
-
         if (_isOtherMessenger()) {
             // These properties should always hold when the message is first submitted (as
             // opposed to being replayed).
@@ -357,6 +345,15 @@ abstract contract CrossDomainMessenger is
             "CrossDomainMessenger: message has already been relayed"
         );
 
+        // If `xDomainMsgSender` is not the default L2 sender, this function
+        // is being re-entered. This marks the message as failed to allow it
+        // to be replayed.
+        if (xDomainMsgSender != Constants.DEFAULT_L2_SENDER) {
+            failedMessages[versionedHash] = true;
+            emit FailedRelayedMessage(versionedHash);
+            return;
+        }
+
         xDomainMsgSender = _sender;
         bool success = SafeCall.callWithMinGas(_target, _minGasLimit, _value, _message);
         xDomainMsgSender = Constants.DEFAULT_L2_SENDER;
@@ -377,9 +374,6 @@ abstract contract CrossDomainMessenger is
                 revert("CrossDomainMessenger: failed to relay message");
             }
         }
-
-        // Clear the reentrancy lock for `versionedHash`
-        reentrancyLocks[versionedHash] = false;
     }
 
     /**