feat: warn when state sync settings are set while enable = false (#…

…1443) ## Description Closes celestiaorg/celestia-app#3736 This adds warn logs when state sync settings are set while `enable = false` as it may lead to unexpected behavior whereas Config is valid. #### PR checklist - [X] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: Rootul P <[email protected]>
celestiaorg · Aug 6, 2024 · b7f138d · b7f138d
1 parent 025a8bc
commit b7f138d
Show file tree

Hide file tree

Showing 10 changed files with 128 additions and 7 deletions.
diff --git a/cmd/cometbft/commands/root.go b/cmd/cometbft/commands/root.go
@@ -87,6 +87,10 @@ var RootCmd = &cobra.Command{
 		}
 
 		logger = logger.With("module", "main")
+
+		for _, possibleMisconfiguration := range config.PossibleMisconfigurations() {
+			logger.Warn(possibleMisconfiguration)
+		}
 		return nil
 	},
 }
diff --git a/config/config.go b/config/config.go
@@ -161,6 +161,16 @@ func (cfg *Config) ValidateBasic() error {
 	return nil
 }
 
+// PossibleMisconfigurations returns a list of possible conflicting entries that
+// may lead to unexpected behavior
+func (cfg *Config) PossibleMisconfigurations() []string {
+	res := []string{}
+	for _, elem := range cfg.StateSync.PossibleMisconfigurations() {
+		res = append(res, fmt.Sprintf("[statesync] section: %s", elem))
+	}
+	return res
+}
+
 //-----------------------------------------------------------------------------
 // BaseConfig
 
@@ -857,6 +867,15 @@ func TestStateSyncConfig() *StateSyncConfig {
 	return DefaultStateSyncConfig()
 }
 
