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/check.go b/cmd/seid/cmd/configmanager/check.go new file mode 100644 index 0000000000..1168a50a3c --- /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 := 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 + } + + 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..ffde32ee2a --- /dev/null +++ b/cmd/seid/cmd/configmanager/check_test.go @@ -0,0 +1,169 @@ +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 +// 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) + } + }) +} + +// 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 c6e6eec499..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 @@ -80,10 +108,23 @@ 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) + keepOwnReportingVisible() 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..8639a8a270 --- /dev/null +++ b/cmd/seid/cmd/configmanager/install.go @@ -0,0 +1,315 @@ +package configmanager + +import ( + "context" + "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" + + // 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. +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 + } + // 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, 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 + // 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 + } + 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, ",")) +} + +// 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 +// 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 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 +} + +// 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]) + } +} + +// 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 +// 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 + } + file, ok := readSeiTomlAt(home) + if !ok { + log.Debug("no readable sei.toml; every key reads as it always has", "home", home) + } + return file, ok +} + +// 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 { + 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, 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 { + 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 new file mode 100644 index 0000000000..f606e5f629 --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -0,0 +1,228 @@ +package configmanager + +import ( + "cmp" + "fmt" + "log/slog" + "os" + "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) + } + + // 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 := 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, "; ")) + 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 "+ + "risking a half-written one; these keys read as they always have", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } + 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 "+ + "every one of its keys reads as it always has", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } + + *ctx.Config = *candidate + 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. +// +// 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(moved, "; ")) +} + +// 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" + +// 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 +// 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, typed map[string]string, log *slog.Logger) { + supplied := false + for _, key := range resolved.Overrides { + if key == logLevelKey { + supplied = true + } + } + 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", + "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) + // 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 new file mode 100644 index 0000000000..e878e80649 --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -0,0 +1,335 @@ +package configmanager + +import ( + "fmt" + "reflect" + "sort" + "strings" + "time" + + "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, error) { + out := map[string]string{} + if cfg == nil { + return out, fmt.Errorf("no configuration to read") + } + var nested map[string]any + if err := mapstructure.Decode(cfg, &nested); err != nil { + return out, err + } + flat := map[string]any{} + flatten("", nested, flat) + for _, key := range keys { + if v, ok := flat[key]; ok { + out[key] = fmt.Sprint(v) + } + } + return out, nil +} + +// 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 + } +} + +// 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 { + out, _ := describe(cfg, keys) + return out +} + +// 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. +// +// 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. +// +// 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 { + n, numeric := asNumber(value) + if !numeric { + continue + } + 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) + 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 keysWhoseFieldIs(ft, prefix, is) { + out[key] = true + } + continue + } + if is(ft) { + out[path] = true + continue + } + if ft.Kind() == reflect.Struct { + for key := range keysWhoseFieldIs(ft, path, is) { + 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 { + 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_agreement_test.go b/cmd/seid/cmd/node_agreement_test.go new file mode 100644 index 0000000000..58245a13db --- /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" +) + +// 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, +// 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 bootGeneratedDefaults = map[string]string{ + "p2p.recv-rate": "5120000", + "p2p.send-rate": "5120000", + "proxy-app": "", + "tx-index.indexer": "[kv]", +} + +// 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", + "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. +// +// 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 := 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 := 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, 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(bootGeneratedDefaults) { + t.Errorf("measured %d divergences and %d are recorded: %v", + len(measured), len(bootGeneratedDefaults), 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 new file mode 100644 index 0000000000..abae1b238c --- /dev/null +++ b/cmd/seid/cmd/node_delivery_test.go @@ -0,0 +1,380 @@ +package cmd + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "testing" + "time" + + "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") + } +} + +// 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("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 { + 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) + } + }) +} + +// 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") + } +} + +// 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 +} 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 +} 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/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/registry.go b/config/registry/registry.go index ba5a1a437f..b34f4095de 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 @@ -523,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 diff --git a/config/registry/resolve.go b/config/registry/resolve.go index 09020e846c..4ebe29671a 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 83f5591a6d..df3290c32b 100644 --- a/config/registry/spec_test.go +++ b/config/registry/spec_test.go @@ -1489,3 +1489,100 @@ func TestAFieldCollectingUnmatchedKeysDeclaresNone(t *testing.T) { t.Errorf("declares %v, want %v; the collector itself is not a setting", registered.Keys, want) } } + +// 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 +} + +// 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) + } + } + } +}