diff --git a/cmd/seid/cmd/configmanager_differential_test.go b/cmd/seid/cmd/configmanager_differential_test.go index 85cfb38b52..159b1ed104 100644 --- a/cmd/seid/cmd/configmanager_differential_test.go +++ b/cmd/seid/cmd/configmanager_differential_test.go @@ -14,6 +14,7 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/sdk/trace" @@ -134,6 +135,89 @@ func runManager(t *testing.T, mgr configmanager.ConfigManager, cmd *cobra.Comman return serverCtx, applyErr } +// requireSameSettings asserts legacy and v2 resolved the same flat settings map, having +// first asserted that both contexts have a viper at all. +// +// The guard is why this is a helper rather than a bare require.Equal. Apply is what +// populates serverCtx.Viper, so a context it never reached carries nil, and comparing +// two of those is this file's central premise reporting a pass on a boot that did not +// happen. Settings panics on a nil viper, so the guard is the redundant half of the +// pair — it is the half that names the broken premise instead of surfacing a nil +// dereference from inside viper. Every settings comparison in this file goes through +// here, so the next one cannot be written without it. +// +// Compared as typed maps, reported as sorted dumps. The comparison has to be the typed +// one, because the whole point of Settings is that int64(8) and "8" differ. The report +// goes through DumpViper because that is the notation the rest of this harness fails in: +// a differing key reads as `evm.max_log_bytes = int64(67108864)` rather than as spew's +// `(string) (len=17) "evm.max_log_bytes": (int64) 67108864`. Both forms diff per key, +// since testify sorts a map before diffing it, so this buys legibility rather than a +// shorter failure. Both dumps are built only after a difference is known, since this +// runs per fuzz execution. +func requireSameSettings(t *testing.T, legacy, v2 *server.Context, msgAndArgs ...any) { + t.Helper() + where := callerContext(msgAndArgs) + require.NotNil(t, legacy.Viper, "legacy Apply left serverCtx.Viper nil: there is no resolved config to compare"+where) + require.NotNil(t, v2.Viper, "v2 Apply left serverCtx.Viper nil: there is no resolved config to compare"+where) + + legacySettings, v2Settings := configtest.Settings(legacy.Viper), configtest.Settings(v2.Viper) + if assert.ObjectsAreEqual(legacySettings, v2Settings) { + return + } + // The dump diff is the readable report. It is a rendering, so two different key sets + // can render alike (a key holding a newline), and the typed assertion below is the + // authority either way: whichever of the two fires, the difference is reported. + require.Equal(t, configtest.DumpViper(legacy.Viper), configtest.DumpViper(v2.Viper), msgAndArgs...) + require.Equal(t, legacySettings, v2Settings, msgAndArgs...) +} + +// requireSameChannels asserts legacy and v2 produced the same two boot channels — the +// Tendermint config struct and the resolved settings — having first asserted that both +// exist. +// +// The Config half needs the same guard the settings half does, and for longer than it +// had one. Apply populates both fields, so a context it never reached carries nil in +// both, and require.Equal on two nil *tmcfg.Config pointers passes: the same vacuous +// pass the settings comparison used to have, on the channel next to it. It went unnoticed +// because AllSettings used to panic on the nil viper two lines later, so the boot failure +// surfaced anyway — as a nil dereference from inside viper rather than as a named +// premise. Guarding settings alone would have left this one masked only by the accident +// that the two are always compared together. +// +// Both channels in one helper because they come from one Apply: a test that has grounds +// to compare either has grounds to compare both, and splitting them is what let the hole +// sit next to its own fix. +func requireSameChannels(t *testing.T, legacy, v2 *server.Context, msgAndArgs ...any) { + t.Helper() + where := callerContext(msgAndArgs) + require.NotNil(t, legacy.Config, "legacy Apply left serverCtx.Config nil: there is no config to compare"+where) + require.NotNil(t, v2.Config, "v2 Apply left serverCtx.Config nil: there is no config to compare"+where) + require.Equal(t, legacy.Config, v2.Config, + "serverCtx.Config differs between legacy and v2"+where) + requireSameSettings(t, legacy, v2, msgAndArgs...) +} + +// callerContext renders a testify msgAndArgs tail as a parenthetical, so a helper can +// prepend its own explanation without discarding the caller's. +// +// The guards inside these helpers name which premise broke; the caller's message names +// which input broke it. A corpus row or a fuzz execution needs both to be reproducible, +// and passing msgAndArgs straight through would make the caller's format string consume +// the helper's text as an argument. +func callerContext(msgAndArgs []any) string { + switch len(msgAndArgs) { + case 0: + return "" + case 1: + return fmt.Sprintf(" (%v)", msgAndArgs[0]) + default: + if format, ok := msgAndArgs[0].(string); ok { + return " (" + fmt.Sprintf(format, msgAndArgs[1:]...) + ")" + } + return " (" + fmt.Sprint(msgAndArgs...) + ")" + } +} + // seedDefaultConfig returns a home carrying a complete, realistic config (all Sei // sections), generated by letting the legacy creator write into a fresh home. func seedDefaultConfig(t *testing.T) *configtest.Home { @@ -220,7 +304,7 @@ func configCorpus() []corpusCase { // // It compares parsed semantics: // - serverCtx.Config (the *tmcfg.Config the node runs on), and -// - serverCtx.Viper.AllSettings() (the AppOptions every Sei section reads via +// - configtest.Settings(serverCtx.Viper) (the AppOptions every Sei section reads via // appOpts.Get), both at end-of-PersistentPreRunE and after the start.go // chain-id mutation. func TestConfigManagerLegacyVsV2Differential(t *testing.T) { @@ -231,17 +315,14 @@ func TestConfigManagerLegacyVsV2Differential(t *testing.T) { legacyCtx := runConfigManager(t, configmanager.LegacyConfigManager{}, home) v2Ctx := runConfigManager(t, configmanager.SeiConfigManager{}, home) - require.Equal(t, legacyCtx.Config, v2Ctx.Config, - "serverCtx.Config differs between legacy and v2") - require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), - "serverCtx.Viper settings differ between legacy and v2") + requireSameChannels(t, legacyCtx, v2Ctx) // The start.go chain-id mutation is identical on both vipers; assert parity // holds after it too (covers the post-mutation snapshot). const chainID = "differential-test-1" legacyCtx.Viper.Set(flags.FlagChainID, chainID) v2Ctx.Viper.Set(flags.FlagChainID, chainID) - require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), + requireSameSettings(t, legacyCtx, v2Ctx, "settings diverge after the start.go chain-id mutation") } @@ -276,10 +357,7 @@ func TestConfigManagerLegacyVsV2Differential_EnvHome(t *testing.T) { require.Equal(t, home.Root, legacyCtx.Viper.GetString(flags.FlagHome), "env-provided home did not drive legacy resolution") - require.Equal(t, legacyCtx.Config, v2Ctx.Config, - "serverCtx.Config differs between legacy and v2 on the env-home path") - require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), - "serverCtx.Viper settings differ between legacy and v2 on the env-home path") + requireSameChannels(t, legacyCtx, v2Ctx, "env-home path") } // TestConfigManagerLegacyVsV2Differential_Corpus widens the parity proof from the @@ -298,10 +376,7 @@ func TestConfigManagerLegacyVsV2Differential_Corpus(t *testing.T) { legacyCtx := runConfigManager(t, configmanager.LegacyConfigManager{}, home) v2Ctx := runConfigManager(t, configmanager.SeiConfigManager{}, home) - require.Equal(t, legacyCtx.Config, v2Ctx.Config, - "serverCtx.Config differs between legacy and v2 (%s)", tc.name) - require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), - "serverCtx.Viper settings differ between legacy and v2 (%s)", tc.name) + requireSameChannels(t, legacyCtx, v2Ctx, "case %q", tc.name) }) } } @@ -318,8 +393,7 @@ func TestConfigManagerV2AdvisoryNeverRefusesBoot(t *testing.T) { require.NoError(t, v2Err, "advisory validation must never refuse boot on a valid config") legacyCtx := runConfigManager(t, configmanager.LegacyConfigManager{}, home) - require.Equal(t, legacyCtx.Config, v2Ctx.Config) - require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings()) + requireSameChannels(t, legacyCtx, v2Ctx) } // TestConfigManagerV2FreshHomeBoots exercises the fresh-home first-boot path: v2's @@ -419,13 +493,13 @@ func TestConfigManagerV2WritesNothing(t *testing.T) { "leaves the resolved channels alone is invisible to every other assertion here") } -// FuzzConfigManagerEnvOnlyKeyParity closes the one class the AllSettings comparison +// FuzzConfigManagerEnvOnlyKeyParity closes the one class the settings comparison // cannot reach. // -// AllSettings enumerates only what viper knows structurally, from the files it read, -// its defaults, overrides and bound flags. A value carried solely by the environment -// for a key absent from app.toml has no enumerable existence, so it appears in neither -// AllSettings nor AllKeys, and every comparison above is blind to it. It is not +// AllKeys enumerates only what viper knows structurally, from the files it read, its +// defaults, overrides and bound flags. A value carried solely by the environment for a +// key absent from app.toml has no enumerable existence, so it appears in neither +// AllKeys nor anything built on it, and every comparison above is blind to it. It is not // invisible to the node: app.New reads through appOpts.Get, and AutomaticEnv resolves // at Get time, so such a value does reach running code. // @@ -513,7 +587,7 @@ func FuzzConfigManagerEnvOnlyKeyParity(f *testing.F) { require.Equal(t, legacyGot, v2Got, "env-only key %q resolves differently between legacy and v2 (env %s=%q). This is "+ - "invisible to the AllSettings comparison, and app.New reads it through "+ + "invisible to the settings comparison, and app.New reads it through "+ "appOpts.Get, so it reaches the running node", key, envKey, value) }) } @@ -558,7 +632,6 @@ func FuzzConfigManagerLegacyVsV2Parity(f *testing.F) { require.Equal(t, legacyErr.Error(), v2Err.Error(), "divergent error (case %q, suffix %q)", tc.name, appTOMLSuffix) return } - require.Equal(t, legacyCtx.Config, v2Ctx.Config, "Config diverges (case %q, suffix %q)", tc.name, appTOMLSuffix) - require.Equal(t, legacyCtx.Viper.AllSettings(), v2Ctx.Viper.AllSettings(), "settings diverge (case %q, suffix %q)", tc.name, appTOMLSuffix) + requireSameChannels(t, legacyCtx, v2Ctx, "case %q, app.toml suffix %q", tc.name, appTOMLSuffix) }) } diff --git a/cmd/seid/cmd/envshadow_config_test.go b/cmd/seid/cmd/envshadow_config_test.go new file mode 100644 index 0000000000..03d28ca639 --- /dev/null +++ b/cmd/seid/cmd/envshadow_config_test.go @@ -0,0 +1,241 @@ +package cmd + +import ( + "testing" + + gigaconfig "github.com/sei-protocol/sei-chain/giga/executor/config" + "github.com/sei-protocol/sei-chain/testutil/configtest" + "github.com/stretchr/testify/require" +) + +// Environment-variable shadowing of a whole config section. +// +// A non-empty SEID_
variable makes every key under that section resolve to +// nothing, so the operator's written value in app.toml is discarded and the reader +// falls back to its in-code default. The variable's own value is never used: setting +// the variable to "false" produces the same result as setting it to "true" or to any +// other string. So this is not the environment overriding the file, which is the +// documented and intended precedence — it is the file's value being dropped and a +// third value, belonging to no layer, taking effect. +// +// The mechanism is viper's isPathShadowedInAutoEnv, which walks every proper prefix of +// a dotted key and, if any prefixed variable is set, returns before the config file is +// consulted. It cannot distinguish "SEID_GIGA_EXECUTOR names a scalar, so +// giga_executor.enabled cannot exist" from "SEID_GIGA_EXECUTOR is unrelated to the key +// I was asked for". +// +// These rows pin the behavior; they do not endorse it. The legacy path has shipped, and +// changing how configuration resolves could silently break an operator who has come to +// depend on the current answer, so the behavior is recorded here and left alone. +// SeiConfigManager corrects it when it owns resolution, and the divergence is ratified +// against these rows rather than discovered against a production node. That correction is +// tracked on PLT-775, so the fix has an anchor that outlives this comment. +// +// The consequence is worth stating where the pin lives, because it is what makes the +// pin worth having rather than trivia. giga_executor.enabled defaults to true +// (giga/executor/config/config.go), its reader is presence-guarded so a nil read keeps +// that default, and app.New feeds it to tmtypes.SkipLastResultsHashValidation.Store — +// which gates whether the node compares block.LastResultsHash against +// state.LastResultsHash (sei-tendermint/internal/state/validation.go). An operator who +// writes `enabled = false` and has any non-empty SEID_GIGA_EXECUTOR in the process +// environment therefore gets giga enabled and that consensus comparison relaxed, with +// nothing in any log saying so. +// +// Two of the three links in that chain are asserted, and the third is described. This +// file asserts the shadow itself and, through gigaconfig.ReadConfig on the resolved +// viper, that the nil read leaves enabled at true. app's +// TestGigaExecutorEnabledDrivesLastResultsHashValidation asserts that enabled reaches the +// atomic, through a real app construction. Only the join between them is prose here, +// because observing it needs a running node, which this suite deliberately does not do +// (testutil/configtest/AGENTS.md, "Out of Scope"). + +// sectionEnvVar returns the environment variable that shadows the given dotted path, +// derived the way the server viper derives it rather than built by hand. +// +// Built by hand it is easy to get wrong in a way that reads as "the defect does not +// exist". The replacer runs over the whole prefixed name, prefix included, and the prefix +// is the running binary's basename — "cmd.test" under go test. Folding only the key and +// joining it to the prefix therefore leaves that dot in place, yielding +// CMD.TEST_GIGA_EXECUTOR, which no environment lookup matches and which shadows nothing. +func sectionEnvVar(t *testing.T, path string) string { + t.Helper() + prefix, err := configtest.ServerEnvPrefix() + if err != nil { + t.Fatalf("resolve server env prefix: %v", err) + } + return configtest.ServerEnvKey(prefix, path) +} + +// homeWithAppTOML returns a fresh fixture home whose app.toml holds the given body. +// Booting it is applyLegacy's job. +func homeWithAppTOML(t *testing.T, body string) *configtest.Home { + t.Helper() + home := configtest.NewHome(t) + home.WriteAppTOML(t, []byte(body)) + return home +} + +// TestSectionEnvVarShadowsItsWholeSection records the shadow itself: the section's keys +// stop resolving, a sibling section is untouched, and enumeration keeps listing the +// shadowed keys. +// +// Every row boots its own fixture home, the baseline included: the legacy path treats a +// node directory as read-write while it reads it and creates config.toml on first boot, +// so one home shared across the rows would vary a second input alongside the variable +// under test (testutil/configtest/AGENTS.md). +func TestSectionEnvVarShadowsItsWholeSection(t *testing.T) { + configtest.Isolate(t) + + const body = `[giga_executor] +enabled = false +occ_enabled = false + +[evm] +max_log_bytes = 100 +` + shadowVar := sectionEnvVar(t, "giga_executor") + + // Baseline: no shadowing variable, so the operator's written values win. + base := applyLegacy(t, homeWithAppTOML(t, body), nil) + require.NoError(t, base.err) + require.Equal(t, false, base.ctx.Viper.Get("giga_executor.enabled"), + "without a shadowing variable the operator's written value must win") + + for _, value := range []string{"true", "false", "1", "0", "anything-at-all"} { + t.Run("value="+value, func(t *testing.T) { + t.Setenv(shadowVar, value) + got := applyLegacy(t, homeWithAppTOML(t, body), nil) + require.NoError(t, got.err, "shadowing must not refuse the boot") + v := got.ctx.Viper + + // Every key under the shadowed section resolves to nothing, whatever the + // variable's value. That is what distinguishes this from an override. + require.Nil(t, v.Get("giga_executor.enabled"), + "%s=%q must make giga_executor.enabled resolve to nothing", shadowVar, value) + require.Nil(t, v.Get("giga_executor.occ_enabled")) + require.False(t, v.IsSet("giga_executor.enabled")) + + // What the nil read costs, asserted through the section's real reader + // rather than inferred from it. ReadConfig is presence-guarded, so a key + // that resolves to nothing leaves the in-code default standing, and + // enabled comes back true from a file that says false. This is the step + // the consensus consequence in the header rests on. + cfg, err := gigaconfig.ReadConfig(v) + require.NoError(t, err, "a shadowed section must not make the reader fail") + require.True(t, cfg.Enabled, + "%s=%q must leave giga_executor.enabled at its in-code default of true, "+ + "against an app.toml that sets it to false", shadowVar, value) + require.Equal(t, gigaconfig.DefaultConfig, cfg, + "a shadowed section must resolve to exactly the in-code defaults, so no "+ + "value the operator wrote survives anywhere in it") + + // A sibling section is untouched: the shadow is scoped to the prefix the + // variable names, not to the whole file. + require.Equal(t, int64(100), v.Get("evm.max_log_bytes"), + "the shadow must not reach a section the variable does not name") + + // Enumeration still reports the shadowed keys as present, which is why a + // reader cannot infer "in effect" from "enumerated". + require.Contains(t, v.AllKeys(), "giga_executor.enabled", + "AllKeys must still list a shadowed key") + + // The same pair, through configtest.Settings, because this is the only place + // it can be asserted. Settings' coverage argument is that AllSettings omits a + // key whose Get returns nil while Settings records it as an explicit nil + // entry — and a key that enumerates but resolves to nothing is what the + // environment shadow produces and nothing else does, so the vipers in + // configtest's own tests cannot construct one. Contains before the nil check: + // indexing an absent key also yields nil, so the two together are what + // distinguish "recorded as nil" from "not recorded". + settings := configtest.Settings(v) + require.Contains(t, settings, "giga_executor.enabled", + "Settings must record a shadowed key, which is the entry AllSettings drops") + require.Nil(t, settings["giga_executor.enabled"], + "Settings must record a shadowed key as an explicit nil entry") + }) + } +} + +// TestEmptySectionEnvVarDoesNotShadow pins the boundary: an empty value is not a +// shadow. viper's env lookup treats an empty value as absent unless AllowEmptyEnv is +// set, and seid does not set it. Recorded because the empty and non-empty cases behave +// differently, and only the pair distinguishes the real rule from "the variable +// exists". +func TestEmptySectionEnvVarDoesNotShadow(t *testing.T) { + configtest.Isolate(t) + + home := homeWithAppTOML(t, "[giga_executor]\nenabled = false\n") + t.Setenv(sectionEnvVar(t, "giga_executor"), "") + + got := applyLegacy(t, home, nil) + require.NoError(t, got.err) + require.Equal(t, false, got.ctx.Viper.Get("giga_executor.enabled"), + "an empty shadowing variable must not shadow; the operator's value must still win") + require.True(t, got.ctx.Viper.IsSet("giga_executor.enabled")) +} + +// TestFullKeyEnvVarDeliversRatherThanShadows pins the other boundary, and it is the row +// that shows the shadow is not precedence. A variable naming the key's FULL path +// delivers its value, which is the intended env-over-file precedence. A variable naming +// a proper prefix of that path delivers nothing and discards the file's value. Same +// key, same environment mechanism, opposite outcomes. +func TestFullKeyEnvVarDeliversRatherThanShadows(t *testing.T) { + configtest.Isolate(t) + + home := homeWithAppTOML(t, "[giga_executor]\nocc_enabled = false\n") + + t.Setenv(sectionEnvVar(t, "giga_executor.occ_enabled"), "true") + delivered := applyLegacy(t, home, nil) + require.NoError(t, delivered.err) + require.Equal(t, "true", delivered.ctx.Viper.Get("giga_executor.occ_enabled"), + "a variable naming the full key path must deliver its value (env over file), and it "+ + "arrives as the untyped string it was written as — the same key read from the file "+ + "layer resolves to a bool, so which layer a value came from changes its Go type") +} + +// TestShadowFoldsSectionPunctuation records that the shadow does not care how the +// section is punctuated, because the replacer folds "." and "-" to "_" before the +// lookup. A dashed section is shadowed by the same variable-name shape as an +// underscored one — which is also why two differently-punctuated names cannot be +// distinguished by environment delivery. +// +// It also records the sharper half of the consequence, because this key's reader is +// shaped differently from giga_executor's. GetConfig reads it unguarded — +// v.GetBool("state-commit.sc-enable") at sei-cosmos/server/config/config.go:621, with no +// presence check — so a nil read is not a fallback to an in-code default, it is +// GetBool(nil), which is false. The two sections therefore fail in opposite directions +// from one mechanism: a shadowed giga_executor.enabled keeps its true default and +// silently enables what the operator disabled, while a shadowed state-commit.sc-enable +// resolves false and silently disables what the operator enabled. Which direction a +// section takes depends only on whether its reader guards the read. +// +// The baseline is what makes the nil assertion mean anything. A nil is evidence of +// shadowing only if the key is known to resolve without the variable set — otherwise a +// fixture typo or a section rename would satisfy the assertion while the dash-folding +// property it exists to pin went untested. +func TestShadowFoldsSectionPunctuation(t *testing.T) { + configtest.Isolate(t) + + const body = "[state-commit]\nsc-enable = true\n" + shadowVar := sectionEnvVar(t, "state-commit") + require.Contains(t, shadowVar, "STATE_COMMIT", + "the replacer must fold the dash to an underscore") + + // Baseline, on its own fixture home: the dashed key resolves, and the reader agrees. + base := applyLegacy(t, homeWithAppTOML(t, body), nil) + require.NoError(t, base.err) + require.Equal(t, true, base.ctx.Viper.Get("state-commit.sc-enable"), + "without a shadowing variable the dashed key must resolve to the written value") + require.True(t, base.ctx.Viper.GetBool("state-commit.sc-enable"), + "the reader must see the operator's value when nothing shadows it") + + t.Setenv(shadowVar, "x") + got := applyLegacy(t, homeWithAppTOML(t, body), nil) + require.NoError(t, got.err) + require.Nil(t, got.ctx.Viper.Get("state-commit.sc-enable"), + "%s must shadow a dashed section the same way it shadows an underscored one", shadowVar) + require.False(t, got.ctx.Viper.GetBool("state-commit.sc-enable"), + "%s must make the unguarded reader resolve false, against an app.toml that sets it "+ + "true — the opposite direction from giga_executor, whose guarded reader keeps its "+ + "true default instead", shadowVar) +} diff --git a/testutil/configtest/AGENTS.md b/testutil/configtest/AGENTS.md index eae9208a03..e5a5a6dc2b 100644 --- a/testutil/configtest/AGENTS.md +++ b/testutil/configtest/AGENTS.md @@ -134,6 +134,26 @@ go test .// -run FuzzXxx -fuzz FuzzXxx -fuzztime 60s # extend a target by Seeds cover every row on an ordinary run. Running the fuzzer by hand explores past the seeds and is worth doing when changing a reader's cast or its guard. +## Comparing two resolved vipers + +Two managers, two boots, or a before/after both reduce to "did these resolve the same +values". Compare `Settings`, and report with `DumpViper`. + +- `configtest.Settings` is the value you assert on: a flat map from dotted key to what + `Get` returns, one entry per `AllKeys` entry. Use it, not `Viper.AllSettings`. + `AllSettings` re-nests the flat key space by splitting on `.`, so when one key is a + dotted prefix of another it drops one of them depending on map iteration order, and + an equality assertion between two of them can fail on identical input. It also omits + every key whose `Get` returns nil, so a key one side enumerates and resolves to + nothing looks the same as a key the other side never enumerated at all. +- `configtest.DumpViper` is for the failure message, not the assertion. It renders one + sorted, type-qualified line per key, which is what a human reads — but it joins with + newlines and does not quote keys, and a TOML key may legally contain a newline, so two + different key sets can render identically. Assert on the map; print the dump. +- `Settings` panics on a nil viper, deliberately. A `server.Context` carries no viper + until `Apply` populates one, and a nil-tolerant comparison would let two contexts + nobody populated compare equal and report a parity that was never established. + ## Hermeticity These tests control more than the arguments to the function under test, because the diff --git a/testutil/configtest/viper.go b/testutil/configtest/viper.go index 5e787a2925..28e80d8282 100644 --- a/testutil/configtest/viper.go +++ b/testutil/configtest/viper.go @@ -34,3 +34,48 @@ func DumpViper(v *viper.Viper) string { } return strings.Join(lines, "\n") } + +// Settings renders a viper instance as a flat map from dotted key to resolved +// value: one entry per AllKeys entry, each value as Get returns it. +// +// Use this, not AllSettings, to compare two vipers, for two independent reasons. +// +// The first is determinism. AllSettings re-nests the flat key space by splitting each +// key on "." and merging the pieces into a tree, and when one key is a dotted prefix +// of another — "giga" alongside "giga.x" — whether the scalar or the sub-tree survives +// depends on map iteration order. Two AllSettings calls over one file then disagree, so +// an equality assertion between two of them can fail on identical input. Get is +// unaffected: it tries longest prefixes first, so both keys resolve. Settings therefore +// keys on AllKeys and reads each key through Get, which is stable by construction. +// +// The second is coverage, and it is the one that makes this comparison stronger rather +// than merely repeatable. AllSettings omits every key whose Get returns nil, on the +// reasoning that AllKeys lists only keys that hold a value. The section-level +// environment shadow this suite pins is precisely where that reasoning fails: AllKeys +// lists giga_executor.enabled while Get returns nil, so AllSettings drops the key +// entirely and Settings records it as an explicit nil entry. Two vipers must therefore +// agree on which keys resolve to nothing, not only on the values of the keys that +// resolve. That is a strictly larger surface: under AllSettings a key one side enumerates +// and resolves to nil is indistinguishable from a key the other side never enumerated at +// all, which is the pair a manager reading a different file set would produce. +// +// A flat map rather than DumpViper's rendered string, for two reasons. Values keep +// their concrete Go type, so int64(8) and "8" do not compare equal. And a key is a +// map key rather than a line in a newline-joined document, so a key containing a +// newline — legal in TOML, and reachable from a fuzz target that appends arbitrary +// bytes — cannot make two different key sets render identically. DumpViper stays +// the right tool for a readable failure message; this is the right tool for the +// assertion itself. +// +// A nil viper is left to panic on AllKeys rather than guarded into an empty result, +// which is the same distinction: DumpViper renders because describing a +// broken state is its job, while an empty result here would let two contexts nobody +// populated compare equal and report a parity that was never established. +func Settings(v *viper.Viper) map[string]any { + keys := v.AllKeys() + settings := make(map[string]any, len(keys)) + for _, k := range keys { + settings[k] = v.Get(k) + } + return settings +} diff --git a/testutil/configtest/viper_settings_test.go b/testutil/configtest/viper_settings_test.go new file mode 100644 index 0000000000..31da47c919 --- /dev/null +++ b/testutil/configtest/viper_settings_test.go @@ -0,0 +1,188 @@ +package configtest_test + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/sei-protocol/sei-chain/testutil/configtest" + "github.com/spf13/viper" + "github.com/stretchr/testify/require" +) + +// newViperOver returns a viper whose only layer is the given app.toml body: no prefix, +// no AutomaticEnv, no replacer. Settings's interaction with the environment is pinned +// against the real server viper in cmd/seid/cmd, so nothing here needs an env layer. +func newViperOver(t *testing.T, body string) *viper.Viper { + t.Helper() + return newViperOverFile(t, writeAppTOML(t, body)) +} + +// writeAppTOML writes body to an app.toml in a fresh temp directory and returns its path. +func writeAppTOML(t *testing.T, body string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "app.toml") + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("write app.toml: %v", err) + } + return path +} + +// newViperOverFile returns a viper that has read the app.toml at path, the same single +// file layer newViperOver builds. It is separate so a test can read one file many times: +// the nondeterminism such a test is after lives in viper's map iteration, not in the +// parse, so re-reading one path varies only the thing under study. +func newViperOverFile(t *testing.T, path string) *viper.Viper { + t.Helper() + v := viper.New() + v.SetConfigFile(path) + if err := v.ReadInConfig(); err != nil { + t.Fatalf("read app.toml: %v", err) + } + return v +} + +// TestSettingsIsStableWhereAllSettingsIsNot is the reason Settings exists, and it is +// written as a comparison so the defect and the fix are visible in one place. +// +// The shape that breaks AllSettings is one key being a dotted prefix of another. That +// makes "giga" ambiguous — a value, or the parent of "giga.x" — and AllSettings resolves +// the ambiguity by map iteration order, which Go randomizes per run. Settings never +// re-nests, so there is no ambiguity to resolve. +// +// Both halves are assertions, and the AllSettings half holds on every single read rather +// than over the sample. Whichever order the re-nesting visits the two keys in, one +// value is destroyed: reaching "giga.x" through a scalar "giga" replaces that scalar with +// a fresh map, and writing "giga" over an existing sub-tree discards the sub-tree. So the +// re-nested tree always holds one leaf fewer than AllKeys has keys. Only *which* value is +// lost varies with the ordering, which is the instability the shape count reports. +// +// Asserting the loss rather than the variation is what makes this a regression guard: it +// cannot depend on hitting both orderings within the sample, and a viper that fixed the +// collision fails here instead of quietly making the test meaningless. +func TestSettingsIsStableWhereAllSettingsIsNot(t *testing.T) { + const body = ` +[section] +"giga" = 1 +"giga.x" = 2 +` + // Small on purpose. Both assertions below are deterministic per read — Settings cannot + // vary, and the leaf loss holds whichever way deepSearch re-nested — so the sample size + // feeds only the informational shape count. At the measured ~4:1 split, 20 reads show + // both orderings better than 99% of the time, and 180 fewer parses keeps this out of + // the package's runtime. + const runs = 20 + + // One file, read many times. Writing it once keeps the parse out of the experiment, + // leaving map iteration order as the only thing that differs between reads. + path := writeAppTOML(t, body) + + // The shape fingerprint is fmt.Sprint of a map, which is stable only because fmt + // sorts map keys before printing (Go 1.12+). Worth stating in a test whose whole + // premise is that map ordering is untrustworthy: the ordering fmt hides is the one + // this test relies on, and the ordering AllSettings exposes is the one it measures. + // Without the sort, every read would look like a distinct shape and the counts below + // would be meaningless rather than wrong. + + settingsShapes := map[string]int{} + allSettingsShapes := map[string]int{} + for i := 0; i < runs; i++ { + v := newViperOverFile(t, path) + settingsShapes[fmt.Sprint(configtest.Settings(v))]++ + + all := v.AllSettings() + allSettingsShapes[fmt.Sprint(all)]++ + + keys := v.AllKeys() + require.Len(t, keys, 2, + "the fixture must present the prefix collision this test is about; got keys %v", keys) + require.Equal(t, len(keys)-1, countLeaves(all), + "AllSettings must lose exactly one of the two colliding values on every read, "+ + "whichever way it re-nested; got %v for keys %v. This characterizes viper, "+ + "not this repo: if it fails after a viper upgrade, AllSettings may have "+ + "fixed the prefix collision, in which case delete this assertion rather "+ + "than working around it — Settings stays correct either way", all, keys) + } + + require.Len(t, settingsShapes, 1, + "Settings must return one shape across %d reads of one file; got %d: %v", + runs, len(settingsShapes), settingsShapes) + + t.Logf("AllSettings returned %d distinct shapes across %d reads, each of them lossy: %v", + len(allSettingsShapes), runs, allSettingsShapes) +} + +// countLeaves counts the non-map values in a nested settings tree, which is how many of +// AllKeys's keys survived being re-nested into it. +func countLeaves(m map[string]any) int { + n := 0 + for _, v := range m { + if sub, ok := v.(map[string]any); ok { + n += countLeaves(sub) + continue + } + n++ + } + return n +} + +// TestSettingsPreservesConcreteTypes pins the property that makes Settings usable for a +// parity assertion: a TOML integer and a TOML string are not interchangeable, so a +// comparison built on Settings catches a manager that resolves one as the other. +func TestSettingsPreservesConcreteTypes(t *testing.T) { + v := newViperOver(t, "[s]\nnum = 8\nstr = \"8\"\n") + got := configtest.Settings(v) + + require.Equal(t, int64(8), got["s.num"], "a TOML integer must stay an integer") + require.Equal(t, "8", got["s.str"], "a TOML string must stay a string") + require.NotEqual(t, got["s.num"], got["s.str"], + "a parity comparison must be able to tell 8 from \"8\"") +} + +// TestSettingsKeysCannotCollideThroughRendering pins the second reason Settings is a map +// rather than a rendered document: a rendering can collide where the underlying key sets +// differ, and a map cannot. +// +// The fixtures are built to reach that collision rather than merely to differ, which is +// the whole difficulty. DumpViper renders each key as `key = type(value)` and joins with +// newlines, and a TOML key may legally contain a newline — so a single key whose text +// reproduces one map's entire rendering makes the two documents byte-identical. The +// second fixture is exactly that: one root key spelled `s.a = int64(1)\ns.b` holding +// int64(2), which renders as `s.a = int64(1)\ns.b = int64(2)` — the same two lines the +// first fixture's two keys produce. +// +// An earlier version of this test used `"a = 1\nb" = 2` and asserted only that the two +// maps differ. That passed for a reason unrelated to the property: the renderings were +// never equal, because DumpViper wraps the value as int64(1) while the key text carried a +// bare 1. Two maps of different sizes are unequal, so the assertion held while the +// collision it exists to demonstrate was never constructed. That is worth recording here, +// because it is the failure this file is otherwise about: an assertion that passes for a +// reason its own comment does not claim. +func TestSettingsKeysCannotCollideThroughRendering(t *testing.T) { + twoKeys := newViperOver(t, "[s]\n\"a\" = 1\n\"b\" = 2\n") + oneKey := newViperOver(t, "\"s.a = int64(1)\\ns.b\" = 2\n") + + // The premise: the two renderings really are identical. Without this the assertion + // below is the vacuous one described above. + require.Equal(t, configtest.DumpViper(twoKeys), configtest.DumpViper(oneKey), + "the fixtures must reach the rendering collision this test is about, or the "+ + "comparison below proves nothing about it") + + // The property: Settings keeps them apart anyway, because it compares map keys rather + // than a joined document. + two, one := configtest.Settings(twoKeys), configtest.Settings(oneKey) + require.NotEqual(t, two, one, + "Settings must distinguish key sets whose rendering collides") + require.Len(t, two, 2) + require.Len(t, one, 1, "a newline-containing key must stay a single entry") +} + +// TestSettingsOnNilViperPanics pins the nil case as a failure rather than a tolerance, +// since the differential builds a server.Context whose Viper stays nil until Apply +// populates it. An empty map would let two such contexts compare equal, so the +// differential's strongest premise would report success on a boot that never ran. The +// panic is what makes the guard at those call sites redundant rather than load-bearing. +func TestSettingsOnNilViperPanics(t *testing.T) { + require.Panics(t, func() { configtest.Settings(nil) }) +}