From b7f138dfa0e4eb560d77e927b495072eeb16bf00 Mon Sep 17 00:00:00 2001 From: nathan haim Date: Tue, 6 Aug 2024 21:07:47 +0200 Subject: [PATCH] feat: warn when state sync settings are set while `enable = false` (#1443) ## Description Closes https://github.com/celestiaorg/celestia-app/issues/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 --- cmd/cometbft/commands/root.go | 4 ++++ config/config.go | 19 ++++++++++++++++++ config/config_test.go | 22 +++++++++++++++++++++ libs/log/filter.go | 37 ++++++++++++++++++++++++++++------- libs/log/filter_test.go | 12 ++++++++++++ libs/log/logger.go | 1 + libs/log/nop_logger.go | 1 + libs/log/tm_logger.go | 12 ++++++++++++ libs/log/tm_logger_test.go | 23 ++++++++++++++++++++++ libs/log/tracing_logger.go | 4 ++++ 10 files changed, 128 insertions(+), 7 deletions(-) diff --git a/cmd/cometbft/commands/root.go b/cmd/cometbft/commands/root.go index 07f8c6d5ed..7a592d0b3d 100644 --- 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 index 5289f5fe1d..fde13fbccf 100644 --- 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 index 43f0a46c49..cb58f14d02 100644 --- 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 index 4b7ed981cd..6e4a09775d 100644 --- 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 index f98fd6e71e..d89b742c81 100644 --- 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 index 22ed68f1a1..ef00acbd22 100644 --- 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 index 12d75abe6b..4f0d4e7fca 100644 --- 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 index ac0d08adb0..b8034d5114 100644 --- 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 index 95b4fd5379..4856f25e2d 100644 --- 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 index d2a6ff44e5..164bf59f5b 100644 --- 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)...) }