Merge remote-tracking branch 'origin/jm/nonreentrant-with-replays' in…

…to clabby/ctb/call-with-min-gas-fix

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,19 +188,19 @@ 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))
 		}
 		return pre
 	}
 }
 
-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/cmd/main_test.go

@@ -79,6 +79,12 @@ func TestNetwork(t *testing.T) {
 	}
 }
 
+func TestDataDir(t *testing.T) {
+	expected := "/tmp/mainTestDataDir"
+	cfg := configForArgs(t, addRequiredArgs("--datadir", expected))
+	require.Equal(t, expected, cfg.DataDir)
+}
+
 func TestL2(t *testing.T) {
 	expected := "https://example.com:8545"
 	cfg := configForArgs(t, addRequiredArgs("--l2", expected))
@@ -170,14 +176,6 @@ func TestL1RPCKind(t *testing.T) {
 	})
 }
 
-// Offline support will be added later, but for now it just bails out with an error
-func TestOfflineModeNotSupported(t *testing.T) {
-	logger := log.New()
-	cfg := config.NewConfig(&chaincfg.Goerli, "genesis.json", common.HexToHash(l1HeadValue), common.HexToHash(l2HeadValue), common.HexToHash(l2ClaimValue))
-	err := FaultProofProgram(logger, cfg)
-	require.ErrorContains(t, err, "offline mode not supported")
-}
-
 func TestL2Claim(t *testing.T) {
 	t.Run("Required", func(t *testing.T) {
 		verifyArgsInvalid(t, "flag l2.claim is required", addRequiredArgsExcept("--l2.claim"))

op-program/host/config/config.go

@@ -18,10 +18,12 @@ 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
+	DataDir       string
 	L2URL         string
 	L2GenesisPath string
 	L1Head        common.Hash
@@ -54,6 +56,9 @@ func (c *Config) Check() error {
 	if (c.L1URL != "") != (c.L2URL != "") {
 		return ErrL1AndL2Inconsistent
 	}
+	if !c.FetchingEnabled() && c.DataDir == "" {
+		return ErrDataDirRequired
+	}
 	return nil
 }
 
@@ -95,6 +100,7 @@ func NewConfigFromCLI(ctx *cli.Context) (*Config, error) {
 	}
 	return &Config{
 		Rollup:        rollupCfg,
+		DataDir:       ctx.GlobalString(flags.DataDir.Name),
 		L2URL:         ctx.GlobalString(flags.L2NodeAddr.Name),
 		L2GenesisPath: ctx.GlobalString(flags.L2GenesisPath.Name),
 		L2Head:        l2Head,

op-program/host/config/config_test.go

@@ -15,7 +15,8 @@ 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)
 }
@@ -121,6 +122,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, validL2GenesisPath, 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,

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