Merge pull request #7566 from ethereum-optimism/generic-flag-fix

op-service: flag copy to protect against default-mutation of generic flag value

cannon/cmd/matcher.go

@@ -67,3 +67,11 @@ func (m *StepMatcherFlag) Matcher() StepMatcher {
 	}
 	return m.matcher
 }
+
+func (m *StepMatcherFlag) Clone() any {
+	var out StepMatcherFlag
+	if err := out.Set(m.repr); err != nil {
+		panic(fmt.Errorf("invalid repr: %w", err))
+	}
+	return &out
+}

op-batcher/cmd/main.go

@@ -9,6 +9,7 @@ import (
 	"github.com/ethereum-optimism/optimism/op-batcher/batcher"
 	"github.com/ethereum-optimism/optimism/op-batcher/cmd/doc"
 	"github.com/ethereum-optimism/optimism/op-batcher/flags"
+	"github.com/ethereum-optimism/optimism/op-service/cliapp"
 	oplog "github.com/ethereum-optimism/optimism/op-service/log"
 	"github.com/ethereum/go-ethereum/log"
 )
@@ -23,7 +24,7 @@ func main() {
 	oplog.SetupDefaults()
 
 	app := cli.NewApp()
-	app.Flags = flags.Flags
+	app.Flags = cliapp.ProtectFlags(flags.Flags)
 	app.Version = fmt.Sprintf("%s-%s-%s", Version, GitCommit, GitDate)
 	app.Name = "op-batcher"
 	app.Usage = "Batch Submitter Service"

op-challenger/cmd/main.go

@@ -11,6 +11,7 @@ import (
 	"github.com/ethereum-optimism/optimism/op-challenger/config"
 	"github.com/ethereum-optimism/optimism/op-challenger/flags"
 	"github.com/ethereum-optimism/optimism/op-challenger/version"
+	"github.com/ethereum-optimism/optimism/op-service/cliapp"
 	oplog "github.com/ethereum-optimism/optimism/op-service/log"
 )
 
@@ -48,7 +49,7 @@ func run(args []string, action ConfigAction) error {
 
 	app := cli.NewApp()
 	app.Version = VersionWithMeta
-	app.Flags = flags.Flags
+	app.Flags = cliapp.ProtectFlags(flags.Flags)
 	app.Name = "op-challenger"
 	app.Usage = "Challenge outputs"
 	app.Description = "Ensures that on chain outputs are correct."

op-challenger/config/config.go

@@ -70,6 +70,11 @@ func (t *TraceType) Set(value string) error {
 	return nil
 }
 
+func (t *TraceType) Clone() any {
+	cpy := *t
+	return &cpy
+}
+
 func ValidTraceType(value TraceType) bool {
 	for _, t := range TraceTypes {
 		if t == value {

op-node/cmd/main.go

@@ -50,7 +50,7 @@ func main() {
 
 	app := cli.NewApp()
 	app.Version = VersionWithMeta
-	app.Flags = flags.Flags
+	app.Flags = cliapp.ProtectFlags(flags.Flags)
 	app.Name = "op-node"
 	app.Usage = "Optimism Rollup Node"
 	app.Description = "The Optimism Rollup Node derives L2 block inputs from L1 data and drives an external L2 Execution Engine to build a L2 chain."

op-proposer/cmd/main.go

@@ -9,6 +9,7 @@ import (
 	"github.com/ethereum-optimism/optimism/op-proposer/cmd/doc"
 	"github.com/ethereum-optimism/optimism/op-proposer/flags"
 	"github.com/ethereum-optimism/optimism/op-proposer/proposer"
+	"github.com/ethereum-optimism/optimism/op-service/cliapp"
 	oplog "github.com/ethereum-optimism/optimism/op-service/log"
 	"github.com/ethereum/go-ethereum/log"
 )
@@ -23,7 +24,7 @@ func main() {
 	oplog.SetupDefaults()
 
 	app := cli.NewApp()
-	app.Flags = flags.Flags
+	app.Flags = cliapp.ProtectFlags(flags.Flags)
 	app.Version = fmt.Sprintf("%s-%s-%s", Version, GitCommit, GitDate)
 	app.Name = "op-proposer"
 	app.Usage = "L2Output Submitter"

op-service/cliapp/flag.go

+package cliapp
+
+import (
+	"fmt"
+
+	"github.com/urfave/cli/v2"
+)
+
+type CloneableGeneric interface {
+	cli.Generic
+	Clone() any
+}
+
+// ProtectFlags ensures that no flags are safe to Apply() flag sets to without accidental flag-value mutations.
+// ProtectFlags panics if any of the flag definitions cannot be protected.
+func ProtectFlags(flags []cli.Flag) []cli.Flag {
+	out := make([]cli.Flag, 0, len(flags))
+	for _, f := range flags {
+		fCopy, err := cloneFlag(f)
+		if err != nil {
+			panic(fmt.Errorf("failed to clone flag %q: %w", f.Names()[0], err))
+		}
+		out = append(out, fCopy)
+	}
+	return out
+}
+
+func cloneFlag(f cli.Flag) (cli.Flag, error) {
+	switch typedFlag := f.(type) {
+	case *cli.GenericFlag:
+		// We have to clone Generic, since it's an interface,
+		// and setting it causes the next use of the flag to have a different default value.
+		if genValue, ok := typedFlag.Value.(CloneableGeneric); ok {
+			cpy := *typedFlag
+			cpyVal, ok := genValue.Clone().(cli.Generic)
+			if !ok {
+				return nil, fmt.Errorf("cloned Generic value is not Generic: %T", typedFlag)
+			}
+			cpy.Value = cpyVal
+			return &cpy, nil
+		} else {
+			return nil, fmt.Errorf("cannot clone Generic value: %T", typedFlag)
+		}
+	default:
+		// Other flag types are safe to re-use, although not strictly safe for concurrent use.
+		// urfave v3 hopefully fixes this.
+		return f, nil
+	}
+}

op-service/cliapp/flag_test.go

+package cliapp
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"github.com/urfave/cli/v2"
+)
+
+type testGeneric string
+
+func (t *testGeneric) Clone() any {
+	cpy := *t
+	return &cpy
+}
+
+func (t *testGeneric) Set(value string) error {
+	*t = testGeneric(value)
+	return nil
+}
+
+func (t *testGeneric) String() string {
+	if t == nil {
+		return "nil"
+	}
+	return string(*t)
+}
+
+var _ CloneableGeneric = (*testGeneric)(nil)
+
+func TestProtectFlags(t *testing.T) {
+	foo := &cli.StringFlag{
+		Name:     "foo",
+		Value:    "123",
+		Required: true,
+	}
+	barValue := testGeneric("original")
+	bar := &cli.GenericFlag{
+		Name:     "bar",
+		Value:    &barValue,
+		Required: true,
+	}
+	var originalFlags = []cli.Flag{
+		foo,
+		bar,
+	}
+	// if we ran with the original flags instead, we see mutation issues:
+	//outFlags := originalFlags
+	outFlags := ProtectFlags(originalFlags)
+	app := &cli.App{
+		Name:  "test",
+		Flags: outFlags,
+		Action: func(ctx *cli.Context) error {
+			require.Equal(t, "a", ctx.String(foo.Name))
+			require.Equal(t, "changed", ctx.Generic(bar.Name).(*testGeneric).String())
+			return nil
+		},
+	}
+	require.NoError(t, app.Run([]string{"test", "--foo=a", "--bar=changed"}))
+	// check that the original flag definitions are still untouched
+	require.Equal(t, "123", foo.Value)
+	require.Equal(t, "original", bar.Value.String())
+}

op-service/log/cli.go

@@ -12,6 +12,7 @@ import (
 	"github.com/ethereum/go-ethereum/log"
 
 	opservice "github.com/ethereum-optimism/optimism/op-service"
+	"github.com/ethereum-optimism/optimism/op-service/cliapp"
 )
 
 const (
@@ -20,6 +21,9 @@ const (
 	ColorFlagName  = "log.color"
 )
 
+// CLIFlags creates flag definitions for the logging utils.
+// Warning: flags are not safe to reuse due to an upstream urfave default-value mutation bug in GenericFlag.
+// Use cliapp.ProtectFlags(flags) to create a copy before passing it into an App if the app runs more than once.
 func CLIFlags(envPrefix string) []cli.Flag {
 	return []cli.Flag{
 		&cli.GenericFlag{
@@ -68,7 +72,12 @@ func (fv LvlFlagValue) LogLvl() log.Lvl {
 	return log.Lvl(fv)
 }
 
-var _ cli.Generic = (*LvlFlagValue)(nil)
+func (fv *LvlFlagValue) Clone() any {
+	cpy := *fv
+	return &cpy
+}
+
+var _ cliapp.CloneableGeneric = (*LvlFlagValue)(nil)
 
 // FormatType defines a type of log format.
 // Supported formats: 'text', 'terminal', 'logfmt', 'json', 'json-pretty'
@@ -133,6 +142,13 @@ func (fv FormatFlagValue) FormatType() FormatType {
 	return FormatType(fv)
 }
 
+func (fv *FormatFlagValue) Clone() any {
+	cpy := *fv
+	return &cpy
+}
+
+var _ cliapp.CloneableGeneric = (*FormatFlagValue)(nil)
+
 type CLIConfig struct {
 	Level  log.Lvl
 	Color  bool

op-service/sources/receipts.go

@@ -149,6 +149,11 @@ func (kind *RPCProviderKind) Set(value string) error {
 	return nil
 }
 
+func (kind *RPCProviderKind) Clone() any {
+	cpy := *kind
+	return &cpy
+}
+
 func ValidRPCProviderKind(value RPCProviderKind) bool {
 	for _, k := range RPCProviderKinds {
 		if k == value {

op-wheel/commands.go

@@ -170,6 +170,14 @@ func (a *TextFlag[T]) Get() T {
 	return a.Value
 }
 
+func (a *TextFlag[T]) Clone() any {
+	var out TextFlag[T]
+	if err := out.Set(a.String()); err != nil {
+		panic(fmt.Errorf("cannot clone invalid text value: %w", err))
+	}
+	return &out
+}
+
 var _ cli.Generic = (*TextFlag[*common.Address])(nil)
 
 func textFlag[T Text](name string, usage string, value T) *cli.GenericFlag {