From 820405e6b2e74197b1475b1d4859bf158972e3e5 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 3 Sep 2026 10:54:37 -0700 Subject: [PATCH 1/3] test(config): record what generated files and the declaration disagree on The decode side recorded four keys where a declared value and a boot-generated file disagree. It resolved one kind of node, so it covered a quarter of the space, and the lookup side had no record at all. Measured across every kind of node and both deliveries. Thirty-six rows in four sets, and eight of them are decode-side rows three kinds of node have that a validator does not. Two are worth naming: p2p.laddr and rpc.laddr bind loopback and are declared on every interface, so a full or archive node adopting a sparse file reaches the network where it did not. One row is a third writer rather than the second. A key with a flag of its own on the start command is answered by that flag's default whether a file mentions it or not. That is pruning, in every kind of node: the flag prunes and the declaration keeps all state history. Eleven further declared keys are left unanswered by a generated file. Their readers each take the source's answer only when there is one, so the comparison has nothing to read for them and they are recorded as a named set instead. A key that stops being answered moves into that set and nothing else would say so. The two records are one, because they had the same shape and the same gap. The two-boot helper is shared rather than held inside one measurement. Each check verified by mutation: a dropped row, a wrong value, an extra row, a dropped kind of node, a row that no longer diverges, reading a decode key off the wrong delivery, and both directions of the unanswered set. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/node_agreement_test.go | 281 ++++++++++++++++++++++------ 1 file changed, 220 insertions(+), 61 deletions(-) diff --git a/cmd/seid/cmd/node_agreement_test.go b/cmd/seid/cmd/node_agreement_test.go index a65ffba43d..0da3f12eef 100644 --- a/cmd/seid/cmd/node_agreement_test.go +++ b/cmd/seid/cmd/node_agreement_test.go @@ -15,47 +15,197 @@ import ( "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. +// divergences is what a node runs for each key where its own generated files and this binary's +// declaration disagree, per kind of node. // // 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. +// in this binary that writes these files: a node started without them gets them generated by the boot +// instead, and the two do not agree. A third value comes from somewhere else again, because a key with a +// flag of its own on the start command is answered by that flag's default whether a file mentions it or +// not, and that default is stated where the flag is declared. // -// 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", - "rpc.pprof-laddr": "localhost:6060", - "tx-index.indexer": "[kv]", +// Per kind of node, because the declaration varies on the kind and the writer does not. The boot's writer +// produces one shape for every kind, so a declaration for a kind that shape does not describe disagrees +// with it. Measuring one kind finds a quarter of the rows. +// +// Each row is a setting a sparse sei.toml moves, because a key that file leaves out takes the declared +// value. What the move does, by group: +// +// listener addresses p2p.laddr and rpc.laddr bind loopback and are declared on every interface, so a +// full or archive node adopting a sparse file reaches the network where it did +// not. rpc.pprof-laddr is the other direction: a debug listener the node opens on +// a fixed port, declared closed. +// rate ceilings p2p.recv-rate and p2p.send-rate cap what one connection may move. The declared +// value is four times what a node runs, so a sparse file raises both. +// connection policy p2p.max-connections is ten times higher declared than a seed runs, and +// p2p.allow-duplicate-ip is declared on where a seed has it off. +// service switches api.enable, grpc.enable, evm.http_enabled, evm.ws_enabled and +// state-store.ss-enable are open on the node and declared closed for a validator +// and a seed, which serve no queries. A sparse file closes them. +// retention pruning is the flag-default row, in every kind: the flag prunes and the +// declaration keeps all state history. min-retain-blocks and +// state-store.ss-keep-recent are the block and state halves of the same choice, +// and each diverges for the one kind whose rule moves it. +// indexing tx-index.indexer decides whether the node indexes transactions. The boot's +// writer produces a file for a node that serves queries, so this row is what a +// resolution for a validator states against a file written for something else. +// +// 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 runs +// instead. +var divergences = map[registry.Mode]map[string]string{ + registry.ModeValidator: { + "api.enable": "true", + "evm.http_enabled": "true", + "evm.ws_enabled": "true", + "grpc.enable": "true", + "p2p.recv-rate": "5120000", + "p2p.send-rate": "5120000", + "pruning": "default", + "rpc.pprof-laddr": "localhost:6060", + "state-store.ss-enable": "true", + "tx-index.indexer": "[kv]", + }, + registry.ModeFull: { + "min-retain-blocks": "0", + "p2p.laddr": "tcp://127.0.0.1:26656", + "p2p.recv-rate": "5120000", + "p2p.send-rate": "5120000", + "pruning": "default", + "rpc.laddr": "tcp://127.0.0.1:26657", + "rpc.pprof-laddr": "localhost:6060", + }, + registry.ModeSeed: { + "api.enable": "true", + "evm.http_enabled": "true", + "evm.ws_enabled": "true", + "grpc.enable": "true", + "p2p.allow-duplicate-ip": "false", + "p2p.max-connections": "100", + "p2p.recv-rate": "5120000", + "p2p.send-rate": "5120000", + "pruning": "default", + "rpc.pprof-laddr": "localhost:6060", + "state-store.ss-enable": "true", + "tx-index.indexer": "[kv]", + }, + registry.ModeArchive: { + "p2p.laddr": "tcp://127.0.0.1:26656", + "p2p.recv-rate": "5120000", + "p2p.send-rate": "5120000", + "pruning": "default", + "rpc.laddr": "tcp://127.0.0.1:26657", + "rpc.pprof-laddr": "localhost:6060", + "state-store.ss-keep-recent": "100000", + }, } -// 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", - "rpc.pprof-laddr": "a debug listener the declaration states is closed. The other writer opens it on a " + - "fixed port, and a flag bound to this key hides that on a node's first boot only, because the " + - "file has not been read yet for the flag's empty default to lose to", - "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", +// keysAGeneratedFileLeavesToTheirReader is every declared key a lookup delivers that a boot-generated file +// does not answer. +// +// Each of their readers starts from a default of its own and takes the source's answer only when there is +// one, so what a node runs for one of these keys is that default and the source states nothing. They are +// named rather than compared because the comparison above has nothing to read. +// +// Every one of these declarations is taken from the same default the reader falls back to, so the value a +// node runs is the declared one. That is not measured here. What reaches it is a file stating every +// declared key and a node started from it. +// +// The same in every kind of node. The writer does not vary on the kind, and which keys are declared does +// not either. +var keysAGeneratedFileLeavesToTheirReader = []string{ + "eth_replay.contract_state_checks", + "evm.enable_test_api", + "evm.max_concurrent_simulation_calls", + "evm.max_tx_pool_txs", + "evm.rpc_stats_interval", + "genesis.import-file", + "state-commit.flatkv.enable-read-write-metrics", + "state-commit.sc-snapshot-writer-limit", + "state-commit.sc-write-mode-enable-auto", + "wasm.memory_cache_size", + "wasm.simulation_gas_limit", } -// TestTheDivergencesFromAGeneratedFileAreTheRecordedOnes measures what a comment would only claim. +// TestTheDivergencesFromGeneratedFilesAreTheRecordedOnes 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. +// Which keys disagree is measured rather than described, because a key that starts diverging fails and so +// does one that stops. Both deliveries, read where each is read: the keys a decode delivers off the struct +// their file is decoded into, and every other key off the source a lookup reads. // -// 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) { +// Driven through a real boot with no files of its own and no sei.toml, so nothing is delivered and what +// the node holds is purely what the boot generated. +func TestTheDivergencesFromGeneratedFilesAreTheRecordedOnes(t *testing.T) { configtest.Isolate(t) - generated := whatTheBootGenerates(t) + ctx := theBootWithNoFileOfItsOwn(t) + decoded := configmanager.DescribeForTest(t, ctx.Config, keysADecodeDelivers()) + + for _, mode := range registry.Modes() { + t.Run(string(mode), func(t *testing.T) { + recorded, records := divergences[mode] + if !records { + t.Fatalf("nothing records what a %s node diverges on, so this kind is declared and no "+ + "measurement covers it", mode) + } + resolved, err := registry.Resolve(mode, registry.Sources{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + + var measured []string + for key, declared := range resolved.Values { + // A key the source does not answer reads as its reader's own default, which the source + // says nothing about. keysAGeneratedFileLeavesToTheirReader holds that set. + got, reads := whatTheNodeHolds(key, decoded, ctx) + if !reads { + continue + } + if fmt.Sprint(declared) == got { + if _, listed := recorded[key]; listed { + t.Errorf("%s no longer diverges on a %s node, both sides being %v. Take it off "+ + "the record, so the record stays the set of keys the writers state "+ + "differently", key, mode, declared) + } + continue + } + measured = append(measured, key) + want, listed := recorded[key] + switch { + case !listed: + t.Errorf("a %s node is declared to run %s as %v and one that let the boot generate "+ + "its files runs %q, and nothing records that. A sei.toml leaving this key out "+ + "moves the setting", mode, key, declared, got) + case want != got: + t.Errorf("a %s node is recorded as running %s as %q and runs %q", mode, key, want, got) + } + } + + sort.Strings(measured) + if len(measured) != len(recorded) { + t.Errorf("measured %d divergences for a %s node and %d are recorded: %v", + len(measured), mode, len(recorded), measured) + } + }) + } +} + +// TestTheKeysAGeneratedFileLeavesToTheirReaderAreTheRecordedOnes holds the set the measurement above skips. +// +// Skipping them is sound only while their reader's default is the declared value, and that holds because +// the declaration is taken from the same default. A key that stops being answered moves into this set and +// nothing else says so, and one that starts being answered leaves it and belongs in the measurement. +func TestTheKeysAGeneratedFileLeavesToTheirReaderAreTheRecordedOnes(t *testing.T) { + configtest.Isolate(t) + ctx := theBootWithNoFileOfItsOwn(t) + + decoded := make(map[string]bool) + for _, key := range keysADecodeDelivers() { + decoded[key] = true + } + recorded := make(map[string]bool, len(keysAGeneratedFileLeavesToTheirReader)) + for _, key := range keysAGeneratedFileLeavesToTheirReader { + recorded[key] = true + } resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{}) if err != nil { @@ -63,50 +213,57 @@ func TestTheDivergencesFromAGeneratedFileAreTheRecordedOnes(t *testing.T) { } 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) - } + for key := range resolved.Values { + if decoded[key] || ctx.Viper.Get(key) != nil { 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) + if !recorded[key] { + t.Errorf("%s is declared and a boot-generated file does not answer it, and nothing records "+ + "that. Its reader's own default is what a node runs for it", key) + } + } + for _, key := range keysAGeneratedFileLeavesToTheirReader { + if answer := ctx.Viper.Get(key); answer != nil { + t.Errorf("%s is recorded as unanswered and a boot-generated file answers it %v, so what a "+ + "node runs for it is measurable and no measurement covers it", key, answer) } } sort.Strings(measured) - if len(measured) != len(bootGeneratedDefaults) { - t.Errorf("measured %d divergences and %d are recorded: %v", - len(measured), len(bootGeneratedDefaults), measured) + if len(measured) != len(keysAGeneratedFileLeavesToTheirReader) { + t.Errorf("measured %d unanswered keys and %d are recorded: %v", + len(measured), len(keysAGeneratedFileLeavesToTheirReader), measured) + } +} + +// whatTheNodeHolds reads one key off whichever of the two deliveries owns it, and reports whether anything +// answers it. +func whatTheNodeHolds(key string, decoded map[string]string, ctx *server.Context) (string, bool) { + if held, byADecode := decoded[key]; byADecode { + return held, true } + answer := ctx.Viper.Get(key) + if answer == nil { + return "", false + } + return fmt.Sprint(answer), true } -// 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 { +// theBootWithNoFileOfItsOwn starts a node with no configuration files of its own and no sei.toml, and +// returns what it holds. Nothing is delivered, so both deliveries read purely what the boot generated. +// +// Booted twice, and the second one is returned. The first writes the files, and a flag bound to a key the +// writer sets overwrites that value before anything reads it back, because the file has not been read yet. +// From the second boot the file is read and wins, so what a node runs from its second start onward is what +// the second boot holds, and a divergence only that boot shows would otherwise be invisible here. +func theBootWithNoFileOfItsOwn(t *testing.T) *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) } - // Booted twice, and the second one is measured. The first writes the file, and a flag bound to a key - // the writer sets overwrites that value before anything reads it back, because the file has not been - // read yet. From the second boot the file is read and wins, so what a node runs from its second start - // onward is what the second boot holds, and a divergence only that boot shows would otherwise be - // invisible here. var ctx *server.Context for boot := 0; boot < 2; boot++ { cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) @@ -122,6 +279,8 @@ func whatTheBootGenerates(t *testing.T) map[string]string { if ctx.Config == nil { t.Fatal("the boot produced no node configuration") } - - return configmanager.DescribeForTest(t, ctx.Config, keysADecodeDelivers()) + if ctx.Viper == nil { + t.Fatal("the boot produced no configuration source") + } + return ctx } From e5b661110c30aa5b475dc28d6476df015521eabe Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 3 Sep 2026 11:40:11 -0700 Subject: [PATCH 2/3] feat(config): write a sei.toml stating what a node already runs A node under this manager answers every declared key from its sei.toml, so a sparse file moves every key it leaves out to the value this binary declares, and there are more than two hundred. Writing that file by hand means finding each of those by reading two files against a binary's defaults. seid config generate writes it from what the node answers today. Printed by default, since the file is read for every setting a node has and an operator reading it before it takes effect is the ordinary case. --write places it, and leaves an existing file alone. Two things make the written file run what the node ran. Every value it states is the node's own, read where that setting's reader reads it: the keys a decode delivers off the struct config.toml is decoded into, and every other key off app.toml over the start command's flag defaults. And a key nothing answers is not stated, because there is no value to state and its reader holds a default of its own. Reading the flag defaults is not an optimisation. Measured: app.toml over those defaults answers all 165 lookup keys exactly as a boot's own source does, and pruning is answered by a flag alone. A source built without them leaves pruning out of the file and silently stops a node pruning. The environment is not read. A variable that answers a declared key goes on answering it afterwards at the same precedence, so writing it into the file would state a value twice and leave the copy with no effect. Verified by starting a node from the written file: 154 keys a lookup answers and 116 a decode delivers, all unchanged across it. A second test holds which keys the file states, because a file full of lines restating the declaration passes a round trip while telling an operator that two hundred settings were decisions. Each check verified by mutation, with the mutation confirmed to compile: a source built without the flag defaults, a file stating nothing, a file stating every answered key, the decode keys omitted, an accepted empty home, an accepted unknown kind of node, and a replaced existing file. The home resolution, the node's own configuration reader and the typed read off that struct move to where both commands share them. Check's own tests pass unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/configmanager/check.go | 54 +--- cmd/seid/cmd/configmanager/generate.go | 255 ++++++++++++++++++ cmd/seid/cmd/configmanager/generate_test.go | 101 +++++++ cmd/seid/cmd/configmanager/nodefiles.go | 129 +++++++++ cmd/seid/cmd/configmanager/tendermint_copy.go | 18 +- cmd/seid/cmd/generate_through_root_test.go | 237 ++++++++++++++++ cmd/seid/cmd/root.go | 1 + 7 files changed, 743 insertions(+), 52 deletions(-) create mode 100644 cmd/seid/cmd/configmanager/generate.go create mode 100644 cmd/seid/cmd/configmanager/generate_test.go create mode 100644 cmd/seid/cmd/configmanager/nodefiles.go create mode 100644 cmd/seid/cmd/generate_through_root_test.go diff --git a/cmd/seid/cmd/configmanager/check.go b/cmd/seid/cmd/configmanager/check.go index 3daf62ddf6..3693ac11a9 100644 --- a/cmd/seid/cmd/configmanager/check.go +++ b/cmd/seid/cmd/configmanager/check.go @@ -6,8 +6,6 @@ import ( "io" "io/fs" "os" - "path" - "path/filepath" "reflect" "sort" "strings" @@ -138,19 +136,11 @@ func report(out io.Writer, line string) { _, _ = fmt.Fprintln(out, line) } // 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, notes []string, found bool, err error) { - home, err := resolveHomeDir(cmd) + // Refused rather than reported when no home is set, because the exit status is this command's answer + // and there is nothing to have an opinion about. + home, err := theHomeThisCommandRuns(cmd) if err != nil { - return nil, nil, false, fmt.Errorf("resolve the home directory: %w", err) - } - // An empty home leaves every path below relative, so the reads land in ./config under whatever - // directory this command was run from. That answers for some other node's files, and answering for - // the wrong node is worse than not answering: an operator runs this to decide whether to restart. - // - // The boot declines the same case. Refused rather than reported, because the exit status is this - // command's answer and there is nothing here to have an opinion about. - if home == "" { - return nil, nil, false, fmt.Errorf("no home directory is set, so there is no sei.toml to check. "+ - "Pass --home, or set %s", theVariableThatSetsTheHome()) + return nil, nil, false, err } file, err := readSeiTomlAt(home) switch { @@ -213,42 +203,6 @@ func checkSeiToml(cmd *cobra.Command) (problems, notes []string, found bool, err return problems, notes, true, nil } -// theVariableThatSetsTheHome names the environment variable the home resolves from. -// -// Derived from the running binary the same way the resolver derives it, so a message naming it cannot -// drift from the name that actually works. -func theVariableThatSetsTheHome() string { - exe, err := os.Executable() - if err != nil { - return "the home variable for this binary" - } - return strings.ToUpper(path.Base(exe)) + "_HOME" -} - -// theNodesOwnConfiguration reads the node's own configuration file into the struct a boot decodes it into. -// -// Decoded rather than read key by key, so the mode and the rehearsal base below come from one read and both -// answer for the same file. A boot unmarshals this file over the same defaults, so a key stated with nothing -// after it arrives empty and an absent key keeps the default, which is what a boot runs with. -// -// A file that is not there is the only absence. Every other failure is a file somebody wrote that a boot -// does not start on, so answering with defaults would pass a node that cannot boot. -func theNodesOwnConfiguration(home string) (*tmcfg.Config, error) { - cfg := tmcfg.DefaultConfig() - v := viper.New() - v.SetConfigFile(filepath.Join(home, "config", "config.toml")) - switch err := v.ReadInConfig(); { - case errors.Is(err, fs.ErrNotExist): - return cfg, nil - case err != nil: - return nil, err - } - if err := v.Unmarshal(cfg); err != nil { - return nil, err - } - return cfg, nil -} - // whatTheFileLeavesToTheDeclaration says how much of this node's configuration the file does not state. // // Every declared key the file leaves out takes the value this binary declares for the kind of node this is, diff --git a/cmd/seid/cmd/configmanager/generate.go b/cmd/seid/cmd/configmanager/generate.go new file mode 100644 index 0000000000..58a0abed7e --- /dev/null +++ b/cmd/seid/cmd/configmanager/generate.go @@ -0,0 +1,255 @@ +package configmanager + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "strings" + + "github.com/spf13/cobra" + + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/config/seitoml" +) + +// flagGenerateMode names the kind of node the written values resolve for. +// +// The same name the provisioning command takes, spelled here rather than shared, because the package +// holding that command imports this one and cannot be imported back. +const flagGenerateMode = "mode" + +// flagGenerateWrite places the file instead of printing it. +const flagGenerateWrite = "write" + +// GenerateCmd writes a sei.toml stating what this node already runs. +// +// A node under this manager answers every declared key from its sei.toml, so a sparse file is a large +// change to a node whose own files were tuned by hand: every declared key the file leaves out moves to the +// value this binary declares, and there are more than two hundred. Writing that file by hand means finding +// each of those by reading two files against a binary's defaults. +// +// This writes it instead, from what the node answers today. A key it states is a key this node answers +// differently from the declaration, so a node started against the written file runs what it ran before. +// +// Two things make the written file run what the node ran, and the divergence filter is neither of them. +// Every value it states is the node's own, read where that setting's reader reads it, so a stated key +// arrives as what the node already held. And a key nothing answers is not stated, because there is no +// value to state: its reader holds a default of its own and stating this binary's declaration instead is +// the one way a writer here changes a setting nobody decided to change. +// +// What the filter does is leave out a key whose answer is already the declared value. Those lines would +// state what the declaration states, so the file is shorter and every line in it marks a decision. +// +// What it does not read is the environment. A variable that answers a declared key goes on answering it +// after this runs, at the same precedence, so baking one into a file would state a value twice and leave +// the copy in the file with no effect. +func GenerateCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "generate", + Short: "Write a sei.toml stating what this node already runs", + Long: "Reads this node's app.toml and config.toml the way a boot reads them, and writes the " + + "sei.toml that makes a node under this manager run what this one runs today.\n\n" + + "Prints the file by default. Pass --write to place it in the node's config directory.", + Args: cobra.NoArgs, + // Nothing here is a usage error once the flags parse, and the production wiring does not silence + // usage, so cobra would print the whole usage block after the error. + SilenceUsage: true, + // A hook of its own, which stops the root one from running. Cobra runs the closest hook it finds. + // The root hook runs the boot's configuration handler, which generates config.toml and app.toml + // when they are absent and copies configuration values into flags. This command reads those files + // to answer what a node runs, so it must not be the thing that creates them, and it must read the + // flags before values are copied into them. + PersistentPreRunE: func(*cobra.Command, []string) error { return nil }, + RunE: func(cmd *cobra.Command, _ []string) error { + home, err := theHomeThisCommandRuns(cmd) + if err != nil { + return err + } + mode, err := theModeTheWrittenValuesResolveFor(cmd) + if err != nil { + return err + } + running, err := whatThisNodeAlreadyRuns(cmd, home) + if err != nil { + return err + } + stated, err := whatTheDeclarationDoesNotAlreadySay(mode, running) + if err != nil { + return err + } + file, err := theFileStating(mode, stated) + if err != nil { + return err + } + return thisFilePlacedOrPrinted(cmd, home, file, len(stated)) + }, + } + cmd.Flags().String(flagGenerateMode, "", "the kind of node the written values resolve for: "+ + modesInOrder()) + cmd.Flags().Bool(flagGenerateWrite, false, + "place the file at config/"+seiTomlName+" instead of printing it") + return cmd +} + +// theModeTheWrittenValuesResolveFor reads the kind of node this file is for. +// +// Required rather than guessed. Every value in the file resolves for one kind, and the kinds differ on +// keys that decide whether a node prunes and whether it serves queries. A guess that lands on the wrong +// one writes a file whose values were chosen against defaults the node does not use. +func theModeTheWrittenValuesResolveFor(cmd *cobra.Command) (registry.Mode, error) { + given, err := cmd.Flags().GetString(flagGenerateMode) + if err != nil { + return "", err + } + if given == "" { + return "", fmt.Errorf("--%s is required: every value written here resolves for one kind of node, "+ + "and the kinds differ on whether a node prunes and whether it serves queries. One of %s", + flagGenerateMode, modesInOrder()) + } + for _, known := range registry.Modes() { + if registry.Mode(given) == known { + return known, nil + } + } + return "", fmt.Errorf("%q is not a kind of node this binary declares defaults for. One of %s", + given, modesInOrder()) +} + +// modesInOrder names every kind of node, for a message that has to list them. +func modesInOrder() string { + names := make([]string, 0, len(registry.Modes())) + for _, mode := range registry.Modes() { + names = append(names, string(mode)) + } + return strings.Join(names, ", ") +} + +// whatThisNodeAlreadyRuns reads the value this node answers for every declared key it answers at all. +// +// Both deliveries, read the way each is delivered. The keys a decode delivers are read off the struct +// their file is decoded into, because that is where their reader looks. Every other key is read off the +// source a lookup reads, which is app.toml over the start command's flag defaults. +// +// A key nothing answers is absent from the result rather than present and empty. Its reader holds a +// default of its own, and a caller cannot tell an unanswered key from one answered with a zero. +func whatThisNodeAlreadyRuns(cmd *cobra.Command, home string) (map[string]any, error) { + source, err := theSourceThisNodeWouldBuild(cmd, home) + if err != nil { + return nil, err + } + own, err := theNodesOwnConfiguration(home) + if err != nil { + return nil, fmt.Errorf("read this node's own configuration: %w", err) + } + + _, ownedByADecode := registry.ResolvedAndOwnedByDecodedSections(registry.Resolved{}) + decoded, unread, err := whatEachKeyHolds(own, ownedByADecode) + if err != nil { + return nil, err + } + if len(unread) > 0 { + return nil, fmt.Errorf("%d of %d keys a decode delivers are not present in the node's "+ + "configuration, so a file written from it would leave them out and move them to their "+ + "declared value: %v", len(unread), len(ownedByADecode), unread) + } + + running := make(map[string]any, len(registry.Keys())) + for key, value := range decoded { + running[key] = value + } + for _, key := range registry.Keys() { + if _, byADecode := decoded[key]; byADecode { + continue + } + if answer := source.Get(key); answer != nil { + running[key] = answer + } + } + return running, nil +} + +// whatTheDeclarationDoesNotAlreadySay returns the keys this node answers differently from the declaration. +// +// Compared as text, because the two sides carry different Go types for the same key often enough that +// comparing values would be comparing shapes. What is written is the node's own value, with the type it +// holds, so it reaches its setting as what it says. +func whatTheDeclarationDoesNotAlreadySay(mode registry.Mode, running map[string]any) (map[string]any, error) { + resolved, err := registry.Resolve(mode, registry.Sources{}) + if err != nil { + return nil, fmt.Errorf("resolve this binary's defaults for a %s node: %w", mode, err) + } + stated := map[string]any{} + for key, declared := range resolved.Values { + answer, answers := running[key] + if !answers { + continue + } + if fmt.Sprint(answer) == fmt.Sprint(declared) { + continue + } + stated[key] = answer + } + return stated, nil +} + +// theFileStating returns a document carrying this binary's schema version, the kind of node, and one line +// per key. +// +// Keys written in order, so two runs over one node produce the same bytes and a difference between two +// files is a difference in what they state. +func theFileStating(mode registry.Mode, stated map[string]any) (*seitoml.File, error) { + file, err := seitoml.New(string(mode)) + if err != nil { + return nil, err + } + keys := make([]string, 0, len(stated)) + for key := range stated { + keys = append(keys, key) + } + sort.Strings(keys) + for _, key := range keys { + if err := file.Set(key, stated[key]); err != nil { + return nil, fmt.Errorf("state %s: %w", key, err) + } + } + return file, nil +} + +// thisFilePlacedOrPrinted prints the file, or puts it where a boot reads it. +// +// Printing is the default. This renders a file a boot reads for every setting a node has, so an operator +// reading it before it takes effect is the ordinary case, and `>` puts it where they want it. +// +// An existing file is never replaced. It holds what somebody decided, and the comments beside those +// decisions, and this command cannot tell a file it wrote last week from one edited since. +func thisFilePlacedOrPrinted(cmd *cobra.Command, home string, file *seitoml.File, stated int) error { + write, err := cmd.Flags().GetBool(flagGenerateWrite) + if err != nil { + return err + } + if !write { + body, err := file.Bytes() + if err != nil { + return err + } + _, err = cmd.OutOrStdout().Write(body) + return err + } + + // Asked before the document is rendered, so a home that already holds a file costs nothing and the + // refusal is the same whatever the file would have said. + path := filepath.Join(home, "config", seiTomlName) + if _, err := os.Stat(path); err == nil { + return fmt.Errorf("%s already exists and states what somebody decided, so it is left alone. "+ + "Print this instead and compare them", path) + } else if !os.IsNotExist(err) { + return err + } + if err := file.Save(path); err != nil { + return err + } + report(cmd.OutOrStdout(), fmt.Sprintf("wrote %s, stating %d of this node's %d declared keys", + path, stated, len(registry.Keys()))) + return nil +} diff --git a/cmd/seid/cmd/configmanager/generate_test.go b/cmd/seid/cmd/configmanager/generate_test.go new file mode 100644 index 0000000000..d33301cf4c --- /dev/null +++ b/cmd/seid/cmd/configmanager/generate_test.go @@ -0,0 +1,101 @@ +package configmanager + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/sei-protocol/sei-chain/sei-cosmos/client/flags" +) + +// runGenerate runs the command on its own, which is what makes the two refusals below reachable. +// +// A command executed with no parent has no sibling start command to read flag defaults off, and a home +// only if the caller registers one. Both are states the production wiring never produces, and both are +// states the command has to refuse rather than answer from. +func runGenerate(t *testing.T, home string, args ...string) (string, error) { + t.Helper() + cmd := GenerateCmd() + cmd.Flags().String(flags.FlagHome, home, "") + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + cmd.SetArgs(args) + err := cmd.Execute() + return out.String(), err +} + +// TestGenerateRefusesWhatItCannotAnswerFrom covers the states where writing a file would be worse than +// writing none. +// +// Each one produces a plausible file rather than an error. That is what makes them worth a test: an +// operator reads a sei.toml this command printed and has no way to tell it describes their node from it +// describing a directory the command happened to be run in. +func TestGenerateRefusesWhatItCannotAnswerFrom(t *testing.T) { + t.Run("no home is set", func(t *testing.T) { + _, err := runGenerate(t, "", "--mode", "validator") + if err == nil { + t.Fatal("the command answered with no home set, so it read whichever config directory the " + + "working directory holds and described some other node's files") + } + if !strings.Contains(err.Error(), "no home directory is set") { + t.Errorf("the refusal says %q, and it has to name the home", err) + } + }) + + t.Run("no start command to read flag defaults off", func(t *testing.T) { + home := t.TempDir() + if err := os.MkdirAll(filepath.Join(home, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + _, err := runGenerate(t, home, "--mode", "validator") + if err == nil { + t.Fatal("the command answered without the start command's flags, so a key answered only by " + + "a flag's default read as answered by nothing and the file would leave it out") + } + if !strings.Contains(err.Error(), "start") { + t.Errorf("the refusal says %q, and it has to name the command it could not find", err) + } + }) + + t.Run("no kind of node is given", func(t *testing.T) { + _, err := runGenerate(t, t.TempDir()) + if err == nil { + t.Fatal("the command answered with no kind of node given, so its values were chosen against " + + "whichever defaults it picked") + } + if !strings.Contains(err.Error(), "--"+flagGenerateMode) { + t.Errorf("the refusal says %q, and it has to name the flag that fixes it", err) + } + }) + + t.Run("the kind of node is not one this binary declares", func(t *testing.T) { + _, err := runGenerate(t, t.TempDir(), "--mode", "sentry") + if err == nil { + t.Fatal("the command accepted a kind of node it declares no defaults for, so every value in " + + "the file was compared against nothing") + } + if !strings.Contains(err.Error(), "sentry") { + t.Errorf("the refusal says %q, and it has to name what was given", err) + } + }) +} + +// TestGenerateNamesTheHomeVariableThatWorks holds the message to the resolver. +// +// The refusal tells an operator which variable sets the home, and a name that does not match what the +// resolver reads sends them to change something with no effect. +func TestGenerateNamesTheHomeVariableThatWorks(t *testing.T) { + _, err := runGenerate(t, "", "--mode", "validator") + if err == nil { + t.Fatal("no refusal to read") + } + if !strings.Contains(err.Error(), theVariableThatSetsTheHome()) { + t.Errorf("the refusal says %q and the resolver reads %s, so an operator following it sets a "+ + "variable nothing looks at", err, theVariableThatSetsTheHome()) + } +} diff --git a/cmd/seid/cmd/configmanager/nodefiles.go b/cmd/seid/cmd/configmanager/nodefiles.go new file mode 100644 index 0000000000..2d786963b1 --- /dev/null +++ b/cmd/seid/cmd/configmanager/nodefiles.go @@ -0,0 +1,129 @@ +// Reading the files a node already has, without the boot's handler. +// +// The handler that builds the boot's source generates config.toml and app.toml when they are absent, and +// copies configuration values into flags. A command that answers a question about those files must do +// neither, so it reads them here instead. + +package configmanager + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path" + "path/filepath" + "strings" + + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" + "github.com/spf13/cobra" + "github.com/spf13/pflag" + "github.com/spf13/viper" +) + +// theHomeThisCommandRuns resolves the home directory this command was given, and refuses an empty one. +// +// Every path a command reads joins the home. An empty one leaves those paths relative, so a read lands in +// ./config under whatever directory the command was run from, which is some other node's files. Answering +// for the wrong node is worse than not answering, so the two are resolved together and no caller can hold +// a home without having checked it. +func theHomeThisCommandRuns(cmd *cobra.Command) (string, error) { + home, err := resolveHomeDir(cmd) + if err != nil { + return "", fmt.Errorf("resolve the home directory: %w", err) + } + if home == "" { + return "", fmt.Errorf("no home directory is set, so the files read here would be whichever ones "+ + "the working directory holds. Pass --home, or set %s", theVariableThatSetsTheHome()) + } + return home, nil +} + +// theVariableThatSetsTheHome names the environment variable the home resolves from. +// +// Derived from the running binary the same way the resolver derives it, so a message naming it cannot +// drift from the name that actually works. +func theVariableThatSetsTheHome() string { + exe, err := os.Executable() + if err != nil { + return "the home variable for this binary" + } + return strings.ToUpper(path.Base(exe)) + "_HOME" +} + +// theNodesOwnConfiguration reads the node's own configuration file into the struct a boot decodes it into. +// +// Decoded rather than read key by key, so every caller sees the same shape a boot sees. A boot unmarshals +// this file over the same defaults, so a key stated with nothing after it arrives empty and an absent key +// keeps the default, which is what a boot runs with. +// +// A file that is not there is the only absence. Every other failure is a file somebody wrote that a boot +// does not start on, so answering with defaults would describe a node that cannot boot. +func theNodesOwnConfiguration(home string) (*tmcfg.Config, error) { + cfg := tmcfg.DefaultConfig() + v := viper.New() + v.SetConfigFile(filepath.Join(home, "config", "config.toml")) + switch err := v.ReadInConfig(); { + case errors.Is(err, fs.ErrNotExist): + return cfg, nil + case err != nil: + return nil, err + } + if err := v.Unmarshal(cfg); err != nil { + return nil, err + } + return cfg, nil +} + +// startCommandName is the subcommand whose flags answer a key a file leaves out. +const startCommandName = "start" + +// theSourceThisNodeWouldBuild returns what this node answers for a key looked up by name, without booting. +// +// A boot binds its start command's flags into the source it builds and then reads app.toml over them, so a +// key with a flag of its own is answered by that flag's default whether the file mentions it or not. The +// same two, in the same order, so a written value outranks a flag's default here as it does there. +// +// A file that is not there leaves the flag defaults answering, which is what a node in that state runs +// until a boot generates one. Every other failure is a file a boot does not start on. +func theSourceThisNodeWouldBuild(cmd *cobra.Command, home string) (*viper.Viper, error) { + set, err := theStartCommandsFlags(cmd) + if err != nil { + return nil, err + } + v := viper.New() + if err := v.BindPFlags(set); err != nil { + return nil, err + } + v.SetConfigFile(filepath.Join(home, "config", "app.toml")) + switch err := v.ReadInConfig(); { + case errors.Is(err, fs.ErrNotExist): + return v, nil + case err != nil: + return nil, err + } + return v, nil +} + +// theStartCommandsFlags returns the flags this binary's start command carries. +// +// Found on the root by name rather than built here, so they are the flags this binary ships and a flag +// added to the start command is carried without anything else changing. +// +// Not found is refused. Without these a key whose only answer is a flag's default reads as unanswered, and +// a caller writing a file from that would leave the key out and move it to its declared value. Pruning is +// the one to picture: the flag prunes and the declaration keeps everything, so the file would silently +// stop a node pruning. +func theStartCommandsFlags(cmd *cobra.Command) (*pflag.FlagSet, error) { + for _, sub := range cmd.Root().Commands() { + if sub.Name() != startCommandName { + continue + } + set := pflag.NewFlagSet(startCommandName, pflag.ContinueOnError) + set.AddFlagSet(sub.Flags()) + set.AddFlagSet(sub.PersistentFlags()) + return set, nil + } + return nil, fmt.Errorf("this binary has no %q command, so the defaults its flags carry cannot be "+ + "read and a key answered only by one would read as answered by nothing", startCommandName) +} diff --git a/cmd/seid/cmd/configmanager/tendermint_copy.go b/cmd/seid/cmd/configmanager/tendermint_copy.go index 32bc3200bd..b9baf02bc5 100644 --- a/cmd/seid/cmd/configmanager/tendermint_copy.go +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -135,7 +135,21 @@ func join(path, field string) string { // it did not move. That is the same statement as a key an operator wrote and got, produced by having read // nothing. func describe(cfg *tmcfg.Config, keys []string) (values map[string]string, unread []string, err error) { - values = map[string]string{} + held, unread, err := whatEachKeyHolds(cfg, keys) + values = make(map[string]string, len(held)) + for key, v := range held { + values[key] = fmt.Sprint(v) + } + return values, unread, err +} + +// whatEachKeyHolds reads the value a node's configuration holds for each key, and names the keys that are +// not in it. +// +// The value as the struct holds it. A caller writing one into a file needs the type the key carries, and a +// number rendered as text reaches its setting as a zero. +func whatEachKeyHolds(cfg *tmcfg.Config, keys []string) (values map[string]any, unread []string, err error) { + values = map[string]any{} if cfg == nil { return values, keys, fmt.Errorf("no configuration to read") } @@ -151,7 +165,7 @@ func describe(cfg *tmcfg.Config, keys []string) (values map[string]string, unrea unread = append(unread, key) continue } - values[key] = fmt.Sprint(v) + values[key] = v } sort.Strings(unread) return values, unread, nil diff --git a/cmd/seid/cmd/generate_through_root_test.go b/cmd/seid/cmd/generate_through_root_test.go new file mode 100644 index 0000000000..64d5f74de6 --- /dev/null +++ b/cmd/seid/cmd/generate_through_root_test.go @@ -0,0 +1,237 @@ +package cmd + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "sort" + "strings" + "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/config/seitoml" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + svrcmd "github.com/sei-protocol/sei-chain/sei-cosmos/server/cmd" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// TestANodeStartedFromTheGeneratedFileRunsWhatItRanBefore is what the command is for. +// +// A node under this manager answers every declared key from its sei.toml, so a file that states too +// little moves settings to their declared value. That is the whole risk of handing an operator a +// generated file, and it is one measurement: read what the node runs, write the file, start the node +// against it, and read again. +// +// Both deliveries, because they are read in different places and a file can be right about one of them. +func TestANodeStartedFromTheGeneratedFileRunsWhatItRanBefore(t *testing.T) { + configtest.Isolate(t) + home := configtest.NewHome(t) + if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + before := whatTheNodeAnswers(t, home.Root) + + out, err := runGenerateThroughRoot(t, home.Root, "--mode", "validator") + if err != nil { + t.Fatalf("generate was refused: %v\n%s", err, out) + } + seiToml := filepath.Join(home.Root, "config", "sei.toml") + if err := os.WriteFile(seiToml, []byte(out), 0o600); err != nil { + t.Fatalf("write %s: %v", seiToml, err) + } + + after := whatTheNodeAnswers(t, home.Root) + + var moved, lost []string + for key, was := range before.lookup { + now, answers := after.lookup[key] + switch { + case !answers: + lost = append(lost, key) + case fmt.Sprint(was) != fmt.Sprint(now): + moved = append(moved, fmt.Sprintf("%s was %v and is %v", key, was, now)) + } + } + sort.Strings(moved) + sort.Strings(lost) + for _, line := range moved { + t.Errorf("a key a lookup delivers moved: %s. The file this command wrote does not state what the "+ + "node ran, so an operator adopting it changes a setting they did not decide to change", line) + } + for _, key := range lost { + t.Errorf("%s was answered before the file and is answered by nothing after it, so its reader now "+ + "holds a default of its own where the node had a value", key) + } + + for key, was := range before.decoded { + if now := after.decoded[key]; was != now { + t.Errorf("a key a decode delivers moved: %s was %q and is %q", key, was, now) + } + } + + if !t.Failed() { + t.Logf("%d keys a lookup answers and %d a decode delivers, all unchanged across the file", + len(before.lookup), len(before.decoded)) + } +} + +// TestTheGeneratedFileStatesTheKeysThatDivergeAndNoOthers holds which keys the command writes. +// +// A file stating fewer keys than these moves a setting, and the test above catches that by starting a +// node. This catches the other direction, which that one cannot: a line stating what the declaration +// already states changes nothing, so a file full of them passes a round trip while telling an operator +// that two hundred settings were decisions. +// +// Measured against the record for every kind of node, because the command derives the set from the files +// and the record derives it from a boot. +func TestTheGeneratedFileStatesTheKeysThatDivergeAndNoOthers(t *testing.T) { + configtest.Isolate(t) + home := configtest.NewHome(t) + if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + whatTheNodeAnswers(t, home.Root) + + for _, mode := range registry.Modes() { + t.Run(string(mode), func(t *testing.T) { + out, err := runGenerateThroughRoot(t, home.Root, "--mode", string(mode)) + if err != nil { + t.Fatalf("generate was refused: %v\n%s", err, out) + } + stated := whatTheFileStates(t, out) + + want := divergences[mode] + + for key := range stated { + if _, records := want[key]; !records { + t.Errorf("the file states %s and nothing records it as diverging, so either it "+ + "states a key that changes nothing or a divergence is unrecorded", key) + } + } + for key := range want { + if _, states := stated[key]; !states { + t.Errorf("%s is recorded as diverging and the file does not state it, so a node "+ + "adopting the file moves that setting to its declared value", key) + } + } + }) + } +} + +// nodeAnswers is what a node holds, read the way each delivery is read. +type nodeAnswers struct { + lookup map[string]any + decoded map[string]string +} + +// whatTheNodeAnswers starts a node in this home and reads every declared key off it. +// +// Booted twice for the reason the agreement measurement is: the first writes the files, and a flag bound +// to a key the writer sets overwrites that value before anything reads it back. +func whatTheNodeAnswers(t *testing.T, home string) nodeAnswers { + t.Helper() + var ctx *server.Context + for boot := 0; boot < 2; boot++ { + cmd := server.StartCmd(nil, home, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home); err != nil { + t.Fatalf("set --home: %v", err) + } + got, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("boot %d was refused: %v", boot+1, err) + } + ctx = got + } + + decoded := map[string]bool{} + for _, key := range keysADecodeDelivers() { + decoded[key] = true + } + lookup := map[string]any{} + for _, key := range registry.Keys() { + if decoded[key] { + continue + } + if answer := ctx.Viper.Get(key); answer != nil { + lookup[key] = answer + } + } + return nodeAnswers{ + lookup: lookup, + decoded: configmanager.DescribeForTest(t, ctx.Config, keysADecodeDelivers()), + } +} + +// whatTheFileStates returns the keys a rendered sei.toml states, leaving out the two the file itself +// carries about its own schema and the kind of node. +func whatTheFileStates(t *testing.T, body string) map[string]any { + t.Helper() + file, err := seitoml.Parse(strings.NewReader(body)) + if err != nil { + t.Fatalf("the command rendered a file this binary cannot read: %v\n%s", err, body) + } + values, err := file.Values() + if err != nil { + t.Fatalf("read the rendered file's values: %v", err) + } + return values +} + +// runGenerateThroughRoot runs `config generate` the way an operator runs it. +// +// Through the real root command, because the command reads the start command's flags off the root to +// answer what a node runs. Built on its own it has no siblings and would refuse. +func runGenerateThroughRoot(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 + root.SetArgs(append([]string{"config", "generate", "--home", home}, extraArgs...)) + err := svrcmd.Execute(root, home) + return out.String(), err +} + +// TestGenerateWritesWhereABootReadsIt covers the flag that places the file. +// +// Printing is the default and a printed file is not in use, so the one thing that makes the command +// finish an operator's task is the file landing where the boot looks for it. +func TestGenerateWritesWhereABootReadsIt(t *testing.T) { + configtest.Isolate(t) + home := configtest.NewHome(t) + if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + before := whatTheNodeAnswers(t, home.Root) + + out, err := runGenerateThroughRoot(t, home.Root, "--mode", "validator", "--write") + if err != nil { + t.Fatalf("generate was refused: %v\n%s", err, out) + } + + path := filepath.Join(home.Root, "config", "sei.toml") + if _, err := os.Stat(path); err != nil { + t.Fatalf("the command reported writing a file and %s is not there: %v\n%s", path, err, out) + } + after := whatTheNodeAnswers(t, home.Root) + for key, was := range before.lookup { + if now, answers := after.lookup[key]; !answers || fmt.Sprint(was) != fmt.Sprint(now) { + t.Errorf("%s was %v and is %v after the file the command placed, so the file it writes is "+ + "not the file it prints", key, was, now) + } + } + + // Run again over the file it just wrote. An operator who repeats the command has a file they did not + // decide to replace. + second, err := runGenerateThroughRoot(t, home.Root, "--mode", "validator", "--write") + if err == nil { + t.Errorf("the second run replaced the file the first one wrote:\n%s", second) + } +} diff --git a/cmd/seid/cmd/root.go b/cmd/seid/cmd/root.go index 9ca0dfef62..30378e3105 100644 --- a/cmd/seid/cmd/root.go +++ b/cmd/seid/cmd/root.go @@ -133,6 +133,7 @@ func initRootCmd( // node's sei.toml and writes nothing. configCmd := config.Cmd() configCmd.AddCommand(configmanager.CheckCmd()) + configCmd.AddCommand(configmanager.GenerateCmd()) rootCmd.AddCommand( InitCmd(app.ModuleBasics, app.DefaultNodeHome), From c67c28803c1986f4822ca1f7717496910178cfb8 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 3 Sep 2026 12:48:45 -0700 Subject: [PATCH 3/3] fix(config): refuse to write a file a boot would ignore, and measure delivery The round trip verifying this command was passing on a node that delivered nothing. A boot refuses a sei.toml whose kind of node disagrees with the kind the node runs as, and it refuses the whole file. The measurement wrote a file for a validator against a node whose own file recorded a full node, so nothing was installed, every key read the same before and after, and the check held for any file at all. Found by breaking the writer so it stated declared values instead of the node's, and getting a pass. The node's own configuration file records the kind, so the disagreement is answerable before anything is written. Refused now, with the shared answer rather than a second copy of the rule, so the one pairing that is not a disagreement stays accepted: the kind that keeps every version of history has no name in that file. A home no node was created in is refused as well. Both files are absent there, so every value would come from a flag's default and the file would describe this binary while looking like a node's configuration. The measurements now prepare a node that runs as the kind they generate for, and let the boot write both files rather than rendering a third. The two writers in this binary disagree on several keys, so a rendered file produces a node matching neither and the divergence record would be measuring the test. Verified by mutation on a boot-generated node and a hand-tuned one, each caught by both: stating declared values instead of the node's, stating nothing, never reading app.toml, never reading config.toml. The kind guard is verified in both directions, including a direct comparison that would refuse an archive node. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/configmanager/generate.go | 60 +++- cmd/seid/cmd/configmanager/generate_test.go | 36 ++- cmd/seid/cmd/generate_through_root_test.go | 296 ++++++++++++++++++-- 3 files changed, 358 insertions(+), 34 deletions(-) diff --git a/cmd/seid/cmd/configmanager/generate.go b/cmd/seid/cmd/configmanager/generate.go index 58a0abed7e..f634fdaa30 100644 --- a/cmd/seid/cmd/configmanager/generate.go +++ b/cmd/seid/cmd/configmanager/generate.go @@ -1,7 +1,9 @@ package configmanager import ( + "errors" "fmt" + "io/fs" "os" "path/filepath" "sort" @@ -11,6 +13,7 @@ import ( "github.com/sei-protocol/sei-chain/config/registry" "github.com/sei-protocol/sei-chain/config/seitoml" + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" ) // flagGenerateMode names the kind of node the written values resolve for. @@ -70,7 +73,17 @@ func GenerateCmd() *cobra.Command { if err != nil { return err } - running, err := whatThisNodeAlreadyRuns(cmd, home) + if err := theHomeHoldsANodeToDescribe(home); err != nil { + return err + } + own, err := theNodesOwnConfiguration(home) + if err != nil { + return fmt.Errorf("read this node's own configuration: %w", err) + } + if err := theKindThisNodeAlreadyRuns(mode, own); err != nil { + return err + } + running, err := whatThisNodeAlreadyRuns(cmd, home, own) if err != nil { return err } @@ -133,15 +146,11 @@ func modesInOrder() string { // // A key nothing answers is absent from the result rather than present and empty. Its reader holds a // default of its own, and a caller cannot tell an unanswered key from one answered with a zero. -func whatThisNodeAlreadyRuns(cmd *cobra.Command, home string) (map[string]any, error) { +func whatThisNodeAlreadyRuns(cmd *cobra.Command, home string, own *tmcfg.Config) (map[string]any, error) { source, err := theSourceThisNodeWouldBuild(cmd, home) if err != nil { return nil, err } - own, err := theNodesOwnConfiguration(home) - if err != nil { - return nil, fmt.Errorf("read this node's own configuration: %w", err) - } _, ownedByADecode := registry.ResolvedAndOwnedByDecodedSections(registry.Resolved{}) decoded, unread, err := whatEachKeyHolds(own, ownedByADecode) @@ -169,6 +178,45 @@ func whatThisNodeAlreadyRuns(cmd *cobra.Command, home string) (map[string]any, e return running, nil } +// theHomeHoldsANodeToDescribe refuses a home that no node has been created in. +// +// Both files are absent in that case, so every value would come from a flag's default and the file +// written would describe this binary rather than a node. It would still look like a node's +// configuration, and the kind recorded in it would be whichever kind was asked for. +// +// The node's own configuration file is the one checked, because a boot generates the other one and a +// home can legitimately hold only the first. +func theHomeHoldsANodeToDescribe(home string) error { + path := filepath.Join(home, "config", "config.toml") + switch _, err := os.Stat(path); { + case errors.Is(err, fs.ErrNotExist): + return fmt.Errorf("%s is not there, so this home holds no node to describe and every value "+ + "written would come from this binary rather than from anything a node runs", path) + case err != nil: + return err + } + return nil +} + +// theKindThisNodeAlreadyRuns refuses a kind of node that disagrees with the one this node's own +// configuration file records. +// +// A boot delivers nothing at all from a file whose kind disagrees with the kind the node runs as, so a +// file written under the wrong kind is not a partly-right file. Every declared key goes on reading as it +// did, and an operator holds a file they have every reason to believe is in use. Refused here, where it +// costs a message. +// +// One pairing is not a disagreement, and the shared answer is used rather than a second copy of the rule: +// the kind that keeps every version of history has no name in the node's own file. +func theKindThisNodeAlreadyRuns(mode registry.Mode, own *tmcfg.Config) error { + if own == nil || !modesDisagree(string(mode), own.Mode) { + return nil + } + return fmt.Errorf("this node's own configuration file records it running as %q and --%s says %q. A "+ + "boot delivers nothing from a file that disagrees, so the file written here would leave every "+ + "declared key reading as it does now", own.Mode, flagGenerateMode, mode) +} + // whatTheDeclarationDoesNotAlreadySay returns the keys this node answers differently from the declaration. // // Compared as text, because the two sides carry different Go types for the same key often enough that diff --git a/cmd/seid/cmd/configmanager/generate_test.go b/cmd/seid/cmd/configmanager/generate_test.go index d33301cf4c..acb25845b1 100644 --- a/cmd/seid/cmd/configmanager/generate_test.go +++ b/cmd/seid/cmd/configmanager/generate_test.go @@ -29,6 +29,24 @@ func runGenerate(t *testing.T, home string, args ...string) (string, error) { return out.String(), err } +// aHomeHoldingANode returns a home with the one file this command requires, recording the kind of node +// the file's own default records. +// +// Enough to get past the checks that come before the one under test, and no more. A full node's own file +// is what a bare default carries, so the kind passed alongside is that one. +func aHomeHoldingANode(t *testing.T) string { + t.Helper() + home := t.TempDir() + if err := os.MkdirAll(filepath.Join(home, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + path := filepath.Join(home, "config", "config.toml") + if err := os.WriteFile(path, []byte("mode = \"full\"\n"), 0o600); err != nil { + t.Fatalf("write %s: %v", path, err) + } + return home +} + // TestGenerateRefusesWhatItCannotAnswerFrom covers the states where writing a file would be worse than // writing none. // @@ -47,12 +65,20 @@ func TestGenerateRefusesWhatItCannotAnswerFrom(t *testing.T) { } }) - t.Run("no start command to read flag defaults off", func(t *testing.T) { - home := t.TempDir() - if err := os.MkdirAll(filepath.Join(home, "config"), 0o750); err != nil { - t.Fatalf("mkdir: %v", err) + t.Run("this home holds no node", func(t *testing.T) { + _, err := runGenerate(t, t.TempDir(), "--mode", "validator") + if err == nil { + t.Fatal("the command described a home no node was created in, so every value in the file " + + "came from this binary rather than from a node") + } + if !strings.Contains(err.Error(), "config.toml") { + t.Errorf("the refusal says %q, and it has to name the file it looked for", err) } - _, err := runGenerate(t, home, "--mode", "validator") + }) + + t.Run("no start command to read flag defaults off", func(t *testing.T) { + home := aHomeHoldingANode(t) + _, err := runGenerate(t, home, "--mode", "full") if err == nil { t.Fatal("the command answered without the start command's flags, so a key answered only by " + "a flag's default read as answered by nothing and the file would leave it out") diff --git a/cmd/seid/cmd/generate_through_root_test.go b/cmd/seid/cmd/generate_through_root_test.go index 64d5f74de6..ee3abeac72 100644 --- a/cmd/seid/cmd/generate_through_root_test.go +++ b/cmd/seid/cmd/generate_through_root_test.go @@ -29,23 +29,20 @@ import ( // Both deliveries, because they are read in different places and a file can be right about one of them. func TestANodeStartedFromTheGeneratedFileRunsWhatItRanBefore(t *testing.T) { configtest.Isolate(t) - home := configtest.NewHome(t) - if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { - t.Fatalf("mkdir: %v", err) - } + home := aNodeRunningAs(t, registry.ModeValidator) - before := whatTheNodeAnswers(t, home.Root) + before := whatTheNodeAnswers(t, home) - out, err := runGenerateThroughRoot(t, home.Root, "--mode", "validator") + out, err := runGenerateThroughRoot(t, home, "--mode", "validator") if err != nil { t.Fatalf("generate was refused: %v\n%s", err, out) } - seiToml := filepath.Join(home.Root, "config", "sei.toml") + seiToml := filepath.Join(home, "config", "sei.toml") if err := os.WriteFile(seiToml, []byte(out), 0o600); err != nil { t.Fatalf("write %s: %v", seiToml, err) } - after := whatTheNodeAnswers(t, home.Root) + after := whatTheNodeAnswers(t, home) var moved, lost []string for key, was := range before.lookup { @@ -91,15 +88,14 @@ func TestANodeStartedFromTheGeneratedFileRunsWhatItRanBefore(t *testing.T) { // and the record derives it from a boot. func TestTheGeneratedFileStatesTheKeysThatDivergeAndNoOthers(t *testing.T) { configtest.Isolate(t) - home := configtest.NewHome(t) - if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { - t.Fatalf("mkdir: %v", err) - } - whatTheNodeAnswers(t, home.Root) for _, mode := range registry.Modes() { t.Run(string(mode), func(t *testing.T) { - out, err := runGenerateThroughRoot(t, home.Root, "--mode", string(mode)) + // A node of this kind, because the command refuses a kind the node's own file contradicts. + home := aNodeRunningAs(t, mode) + whatTheNodeAnswers(t, home) + + out, err := runGenerateThroughRoot(t, home, "--mode", string(mode)) if err != nil { t.Fatalf("generate was refused: %v\n%s", err, out) } @@ -123,6 +119,64 @@ func TestTheGeneratedFileStatesTheKeysThatDivergeAndNoOthers(t *testing.T) { } } +// theTendermintKind is the kind of node the node's own configuration file records for a declared kind. +// +// The kind that keeps every version of history has no name in that file. The command that writes it +// writes the query-serving name instead, and the delivery accepts that one pairing. +func theTendermintKind(mode registry.Mode) string { + if mode == registry.ModeArchive { + return string(registry.ModeFull) + } + return string(mode) +} + +// aNodeRunningAs prepares a home whose own configuration file records this kind of node. +// +// The boot writes both files and then one line is replaced, rather than a file being rendered here. The +// two writers in this binary disagree on several keys, and rendering a third file would produce a node +// matching neither, so a measurement against what either writer produces would be measuring this helper. +// +// The kind matters because the delivery refuses a sei.toml whose kind disagrees with the kind the node +// runs as, and it refuses the whole file. A measurement over a node whose file says something else reads +// the same before and after, and holds for any file at all. +func aNodeRunningAs(t *testing.T, mode registry.Mode) string { + t.Helper() + home := configtest.NewHome(t) + if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + whatTheNodeAnswers(t, home.Root) + + path := filepath.Join(home.Root, "config", "config.toml") + body, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read the node's configuration file: %v", err) + } + lines := strings.Split(string(body), "\n") + replaced := false + for i, line := range lines { + // At the file's root, so the search stops at the first table header. A key of this name inside a + // section is a different setting. + if strings.HasPrefix(strings.TrimSpace(line), "[") { + break + } + if !strings.HasPrefix(strings.TrimSpace(line), "mode ") { + continue + } + lines[i] = "mode = \"" + theTendermintKind(mode) + "\"" + replaced = true + break + } + if !replaced { + t.Fatal("the node's configuration file records no kind of node at its root, so this helper " + + "cannot set one and every test using it would run against whatever kind the writer chose") + } + if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")), 0o600); err != nil { + t.Fatalf("write the node's configuration file: %v", err) + } + return home.Root +} + // nodeAnswers is what a node holds, read the way each delivery is read. type nodeAnswers struct { lookup map[string]any @@ -205,22 +259,19 @@ func runGenerateThroughRoot(t *testing.T, home string, extraArgs ...string) (str // finish an operator's task is the file landing where the boot looks for it. func TestGenerateWritesWhereABootReadsIt(t *testing.T) { configtest.Isolate(t) - home := configtest.NewHome(t) - if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { - t.Fatalf("mkdir: %v", err) - } - before := whatTheNodeAnswers(t, home.Root) + home := aNodeRunningAs(t, registry.ModeValidator) + before := whatTheNodeAnswers(t, home) - out, err := runGenerateThroughRoot(t, home.Root, "--mode", "validator", "--write") + out, err := runGenerateThroughRoot(t, home, "--mode", "validator", "--write") if err != nil { t.Fatalf("generate was refused: %v\n%s", err, out) } - path := filepath.Join(home.Root, "config", "sei.toml") + path := filepath.Join(home, "config", "sei.toml") if _, err := os.Stat(path); err != nil { t.Fatalf("the command reported writing a file and %s is not there: %v\n%s", path, err, out) } - after := whatTheNodeAnswers(t, home.Root) + after := whatTheNodeAnswers(t, home) for key, was := range before.lookup { if now, answers := after.lookup[key]; !answers || fmt.Sprint(was) != fmt.Sprint(now) { t.Errorf("%s was %v and is %v after the file the command placed, so the file it writes is "+ @@ -230,8 +281,207 @@ func TestGenerateWritesWhereABootReadsIt(t *testing.T) { // Run again over the file it just wrote. An operator who repeats the command has a file they did not // decide to replace. - second, err := runGenerateThroughRoot(t, home.Root, "--mode", "validator", "--write") + second, err := runGenerateThroughRoot(t, home, "--mode", "validator", "--write") if err == nil { t.Errorf("the second run replaced the file the first one wrote:\n%s", second) } } + +// theTunedSettings are values written into a node's own files by hand, one per shape a writer has to +// render and per delivery it has to read. +// +// Chosen to differ from both writers. A value matching either one would be stated or left out for a +// reason that has nothing to do with an operator having set it, so the test would pass without covering +// what it claims. +// +// The retention and connection rows also agree with neither: the declaration and the generated file both +// state something else again, so each of these is a third value. +var theTunedSettings = []struct { + file string + section string + key string + written string +}{ + // Answered by a flag's default when no file states it, so this covers a written value outranking one. + {"app.toml", "", "pruning", `"everything"`}, + {"app.toml", "api", "max-open-connections", "4321"}, + {"app.toml", "state-store", "ss-keep-recent", "777"}, + // The keys a decode delivers, read off a struct rather than by name. + {"config.toml", "mempool", "size", "4321"}, + {"config.toml", "p2p", "max-connections", "55"}, + {"config.toml", "rpc", "max-subscription-clients", "33"}, + {"config.toml", "consensus", "create-empty-blocks-interval", `"7s"`}, +} + +// TestANodeWhoseFilesWereTunedByHandRunsWhatItRanBefore is the case the command exists for. +// +// The measurement above starts from files a boot generated, where every value is one writer or the other. +// A node worth running this on is not that node: somebody edited its files, and the values they chose +// agree with neither writer. +// +// Those values are also the ones a divergence record cannot cover, because they are not a property of +// this binary. A key an operator set to the declared value is left out and a key they set to anything +// else is stated, and which keys those are is known only by reading their files. +func TestANodeWhoseFilesWereTunedByHandRunsWhatItRanBefore(t *testing.T) { + configtest.Isolate(t) + home := aNodeRunningAs(t, registry.ModeValidator) + + // Started once so the files exist to edit. What it answers is not measured: the edits below are what + // this node runs, and they are applied before anything reads it. + whatTheNodeAnswers(t, home) + tuneByHand(t, home) + + before := whatTheNodeAnswers(t, home) + for _, setting := range theTunedSettings { + key := setting.key + if setting.section != "" { + key = setting.section + "." + setting.key + } + if _, reads := before.lookup[key]; !reads { + if _, decoded := before.decoded[key]; !decoded { + t.Fatalf("%s reads as answered by nothing after being written into %s, so the edit did "+ + "not take and this test measures a node nobody tuned", key, setting.file) + } + } + } + + out, err := runGenerateThroughRoot(t, home, "--mode", "validator") + if err != nil { + t.Fatalf("generate was refused: %v\n%s", err, out) + } + if err := os.WriteFile(filepath.Join(home, "config", "sei.toml"), []byte(out), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + + // Every edited key has to be stated. Each of them differs from the declaration, so one the file leaves + // out is a setting an operator chose and the node stops running. + stated := whatTheFileStates(t, out) + for _, setting := range theTunedSettings { + key := setting.key + if setting.section != "" { + key = setting.section + "." + setting.key + } + if _, states := stated[key]; !states { + t.Errorf("%s was written into %s by hand and the generated file does not state it, so the "+ + "node moves to the declared value and loses what an operator chose", key, setting.file) + } + } + + after := whatTheNodeAnswers(t, home) + var moved, lost []string + for key, was := range before.lookup { + now, answers := after.lookup[key] + switch { + case !answers: + lost = append(lost, key) + case fmt.Sprint(was) != fmt.Sprint(now): + moved = append(moved, fmt.Sprintf("%s was %v and is %v", key, was, now)) + } + } + for key, was := range before.decoded { + if now := after.decoded[key]; was != now { + moved = append(moved, fmt.Sprintf("%s was %q and is %q", key, was, now)) + } + } + sort.Strings(moved) + sort.Strings(lost) + for _, line := range moved { + t.Errorf("a tuned node changed across the generated file: %s", line) + } + for _, key := range lost { + t.Errorf("%s was answered before the file and is answered by nothing after it", key) + } + + if !t.Failed() { + t.Logf("%d hand-written values, %d keys a lookup answers and %d a decode delivers, all unchanged", + len(theTunedSettings), len(before.lookup), len(before.decoded)) + } +} + +// tuneByHand writes theTunedSettings into the node's own files, replacing each key's line where it sits. +// +// The line is replaced rather than the file rewritten, because a rewrite from a decoded map is not what an +// operator does and would drop every comment the file ships with, including the ones naming the keys this +// test does not touch. +func tuneByHand(t *testing.T, home string) { + t.Helper() + for _, name := range []string{"app.toml", "config.toml"} { + path := filepath.Join(home, "config", name) + body, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", name, err) + } + lines := strings.Split(string(body), "\n") + + for _, setting := range theTunedSettings { + if setting.file != name { + continue + } + replaced := false + section := "" + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "[") && strings.HasSuffix(trimmed, "]") { + section = strings.Trim(trimmed, "[]") + continue + } + if section != setting.section || !strings.HasPrefix(trimmed, setting.key+" ") { + continue + } + lines[i] = setting.key + " = " + setting.written + replaced = true + break + } + switch { + case replaced: + // A key at the file's root that the generator does not write is the flag-answered case, and + // writing it is what an operator does. Inserted at the top, because a bare key after a table + // header belongs to that table rather than to the root. + case setting.section == "": + lines = append([]string{setting.key + " = " + setting.written}, lines...) + default: + t.Fatalf("%s states no %q under %q, so this test would measure a value nobody wrote. The "+ + "key was renamed, moved section, or is no longer written by the generator", + name, setting.key, setting.section) + } + } + + if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")), 0o600); err != nil { + t.Fatalf("write %s: %v", name, err) + } + } +} + +// TestGenerateRefusesAKindTheNodeContradicts covers the file that is written, adopted, and then ignored. +// +// The delivery refuses a sei.toml whose kind disagrees with the kind the node runs as, and it refuses the +// whole file rather than the keys that differ. So a file written under the wrong kind leaves every +// declared key reading as it did, on a node whose operator has every reason to believe otherwise. The +// refusal costs a message instead. +func TestGenerateRefusesAKindTheNodeContradicts(t *testing.T) { + configtest.Isolate(t) + + t.Run("a kind the node's own file contradicts", func(t *testing.T) { + home := aNodeRunningAs(t, registry.ModeValidator) + out, err := runGenerateThroughRoot(t, home, "--mode", "full", "--write") + if err == nil { + t.Fatalf("the command wrote a file for a full node against one running as a validator, and a "+ + "boot delivers nothing from it:\n%s", out) + } + if _, err := os.Stat(filepath.Join(home, "config", "sei.toml")); !os.IsNotExist(err) { + t.Error("the refused run left a sei.toml behind, so the next boot reads a file this command " + + "declined to stand behind") + } + }) + + // The one pairing that is not a disagreement. Refusing it would leave the kind that keeps every + // version of history with no way to run this command at all, because that kind has no name in the + // node's own configuration file. + t.Run("the kind that keeps all history, whose file says it serves queries", func(t *testing.T) { + home := aNodeRunningAs(t, registry.ModeArchive) + if _, err := runGenerateThroughRoot(t, home, "--mode", "archive"); err != nil { + t.Errorf("the command refused an archive node: %v. Its own file records the query-serving "+ + "kind because that is the only name it has, and the delivery accepts the pair", err) + } + }) +}