From d5e464341979600fdb767a6e5906b928eb0658bc Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 24 Aug 2026 09:01:37 -0700 Subject: [PATCH 1/7] feat(config): install what sei.toml supplies at boot A node that selects this configuration manager reads sei.toml, resolves it against the binary's declared defaults, and installs the result into the source the rest of the boot reads. Selecting the manager is a switch rather than a configuration change: a node with no sei.toml, an unreadable one, or one naming a mode this binary does not declare defaults for installs nothing and every key reads as it always has. Only the keys something other than the defaults supplied are installed. A resolution answers for every declared key, so installing all of it would write a default over whatever an operator's app.toml holds for each of the hundred and fifty keys their sei.toml does not mention. A key reaches the node exactly when a source supplied it. The flag snapshot is taken at the entry to Apply, before the handler that copies configuration values into flags and marks them changed. After that runs, a flag an operator typed and a key their app.toml holds cannot be told apart, and a flag layer built from that state would put app.toml above sei.toml. Two things nothing else reports are logged: a key the file writes that no section declares, and an environment variable set for a key the environment cannot carry. Resolve now reports an undeclared name for the file only. A node is started with flags that were never configuration keys, so pooling all three sources warned about thirty-nine working flags on every boot with the file's one real typo somewhere inside the list. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/boot_install_test.go | 267 ++++++++++++++++++++ cmd/seid/cmd/configmanager/configmanager.go | 14 +- cmd/seid/cmd/configmanager/install.go | 199 +++++++++++++++ config/registry/doc.go | 9 +- config/registry/resolve.go | 32 ++- config/registry/spec_test.go | 52 ++++ 6 files changed, 562 insertions(+), 11 deletions(-) create mode 100644 cmd/seid/cmd/boot_install_test.go create mode 100644 cmd/seid/cmd/configmanager/install.go diff --git a/cmd/seid/cmd/boot_install_test.go b/cmd/seid/cmd/boot_install_test.go new file mode 100644 index 0000000000..8e12d33f73 --- /dev/null +++ b/cmd/seid/cmd/boot_install_test.go @@ -0,0 +1,267 @@ +package cmd + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "go.opentelemetry.io/otel/sdk/trace" + + "github.com/sei-protocol/sei-chain/cmd/seid/cmd/configmanager" + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// Keys these tests measure through, and why each one. +// +// The first two are declared keys that nothing else in a booting node answers: no start flag carries them +// and the generated app.toml does not name them, so a value read back for one came from this install and +// from nowhere else. Of a hundred and fifty declared keys only eleven are like that, and the rest are +// reachable by a flag of the same name, whose registration default answers before the lookup comes back +// empty. Measuring through one of those would be reading the flag's default and calling it an install. +// +// The third is the opposite case on purpose: a key a start flag does carry, so it is the one that can show +// the flag channel reaching a declared key at all. +const ( + bootProbeKey = "evm.max_tx_pool_txs" + bootUntouchedKey = "state-commit.sc-snapshot-writer-limit" + bootFlagKey = "state-sync.snapshot-keep-recent" +) + +// bootWith runs a real boot against a sei.toml and returns the source a node would read. +// +// Flags are set through the command rather than handed to the install, because it is the flag being marked +// changed that the snapshot reads. A value poked in directly would hold even if the boot never looked at +// the command line. +func bootWith(t *testing.T, body string, typed map[string]string) *server.Context { + t.Helper() + home := configtest.NewHome(t) + if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + if body != "" { + path := filepath.Join(home.Root, "config", "sei.toml") + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + } + + cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home.Root); err != nil { + t.Fatalf("set --home: %v", err) + } + for name, value := range typed { + if err := cmd.Flags().Set(name, value); err != nil { + t.Fatalf("set --%s=%s: %v", name, value, err) + } + } + ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("the boot was refused: %v", err) + } + return ctx +} + +// seiTomlWriting returns a file body that writes one key, wherever that key belongs. +// +// A key with no section goes above every table. Once a table heading is open every bare key after it +// belongs to that table, so a node-wide setting written after one would be read under the wrong name. +func seiTomlWriting(key, value string) string { + const header = "schema_version = 1\nnode_mode = \"validator\"\n" + if i := indexOf(key, '.'); i >= 0 { + return header + "\n[" + key[:i] + "]\n" + key[i+1:] + " = " + value + "\n" + } + return header + key + " = " + value + "\n" +} + +func indexOf(s string, c byte) int { + for i := 0; i < len(s); i++ { + if s[i] == c { + return i + } + } + return -1 +} + +// TestEachChannelWinsOverTheOneBelowIt drives the declared order through a real boot. +// +// Every channel that can carry a value has to reach the resolution. A channel that is not wired does not +// fail, it stops applying: a value an operator supplied through it loses to a lower layer and nothing +// reports it. So each one is supplied a value and the declared order has to hold. +func TestEachChannelWinsOverTheOneBelowIt(t *testing.T) { + t.Run("nothing written leaves the key as it was", func(t *testing.T) { + configtest.Isolate(t) + ctx := bootWith(t, "schema_version = 1\nnode_mode = \"validator\"\n", nil) + if got := ctx.Viper.Get(bootProbeKey); got != nil { + t.Errorf("%s reads %#v with nothing written. A file that supplies no value installs nothing, "+ + "so this key should read as it did before the manager ran", bootProbeKey, got) + } + }) + + t.Run("the file beats the default", func(t *testing.T) { + configtest.Isolate(t) + ctx := bootWith(t, seiTomlWriting(bootProbeKey, "111"), nil) + if got := ctx.Viper.Get(bootProbeKey); !sameSetting(got, int64(111)) { + t.Errorf("%s reads %#v with 111 written, want 111. A file channel that is not passed to the "+ + "resolution leaves the operator's value losing to the default", bootProbeKey, got) + } + }) + + t.Run("the environment beats the file", func(t *testing.T) { + configtest.Isolate(t) + t.Setenv(registry.EnvName(bootProbeKey), "222") + ctx := bootWith(t, seiTomlWriting(bootProbeKey, "111"), nil) + if got := ctx.Viper.Get(bootProbeKey); !sameSetting(got, "222") { + t.Errorf("%s reads %#v with 111 in the file and 222 in the environment, want 222", bootProbeKey, got) + } + }) + + t.Run("a typed flag beats both", func(t *testing.T) { + configtest.Isolate(t) + t.Setenv(registry.EnvName(bootFlagKey), "222") + ctx := bootWith(t, seiTomlWriting(bootFlagKey, "111"), map[string]string{bootFlagKey: "333"}) + if got := ctx.Viper.Get(bootFlagKey); !sameSetting(got, "333") { + t.Errorf("%s reads %#v with 111 in the file, 222 in the environment and --%s=333 typed, "+ + "want 333. An operator who types a flag to override a file has to win, and a flag "+ + "whose name never reaches the resolution loses to both", bootFlagKey, got, bootFlagKey) + } + }) +} + +// TestOnlyWhatASourceSuppliedIsInstalled is the property that makes this safe to enable. +// +// A resolution answers for every declared key. Installing all of it would write a default over whatever a +// node's app.toml holds for every key its sei.toml does not mention, so moving one setting would replace a +// hundred and fifty. This installs only what a source supplied, so a key reaches a node exactly when +// somebody asked for it. +// +// Measured as an absence rather than against a value read back from a second boot. A baseline taken through +// this same install would carry whatever the install wrote, so an install that wrote a default over every +// key would write the same one twice and the two runs would agree. The assertion is that the key is not +// there at all, which no install can satisfy by being wrong the same way twice. +func TestOnlyWhatASourceSuppliedIsInstalled(t *testing.T) { + configtest.Isolate(t) + + // The declared value is read out first, because a key whose declaration answers nothing would pass + // this whether the install was contained or not. + const untouched = bootUntouchedKey + resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if resolved.Values[untouched] == nil { + t.Fatalf("%s declares no value, so an install that wrote every declared default would leave it "+ + "absent too and this would measure nothing", untouched) + } + + ctx := bootWith(t, seiTomlWriting(bootProbeKey, "111"), nil) + + if got := ctx.Viper.Get(untouched); got != nil { + t.Errorf("%s reads %#v after a file that never mentions it, and %s declares %#v. Installing a "+ + "declared default over a key nobody wrote replaces an operator's configuration rather than "+ + "moving one setting of it", untouched, got, untouched, resolved.Values[untouched]) + } + if got := ctx.Viper.Get(bootProbeKey); !sameSetting(got, int64(111)) { + t.Errorf("%s reads %#v, so nothing was installed at all and the check above holds for an install "+ + "that does nothing", bootProbeKey, got) + } +} + +// TestAppTomlDoesNotReachTheFlagChannel is the guard on where the flag snapshot is taken. +// +// The handler this manager re-enters copies configuration values into flags, so that a file can supply a +// flag's default: for every flag whose name its source knows a value for, it calls Set, and Set marks the +// flag changed. After that has run, a flag an operator typed and a key their app.toml holds cannot be told +// apart. +// +// A flag channel built from that state puts app.toml at the top of the order, above sei.toml, which is a +// worse inversion than the one the channel exists to prevent. Taking the snapshot at the entry to Apply is +// what keeps the two apart, and there is no later point where the truth survives. +func TestAppTomlDoesNotReachTheFlagChannel(t *testing.T) { + const key = "state-sync.snapshot-keep-recent" + if _, declared := declaredKey(key); !declared { + t.Skipf("%s is not declared, so this cannot happen through it", key) + } + configtest.Isolate(t) + + home := configtest.NewHome(t) + // app.toml holds one value and sei.toml another, and the operator typed no flag at all. + home.WriteAppTOML(t, []byte("[state-sync]\nsnapshot-keep-recent = 77\n")) + if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + body := "schema_version = 1\nnode_mode = \"validator\"\n\n[state-sync]\nsnapshot-keep-recent = 111\n" + if err := os.WriteFile(filepath.Join(home.Root, "config", "sei.toml"), []byte(body), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + + cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home.Root); err != nil { + t.Fatalf("set --home: %v", err) + } + ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("the boot was refused: %v", err) + } + + if got := ctx.Viper.Get(key); !sameSetting(got, int64(111)) { + t.Errorf("%s reads %#v with 77 in app.toml, 111 in sei.toml and no flag typed, want 111.\n\n"+ + "A value of 77 means app.toml arrived through the flag channel, because the handler marked "+ + "the flag changed on its behalf. The snapshot has to be taken before the handler runs", key, got) + } +} + +// TestAFileThisBinaryCannotUseLeavesTheNodeAsItWas is the promise that makes the switch safe. +// +// Selecting this manager is a switch rather than a configuration change, so a file it cannot use installs +// nothing and the node reads what it always read. Refusing instead would turn a mistyped line in a +// hand-editable file into an outage on the next restart. +// +// Every case writes a value for a declared key, so a file that was wrongly accepted would install one and +// the assertion would see it. A case supplying nothing would read as unusable whether it was refused or +// accepted, which measures the absence of a value rather than the refusal. +func TestAFileThisBinaryCannotUseLeavesTheNodeAsItWas(t *testing.T) { + supplies := "\n[evm]\nmax_tx_pool_txs = 111\n" + for name, body := range map[string]string{ + "no file at all": "", + "a mode nothing knows": "schema_version = 1\nnode_mode = \"sentry\"\n" + supplies, + "no mode at all": "schema_version = 1\n" + supplies, + "not parseable": "schema_version = 1\nnode_mode = \"validator\"\n[evm\n" + supplies, + } { + t.Run(name, func(t *testing.T) { + configtest.Isolate(t) + ctx := bootWith(t, body, nil) + if got := ctx.Viper.Get(bootProbeKey); got != nil { + t.Errorf("%s reads %#v, so a value was installed from a file this binary cannot use. "+ + "A node whose file names a mode this binary does not know would run one mode's "+ + "answers while being configured as another", bootProbeKey, got) + } + }) + } +} + +// sameSetting compares two resolved values without caring which shape carried them. +// +// A value reaches a source as its own Go type from a default, as whatever the file format decoded to from +// a file, and as one string from a variable. A comparison that insisted on the type would be asserting +// which channel answered rather than what the node reads. +func sameSetting(a, b any) bool { + if a == nil || b == nil { + return a == nil && b == nil + } + return fmt.Sprint(a) == fmt.Sprint(b) +} + +// declaredKey reports whether the registry declares a key. +func declaredKey(key string) (string, bool) { + for _, section := range registry.Sections() { + for _, k := range section.Keys { + if k == key { + return section.Name, true + } + } + } + return "", false +} diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index c6e6eec499..3e53e317e6 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -80,10 +80,22 @@ func (m SeiConfigManager) log() *slog.Logger { // handler and return nil, turning a boot the legacy path aborts into a successful one. // TestApplyPropagatesALegacyHandlerPanic fails on that combination. func (m SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { + // Before the handler, because the handler copies configuration values into flags and marks them + // changed. Afterwards there is no way to tell a flag an operator typed from a key their app.toml + // holds, and treating the second as the first would put app.toml above sei.toml. + typed := TypedFlags(cmd) + out := validateAdvisory(cmd) err := server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) reportAdvisory(m.log(), out) - return err + if err != nil { + return err + } + + // After the handler, because the source it builds is the one the resolved values go into and it does + // not exist before. Nothing this does can refuse the boot. + installResolved(cmd, typed, m.log()) + return nil } // reportAdvisory logs an advisory outcome, containing a panic from the logging itself. diff --git a/cmd/seid/cmd/configmanager/install.go b/cmd/seid/cmd/configmanager/install.go new file mode 100644 index 0000000000..eec7c6797d --- /dev/null +++ b/cmd/seid/cmd/configmanager/install.go @@ -0,0 +1,199 @@ +package configmanager + +import ( + "log/slog" + "os" + "path/filepath" + "strings" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" + + "github.com/sei-protocol/sei-chain/config/appopts" + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/config/seitoml" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + + // The sections whose keys belong to the upstream server, which nothing else imports. A section + // reaches the registry through its owning package's initialisation, so a section nothing imports is + // absent from what this installs and absent silently, since an undeclared key is left to whatever + // answered it before. + _ "github.com/sei-protocol/sei-chain/config/cosmosbase" +) + +// seiTomlName is the file this manager reads. +const seiTomlName = "sei.toml" + +// installResolved puts the values sei.toml supplies into the source the boot has just built. +// +// Nothing here can stop a node starting. A node with no sei.toml, an unreadable one, or one recording a +// mode this binary does not know installs nothing and reads exactly as it always has, so selecting this +// manager is a switch rather than a configuration change. Refusing instead would turn a mistyped line in +// a hand-editable file into an outage on the next restart. +func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logger) { + ctx := server.GetServerContextFromCmd(cmd) + if ctx == nil || ctx.Viper == nil { + log.Warn("no configuration source to install into; every key reads as it always has") + return + } + + file, ok := readSeiToml(cmd, log) + if !ok { + return + } + mode, ok := recordedMode(file, log) + if !ok { + return + } + written, err := file.Values() + if err != nil { + log.Warn("cannot read the values sei.toml writes; every key reads as it always has", "err", err) + return + } + + // Every channel an operator can use. Omitting one installs a lower layer over the top of what they + // chose, which is a value silently ignored rather than a value overridden. The flag channel matters + // most: an installed value sits above a bound flag, so a declared key a flag also delivers would + // resolve without ever seeing the command line and then bury it. + resolved, err := registry.Resolve(registry.Mode(mode), registry.Sources{ + File: written, + LookupEnv: os.LookupEnv, + Flags: flagValues(typed), + }) + if err != nil { + log.Warn("cannot resolve this node's configuration; every key reads as it always has", + "mode", mode, "err", err) + return + } + reportWhatTheFileDidNotReach(resolved, log) + + supplied := onlyWhatASourceSupplied(resolved) + if len(supplied.Values) == 0 { + log.Info("sei.toml supplies no declared value; every key reads as it always has", "mode", mode) + return + } + report, err := appopts.Install(ctx.Viper, supplied) + if err != nil { + log.Warn("cannot install the values sei.toml supplies; every key reads as it always has", + "err", err) + return + } + log.Info("configuration installed", "mode", mode, + "installed", strings.Join(report.Installed, ",")) +} + +// onlyWhatASourceSupplied narrows a resolution to the keys something other than the defaults answered. +// +// This is the whole difference between moving a setting and replacing a file. A resolution answers for +// every declared key, so installing all of it would write a default over whatever an operator's app.toml +// holds for every key their sei.toml does not mention: a hundred and fifty settings replaced because they +// moved one. Installing only what a source supplied means a key reaches the node exactly when somebody +// asked for it, and every other key reads as it always has. +// +// It also means a declared default never reaches a running node, which is what lets a default state what +// the provisioning command writes rather than having to state what each node already runs. +func onlyWhatASourceSupplied(resolved registry.Resolved) registry.Resolved { + out := registry.Resolved{Values: make(map[string]any, len(resolved.Overrides))} + for _, key := range resolved.Overrides { + out.Values[key] = resolved.Values[key] + } + return out +} + +// reportWhatTheFileDidNotReach says what an operator asked for that had no effect. +// +// Two things, and neither is visible anywhere else. A key no section declares is one this file cannot +// deliver, so it reads as a setting and changes nothing. And a variable set for a key no environment +// variable can carry is ignored on purpose, with the reason recorded where the key is declared. +// +// Reported once each rather than per key, because a node resolves over a hundred declared keys and a line +// each would bury the two or three that matter in the noise it creates. +func reportWhatTheFileDidNotReach(resolved registry.Resolved, log *slog.Logger) { + if len(resolved.Unknown) > 0 { + log.Warn("sei.toml writes keys no section declares; they have no effect", + "count", len(resolved.Unknown), "keys", strings.Join(resolved.Unknown, ",")) + } + if len(resolved.Ignored) == 0 { + return + } + cannot := registry.EnvCannotDeliver() + for _, key := range resolved.Ignored { + log.Warn("an environment variable is set for a key the environment cannot supply; it has no "+ + "effect and the file's value applies", "key", key, "variable", registry.EnvName(key), + "why", cannot[key]) + } +} + +// readSeiToml loads the node's sei.toml, reporting the ordinary absence quietly. +// +// A node that has not generated one is the expected state while sections are still moving, so that is not +// a warning. A file that exists and will not parse is, because somebody wrote it and it is not doing what +// they think. +func readSeiToml(cmd *cobra.Command, log *slog.Logger) (*seitoml.File, bool) { + home, err := resolveHomeDir(cmd) + if err != nil { + log.Warn("cannot resolve the home directory; every key reads as it always has", "err", err) + return nil, false + } + path := filepath.Join(home, "config", seiTomlName) + + file, err := seitoml.Load(path) + if err != nil { + log.Debug("no readable sei.toml; every key reads as it always has", "path", path, "err", err) + return nil, false + } + return file, true +} + +// recordedMode reads the node mode the file records. +// +// Every value a node reads through the registry is the resolution for one mode, so a file that does not +// say which cannot be used at all. Reported rather than guessed: guessing picks one mode's answers for a +// node configured as another. +// +// Whether the mode is one this binary knows is not checked here. The resolution refuses a mode no section +// declares defaults for, and it names the modes there are, so a check here would be the same guard a +// second time and a worse message. +func recordedMode(file *seitoml.File, log *slog.Logger) (string, bool) { + mode, err := file.Mode() + if err != nil { + log.Warn("sei.toml records no usable node mode; every key reads as it always has", "err", err) + return "", false + } + return mode, true +} + +// TypedFlags records which flags this invocation carried, and has to run before anything else touches +// them. +// +// A flag reports itself changed when something called Set on it, and the handler this manager re-enters +// calls Set on every flag whose name its configuration knows a value for, so that a file can supply a +// flag's default. After that has run, a flag an operator typed and a key their app.toml holds are +// indistinguishable, and a flag channel built from that state would put app.toml above sei.toml. That is +// a worse inversion than the one the channel exists to prevent: the file an operator is being migrated +// onto would lose to the file they are being migrated off. +// +// So the snapshot is taken at the one point before that happens, which is the entry to Apply. Taking it +// there rather than inside the install is the difference between an invariant and a convention, because +// there is no later point at which the truth is still available. +func TypedFlags(cmd *cobra.Command) map[string]string { + out := map[string]string{} + cmd.Flags().VisitAll(func(f *pflag.Flag) { + if f.Changed { + out[strings.ToLower(f.Name)] = f.Value.String() + } + }) + return out +} + +// flagValues renders a snapshot of typed flags as a configuration source. +func flagValues(typed map[string]string) map[string]any { + if len(typed) == 0 { + return nil + } + out := make(map[string]any, len(typed)) + for name, value := range typed { + out[name] = value + } + return out +} diff --git a/config/registry/doc.go b/config/registry/doc.go index 46647deaa0..2610487898 100644 --- a/config/registry/doc.go +++ b/config/registry/doc.go @@ -58,10 +58,17 @@ // no exported value repeats it. // // Alongside the values it reports which keys something other than the defaults supplied, and which -// keys a source carried that no section declares. The first is what a diff renders, since a written +// keys the file carried that no section declares. The first is what a diff renders, since a written // value and a default are otherwise indistinguishable once merged. The second is why a typo in an // operator's file is visible rather than silently dropped. // +// The file and not every source, because an undeclared name means something different in each. A file +// exists to carry declared keys, so one that is not is a typo. The environment layer looks up only +// names a section declares, so it cannot produce one. The command line is a namespace this package does +// not own: most of the flags a node starts with were never configuration keys, and a misspelled one is +// refused by the command before any of this runs, so reporting those would bury the file's one typo +// under forty names working exactly as intended. +// // One channel has a per-key hole. A reader that takes its value's exact type cannot be handed the one // string an environment carries, so a section may refuse that channel for such a key, and the file's // value applies instead of a value that would stop the node. The variable is still read and its value diff --git a/config/registry/resolve.go b/config/registry/resolve.go index 9db27c1506..01d85d8708 100644 --- a/config/registry/resolve.go +++ b/config/registry/resolve.go @@ -31,7 +31,14 @@ type Resolved struct { // An ignored one is read, and the operator reached for the one channel that cannot carry it, so the // value they wrote elsewhere is what applies. EnvCannotDeliver says why, per key. Ignored []string - // Unknown are keys a source carried that no section declares, sorted. + // Unknown are keys the file carried that no section declares, sorted. + // + // The file only, and not every source, because an undeclared name means something different in each. + // A file exists to carry declared keys, so one that is not is a typo. The environment layer looks up + // only names a section declares, so it cannot produce one at all. The command line is a namespace + // this package does not own: most of the flags a node starts with are not configuration keys, and a + // misspelled one is refused by the command before any of this runs, so reporting those would bury the + // file's one typo under forty names that are working exactly as intended. // // Reported rather than an error, because what to do about one is the caller's decision: a // generate path may want to refuse, while a boot on an operator's existing file must not. @@ -121,16 +128,23 @@ func Resolve(mode Mode, from Sources) (Resolved, error) { // order, which is why nothing exports it. fromEnv, ignored := envValues(declared, undeliverable, from.LookupEnv) out.Ignored = ignored - for _, values := range []map[string]any{ - fileValues(from.File), - fromEnv, - from.Flags, + for _, layer := range []struct { + values map[string]any + // namesAreAllKeys says every name in this layer is meant to be a declared key, so one that is + // not gets reported. True of the file alone; Unknown records why. + namesAreAllKeys bool + }{ + {values: fileValues(from.File), namesAreAllKeys: true}, + {values: fromEnv}, + {values: from.Flags}, } { - for key, v := range values { + for key, v := range layer.values { if !declared[key] { - // A key nothing declares cannot be resolved into anything, and silently dropping it is - // how an operator's typo becomes invisible. - unknown[key] = true + // A key nothing declares cannot be resolved into anything, and silently dropping one the + // operator meant as a setting is how a typo becomes invisible. + if layer.namesAreAllKeys { + unknown[key] = true + } continue } out.Values[key] = v diff --git a/config/registry/spec_test.go b/config/registry/spec_test.go index 9091b8a965..06be013df5 100644 --- a/config/registry/spec_test.go +++ b/config/registry/spec_test.go @@ -1406,3 +1406,55 @@ func TestAFieldExcludedFromConfigDeclaresNoKey(t *testing.T) { t.Errorf("the refusal reads %q and does not name the untagged field", got) } } + +// TestOnlyTheFileReportsAKeyNoSectionDeclares holds the one distinction between the three layers. +// +// An undeclared name means something different in each. In a file it is a typo, and reporting it is the +// only way an operator learns their setting does nothing. On the command line it is the ordinary case: +// most of the flags a node starts with are not configuration keys, so reporting them would produce a +// warning naming forty flags that work on every boot, with the file's one real typo somewhere inside it. +func TestOnlyTheFileReportsAKeyNoSectionDeclares(t *testing.T) { + type layers struct { + Kept string `mapstructure:"kept"` + } + registry.RegisterSection("layers_undeclared_names", &layers{}, func(registry.Mode) any { + return layers{Kept: "declared"} + }) + + resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{ + File: map[string]any{ + "layers_undeclared_names.kept": "from-file", + "layers_undeclared_names.typo": "x", + }, + Flags: map[string]any{"home": "/tmp", "log_level": "info"}, + }) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + + for _, key := range resolved.Unknown { + if key == "home" || key == "log_level" { + t.Errorf("%q is reported as a key no section declares, and it is a flag name. Every boot "+ + "carries flags that were never configuration keys, so this fires always and the file's "+ + "one real typo is somewhere inside a list of forty", key) + } + } + if !contains(resolved.Unknown, "layers_undeclared_names.typo") { + t.Errorf("Unknown is %v and does not name the file's misspelled key. An operator learns their "+ + "setting does nothing only from this", resolved.Unknown) + } + if got := resolved.Values["layers_undeclared_names.kept"]; got != "from-file" { + t.Errorf("layers_undeclared_names.kept is %#v, want the file's value; not reporting a layer's "+ + "undeclared names must not stop its declared ones applying", got) + } +} + +// contains reports whether a sorted key list holds a key. +func contains(keys []string, want string) bool { + for _, key := range keys { + if key == want { + return true + } + } + return false +} From 973f12fa06261b1b080f80a9d8ec65092fe7977b Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 24 Aug 2026 13:13:49 -0700 Subject: [PATCH 2/7] feat(config): deliver the node configuration file's written values A value written for one of those sections resolved correctly and reached nothing. The boot's handler reads that file once into a struct before the install runs, and the node reads the struct, so a value put into the source afterwards was ignored in silence. Those sections are now decoded into that struct instead, which is the mechanism the handler used and therefore the same casts, tags and hooks. A section declares which of the two ways its reader takes, and the sections of this file declare it in the same loop that registers them, so one cannot be registered without being delivered. A section that disagrees fails a test rather than quietly changing nothing. Four properties hold it. Only what a source supplied is decoded. The struct already holds what the node's own file said, so delivering a default over it would replace an operator's file with one nobody chose, on every boot, for every key their sei.toml does not mention. A section at a time. A decode is all or nothing for whatever it is handed, so one refused value would otherwise cost every key in the file rather than the keys of the section it appeared in. Decoded into a copy and published by replacing it. A decoder gathers errors and keeps going, so a refused value partway leaves its target holding some new values and some old. The copy is of the configuration the node already has and not a fresh one, because what a decoder writes can depend on what its target already holds, and only a copy holds the same things. It shares nothing: a decoder writes a list into the array its target holds, so one shared list would edit the original. The log level is applied from the resolution before any of this. A refusal is reported at a level an operator may have raised the threshold above, so waiting for a successful decode would mean the one setting somebody changes to see a refusal is the setting a refusal suppresses. Keys nothing in this binary writes are not declared. A cluster controller resolves a peer set from live discovery and patches the addresses in, a node computes a trust height and hash from the chain tip each start, and a moniker is stamped per instance. A value from here would be decoded over whichever already ran, with the change visible only in memory. Two keys that no longer have any effect and two that exist to make a node misbehave are left out as well. A field holding an interface can no longer be declared. What a decoder writes into one depends on what the field already holds, so a rehearsal in a copy would answer for the copy. Every delivered key that moved is logged with what it moved from. The node's own file still says what it said, and every tool an operator reaches for reads that file, so this is the only place the two can be told apart. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/configmanager/install.go | 38 +++- cmd/seid/cmd/configmanager/tendermint.go | 195 ++++++++++++++++++ cmd/seid/cmd/configmanager/tendermint_copy.go | 184 +++++++++++++++++ .../cmd/configmanager/tendermint_copy_test.go | 115 +++++++++++ cmd/seid/cmd/node_delivery_test.go | 183 ++++++++++++++++ config/registry/delivery.go | 113 ++++++++++ config/registry/registry.go | 97 +++++++-- config/registry/spec_test.go | 45 ++++ config/tendermintbase/tendermintbase.go | 71 ++++++- config/tendermintbase/tendermintbase_test.go | 83 +++++++- 10 files changed, 1090 insertions(+), 34 deletions(-) create mode 100644 cmd/seid/cmd/configmanager/tendermint.go create mode 100644 cmd/seid/cmd/configmanager/tendermint_copy.go create mode 100644 cmd/seid/cmd/configmanager/tendermint_copy_test.go create mode 100644 cmd/seid/cmd/node_delivery_test.go create mode 100644 config/registry/delivery.go diff --git a/cmd/seid/cmd/configmanager/install.go b/cmd/seid/cmd/configmanager/install.go index eec7c6797d..0985e9dad4 100644 --- a/cmd/seid/cmd/configmanager/install.go +++ b/cmd/seid/cmd/configmanager/install.go @@ -19,6 +19,10 @@ import ( // absent from what this installs and absent silently, since an undeclared key is left to whatever // answered it before. _ "github.com/sei-protocol/sei-chain/config/cosmosbase" + + // The sections whose keys belong to the node's own configuration file, which nothing else imports + // either. These are the sections the delivery beside this one decodes rather than installs. + _ "github.com/sei-protocol/sei-chain/config/tendermintbase" ) // seiTomlName is the file this manager reads. @@ -67,7 +71,17 @@ func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logg } reportWhatTheFileDidNotReach(resolved, log) - supplied := onlyWhatASourceSupplied(resolved) + // First, because every failure below is a log line and a refusal is reported at a level an operator + // may have raised the threshold above. Doing this after would mean the one setting somebody changes + // in order to see a refusal is the setting a refusal suppresses. + applyResolvedLogLevel(resolved, log) + + // The sections a reader looks up key by key, and the sections a reader decodes whole. Two deliveries, + // because putting a value into the source is no delivery at all for the second kind: their file is + // read into a struct before this runs and nothing consults the source for them afterwards. + deliverDecodedSections(ctx, resolved, log) + + supplied := onlyWhatALookupSourceSupplied(resolved) if len(supplied.Values) == 0 { log.Info("sei.toml supplies no declared value; every key reads as it always has", "mode", mode) return @@ -82,7 +96,8 @@ func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logg "installed", strings.Join(report.Installed, ",")) } -// onlyWhatASourceSupplied narrows a resolution to the keys something other than the defaults answered. +// onlyWhatALookupSourceSupplied narrows a resolution to the keys something other than the defaults +// answered, for the sections whose readers look a key up rather than decoding one. // // This is the whole difference between moving a setting and replacing a file. A resolution answers for // every declared key, so installing all of it would write a default over whatever an operator's app.toml @@ -92,9 +107,26 @@ func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logg // // It also means a declared default never reaches a running node, which is what lets a default state what // the provisioning command writes rather than having to state what each node already runs. -func onlyWhatASourceSupplied(resolved registry.Resolved) registry.Resolved { +func onlyWhatALookupSourceSupplied(resolved registry.Resolved) registry.Resolved { + decoded := registry.DecodedSections() + owning := map[string]bool{} + for _, section := range registry.Sections() { + if _, ok := decoded[section.Name]; !ok { + continue + } + for _, key := range section.Keys { + owning[key] = true + } + } + out := registry.Resolved{Values: make(map[string]any, len(resolved.Overrides))} for _, key := range resolved.Overrides { + // A key both deliveries carried would be installed into the source as well as decoded, and the + // install refuses a key its own contract does not cover, which would take the whole install down + // and with it every key of every other section. + if owning[key] { + continue + } out.Values[key] = resolved.Values[key] } return out diff --git a/cmd/seid/cmd/configmanager/tendermint.go b/cmd/seid/cmd/configmanager/tendermint.go new file mode 100644 index 0000000000..e38854c401 --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -0,0 +1,195 @@ +package configmanager + +import ( + "fmt" + "log/slog" + "sort" + "strings" + + "github.com/spf13/viper" + + "github.com/sei-protocol/seilog" + + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// deliverDecodedSections puts the resolved values of the decoded sections into the node's own +// configuration. +// +// Putting a value into the source a node reads is the whole delivery for a section whose reader looks its +// keys up one at a time. It is no delivery at all for the sections this covers, which the boot's handler +// reads once into a struct before this runs. Those values are decoded into that struct instead, which is +// the same mechanism the handler used and therefore the same casts, the same tags and the same hooks. +// +// Nothing here can stop a node starting, which is the one promise this manager makes. +func deliverDecodedSections(ctx *server.Context, resolved registry.Resolved, log *slog.Logger) { + bySection := registry.SuppliedByDecodedSection(resolved) + if len(bySection) == 0 { + return + } + if ctx == nil || ctx.Config == nil { + log.Error("no node configuration to deliver into; every one of these keys reads as it always has", + "sections", len(bySection)) + return + } + + // One section at a time. A decode is all or nothing for whatever it is handed, so a single value a + // decoder refuses would otherwise cost every key in the file rather than the keys of the section it + // appeared in. An operator who fixes one setting and mistypes another has to end up with the first + // one applied. + for _, name := range sortedSectionNames(bySection) { + deliverOneSection(ctx, name, bySection[name], log) + } +} + +// deliverOneSection decodes one section's resolved values into the node's configuration. +// +// Decoded into a copy of that configuration first, and published by replacing it. A decoder gathers errors +// and keeps going, so a value it refuses partway leaves its target holding some of the new values and some +// of the old, with nothing to compare against and no way back. Rehearsing into a copy of the configuration +// the node already has, rather than into a fresh one, is what makes the rehearsal answer the same question: +// what a decoder writes can depend on what the target already holds, and only a copy holds the same things. +func deliverOneSection(ctx *server.Context, name string, values map[string]any, log *slog.Logger) { + keys := sortedKeys(values) + + source := viper.New() + for key, value := range values { + source.Set(key, value) + } + + candidate, err := copyNodeConfig(ctx.Config) + if err != nil { + log.Error("cannot copy this node's configuration, so nothing can be delivered into it without "+ + "risking a half-written one; these keys read as they always have", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } + before := describe(ctx.Config, keys) + + if err := source.Unmarshal(candidate); err != nil { + log.Error("a written value in this section was refused, so none of the section is applied and "+ + "every one of its keys reads as it always has", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } + + *ctx.Config = *candidate + reportWhatMoved(name, keys, before, describe(ctx.Config, keys), log) +} + +// copyNodeConfig returns a configuration that holds what this one holds and shares nothing with it. +// +// A shallow copy would share every section, so a decode into the copy would write through to the original +// and a refused value would leave exactly the half-written configuration the copy exists to prevent. This +// copies the top level and every section under it. +// +// Written against the type rather than field by field, so a section added to it is copied without this +// function changing. A field this cannot copy is an error rather than a silent share. +func copyNodeConfig(from *tmcfg.Config) (*tmcfg.Config, error) { + if from == nil { + return nil, fmt.Errorf("no configuration to copy") + } + out := *from + if err := detachSections(&out, from); err != nil { + return nil, err + } + return &out, nil +} + +// reportWhatMoved names every key whose value the delivery changed, and what it changed from. +// +// The node's own configuration file still says what it said, and every tool an operator reaches for reads +// that file: a patch command, a validator, an audit, somebody reading it over their shoulder at three in +// the morning. None of them describes the running node after this. This log line is the only place the two +// can be told apart, so it names the key, what the file gave it and what the node now runs. +// +// Keys that did not move are not reported. An operator who writes the value their file already held has +// changed nothing, and a line saying so buries the ones that did. +func reportWhatMoved(name string, keys []string, before, after map[string]string, log *slog.Logger) { + var moved []string + for _, key := range keys { + if before[key] != after[key] { + moved = append(moved, fmt.Sprintf("%s: %s -> %s", key, before[key], after[key])) + } + } + if len(moved) == 0 { + log.Info("this section's written values match what the node's own file already gave it", + "section", name, "keys", len(keys)) + return + } + log.Info("this section's settings now differ from what the node's own configuration file says", + "section", name, "changed", strings.Join(capDelivered(moved), "; ")) +} + +// capDelivered bounds a report so one file cannot fill a node's log at boot. +func capDelivered(lines []string) []string { + const most = 20 + if len(lines) <= most { + return lines + } + return append(lines[:most:most], fmt.Sprintf("and %d more", len(lines)-most)) +} + +// sortedKeys returns a map's keys in a fixed order, so a log line does not vary between runs. +func sortedKeys(values map[string]any) []string { + out := make([]string, 0, len(values)) + for key := range values { + out = append(out, key) + } + sort.Strings(out) + return out +} + +// sortedSectionNames returns the sections to deliver in a fixed order. +func sortedSectionNames(bySection map[string]map[string]any) []string { + out := make([]string, 0, len(bySection)) + for name := range bySection { + out = append(out, name) + } + sort.Strings(out) + return out +} + +// logLevelKey is the one delivered setting the struct is not the end of. +const logLevelKey = "log-level" + +// applyResolvedLogLevel hands a resolved log level to the logger, which the struct alone does not reach. +// +// The boot's handler reads the level off the struct and sets it before any of this runs, so a value that +// only reaches the struct moves a field and changes no logging. A setting that appears to take and does not +// is what this key space exists to remove. +// +// Applied from the resolution rather than after the decode, and before it. Every failure this manager can +// have is a log line, and a refusal is reported at a level an operator may have raised the threshold above. +// Waiting for a successful decode would mean the one setting somebody changes in order to see a refusal is +// the setting a refusal suppresses. +// +// Which value arrives is already decided: the resolution ranks a flag over the environment over the file. +// A level that cannot be read is reported and skipped, and the node keeps the level it had. +func applyResolvedLogLevel(resolved registry.Resolved, log *slog.Logger) { + supplied := false + for _, key := range resolved.Overrides { + if key == logLevelKey { + supplied = true + } + } + if !supplied { + return + } + text, isText := resolved.Values[logLevelKey].(string) + if !isText { + log.Error("the resolved log level is not text; the node keeps the level it already had", + "value", resolved.Values[logLevelKey]) + return + } + var level slog.Level + if err := level.UnmarshalText([]byte(text)); err != nil { + log.Error("the resolved log level cannot be read; the node keeps the level it already had", + "level", text, "err", err) + return + } + seilog.SetDefaultLevel(level, true) + log.Info("resolved log level applied", "level", text) +} diff --git a/cmd/seid/cmd/configmanager/tendermint_copy.go b/cmd/seid/cmd/configmanager/tendermint_copy.go new file mode 100644 index 0000000000..ac51a1a49c --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -0,0 +1,184 @@ +package configmanager + +import ( + "fmt" + "reflect" + "sort" + "strings" + + "github.com/go-viper/mapstructure/v2" + + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// detachSections makes a copy hold what the original holds without sharing anything a decode can write +// through. +// +// A copy of the struct alone shares every section, every list and every map it points at. A decoder writes +// a list into the array its target already holds, so a shared one means the rehearsal edits the original +// and a refused value leaves exactly the half-written configuration the copy exists to prevent. +// +// Walked over the type rather than field by field, so a section or a list added to the node's configuration +// is detached without this changing. A field it cannot detach is an error rather than a silent share, and +// the test beside this holds every reference in the type against that promise. +func detachSections(out, from *tmcfg.Config) error { + if out == nil || from == nil { + return fmt.Errorf("no configuration to detach") + } + return detachValue(reflect.ValueOf(out).Elem(), "") +} + +// detachValue replaces every reference under v with one nothing else holds. +// +// An unexported field is skipped rather than refused. The copy this walks was made by assigning the struct, +// which copies unexported fields by value, and a decoder cannot write to one either. +func detachValue(v reflect.Value, path string) error { + switch v.Kind() { + case reflect.Pointer: + if v.IsNil() || !v.CanSet() { + return nil + } + fresh := reflect.New(v.Type().Elem()) + fresh.Elem().Set(v.Elem()) + if err := detachValue(fresh.Elem(), path); err != nil { + return err + } + v.Set(fresh) + + case reflect.Struct: + for i := 0; i < v.NumField(); i++ { + f := v.Type().Field(i) + if !v.Field(i).CanSet() { + continue + } + if err := detachValue(v.Field(i), join(path, f.Name)); err != nil { + return err + } + } + + case reflect.Slice: + if v.IsNil() || !v.CanSet() { + return nil + } + fresh := reflect.MakeSlice(v.Type(), v.Len(), v.Len()) + reflect.Copy(fresh, v) + for i := 0; i < fresh.Len(); i++ { + if err := detachValue(fresh.Index(i), path); err != nil { + return err + } + } + v.Set(fresh) + + case reflect.Map: + if v.IsNil() || !v.CanSet() { + return nil + } + fresh := reflect.MakeMapWithSize(v.Type(), v.Len()) + for _, key := range v.MapKeys() { + elem := reflect.New(v.Type().Elem()).Elem() + elem.Set(v.MapIndex(key)) + if err := detachValue(elem, path); err != nil { + return err + } + fresh.SetMapIndex(key, elem) + } + v.Set(fresh) + + case reflect.Interface: + if v.IsNil() || !v.CanSet() { + return nil + } + inner := v.Elem() + fresh := reflect.New(inner.Type()).Elem() + fresh.Set(inner) + if err := detachValue(fresh, path); err != nil { + return err + } + v.Set(fresh) + + case reflect.Chan, reflect.Func, reflect.UnsafePointer: + return fmt.Errorf("%s is a %s, which cannot be copied", path, v.Kind()) + } + return nil +} + +// join builds a field path for a message. +func join(path, field string) string { + if path == "" { + return field + } + return path + "." + field +} + +// describe reads the value the node's configuration currently holds for each key, as text. +// +// Read through the same tags the decode writes through, so a key names the same field in both directions. +// Held as text because what a report needs is whether two values differ and what they are, and comparing +// the shapes a decode produced against the shapes a struct holds would answer a different question. +func describe(cfg *tmcfg.Config, keys []string) map[string]string { + out := map[string]string{} + if cfg == nil { + return out + } + var nested map[string]any + if err := mapstructure.Decode(cfg, &nested); err != nil { + return out + } + flat := map[string]any{} + flatten("", nested, flat) + for _, key := range keys { + if v, ok := flat[key]; ok { + out[key] = fmt.Sprint(v) + } + } + return out +} + +// flatten turns a nested map into one keyed by dotted path. +func flatten(prefix string, in map[string]any, out map[string]any) { + for name, value := range in { + path := name + if prefix != "" { + path = prefix + "." + name + } + if inner, nested := value.(map[string]any); nested { + flatten(path, inner, out) + continue + } + out[path] = value + } +} + +// referencePathsIn returns every path in a type that a copy has to detach, for the test that holds +// detachSections to the type it copies. +func referencePathsIn(t reflect.Type, path string, seen map[reflect.Type]bool) []string { + if seen[t] { + return nil + } + seen[t] = true + defer delete(seen, t) + + var out []string + switch t.Kind() { + case reflect.Pointer, reflect.Slice, reflect.Map, reflect.Interface: + if path != "" { + out = append(out, path) + } + if t.Kind() != reflect.Interface { + out = append(out, referencePathsIn(t.Elem(), path, seen)...) + } + case reflect.Struct: + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + if f.PkgPath != "" { + continue + } + out = append(out, referencePathsIn(f.Type, join(path, f.Name), seen)...) + } + } + sort.Strings(out) + return out +} + +// samePath reports whether two paths name the same field, ignoring repeats a pointer produces. +func samePath(a, b string) bool { return strings.TrimSuffix(a, ".") == strings.TrimSuffix(b, ".") } diff --git a/cmd/seid/cmd/configmanager/tendermint_copy_test.go b/cmd/seid/cmd/configmanager/tendermint_copy_test.go new file mode 100644 index 0000000000..65c8220a1f --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint_copy_test.go @@ -0,0 +1,115 @@ +package configmanager + +import ( + "reflect" + "testing" + + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// TestTheCopyShareNothingWithWhatItCopied is the property the rollback rests on. +// +// The delivery decodes into a copy and publishes by replacing, so a refused value leaves the node's +// configuration untouched. That holds only if the copy shares nothing the decode can write through, and a +// decoder writes a list into the array its target already holds. One shared section, list or map and the +// rehearsal edits the original. +// +// Walked over the whole type rather than the fields anyone thought of, so a reference added to the node's +// configuration fails here rather than quietly sharing. +func TestTheCopyShareNothingWithWhatItCopied(t *testing.T) { + from := tmcfg.DefaultConfig() + // Give every list something in it, so a shared backing array is observable. + from.RPC.CORSAllowedOrigins = []string{"a", "b", "c"} + from.StateSync.RPCServers = []string{"one:1", "two:2"} + from.TxIndex.Indexer = []string{"kv"} + from.Other = map[string]any{"left": "over"} + + out, err := copyNodeConfig(from) + if err != nil { + t.Fatalf("copyNodeConfig: %v", err) + } + + for _, path := range referencePathsIn(reflect.TypeOf(tmcfg.Config{}), "", map[reflect.Type]bool{}) { + a, okA := fieldByPath(reflect.ValueOf(from).Elem(), path) + b, okB := fieldByPath(reflect.ValueOf(out).Elem(), path) + if !okA || !okB { + continue + } + if shares(a, b) { + t.Errorf("%s is shared between the node's configuration and the copy, so a decode into the "+ + "copy writes through to the node and a refused value cannot be rolled back", path) + } + } +} + +// TestTheCopyHoldsWhatItCopied is the other half: detaching must not lose a value. +// +// A copy that shares nothing and holds nothing would pass the test above and deliver a configuration of +// zeroes over a running node. +func TestTheCopyHoldsWhatItCopied(t *testing.T) { + from := tmcfg.DefaultConfig() + from.RPC.CORSAllowedOrigins = []string{"a", "b", "c"} + from.Mempool.Size = 4321 + from.Instrumentation.Prometheus = true + from.Other = map[string]any{"left": "over"} + + out, err := copyNodeConfig(from) + if err != nil { + t.Fatalf("copyNodeConfig: %v", err) + } + if !reflect.DeepEqual(from, out) { + t.Error("the copy does not hold what it copied; a delivery would publish a configuration that " + + "differs from the node's in ways nobody wrote") + } +} + +// fieldByPath walks a dotted field path, following pointers. +func fieldByPath(v reflect.Value, path string) (reflect.Value, bool) { + for _, name := range splitPath(path) { + for v.Kind() == reflect.Pointer { + if v.IsNil() { + return reflect.Value{}, false + } + v = v.Elem() + } + if v.Kind() != reflect.Struct { + return reflect.Value{}, false + } + f := v.FieldByName(name) + if !f.IsValid() { + return reflect.Value{}, false + } + v = f + } + return v, true +} + +// splitPath breaks a dotted field path into its names. +func splitPath(path string) []string { + if path == "" { + return nil + } + var out []string + start := 0 + for i := 0; i < len(path); i++ { + if path[i] == '.' { + out = append(out, path[start:i]) + start = i + 1 + } + } + return append(out, path[start:]) +} + +// shares reports whether two values point at the same memory. +func shares(a, b reflect.Value) bool { + if a.Kind() != b.Kind() { + return false + } + switch a.Kind() { + case reflect.Pointer, reflect.Map: + return !a.IsNil() && !b.IsNil() && a.Pointer() == b.Pointer() + case reflect.Slice: + return a.Len() > 0 && b.Len() > 0 && a.Pointer() == b.Pointer() + } + return false +} diff --git a/cmd/seid/cmd/node_delivery_test.go b/cmd/seid/cmd/node_delivery_test.go new file mode 100644 index 0000000000..fae4911c87 --- /dev/null +++ b/cmd/seid/cmd/node_delivery_test.go @@ -0,0 +1,183 @@ +package cmd + +import ( + "os" + "path/filepath" + "testing" + + "go.opentelemetry.io/otel/sdk/trace" + + "github.com/sei-protocol/sei-chain/cmd/seid/cmd/configmanager" + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// The second delivery, driven the way an operator reaches it. +// +// A section whose reader looks its keys up one at a time is delivered by putting the value into the source. +// The node's own configuration file is read once into a struct before any of that, so a value put into the +// source reaches nothing and has to be decoded into the struct instead. These read the setting the node +// runs rather than the source it was resolved into, because a key can be correct in the source and absent +// from the struct. + +// bootWithNodeFile runs a real boot against a sei.toml and a generated node configuration file. +func bootWithNodeFile(t *testing.T, seiToml string, edit func(*tmcfg.Config)) *server.Context { + t.Helper() + home := configtest.NewHome(t) + dir := filepath.Join(home.Root, "config") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + // The node's own file, generated the way the node generates it, so what the delivery writes over is + // what an operator would actually have. + live := tmcfg.DefaultConfig() + if edit != nil { + edit(live) + } + if err := tmcfg.WriteConfigFile(home.Root, live); err != nil { + t.Fatalf("render the node's configuration file: %v", err) + } + if seiToml != "" { + if err := os.WriteFile(filepath.Join(dir, "sei.toml"), []byte(seiToml), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + } + + cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home.Root); err != nil { + t.Fatalf("set --home: %v", err) + } + ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("the boot was refused: %v", err) + } + if ctx.Config == nil { + t.Fatal("the boot produced no node configuration") + } + return ctx +} + +const nodeFileHeader = "schema_version = 1\nnode_mode = \"validator\"\n" + +// TestAWrittenValueReachesTheNodesOwnConfiguration is the property the whole thing rests on. +func TestAWrittenValueReachesTheNodesOwnConfiguration(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[instrumentation]\nprometheus = true\nmax-open-connections = 41\n", nil) + + if !ctx.Config.Instrumentation.Prometheus { + t.Error("sei.toml turned the metrics listener on and the node runs with it off. The value was " + + "resolved and put into a source that nothing reading this file ever consults") + } + if got := ctx.Config.Instrumentation.MaxOpenConnections; got != 41 { + t.Errorf("sei.toml set max-open-connections to 41 and the node runs %d", got) + } +} + +// TestAnUnwrittenKeyKeepsWhatTheNodesOwnFileSaid separates delivering a value from overwriting one. +// +// A section read by a lookup can be delivered whole, because its reader has nowhere else to get a value +// from. A section read by a decode already holds what its own file said, put there before this ran. So a +// key the operator's sei.toml does not mention has to arrive at whatever that file gave it, and delivering +// a default instead replaces their file with one nobody chose, on every boot. +// +// The fixture turns the key on in the node's own file, where the default is off, so the two disagree. +// Without that they agree and the overwrite is invisible. +func TestAnUnwrittenKeyKeepsWhatTheNodesOwnFileSaid(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nsize = 4321\n", func(live *tmcfg.Config) { + live.Instrumentation.Prometheus = true + }) + + if got := ctx.Config.Mempool.Size; got != 4321 { + t.Fatalf("the written key arrived as %d, so this test cannot tell the two cases apart", got) + } + if !ctx.Config.Instrumentation.Prometheus { + t.Error("the node's own file turned the metrics listener on, sei.toml said nothing about it, and " + + "the node runs with it off. A default was delivered over the operator's own file, which " + + "happens on every boot for every key their sei.toml does not mention") + } +} + +// TestARefusedValueLeavesItsSectionAlone is the promise that makes this safe to enable. +// +// A decoder gathers errors and keeps going, so a value it refuses partway leaves its target holding some +// new values and some old, with nothing to compare against. The delivery decodes into a copy and publishes +// by replacing, so a refused value leaves the section exactly as the node had it. +func TestARefusedValueLeavesItsSectionAlone(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[instrumentation]\nprometheus = true\nmax-open-connections = \"not a number\"\n", nil) + + if got := ctx.Config.Instrumentation.MaxOpenConnections; got != 3 { + t.Errorf("max-open-connections is %d after a refused value, want the 3 the node had. A "+ + "partly applied decode leaves settings nobody chose", got) + } + if ctx.Config.Instrumentation.Prometheus { + t.Error("the value beside the refused one was applied, so a partial decode was published. " + + "Either all of a section's values arrive or none do") + } +} + +// TestARefusedValueCostsOnlyItsOwnSection is why the delivery is per section. +// +// One decode for the whole file would mean an operator who fixed one setting and mistyped another boots +// with neither applied. The mistyped section is lost; the one beside it is not. +func TestARefusedValueCostsOnlyItsOwnSection(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[instrumentation]\nmax-open-connections = \"not a number\"\n"+ + "\n[mempool]\nsize = 4321\n", nil) + + if got := ctx.Config.Instrumentation.MaxOpenConnections; got != 3 { + t.Errorf("the refused section was applied anyway, reading %d", got) + } + if got := ctx.Config.Mempool.Size; got != 4321 { + t.Errorf("mempool.size is %d and sei.toml set it to 4321. A value refused in one section took "+ + "another section's settings down with it", got) + } +} + +// TestEachChannelWinsForADecodedKeyToo is precedence, asserted where a decoded value lands. +func TestEachChannelWinsForADecodedKeyToo(t *testing.T) { + const key = "rpc.max-open-connections" + body := nodeFileHeader + "\n[rpc]\nmax-open-connections = 111\n" + + t.Run("the file beats what the node's own file said", func(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, body, nil) + if got := ctx.Config.RPC.MaxOpenConnections; got != 111 { + t.Errorf("the node runs %d with 111 in sei.toml; the value resolved and never reached the "+ + "struct the node reads", got) + } + }) + + t.Run("the environment beats the file", func(t *testing.T) { + configtest.Isolate(t) + t.Setenv(registry.EnvName(key), "222") + ctx := bootWithNodeFile(t, body, nil) + if got := ctx.Config.RPC.MaxOpenConnections; got != 222 { + t.Errorf("the node runs %d with 222 in the environment and 111 in the file", got) + } + }) +} + +// TestTheDeliveryLeavesTheRootDirectoryAlone is what the root-directory exclusions buy. +func TestTheDeliveryLeavesTheRootDirectoryAlone(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[instrumentation]\nprometheus = true\n", nil) + + if !ctx.Config.Instrumentation.Prometheus { + t.Fatal("the delivery did not run, so this test would pass with the root directory declared") + } + if ctx.Config.RootDir == "" { + t.Error("the node's root directory is empty after the delivery") + } + if ctx.Config.PrivValidator.RootDir == "" { + t.Error("the signing key's root directory is empty after the delivery. A node that cannot find " + + "its key does not sign") + } +} diff --git a/config/registry/delivery.go b/config/registry/delivery.go new file mode 100644 index 0000000000..5967511b62 --- /dev/null +++ b/config/registry/delivery.go @@ -0,0 +1,113 @@ +package registry + +import ( + "fmt" + "sort" +) + +// decodedNotLookedUp holds the sections whose values reach their reader by a decode, and why. +var decodedNotLookedUp = map[string]string{} + +// DeclareDecodedNotLookedUp records that a section's values reach their reader by being decoded into a +// struct, rather than by a lookup in the source a node reads. +// +// Almost every section is read the other way: a reader asks for a key by name, so putting the resolved +// value into that source is the whole delivery. The sections this names are read once, by decoding a file +// into a struct before any of that happens, and a value put into the source afterwards reaches nothing. +// They need delivering a second way. +// +// The reason is required and names the struct the values are decoded into, which is what a reader has to +// check the claim against. A section declared with no reason is recorded as a defect rather than accepted, +// because the claim is the whole basis for delivering its keys differently. +func DeclareDecodedNotLookedUp(section, why string) { + mu.Lock() + defer mu.Unlock() + if why == "" { + defects = append(defects, Defect{Section: section, Err: fmt.Errorf( + "declared as decoded rather than looked up with no reason; the reason names the struct its " + + "values are decoded into, which is what a reader checks the claim against")}) + return + } + decodedNotLookedUp[section] = why +} + +// DecodedSections returns the sections whose values reach their reader by a decode, with the reason each +// gave, so a caller can report what it is about to do and to what. +func DecodedSections() map[string]string { + mu.RLock() + defer mu.RUnlock() + out := make(map[string]string, len(decodedNotLookedUp)) + for name, why := range decodedNotLookedUp { + out[name] = why + } + return out +} + +// SuppliedByDecodedSection splits a resolution into the values each decoded section has to deliver +// itself, keyed by section name and then by dotted key. +// +// Split per section rather than pooled, because a decode is all or nothing for whatever it is handed. One +// value a decoder refuses would otherwise cost every key in the file rather than the keys of the one +// section it appeared in, and an operator who fixed one setting and mistyped another would boot with +// neither applied and no way to tell which. +// +// The defaults are deliberately left out, and this is the difference between delivering a value and +// overwriting one. A section read by a lookup can be delivered whole, because its reader has nowhere else +// to get a value from. A section read by a decode already holds what its own file said, put there before +// any of this ran. Delivering a default over that replaces the operator's file with one nobody chose, on +// every boot, for every key their file does not mention. +// +// So a key that took its default is skipped and a key any other layer answered is delivered. That includes +// an operator writing the default value explicitly, because what is recorded is which layer answered and +// not whether the answer differs from the default: writing false where the file says true has to arrive. +func SuppliedByDecodedSection(resolved Resolved) map[string]map[string]any { + owning := DecodedSections() + + supplied := make(map[string]bool, len(resolved.Overrides)) + for _, key := range resolved.Overrides { + supplied[key] = true + } + + out := map[string]map[string]any{} + for _, section := range Sections() { + if _, owned := owning[section.Name]; !owned { + continue + } + for _, key := range section.Keys { + if !supplied[key] { + continue + } + if out[section.Name] == nil { + out[section.Name] = map[string]any{} + } + out[section.Name][key] = resolved.Values[key] + } + } + return out +} + +// UndeliveredSections returns the registered sections that named no delivery, sorted. +// +// A section reaches its reader one of two ways and the registry cannot tell which, so the answer is +// declared. A section that declares nothing is treated as read by a lookup, which is right for almost all +// of them and silently wrong for the rest: its keys resolve, install into the source, and change nothing +// the node runs. That is the failure this package exists to remove, so the set is reported and a caller +// linking every section holds it to what it expects. +func UndeliveredSections(expectDecoded map[string]bool) []string { + var out []string + for _, section := range Sections() { + if expectDecoded[section.Name] != DecodedNotLookedUp(section.Name) { + out = append(out, section.Name) + } + } + sort.Strings(out) + return out +} + +// DecodedNotLookedUp reports whether a section's values reach their reader by a decode. +func DecodedNotLookedUp(section string) bool { + mu.RLock() + defer mu.RUnlock() + _, ok := decodedNotLookedUp[section] + return ok +} diff --git a/config/registry/registry.go b/config/registry/registry.go index ba5a1a437f..0ee95965c2 100644 --- a/config/registry/registry.go +++ b/config/registry/registry.go @@ -130,11 +130,14 @@ func RegisterRootKeysExcluding(name string, prototype any, defaults func(Mode) a // record is the one path both registrations take. func record(name, prefix string, prototype any, defaults func(Mode) any, excluding []string) { - keys, err := deriveKeys(name, prefix, prototype) - var excluded []string + found, err := deriveKeys(name, prefix, prototype) + keys, excluded := found.keys, []string(nil) if err == nil { keys, excluded, err = withoutExcluded(prefix, keys, excluding) } + if err == nil { + err = refuseDeclaredInterfaces(keys, found.interfaces) + } mu.Lock() defer mu.Unlock() @@ -158,6 +161,37 @@ func record(name, prefix string, prototype any, defaults func(Mode) any, excludi } } +// refuseDeclaredInterfaces refuses a declared path whose field holds an interface. +// +// What a decoder writes into an interface depends on what the field already holds rather than on the +// field's type, so two structs of one type can accept and refuse the same written value. A caller that +// rehearses a decode into a copy to learn whether the real one will succeed gets an answer about the copy, +// and the two differ exactly where their existing values do. +// +// Checked against the declared paths and not the struct's fields, because a section may exclude such a +// field. An excluded path is not declared, and how a path nobody can write decodes is not a property worth +// refusing. +func refuseDeclaredInterfaces(keys, interfaces []string) error { + if len(interfaces) == 0 { + return nil + } + declared := make(map[string]bool, len(keys)) + for _, key := range keys { + declared[key] = true + } + var bad []string + for _, key := range interfaces { + if declared[key] { + bad = append(bad, key) + } + } + if len(bad) > 0 { + return fmt.Errorf("%v name fields holding an interface, so what a written value decodes to "+ + "depends on what the field already holds and not on the field's type", bad) + } + return nil +} + // withoutExcluded splits derived paths into the ones a section declares and the ones it does not. // // An exclusion is spelled relative to the section, so this is where it becomes the dotted key both walks @@ -282,49 +316,63 @@ func Keys() []string { // outside state-commit.flatkv.*. Ninety-two operator-facing keys reach their field only through a // spelling the tags do not produce, and a silent fallback is what made that invisible. Refusing to // guess is what keeps the tag authoritative. -func deriveKeys(name, prefix string, prototype any) ([]string, error) { +func deriveKeys(name, prefix string, prototype any) (derived, error) { if name == "" { - return nil, fmt.Errorf("section name is empty") + return derived{}, fmt.Errorf("section name is empty") } if name != strings.ToLower(name) { - return nil, fmt.Errorf("section name %q is not lower case; configuration sources "+ + return derived{}, fmt.Errorf("section name %q is not lower case; configuration sources "+ "enumerate lower-cased, so a key under it would never match a written one", name) } if bad, found := unaddressableChar(name); found { - return nil, fmt.Errorf("section name %q carries %q, and a section is one segment. A dotted name "+ + return derived{}, fmt.Errorf("section name %q carries %q, and a section is one segment. A dotted name "+ "declares keys inside another section's subtree, where the two sections' defaults land in "+ "one map and whichever renders last silently wins; a space cannot be written in an "+ "environment variable name at all", name, bad) } if prototype == nil { - return nil, fmt.Errorf("no struct") + return derived{}, fmt.Errorf("no struct") } t := reflect.TypeOf(prototype) for t.Kind() == reflect.Ptr { t = t.Elem() } if t.Kind() != reflect.Struct { - return nil, fmt.Errorf("%s is not a struct", t.Kind()) + return derived{}, fmt.Errorf("%s is not a struct", t.Kind()) } - var keys []string - if err := walk(t, prefix, &keys, map[reflect.Type]bool{}); err != nil { - return nil, err + var found derived + if err := walk(t, prefix, &found, map[reflect.Type]bool{}); err != nil { + return derived{}, err } + keys := found.keys if len(keys) == 0 { - return nil, fmt.Errorf("declares no keys") + return derived{}, fmt.Errorf("declares no keys") } sort.Strings(keys) + sort.Strings(found.interfaces) // A path two fields both produce leaves one of them unreachable, and which one is not // observable: the value walk writes them into one map. That is the unaddressable-key failure // this package exists to refuse, so it cannot be allowed to arrive through the package itself. for i := 1; i < len(keys); i++ { if keys[i] == keys[i-1] { - return nil, fmt.Errorf("two fields both declare %q, so one of them is unreachable and "+ + return derived{}, fmt.Errorf("two fields both declare %q, so one of them is unreachable and "+ "which one is not observable", keys[i]) } } - return keys, nil + found.keys = keys + return found, nil +} + +// derived is what one walk of a section's type collects. +// +// Two lists rather than one, because a path whose field holds an interface is not refused where it is +// found. A section may exclude it, and an excluded path is not declared, so nothing about how it decodes +// matters. The refusal belongs after the exclusions are known. +type derived struct { + keys []string + // interfaces are paths whose field holds an interface, sorted with the keys they appear among. + interfaces []string } // walk appends the dotted keys a struct declares under prefix. @@ -332,7 +380,7 @@ func deriveKeys(name, prefix string, prototype any) ([]string, error) { // open carries the struct types on the current path, so a self-referential one is refused rather than // recursed into. A stack overflow cannot be recovered into a Defect, so this is the one refusal that // has to happen before the recursion rather than after it. -func walk(t reflect.Type, prefix string, keys *[]string, open map[reflect.Type]bool) error { +func walk(t reflect.Type, prefix string, found *derived, open map[reflect.Type]bool) error { if open[t] { return fmt.Errorf("%s is %s, which contains itself; a key space derived from it has no end", prefix, t) @@ -373,7 +421,7 @@ func walk(t reflect.Type, prefix string, keys *[]string, open map[reflect.Type]b if ft.Kind() != reflect.Struct { return fmt.Errorf("%s.%s is squashed but is a %s, not a struct", prefix, f.Name, ft.Kind()) } - if err := walkSubtree(ft, prefix, join(prefix, f.Name), keys, open); err != nil { + if err := walkSubtree(ft, prefix, join(prefix, f.Name), found, open); err != nil { return err } continue @@ -381,12 +429,17 @@ func walk(t reflect.Type, prefix string, keys *[]string, open map[reflect.Type]b path := join(prefix, tag) if ft.Kind() == reflect.Struct && !isLeaf(ft) { - if err := walkSubtree(ft, path, join(prefix, f.Name), keys, open); err != nil { + if err := walkSubtree(ft, path, join(prefix, f.Name), found, open); err != nil { return err } continue } - *keys = append(*keys, path) + found.keys = append(found.keys, path) + // Recorded rather than refused. An excluded path is not declared, so how it decodes never + // matters; record decides, once the exclusions are known. + if ft.Kind() == reflect.Interface { + found.interfaces = append(found.interfaces, path) + } } return nil } @@ -404,12 +457,12 @@ func join(prefix, segment string) string { // A struct configuration cannot reach is a setting an operator writes into nothing. A defined type // over a leaf, an empty struct, and a struct whose every field is unexported all arrive here having // contributed nothing, and both walks agree about it, so no later check can see the loss. -func walkSubtree(t reflect.Type, path, field string, keys *[]string, open map[reflect.Type]bool) error { - before := len(*keys) - if err := walk(t, path, keys, open); err != nil { +func walkSubtree(t reflect.Type, path, field string, found *derived, open map[reflect.Type]bool) error { + before := len(found.keys) + if err := walk(t, path, found, open); err != nil { return err } - if len(*keys) == before { + if len(found.keys) == before { return fmt.Errorf("%s is a %s that declares no key, so configuration cannot reach it", field, t) } return nil diff --git a/config/registry/spec_test.go b/config/registry/spec_test.go index 76c3cdb10a..df3290c32b 100644 --- a/config/registry/spec_test.go +++ b/config/registry/spec_test.go @@ -1541,3 +1541,48 @@ func contains(keys []string, want string) bool { } return false } + +// TestADeclaredInterfaceFieldIsRefusedAndAnExcludedOneIsNot holds the one property a rehearsed decode +// rests on. +// +// What a decoder writes into an interface depends on what the field already holds, so two structs of the +// same type can accept and refuse the same written value. A caller that decodes into a copy first to learn +// whether the real decode will succeed gets an answer about the copy, and the two differ exactly where +// their existing values do. +// +// Both directions matter. A section that declares such a field is refused, because nothing downstream can +// reason about it. A section that excludes it registers, because a path nobody can write has no decode to +// reason about, and the fields that carry this shape in practice are settings a reader has removed. +func TestADeclaredInterfaceFieldIsRefusedAndAnExcludedOneIsNot(t *testing.T) { + type holdsAny struct { + Kept string `mapstructure:"kept"` + Removed *any `mapstructure:"removed"` + } + + registry.RegisterSection("interface_declared", &holdsAny{}, func(registry.Mode) any { + return holdsAny{Kept: "a"} + }) + if _, ok := registry.Lookup("interface_declared"); ok { + t.Error("a section declaring a field that holds an interface registered") + } + var named bool + for _, d := range registry.Defects() { + if d.Section == "interface_declared" && strings.Contains(d.Err.Error(), "holding an interface") { + named = true + } + } + if !named { + t.Errorf("no defect says the field holds an interface; Defects: %v", registry.Defects()) + } + + registry.RegisterSectionExcluding("interface_excluded", &holdsAny{}, func(registry.Mode) any { + return holdsAny{Kept: "a"} + }, "removed") + registered, ok := registry.Lookup("interface_excluded") + if !ok { + t.Fatalf("excluding the field did not make the section usable; Defects: %v", registry.Defects()) + } + if want := []string{"interface_excluded.kept"}; !reflect.DeepEqual(registered.Keys, want) { + t.Errorf("declares %v, want %v", registered.Keys, want) + } +} diff --git a/config/tendermintbase/tendermintbase.go b/config/tendermintbase/tendermintbase.go index 1c39637fdf..545e2ee254 100644 --- a/config/tendermintbase/tendermintbase.go +++ b/config/tendermintbase/tendermintbase.go @@ -85,7 +85,7 @@ var removedSettings = []string{ // a key here is a key that reader resolves rather than a second spelling of it. func init() { registry.RegisterSectionExcluding(P2PSectionName, &tmcfg.P2PConfig{}, p2pDefaults, - filledFromTheCommandLine, "max-outbound-connections") + notDeclaredBy(P2PSectionName, filledFromTheCommandLine, "max-outbound-connections")...) registry.RegisterSectionExcluding(RPCSectionName, &tmcfg.RPCConfig{}, rpcDefaults, filledFromTheCommandLine) registry.RegisterSectionExcluding(ConsensusSectionName, &tmcfg.ConsensusConfig{}, consensusDefaults, @@ -93,7 +93,7 @@ func init() { registry.RegisterSectionExcluding(MempoolSectionName, &tmcfg.MempoolConfig{}, mempoolDefaults, filledFromTheCommandLine) registry.RegisterSectionExcluding(StateSyncSectionName, &tmcfg.StateSyncConfig{}, stateSyncDefaults, - "rpc-servers") + notDeclaredBy(StateSyncSectionName, "rpc-servers")...) registry.RegisterSection(TxIndexSectionName, &tmcfg.TxIndexConfig{}, txIndexDefaults) registry.RegisterSection(InstrumentationSectionName, &tmcfg.InstrumentationConfig{}, instrumentationDefaults) @@ -102,7 +102,39 @@ func init() { registry.RegisterSection(SelfRemediationSectionName, &tmcfg.SelfRemediationConfig{}, selfRemediationDefaults) registry.RegisterRootKeysExcluding(RootSectionName, &nodeRootSchema{}, rootDefaults, - notWritableInThisFile...) + notDeclaredBy(RootSectionName, append(notWritableInThisFile, noLongerHasAnyEffect...)...)...) + + // Each of these reaches its reader by a decode rather than a lookup, so the boot delivers them a + // second way. Declared in the same loop that names them, so a section registered above and forgotten + // here would be a section this package does not list at all. + for _, name := range declaredSectionNames() { + registry.DeclareDecodedNotLookedUp(name, + "decoded into the node's own configuration struct by the boot's handler, which reads that "+ + "file once; nothing looks these keys up afterwards") + } +} + +// declaredSectionNames are the sections this package registers. +// +// One list, read by the registration that marks them all as decoded and by the test that holds the two +// against each other, so a section can not be registered without being delivered. +func declaredSectionNames() []string { + return []string{ + P2PSectionName, RPCSectionName, ConsensusSectionName, MempoolSectionName, StateSyncSectionName, + TxIndexSectionName, InstrumentationSectionName, PrivValidatorSectionName, + SelfRemediationSectionName, RootSectionName, + } +} + +// notDeclaredBy gathers every path one section leaves out, from the reasons that apply to it. +// +// Gathered rather than written out per registration, because a path left out for a reason that covers +// several sections is easy to add to one and forget in the others. +func notDeclaredBy(section string, also ...string) []string { + out := append([]string{}, also...) + out = append(out, writtenBySomethingOutsideTheBinary[section]...) + out = append(out, forTestsOnly[section]...) + return out } // forMode is the configuration the seid init command writes for a kind of node. @@ -129,6 +161,39 @@ func forMode(mode registry.Mode) *tmcfg.Config { // signing key does not start. const filledFromTheCommandLine = "home" +// writtenBySomethingOutsideTheBinary are paths a node's own file receives from elsewhere at boot. +// +// The rule that keeps these out is not that the binary fills them in, which is what the root directory +// does. It is that something else does, and this file cannot see it. The cluster's node controller resolves +// a peer set from live discovery and patches the addresses in; a node computes a trust height and hash from +// the chain tip each time it starts; a moniker is stamped per instance. A value declared here would be +// decoded over whichever of those already ran, and the file it came from would keep saying otherwise. +// +// The moniker has a second reason on its own. Its default is the host name of whatever machine resolved it, +// so no two machines agree on what this key declares. +var writtenBySomethingOutsideTheBinary = map[string][]string{ + P2PSectionName: {"external-address", "persistent-peers"}, + StateSyncSectionName: {"trust-height", "trust-hash"}, + RootSectionName: {"moniker"}, +} + +// noLongerHasAnyEffect are paths a reader keeps and ignores. +// +// The out-of-process application interface was removed, and the flag that carries this key is marked +// deprecated where it is declared, saying the flag is ignored. A declared key whose only effect is nothing +// is a setting an operator can spend an afternoon on. +var noLongerHasAnyEffect = []string{"abci"} + +// forTestsOnly are paths that exist to make a node misbehave. +// +// One makes every dial fail and one runs the node against a stub application. Neither has a use on a real +// network, and both are reachable from a file an operator edits by hand, on a node whose request surface +// faces the outside. +var forTestsOnly = map[string][]string{ + P2PSectionName: {"test-dial-fail"}, + RootSectionName: {"mock-app"}, +} + // The other path the peer-to-peer section does not declare. // // The outbound connection ceiling is a pointer the defaults leave unset, and unset is what selects the diff --git a/config/tendermintbase/tendermintbase_test.go b/config/tendermintbase/tendermintbase_test.go index f3904e701f..0cbc91d749 100644 --- a/config/tendermintbase/tendermintbase_test.go +++ b/config/tendermintbase/tendermintbase_test.go @@ -133,11 +133,11 @@ func TestTheDeclaredKeysAreTheOnesTheReaderDecodes(t *testing.T) { proto any exclude int }{ - {P2PSectionName, &tmcfg.P2PConfig{}, 2}, + {P2PSectionName, &tmcfg.P2PConfig{}, 5}, {RPCSectionName, &tmcfg.RPCConfig{}, 1}, {ConsensusSectionName, &tmcfg.ConsensusConfig{}, len(removedSettings) + 1}, {MempoolSectionName, &tmcfg.MempoolConfig{}, 1}, - {StateSyncSectionName, &tmcfg.StateSyncConfig{}, 1}, + {StateSyncSectionName, &tmcfg.StateSyncConfig{}, 3}, {TxIndexSectionName, &tmcfg.TxIndexConfig{}, 0}, {InstrumentationSectionName, &tmcfg.InstrumentationConfig{}, 0}, {PrivValidatorSectionName, &tmcfg.PrivValidatorConfig{}, 1}, @@ -362,8 +362,8 @@ func TestTheStateSyncExclusionIsThePathWithNoDefault(t *testing.T) { if !ok { t.Fatalf("%s is not registered; Defects: %v", StateSyncSectionName, registry.Defects()) } - if want := []string{StateSyncSectionName + ".rpc-servers"}; !reflect.DeepEqual(registered.Excluded, want) { - t.Fatalf("excluded is %v, want %v", registered.Excluded, want) + if !slices.Contains(registered.Excluded, StateSyncSectionName+".rpc-servers") { + t.Fatalf("excluded is %v and does not name the snapshot servers", registered.Excluded) } if got := tmcfg.DefaultStateSyncConfig().RPCServers; len(got) != 0 { t.Errorf("the node now defaults the snapshot servers to %v, so it states a value and the key "+ @@ -471,8 +471,10 @@ func TestTheRootPathsLeftOutAreTheOnesTheFileAlreadyStates(t *testing.T) { "for something already settled", key, why) } } - if len(registered.Excluded) != 2 { - t.Errorf("the root section excludes %v and two paths were expected", registered.Excluded) + for _, key := range []string{"abci", "mock-app", "moniker"} { + if declared[key] { + t.Errorf("%q is declared at the root and it is left out for a reason of its own", key) + } } } @@ -521,3 +523,72 @@ func TestNoSectionDeclaresTheRootDirectory(t *testing.T) { } } } + +// TestEverySectionThisPackageRegistersIsDeliveredByADecode is the partition, held from this side. +// +// A section reaches its reader one of two ways and the registry cannot tell which, so it is declared. A +// section that declares nothing is treated as read by a lookup, which is right for almost every section +// elsewhere and silently wrong for every one of these: its keys would resolve, install into the source a +// node reads, and change nothing the node runs. That is the exact failure this key space exists to remove. +// +// So the set is held both ways. Every section this package registers has to be declared decoded, and no +// section it does not register may be, because a section marked decoded whose values nothing decodes is +// undelivered in the other direction. +func TestEverySectionThisPackageRegistersIsDeliveredByADecode(t *testing.T) { + mine := map[string]bool{} + for _, name := range declaredSectionNames() { + mine[name] = true + if !registry.DecodedNotLookedUp(name) { + t.Errorf("%s is registered here and is not declared as delivered by a decode, so its keys "+ + "would be installed into a source nothing reads them from", name) + } + } + for name, why := range registry.DecodedSections() { + if !mine[name] { + continue + } + if why == "" { + t.Errorf("%s is declared decoded with no reason naming what decodes it", name) + } + } + if wrong := registry.UndeliveredSections(mine); len(wrong) > 0 { + t.Errorf("these sections disagree with what this package expects of them: %v. A section is "+ + "either read by a lookup or read by a decode, and one delivered the other way changes "+ + "nothing a node runs", wrong) + } +} + +// TestThePathsWrittenFromOutsideTheBinaryAreNotDeclared covers the exclusions with no local cause. +// +// Nothing in this repository fills these in, which is why declaring them looks harmless from here. A +// cluster controller resolves a peer set from live discovery and patches the addresses into the node's own +// file; a node computes a trust height and hash from the chain tip each time it starts; a moniker is +// stamped per instance. A declared value would be decoded over whichever of those already ran, and the +// only record of the change would be in memory. +func TestThePathsWrittenFromOutsideTheBinaryAreNotDeclared(t *testing.T) { + for section, paths := range writtenBySomethingOutsideTheBinary { + registered, ok := registry.Lookup(section) + if !ok { + t.Errorf("%s is not registered; Defects: %v", section, registry.Defects()) + continue + } + declared := map[string]bool{} + for _, key := range registered.Keys { + declared[key] = true + } + for _, rel := range paths { + key := rel + if registered.Prefix != "" { + key = registered.Prefix + "." + rel + } + if declared[key] { + t.Errorf("%s is declared, and something outside this binary writes it into the node's "+ + "own file at boot. A value from here would be applied over that, in memory only", key) + } + if !slices.Contains(registered.Excluded, key) { + t.Errorf("%s is neither declared nor excluded, so the exclusion naming it covers nothing "+ + "and the registration should have been refused", key) + } + } + } +} From 54a8195c5df4feb0cca378cfe7bce95c0dc9d18a Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 24 Aug 2026 13:27:13 -0700 Subject: [PATCH 3/7] feat(config): close four gaps around the node configuration delivery A typed flag now reaches the key it carries. A flag's name and that key are not always spelled the same: the node's own flags separate words with an underscore where the tag they decode through uses a hyphen. Compared as strings such a flag looked like a name nothing declares, so it was dropped and the file won over the command line, which inverts the one channel an operator reaches for during an incident. Matched through the environment spelling instead, where a dot, a hyphen and an underscore are one character. The registry already refuses to let two declared keys share that spelling, so a flag matches at most one key. A length of time written as a plain number is refused. The file format has no way to say how long something is, so a length of time is written as text with a unit. A plain number decodes cleanly and reads as nanoseconds, so sixty means sixty billionths of a second and the node starts. Nothing later objects, because nothing later can tell. The delivery is the only place that can: the resolution sees a number and a key, and only the struct says the key is a length of time. The two writers of this file are measured against each other. A declared value is what the init command writes for a kind of node, and that command is not the only thing here that writes the file: a node started without one gets it generated by the boot. They disagree on four keys, and the record says which and what a node gets instead. Two were predicted and two were not. The profiling address does not diverge after all, because the boot assigns it and a bound flag's empty default overwrites it inside the same function. The transaction indexer does, because the boot's writer applies none of the rules that vary a setting by kind of node, so a validator that let it generate this file indexes every transaction. A command answers, without starting a node, whether this binary can use a sei.toml. A boot may not refuse a file, so every value it cannot use is a report on a node that has already restarted, and a fleet rolling a change forward reads it after the change is on every node. The same questions have exact answers beforehand: the file, the binary and the environment are the whole input. Run it first and a mistyped value costs a failed check rather than a restart. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/configmanager/check.go | 141 ++++++++++++++++++ cmd/seid/cmd/configmanager/check_test.go | 97 ++++++++++++ cmd/seid/cmd/configmanager/install.go | 42 +++++- cmd/seid/cmd/configmanager/tendermint.go | 9 ++ cmd/seid/cmd/configmanager/tendermint_copy.go | 74 +++++++++ cmd/seid/cmd/node_agreement_test.go | 125 ++++++++++++++++ cmd/seid/cmd/node_delivery_test.go | 76 ++++++++++ cmd/seid/cmd/root.go | 14 ++ 8 files changed, 573 insertions(+), 5 deletions(-) create mode 100644 cmd/seid/cmd/configmanager/check.go create mode 100644 cmd/seid/cmd/configmanager/check_test.go create mode 100644 cmd/seid/cmd/node_agreement_test.go diff --git a/cmd/seid/cmd/configmanager/check.go b/cmd/seid/cmd/configmanager/check.go new file mode 100644 index 0000000000..b83897b193 --- /dev/null +++ b/cmd/seid/cmd/configmanager/check.go @@ -0,0 +1,141 @@ +package configmanager + +import ( + "fmt" + "io" + "os" + "sort" + "strings" + + "github.com/spf13/cobra" + "github.com/spf13/viper" + + "github.com/sei-protocol/sei-chain/config/registry" + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// CheckCmd answers, without starting a node, whether this binary can use a sei.toml. +// +// A boot may not refuse a file. A node that stopped because one line was mistyped is worse than a node +// running the value it ran yesterday, so every failure at boot is a report and the node keeps going. That +// makes the report the only signal, and a fleet rolling a configuration change forward reads it after the +// change is already on every node. +// +// The same questions have exact answers before then. The file, the binary and the environment are all the +// input, so a refusal is deterministic: the same file against the same binary gives the same answer here as +// it will at boot. This asks them where an answer costs a failed check rather than a restart. +func CheckCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "check", + Short: "Report whether this binary can use the node's sei.toml", + Long: "Resolves the node's sei.toml the way a boot resolves it and reports every value this " + + "binary would refuse, without starting anything. Exits non-zero if there is one.\n\n" + + "A boot cannot refuse a file, so it applies what it can and reports the rest. Running this " + + "first is how a mistyped value costs a failed check rather than a restart.", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + problems, found, err := checkSeiToml(cmd) + if err != nil { + return err + } + if !found { + report(cmd.OutOrStdout(), "this node has no sei.toml, so every key reads as it always "+ + "has and there is nothing here to be wrong") + return nil + } + out := cmd.OutOrStdout() + for _, line := range problems { + report(out, line) + } + if len(problems) > 0 { + return fmt.Errorf("%d problem(s); a boot would apply what it could and report the rest", + len(problems)) + } + report(out, "every value this file supplies is one this binary can use") + return nil + }, + } + return cmd +} + +// report writes one line of the answer. +// +// A failed write is dropped rather than returned. Where this runs the answer is the exit status, and a +// caller that cannot read the report still gets that. +func report(out io.Writer, line string) { _, _ = fmt.Fprintln(out, line) } + +// checkSeiToml resolves the node's file and returns what a boot would refuse, in the order it would. +// +// The absence of a file is not a problem to report. A node without one reads exactly as it always has, so +// there is nothing here that could be wrong. +func checkSeiToml(cmd *cobra.Command) (problems []string, found bool, err error) { + home, err := resolveHomeDir(cmd) + if err != nil { + return nil, false, fmt.Errorf("resolve the home directory: %w", err) + } + file, ok := readSeiTomlAt(home) + if !ok { + return nil, false, nil + } + mode, err := file.Mode() + if err != nil { + return []string{fmt.Sprintf("sei.toml records no usable node mode: %v", err)}, true, nil + } + written, err := file.Values() + if err != nil { + return []string{fmt.Sprintf("sei.toml cannot be read: %v", err)}, true, nil + } + + resolved, err := registry.Resolve(registry.Mode(mode), registry.Sources{ + File: written, + LookupEnv: os.LookupEnv, + Flags: flagValues(TypedFlags(cmd)), + }) + if err != nil { + return []string{fmt.Sprintf("this node's configuration cannot be resolved: %v", err)}, true, nil + } + + for _, key := range resolved.Unknown { + problems = append(problems, fmt.Sprintf("%s: sei.toml writes this and no section declares it, "+ + "so it has no effect", key)) + } + problems = append(problems, whatADecodeWouldRefuse(resolved)...) + return problems, true, nil +} + +// whatADecodeWouldRefuse rehearses each decoded section the way the boot's delivery does. +// +// Rehearsed against a fresh configuration rather than a running node's, because there is no node here. That +// is a weaker target than the delivery uses, and the difference is the point: a value this accepts may still +// be refused at boot if the field it lands on holds something this cannot see. It is why this reports what +// it can answer and the boot still reports what it finds. +func whatADecodeWouldRefuse(resolved registry.Resolved) []string { + bySection := registry.SuppliedByDecodedSection(resolved) + var problems []string + for _, name := range sortedSectionNames(bySection) { + values := bySection[name] + base := tmcfg.DefaultConfig() + + if bad := refuseBareNumbersForDurations(base, values); len(bad) > 0 { + problems = append(problems, fmt.Sprintf("[%s]: %s is a length of time written as a plain "+ + "number, which reads as nanoseconds", name, strings.Join(bad, "; "))) + continue + } + + source := viper.New() + for key, value := range values { + source.Set(key, value) + } + candidate, err := copyNodeConfig(base) + if err != nil { + problems = append(problems, fmt.Sprintf("[%s]: cannot be rehearsed: %v", name, err)) + continue + } + if err := source.Unmarshal(candidate); err != nil { + problems = append(problems, fmt.Sprintf("[%s]: %v, so none of this section would apply "+ + "(keys: %s)", name, err, strings.Join(sortedKeys(values), ","))) + } + } + sort.Strings(problems) + return problems +} diff --git a/cmd/seid/cmd/configmanager/check_test.go b/cmd/seid/cmd/configmanager/check_test.go new file mode 100644 index 0000000000..1b6ebdf741 --- /dev/null +++ b/cmd/seid/cmd/configmanager/check_test.go @@ -0,0 +1,97 @@ +package configmanager + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/sei-protocol/sei-chain/sei-cosmos/client/flags" +) + +// runCheck runs the command against a home holding the given sei.toml, and returns what it printed and +// whether it failed. +func runCheck(t *testing.T, body string) (string, error) { + t.Helper() + home := t.TempDir() + if err := os.MkdirAll(filepath.Join(home, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + if body != "" { + if err := os.WriteFile(filepath.Join(home, "config", seiTomlName), []byte(body), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + } + + cmd := CheckCmd() + cmd.Flags().String(flags.FlagHome, home, "") + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + err := cmd.Execute() + return out.String(), err +} + +// TestTheCheckFailsOnWhatABootWouldRefuse is the point of the command. +// +// A boot may not refuse a file, so every value it cannot use is a report on a node that has already +// restarted. The same questions have exact answers beforehand, and this is where an answer costs a failed +// check instead. +func TestTheCheckFailsOnWhatABootWouldRefuse(t *testing.T) { + const header = "schema_version = 1\nnode_mode = \"validator\"\n" + + t.Run("a file this binary can use passes", func(t *testing.T) { + out, err := runCheck(t, header+"\n[mempool]\nttl-duration = \"60s\"\nsize = 4321\n") + if err != nil { + t.Errorf("a usable file was refused: %v\n%s", err, out) + } + }) + + t.Run("no file at all is not a problem", func(t *testing.T) { + out, err := runCheck(t, "") + if err != nil { + t.Errorf("a node with no sei.toml was refused: %v", err) + } + if !strings.Contains(out, "no sei.toml") { + t.Errorf("the report does not say the file is absent, so a missing file reads as a clean "+ + "one:\n%s", out) + } + }) + + t.Run("a length of time written as a plain number fails", func(t *testing.T) { + out, err := runCheck(t, header+"\n[mempool]\nttl-duration = 60\n") + if err == nil { + t.Errorf("a plain number in a length of time passed:\n%s", out) + } + if !strings.Contains(out, "nanoseconds") { + t.Errorf("the report does not say what is wrong with it:\n%s", out) + } + }) + + t.Run("a value the decode refuses fails", func(t *testing.T) { + out, err := runCheck(t, header+"\n[instrumentation]\nmax-open-connections = \"not a number\"\n") + if err == nil { + t.Errorf("a value no decode accepts passed:\n%s", out) + } + }) + + t.Run("a key no section declares is reported", func(t *testing.T) { + out, err := runCheck(t, header+"\n[mempool]\nnot-a-key = 1\n") + if err == nil { + t.Errorf("a key nothing declares passed:\n%s", out) + } + if !strings.Contains(out, "no effect") { + t.Errorf("the report does not say the key has no effect:\n%s", out) + } + }) + + t.Run("a mode this binary does not know fails", func(t *testing.T) { + out, err := runCheck(t, "schema_version = 1\nnode_mode = \"sentry\"\n") + if err == nil { + t.Errorf("a mode nothing declares passed:\n%s", out) + } + }) +} diff --git a/cmd/seid/cmd/configmanager/install.go b/cmd/seid/cmd/configmanager/install.go index 0985e9dad4..2e937d485d 100644 --- a/cmd/seid/cmd/configmanager/install.go +++ b/cmd/seid/cmd/configmanager/install.go @@ -167,11 +167,20 @@ func readSeiToml(cmd *cobra.Command, log *slog.Logger) (*seitoml.File, bool) { log.Warn("cannot resolve the home directory; every key reads as it always has", "err", err) return nil, false } - path := filepath.Join(home, "config", seiTomlName) + file, ok := readSeiTomlAt(home) + if !ok { + log.Debug("no readable sei.toml; every key reads as it always has", "home", home) + } + return file, ok +} - file, err := seitoml.Load(path) +// readSeiTomlAt loads the sei.toml under a home directory. +// +// Separate from the reporting, so the check command can ask the same question without a logger and get the +// same answer for the same file. +func readSeiTomlAt(home string) (*seitoml.File, bool) { + file, err := seitoml.Load(filepath.Join(home, "config", seiTomlName)) if err != nil { - log.Debug("no readable sei.toml; every key reads as it always has", "path", path, "err", err) return nil, false } return file, true @@ -218,14 +227,37 @@ func TypedFlags(cmd *cobra.Command) map[string]string { return out } -// flagValues renders a snapshot of typed flags as a configuration source. +// flagValues renders a snapshot of typed flags as a configuration source, under the keys the sections +// declare. +// +// A flag's name and the key it carries are not always spelled the same. The node's own flags separate words +// with an underscore where the tag they decode through uses a hyphen, so a flag named for a declared key is +// not equal to it, and comparing the two by string leaves an operator's typed flag looking like a name +// nothing declares. It is then dropped, and the file wins over the command line: the one channel somebody +// reaches for during an incident is the one that loses. +// +// Matched through the environment spelling, where a dot and a hyphen and an underscore are all the same +// character. That is an equivalence the registry already refuses to let two declared keys share, so a flag +// matches at most one key and no ambiguity is possible here. +// +// A flag matching no declared key is left under its own name. Most of the flags a node starts with were +// never configuration keys, and the resolution reports the unmatched ones from the file alone. func flagValues(typed map[string]string) map[string]any { if len(typed) == 0 { return nil } + byEnvName := map[string]string{} + for _, key := range registry.Keys() { + byEnvName[registry.EnvName(key)] = key + } + out := make(map[string]any, len(typed)) for name, value := range typed { - out[name] = value + key := name + if declared, ok := byEnvName[registry.EnvName(name)]; ok { + key = declared + } + out[key] = value } return out } diff --git a/cmd/seid/cmd/configmanager/tendermint.go b/cmd/seid/cmd/configmanager/tendermint.go index e38854c401..2b7f36a571 100644 --- a/cmd/seid/cmd/configmanager/tendermint.go +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -59,6 +59,15 @@ func deliverOneSection(ctx *server.Context, name string, values map[string]any, source.Set(key, value) } + // Refused before the decode, because a plain number where a length of time belongs decodes cleanly + // and means nanoseconds. Nothing after this can tell that apart from a value somebody meant. + if bad := refuseBareNumbersForDurations(ctx.Config, values); len(bad) > 0 { + log.Error("a length of time in this section is written as a plain number, which reads as "+ + "nanoseconds; none of the section is applied and every one of its keys reads as it always has", + "section", name, "written", strings.Join(bad, "; ")) + return + } + candidate, err := copyNodeConfig(ctx.Config) if err != nil { log.Error("cannot copy this node's configuration, so nothing can be delivered into it without "+ diff --git a/cmd/seid/cmd/configmanager/tendermint_copy.go b/cmd/seid/cmd/configmanager/tendermint_copy.go index ac51a1a49c..142f9f66e7 100644 --- a/cmd/seid/cmd/configmanager/tendermint_copy.go +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -5,6 +5,7 @@ import ( "reflect" "sort" "strings" + "time" "github.com/go-viper/mapstructure/v2" @@ -149,6 +150,79 @@ func flatten(prefix string, in map[string]any, out map[string]any) { } } +// DescribeForTest reads what a node's configuration holds for each key, as text. +// +// Exported for the test that measures the two generators against each other, which lives beside the boot +// because only a boot produces a generated file. +func DescribeForTest(cfg *tmcfg.Config, keys []string) map[string]string { return describe(cfg, keys) } + +// refuseBareNumbersForDurations reports the keys written as a plain number where the field is a length of +// time, with what an operator should have written. +// +// The file format has no way to say how long something is, so a length of time is written as text with a +// unit. A plain number is accepted by the decoder and read as nanoseconds, which is the shortest unit there +// is: sixty means sixty billionths of a second, the node starts, and the setting is off by a factor of a +// billion. Nothing later objects, because the value decoded cleanly. +// +// So the delivery refuses it and says what to write instead. This is the one place the check can happen: the +// resolution sees a number and a key, and only the struct says the key is a length of time. +func refuseBareNumbersForDurations(cfg *tmcfg.Config, values map[string]any) []string { + durations := durationKeys(reflect.TypeOf(*cfg), "") + var bad []string + for key, value := range values { + if !durations[key] { + continue + } + switch value.(type) { + case int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float32, float64: + bad = append(bad, fmt.Sprintf("%s = %v (write a unit, as \"%vs\")", key, value, value)) + } + } + sort.Strings(bad) + return bad +} + +// durationKeys returns the dotted keys whose field is a length of time. +func durationKeys(t reflect.Type, prefix string) map[string]bool { + out := map[string]bool{} + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + if f.PkgPath != "" { + continue + } + tag, ok := f.Tag.Lookup("mapstructure") + if !ok { + continue + } + name := strings.Split(tag, ",")[0] + squash := strings.Contains(tag, ",squash") + ft := f.Type + for ft.Kind() == reflect.Pointer { + ft = ft.Elem() + } + path := name + if prefix != "" && name != "" { + path = prefix + "." + name + } + if squash { + for key := range durationKeys(ft, prefix) { + out[key] = true + } + continue + } + if ft == reflect.TypeOf(time.Duration(0)) { + out[path] = true + continue + } + if ft.Kind() == reflect.Struct { + for key := range durationKeys(ft, path) { + out[key] = true + } + } + } + return out +} + // referencePathsIn returns every path in a type that a copy has to detach, for the test that holds // detachSections to the type it copies. func referencePathsIn(t reflect.Type, path string, seen map[reflect.Type]bool) []string { diff --git a/cmd/seid/cmd/node_agreement_test.go b/cmd/seid/cmd/node_agreement_test.go new file mode 100644 index 0000000000..612bdb1696 --- /dev/null +++ b/cmd/seid/cmd/node_agreement_test.go @@ -0,0 +1,125 @@ +package cmd + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "testing" + + "go.opentelemetry.io/otel/sdk/trace" + + "github.com/sei-protocol/sei-chain/cmd/seid/cmd/configmanager" + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// whatANodeWithNoFileRuns is what each diverging key resolves to for a node that has no configuration file +// of its own, and lets the boot make one. +// +// A declared value is what the init command writes for a kind of node. That command is not the only thing +// in this binary that writes this file: a node started without one gets it generated by the boot instead, +// and the two do not agree. These are the keys where they differ, with what the second one produces. +// +// Held as text because the two sides carry different Go types for the same key often enough that comparing +// values would be comparing shapes. What matters is which keys disagree and what a node gets instead. +var whatANodeWithNoFileRuns = map[string]string{ + "p2p.recv-rate": "5120000", + "p2p.send-rate": "5120000", + "proxy-app": "", + "tx-index.indexer": "[kv]", +} + +// whyEachMatters says what a node gets, and it is the reason each row is measured rather than described. +var whyEachMatters = map[string]string{ + "p2p.recv-rate": "the ceiling on what one connection may pull, four times lower than the declared " + + "value, so a node adopting a file generated by the other writer would have it raised", + "p2p.send-rate": "the same ceiling in the other direction", + "proxy-app": "the address of the application the node talks to, which the boot assigns and then a " + + "bound flag's empty default overwrites inside the same function", + "tx-index.indexer": "whether the node indexes transactions. The boot's writer applies none of the " + + "rules that vary a setting by kind of node, so a validator that let it generate this file " + + "indexes every transaction, which is what the init command writes for a node that serves queries " + + "and the opposite of what it writes for a validator", +} + +// TestTheDivergencesFromAGeneratedFileAreTheRecordedOnes measures what a comment would only claim. +// +// The init command and the boot both generate this file and they disagree, so a declared value is what one +// of them writes and not simply what a generated file carries. Which keys those are is measured here rather +// than described, because a key that starts diverging fails and so does one that stops. +// +// Driven through a real boot with no configuration file of its own and no sei.toml, so nothing is delivered +// and what the node holds is purely what the boot generated. +func TestTheDivergencesFromAGeneratedFileAreTheRecordedOnes(t *testing.T) { + configtest.Isolate(t) + generated := whatTheBootGenerates(t) + + resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + + var measured []string + for key, got := range generated { + declared, declares := resolved.Values[key] + if !declares { + continue + } + if fmt.Sprint(declared) == got { + if _, listed := whatANodeWithNoFileRuns[key]; listed { + t.Errorf("%s no longer diverges, both sides being %v. Take it off the record, so the "+ + "record stays the set of keys the two generators state differently", key, declared) + } + continue + } + measured = append(measured, key) + want, listed := whatANodeWithNoFileRuns[key] + switch { + case !listed: + t.Errorf("%s is declared as %v and a node that let the boot generate its file runs %q, and "+ + "nothing records that. %s", key, declared, got, whyEachMatters[key]) + case want != got: + t.Errorf("%s is recorded as running %q and runs %q", key, want, got) + } + } + + sort.Strings(measured) + if len(measured) != len(whatANodeWithNoFileRuns) { + t.Errorf("measured %d divergences and %d are recorded: %v", + len(measured), len(whatANodeWithNoFileRuns), measured) + } +} + +// whatTheBootGenerates returns what a node holds for every declared key of the decoded sections, having +// started with no configuration file of its own. +func whatTheBootGenerates(t *testing.T) map[string]string { + t.Helper() + home := configtest.NewHome(t) + if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home.Root); err != nil { + t.Fatalf("set --home: %v", err) + } + ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("the boot was refused: %v", err) + } + if ctx.Config == nil { + t.Fatal("the boot produced no node configuration") + } + + var keys []string + for name := range registry.DecodedSections() { + section, ok := registry.Lookup(name) + if !ok { + continue + } + keys = append(keys, section.Keys...) + } + return configmanager.DescribeForTest(ctx.Config, keys) +} diff --git a/cmd/seid/cmd/node_delivery_test.go b/cmd/seid/cmd/node_delivery_test.go index fae4911c87..0f870ee592 100644 --- a/cmd/seid/cmd/node_delivery_test.go +++ b/cmd/seid/cmd/node_delivery_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "testing" + "time" "go.opentelemetry.io/otel/sdk/trace" @@ -181,3 +182,78 @@ func TestTheDeliveryLeavesTheRootDirectoryAlone(t *testing.T) { "its key does not sign") } } + +// TestATypedFlagReachesTheKeyItCarries covers the one channel an operator reaches for under pressure. +// +// A flag's name and the key it carries are not always spelled the same: the node's own flags separate words +// with an underscore where the tag they decode through uses a hyphen. Compared as strings such a flag looks +// like a name nothing declares, so it is dropped, and the file wins over the command line. +// +// Driven with the file and the flag disagreeing, and read off the struct the node runs from, because this +// key belongs to a section delivered by a decode. +func TestATypedFlagReachesTheKeyItCarries(t *testing.T) { + const key = "p2p.unconditional-peer-ids" + const flag = "p2p.unconditional_peer_ids" + configtest.Isolate(t) + + home := configtest.NewHome(t) + dir := filepath.Join(home.Root, "config") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatal(err) + } + if err := tmcfg.WriteConfigFile(home.Root, tmcfg.DefaultConfig()); err != nil { + t.Fatalf("render the node's configuration file: %v", err) + } + body := nodeFileHeader + "\n[p2p]\nunconditional-peer-ids = \"from-the-file\"\n" + if err := os.WriteFile(filepath.Join(dir, "sei.toml"), []byte(body), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + + cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home.Root); err != nil { + t.Fatal(err) + } + if err := cmd.Flags().Set(flag, "from-the-command-line"); err != nil { + t.Skipf("--%s is not on this command, so nothing here can carry the key: %v", flag, err) + } + ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("the boot was refused: %v", err) + } + + if got := ctx.Config.P2P.UnconditionalPeerIDs; got != "from-the-command-line" { + t.Errorf("the node runs %q with --%s typed and a different value in the file, want the typed "+ + "one. The flag's name and the key it carries are spelled differently, so comparing them as "+ + "strings drops the flag and the file wins over the command line", got, flag) + } +} + +// TestALengthOfTimeWrittenAsAPlainNumberIsRefused covers a value that decodes cleanly and is wrong by a +// factor of a billion. +// +// The file format has no way to say how long something is, so a length of time is written as text with a +// unit. A plain number decodes as nanoseconds, the shortest unit there is, so sixty means sixty billionths +// of a second and the node starts. Nothing later objects, because nothing later can tell. +func TestALengthOfTimeWrittenAsAPlainNumberIsRefused(t *testing.T) { + configtest.Isolate(t) + was := tmcfg.DefaultConfig().Mempool.TTLDuration + + t.Run("a plain number is refused and the section is left alone", func(t *testing.T) { + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nttl-duration = 60\nsize = 4321\n", nil) + if got := ctx.Config.Mempool.TTLDuration; got != was { + t.Errorf("the node runs a time-to-live of %v after a plain 60 was written, want the %v it "+ + "had. Sixty read as nanoseconds is sixty billionths of a second", got, was) + } + if got := ctx.Config.Mempool.Size; got == 4321 { + t.Error("the value beside the refused one was applied, so the section was published in part") + } + }) + + t.Run("the same number with a unit is applied", func(t *testing.T) { + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nttl-duration = \"60s\"\n", nil) + if got := ctx.Config.Mempool.TTLDuration; got != 60*time.Second { + t.Errorf("the node runs %v with \"60s\" written, want 60s. Refusing a plain number must not "+ + "refuse the written form an operator is being asked for", got) + } + }) +} diff --git a/cmd/seid/cmd/root.go b/cmd/seid/cmd/root.go index b4cfacac90..cd3e95b2e7 100644 --- a/cmd/seid/cmd/root.go +++ b/cmd/seid/cmd/root.go @@ -145,6 +145,7 @@ func initRootCmd( tmcli.NewCompletionCmd(rootCmd, true), debugCmd, config.Cmd(), + seiConfigCmd(), tools.ToolCmd(), SnapshotCmd(), LogLevelCmd(), @@ -477,3 +478,16 @@ supply_enabled = {{ .LightInvariance.SupplyEnabled }} return customAppTemplate, customAppConfig } + +// seiConfigCmd groups the commands that answer questions about a node's sei.toml. +// +// Its own group rather than a subcommand of the existing configuration command, which reads and writes the +// files this one is about rather than the file that replaces them. +func seiConfigCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "sei-config", + Short: "Inspect the node's sei.toml", + } + cmd.AddCommand(configmanager.CheckCmd()) + return cmd +} From 2b3919fabd8cda2a5cd9e93ffa1fc2606751dd40 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 24 Aug 2026 14:19:03 -0700 Subject: [PATCH 4/7] fix(config): repair what a peer review measured, and pin each repair Two reviews ran this code rather than read it. Seven things it got wrong. A log level resolved here beat one exported in the environment. The logger reads a variable of its own before any of this runs, under a name that is not the one this key answers to, and the boot's handler steps aside when it is set: a flag beats it and a file does not. Applying regardless put the file above it, so an operator who exported a level and adopted this file found the level ignored. A typed flag still wins, which is the order that was already there. This manager's own reporting is held at a level its reports survive. One level covers every logger in the process and an operator writes it, so a fleet that runs its nodes quiet silenced every report: what was applied, what moved, what was refused. Measured, a value was delivered and nothing was said about it. The floor goes on after anything that sets a level, because both setters reach every logger. A length of time written as zero is applied. It was refused as a plain number, and the reason a plain number is refused is that it means nanoseconds and is out by a factor of a billion. At zero there is no factor: several of these settings document zero as the way to turn them off and three declare it. An operator writing it lost every other key in the section. A negative number is refused where the setting cannot hold one. Minus one is how an operator says no limit in most software they have used, and the decoder wraps it to the largest value the field has, so the ceiling on connected peers stopped bounding anything and a window in seconds became centuries. The report of a key nothing declares sat one line above the level, so a file that raised the level to see its own mistakes still could not. The reader that names what moved swallowed a failure, so a section it could not read at all was reported as a section whose values all matched. A bounded report could drop any key, in alphabetical order. The bound guarded against a file filling a boot log, which a list bounded by its own section cannot do. Two comments an operator reads named the wrong mechanism, and one row of the divergence record gave the wrong reason for a real difference. A disagreement about what kind of node this is is now reported. Two files state that under different names, and nothing compared them, so a node resolving a validator's values while running as a query-serving node read correctly in every report about it. Reported rather than corrected: what kind of node this is gets decided when it is provisioned. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/configmanager/check.go | 2 +- cmd/seid/cmd/configmanager/check_test.go | 72 +++++++++++ cmd/seid/cmd/configmanager/configmanager.go | 29 +++++ cmd/seid/cmd/configmanager/install.go | 60 ++++++++- cmd/seid/cmd/configmanager/tendermint.go | 52 ++++++-- cmd/seid/cmd/configmanager/tendermint_copy.go | 121 ++++++++++++++---- cmd/seid/cmd/node_agreement_test.go | 12 +- cmd/seid/cmd/node_delivery_test.go | 60 +++++++++ config/registry/registry.go | 1 + 9 files changed, 362 insertions(+), 47 deletions(-) diff --git a/cmd/seid/cmd/configmanager/check.go b/cmd/seid/cmd/configmanager/check.go index b83897b193..1168a50a3c 100644 --- a/cmd/seid/cmd/configmanager/check.go +++ b/cmd/seid/cmd/configmanager/check.go @@ -116,7 +116,7 @@ func whatADecodeWouldRefuse(resolved registry.Resolved) []string { values := bySection[name] base := tmcfg.DefaultConfig() - if bad := refuseBareNumbersForDurations(base, values); len(bad) > 0 { + if bad := refuseWhatDecodesToSomethingElse(base, values); len(bad) > 0 { problems = append(problems, fmt.Sprintf("[%s]: %s is a length of time written as a plain "+ "number, which reads as nanoseconds", name, strings.Join(bad, "; "))) continue diff --git a/cmd/seid/cmd/configmanager/check_test.go b/cmd/seid/cmd/configmanager/check_test.go index 1b6ebdf741..ffde32ee2a 100644 --- a/cmd/seid/cmd/configmanager/check_test.go +++ b/cmd/seid/cmd/configmanager/check_test.go @@ -2,12 +2,18 @@ package configmanager import ( "bytes" + "context" + "log/slog" "os" "path/filepath" "strings" "testing" "github.com/sei-protocol/sei-chain/sei-cosmos/client/flags" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + serverconfig "github.com/sei-protocol/sei-chain/sei-cosmos/server/config" + "github.com/sei-protocol/sei-chain/testutil/configtest" + "go.opentelemetry.io/otel/sdk/trace" ) // runCheck runs the command against a home holding the given sei.toml, and returns what it printed and @@ -95,3 +101,69 @@ func TestTheCheckFailsOnWhatABootWouldRefuse(t *testing.T) { } }) } + +// TestADisagreementAboutTheKindOfNodeIsFound covers a fact two files state under different names. +// +// sei.toml records the kind of node at its top and every value resolved through this manager is the answer +// for that kind. The node's own configuration file states it again in a key of its own, and that one is what +// the node runs as. Nothing here declares the second on purpose, so the two can be written to disagree, and +// a node that resolves a validator's values while running as a full node reads correctly in every report +// about it. +func TestADisagreementAboutTheKindOfNodeIsFound(t *testing.T) { + for _, tc := range []struct { + recorded, running string + disagree bool + why string + }{ + {"validator", "validator", false, "the same kind is not a disagreement"}, + {"validator", "full", true, "a validator that runs as a query-serving node serves queries"}, + {"full", "validator", true, "a node resolved for queries that runs as a validator holds a key"}, + {"seed", "full", true, "a seed exists to serve peers and would be serving queries"}, + {"archive", "full", false, "the kind that keeps every version has no name of its own in that " + + "file, so the command that writes it writes this one"}, + {"archive", "validator", true, "an archive that runs as a validator is a disagreement"}, + } { + if got := modesDisagree(tc.recorded, tc.running); got != tc.disagree { + t.Errorf("sei.toml %q against a node running %q reports disagree=%v, want %v: %s", + tc.recorded, tc.running, got, tc.disagree, tc.why) + } + } +} + +// TestApplyReportsADisagreementAboutTheKindOfNode drives the real Apply, so the wiring is what is asserted. +// +// The test beside this one holds the decision, which a comparison never reached would still pass. This one +// gives the two files different kinds of node and looks for the report, so removing the call fails here. +func TestApplyReportsADisagreementAboutTheKindOfNode(t *testing.T) { + configtest.Isolate(t) + root := writeMinimalHome(t, "mode = \"full\"\n", "") + if err := os.WriteFile(filepath.Join(root, "config", seiTomlName), + []byte("schema_version = 1\nnode_mode = \"validator\"\n"), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + + cmd := server.StartCmd(nil, "/foobar", []trace.TracerProviderOption{}) + if err := cmd.Flags().Set(flags.FlagHome, root); err != nil { + t.Fatalf("set --home: %v", err) + } + serverCtx := &server.Context{} + cmd.SetContext(context.WithValue(context.Background(), server.ServerContextKey, serverCtx)) + + capture := &capturingHandler{} + mgr := SeiConfigManager{logger: slog.New(capture)} + if err := mgr.Apply(cmd, serverconfig.DefaultConfigTemplate, serverconfig.DefaultConfig()); err != nil { + t.Fatalf("the fixture is meant to boot, so this is the fixture: %v", err) + } + + var found bool + for _, r := range capture.records { + if strings.Contains(r.Message, "one kind of node") { + found = true + } + } + if !found { + t.Error("sei.toml said validator, the node's own file said full, and nothing reported it. A " + + "node resolving a validator's values while running as a query-serving node reads correctly " + + "in every other report about it") + } +} diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index 3e53e317e6..cd1b62376b 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -21,6 +21,13 @@ import ( var logger = seilog.NewLogger("cmd", "seid", "configmanager") +// loggerName is the name the logger above is registered under, and ownReportingFloor is the level its +// reports are held at. +const ( + loggerName = "cmd/seid/configmanager" + ownReportingFloor = slog.LevelInfo +) + // EnvVar gates which configuration manager seid uses. const EnvVar = "SEI_CONFIG_MANAGER" @@ -55,6 +62,27 @@ type SeiConfigManager struct { } // log returns the logger to report through, and never returns nil. +// keepOwnReportingVisible holds this package's own logger at a level its reports survive. +// +// Called after anything that may have set a level, and it is called more than once for that reason: the +// handler sets one, and a level this manager resolves sets another. Both set every logger in the process, so +// a floor applied before either is simply overwritten. +// +// The handler this manager re-enters sets one level across every logger in the process, from a key an +// operator writes, and a fleet that runs its nodes quiet sets it above the level these reports use. Every +// outcome here is a report: what was applied, what moved, what was refused and what had no effect. Silenced, +// the manager becomes a component that changes what a node runs and says nothing about it, and the file +// stops being something an operator can reason about from the node itself. +// +// So this one logger keeps a floor, and only this one. Raising the level for the rest of the process is +// still the operator's to choose. +func keepOwnReportingVisible() { + if seilog.SetLevel(loggerName, ownReportingFloor) == 0 { + // Nothing to hold, which happens when a caller supplied a logger of its own. + return + } +} + func (m SeiConfigManager) log() *slog.Logger { if m.logger != nil { return m.logger @@ -87,6 +115,7 @@ func (m SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate stri out := validateAdvisory(cmd) err := server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) + keepOwnReportingVisible() reportAdvisory(m.log(), out) if err != nil { return err diff --git a/cmd/seid/cmd/configmanager/install.go b/cmd/seid/cmd/configmanager/install.go index 2e937d485d..f39ee310d1 100644 --- a/cmd/seid/cmd/configmanager/install.go +++ b/cmd/seid/cmd/configmanager/install.go @@ -1,6 +1,7 @@ package configmanager import ( + "context" "log/slog" "os" "path/filepath" @@ -69,12 +70,16 @@ func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logg "mode", mode, "err", err) return } - reportWhatTheFileDidNotReach(resolved, log) - - // First, because every failure below is a log line and a refusal is reported at a level an operator + // First, because every report below is a log line and a refusal is reported at a level an operator // may have raised the threshold above. Doing this after would mean the one setting somebody changes // in order to see a refusal is the setting a refusal suppresses. - applyResolvedLogLevel(resolved, log) + applyResolvedLogLevel(resolved, typed, log) + + // After the level, so a file that raises it can report its own mistakes. A key nothing declares is the + // most common thing an operator gets wrong and the only signal they have for it. + reportWhatTheFileDidNotReach(resolved, log) + + reportWhatTheFileSaysTheNodeIs(ctx, mode, log) // The sections a reader looks up key by key, and the sections a reader decodes whole. Two deliveries, // because putting a value into the source is no delivery at all for the second kind: their file is @@ -156,6 +161,53 @@ func reportWhatTheFileDidNotReach(resolved registry.Resolved, log *slog.Logger) } } +// reportWhatTheFileSaysTheNodeIs names a disagreement about what kind of node this is. +// +// Two files state that, under different names. sei.toml records it at the top, and every value resolved +// through this manager is the answer for that kind of node. The node's own configuration file states it +// again in a key of its own, and that one is what the node runs as. +// +// This manager does not declare the second, on purpose: two keys for one fact can be written to disagree, +// and then a resolution answers for one while the node is the other. Not declaring it means nothing here +// can change it, which leaves the disagreement possible and unreported. A node whose file says validator +// while it runs as a full node resolves a validator's values and serves queries, and every report about it +// reads correctly. +// +// So it is compared and reported. Reported rather than corrected, because what kind of node this is gets +// decided when it is provisioned, and a configuration manager is not the thing that should change it. +func reportWhatTheFileSaysTheNodeIs(ctx *server.Context, mode string, log *slog.Logger) { + if ctx == nil || ctx.Config == nil || ctx.Config.Mode == "" { + return + } + running := ctx.Config.Mode + if !modesDisagree(mode, running) { + return + } + log.Error("sei.toml says this is one kind of node and the node's own configuration file says another; "+ + "every value resolved here is the answer for the first and the node runs as the second", + "sei.toml", mode, "running", running) +} + +// modesDisagree reports whether the kind of node sei.toml records and the kind the node runs as are +// different kinds. +// +// One pairing is not a disagreement. The kind that keeps every version of history has no name of its own in +// the node's own configuration file, so the command that writes that file writes the query-serving name +// instead, and the difference between them lives in settings the node's own file does not carry. +func modesDisagree(recorded, running string) bool { + if recorded == running { + return false + } + return !(recorded == string(registry.ModeArchive) && running == string(registry.ModeFull)) +} + +// OwnReportingEnabledForTest reports whether this package's logger would emit at the level its reports use. +// +// Exported for the test that holds the floor, because the thing under test is a level and not a message. +func OwnReportingEnabledForTest() bool { + return logger.Enabled(context.Background(), ownReportingFloor) +} + // readSeiToml loads the node's sei.toml, reporting the ordinary absence quietly. // // A node that has not generated one is the expected state while sections are still moving, so that is not diff --git a/cmd/seid/cmd/configmanager/tendermint.go b/cmd/seid/cmd/configmanager/tendermint.go index 2b7f36a571..f606e5f629 100644 --- a/cmd/seid/cmd/configmanager/tendermint.go +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -1,8 +1,10 @@ package configmanager import ( + "cmp" "fmt" "log/slog" + "os" "sort" "strings" @@ -61,7 +63,7 @@ func deliverOneSection(ctx *server.Context, name string, values map[string]any, // Refused before the decode, because a plain number where a length of time belongs decodes cleanly // and means nanoseconds. Nothing after this can tell that apart from a value somebody meant. - if bad := refuseBareNumbersForDurations(ctx.Config, values); len(bad) > 0 { + if bad := refuseWhatDecodesToSomethingElse(ctx.Config, values); len(bad) > 0 { log.Error("a length of time in this section is written as a plain number, which reads as "+ "nanoseconds; none of the section is applied and every one of its keys reads as it always has", "section", name, "written", strings.Join(bad, "; ")) @@ -75,7 +77,7 @@ func deliverOneSection(ctx *server.Context, name string, values map[string]any, "section", name, "keys", strings.Join(keys, ","), "err", err) return } - before := describe(ctx.Config, keys) + before, readErr := describe(ctx.Config, keys) if err := source.Unmarshal(candidate); err != nil { log.Error("a written value in this section was refused, so none of the section is applied and "+ @@ -85,7 +87,16 @@ func deliverOneSection(ctx *server.Context, name string, values map[string]any, } *ctx.Config = *candidate - reportWhatMoved(name, keys, before, describe(ctx.Config, keys), log) + after, afterErr := describe(ctx.Config, keys) + if readErr != nil || afterErr != nil { + // Reported rather than compared. Two unreadable sides look identical, so comparing them would + // say every value matched, which is a statement about nothing produced by reading nothing. + log.Error("this section was applied and what moved cannot be read, so nothing here says which "+ + "settings now differ from the node's own file", "section", name, + "keys", strings.Join(keys, ","), "err", cmp.Or(readErr, afterErr)) + return + } + reportWhatMoved(name, keys, before, after, log) } // copyNodeConfig returns a configuration that holds what this one holds and shares nothing with it. @@ -129,16 +140,7 @@ func reportWhatMoved(name string, keys []string, before, after map[string]string return } log.Info("this section's settings now differ from what the node's own configuration file says", - "section", name, "changed", strings.Join(capDelivered(moved), "; ")) -} - -// capDelivered bounds a report so one file cannot fill a node's log at boot. -func capDelivered(lines []string) []string { - const most = 20 - if len(lines) <= most { - return lines - } - return append(lines[:most:most], fmt.Sprintf("and %d more", len(lines)-most)) + "section", name, "changed", strings.Join(moved, "; ")) } // sortedKeys returns a map's keys in a fixed order, so a log line does not vary between runs. @@ -164,6 +166,12 @@ func sortedSectionNames(bySection map[string]map[string]any) []string { // logLevelKey is the one delivered setting the struct is not the end of. const logLevelKey = "log-level" +// loggerOwnVariable is the environment variable the logger itself reads when it starts. +// +// Not the variable this key answers to in the resolution, which carries the binary's own prefix. Two names +// for one setting, and the older one is read before any of this runs. +const loggerOwnVariable = "SEI_LOG_LEVEL" + // applyResolvedLogLevel hands a resolved log level to the logger, which the struct alone does not reach. // // The boot's handler reads the level off the struct and sets it before any of this runs, so a value that @@ -177,7 +185,7 @@ const logLevelKey = "log-level" // // Which value arrives is already decided: the resolution ranks a flag over the environment over the file. // A level that cannot be read is reported and skipped, and the node keeps the level it had. -func applyResolvedLogLevel(resolved registry.Resolved, log *slog.Logger) { +func applyResolvedLogLevel(resolved registry.Resolved, typed map[string]string, log *slog.Logger) { supplied := false for _, key := range resolved.Overrides { if key == logLevelKey { @@ -187,6 +195,20 @@ func applyResolvedLogLevel(resolved registry.Resolved, log *slog.Logger) { if !supplied { return } + + // The logger reads a variable of its own at start-up, under a name that is not the one this key + // answers to, and the boot's own handler steps aside when it is set: a flag beats it and a file does + // not. Applying here regardless would put the file above it, so an operator who exported a level and + // then adopted this file would find the level they exported ignored. A typed flag still wins, which is + // the order that was already there. + if _, fromFlag := flagValues(typed)[logLevelKey]; !fromFlag { + if os.Getenv(loggerOwnVariable) != "" { + log.Info("a log level is set in the environment under the logger's own variable, which the "+ + "node already applied; the level this file supplies is not used", + "variable", loggerOwnVariable, "ignored", resolved.Values[logLevelKey]) + return + } + } text, isText := resolved.Values[logLevelKey].(string) if !isText { log.Error("the resolved log level is not text; the node keeps the level it already had", @@ -200,5 +222,7 @@ func applyResolvedLogLevel(resolved registry.Resolved, log *slog.Logger) { return } seilog.SetDefaultLevel(level, true) + // That set every logger in the process, this one included, so the floor goes back on. + keepOwnReportingVisible() log.Info("resolved log level applied", "level", text) } diff --git a/cmd/seid/cmd/configmanager/tendermint_copy.go b/cmd/seid/cmd/configmanager/tendermint_copy.go index 142f9f66e7..e878e80649 100644 --- a/cmd/seid/cmd/configmanager/tendermint_copy.go +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -116,14 +116,14 @@ func join(path, field string) string { // Read through the same tags the decode writes through, so a key names the same field in both directions. // Held as text because what a report needs is whether two values differ and what they are, and comparing // the shapes a decode produced against the shapes a struct holds would answer a different question. -func describe(cfg *tmcfg.Config, keys []string) map[string]string { +func describe(cfg *tmcfg.Config, keys []string) (map[string]string, error) { out := map[string]string{} if cfg == nil { - return out + return out, fmt.Errorf("no configuration to read") } var nested map[string]any if err := mapstructure.Decode(cfg, &nested); err != nil { - return out + return out, err } flat := map[string]any{} flatten("", nested, flat) @@ -132,7 +132,7 @@ func describe(cfg *tmcfg.Config, keys []string) map[string]string { out[key] = fmt.Sprint(v) } } - return out + return out, nil } // flatten turns a nested map into one keyed by dotted path. @@ -154,36 +154,113 @@ func flatten(prefix string, in map[string]any, out map[string]any) { // // Exported for the test that measures the two generators against each other, which lives beside the boot // because only a boot produces a generated file. -func DescribeForTest(cfg *tmcfg.Config, keys []string) map[string]string { return describe(cfg, keys) } +func DescribeForTest(cfg *tmcfg.Config, keys []string) map[string]string { + out, _ := describe(cfg, keys) + return out +} -// refuseBareNumbersForDurations reports the keys written as a plain number where the field is a length of -// time, with what an operator should have written. +// refuseWhatDecodesToSomethingElse reports written values the decoder accepts and turns into something the +// operator did not mean, with what they should have written. +// +// Two shapes, and both decode cleanly, which is why nothing later objects. +// +// A length of time has no form of its own in the file, so it is written as text with a unit. A plain number +// is read as nanoseconds, the shortest unit there is, so sixty means sixty billionths of a second. Zero is +// the exception and is allowed: nanoseconds and seconds are the same at zero, and zero is the documented way +// to turn several of these settings off. // -// The file format has no way to say how long something is, so a length of time is written as text with a -// unit. A plain number is accepted by the decoder and read as nanoseconds, which is the shortest unit there -// is: sixty means sixty billionths of a second, the node starts, and the setting is off by a factor of a -// billion. Nothing later objects, because the value decoded cleanly. +// A negative number written where the field cannot hold one wraps to the largest value that field has. So +// minus one, which is how an operator says "no limit" in most software they have used, becomes a limit of +// eighteen million million million: the ceiling on connected peers stops bounding anything, and a window +// measured in seconds becomes six centuries. // -// So the delivery refuses it and says what to write instead. This is the one place the check can happen: the -// resolution sees a number and a key, and only the struct says the key is a length of time. -func refuseBareNumbersForDurations(cfg *tmcfg.Config, values map[string]any) []string { - durations := durationKeys(reflect.TypeOf(*cfg), "") +// This is the one place either can be caught. The resolution sees a number and a key; only the struct says +// what the key is. +func refuseWhatDecodesToSomethingElse(cfg *tmcfg.Config, values map[string]any) []string { + t := reflect.TypeOf(*cfg) + durations := durationKeys(t, "") + unsigned := unsignedKeys(t, "") + var bad []string for key, value := range values { - if !durations[key] { + n, numeric := asNumber(value) + if !numeric { continue } - switch value.(type) { - case int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float32, float64: - bad = append(bad, fmt.Sprintf("%s = %v (write a unit, as \"%vs\")", key, value, value)) + switch { + case durations[key] && n != 0: + bad = append(bad, fmt.Sprintf("%s = %v is a length of time, so write a unit, as %q", + key, value, fmt.Sprintf("%vs", value))) + case unsigned[key] && n < 0: + bad = append(bad, fmt.Sprintf("%s = %v cannot be negative, and decodes to the largest value "+ + "this setting can hold rather than to no limit", key, value)) } } sort.Strings(bad) return bad } +// asNumber reports whether a written value arrived as a number, and what it was. +// +// Held as a float because what the checks above ask is whether it is zero and whether it is negative, and +// every numeric shape a file, a variable or a flag can carry answers both. +func asNumber(value any) (float64, bool) { + switch v := value.(type) { + case int: + return float64(v), true + case int8: + return float64(v), true + case int16: + return float64(v), true + case int32: + return float64(v), true + case int64: + return float64(v), true + case uint: + return float64(v), true + case uint8: + return float64(v), true + case uint16: + return float64(v), true + case uint32: + return float64(v), true + case uint64: + return float64(v), true + case float32: + return float64(v), true + case float64: + return v, true + } + return 0, false +} + +// unsignedKeys returns the dotted keys whose field cannot hold a negative number. +func unsignedKeys(t reflect.Type, prefix string) map[string]bool { + return keysWhoseFieldIs(t, prefix, func(ft reflect.Type) bool { + switch ft.Kind() { + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + return true + } + return false + }) +} + // durationKeys returns the dotted keys whose field is a length of time. +// +// Matched by conversion rather than by identity, so a named type over the same underlying number is a length +// of time too. func durationKeys(t reflect.Type, prefix string) map[string]bool { + durationType := reflect.TypeOf(time.Duration(0)) + return keysWhoseFieldIs(t, prefix, func(ft reflect.Type) bool { + return ft.Kind() == reflect.Int64 && ft.ConvertibleTo(durationType) && ft != reflect.TypeOf(int64(0)) + }) +} + +// keysWhoseFieldIs returns the dotted keys whose field answers a question about its type. +// +// One walk for every such question, over the same tag rules the declaration derives keys by, so a key found +// here is a key that can be written. +func keysWhoseFieldIs(t reflect.Type, prefix string, is func(reflect.Type) bool) map[string]bool { out := map[string]bool{} for i := 0; i < t.NumField(); i++ { f := t.Field(i) @@ -205,17 +282,17 @@ func durationKeys(t reflect.Type, prefix string) map[string]bool { path = prefix + "." + name } if squash { - for key := range durationKeys(ft, prefix) { + for key := range keysWhoseFieldIs(ft, prefix, is) { out[key] = true } continue } - if ft == reflect.TypeOf(time.Duration(0)) { + if is(ft) { out[path] = true continue } if ft.Kind() == reflect.Struct { - for key := range durationKeys(ft, path) { + for key := range keysWhoseFieldIs(ft, path, is) { out[key] = true } } diff --git a/cmd/seid/cmd/node_agreement_test.go b/cmd/seid/cmd/node_agreement_test.go index 612bdb1696..55f48bbc37 100644 --- a/cmd/seid/cmd/node_agreement_test.go +++ b/cmd/seid/cmd/node_agreement_test.go @@ -36,12 +36,12 @@ var whyEachMatters = map[string]string{ "p2p.recv-rate": "the ceiling on what one connection may pull, four times lower than the declared " + "value, so a node adopting a file generated by the other writer would have it raised", "p2p.send-rate": "the same ceiling in the other direction", - "proxy-app": "the address of the application the node talks to, which the boot assigns and then a " + - "bound flag's empty default overwrites inside the same function", - "tx-index.indexer": "whether the node indexes transactions. The boot's writer applies none of the " + - "rules that vary a setting by kind of node, so a validator that let it generate this file " + - "indexes every transaction, which is what the init command writes for a node that serves queries " + - "and the opposite of what it writes for a validator", + "proxy-app": "a setting no reader reads, whose declared value comes from the node's own defaults and " + + "which a bound flag's empty default overwrites during the boot's own decode", + "tx-index.indexer": "whether the node indexes transactions. The boot's writer produces a file for a " + + "node that serves queries, because the kind it defaults to is that one, so this row is what a " + + "resolution for a validator states against a file generated for something else. The pair in that " + + "file agrees with itself; what disagrees is the kind of node each side is describing", } // TestTheDivergencesFromAGeneratedFileAreTheRecordedOnes measures what a comment would only claim. diff --git a/cmd/seid/cmd/node_delivery_test.go b/cmd/seid/cmd/node_delivery_test.go index 0f870ee592..12da9b63a7 100644 --- a/cmd/seid/cmd/node_delivery_test.go +++ b/cmd/seid/cmd/node_delivery_test.go @@ -249,6 +249,21 @@ func TestALengthOfTimeWrittenAsAPlainNumberIsRefused(t *testing.T) { } }) + t.Run("zero is applied, because zero is the same in every unit", func(t *testing.T) { + // Several of these settings document zero as the way to turn them off, and three declare it as + // their value, so an operator writing it is doing the ordinary thing. Refusing it would cost them + // every other key in the section. + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[rpc]\ntimeout-read-header = 0\nmax-open-connections = 41\n", nil) + if got := ctx.Config.RPC.TimeoutReadHeader; got != 0 { + t.Errorf("the node runs a read-header timeout of %v with 0 written, want 0", got) + } + if got := ctx.Config.RPC.MaxOpenConnections; got != 41 { + t.Errorf("max-open-connections is %d, so writing a zero length of time cost the section. "+ + "Zero nanoseconds and zero seconds are the same value, so there is nothing to refuse", got) + } + }) + t.Run("the same number with a unit is applied", func(t *testing.T) { ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nttl-duration = \"60s\"\n", nil) if got := ctx.Config.Mempool.TTLDuration; got != 60*time.Second { @@ -257,3 +272,48 @@ func TestALengthOfTimeWrittenAsAPlainNumberIsRefused(t *testing.T) { } }) } + +// TestTheReportSurvivesAQuietNode is what a fleet running its nodes quiet needs. +// +// One log level covers every logger in the process and an operator writes it. A fleet that sets it above the +// level these reports use turns this manager into a component that changes what a node runs and says nothing +// about it, and the report is the only place the node's own file and the running settings can be told apart. +// +// The level is what is asserted rather than a message, because a message can be absent for reasons that have +// nothing to do with whether it would have been printed. +func TestTheReportSurvivesAQuietNode(t *testing.T) { + configtest.Isolate(t) + + ctx := bootWithNodeFile(t, nodeFileHeader+"log-level = \"error\"\n\n[mempool]\nsize = 4321\n", nil) + if got := ctx.Config.Mempool.Size; got != 4321 { + t.Fatalf("the value was not delivered (%d), so this test cannot show a report being kept", got) + } + if !configmanager.OwnReportingEnabledForTest() { + t.Error("a node whose file sets the level to error delivered a value and this manager's own " + + "reporting is switched off. The report is the only signal it has, and the node's own file " + + "and its running settings can be told apart nowhere else") + } +} + +// TestANegativeNumberWhereTheSettingCannotHoldOneIsRefused covers the habit of writing minus one for +// "no limit". +// +// Most software an operator has used takes minus one that way. Here the field cannot hold a negative number, +// so the decoder wraps it to the largest value the field has: the ceiling on connected peers stops bounding +// anything, and a window measured in seconds becomes centuries. The value decodes cleanly, so nothing later +// objects. +func TestANegativeNumberWhereTheSettingCannotHoldOneIsRefused(t *testing.T) { + configtest.Isolate(t) + was := tmcfg.DefaultConfig().P2P.MaxConnections + + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[p2p]\nmax-connections = -1\nsend-rate = 1234567\n", nil) + + if got := ctx.Config.P2P.MaxConnections; got != was { + t.Errorf("the node allows %d connected peers after minus one was written, want the %d it had. "+ + "Minus one wraps to the largest value this setting can hold, which is no bound at all", got, was) + } + if got := ctx.Config.P2P.SendRate; got == 1234567 { + t.Error("the value beside the refused one was applied, so the section was published in part") + } +} diff --git a/config/registry/registry.go b/config/registry/registry.go index 0ee95965c2..b34f4095de 100644 --- a/config/registry/registry.go +++ b/config/registry/registry.go @@ -576,6 +576,7 @@ func isLeaf(t reflect.Type) bool { // another's declared set. func Reset() { mu.Lock() + decodedNotLookedUp = map[string]string{} defer mu.Unlock() sections = map[string]Section{} defects = nil From 0faead85e23b34a0dd1aa84bf2aa849a3c4e3479 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 24 Aug 2026 14:19:23 -0700 Subject: [PATCH 5/7] style(config): state the mode comparison without a negated conjunction Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/configmanager/install.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/seid/cmd/configmanager/install.go b/cmd/seid/cmd/configmanager/install.go index f39ee310d1..8639a8a270 100644 --- a/cmd/seid/cmd/configmanager/install.go +++ b/cmd/seid/cmd/configmanager/install.go @@ -198,7 +198,7 @@ func modesDisagree(recorded, running string) bool { if recorded == running { return false } - return !(recorded == string(registry.ModeArchive) && running == string(registry.ModeFull)) + return recorded != string(registry.ModeArchive) || running != string(registry.ModeFull) } // OwnReportingEnabledForTest reports whether this package's logger would emit at the level its reports use. From 798f11a2201fa5358114f6a748c219c0a48c9d3b Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 24 Aug 2026 18:16:26 -0700 Subject: [PATCH 6/7] test(config): name the rule both deliveries depend on A declared value is what a provisioning command writes for a kind of node, not what any particular node runs, so delivering one would replace a setting an operator never mentioned on every boot. Both deliveries avoid that by narrowing to the keys a source supplied, and each does it in its own function. That makes it a rule three call sites remember rather than one a single function enforces. Until the narrowing has one home this is the guard: boot with a file supplying one key and assert nothing else moved, in either delivery, for every kind of node. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/node_delivery_test.go | 61 ++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/cmd/seid/cmd/node_delivery_test.go b/cmd/seid/cmd/node_delivery_test.go index 12da9b63a7..abae1b238c 100644 --- a/cmd/seid/cmd/node_delivery_test.go +++ b/cmd/seid/cmd/node_delivery_test.go @@ -1,8 +1,10 @@ package cmd import ( + "fmt" "os" "path/filepath" + "sort" "testing" "time" @@ -317,3 +319,62 @@ func TestANegativeNumberWhereTheSettingCannotHoldOneIsRefused(t *testing.T) { t.Error("the value beside the refused one was applied, so the section was published in part") } } + +// TestNoDeliveryCarriesADeclaredDefault is the one rule both deliveries depend on, named. +// +// A resolution answers for every declared key, and a declared value is what a provisioning command writes +// for a kind of node rather than what any particular node runs. Delivering one would replace a setting an +// operator never mentioned, on every boot, for every key their file omits. Both deliveries avoid that by +// narrowing to the keys a source supplied, and each does it in its own function. +// +// That makes it a rule three call sites remember rather than one a single function enforces, which is the +// shape this repository's own guidance says to guard. Until the narrowing has one home, this is the guard: +// it boots with a file that supplies one key and asserts that nothing else moved anywhere, across both +// deliveries and every mode. +func TestNoDeliveryCarriesADeclaredDefault(t *testing.T) { + for _, mode := range registry.Modes() { + t.Run(string(mode), func(t *testing.T) { + configtest.Isolate(t) + + // What the node holds before any file supplies anything. + bare := bootWithNodeFile(t, "schema_version = 1\nnode_mode = \""+string(mode)+"\"\n", nil) + keys := everyDeclaredKey() + before := configmanager.DescribeForTest(bare.Config, keys) + beforeSource := map[string]string{} + for _, key := range keys { + beforeSource[key] = fmt.Sprint(bare.Viper.Get(key)) + } + + // The same node, with a file supplying exactly one key. + after := bootWithNodeFile(t, "schema_version = 1\nnode_mode = \""+string(mode)+"\"\n"+ + "\n[mempool]\nsize = 4321\n", nil) + if got := after.Config.Mempool.Size; got != 4321 { + t.Fatalf("the one supplied key arrived as %d, so nothing was delivered and this test "+ + "would pass for a delivery that does nothing", got) + } + + afterDescribed := configmanager.DescribeForTest(after.Config, keys) + for _, key := range keys { + if key == "mempool.size" { + continue + } + if afterDescribed[key] != before[key] { + t.Errorf("%s reads %q after a file that supplies only mempool.size, and %q before. A "+ + "declared default was delivered over a setting nobody wrote", + key, afterDescribed[key], before[key]) + } + if got := fmt.Sprint(after.Viper.Get(key)); got != beforeSource[key] { + t.Errorf("%s reads %q in the source and %q before it. A declared default was "+ + "installed for a key nobody wrote", key, got, beforeSource[key]) + } + } + }) + } +} + +// everyDeclaredKey returns every key any registered section declares, sorted. +func everyDeclaredKey() []string { + keys := registry.Keys() + sort.Strings(keys) + return keys +} From 42e67116810b42d2c41c1cfbcfe8b25831fdf4e5 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Tue, 25 Aug 2026 10:17:02 -0700 Subject: [PATCH 7/7] refactor(config): name the other divergence record for what it holds The same rename as its sibling. This record holds what the boot's own writer produces for a node that arrives without a configuration file, so it says that, and the explanations beside it are reasoning. Named for that writer rather than for the manager, because the two records measure different things: one is what the manager this replaces resolves, this is what the second of the binary's two writers generates. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/node_agreement_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/cmd/seid/cmd/node_agreement_test.go b/cmd/seid/cmd/node_agreement_test.go index 55f48bbc37..58245a13db 100644 --- a/cmd/seid/cmd/node_agreement_test.go +++ b/cmd/seid/cmd/node_agreement_test.go @@ -15,8 +15,8 @@ import ( "github.com/sei-protocol/sei-chain/testutil/configtest" ) -// whatANodeWithNoFileRuns is what each diverging key resolves to for a node that has no configuration file -// of its own, and lets the boot make one. +// bootGeneratedDefaults is what each diverging key resolves to for a node that has no configuration file of +// its own and lets the boot generate one. // // A declared value is what the init command writes for a kind of node. That command is not the only thing // in this binary that writes this file: a node started without one gets it generated by the boot instead, @@ -24,15 +24,15 @@ import ( // // Held as text because the two sides carry different Go types for the same key often enough that comparing // values would be comparing shapes. What matters is which keys disagree and what a node gets instead. -var whatANodeWithNoFileRuns = map[string]string{ +var bootGeneratedDefaults = map[string]string{ "p2p.recv-rate": "5120000", "p2p.send-rate": "5120000", "proxy-app": "", "tx-index.indexer": "[kv]", } -// whyEachMatters says what a node gets, and it is the reason each row is measured rather than described. -var whyEachMatters = map[string]string{ +// reasoning says what a node gets, and it is why each row is measured rather than described. +var reasoning = map[string]string{ "p2p.recv-rate": "the ceiling on what one connection may pull, four times lower than the declared " + "value, so a node adopting a file generated by the other writer would have it raised", "p2p.send-rate": "the same ceiling in the other direction", @@ -68,27 +68,27 @@ func TestTheDivergencesFromAGeneratedFileAreTheRecordedOnes(t *testing.T) { continue } if fmt.Sprint(declared) == got { - if _, listed := whatANodeWithNoFileRuns[key]; listed { + if _, listed := bootGeneratedDefaults[key]; listed { t.Errorf("%s no longer diverges, both sides being %v. Take it off the record, so the "+ "record stays the set of keys the two generators state differently", key, declared) } continue } measured = append(measured, key) - want, listed := whatANodeWithNoFileRuns[key] + want, listed := bootGeneratedDefaults[key] switch { case !listed: t.Errorf("%s is declared as %v and a node that let the boot generate its file runs %q, and "+ - "nothing records that. %s", key, declared, got, whyEachMatters[key]) + "nothing records that. %s", key, declared, got, reasoning[key]) case want != got: t.Errorf("%s is recorded as running %q and runs %q", key, want, got) } } sort.Strings(measured) - if len(measured) != len(whatANodeWithNoFileRuns) { + if len(measured) != len(bootGeneratedDefaults) { t.Errorf("measured %d divergences and %d are recorded: %v", - len(measured), len(whatANodeWithNoFileRuns), measured) + len(measured), len(bootGeneratedDefaults), measured) } }