From ccbde2559e85eff931eb3c47ef1731ed90d92677 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 27 Aug 2026 13:09:57 -0700 Subject: [PATCH 1/2] feat(config): a command that says what a node's sei.toml reaches An operator running this before a restart is asking whether their file is right. It answers without starting a node: which declared keys the file supplies, which names reach nothing, and which written values a delivery would refuse. Predicting the refusals is why this sits on top of the deliveries rather than beside them. The answer has to come from the same code that would refuse the value at boot, or it is a second opinion that can disagree with the one that matters. A file that cannot be read is reported as a problem of a file that was found, so the command exits non-zero. It previously reported that the node had no file at all, which is both wrong and the answer least likely to make an operator look. Its own group of commands rather than a subcommand of the existing one, which reads and writes the files this is about rather than the file that replaces them. Verified by mutation: collapsing an unreadable file back to an absent one fails all three of the cases an operator hits. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/configmanager/check.go | 152 ++++++++++++++++ cmd/seid/cmd/configmanager/check_test.go | 211 +++++++++++++++++++++++ cmd/seid/cmd/root.go | 14 ++ 3 files changed, 377 insertions(+) create mode 100644 cmd/seid/cmd/configmanager/check.go create mode 100644 cmd/seid/cmd/configmanager/check_test.go diff --git a/cmd/seid/cmd/configmanager/check.go b/cmd/seid/cmd/configmanager/check.go new file mode 100644 index 0000000000..de55dde3fc --- /dev/null +++ b/cmd/seid/cmd/configmanager/check.go @@ -0,0 +1,152 @@ +package configmanager + +import ( + "errors" + "fmt" + "io" + "io/fs" + "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. A file that exists and will not read is the opposite, and is +// reported as a problem of a file that was found. +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, err := readSeiTomlAt(home) + switch { + case errors.Is(err, fs.ErrNotExist): + // The absence of a file is the one case with nothing to report. + return nil, false, nil + case err != nil: + // A file that exists and will not read is the case this command exists for. Reported as a problem + // of a file that was found, so the command exits non-zero: an operator running this before a + // restart is asking whether their file is right, and answering that they have no file is both + // wrong and the answer least likely to make them look. + return []string{fmt.Sprintf("sei.toml cannot be read: %v", err)}, true, 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..f324f1213d --- /dev/null +++ b/cmd/seid/cmd/configmanager/check_test.go @@ -0,0 +1,211 @@ +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" + "github.com/spf13/cobra" + "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") + } +} + +// TestCheckReportsAFileItCannotRead holds the difference the command exists to tell an operator. +// +// Running this before a restart asks whether the file is right. A file that will not parse, or records a +// schema this binary does not know, or names no node kind, is the case where the answer matters most, and +// answering that the node has no file is both wrong and the answer least likely to make anyone look. +func TestCheckReportsAFileItCannotRead(t *testing.T) { + for _, tc := range []struct { + name string + body string + }{ + {"unparseable", "[evm\n"}, + {"unknown schema", "schema_version = 99\nnode_mode = \"validator\"\n"}, + {"no node mode", "schema_version = 1\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + home := t.TempDir() + if err := os.MkdirAll(filepath.Join(home, "config"), 0o750); err != nil { + t.Fatalf("make a home: %v", err) + } + if err := os.WriteFile(filepath.Join(home, "config", seiTomlName), + []byte(tc.body), 0o600); err != nil { + t.Fatalf("write the file: %v", err) + } + + cmd := &cobra.Command{} + cmd.Flags().String(flags.FlagHome, home, "") + problems, found, err := checkSeiToml(cmd) + if err != nil { + t.Fatalf("checkSeiToml: %v", err) + } + if !found { + t.Errorf("a %s sei.toml is reported as no file at all, so the command exits zero on it", + tc.name) + } + if len(problems) == 0 { + t.Errorf("a %s sei.toml produced no problem to report", tc.name) + } + }) + } +} diff --git a/cmd/seid/cmd/root.go b/cmd/seid/cmd/root.go index 4b161cf4de..b4bba7b069 100644 --- a/cmd/seid/cmd/root.go +++ b/cmd/seid/cmd/root.go @@ -145,6 +145,7 @@ func initRootCmd( tmcli.NewCompletionCmd(rootCmd, true), debugCmd, config.Cmd(), + seiConfigCmd(), tools.ToolCmd(), SnapshotCmd(), LogLevelCmd(), @@ -477,3 +478,16 @@ supply_enabled = {{ .LightInvariance.SupplyEnabled }} return customAppTemplate, customAppConfig } + +// seiConfigCmd groups the commands that answer questions about a node's sei.toml. +// +// Its own group rather than a subcommand of the existing configuration command, which reads and writes the +// files this one is about rather than the file that replaces them. +func seiConfigCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "sei-config", + Short: "Inspect the node's sei.toml", + } + cmd.AddCommand(configmanager.CheckCmd()) + return cmd +} From 0dc6b0029d3983f717a00cfd4d51bf174ec2e113 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 27 Aug 2026 14:34:08 -0700 Subject: [PATCH 2/2] fix(config): make the pre-flight command answer for the node an operator runs it on The command failed on a correct file. Run the way a runbook runs it, with --home, it exited non-zero and named `home` as a key sei.toml writes: $ seid sei-config check --home /var/lib/sei home: sei.toml writes this and no section declares it, so it has no effect Error: 1 problem(s); a boot would apply what it could and report the rest The file contains no such key. Every unmatched flag reached the resolution under its own name and was counted with the file's own undeclared keys. It now reports only the file's. A pre-flight that fails every time carries nothing, and this command is the whole compensating control for a boot that may not refuse a file. It also wrote the files it was asked about. The root command's hook runs the configuration handler, which generates config.toml and app.toml when they are absent, so asking a question about one file created two others. The same hook copies configuration values into flags and marks them changed, which is exactly the state that makes a flag indistinguishable from a key an operator's app.toml holds, and this command reports on what was typed. The command group now carries a hook of its own, which stops both. It was silent on the question with the largest consequence. Two files record what kind of node this is, under different names, and nothing keeps them in step. A node whose sei.toml says validator while its own file says full resolves a validator's answers and serves queries, and every report about it reads correctly. The boot reports that at its loudest level and the check did not ask. A node that has no configuration file of its own has nothing to disagree with, and that case is now answered in the one function both callers pass through rather than at each of them. It said nothing about whether a boot would read the file at all. Until the gate is switched a boot reads none of it, so a passing check read as "in use and correct" on every node, which invites trusting a file nothing reads. It now says so, and it names a gate value this binary would refuse outright. The refusal messages spliced two sentences into one, and the tests could not see any of this: they executed the command with no parent, so no hook ran, no flag was marked changed, and no file was generated. The new tests run it through the real root command with the wiring the binary uses. Not rehearsed: the install into the source a node builds, because that source does not exist until a boot builds it. A key can be refused there for a reason nothing here can see. Stated in the command's own documentation, along with the fact that this reads the environment of whoever runs it rather than the node's. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/check_through_root_test.go | 144 +++++++++++++++++++++++ cmd/seid/cmd/configmanager/check.go | 67 +++++++++-- cmd/seid/cmd/configmanager/check_test.go | 108 +++++++++++++++++ cmd/seid/cmd/configmanager/install.go | 12 +- cmd/seid/cmd/root.go | 11 ++ 5 files changed, 330 insertions(+), 12 deletions(-) create mode 100644 cmd/seid/cmd/check_through_root_test.go diff --git a/cmd/seid/cmd/check_through_root_test.go b/cmd/seid/cmd/check_through_root_test.go new file mode 100644 index 0000000000..c1f8b1427c --- /dev/null +++ b/cmd/seid/cmd/check_through_root_test.go @@ -0,0 +1,144 @@ +package cmd + +import ( + "bytes" + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/sei-protocol/sei-chain/sei-cosmos/client" + "github.com/sei-protocol/sei-chain/sei-cosmos/client/flags" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + tmcli "github.com/sei-protocol/sei-chain/sei-tendermint/libs/cli" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// runCheckThroughRoot runs `sei-config check --home ` the way an operator and a runbook run it. +// +// Through the real root command, because that is what makes the difference. A command executed on its own +// has no parent, so nothing the root does before it happens: no hook runs, no flag is marked changed by +// anything but the caller, and no file is generated. Every one of those is a thing this command has to be +// right about, and a test that builds the command directly cannot see any of them. +func runCheckThroughRoot(t *testing.T, home string, extraArgs ...string) (string, error) { + t.Helper() + + root, _ := NewRootCmd() + var out bytes.Buffer + root.SetOut(&out) + root.SetErr(&out) + root.SilenceUsage = true + root.SilenceErrors = true + + // The same wiring the binary uses. --home is registered by PrepareBaseCmd rather than by the root + // command itself, so a root built without it is not the command an operator runs. + srvCtx := server.NewDefaultContext() + ctx := context.WithValue(context.Background(), client.ClientContextKey, &client.Context{}) + ctx = context.WithValue(ctx, server.ServerContextKey, srvCtx) + root.PersistentFlags().String(flags.FlagLogLevel, "", "") + root.PersistentFlags().String(flags.FlagLogFormat, "", "") + executor := tmcli.PrepareBaseCmd(root, "", home) + + root.SetArgs(append([]string{"sei-config", "check", "--home", home}, extraArgs...)) + err := executor.ExecuteContext(ctx) + return out.String(), err +} + +// writeSeiToml puts a sei.toml in a home that holds nothing else. +func writeSeiToml(t *testing.T, body string) string { + t.Helper() + home := t.TempDir() + if err := os.MkdirAll(filepath.Join(home, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(filepath.Join(home, "config", "sei.toml"), []byte(body), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + return home +} + +// TestTheCheckPassesACorrectFileWhenRunAsAnOperatorRunsIt is the answer the command exists to give. +// +// The one flag every real invocation carries is --home, and it names no setting: the node is told where its +// files are, not what to put in them. It arrives in the resolution beside the file's own keys, and counted +// with them it is a key nothing declares, so the command failed on a correct file and named a key the file +// does not contain. A pre-flight that fails every time carries nothing, and it is the whole compensating +// control for a boot that may not refuse a file. +func TestTheCheckPassesACorrectFileWhenRunAsAnOperatorRunsIt(t *testing.T) { + configtest.Isolate(t) + home := writeSeiToml(t, "schema_version = 1\nnode_mode = \"validator\"\n\n[mempool]\nsize = 4321\n") + + out, err := runCheckThroughRoot(t, home) + if err != nil { + t.Errorf("a correct file was refused when the command was run with --home: %v\n%s", err, out) + } + for _, flag := range []string{"home", "trace", "chain-id"} { + if strings.Contains(out, flag+": sei.toml writes this") { + t.Errorf("the report names --%s as something sei.toml wrote. The file does not contain it, "+ + "and an operator told this on every run stops reading the one signal they have", flag) + } + } +} + +// TestTheCheckDoesNotGenerateTheFilesItIsAskedAbout holds the command to answering rather than acting. +// +// The root command's hook runs the configuration handler, which writes config.toml and app.toml when they +// are absent. A command that reports on one file would then create two others as a side effect, on a node +// an operator was only asking a question about. +func TestTheCheckDoesNotGenerateTheFilesItIsAskedAbout(t *testing.T) { + configtest.Isolate(t) + home := writeSeiToml(t, "schema_version = 1\nnode_mode = \"validator\"\n\n[mempool]\nsize = 4321\n") + + // The verdict is not what this measures, and asserting it first would stop the measurement whenever + // something else about the check is wrong. + if out, err := runCheckThroughRoot(t, home); err != nil { + t.Logf("the check reported a problem, which is not what this test is about: %v\n%s", err, out) + } + for _, name := range []string{"config.toml", "app.toml"} { + if _, err := os.Stat(filepath.Join(home, "config", name)); err == nil { + t.Errorf("%s was created by a command that answers a question about a different file", name) + } + } +} + +// TestTheCheckStillFailsAFileWithSomethingWrongInIt keeps the fixes above from making it answer nothing. +// +// Passing a correct file and generating no files are both satisfied by a command that does nothing at all, +// so the failure has to be shown to still happen through the same path. +func TestTheCheckStillFailsAFileWithSomethingWrongInIt(t *testing.T) { + configtest.Isolate(t) + for _, tc := range []struct { + name string + body string + says string + }{ + { + name: "a key no section declares", + body: "schema_version = 1\nnode_mode = \"validator\"\n\n[mempool]\nsizze = 4321\n", + says: "no section declares it", + }, + { + name: "a length of time as a plain number", + body: "schema_version = 1\nnode_mode = \"validator\"\n\n[mempool]\nttl-duration = 60\n", + says: "nanoseconds", + }, + { + name: "a number larger than the setting holds", + body: "schema_version = 1\nnode_mode = \"validator\"\n\n[p2p]\nmax-connections = 1e20\n", + says: "larger than this setting can hold", + }, + } { + t.Run(tc.name, func(t *testing.T) { + out, err := runCheckThroughRoot(t, writeSeiToml(t, tc.body)) + if err == nil { + t.Fatalf("the file was accepted, so a boot would apply what it could and the operator "+ + "would find out afterwards\n%s", out) + } + if !strings.Contains(out, tc.says) { + t.Errorf("the report does not say %q, so it does not tell an operator what to change:\n%s", + tc.says, out) + } + }) + } +} diff --git a/cmd/seid/cmd/configmanager/check.go b/cmd/seid/cmd/configmanager/check.go index de55dde3fc..9f201da06f 100644 --- a/cmd/seid/cmd/configmanager/check.go +++ b/cmd/seid/cmd/configmanager/check.go @@ -6,6 +6,7 @@ import ( "io" "io/fs" "os" + "path/filepath" "sort" "strings" @@ -24,8 +25,17 @@ import ( // 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. +// input, so the same file against the same binary gives the same answer here as it will at boot, for the +// same environment. That last part is a real condition and not a formality: this reads the environment of +// whoever runs it, and a node started by an init system or a container runtime has a different one. A +// variable that answers a declared key is a variable this cannot see unless it is set here too. +// +// What it does not rehearse is the install into the source a node builds, because that source does not +// exist until a boot builds it. A key can be refused there for a reason nothing here can see, and the whole +// install is dropped when it is. That is worth adding when the surface it covers is more than a handful of +// keys on a live node. +// +// This asks what it can answer where an answer costs a failed check rather than a restart. func CheckCmd() *cobra.Command { cmd := &cobra.Command{ Use: "check", @@ -46,6 +56,7 @@ func CheckCmd() *cobra.Command { return nil } out := cmd.OutOrStdout() + reportWhetherABootWouldReadThisFile(out, os.Getenv) for _, line := range problems { report(out, line) } @@ -60,6 +71,25 @@ func CheckCmd() *cobra.Command { return cmd } +// reportWhetherABootWouldReadThisFile says whether this node is set up to use the file at all. +// +// Without it, a passing check reads as "this file is in use and correct" on a node where a boot ignores it +// completely, which is the state every node is in until an operator switches the gate. That is the wrong +// conclusion in the more dangerous direction: it invites somebody to trust a file nothing reads. +// +// Answered from the environment this command runs in, which is the same limitation the resolution has. +func reportWhetherABootWouldReadThisFile(out io.Writer, getenv func(string) string) { + if _, err := Select(getenv); err != nil { + report(out, fmt.Sprintf("%s is set to something this binary does not accept, so a boot would "+ + "refuse before reaching this file: %v", EnvVar, err)) + return + } + if getenv(EnvVar) != "v2" { + report(out, fmt.Sprintf("%s is not set to v2 for this command, so a boot in the same environment "+ + "reads none of this file. What follows is what it would reach if it were", EnvVar)) + } +} + // 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 @@ -106,14 +136,36 @@ func checkSeiToml(cmd *cobra.Command) (problems []string, found bool, err error) return []string{fmt.Sprintf("this node's configuration cannot be resolved: %v", err)}, true, nil } - for _, key := range resolved.Unknown { + // Only the file's own keys. A flag matching no declared key arrives in the same resolution and is not + // a mistake: every command carries flags that name no setting, so reporting those would fail this + // check on every invocation that types one, including a correct file. + for _, key := range resolved.UnknownInFile { problems = append(problems, fmt.Sprintf("%s: sei.toml writes this and no section declares it, "+ "so it has no effect", key)) } + if running := theModeTheNodesOwnFileRecords(home); modesDisagree(mode, running) { + problems = append(problems, fmt.Sprintf("sei.toml says this is a %s and the node's own "+ + "configuration file says %s. Every value resolved here is the answer for the first and the "+ + "node would run as the second", mode, running)) + } problems = append(problems, whatADecodeWouldRefuse(resolved)...) return problems, true, nil } +// theModeTheNodesOwnFileRecords reads what kind of node the node's own configuration file says this is. +// +// Read from the file here rather than taken from a running node, because nothing is running. An absent file +// or an absent key answers empty, which is not a disagreement: a node that was never initialised has +// nothing to disagree with. +func theModeTheNodesOwnFileRecords(home string) string { + v := viper.New() + v.SetConfigFile(filepath.Join(home, "config", "config.toml")) + if err := v.ReadInConfig(); err != nil { + return "" + } + return v.GetString("mode") +} + // 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 @@ -123,13 +175,14 @@ func checkSeiToml(cmd *cobra.Command) (problems []string, found bool, err error) func whatADecodeWouldRefuse(resolved registry.Resolved) []string { bySection := registry.SuppliedByDecodedSection(resolved) var problems []string - for _, name := range sortedSectionNames(bySection) { + for _, name := range sortedKeys(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, "; "))) + // Each message says what is wrong with the value it names, and there is more than one thing that + // can be. Stating one of them here would describe the others wrongly. + if bad := whatDecodesToSomethingElse(base, values); len(bad) > 0 { + problems = append(problems, fmt.Sprintf("[%s]: %s", name, strings.Join(bad, "; "))) continue } diff --git a/cmd/seid/cmd/configmanager/check_test.go b/cmd/seid/cmd/configmanager/check_test.go index f324f1213d..1bd21bb656 100644 --- a/cmd/seid/cmd/configmanager/check_test.go +++ b/cmd/seid/cmd/configmanager/check_test.go @@ -209,3 +209,111 @@ func TestCheckReportsAFileItCannotRead(t *testing.T) { }) } } + +// TestTheCheckSaysWhetherABootWouldReadTheFileAtAll covers the conclusion an operator would otherwise draw. +// +// Until the gate is switched, a boot reads none of this file. A check that answers only about the file's +// contents reads as "in use and correct" on every one of those nodes, which invites somebody to trust a +// file nothing reads. That is the wrong conclusion in the dangerous direction. +func TestTheCheckSaysWhetherABootWouldReadTheFileAtAll(t *testing.T) { + for _, tc := range []struct { + name string + value string + says bool + }{ + {"unset", "", true}, + {"legacy", "legacy", true}, + {"v2", "v2", false}, + } { + t.Run(tc.name, func(t *testing.T) { + var out bytes.Buffer + reportWhetherABootWouldReadThisFile(&out, func(string) string { return tc.value }) + said := strings.Contains(out.String(), "reads none of this file") + if said != tc.says { + t.Errorf("with %s=%q the command says a boot reads none of the file: %v, want %v.\n%s", + EnvVar, tc.value, said, tc.says, out.String()) + } + }) + } +} + +// TestTheCheckSaysWhenTheGateItselfIsWrong covers a value a boot refuses outright. +// +// The gate is matched exactly and never falls back, so a misspelling stops the node before it reaches any +// file. A check that reported only on the file would pass, and the node would not start. +func TestTheCheckSaysWhenTheGateItselfIsWrong(t *testing.T) { + var out bytes.Buffer + reportWhetherABootWouldReadThisFile(&out, func(string) string { return "V2" }) + if !strings.Contains(out.String(), "does not accept") { + t.Errorf("a gate value this binary refuses is not reported, so the check passes for a node that "+ + "would not start:\n%s", out.String()) + } +} + +// runCheckWithNodeFile runs the command against a home holding both files. +func runCheckWithNodeFile(t *testing.T, seiToml, configToml 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 err := os.WriteFile(filepath.Join(home, "config", seiTomlName), []byte(seiToml), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + if configToml != "" { + path := filepath.Join(home, "config", "config.toml") + if err := os.WriteFile(path, []byte(configToml), 0o600); err != nil { + t.Fatalf("write config.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 +} + +// TestTheCheckFindsADisagreementAboutWhatKindOfNodeThisIs covers the question with the largest consequence. +// +// Two files record what kind of node this is, under different names, and nothing keeps them in step. A node +// whose sei.toml says validator while its own file says full resolves a validator's answers and runs as a +// full node, serving queries. Every report about it reads correctly, which is what makes it worth catching +// before a restart rather than after. +// +// The boot reports this at its loudest level. A pre-flight that does not ask is silent on the one thing an +// operator most needs to know before they restart. +func TestTheCheckFindsADisagreementAboutWhatKindOfNodeThisIs(t *testing.T) { + const seiToml = "schema_version = 1\nnode_mode = \"validator\"\n\n[mempool]\nsize = 4321\n" + + t.Run("a disagreement is reported", func(t *testing.T) { + out, err := runCheckWithNodeFile(t, seiToml, "mode = \"full\"\n") + if err == nil { + t.Errorf("sei.toml says validator and the node's own file says full, and the check passed:\n%s", + out) + } + if !strings.Contains(out, "node's own") { + t.Errorf("the report does not name the disagreement:\n%s", out) + } + }) + + t.Run("agreement is not a problem", func(t *testing.T) { + out, err := runCheckWithNodeFile(t, seiToml, "mode = \"validator\"\n") + if err != nil { + t.Errorf("both files say validator and the check failed: %v\n%s", err, out) + } + }) + + t.Run("a node with no configuration file of its own has nothing to disagree with", func(t *testing.T) { + // Every node is in this state before it is initialised, and answering that it disagrees with a file + // it does not have would fail the check on a correct sei.toml. + out, err := runCheckWithNodeFile(t, seiToml, "") + if err != nil { + t.Errorf("a node with no configuration file of its own failed the check: %v\n%s", err, out) + } + }) +} diff --git a/cmd/seid/cmd/configmanager/install.go b/cmd/seid/cmd/configmanager/install.go index bab0cc4054..2fea064be7 100644 --- a/cmd/seid/cmd/configmanager/install.go +++ b/cmd/seid/cmd/configmanager/install.go @@ -219,7 +219,7 @@ func sortedKeys[V any](m map[string]V) []string { // 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 == "" { + if ctx == nil || ctx.Config == nil { return } running := ctx.Config.Mode @@ -234,11 +234,13 @@ func reportWhatTheFileSaysTheNodeIs(ctx *server.Context, mode string, log *slog. // 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. +// Two cases are not a disagreement. An empty running mode means the node's own file does not state one, so +// there is nothing to disagree with: a node that was never initialised, or one whose file predates the key. +// And 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 { + if running == "" || recorded == running { return false } return recorded != string(registry.ModeArchive) || running != string(registry.ModeFull) diff --git a/cmd/seid/cmd/root.go b/cmd/seid/cmd/root.go index b4bba7b069..0f04753da5 100644 --- a/cmd/seid/cmd/root.go +++ b/cmd/seid/cmd/root.go @@ -487,6 +487,17 @@ func seiConfigCmd() *cobra.Command { cmd := &cobra.Command{ Use: "sei-config", Short: "Inspect the node's sei.toml", + // A hook of its own, which stops the root one from running. Two things follow, and both are + // required rather than convenient. + // + // The root hook writes files. It runs the configuration handler, which generates config.toml and + // app.toml when they are absent, so a command that answers a question about a file would create + // two others as a side effect. + // + // It also copies configuration values into flags and marks them changed, which is exactly the + // state that makes a flag indistinguishable from a key an operator's app.toml holds. A command + // reading its flags after that cannot tell what was typed, and it reports on what was typed. + PersistentPreRunE: func(*cobra.Command, []string) error { return nil }, } cmd.AddCommand(configmanager.CheckCmd()) return cmd