+// PossibleMisconfigurations returns a list of possible conflicting entries that
+// may lead to unexpected behavior
+func (cfg *StateSyncConfig) PossibleMisconfigurations() []string {
+	if !cfg.Enable && len(cfg.RPCServers) != 0 {
+		return []string{"rpc_servers specified but enable = false"}
+	}
+	return []string{}
+}
+
 // ValidateBasic performs basic validation.
 func (cfg *StateSyncConfig) ValidateBasic() error {
 	if cfg.Enable {

diff --git a/config/config_test.go b/config/config_test.go
@@ -38,6 +38,17 @@ func TestConfigValidateBasic(t *testing.T) {
 	assert.Error(t, cfg.ValidateBasic())
 }
 
+func TestConfigPossibleMisconfigurations(t *testing.T) {
+	cfg := DefaultConfig()
+	require.Len(t, cfg.PossibleMisconfigurations(), 0)
+	// providing rpc_servers while enable = false is a possible misconfiguration
+	cfg.StateSync.RPCServers = []string{"first_rpc"}
+	require.Equal(t, []string{"[statesync] section: rpc_servers specified but enable = false"}, cfg.PossibleMisconfigurations())
+	// enabling statesync deletes possible misconfiguration
+	cfg.StateSync.Enable = true
+	require.Len(t, cfg.PossibleMisconfigurations(), 0)
+}
+
 func TestTLSConfiguration(t *testing.T) {
 	assert := assert.New(t)
 	cfg := DefaultConfig()
@@ -127,6 +138,17 @@ func TestStateSyncConfigValidateBasic(t *testing.T) {
 	require.NoError(t, cfg.ValidateBasic())
 }
 
+func TestStateSyncPossibleMisconfigurations(t *testing.T) {
+	cfg := DefaultStateSyncConfig()
+	require.Len(t, cfg.PossibleMisconfigurations(), 0)
+	// providing rpc_servers while enable = false is a possible misconfiguration
+	cfg.RPCServers = []string{"first_rpc"}
+	require.Equal(t, []string{"rpc_servers specified but enable = false"}, cfg.PossibleMisconfigurations())
+	// enabling statesync deletes possible misconfiguration
+	cfg.Enable = true
+	require.Len(t, cfg.PossibleMisconfigurations(), 0)
+}
+
 func TestFastSyncConfigValidateBasic(t *testing.T) {
 	cfg := TestFastSyncConfig()
 	assert.NoError(t, cfg.ValidateBasic())

diff --git a/libs/log/filter.go b/libs/log/filter.go
@@ -7,6 +7,7 @@ type level byte
 const (
 	levelDebug level = 1 << iota
 	levelInfo
+	levelWarn
 	levelError
 )
 
@@ -54,6 +55,14 @@ func (l *filter) Debug(msg string, keyvals ...interface{}) {
 	l.next.Debug(msg, keyvals...)
 }
 
+func (l *filter) Warn(msg string, keyvals ...interface{}) {
+	levelAllowed := l.allowed&levelWarn != 0
+	if !levelAllowed {
+		return
+	}
+	l.next.Warn(msg, keyvals...)
+}
+
 func (l *filter) Error(msg string, keyvals ...interface{}) {
 	levelAllowed := l.allowed&levelError != 0
 	if !levelAllowed {
@@ -137,12 +146,14 @@ func AllowLevel(lvl string) (Option, error) {
 		return AllowDebug(), nil
 	case "info":
 		return AllowInfo(), nil
+	case "warn":
+		return AllowWarn(), nil
 	case "error":
 		return AllowError(), nil
 	case "none":
 		return AllowNone(), nil
 	default:
-		return nil, fmt.Errorf("expected either \"info\", \"debug\", \"error\" or \"none\" level, given %s", lvl)
+		return nil, fmt.Errorf("expected either \"info\", \"debug\", \"warn\", \"error\" or \"none\" level, given %s", lvl)
 	}
 }
 
@@ -153,12 +164,17 @@ func AllowAll() Option {
 
 // AllowDebug allows error, info and debug level log events to pass.
 func AllowDebug() Option {
-	return allowed(levelError | levelInfo | levelDebug)
+	return allowed(levelError | levelWarn | levelInfo | levelDebug)
 }
 
 // AllowInfo allows error and info level log events to pass.
 func AllowInfo() Option {
-	return allowed(levelError | levelInfo)
+	return allowed(levelError | levelWarn | levelInfo)
+}
+
+// AllowWarn allows error and warn level log events to pass.
+func AllowWarn() Option {
+	return allowed(levelError | levelWarn)
 }
 
 // AllowError allows only error level log events to pass.
@@ -175,14 +191,21 @@ func allowed(allowed level) Option {
 	return func(l *filter) { l.allowed = allowed }
 }
 
-// AllowDebugWith allows error, info and debug level log events to pass for a specific key value pair.
+// AllowDebugWith allows error, warn, info and debug level log events to pass for a specific key value pair.
 func AllowDebugWith(key interface{}, value interface{}) Option {
-	return func(l *filter) { l.allowedKeyvals[keyval{key, value}] = levelError | levelInfo | levelDebug }
+	return func(l *filter) {
+		l.allowedKeyvals[keyval{key, value}] = levelError | levelWarn | levelInfo | levelDebug
+	}
 }
 
-// AllowInfoWith allows error and info level log events to pass for a specific key value pair.
+// AllowInfoWith allows error, warn, and info level log events to pass for a specific key value pair.
 func AllowInfoWith(key interface{}, value interface{}) Option {
-	return func(l *filter) { l.allowedKeyvals[keyval{key, value}] = levelError | levelInfo }
+	return func(l *filter) { l.allowedKeyvals[keyval{key, value}] = levelError | levelWarn | levelInfo }
+}
+
+// AllowWarnWith allows only error and warn log events to pass for a specific key value pair.
+func AllowWarnWith(key interface{}, value interface{}) Option {
+	return func(l *filter) { l.allowedKeyvals[keyval{key, value}] = levelError | levelWarn }
 }
 
 // AllowErrorWith allows only error level log events to pass for a specific key value pair.

diff --git a/libs/log/filter_test.go b/libs/log/filter_test.go
@@ -20,6 +20,7 @@ func TestVariousLevels(t *testing.T) {
 			strings.Join([]string{
 				`{"_msg":"here","level":"debug","this is":"debug log"}`,
 				`{"_msg":"here","level":"info","this is":"info log"}`,
+				`{"_msg":"here","level":"warn","this is":"warn log"}`,
 				`{"_msg":"here","level":"error","this is":"error log"}`,
 			}, "\n"),
 		},
@@ -29,6 +30,7 @@ func TestVariousLevels(t *testing.T) {
 			strings.Join([]string{
 				`{"_msg":"here","level":"debug","this is":"debug log"}`,
 				`{"_msg":"here","level":"info","this is":"info log"}`,
+				`{"_msg":"here","level":"warn","this is":"warn log"}`,
 				`{"_msg":"here","level":"error","this is":"error log"}`,
 			}, "\n"),
 		},
@@ -37,6 +39,15 @@ func TestVariousLevels(t *testing.T) {
 			log.AllowInfo(),
 			strings.Join([]string{
 				`{"_msg":"here","level":"info","this is":"info log"}`,
+				`{"_msg":"here","level":"warn","this is":"warn log"}`,
+				`{"_msg":"here","level":"error","this is":"error log"}`,
+			}, "\n"),
+		},
+		{
+			"AllowWarn",
+			log.AllowWarn(),
+			strings.Join([]string{
+				`{"_msg":"here","level":"warn","this is":"warn log"}`,
 				`{"_msg":"here","level":"error","this is":"error log"}`,
 			}, "\n"),
 		},
@@ -62,6 +73,7 @@ func TestVariousLevels(t *testing.T) {
 
 			logger.Debug("here", "this is", "debug log")
 			logger.Info("here", "this is", "info log")
+			logger.Warn("here", "this is", "warn log")
 			logger.Error("here", "this is", "error log")
 
 			if want, have := tc.want, strings.TrimSpace(buf.String()); want != have {

diff --git a/libs/log/logger.go b/libs/log/logger.go
@@ -10,6 +10,7 @@ import (
 type Logger interface {
 	Debug(msg string, keyvals ...interface{})
 	Info(msg string, keyvals ...interface{})
+	Warn(msg string, keyvals ...interface{})
 	Error(msg string, keyvals ...interface{})
 
 	With(keyvals ...interface{}) Logger

diff --git a/libs/log/nop_logger.go b/libs/log/nop_logger.go
@@ -10,6 +10,7 @@ func NewNopLogger() Logger { return &nopLogger{} }
 
 func (nopLogger) Info(string, ...interface{})  {}
 func (nopLogger) Debug(string, ...interface{}) {}
+func (nopLogger) Warn(string, ...interface{})  {}
 func (nopLogger) Error(string, ...interface{}) {}
 
 func (l *nopLogger) With(...interface{}) Logger {

diff --git a/libs/log/tm_logger.go b/libs/log/tm_logger.go
@@ -33,6 +33,8 @@ func NewTMLogger(w io.Writer) Logger {
 		switch keyvals[1].(kitlevel.Value).String() {
 		case "debug":
 			return term.FgBgColor{Fg: term.DarkGray}
+		case "warn":
+			return term.FgBgColor{Fg: term.Yellow}
 		case "error":
 			return term.FgBgColor{Fg: term.Red}
 		default:
@@ -79,6 +81,16 @@ func (l *tmLogger) Error(msg string, keyvals ...interface{}) {
 	}
 }
 
+// Warn logs a message at level Warn.
+func (l *tmLogger) Warn(msg string, keyvals ...interface{}) {
+	lWithLevel := kitlevel.Warn(l.srcLogger)
+
+	lWithMsg := kitlog.With(lWithLevel, msgKey, msg)
+	if err := lWithMsg.Log(keyvals...); err != nil {
+		lWithMsg.Log("err", err) //nolint:errcheck // no need to check error again
+	}
+}
+
 // With returns a new contextual logger with keyvals prepended to those passed
 // to calls to Info, Debug or Error.
 func (l *tmLogger) With(keyvals ...interface{}) Logger {

diff --git a/libs/log/tm_logger_test.go b/libs/log/tm_logger_test.go
@@ -66,6 +66,29 @@ func TestDebug(t *testing.T) {
 	}
 }
 
+func TestWarn(t *testing.T) {
+	var bufWarn bytes.Buffer
+
+	ld := log.NewTMLogger(&bufWarn)
+	ld.Warn("Client initialized with old header (trusted is more recent)",
+		"old", 42,
+		"trustedHeight", "forty two",
+		"trustedHash", []byte("test me"))
+
+	msg := strings.TrimSpace(bufWarn.String())
+
+	// Remove the timestamp information to allow
+	// us to test against the expected message.
+	receivedmsg := strings.Split(msg, "] ")[1]
+
+	const expectedmsg = `Client initialized with old header
+	(trusted is more recent) old=42 trustedHeight="forty two"
+	trustedHash=74657374206D65`
+	if strings.EqualFold(receivedmsg, expectedmsg) {
+		t.Fatalf("received %s, expected %s", receivedmsg, expectedmsg)
+	}
+}
+
 func TestError(t *testing.T) {
 	var bufErr bytes.Buffer
 

diff --git a/libs/log/tracing_logger.go b/libs/log/tracing_logger.go
@@ -36,6 +36,10 @@ func (l *tracingLogger) Debug(msg string, keyvals ...interface{}) {
 	l.next.Debug(msg, formatErrors(keyvals)...)
 }
 
+func (l *tracingLogger) Warn(msg string, keyvals ...interface{}) {
+	l.next.Warn(msg, formatErrors(keyvals)...)
+}
+
 func (l *tracingLogger) Error(msg string, keyvals ...interface{}) {
 	l.next.Error(msg, formatErrors(keyvals)...)
 }