From 99ee69e1bf4e31a2994a94c89f18c13974541812 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 27 Aug 2026 13:04:52 -0700 Subject: [PATCH 01/10] feat(config): deliver sei.toml into the struct a whole-file reader decoded A node's own configuration file is read into a struct by the boot's handler before this runs, and nothing consults the key-value source for those settings afterwards. So the install of the previous change is no delivery at all for them: the values have to be decoded into that struct instead. Decoded into a deep copy of the live configuration and published by replacing it, rather than decoded in place. What a decoder writes depends on what the target already holds, so the copy has to be of the node's own configuration and not a fresh one. One section at a time. A decode is all or nothing for whatever it is handed, so a single value a decoder refuses would otherwise cost every key in the file rather than the keys of the section it appeared in. An operator who fixes one setting and mistypes another has to end up with the first one applied. The resolved log level is applied before any of the reporting, because a refusal is reported at a level an operator may have raised the threshold above, and doing it after would mean the one setting somebody changes in order to see a refusal is the setting a refusal suppresses. One report corrected. The line saying the file supplied no declared value describes the lookup delivery alone, and both run in one pass, so an operator whose file moved a setting through a decode was told the file supplied nothing a few lines later. It is now scoped to the delivery it describes. Verified by mutation, and the first attempt at that verification was a false pass worth recording: the pattern had not applied, so an unmutated run was read as proof. With the mutation genuinely in place nothing failed, which is how the new test came to be written. Co-Authored-By: Claude Opus 5 (1M context) --- .../cmd/configmanager/decode_report_test.go | 56 +++ cmd/seid/cmd/configmanager/install.go | 86 +--- cmd/seid/cmd/configmanager/tendermint.go | 207 ++++++++++ cmd/seid/cmd/configmanager/tendermint_copy.go | 335 +++++++++++++++ .../cmd/configmanager/tendermint_copy_test.go | 115 ++++++ cmd/seid/cmd/node_agreement_test.go | 135 +++++++ cmd/seid/cmd/node_delivery_test.go | 382 ++++++++++++++++++ config/registry/delivery.go | 24 +- config/registry/delivery_test.go | 29 +- 9 files changed, 1276 insertions(+), 93 deletions(-) create mode 100644 cmd/seid/cmd/configmanager/decode_report_test.go create mode 100644 cmd/seid/cmd/configmanager/tendermint.go create mode 100644 cmd/seid/cmd/configmanager/tendermint_copy.go create mode 100644 cmd/seid/cmd/configmanager/tendermint_copy_test.go create mode 100644 cmd/seid/cmd/node_agreement_test.go create mode 100644 cmd/seid/cmd/node_delivery_test.go diff --git a/cmd/seid/cmd/configmanager/decode_report_test.go b/cmd/seid/cmd/configmanager/decode_report_test.go new file mode 100644 index 0000000000..80da39930b --- /dev/null +++ b/cmd/seid/cmd/configmanager/decode_report_test.go @@ -0,0 +1,56 @@ +package configmanager + +import ( + "bytes" + "fmt" + "log/slog" + "strconv" + "testing" + + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// TestEveryDeclaredKeyOfADecodedSectionIsDelivered is what makes sei.toml the configuration for the +// sections a reader decodes whole. +// +// The install cannot reach them, so a value only arrives through this decode. Every key the resolution +// answered has to arrive, not only the ones a source wrote, or a key sei.toml leaves out would keep +// whatever config.toml said and the file would be a patch rather than the configuration. +func TestEveryDeclaredKeyOfADecodedSectionIsDelivered(t *testing.T) { + var out bytes.Buffer + log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug})) + + // A node holding a value nobody wrote in sei.toml. + live := tmcfg.DefaultConfig() + live.P2P.MaxConnections = 999 + ctx := &server.Context{Config: live} + + resolved, err := registry.Resolve(registry.ModeFull, registry.Sources{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + forADecode, _ := registry.ResolvedAndOwnedByDecodedSections(resolved) + deliverDecodedSections(ctx, forADecode, log) + + declared := resolved.Values["p2p.max-connections"] + if declared == nil { + t.Fatal("p2p.max-connections declares nothing, so this measures nothing") + } + if got := uint64(ctx.Config.P2P.MaxConnections); got != asUint(t, declared) { + t.Errorf("the node runs a peer ceiling of %d after a resolution that declares %v and a sei.toml "+ + "that mentions nothing, want the declared value. A key the file leaves out has to take the "+ + "declaration, or config.toml is still the configuration", got, declared) + } +} + +// asUint reads a declared numeric value as an unsigned number. +func asUint(t *testing.T, v any) uint64 { + t.Helper() + n, err := strconv.ParseUint(fmt.Sprint(v), 10, 64) + if err != nil { + t.Fatalf("the declared value %v is not a number: %v", v, err) + } + return n +} diff --git a/cmd/seid/cmd/configmanager/install.go b/cmd/seid/cmd/configmanager/install.go index 0bc2e71b1a..5d9270adf7 100644 --- a/cmd/seid/cmd/configmanager/install.go +++ b/cmd/seid/cmd/configmanager/install.go @@ -82,12 +82,17 @@ func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logg "mode", mode, "err", err) return } - // Before the report below, because a refused registration is what makes that one point at the wrong - // file, and an operator reading in order should meet the cause first. + // First, because every report below is a log line and a refusal is reported at a level an operator + // may have raised the threshold above. Doing this after would mean the one setting somebody changes + // in order to see a refusal is the setting a refusal suppresses. + applyResolvedLogLevel(resolved, typed, log) + + // Before the reports it explains, because a refused registration is what makes the next one point at + // the wrong file, and an operator reading in order should meet the cause first. reportWhatThisBinaryCouldNotUse(resolved, log) - // A key nothing declares is the most common thing an operator gets wrong and the only signal they - // have for it. + // After the level, so a file that raises it can report its own mistakes. A key nothing declares is + // the most common thing an operator gets wrong and the only signal they have for it. reportWhatTheFileDidNotReach(resolved, log) reportWhatTheFileSaysTheNodeIs(ctx, mode, log) @@ -96,27 +101,21 @@ func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logg // boot drop to debug everywhere else. On `seid keys list` nobody asked, and a line held above the // operator's own level buries the reports beside it that are actionable. // - // Holding keys back joins them. It is a problem, but not one an operator can act on, and its trigger - // is any sei.toml carrying a [p2p], [mempool] or root key, which is nearly every file somebody would - // write. It keeps its own level on the boot, because that is the one place holding them back changes - // what the node runs. - // // What a refused registration says, and what a key nothing declares says, report everywhere. Both are // things to fix. - said, warned := log.Info, log.Warn + said := log.Info if !runsANode(cmd) { - said, warned = log.Debug, log.Debug + said = log.Debug } - // One read for both halves. A section arriving between two reads would be absent from what is - // reported and present in what is dropped, which is undelivered and unreported at once. - forADecode, ownedByADecode := registry.SuppliedAndOwnedByDecodedSections(resolved) + // One read, and both halves are used: the values a decode has to deliver, and every key those + // sections own so the install below leaves them out. Two reads would let a section arrive between + // them, absent from the delivery and present in what the install drops. + forADecode, ownedByADecode := registry.ResolvedAndOwnedByDecodedSections(resolved) - // Only what the file itself wrote. The supplied set is filled by every channel, and a flag or a - // variable answering one of these keys does reach the node, so reporting it as read-as-it-always-has - // would be false as well as pointed at the wrong file. - heldFromTheFile := whatTheFileWroteForADecode(forADecode, written) - reportWhatThisInstallHoldsBack(heldFromTheFile, warned) + // The second delivery. Their file is read into a struct before this runs and nothing consults the + // source for them afterwards, so the values are decoded into that struct instead. + deliverDecodedSections(ctx, forADecode, log) // Every declared key a lookup reads, whether sei.toml mentioned it or not. There is no case where this // is empty for a reason an operator caused: the paths above already returned for a file that could not @@ -194,55 +193,6 @@ func everyKeyALookupReads(resolved registry.Resolved, ownedByADecode []string) r return out } -// whatTheFileWroteForADecode narrows the decoded sections' supplied values to the keys the file itself -// wrote, sorted. -// -// The supplied set is filled by the file, the environment and the flags alike, and only the file's keys are -// ones this install holds back in a way an operator can act on. A flag answering one of these keys reaches -// the node through the flag, so reporting it as reading the way it always has would be untrue, and naming -// the file for it would be untrue twice. -// -// Matched lower-cased, which is how the resolution matches a file's keys. -func whatTheFileWroteForADecode(bySection map[string]map[string]any, written map[string]any) []string { - inTheFile := make(map[string]bool, len(written)) - for key := range written { - inTheFile[strings.ToLower(key)] = true - } - var keys []string - for _, values := range bySection { - for key := range values { - if inTheFile[strings.ToLower(key)] { - keys = append(keys, key) - } - } - } - sort.Strings(keys) - return keys -} - -// reportWhatThisInstallHoldsBack names the supplied keys this install cannot deliver. -// -// A section whose reader decodes its file whole was read before this ran, so putting a value into the -// source reaches nothing for it. Those keys are left out on purpose. -// -// Left unreported they are invisible. They are absent from what was installed, and they are declared, so -// they are absent from the undeclared keys too. An operator who supplied only such keys would be told -// nothing was supplied while their node ran the old values, which is the failure this whole surface exists -// to remove. -// -// No source is named, because the resolution does not record which one answered. A file, a variable and a -// flag all arrive here as an override, and naming the file for a value an environment variable supplied is -// the same misattribution the undeclared-key report was split apart to end. -func reportWhatThisInstallHoldsBack(keys []string, say func(string, ...any)) { - if len(keys) == 0 { - return - } - shown, omitted := capLoggedItems(keys) - say("sei.toml writes keys whose reader decodes its file whole; this install cannot deliver them "+ - "and they read as they always have", - "count", len(keys), "keys", strings.Join(shown, ","), "omitted", omitted) -} - // reportWhatThisBinaryCouldNotUse names a registration this binary's own source got wrong. // // Not the operator's mistake and nothing they can fix. It reaches them anyway: a refused registration diff --git a/cmd/seid/cmd/configmanager/tendermint.go b/cmd/seid/cmd/configmanager/tendermint.go new file mode 100644 index 0000000000..0904acd347 --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -0,0 +1,207 @@ +package configmanager + +import ( + "cmp" + "fmt" + "log/slog" + "os" + "strings" + + "github.com/spf13/viper" + + "github.com/sei-protocol/seilog" + + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// deliverDecodedSections puts the resolved values of the decoded sections into the node's own +// configuration. +// +// Putting a value into the source a node reads is the whole delivery for a section whose reader looks its +// keys up one at a time. It is no delivery at all for the sections this covers, which the boot's handler +// reads once into a struct before this runs. Those values are decoded into that struct instead, which is +// the same mechanism the handler used and therefore the same casts, the same tags and the same hooks. +// +// Nothing here can stop a node starting, which is the one promise this manager makes. +func deliverDecodedSections(ctx *server.Context, bySection map[string]map[string]any, + log *slog.Logger) { + if len(bySection) == 0 { + return + } + if ctx == nil || ctx.Config == nil { + log.Error("no node configuration to deliver into; every one of these keys reads as it always has", + "sections", len(bySection)) + return + } + + // One section at a time. A decode is all or nothing for whatever it is handed, so a single value a + // decoder refuses would otherwise cost every key in the file rather than the keys of the section it + // appeared in. An operator who fixes one setting and mistypes another has to end up with the first + // one applied. + for _, name := range sortedKeys(bySection) { + deliverOneSection(ctx, name, bySection[name], log) + } +} + +// deliverOneSection decodes one section's resolved values into the node's configuration. +// +// Decoded into a copy of that configuration first, and published by replacing it. A decoder gathers errors +// and keeps going, so a value it refuses partway leaves its target holding some of the new values and some +// of the old, with nothing to compare against and no way back. Rehearsing into a copy of the configuration +// the node already has, rather than into a fresh one, is what makes the rehearsal answer the same question: +// what a decoder writes can depend on what the target already holds, and only a copy holds the same things. +func deliverOneSection(ctx *server.Context, name string, values map[string]any, log *slog.Logger) { + keys := sortedKeys(values) + + source := viper.New() + for key, value := range values { + source.Set(key, value) + } + + // Refused before the decode, because a plain number where a length of time belongs decodes cleanly + // and means nanoseconds. Nothing after this can tell that apart from a value somebody meant. + if bad := refuseWhatDecodesToSomethingElse(ctx.Config, values); len(bad) > 0 { + log.Error("a length of time in this section is written as a plain number, which reads as "+ + "nanoseconds; none of the section is applied and every one of its keys reads as it always has", + "section", name, "written", strings.Join(bad, "; ")) + return + } + + candidate, err := copyNodeConfig(ctx.Config) + if err != nil { + log.Error("cannot copy this node's configuration, so nothing can be delivered into it without "+ + "risking a half-written one; these keys read as they always have", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } + before, readErr := describe(ctx.Config, keys) + + if err := source.Unmarshal(candidate); err != nil { + log.Error("a written value in this section was refused, so none of the section is applied and "+ + "every one of its keys reads as it always has", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } + + *ctx.Config = *candidate + after, afterErr := describe(ctx.Config, keys) + if readErr != nil || afterErr != nil { + // Reported rather than compared. Two unreadable sides look identical, so comparing them would + // say every value matched, which is a statement about nothing produced by reading nothing. + log.Error("this section was applied and what moved cannot be read, so nothing here says which "+ + "settings now differ from the node's own file", "section", name, + "keys", strings.Join(keys, ","), "err", cmp.Or(readErr, afterErr)) + return + } + reportWhatMoved(name, keys, before, after, log) +} + +// copyNodeConfig returns a configuration that holds what this one holds and shares nothing with it. +// +// A shallow copy would share every section, so a decode into the copy would write through to the original +// and a refused value would leave exactly the half-written configuration the copy exists to prevent. This +// copies the top level and every section under it. +// +// Written against the type rather than field by field, so a section added to it is copied without this +// function changing. A field this cannot copy is an error rather than a silent share. +func copyNodeConfig(from *tmcfg.Config) (*tmcfg.Config, error) { + if from == nil { + return nil, fmt.Errorf("no configuration to copy") + } + out := *from + if err := detachSections(&out, from); err != nil { + return nil, err + } + return &out, nil +} + +// reportWhatMoved names every key whose value the delivery changed, and what it changed from. +// +// The node's own configuration file still says what it said, and every tool an operator reaches for reads +// that file: a patch command, a validator, an audit, somebody reading it over their shoulder at three in +// the morning. None of them describes the running node after this. This log line is the only place the two +// can be told apart, so it names the key, what the file gave it and what the node now runs. +// +// Keys that did not move are not reported. An operator who writes the value their file already held has +// changed nothing, and a line saying so buries the ones that did. +func reportWhatMoved(name string, keys []string, before, after map[string]string, log *slog.Logger) { + var moved []string + for _, key := range keys { + if before[key] != after[key] { + moved = append(moved, fmt.Sprintf("%s: %s -> %s", key, before[key], after[key])) + } + } + if len(moved) == 0 { + log.Info("this section's written values match what the node's own file already gave it", + "section", name, "keys", len(keys)) + return + } + log.Info("this section's settings now differ from what the node's own configuration file says", + "section", name, "changed", strings.Join(moved, "; ")) +} + +// logLevelKey is the one delivered setting the struct is not the end of. +const logLevelKey = "log-level" + +// loggerOwnVariable is the environment variable the logger itself reads when it starts. +// +// Not the variable this key answers to in the resolution, which carries the binary's own prefix. Two names +// for one setting, and the older one is read before any of this runs. +const loggerOwnVariable = "SEI_LOG_LEVEL" + +// applyResolvedLogLevel hands a resolved log level to the logger, which the struct alone does not reach. +// +// The boot's handler reads the level off the struct and sets it before any of this runs, so a value that +// only reaches the struct moves a field and changes no logging. A setting that appears to take and does not +// is what this key space exists to remove. +// +// Applied from the resolution rather than after the decode, and before it. Every failure this manager can +// have is a log line, and a refusal is reported at a level an operator may have raised the threshold above. +// Waiting for a successful decode would mean the one setting somebody changes in order to see a refusal is +// the setting a refusal suppresses. +// +// Which value arrives is already decided: the resolution ranks a flag over the environment over the file. +// A level that cannot be read is reported and skipped, and the node keeps the level it had. +func applyResolvedLogLevel(resolved registry.Resolved, typed map[string]string, log *slog.Logger) { + supplied := false + for _, key := range resolved.Overrides { + if key == logLevelKey { + supplied = true + } + } + if !supplied { + return + } + + // The logger reads a variable of its own at start-up, under a name that is not the one this key + // answers to, and the boot's own handler steps aside when it is set: a flag beats it and a file does + // not. Applying here regardless would put the file above it, so an operator who exported a level and + // then adopted this file would find the level they exported ignored. A typed flag still wins, which is + // the order that was already there. + if _, fromFlag := flagValues(typed)[logLevelKey]; !fromFlag { + if os.Getenv(loggerOwnVariable) != "" { + log.Info("a log level is set in the environment under the logger's own variable, which the "+ + "node already applied; the level this file supplies is not used", + "variable", loggerOwnVariable, "ignored", resolved.Values[logLevelKey]) + return + } + } + text, isText := resolved.Values[logLevelKey].(string) + if !isText { + log.Error("the resolved log level is not text; the node keeps the level it already had", + "value", resolved.Values[logLevelKey]) + return + } + var level slog.Level + if err := level.UnmarshalText([]byte(text)); err != nil { + log.Error("the resolved log level cannot be read; the node keeps the level it already had", + "level", text, "err", err) + return + } + seilog.SetDefaultLevel(level, true) + // That set every logger in the process, this one included, so the floor goes back on. + keepOwnReportingVisible() + log.Info("resolved log level applied", "level", text) +} diff --git a/cmd/seid/cmd/configmanager/tendermint_copy.go b/cmd/seid/cmd/configmanager/tendermint_copy.go new file mode 100644 index 0000000000..e878e80649 --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -0,0 +1,335 @@ +package configmanager + +import ( + "fmt" + "reflect" + "sort" + "strings" + "time" + + "github.com/go-viper/mapstructure/v2" + + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// detachSections makes a copy hold what the original holds without sharing anything a decode can write +// through. +// +// A copy of the struct alone shares every section, every list and every map it points at. A decoder writes +// a list into the array its target already holds, so a shared one means the rehearsal edits the original +// and a refused value leaves exactly the half-written configuration the copy exists to prevent. +// +// Walked over the type rather than field by field, so a section or a list added to the node's configuration +// is detached without this changing. A field it cannot detach is an error rather than a silent share, and +// the test beside this holds every reference in the type against that promise. +func detachSections(out, from *tmcfg.Config) error { + if out == nil || from == nil { + return fmt.Errorf("no configuration to detach") + } + return detachValue(reflect.ValueOf(out).Elem(), "") +} + +// detachValue replaces every reference under v with one nothing else holds. +// +// An unexported field is skipped rather than refused. The copy this walks was made by assigning the struct, +// which copies unexported fields by value, and a decoder cannot write to one either. +func detachValue(v reflect.Value, path string) error { + switch v.Kind() { + case reflect.Pointer: + if v.IsNil() || !v.CanSet() { + return nil + } + fresh := reflect.New(v.Type().Elem()) + fresh.Elem().Set(v.Elem()) + if err := detachValue(fresh.Elem(), path); err != nil { + return err + } + v.Set(fresh) + + case reflect.Struct: + for i := 0; i < v.NumField(); i++ { + f := v.Type().Field(i) + if !v.Field(i).CanSet() { + continue + } + if err := detachValue(v.Field(i), join(path, f.Name)); err != nil { + return err + } + } + + case reflect.Slice: + if v.IsNil() || !v.CanSet() { + return nil + } + fresh := reflect.MakeSlice(v.Type(), v.Len(), v.Len()) + reflect.Copy(fresh, v) + for i := 0; i < fresh.Len(); i++ { + if err := detachValue(fresh.Index(i), path); err != nil { + return err + } + } + v.Set(fresh) + + case reflect.Map: + if v.IsNil() || !v.CanSet() { + return nil + } + fresh := reflect.MakeMapWithSize(v.Type(), v.Len()) + for _, key := range v.MapKeys() { + elem := reflect.New(v.Type().Elem()).Elem() + elem.Set(v.MapIndex(key)) + if err := detachValue(elem, path); err != nil { + return err + } + fresh.SetMapIndex(key, elem) + } + v.Set(fresh) + + case reflect.Interface: + if v.IsNil() || !v.CanSet() { + return nil + } + inner := v.Elem() + fresh := reflect.New(inner.Type()).Elem() + fresh.Set(inner) + if err := detachValue(fresh, path); err != nil { + return err + } + v.Set(fresh) + + case reflect.Chan, reflect.Func, reflect.UnsafePointer: + return fmt.Errorf("%s is a %s, which cannot be copied", path, v.Kind()) + } + return nil +} + +// join builds a field path for a message. +func join(path, field string) string { + if path == "" { + return field + } + return path + "." + field +} + +// describe reads the value the node's configuration currently holds for each key, as text. +// +// Read through the same tags the decode writes through, so a key names the same field in both directions. +// Held as text because what a report needs is whether two values differ and what they are, and comparing +// the shapes a decode produced against the shapes a struct holds would answer a different question. +func describe(cfg *tmcfg.Config, keys []string) (map[string]string, error) { + out := map[string]string{} + if cfg == nil { + return out, fmt.Errorf("no configuration to read") + } + var nested map[string]any + if err := mapstructure.Decode(cfg, &nested); err != nil { + return out, err + } + flat := map[string]any{} + flatten("", nested, flat) + for _, key := range keys { + if v, ok := flat[key]; ok { + out[key] = fmt.Sprint(v) + } + } + return out, nil +} + +// flatten turns a nested map into one keyed by dotted path. +func flatten(prefix string, in map[string]any, out map[string]any) { + for name, value := range in { + path := name + if prefix != "" { + path = prefix + "." + name + } + if inner, nested := value.(map[string]any); nested { + flatten(path, inner, out) + continue + } + out[path] = value + } +} + +// DescribeForTest reads what a node's configuration holds for each key, as text. +// +// Exported for the test that measures the two generators against each other, which lives beside the boot +// because only a boot produces a generated file. +func DescribeForTest(cfg *tmcfg.Config, keys []string) map[string]string { + out, _ := describe(cfg, keys) + return out +} + +// refuseWhatDecodesToSomethingElse reports written values the decoder accepts and turns into something the +// operator did not mean, with what they should have written. +// +// Two shapes, and both decode cleanly, which is why nothing later objects. +// +// A length of time has no form of its own in the file, so it is written as text with a unit. A plain number +// is read as nanoseconds, the shortest unit there is, so sixty means sixty billionths of a second. Zero is +// the exception and is allowed: nanoseconds and seconds are the same at zero, and zero is the documented way +// to turn several of these settings off. +// +// A negative number written where the field cannot hold one wraps to the largest value that field has. So +// minus one, which is how an operator says "no limit" in most software they have used, becomes a limit of +// eighteen million million million: the ceiling on connected peers stops bounding anything, and a window +// measured in seconds becomes six centuries. +// +// This is the one place either can be caught. The resolution sees a number and a key; only the struct says +// what the key is. +func refuseWhatDecodesToSomethingElse(cfg *tmcfg.Config, values map[string]any) []string { + t := reflect.TypeOf(*cfg) + durations := durationKeys(t, "") + unsigned := unsignedKeys(t, "") + + var bad []string + for key, value := range values { + n, numeric := asNumber(value) + if !numeric { + continue + } + switch { + case durations[key] && n != 0: + bad = append(bad, fmt.Sprintf("%s = %v is a length of time, so write a unit, as %q", + key, value, fmt.Sprintf("%vs", value))) + case unsigned[key] && n < 0: + bad = append(bad, fmt.Sprintf("%s = %v cannot be negative, and decodes to the largest value "+ + "this setting can hold rather than to no limit", key, value)) + } + } + sort.Strings(bad) + return bad +} + +// asNumber reports whether a written value arrived as a number, and what it was. +// +// Held as a float because what the checks above ask is whether it is zero and whether it is negative, and +// every numeric shape a file, a variable or a flag can carry answers both. +func asNumber(value any) (float64, bool) { + switch v := value.(type) { + case int: + return float64(v), true + case int8: + return float64(v), true + case int16: + return float64(v), true + case int32: + return float64(v), true + case int64: + return float64(v), true + case uint: + return float64(v), true + case uint8: + return float64(v), true + case uint16: + return float64(v), true + case uint32: + return float64(v), true + case uint64: + return float64(v), true + case float32: + return float64(v), true + case float64: + return v, true + } + return 0, false +} + +// unsignedKeys returns the dotted keys whose field cannot hold a negative number. +func unsignedKeys(t reflect.Type, prefix string) map[string]bool { + return keysWhoseFieldIs(t, prefix, func(ft reflect.Type) bool { + switch ft.Kind() { + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + return true + } + return false + }) +} + +// durationKeys returns the dotted keys whose field is a length of time. +// +// Matched by conversion rather than by identity, so a named type over the same underlying number is a length +// of time too. +func durationKeys(t reflect.Type, prefix string) map[string]bool { + durationType := reflect.TypeOf(time.Duration(0)) + return keysWhoseFieldIs(t, prefix, func(ft reflect.Type) bool { + return ft.Kind() == reflect.Int64 && ft.ConvertibleTo(durationType) && ft != reflect.TypeOf(int64(0)) + }) +} + +// keysWhoseFieldIs returns the dotted keys whose field answers a question about its type. +// +// One walk for every such question, over the same tag rules the declaration derives keys by, so a key found +// here is a key that can be written. +func keysWhoseFieldIs(t reflect.Type, prefix string, is func(reflect.Type) bool) map[string]bool { + out := map[string]bool{} + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + if f.PkgPath != "" { + continue + } + tag, ok := f.Tag.Lookup("mapstructure") + if !ok { + continue + } + name := strings.Split(tag, ",")[0] + squash := strings.Contains(tag, ",squash") + ft := f.Type + for ft.Kind() == reflect.Pointer { + ft = ft.Elem() + } + path := name + if prefix != "" && name != "" { + path = prefix + "." + name + } + if squash { + for key := range keysWhoseFieldIs(ft, prefix, is) { + out[key] = true + } + continue + } + if is(ft) { + out[path] = true + continue + } + if ft.Kind() == reflect.Struct { + for key := range keysWhoseFieldIs(ft, path, is) { + out[key] = true + } + } + } + return out +} + +// referencePathsIn returns every path in a type that a copy has to detach, for the test that holds +// detachSections to the type it copies. +func referencePathsIn(t reflect.Type, path string, seen map[reflect.Type]bool) []string { + if seen[t] { + return nil + } + seen[t] = true + defer delete(seen, t) + + var out []string + switch t.Kind() { + case reflect.Pointer, reflect.Slice, reflect.Map, reflect.Interface: + if path != "" { + out = append(out, path) + } + if t.Kind() != reflect.Interface { + out = append(out, referencePathsIn(t.Elem(), path, seen)...) + } + case reflect.Struct: + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + if f.PkgPath != "" { + continue + } + out = append(out, referencePathsIn(f.Type, join(path, f.Name), seen)...) + } + } + sort.Strings(out) + return out +} + +// samePath reports whether two paths name the same field, ignoring repeats a pointer produces. +func samePath(a, b string) bool { return strings.TrimSuffix(a, ".") == strings.TrimSuffix(b, ".") } diff --git a/cmd/seid/cmd/configmanager/tendermint_copy_test.go b/cmd/seid/cmd/configmanager/tendermint_copy_test.go new file mode 100644 index 0000000000..65c8220a1f --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint_copy_test.go @@ -0,0 +1,115 @@ +package configmanager + +import ( + "reflect" + "testing" + + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// TestTheCopyShareNothingWithWhatItCopied is the property the rollback rests on. +// +// The delivery decodes into a copy and publishes by replacing, so a refused value leaves the node's +// configuration untouched. That holds only if the copy shares nothing the decode can write through, and a +// decoder writes a list into the array its target already holds. One shared section, list or map and the +// rehearsal edits the original. +// +// Walked over the whole type rather than the fields anyone thought of, so a reference added to the node's +// configuration fails here rather than quietly sharing. +func TestTheCopyShareNothingWithWhatItCopied(t *testing.T) { + from := tmcfg.DefaultConfig() + // Give every list something in it, so a shared backing array is observable. + from.RPC.CORSAllowedOrigins = []string{"a", "b", "c"} + from.StateSync.RPCServers = []string{"one:1", "two:2"} + from.TxIndex.Indexer = []string{"kv"} + from.Other = map[string]any{"left": "over"} + + out, err := copyNodeConfig(from) + if err != nil { + t.Fatalf("copyNodeConfig: %v", err) + } + + for _, path := range referencePathsIn(reflect.TypeOf(tmcfg.Config{}), "", map[reflect.Type]bool{}) { + a, okA := fieldByPath(reflect.ValueOf(from).Elem(), path) + b, okB := fieldByPath(reflect.ValueOf(out).Elem(), path) + if !okA || !okB { + continue + } + if shares(a, b) { + t.Errorf("%s is shared between the node's configuration and the copy, so a decode into the "+ + "copy writes through to the node and a refused value cannot be rolled back", path) + } + } +} + +// TestTheCopyHoldsWhatItCopied is the other half: detaching must not lose a value. +// +// A copy that shares nothing and holds nothing would pass the test above and deliver a configuration of +// zeroes over a running node. +func TestTheCopyHoldsWhatItCopied(t *testing.T) { + from := tmcfg.DefaultConfig() + from.RPC.CORSAllowedOrigins = []string{"a", "b", "c"} + from.Mempool.Size = 4321 + from.Instrumentation.Prometheus = true + from.Other = map[string]any{"left": "over"} + + out, err := copyNodeConfig(from) + if err != nil { + t.Fatalf("copyNodeConfig: %v", err) + } + if !reflect.DeepEqual(from, out) { + t.Error("the copy does not hold what it copied; a delivery would publish a configuration that " + + "differs from the node's in ways nobody wrote") + } +} + +// fieldByPath walks a dotted field path, following pointers. +func fieldByPath(v reflect.Value, path string) (reflect.Value, bool) { + for _, name := range splitPath(path) { + for v.Kind() == reflect.Pointer { + if v.IsNil() { + return reflect.Value{}, false + } + v = v.Elem() + } + if v.Kind() != reflect.Struct { + return reflect.Value{}, false + } + f := v.FieldByName(name) + if !f.IsValid() { + return reflect.Value{}, false + } + v = f + } + return v, true +} + +// splitPath breaks a dotted field path into its names. +func splitPath(path string) []string { + if path == "" { + return nil + } + var out []string + start := 0 + for i := 0; i < len(path); i++ { + if path[i] == '.' { + out = append(out, path[start:i]) + start = i + 1 + } + } + return append(out, path[start:]) +} + +// shares reports whether two values point at the same memory. +func shares(a, b reflect.Value) bool { + if a.Kind() != b.Kind() { + return false + } + switch a.Kind() { + case reflect.Pointer, reflect.Map: + return !a.IsNil() && !b.IsNil() && a.Pointer() == b.Pointer() + case reflect.Slice: + return a.Len() > 0 && b.Len() > 0 && a.Pointer() == b.Pointer() + } + return false +} diff --git a/cmd/seid/cmd/node_agreement_test.go b/cmd/seid/cmd/node_agreement_test.go new file mode 100644 index 0000000000..0d7ed79680 --- /dev/null +++ b/cmd/seid/cmd/node_agreement_test.go @@ -0,0 +1,135 @@ +package cmd + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "testing" + + "go.opentelemetry.io/otel/sdk/trace" + + "github.com/sei-protocol/sei-chain/cmd/seid/cmd/configmanager" + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// bootGeneratedDefaults is what each diverging key resolves to for a node that has no configuration file of +// its own and lets the boot generate one. +// +// A declared value is what the init command writes for a kind of node. That command is not the only thing +// in this binary that writes this file: a node started without one gets it generated by the boot instead, +// and the two do not agree. These are the keys where they differ, with what the second one produces. +// +// Held as text because the two sides carry different Go types for the same key often enough that comparing +// values would be comparing shapes. What matters is which keys disagree and what a node gets instead. +var bootGeneratedDefaults = map[string]string{ + "p2p.recv-rate": "5120000", + "p2p.send-rate": "5120000", + "rpc.pprof-laddr": "localhost:6060", + "tx-index.indexer": "[kv]", +} + +// reasoning says what a node gets, and it is why each row is measured rather than described. +var reasoning = map[string]string{ + "p2p.recv-rate": "the ceiling on what one connection may pull, four times lower than the declared " + + "value, so a node adopting a file generated by the other writer would have it raised", + "p2p.send-rate": "the same ceiling in the other direction", + "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", +} + +// TestTheDivergencesFromAGeneratedFileAreTheRecordedOnes measures what a comment would only claim. +// +// The init command and the boot both generate this file and they disagree, so a declared value is what one +// of them writes and not simply what a generated file carries. Which keys those are is measured here rather +// than described, because a key that starts diverging fails and so does one that stops. +// +// Driven through a real boot with no configuration file of its own and no sei.toml, so nothing is delivered +// and what the node holds is purely what the boot generated. +func TestTheDivergencesFromAGeneratedFileAreTheRecordedOnes(t *testing.T) { + configtest.Isolate(t) + generated := whatTheBootGenerates(t) + + resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + + var measured []string + for key, got := range generated { + declared, declares := resolved.Values[key] + if !declares { + continue + } + if fmt.Sprint(declared) == got { + if _, listed := bootGeneratedDefaults[key]; listed { + t.Errorf("%s no longer diverges, both sides being %v. Take it off the record, so the "+ + "record stays the set of keys the two generators state differently", key, declared) + } + continue + } + measured = append(measured, key) + want, listed := bootGeneratedDefaults[key] + switch { + case !listed: + t.Errorf("%s is declared as %v and a node that let the boot generate its file runs %q, and "+ + "nothing records that. %s", key, declared, got, reasoning[key]) + case want != got: + t.Errorf("%s is recorded as running %q and runs %q", key, want, got) + } + } + + sort.Strings(measured) + if len(measured) != len(bootGeneratedDefaults) { + t.Errorf("measured %d divergences and %d are recorded: %v", + len(measured), len(bootGeneratedDefaults), measured) + } +} + +// whatTheBootGenerates returns what a node holds for every declared key of the decoded sections, having +// started with no configuration file of its own. +func whatTheBootGenerates(t *testing.T) map[string]string { + t.Helper() + home := configtest.NewHome(t) + if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + // 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{}) + if err := cmd.Flags().Set("home", home.Root); 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 + } + if ctx.Config == nil { + t.Fatal("the boot produced no node configuration") + } + + var keys []string + for name := range registry.DecodedSections() { + section, ok := registry.Lookup(name) + if !ok { + continue + } + keys = append(keys, section.Keys...) + } + return configmanager.DescribeForTest(ctx.Config, keys) +} diff --git a/cmd/seid/cmd/node_delivery_test.go b/cmd/seid/cmd/node_delivery_test.go new file mode 100644 index 0000000000..0ba8d5508c --- /dev/null +++ b/cmd/seid/cmd/node_delivery_test.go @@ -0,0 +1,382 @@ +package cmd + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "testing" + "time" + + "go.opentelemetry.io/otel/sdk/trace" + + "github.com/sei-protocol/sei-chain/cmd/seid/cmd/configmanager" + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// The second delivery, driven the way an operator reaches it. +// +// A section whose reader looks its keys up one at a time is delivered by putting the value into the source. +// The node's own configuration file is read once into a struct before any of that, so a value put into the +// source reaches nothing and has to be decoded into the struct instead. These read the setting the node +// runs rather than the source it was resolved into, because a key can be correct in the source and absent +// from the struct. + +// bootWithNodeFile runs a real boot against a sei.toml and a generated node configuration file. +func bootWithNodeFile(t *testing.T, seiToml string, edit func(*tmcfg.Config)) *server.Context { + t.Helper() + home := configtest.NewHome(t) + dir := filepath.Join(home.Root, "config") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + // The node's own file, generated the way the node generates it, so what the delivery writes over is + // what an operator would actually have. + live := tmcfg.DefaultConfig() + if edit != nil { + edit(live) + } + if err := tmcfg.WriteConfigFile(home.Root, live); err != nil { + t.Fatalf("render the node's configuration file: %v", err) + } + if seiToml != "" { + if err := os.WriteFile(filepath.Join(dir, "sei.toml"), []byte(seiToml), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + } + + cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home.Root); err != nil { + t.Fatalf("set --home: %v", err) + } + ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("the boot was refused: %v", err) + } + if ctx.Config == nil { + t.Fatal("the boot produced no node configuration") + } + return ctx +} + +const nodeFileHeader = "schema_version = 1\nnode_mode = \"validator\"\n" + +// TestAWrittenValueReachesTheNodesOwnConfiguration is the property the whole thing rests on. +func TestAWrittenValueReachesTheNodesOwnConfiguration(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[instrumentation]\nprometheus = true\nmax-open-connections = 41\n", nil) + + if !ctx.Config.Instrumentation.Prometheus { + t.Error("sei.toml turned the metrics listener on and the node runs with it off. The value was " + + "resolved and put into a source that nothing reading this file ever consults") + } + if got := ctx.Config.Instrumentation.MaxOpenConnections; got != 41 { + t.Errorf("sei.toml set max-open-connections to 41 and the node runs %d", got) + } +} + +// TestAnUnwrittenKeyTakesTheDeclaredValue is what makes sei.toml the configuration rather than a patch. +// +// A section read by a decode holds what its own file said, and that file is not consulted for a declared +// key under this manager. So a key sei.toml does not mention arrives at the value this binary declares for +// the kind of node this is, and what config.toml said about it does not survive. +// +// The fixture turns the key on in the node's own file, where the declared value is off, so the two +// disagree. Without that they agree and there is nothing to observe. +// +// This is the change with the largest consequence for an existing node, which is why a path that renders +// sei.toml from the files a node already has must land before this is switched on anywhere. +func TestAnUnwrittenKeyTakesTheDeclaredValue(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nsize = 4321\n", func(live *tmcfg.Config) { + live.Instrumentation.Prometheus = true + }) + + if got := ctx.Config.Mempool.Size; got != 4321 { + t.Fatalf("the written key arrived as %d, so this test cannot tell the two cases apart", got) + } + if ctx.Config.Instrumentation.Prometheus { + t.Error("the node's own file turned the metrics listener on, sei.toml said nothing about it, and " + + "the node still runs with it on. config.toml is still answering for a declared key, so " + + "sei.toml is a patch on the configuration rather than the configuration") + } +} + +// TestARefusedValueLeavesItsSectionAlone is the promise that makes this safe to enable. +// +// A decoder gathers errors and keeps going, so a value it refuses partway leaves its target holding some +// new values and some old, with nothing to compare against. The delivery decodes into a copy and publishes +// by replacing, so a refused value leaves the section exactly as the node had it. +func TestARefusedValueLeavesItsSectionAlone(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[instrumentation]\nprometheus = true\nmax-open-connections = \"not a number\"\n", nil) + + if got := ctx.Config.Instrumentation.MaxOpenConnections; got != 3 { + t.Errorf("max-open-connections is %d after a refused value, want the 3 the node had. A "+ + "partly applied decode leaves settings nobody chose", got) + } + if ctx.Config.Instrumentation.Prometheus { + t.Error("the value beside the refused one was applied, so a partial decode was published. " + + "Either all of a section's values arrive or none do") + } +} + +// TestARefusedValueCostsOnlyItsOwnSection is why the delivery is per section. +// +// One decode for the whole file would mean an operator who fixed one setting and mistyped another boots +// with neither applied. The mistyped section is lost; the one beside it is not. +func TestARefusedValueCostsOnlyItsOwnSection(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[instrumentation]\nmax-open-connections = \"not a number\"\n"+ + "\n[mempool]\nsize = 4321\n", nil) + + if got := ctx.Config.Instrumentation.MaxOpenConnections; got != 3 { + t.Errorf("the refused section was applied anyway, reading %d", got) + } + if got := ctx.Config.Mempool.Size; got != 4321 { + t.Errorf("mempool.size is %d and sei.toml set it to 4321. A value refused in one section took "+ + "another section's settings down with it", got) + } +} + +// TestEachChannelWinsForADecodedKeyToo is precedence, asserted where a decoded value lands. +func TestEachChannelWinsForADecodedKeyToo(t *testing.T) { + const key = "rpc.max-open-connections" + body := nodeFileHeader + "\n[rpc]\nmax-open-connections = 111\n" + + t.Run("the file beats what the node's own file said", func(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, body, nil) + if got := ctx.Config.RPC.MaxOpenConnections; got != 111 { + t.Errorf("the node runs %d with 111 in sei.toml; the value resolved and never reached the "+ + "struct the node reads", got) + } + }) + + t.Run("the environment beats the file", func(t *testing.T) { + configtest.Isolate(t) + t.Setenv(registry.EnvName(key), "222") + ctx := bootWithNodeFile(t, body, nil) + if got := ctx.Config.RPC.MaxOpenConnections; got != 222 { + t.Errorf("the node runs %d with 222 in the environment and 111 in the file", got) + } + }) +} + +// TestTheDeliveryLeavesTheRootDirectoryAlone is what the root-directory exclusions buy. +func TestTheDeliveryLeavesTheRootDirectoryAlone(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[instrumentation]\nprometheus = true\n", nil) + + if !ctx.Config.Instrumentation.Prometheus { + t.Fatal("the delivery did not run, so this test would pass with the root directory declared") + } + if ctx.Config.RootDir == "" { + t.Error("the node's root directory is empty after the delivery") + } + if ctx.Config.PrivValidator.RootDir == "" { + t.Error("the signing key's root directory is empty after the delivery. A node that cannot find " + + "its key does not sign") + } +} + +// TestATypedFlagReachesTheKeyItCarries covers the one channel an operator reaches for under pressure. +// +// A flag's name and the key it carries are not always spelled the same: the node's own flags separate words +// with an underscore where the tag they decode through uses a hyphen. Compared as strings such a flag looks +// like a name nothing declares, so it is dropped, and the file wins over the command line. +// +// Driven with the file and the flag disagreeing, and read off the struct the node runs from, because this +// key belongs to a section delivered by a decode. +func TestATypedFlagReachesTheKeyItCarries(t *testing.T) { + const key = "p2p.unconditional-peer-ids" + const flag = "p2p.unconditional_peer_ids" + configtest.Isolate(t) + + home := configtest.NewHome(t) + dir := filepath.Join(home.Root, "config") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatal(err) + } + if err := tmcfg.WriteConfigFile(home.Root, tmcfg.DefaultConfig()); err != nil { + t.Fatalf("render the node's configuration file: %v", err) + } + body := nodeFileHeader + "\n[p2p]\nunconditional-peer-ids = \"from-the-file\"\n" + if err := os.WriteFile(filepath.Join(dir, "sei.toml"), []byte(body), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + + cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home.Root); err != nil { + t.Fatal(err) + } + if err := cmd.Flags().Set(flag, "from-the-command-line"); err != nil { + t.Skipf("--%s is not on this command, so nothing here can carry the key: %v", flag, err) + } + ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("the boot was refused: %v", err) + } + + if got := ctx.Config.P2P.UnconditionalPeerIDs; got != "from-the-command-line" { + t.Errorf("the node runs %q with --%s typed and a different value in the file, want the typed "+ + "one. The flag's name and the key it carries are spelled differently, so comparing them as "+ + "strings drops the flag and the file wins over the command line", got, flag) + } +} + +// TestALengthOfTimeWrittenAsAPlainNumberIsRefused covers a value that decodes cleanly and is wrong by a +// factor of a billion. +// +// The file format has no way to say how long something is, so a length of time is written as text with a +// unit. A plain number decodes as nanoseconds, the shortest unit there is, so sixty means sixty billionths +// of a second and the node starts. Nothing later objects, because nothing later can tell. +func TestALengthOfTimeWrittenAsAPlainNumberIsRefused(t *testing.T) { + configtest.Isolate(t) + was := tmcfg.DefaultConfig().Mempool.TTLDuration + + t.Run("a plain number is refused and the section is left alone", func(t *testing.T) { + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nttl-duration = 60\nsize = 4321\n", nil) + if got := ctx.Config.Mempool.TTLDuration; got != was { + t.Errorf("the node runs a time-to-live of %v after a plain 60 was written, want the %v it "+ + "had. Sixty read as nanoseconds is sixty billionths of a second", got, was) + } + if got := ctx.Config.Mempool.Size; got == 4321 { + t.Error("the value beside the refused one was applied, so the section was published in part") + } + }) + + t.Run("zero is applied, because zero is the same in every unit", func(t *testing.T) { + // Several of these settings document zero as the way to turn them off, and three declare it as + // their value, so an operator writing it is doing the ordinary thing. Refusing it would cost them + // every other key in the section. + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[rpc]\ntimeout-read-header = 0\nmax-open-connections = 41\n", nil) + if got := ctx.Config.RPC.TimeoutReadHeader; got != 0 { + t.Errorf("the node runs a read-header timeout of %v with 0 written, want 0", got) + } + if got := ctx.Config.RPC.MaxOpenConnections; got != 41 { + t.Errorf("max-open-connections is %d, so writing a zero length of time cost the section. "+ + "Zero nanoseconds and zero seconds are the same value, so there is nothing to refuse", got) + } + }) + + t.Run("the same number with a unit is applied", func(t *testing.T) { + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nttl-duration = \"60s\"\n", nil) + if got := ctx.Config.Mempool.TTLDuration; got != 60*time.Second { + t.Errorf("the node runs %v with \"60s\" written, want 60s. Refusing a plain number must not "+ + "refuse the written form an operator is being asked for", got) + } + }) +} + +// TestTheReportSurvivesAQuietNode is what a fleet running its nodes quiet needs. +// +// One log level covers every logger in the process and an operator writes it. A fleet that sets it above the +// level these reports use turns this manager into a component that changes what a node runs and says nothing +// about it, and the report is the only place the node's own file and the running settings can be told apart. +// +// The level is what is asserted rather than a message, because a message can be absent for reasons that have +// nothing to do with whether it would have been printed. +func TestTheReportSurvivesAQuietNode(t *testing.T) { + configtest.Isolate(t) + + ctx := bootWithNodeFile(t, nodeFileHeader+"log-level = \"error\"\n\n[mempool]\nsize = 4321\n", nil) + if got := ctx.Config.Mempool.Size; got != 4321 { + t.Fatalf("the value was not delivered (%d), so this test cannot show a report being kept", got) + } + if !configmanager.OwnReportingEnabledForTest() { + t.Error("a node whose file sets the level to error delivered a value and this manager's own " + + "reporting is switched off. The report is the only signal it has, and the node's own file " + + "and its running settings can be told apart nowhere else") + } +} + +// TestANegativeNumberWhereTheSettingCannotHoldOneIsRefused covers the habit of writing minus one for +// "no limit". +// +// Most software an operator has used takes minus one that way. Here the field cannot hold a negative number, +// so the decoder wraps it to the largest value the field has: the ceiling on connected peers stops bounding +// anything, and a window measured in seconds becomes centuries. The value decodes cleanly, so nothing later +// objects. +func TestANegativeNumberWhereTheSettingCannotHoldOneIsRefused(t *testing.T) { + configtest.Isolate(t) + was := tmcfg.DefaultConfig().P2P.MaxConnections + + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[p2p]\nmax-connections = -1\nsend-rate = 1234567\n", nil) + + if got := ctx.Config.P2P.MaxConnections; got != was { + t.Errorf("the node allows %d connected peers after minus one was written, want the %d it had. "+ + "Minus one wraps to the largest value this setting can hold, which is no bound at all", got, was) + } + if got := ctx.Config.P2P.SendRate; got == 1234567 { + t.Error("the value beside the refused one was applied, so the section was published in part") + } +} + +// TestNoDeliveryCarriesADeclaredDefault is the one rule both deliveries depend on, named. +// +// A resolution answers for every declared key, and a declared value is what a provisioning command writes +// for a kind of node rather than what any particular node runs. Delivering one would replace a setting an +// operator never mentioned, on every boot, for every key their file omits. Both deliveries avoid that by +// narrowing to the keys a source supplied, and each does it in its own function. +// +// That makes it a rule three call sites remember rather than one a single function enforces, which is the +// shape this repository's own guidance says to guard. Until the narrowing has one home, this is the guard: +// it boots with a file that supplies one key and asserts that nothing else moved anywhere, across both +// deliveries and every mode. +func TestNoDeliveryCarriesADeclaredDefault(t *testing.T) { + for _, mode := range registry.Modes() { + t.Run(string(mode), func(t *testing.T) { + configtest.Isolate(t) + + // What the node holds before any file supplies anything. + bare := bootWithNodeFile(t, "schema_version = 1\nnode_mode = \""+string(mode)+"\"\n", nil) + keys := everyDeclaredKey() + before := configmanager.DescribeForTest(bare.Config, keys) + beforeSource := map[string]string{} + for _, key := range keys { + beforeSource[key] = fmt.Sprint(bare.Viper.Get(key)) + } + + // The same node, with a file supplying exactly one key. + after := bootWithNodeFile(t, "schema_version = 1\nnode_mode = \""+string(mode)+"\"\n"+ + "\n[mempool]\nsize = 4321\n", nil) + if got := after.Config.Mempool.Size; got != 4321 { + t.Fatalf("the one supplied key arrived as %d, so nothing was delivered and this test "+ + "would pass for a delivery that does nothing", got) + } + + afterDescribed := configmanager.DescribeForTest(after.Config, keys) + for _, key := range keys { + if key == "mempool.size" { + continue + } + if afterDescribed[key] != before[key] { + t.Errorf("%s reads %q after a file that supplies only mempool.size, and %q before. A "+ + "declared default was delivered over a setting nobody wrote", + key, afterDescribed[key], before[key]) + } + if got := fmt.Sprint(after.Viper.Get(key)); got != beforeSource[key] { + t.Errorf("%s reads %q in the source and %q before it. A declared default was "+ + "installed for a key nobody wrote", key, got, beforeSource[key]) + } + } + }) + } +} + +// everyDeclaredKey returns every key any registered section declares, sorted. +func everyDeclaredKey() []string { + keys := registry.Keys() + sort.Strings(keys) + return keys +} diff --git a/config/registry/delivery.go b/config/registry/delivery.go index 69a03b0dc9..7a2f19efb2 100644 --- a/config/registry/delivery.go +++ b/config/registry/delivery.go @@ -50,23 +50,24 @@ func DecodedSections() map[string]string { return out } -// SuppliedAndOwnedByDecodedSections answers both halves of the decoded-delivery question from one read. +// ResolvedAndOwnedByDecodedSections answers both halves of the decoded-delivery question from one read. // -// The values each decoded section was supplied, and every key those sections own whether supplied or not. A -// caller needs both: the first to deliver or report, the second to know what to leave out of a delivery that +// The resolved values each decoded section has to be handed, and every key those sections own. A caller +// needs both: the first to deliver or report, the second to know what to leave out of a delivery that // cannot carry it. // +// Every key the resolution answered, not only the ones a source wrote. A key sei.toml leaves out takes the +// value this binary declares, and that has to reach the struct like any other, because the struct is what +// the node reads and config.toml is not consulted for a declared key. That does replace what an operator's +// config.toml said for a key their sei.toml does not mention, and it is meant to: one file states what a +// node runs. +// // Together rather than from two calls, because a section arriving between two reads is absent from one // answer and present in the other, so its keys are dropped from the install by the second and not reported // by the first. Silently undelivered and unreported is the outcome one read exists to prevent. -func SuppliedAndOwnedByDecodedSections(resolved Resolved) (map[string]map[string]any, []string) { +func ResolvedAndOwnedByDecodedSections(resolved Resolved) (map[string]map[string]any, []string) { registered, _, owning := snapshot() - supplied := make(map[string]bool, len(resolved.Overrides)) - for _, key := range resolved.Overrides { - supplied[key] = true - } - out := map[string]map[string]any{} var everyKey []string for _, section := range registered { @@ -75,13 +76,14 @@ func SuppliedAndOwnedByDecodedSections(resolved Resolved) (map[string]map[string } everyKey = append(everyKey, section.Keys...) for _, key := range section.Keys { - if !supplied[key] { + value, answered := resolved.Values[key] + if !answered { continue } if out[section.Name] == nil { out[section.Name] = map[string]any{} } - out[section.Name][key] = resolved.Values[key] + out[section.Name][key] = value } } sort.Strings(everyKey) diff --git a/config/registry/delivery_test.go b/config/registry/delivery_test.go index dee2eacd16..ac23d01a7f 100644 --- a/config/registry/delivery_test.go +++ b/config/registry/delivery_test.go @@ -117,13 +117,13 @@ func TestResetClearsHowSectionsAreDelivered(t *testing.T) { } } -// TestOnlyASuppliedValueReachesADecodedSection is the difference between delivering a value and -// replacing an operator's file. +// TestEveryResolvedValueReachesADecodedSection is what makes one file the configuration. // -// A section read by a decode already holds what its own file said. Handing it a default would rewrite -// that on every boot for every key the operator's file does not mention, so a key that took its default -// is skipped and a key any other layer answered is delivered. -func TestOnlyASuppliedValueReachesADecodedSection(t *testing.T) { +// A section read by a decode holds what its own file said, and that file is not consulted for a declared +// key under this manager. So every key the resolution answered has to be handed over, including the ones +// that took their declared value, or a key sei.toml leaves out would keep whatever was on disk and the +// file would be a patch rather than the configuration. +func TestEveryResolvedValueReachesADecodedSection(t *testing.T) { registry.Reset() registerProbe(t, "mempool") registerProbe(t, "api") @@ -137,15 +137,16 @@ func TestOnlyASuppliedValueReachesADecodedSection(t *testing.T) { t.Fatalf("Resolve: %v", err) } - got, _ := registry.SuppliedAndOwnedByDecodedSections(resolved) - want := map[string]map[string]any{"mempool": {"mempool.a": "from the file"}} + got, _ := registry.ResolvedAndOwnedByDecodedSections(resolved) + want := map[string]map[string]any{ + "mempool": {"mempool.a": "from the file", "mempool.b": "from the default"}, + } if !reflect.DeepEqual(got, want) { - t.Errorf("the decoded sections are handed %v, want %v. A key nobody wrote arriving here replaces "+ - "an operator's own value, and a section delivered by a lookup arriving here is delivered "+ - "twice", got, want) + t.Errorf("the decoded sections are handed %v, want %v. Every key the resolution answered has to "+ + "arrive, and a section a lookup delivers must not", got, want) } - if _, held := got["mempool"]["mempool.b"]; held { - t.Error("mempool.b took its default and was handed to the decode anyway, which writes a value " + - "nobody chose over whatever the node's own file holds") + if _, wrong := got["api"]; wrong { + t.Error("api is delivered by a lookup and was handed to the decode as well, so its keys would " + + "be delivered twice") } } From 872f97bb5cc7e72c8c0958bd8f74ee348a036b47 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 27 Aug 2026 14:24:31 -0700 Subject: [PATCH 02/10] fix(config): keep every section pointer, and catch three more values that decode to something else The delivery replaced the node's whole configuration struct. Every section under it sits behind a pointer of its own, and components take the section rather than the configuration holding it, so replacing the top level swapped all nine pointers for fresh ones. Anything already holding a section went on reading the values that section had before the delivery. It was correct only because the delivery happens to run before those components are built, and nothing stated that order. Each section is now assigned through its own pointer, so identity survives and a holder reads the delivered value whichever side of the delivery it took its pointer from. The guard on values that decode to something other than what they say checked the sign and not the magnitude. Measured through the pre-flight command on real files: [p2p] max-connections = 1e20 approved, then applied as 18446744073709551615 [mempool] size = 1.5 approved, then applied as 1 The first saturates to the largest value the field holds, which is the same outcome the guard already refuses a minus one for. The second truncates, so a mempool written between one and two carries a single transaction. Both are refused now, and the walk that answers what a key's field is returns the field's type rather than a yes or no, so three walks over the same tags became one. Both callers of that guard printed one of its reasons for all of them. A negative number came out as "cannot be negative, and decodes to the largest value this setting can hold rather than to no limit is a length of time written as a plain number, which reads as nanoseconds". Each message already stands alone, so the callers print what they were given. A password reached the log. The transaction index takes a PostgreSQL connection string, and the report naming what a delivery changed is the only place the running configuration is written down. Nothing logs that string today. A value carrying a password now has it taken out, detected in the value rather than from a list of keys somebody keeps in step. An unread key reported as a key that did not move. Reading a value used a missing map entry to mean "could not read", so a key absent from both sides compared equal and was reported as unchanged, which is what a key an operator wrote and got looks like. The read now names what it could not read, and the caller says so. The copy walked the type and had no case for an array, so an array of pointers would have been shared. The test holding it to that promise had the same blind spot, and its share check had no case for an interface, so eleven of the twenty-six paths it enumerated could not fail. All three are fixed. A test comparing a hundred and fifty keys was comparing the absence of a value with the absence of a value for a hundred and forty of them: the node's own configuration holds the decoded sections and it was handed every declared key. The read now fails the test rather than answering partially, and the test compares each key through the delivery that owns it. Two tests skipped on a precondition that is the thing they measure, dead code, a signature whose second argument was never read, and two doc comments claiming a guarantee their bodies do not give are all corrected. The precedence between sei.toml and the node's own files is written down. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/boot_install_test.go | 20 +- .../cmd/configmanager/decode_report_test.go | 55 ++++ cmd/seid/cmd/configmanager/doc.go | 37 ++- cmd/seid/cmd/configmanager/tendermint.go | 80 ++++- cmd/seid/cmd/configmanager/tendermint_copy.go | 273 +++++++++++++----- .../cmd/configmanager/tendermint_copy_test.go | 58 ++++ cmd/seid/cmd/node_agreement_test.go | 10 +- cmd/seid/cmd/node_delivery_test.go | 71 ++++- 8 files changed, 483 insertions(+), 121 deletions(-) diff --git a/cmd/seid/cmd/boot_install_test.go b/cmd/seid/cmd/boot_install_test.go index 0d72101b41..a732ce1e14 100644 --- a/cmd/seid/cmd/boot_install_test.go +++ b/cmd/seid/cmd/boot_install_test.go @@ -188,7 +188,8 @@ func TestEveryDeclaredKeyIsInstalled(t *testing.T) { func TestAppTomlDoesNotReachTheFlagChannel(t *testing.T) { const key = "state-sync.snapshot-keep-recent" if _, declared := declaredKey(key); !declared { - t.Skipf("%s is not declared, so this cannot happen through it", key) + t.Fatalf("%s is not declared, so this test cannot reach the inversion it exists for. Skipping "+ + "instead would leave the only guard on it passing while measuring nothing", key) } configtest.Isolate(t) @@ -230,15 +231,18 @@ func TestAppTomlDoesNotReachTheFlagChannel(t *testing.T) { // accepted, which measures the absence of a value rather than the refusal. func TestAFileThisBinaryCannotUseLeavesTheNodeAsItWas(t *testing.T) { supplies := "\n[evm]\nmax_tx_pool_txs = 111\n" - for name, body := range map[string]string{ - "no file at all": "", - "a mode nothing knows": "schema_version = 1\nnode_mode = \"sentry\"\n" + supplies, - "no mode at all": "schema_version = 1\n" + supplies, - "not parseable": "schema_version = 1\nnode_mode = \"validator\"\n[evm\n" + supplies, + for _, tc := range []struct { + name string + body string + }{ + {"no file at all", ""}, + {"a mode nothing knows", "schema_version = 1\nnode_mode = \"sentry\"\n" + supplies}, + {"no mode at all", "schema_version = 1\n" + supplies}, + {"not parseable", "schema_version = 1\nnode_mode = \"validator\"\n[evm\n" + supplies}, } { - t.Run(name, func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) { configtest.Isolate(t) - ctx := bootWith(t, body, nil) + ctx := bootWith(t, tc.body, nil) if got := ctx.Viper.Get(bootProbeKey); got != nil { t.Errorf("%s reads %#v, so a value was installed from a file this binary cannot use. "+ "A node whose file names a mode this binary does not know would run one mode's "+ diff --git a/cmd/seid/cmd/configmanager/decode_report_test.go b/cmd/seid/cmd/configmanager/decode_report_test.go index 80da39930b..9a918a6ef6 100644 --- a/cmd/seid/cmd/configmanager/decode_report_test.go +++ b/cmd/seid/cmd/configmanager/decode_report_test.go @@ -5,6 +5,7 @@ import ( "fmt" "log/slog" "strconv" + "strings" "testing" "github.com/sei-protocol/sei-chain/config/registry" @@ -54,3 +55,57 @@ func asUint(t *testing.T, v any) uint64 { } return n } + +// TestAPasswordInASettingDoesNotReachTheReport covers the one value here that is a secret. +// +// The transaction index can be told to write to PostgreSQL, and it is told so with a connection string that +// carries the password in it. This report is the only place the running configuration is written down, +// which makes it the only place that password reaches a log file, a journal and whatever ships them onward. +// The node's own configuration file holds the same string, and nothing there reads it out to a log. +// +// The report cannot be turned down either: this package holds its own logger at a floor so a quiet fleet +// still sees what a delivery changed. +func TestAPasswordInASettingDoesNotReachTheReport(t *testing.T) { + const password = "sup3rs3cret" + const dsn = "postgres://seid:" + password + "@10.0.0.9:5432/idx" + + var out bytes.Buffer + log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug})) + reportWhatMoved("tx-index", + []string{"tx-index.psql-conn"}, + map[string]string{"tx-index.psql-conn": ""}, + map[string]string{"tx-index.psql-conn": dsn}, + log) + + if strings.Contains(out.String(), password) { + t.Errorf("the report carries the password from a connection string: %s", out.String()) + } + if !strings.Contains(out.String(), "10.0.0.9:5432") { + t.Errorf("the report no longer says where the index writes, so an operator cannot tell what "+ + "moved: %s", out.String()) + } + if !strings.Contains(out.String(), "tx-index.psql-conn") { + t.Errorf("the report does not name the key that moved: %s", out.String()) + } +} + +// TestAValueWithNoPasswordIsReportedAsWritten keeps the redaction from rewriting ordinary values. +// +// Most settings are not connection strings, and a value an operator reads back has to be the one they +// wrote. A path, a host and port, and a list of words all parse as something a URL parser accepts, so the +// narrow case is what has to be detected rather than anything that parses. +func TestAValueWithNoPasswordIsReportedAsWritten(t *testing.T) { + for _, value := range []string{ + "tcp://0.0.0.0:26656", + "/var/lib/sei/data", + "kv", + "", + "postgres://seid@10.0.0.9:5432/idx", + "a,b,c", + } { + if got := withoutCredentials(value); got != value { + t.Errorf("%q is reported as %q, and an operator reading it back has to see what they wrote", + value, got) + } + } +} diff --git a/cmd/seid/cmd/configmanager/doc.go b/cmd/seid/cmd/configmanager/doc.go index 39af536a17..20d7639a7f 100644 --- a/cmd/seid/cmd/configmanager/doc.go +++ b/cmd/seid/cmd/configmanager/doc.go @@ -28,28 +28,39 @@ // anywhere, and seid init has to write one for a new node. Neither exists yet, which is why the gate // defaults to the legacy manager. // -// # Delivering a value +// # Two deliveries, because a node reads a setting two ways // -// A node reads a setting one of two ways, and only one of them can be delivered from here today. Most -// settings are looked up by name from a source the boot builds, so a resolved value reaches them by being -// installed into that source. The settings the node's own configuration file carries are read once, by +// Most settings are looked up by name from a source the boot builds, so a resolved value reaches them by +// being installed into that source. The settings the node's own configuration file carries are read once, by // decoding that file into a struct before any lookup happens, so a value installed into the source -// afterwards reaches nothing at all. Those sections are identified and deliberately left out of the -// install, because installing a value that changes nothing is worse than not installing it: it reads as -// applied everywhere except in the node. They are reported instead, and delivered by the change after this -// one. +// afterwards reaches nothing at all. Those are decoded into a copy of the struct and published into it. +// +// A section names which of the two it needs, and the registry answers for the name. Nothing can tell from +// the outside: both look like a key with a value. +// +// # Precedence +// +// A value in sei.toml wins over the same key in app.toml or config.toml. The node's own files are read +// first and sei.toml is delivered on top, so for a key both state, the running node uses sei.toml's and the +// other file still says what it said. +// +// That is why the reports name every key that moved. After the delivery, neither of the node's own files +// describes what it is running, and nothing else does either. // // # Refusing nothing // // Nothing here can stop a node starting. A missing sei.toml, an unreadable one, a mode this binary does not -// know, a value the install refuses, or a panic in the delivery itself all leave every key reading as it -// always has, and the node starts. A mistyped line in a hand-edited file must not become an outage on the -// next restart. +// know, a value that decodes to something other than what it says, or a panic in the delivery itself all +// leave every key reading as it always has, and the node starts. A mistyped line in a hand-edited file must +// not become an outage on the next restart. +// +// A refusal is per section, not per file, because a decode is all or nothing for whatever it is handed. An +// operator who fixes one setting and mistypes another gets the first one. // // What that costs is that a value which does not arrive is reported rather than refused, which makes these // reports the only signal an operator has. So they are held at a floor that survives a fleet running its -// nodes quiet, without lowering a level an operator raised; they name the source they are about; and they -// are bounded, because a report that fires on every boot is one nobody reads. +// nodes quiet, without lowering a level an operator raised; they name the source they are about; they carry +// no password; and they are bounded, because a report that fires on every boot is one nobody reads. // // Deferred: a path that writes sei.toml, so a node's configuration can be rendered from its existing files // rather than only read out of a file somebody has to author by hand. diff --git a/cmd/seid/cmd/configmanager/tendermint.go b/cmd/seid/cmd/configmanager/tendermint.go index 0904acd347..60baeb84af 100644 --- a/cmd/seid/cmd/configmanager/tendermint.go +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -4,6 +4,7 @@ import ( "cmp" "fmt" "log/slog" + "net/url" "os" "strings" @@ -36,11 +37,19 @@ func deliverDecodedSections(ctx *server.Context, bySection map[string]map[string return } + // Each section names why its values need decoding rather than installing, and the reason names the + // struct they are decoded into. Reported here because this is the only place that claim is acted on: + // a section whose reason no longer describes what reads it is delivered the wrong way, and there is + // nothing else that would show it. + reasons := registry.DecodedSections() + // One section at a time. A decode is all or nothing for whatever it is handed, so a single value a // decoder refuses would otherwise cost every key in the file rather than the keys of the section it // appeared in. An operator who fixes one setting and mistypes another has to end up with the first // one applied. for _, name := range sortedKeys(bySection) { + log.Debug("delivering a section by decoding it rather than by a lookup", + "section", name, "why", reasons[name], "keys", len(bySection[name])) deliverOneSection(ctx, name, bySection[name], log) } } @@ -62,9 +71,11 @@ func deliverOneSection(ctx *server.Context, name string, values map[string]any, // Refused before the decode, because a plain number where a length of time belongs decodes cleanly // and means nanoseconds. Nothing after this can tell that apart from a value somebody meant. - if bad := refuseWhatDecodesToSomethingElse(ctx.Config, values); len(bad) > 0 { - log.Error("a length of time in this section is written as a plain number, which reads as "+ - "nanoseconds; none of the section is applied and every one of its keys reads as it always has", + if bad := whatDecodesToSomethingElse(ctx.Config, values); len(bad) > 0 { + // 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. + log.Error("a written value in this section decodes to something other than what it says; none of "+ + "the section is applied and every one of its keys reads as it always has", "section", name, "written", strings.Join(bad, "; ")) return } @@ -76,7 +87,7 @@ func deliverOneSection(ctx *server.Context, name string, values map[string]any, "section", name, "keys", strings.Join(keys, ","), "err", err) return } - before, readErr := describe(ctx.Config, keys) + before, unreadBefore, readErr := describe(ctx.Config, keys) if err := source.Unmarshal(candidate); err != nil { log.Error("a written value in this section was refused, so none of the section is applied and "+ @@ -85,8 +96,12 @@ func deliverOneSection(ctx *server.Context, name string, values map[string]any, return } - *ctx.Config = *candidate - after, afterErr := describe(ctx.Config, keys) + if err := publishNodeConfig(ctx.Config, candidate); err != nil { + log.Error("cannot publish this node's configuration, so these keys read as they always have", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } + after, unreadAfter, afterErr := describe(ctx.Config, keys) if readErr != nil || afterErr != nil { // Reported rather than compared. Two unreadable sides look identical, so comparing them would // say every value matched, which is a statement about nothing produced by reading nothing. @@ -95,6 +110,14 @@ func deliverOneSection(ctx *server.Context, name string, values map[string]any, "keys", strings.Join(keys, ","), "err", cmp.Or(readErr, afterErr)) return } + // The same hazard one key at a time. A key absent from both answers compares equal, so it would be + // reported as a setting that did not move, which is what a key an operator wrote and got looks like. + if unread := append(unreadBefore, unreadAfter...); len(unread) > 0 { + shown, omitted := capLoggedItems(sortedKeys(asSet(unread))) + log.Error("this section was applied and some of its keys cannot be read back, so nothing here "+ + "says whether those moved", "section", name, "count", len(shown)+omitted, + "keys", strings.Join(shown, ","), "omitted", omitted) + } reportWhatMoved(name, keys, before, after, log) } @@ -105,13 +128,15 @@ func deliverOneSection(ctx *server.Context, name string, values map[string]any, // copies the top level and every section under it. // // Written against the type rather than field by field, so a section added to it is copied without this -// function changing. A field this cannot copy is an error rather than a silent share. +// function changing. Every exported reference gets one of its own and one that cannot is an error rather +// than a silent share. An unexported field is copied by value and shared, which is safe only because the +// decoder this protects against cannot write to one. func copyNodeConfig(from *tmcfg.Config) (*tmcfg.Config, error) { if from == nil { return nil, fmt.Errorf("no configuration to copy") } out := *from - if err := detachSections(&out, from); err != nil { + if err := detachReferences(&out); err != nil { return nil, err } return &out, nil @@ -126,11 +151,19 @@ func copyNodeConfig(from *tmcfg.Config) (*tmcfg.Config, error) { // // Keys that did not move are not reported. An operator who writes the value their file already held has // changed nothing, and a line saying so buries the ones that did. +// +// A value carrying a password has the password taken out. This is the only place the running configuration +// is written down, so it is also the only place one would reach a log file, a journal, and whatever ships +// them onward. The node's own configuration file holds the same string and nothing reads it out to a log. +// +// The rendered list is capped for the reason every other one here is: the count is what an operator alerts +// on, and one line per key of a large section buries whichever of them mattered. func reportWhatMoved(name string, keys []string, before, after map[string]string, log *slog.Logger) { var moved []string for _, key := range keys { if before[key] != after[key] { - moved = append(moved, fmt.Sprintf("%s: %s -> %s", key, before[key], after[key])) + moved = append(moved, fmt.Sprintf("%s: %s -> %s", key, + withoutCredentials(before[key]), withoutCredentials(after[key]))) } } if len(moved) == 0 { @@ -138,8 +171,9 @@ func reportWhatMoved(name string, keys []string, before, after map[string]string "section", name, "keys", len(keys)) return } + shown, omitted := capLoggedItems(moved) log.Info("this section's settings now differ from what the node's own configuration file says", - "section", name, "changed", strings.Join(moved, "; ")) + "section", name, "count", len(moved), "changed", strings.Join(shown, "; "), "omitted", omitted) } // logLevelKey is the one delivered setting the struct is not the end of. @@ -151,6 +185,32 @@ const logLevelKey = "log-level" // for one setting, and the older one is read before any of this runs. const loggerOwnVariable = "SEI_LOG_LEVEL" +// asSet collapses repeats, so a key unread on both sides is named once. +func asSet(keys []string) map[string]struct{} { + out := make(map[string]struct{}, len(keys)) + for _, key := range keys { + out[key] = struct{}{} + } + return out +} + +// withoutCredentials removes a password from a value that carries one. +// +// A setting can hold a connection string, and a connection string can hold a password. Detected in the +// value rather than declared per key, because a list of the keys that can hold one is a list somebody keeps +// in step with every section anyone adds, and the first key forgotten is a password in a log. +func withoutCredentials(value string) string { + u, err := url.Parse(value) + if err != nil || u.User == nil { + return value + } + if _, set := u.User.Password(); !set { + return value + } + u.User = url.UserPassword(u.User.Username(), "xxxxx") + return u.String() +} + // applyResolvedLogLevel hands a resolved log level to the logger, which the struct alone does not reach. // // The boot's handler reads the level off the struct and sets it before any of this runs, so a value that diff --git a/cmd/seid/cmd/configmanager/tendermint_copy.go b/cmd/seid/cmd/configmanager/tendermint_copy.go index e878e80649..0d282f6257 100644 --- a/cmd/seid/cmd/configmanager/tendermint_copy.go +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -2,9 +2,11 @@ package configmanager import ( "fmt" + "math" "reflect" "sort" "strings" + "testing" "time" "github.com/go-viper/mapstructure/v2" @@ -12,21 +14,23 @@ import ( tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" ) -// detachSections makes a copy hold what the original holds without sharing anything a decode can write -// through. +// detachReferences replaces every reference under cfg with one nothing else holds, so a decode into cfg +// cannot write through to whatever cfg was copied from. // // A copy of the struct alone shares every section, every list and every map it points at. A decoder writes // a list into the array its target already holds, so a shared one means the rehearsal edits the original // and a refused value leaves exactly the half-written configuration the copy exists to prevent. // // Walked over the type rather than field by field, so a section or a list added to the node's configuration -// is detached without this changing. A field it cannot detach is an error rather than a silent share, and -// the test beside this holds every reference in the type against that promise. -func detachSections(out, from *tmcfg.Config) error { - if out == nil || from == nil { +// is detached without this changing. Every exported reference gets one of its own and one that cannot is an +// error; an unexported field keeps what it was copied with, which is safe only because the decoder this +// guards against cannot write to one either. The test beside this walks the same type and holds each +// reference it finds to having been detached. +func detachReferences(cfg *tmcfg.Config) error { + if cfg == nil { return fmt.Errorf("no configuration to detach") } - return detachValue(reflect.ValueOf(out).Elem(), "") + return detachValue(reflect.ValueOf(cfg).Elem(), "") } // detachValue replaces every reference under v with one nothing else holds. @@ -97,6 +101,18 @@ func detachValue(v reflect.Value, path string) error { } v.Set(fresh) + case reflect.Array: + // An array holds its elements rather than pointing at them, so the copy already has its own. Each + // element still needs detaching, because what an element holds can be a reference. + if !v.CanSet() { + return nil + } + for i := 0; i < v.Len(); i++ { + if err := detachValue(v.Index(i), path); err != nil { + return err + } + } + case reflect.Chan, reflect.Func, reflect.UnsafePointer: return fmt.Errorf("%s is a %s, which cannot be copied", path, v.Kind()) } @@ -111,28 +127,38 @@ func join(path, field string) string { return path + "." + field } -// describe reads the value the node's configuration currently holds for each key, as text. +// describe reads the value the node's configuration currently holds for each key, as text, and names the +// keys it could not read. // // Read through the same tags the decode writes through, so a key names the same field in both directions. // Held as text because what a report needs is whether two values differ and what they are, and comparing // the shapes a decode produced against the shapes a struct holds would answer a different question. -func describe(cfg *tmcfg.Config, keys []string) (map[string]string, error) { - out := map[string]string{} +// +// The unread keys are returned rather than left out of the answer. A key missing from a map reads as an +// empty value, so a caller comparing two answers finds an unread key equal on both sides and reports that +// 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{} if cfg == nil { - return out, fmt.Errorf("no configuration to read") + return values, keys, fmt.Errorf("no configuration to read") } var nested map[string]any if err := mapstructure.Decode(cfg, &nested); err != nil { - return out, err + return values, keys, err } flat := map[string]any{} flatten("", nested, flat) for _, key := range keys { - if v, ok := flat[key]; ok { - out[key] = fmt.Sprint(v) + v, ok := flat[key] + if !ok { + unread = append(unread, key) + continue } + values[key] = fmt.Sprint(v) } - return out, nil + sort.Strings(unread) + return values, unread, nil } // flatten turns a nested map into one keyed by dotted path. @@ -152,17 +178,29 @@ func flatten(prefix string, in map[string]any, out map[string]any) { // DescribeForTest reads what a node's configuration holds for each key, as text. // -// Exported for the test that measures the two generators against each other, which lives beside the boot -// because only a boot produces a generated file. -func DescribeForTest(cfg *tmcfg.Config, keys []string) map[string]string { - out, _ := describe(cfg, keys) - return out +// Exported for the tests that measure a booted node's configuration, which live beside the boot because +// only a boot produces one. +// +// It fails the test rather than answering partially. A caller comparing two of these answers over a hundred +// keys finds every value equal when both are empty, so an answer produced by reading nothing is +// indistinguishable from a node where nothing moved. +func DescribeForTest(t *testing.T, cfg *tmcfg.Config, keys []string) map[string]string { + t.Helper() + values, unread, err := describe(cfg, keys) + if err != nil { + t.Fatalf("reading %d keys off the node's configuration: %v", len(keys), err) + } + if len(unread) > 0 { + t.Fatalf("%d of %d keys are not present in the node's configuration, so a comparison over them "+ + "would find every one unchanged: %v", len(unread), len(keys), unread) + } + return values } // refuseWhatDecodesToSomethingElse reports written values the decoder accepts and turns into something the // operator did not mean, with what they should have written. // -// Two shapes, and both decode cleanly, which is why nothing later objects. +// Four shapes, and every one of them decodes cleanly, which is why nothing later objects. // // A length of time has no form of its own in the file, so it is written as text with a unit. A plain number // is read as nanoseconds, the shortest unit there is, so sixty means sixty billionths of a second. Zero is @@ -174,26 +212,43 @@ func DescribeForTest(cfg *tmcfg.Config, keys []string) map[string]string { // eighteen million million million: the ceiling on connected peers stops bounding anything, and a window // measured in seconds becomes six centuries. // -// This is the one place either can be caught. The resolution sees a number and a key; only the struct says -// what the key is. -func refuseWhatDecodesToSomethingElse(cfg *tmcfg.Config, values map[string]any) []string { - t := reflect.TypeOf(*cfg) - durations := durationKeys(t, "") - unsigned := unsignedKeys(t, "") +// A number too large for the field reaches the same place from the other direction. It saturates rather +// than being refused, so the largest value the field holds is what the setting means, and a ceiling written +// far too high stops being a ceiling at all. +// +// A fraction written where the field holds whole numbers is truncated rather than rounded, so a size +// written as one and a half decodes to one. That is a mempool of a single transaction where the operator +// wrote something between one and two. +// +// This is the one place any of them can be caught. The resolution sees a number and a key; only the struct +// says what the key is, and the range and the whole-number rule are both facts about the field. +func whatDecodesToSomethingElse(cfg *tmcfg.Config, values map[string]any) []string { + fields := keyFieldTypes(reflect.TypeOf(*cfg), "") var bad []string for key, value := range values { + ft, known := fields[key] + if !known { + continue + } n, numeric := asNumber(value) if !numeric { continue } switch { - case durations[key] && n != 0: - bad = append(bad, fmt.Sprintf("%s = %v is a length of time, so write a unit, as %q", - key, value, fmt.Sprintf("%vs", value))) - case unsigned[key] && n < 0: + case isDuration(ft) && n != 0: + bad = append(bad, fmt.Sprintf("%s = %v is a length of time written as a plain number, which "+ + "reads as nanoseconds; write a unit, as %q", key, value, fmt.Sprintf("%vs", value))) + case !holdsAWholeNumber(ft): + case n < 0 && reflect.New(ft).Elem().CanUint(): bad = append(bad, fmt.Sprintf("%s = %v cannot be negative, and decodes to the largest value "+ "this setting can hold rather than to no limit", key, value)) + case n != math.Trunc(n): + bad = append(bad, fmt.Sprintf("%s = %v is a whole-number setting, and the fraction is dropped "+ + "rather than rounded, so it decodes to %v", key, value, math.Trunc(n))) + case !reachesTheFieldAsItself(n, ft): + bad = append(bad, fmt.Sprintf("%s = %v is larger than this setting can hold, and decodes to "+ + "its largest value rather than to what is written", key, value)) } } sort.Strings(bad) @@ -234,34 +289,14 @@ func asNumber(value any) (float64, bool) { return 0, false } -// unsignedKeys returns the dotted keys whose field cannot hold a negative number. -func unsignedKeys(t reflect.Type, prefix string) map[string]bool { - return keysWhoseFieldIs(t, prefix, func(ft reflect.Type) bool { - switch ft.Kind() { - case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - return true - } - return false - }) -} - -// durationKeys returns the dotted keys whose field is a length of time. -// -// Matched by conversion rather than by identity, so a named type over the same underlying number is a length -// of time too. -func durationKeys(t reflect.Type, prefix string) map[string]bool { - durationType := reflect.TypeOf(time.Duration(0)) - return keysWhoseFieldIs(t, prefix, func(ft reflect.Type) bool { - return ft.Kind() == reflect.Int64 && ft.ConvertibleTo(durationType) && ft != reflect.TypeOf(int64(0)) - }) -} - -// keysWhoseFieldIs returns the dotted keys whose field answers a question about its type. +// keyFieldTypes returns every dotted key this type declares and the type of the field it names. // -// One walk for every such question, over the same tag rules the declaration derives keys by, so a key found -// here is a key that can be written. -func keysWhoseFieldIs(t reflect.Type, prefix string, is func(reflect.Type) bool) map[string]bool { - out := map[string]bool{} +// One walk, over the same tag rules the declaration derives keys by, so a key found here is a key that can +// be written. It answers with the field's type rather than with a yes or no, because the questions asked of +// it differ: one is whether a length of time was written as a bare number, another is whether a written +// number is one the field can hold at all. +func keyFieldTypes(t reflect.Type, prefix string) map[string]reflect.Type { + out := map[string]reflect.Type{} for i := 0; i < t.NumField(); i++ { f := t.Field(i) if f.PkgPath != "" { @@ -281,27 +316,60 @@ func keysWhoseFieldIs(t reflect.Type, prefix string, is func(reflect.Type) bool) if prefix != "" && name != "" { path = prefix + "." + name } - if squash { - for key := range keysWhoseFieldIs(ft, prefix, is) { - out[key] = true + switch { + case squash: + for key, kt := range keyFieldTypes(ft, prefix) { + out[key] = kt } - continue - } - if is(ft) { - out[path] = true - continue - } - if ft.Kind() == reflect.Struct { - for key := range keysWhoseFieldIs(ft, path, is) { - out[key] = true + case ft.Kind() == reflect.Struct && !isDuration(ft): + for key, kt := range keyFieldTypes(ft, path) { + out[key] = kt } + default: + out[path] = ft } } return out } -// referencePathsIn returns every path in a type that a copy has to detach, for the test that holds -// detachSections to the type it copies. +// isDuration reports whether a field holds a length of time. +// +// Matched by conversion rather than by identity, so a named type over the same underlying number is a +// length of time too. A plain int64 is excluded, because every length of time is one and it is not. +func isDuration(ft reflect.Type) bool { + return ft.Kind() == reflect.Int64 && ft.ConvertibleTo(reflect.TypeOf(time.Duration(0))) && + ft != reflect.TypeOf(int64(0)) +} + +// holdsAWholeNumber reports whether a field holds an integer of some width. +func holdsAWholeNumber(ft reflect.Type) bool { + v := reflect.New(ft).Elem() + return v.CanInt() || v.CanUint() +} + +// reachesTheFieldAsItself reports whether a written number arrives at a field of this type unchanged. +// +// A number outside the range a field holds is not refused by the decoder. It saturates, so the largest +// value the field has is what the setting ends up meaning, which for a ceiling is no ceiling at all. +func reachesTheFieldAsItself(n float64, ft reflect.Type) bool { + v := reflect.New(ft).Elem() + switch { + case v.CanInt(): + if n < math.MinInt64 || n > math.MaxInt64 { + return false + } + return !v.OverflowInt(int64(n)) + case v.CanUint(): + if n < 0 || n > math.MaxUint64 { + return false + } + return !v.OverflowUint(uint64(n)) + } + return true +} + +// referencePathsIn returns every path in a type that a copy has to detach, for the test that holds the +// detach to the type it walks. func referencePathsIn(t reflect.Type, path string, seen map[reflect.Type]bool) []string { if seen[t] { return nil @@ -318,6 +386,10 @@ func referencePathsIn(t reflect.Type, path string, seen map[reflect.Type]bool) [ if t.Kind() != reflect.Interface { out = append(out, referencePathsIn(t.Elem(), path, seen)...) } + case reflect.Array: + // The array itself is not a reference, so it is not a path of its own. What it holds can be, and + // leaving this case out gives this walk the same blind spot as the copy it holds to account. + out = append(out, referencePathsIn(t.Elem(), path, seen)...) case reflect.Struct: for i := 0; i < t.NumField(); i++ { f := t.Field(i) @@ -331,5 +403,58 @@ func referencePathsIn(t reflect.Type, path string, seen map[reflect.Type]bool) [ return out } -// samePath reports whether two paths name the same field, ignoring repeats a pointer produces. -func samePath(a, b string) bool { return strings.TrimSuffix(a, ".") == strings.TrimSuffix(b, ".") } +// publishNodeConfig makes a node's configuration hold what the candidate holds, without replacing it. +// +// Field by field rather than by assigning the whole struct. The configuration is one struct behind one +// pointer, and every section under it is a pointer of its own that components take and keep: constructors +// throughout the node ask for a section rather than for the configuration holding it. Assigning the whole +// struct swaps every one of those pointers for a fresh one, so anything already holding a section goes on +// reading the values that section had before the delivery, and nothing says so. +// +// Assigning through each pointer instead leaves every pointer identity as it was, so a component reads the +// delivered values whether it took its pointer before this ran or after. That removes the ordering the +// delivery would otherwise depend on, which is an ordering nothing states and no test holds. +func publishNodeConfig(target, candidate *tmcfg.Config) error { + if target == nil || candidate == nil { + return fmt.Errorf("no configuration to publish into") + } + return publishValue(reflect.ValueOf(target).Elem(), reflect.ValueOf(candidate).Elem(), "") +} + +// publishValue assigns candidate into target, following a pointer rather than replacing it. +// +// A pointer to a struct is followed and assigned through, which is what keeps the identity whatever holds it +// depends on. Everything else is assigned, and that is what carries the values. A pointer the target does +// not have yet is assigned rather than followed, because there is nothing to assign through. +// +// An unexported field is skipped, for the reason the detach skips one: the candidate was made by assigning +// the struct, so it already holds the same value, and a decoder cannot write to one either. +func publishValue(target, candidate reflect.Value, path string) error { + if target.Kind() != reflect.Struct { + target.Set(candidate) + return nil + } + for i := 0; i < target.NumField(); i++ { + f := target.Type().Field(i) + tf, cf := target.Field(i), candidate.Field(i) + if !tf.CanSet() { + continue + } + at := join(path, f.Name) + followable := f.Type.Kind() == reflect.Pointer && f.Type.Elem().Kind() == reflect.Struct + if followable && !tf.IsNil() && !cf.IsNil() { + if err := publishValue(tf.Elem(), cf.Elem(), at); err != nil { + return err + } + continue + } + if f.Type.Kind() == reflect.Struct { + if err := publishValue(tf, cf, at); err != nil { + return err + } + continue + } + tf.Set(cf) + } + return nil +} diff --git a/cmd/seid/cmd/configmanager/tendermint_copy_test.go b/cmd/seid/cmd/configmanager/tendermint_copy_test.go index 65c8220a1f..c9e2458969 100644 --- a/cmd/seid/cmd/configmanager/tendermint_copy_test.go +++ b/cmd/seid/cmd/configmanager/tendermint_copy_test.go @@ -101,6 +101,10 @@ func splitPath(path string) []string { } // shares reports whether two values point at the same memory. +// +// An interface is followed to what it holds. Without that case every interface-typed field answers "not +// shared" whatever it holds, and this type carries nine of them, so the assertion using this would pass for +// two fields holding the identical pointer. func shares(a, b reflect.Value) bool { if a.Kind() != b.Kind() { return false @@ -110,6 +114,60 @@ func shares(a, b reflect.Value) bool { return !a.IsNil() && !b.IsNil() && a.Pointer() == b.Pointer() case reflect.Slice: return a.Len() > 0 && b.Len() > 0 && a.Pointer() == b.Pointer() + case reflect.Interface: + return !a.IsNil() && !b.IsNil() && shares(a.Elem(), b.Elem()) } return false } + +// TestPublishingKeepsThePointerEverySectionIsHeldBy is the property the delivery rests on, and nothing +// else in the node states it. +// +// A component takes a section rather than the configuration that holds it, so it keeps a pointer of its own +// from whenever it was built. Assigning the whole configuration replaces every one of those pointers with a +// fresh one, which leaves each holder reading the values its section had before the delivery ran. The +// delivery would then be correct only because it happens to run before anything is built, and nothing +// states that order or fails when it changes. +// +// Driven from the type, so a section added to the node's configuration is covered without this changing. +func TestPublishingKeepsThePointerEverySectionIsHeldBy(t *testing.T) { + target := tmcfg.DefaultConfig() + candidate, err := copyNodeConfig(target) + if err != nil { + t.Fatalf("copyNodeConfig: %v", err) + } + + held := map[string]uintptr{} + held4321 := reflect.ValueOf(target).Elem() + for i := 0; i < held4321.NumField(); i++ { + f := held4321.Type().Field(i) + if f.Type.Kind() != reflect.Pointer || f.Type.Elem().Kind() != reflect.Struct { + continue + } + if held4321.Field(i).IsNil() { + continue + } + held[f.Name] = held4321.Field(i).Pointer() + } + if len(held) == 0 { + t.Fatal("this configuration holds no section behind a pointer of its own, so there is no identity " + + "here to keep and this test measures nothing") + } + + candidate.Mempool.Size = 4321 + if err := publishNodeConfig(target, candidate); err != nil { + t.Fatalf("publishNodeConfig: %v", err) + } + + after := reflect.ValueOf(target).Elem() + for name, was := range held { + if got := after.FieldByName(name).Pointer(); got != was { + t.Errorf("%s sits behind a different pointer after the delivery, so a component that took it "+ + "beforehand goes on reading the values it had before", name) + } + } + if got := target.Mempool.Size; got != 4321 { + t.Errorf("the delivered value reads %d, want 4321. Keeping the pointer is only worth anything if "+ + "the value arrives through it", got) + } +} diff --git a/cmd/seid/cmd/node_agreement_test.go b/cmd/seid/cmd/node_agreement_test.go index 0d7ed79680..a65ffba43d 100644 --- a/cmd/seid/cmd/node_agreement_test.go +++ b/cmd/seid/cmd/node_agreement_test.go @@ -123,13 +123,5 @@ func whatTheBootGenerates(t *testing.T) map[string]string { t.Fatal("the boot produced no node configuration") } - var keys []string - for name := range registry.DecodedSections() { - section, ok := registry.Lookup(name) - if !ok { - continue - } - keys = append(keys, section.Keys...) - } - return configmanager.DescribeForTest(ctx.Config, keys) + return configmanager.DescribeForTest(t, ctx.Config, keysADecodeDelivers()) } diff --git a/cmd/seid/cmd/node_delivery_test.go b/cmd/seid/cmd/node_delivery_test.go index 0ba8d5508c..709cf984df 100644 --- a/cmd/seid/cmd/node_delivery_test.go +++ b/cmd/seid/cmd/node_delivery_test.go @@ -218,7 +218,9 @@ func TestATypedFlagReachesTheKeyItCarries(t *testing.T) { t.Fatal(err) } if err := cmd.Flags().Set(flag, "from-the-command-line"); err != nil { - t.Skipf("--%s is not on this command, so nothing here can carry the key: %v", flag, err) + t.Fatalf("--%s is not on this command, so nothing here can carry the key: %v. This is the only "+ + "guard on a flag name and its key being spelled differently, and skipping would leave it "+ + "passing while measuring nothing", flag, err) } ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) if err != nil { @@ -341,7 +343,11 @@ func TestNoDeliveryCarriesADeclaredDefault(t *testing.T) { // What the node holds before any file supplies anything. bare := bootWithNodeFile(t, "schema_version = 1\nnode_mode = \""+string(mode)+"\"\n", nil) keys := everyDeclaredKey() - before := configmanager.DescribeForTest(bare.Config, keys) + // The node's own configuration holds the decoded sections and nothing else, so those are the + // only keys it can be read for. Handing it the rest compares an absent value with an absent + // value, which reports that every one of them is unchanged whatever the delivery did. + decodedKeys := keysADecodeDelivers() + before := configmanager.DescribeForTest(t, bare.Config, decodedKeys) beforeSource := map[string]string{} for _, key := range keys { beforeSource[key] = fmt.Sprint(bare.Viper.Get(key)) @@ -355,15 +361,20 @@ func TestNoDeliveryCarriesADeclaredDefault(t *testing.T) { "would pass for a delivery that does nothing", got) } - afterDescribed := configmanager.DescribeForTest(after.Config, keys) - for _, key := range keys { + afterDescribed := configmanager.DescribeForTest(t, after.Config, decodedKeys) + for _, key := range decodedKeys { if key == "mempool.size" { continue } if afterDescribed[key] != before[key] { - t.Errorf("%s reads %q after a file that supplies only mempool.size, and %q before. A "+ - "declared default was delivered over a setting nobody wrote", - key, afterDescribed[key], before[key]) + t.Errorf("%s reads %q in the node's configuration after a file that supplies only "+ + "mempool.size, and %q before. A declared default was delivered over a setting "+ + "nobody wrote", key, afterDescribed[key], before[key]) + } + } + for _, key := range keys { + if key == "mempool.size" { + continue } if got := fmt.Sprint(after.Viper.Get(key)); got != beforeSource[key] { t.Errorf("%s reads %q in the source and %q before it. A declared default was "+ @@ -380,3 +391,49 @@ func everyDeclaredKey() []string { sort.Strings(keys) return keys } + +// TestANumberTooLargeForTheSettingIsRefused reaches the same failure as a negative one, from the other side. +// +// A number the field cannot hold is not refused by the decoder. It saturates, so the largest value the field +// has becomes what the setting means. That is precisely the outcome the guard beside this refuses a minus +// one for, and a number written far too high arrives at it without passing anything that objects. +func TestANumberTooLargeForTheSettingIsRefused(t *testing.T) { + configtest.Isolate(t) + was := tmcfg.DefaultConfig().P2P.MaxConnections + + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[p2p]\nmax-connections = 1e20\nmax-incoming-connection-attempts = 7\n", + nil) + if got := ctx.Config.P2P.MaxConnections; got != was { + t.Errorf("the node runs a connection ceiling of %d after 1e20 was written, want the %d it had. A "+ + "number that size saturates to the largest the field holds, so the ceiling bounds nothing", + got, was) + } + if got := ctx.Config.P2P.MaxIncomingConnectionAttempts; got == 7 { + t.Error("the value beside the refused one was applied, so the section was published in part") + } +} + +// TestAFractionWhereTheSettingHoldsWholeNumbersIsRefused covers a value that decodes to a different number. +// +// A fraction is truncated rather than rounded, so a mempool written as one and a half decodes to one. +// Nothing later objects, because by the time anything reads it the value is a whole number and a perfectly +// ordinary one. +func TestAFractionWhereTheSettingHoldsWholeNumbersIsRefused(t *testing.T) { + configtest.Isolate(t) + was := tmcfg.DefaultConfig().Mempool.Size + + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nsize = 1.5\n", nil) + if got := ctx.Config.Mempool.Size; got != was { + t.Errorf("the node runs a mempool of %d after 1.5 was written, want the %d it had. The fraction "+ + "is dropped rather than rounded, so the node would carry a single transaction", got, was) + } +} + +// keysADecodeDelivers returns every key the decoded sections own, sorted. +// +// Through the one accessor that answers both halves from a single read of the registry, so a test cannot +// see a registry the boot did not. +func keysADecodeDelivers() []string { + _, keys := registry.ResolvedAndOwnedByDecodedSections(registry.Resolved{}) + return keys +} From febe72feea9165cd15c2cee017c34ddbb1dfac21 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 31 Aug 2026 07:26:17 -0700 Subject: [PATCH 03/10] fix(deps): mark mapstructure as a direct dependency The decode delivery imports go-viper/mapstructure directly, and go.mod still carried it as indirect. The lint job runs a `go mod tidy` check, which failed on that line for this branch and every branch above it. Co-Authored-By: Claude Opus 5 (1M context) --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index e46ab4e48e..736705c6c1 100644 --- a/go.mod +++ b/go.mod @@ -188,7 +188,7 @@ require ( github.com/go-logr/stdr v1.2.2 // indirect github.com/go-ole/go-ole v1.3.0 // indirect github.com/go-sourcemap/sourcemap v2.1.3+incompatible // indirect - github.com/go-viper/mapstructure/v2 v2.5.0 // indirect + github.com/go-viper/mapstructure/v2 v2.5.0 github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2 // indirect github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 // indirect github.com/golang/snappy v1.0.0 // indirect From 28afebcf34c750b3f48754734f49319ce7548518 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 31 Aug 2026 07:43:09 -0700 Subject: [PATCH 04/10] fix(config): remove a password from every form a connection string carries one in The redaction only handled the userinfo of a URL. PostgreSQL accepts a password three other ways, and all three reached the log: postgres://seid@host/idx?password=... postgres://seid@host/idx?sslpassword=... host=... user=seid password=... dbname=idx Four of five forms leaked. A named password field is now removed wherever it appears, with the prefix kept so sslpassword still reads as itself, and the value running only to the next separator so nothing after it is swallowed. Co-Authored-By: Claude Opus 5 (1M context) --- .../cmd/configmanager/decode_report_test.go | 45 ++++++++++++------- cmd/seid/cmd/configmanager/tendermint.go | 37 ++++++++++----- 2 files changed, 54 insertions(+), 28 deletions(-) diff --git a/cmd/seid/cmd/configmanager/decode_report_test.go b/cmd/seid/cmd/configmanager/decode_report_test.go index 9a918a6ef6..72fab8535d 100644 --- a/cmd/seid/cmd/configmanager/decode_report_test.go +++ b/cmd/seid/cmd/configmanager/decode_report_test.go @@ -67,25 +67,34 @@ func asUint(t *testing.T, v any) uint64 { // still sees what a delivery changed. func TestAPasswordInASettingDoesNotReachTheReport(t *testing.T) { const password = "sup3rs3cret" - const dsn = "postgres://seid:" + password + "@10.0.0.9:5432/idx" - var out bytes.Buffer - log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug})) - reportWhatMoved("tx-index", - []string{"tx-index.psql-conn"}, - map[string]string{"tx-index.psql-conn": ""}, - map[string]string{"tx-index.psql-conn": dsn}, - log) + // Every form PostgreSQL accepts a password in. The userinfo of a URL is the one that comes to mind; + // the other three are just as writable and were the ones that leaked. + for _, dsn := range []string{ + "postgres://seid:" + password + "@10.0.0.9:5432/idx", + "postgres://seid@10.0.0.9:5432/idx?password=" + password, + "postgres://seid@10.0.0.9:5432/idx?sslpassword=" + password, + "host=10.0.0.9 port=5432 user=seid password=" + password + " dbname=idx", + "postgresql://seid@10.0.0.9/idx?password=" + password + "&sslmode=require", + } { + var out bytes.Buffer + log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug})) + reportWhatMoved("tx-index", + []string{"tx-index.psql-conn"}, + map[string]string{"tx-index.psql-conn": ""}, + map[string]string{"tx-index.psql-conn": dsn}, + log) - if strings.Contains(out.String(), password) { - t.Errorf("the report carries the password from a connection string: %s", out.String()) - } - if !strings.Contains(out.String(), "10.0.0.9:5432") { - t.Errorf("the report no longer says where the index writes, so an operator cannot tell what "+ - "moved: %s", out.String()) - } - if !strings.Contains(out.String(), "tx-index.psql-conn") { - t.Errorf("the report does not name the key that moved: %s", out.String()) + if strings.Contains(out.String(), password) { + t.Errorf("the report carries the password from %q:\n%s", dsn, out.String()) + } + if !strings.Contains(out.String(), "10.0.0.9") { + t.Errorf("the report no longer says where the index writes, so an operator cannot tell what "+ + "moved: %s", out.String()) + } + if !strings.Contains(out.String(), "tx-index.psql-conn") { + t.Errorf("the report does not name the key that moved: %s", out.String()) + } } } @@ -102,6 +111,8 @@ func TestAValueWithNoPasswordIsReportedAsWritten(t *testing.T) { "", "postgres://seid@10.0.0.9:5432/idx", "a,b,c", + "host=10.0.0.9 port=5432 user=seid dbname=idx", + "postgres://seid@10.0.0.9/idx?sslmode=require", } { if got := withoutCredentials(value); got != value { t.Errorf("%q is reported as %q, and an operator reading it back has to see what they wrote", diff --git a/cmd/seid/cmd/configmanager/tendermint.go b/cmd/seid/cmd/configmanager/tendermint.go index 60baeb84af..12d6e6d895 100644 --- a/cmd/seid/cmd/configmanager/tendermint.go +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -6,6 +6,7 @@ import ( "log/slog" "net/url" "os" + "regexp" "strings" "github.com/spf13/viper" @@ -194,21 +195,35 @@ func asSet(keys []string) map[string]struct{} { return out } +// redacted stands in for a secret a report would otherwise carry. +const redacted = "xxxxx" + +// passwordAssignment matches a password given as a named field, wherever it appears. +// +// A connection string carries one three ways and this covers two of them: a query parameter, as in +// "?password=", and one keyword of a space-separated list, as in "password=". PostgreSQL accepts both, +// and it accepts prefixed spellings such as "sslpassword", which the leading group keeps intact. +// +// The value runs to the next separator or the end, so nothing after it is swallowed. +var passwordAssignment = regexp.MustCompile(`(?i)(password|passwd|pwd)(\s*=\s*)[^&\s]*`) + // withoutCredentials removes a password from a value that carries one. // -// A setting can hold a connection string, and a connection string can hold a password. Detected in the -// value rather than declared per key, because a list of the keys that can hold one is a list somebody keeps -// in step with every section anyone adds, and the first key forgotten is a password in a log. +// A setting can hold a connection string, and a connection string can carry a password in the userinfo of +// a URL, in a query parameter, or as one keyword of a list. All three are forms an operator can write, so +// all three are removed. +// +// Detected in the value rather than declared per key, because a list of the keys that can hold one is a +// list somebody keeps in step with every section anyone adds, and the first key forgotten is a password in +// a log. func withoutCredentials(value string) string { - u, err := url.Parse(value) - if err != nil || u.User == nil { - return value - } - if _, set := u.User.Password(); !set { - return value + if u, err := url.Parse(value); err == nil && u.User != nil { + if _, set := u.User.Password(); set { + u.User = url.UserPassword(u.User.Username(), redacted) + value = u.String() + } } - u.User = url.UserPassword(u.User.Username(), "xxxxx") - return u.String() + return passwordAssignment.ReplaceAllString(value, "${1}${2}"+redacted) } // applyResolvedLogLevel hands a resolved log level to the logger, which the struct alone does not reach. From 8bd1f133a3780b3fd3a1188d5ba112d3c05b5f01 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 31 Aug 2026 08:08:20 -0700 Subject: [PATCH 05/10] fix(config): name the subject of a doc comment, and drop the report this delivery makes untrue The doc comment on the decode guard still opened with the name it had before the rename, so it did not name the function it documents. Swept the rest of the package: every other doc comment names its own subject. The install's report of keys it holds back goes away here, because this is where they stop being held back. The delivery it describes now exists, so the report that said those values do not arrive would be the false one. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/configmanager/tendermint_copy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/seid/cmd/configmanager/tendermint_copy.go b/cmd/seid/cmd/configmanager/tendermint_copy.go index 0d282f6257..c7f41a4e1d 100644 --- a/cmd/seid/cmd/configmanager/tendermint_copy.go +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -197,7 +197,7 @@ func DescribeForTest(t *testing.T, cfg *tmcfg.Config, keys []string) map[string] return values } -// refuseWhatDecodesToSomethingElse reports written values the decoder accepts and turns into something the +// whatDecodesToSomethingElse reports written values the decoder accepts and turns into something the // operator did not mean, with what they should have written. // // Four shapes, and every one of them decodes cleanly, which is why nothing later objects. From b7bc3ea4bb9add8c0745313ebdadafe53f3de284 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 31 Aug 2026 09:00:46 -0700 Subject: [PATCH 06/10] fix(config): hold a delivered section to the node's own rules A value can decode cleanly, mean exactly what it says, and still be one the node refuses. A transaction-size ceiling of minus one is a valid int, so none of the four shapes the decode guard covers applies to it. It decoded to -1 and published, and the node then measured every transaction against it and found all of them larger, so it would have accepted none. Around thirty checks of that kind live in the node's own validation and nothing above this could see any of them. Checked on the rehearsal copy, which is the one place a copy exists to check, so it inherits the section-scoped refusal: a bad value costs its own section rather than the file. This is parity rather than a regression, since an existing config.toml is not validated either, but the copy makes it cheap to close for the channel added here. A quoted password leaked its tail. PostgreSQL accepts a keyword value in quotes and a password may hold spaces, so a run that stopped at the first one redacted the first word and left the rest in the log line. The test could not see it either, because it compared the whole secret and the first word was genuinely gone; it now asserts on every word. The production file no longer imports testing. The helper that needed it takes the two methods it uses as an interface instead, which keeps the property that mattered: it can still end a test, so it cannot answer without having measured. The type walk beside it moved into the test that is its only caller. Five other dependencies already pull testing into this binary, so that part of the concern was not new, but a production file importing it is worth not adding to. Three doc comments named the mechanism this delivery deliberately does not use, and a test variable carried a number that meant nothing to it. Co-Authored-By: Claude Opus 5 (1M context) --- .../cmd/configmanager/decode_report_test.go | 34 ++++++++----- cmd/seid/cmd/configmanager/tendermint.go | 32 ++++++++++--- cmd/seid/cmd/configmanager/tendermint_copy.go | 48 +++++-------------- .../cmd/configmanager/tendermint_copy_test.go | 48 ++++++++++++++++--- cmd/seid/cmd/node_delivery_test.go | 25 +++++++++- 5 files changed, 123 insertions(+), 64 deletions(-) diff --git a/cmd/seid/cmd/configmanager/decode_report_test.go b/cmd/seid/cmd/configmanager/decode_report_test.go index 72fab8535d..997e1ac22a 100644 --- a/cmd/seid/cmd/configmanager/decode_report_test.go +++ b/cmd/seid/cmd/configmanager/decode_report_test.go @@ -66,27 +66,35 @@ func asUint(t *testing.T, v any) uint64 { // The report cannot be turned down either: this package holds its own logger at a floor so a quiet fleet // still sees what a delivery changed. func TestAPasswordInASettingDoesNotReachTheReport(t *testing.T) { - const password = "sup3rs3cret" - - // Every form PostgreSQL accepts a password in. The userinfo of a URL is the one that comes to mind; - // the other three are just as writable and were the ones that leaked. - for _, dsn := range []string{ - "postgres://seid:" + password + "@10.0.0.9:5432/idx", - "postgres://seid@10.0.0.9:5432/idx?password=" + password, - "postgres://seid@10.0.0.9:5432/idx?sslpassword=" + password, - "host=10.0.0.9 port=5432 user=seid password=" + password + " dbname=idx", - "postgresql://seid@10.0.0.9/idx?password=" + password + "&sslmode=require", + // Each case carries its own secret, and the whole secret has to be gone. A shared one-word secret + // hides the failure that matters here: a run that stops at the first space redacts the first word and + // leaves the rest, and the rest is still the password. + for _, tc := range []struct{ secret, dsn string }{ + {"sup3rs3cret", "postgres://seid:sup3rs3cret@10.0.0.9:5432/idx"}, + {"sup3rs3cret", "postgres://seid@10.0.0.9:5432/idx?password=sup3rs3cret"}, + {"sup3rs3cret", "postgres://seid@10.0.0.9:5432/idx?sslpassword=sup3rs3cret"}, + {"sup3rs3cret", "host=10.0.0.9 port=5432 user=seid password=sup3rs3cret dbname=idx"}, + {"sup3rs3cret", "postgresql://seid@10.0.0.9/idx?password=sup3rs3cret&sslmode=require"}, + // PostgreSQL accepts a quoted keyword value, and a password may hold spaces. + {"alpha bravo charlie", "host=10.0.0.9 user=seid password='alpha bravo charlie' dbname=idx"}, + {"alpha bravo charlie", `host=10.0.0.9 user=seid password="alpha bravo charlie" dbname=idx`}, } { var out bytes.Buffer log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug})) reportWhatMoved("tx-index", []string{"tx-index.psql-conn"}, map[string]string{"tx-index.psql-conn": ""}, - map[string]string{"tx-index.psql-conn": dsn}, + map[string]string{"tx-index.psql-conn": tc.dsn}, log) - if strings.Contains(out.String(), password) { - t.Errorf("the report carries the password from %q:\n%s", dsn, out.String()) + // Every word of the secret, not the whole string. A run that stops at the first space redacts the + // first word and leaves the rest, and the rest is still the password: checking only for the whole + // secret cannot see that, because the first word is genuinely gone. + for _, word := range strings.Fields(tc.secret) { + if strings.Contains(out.String(), word) { + t.Errorf("the report carries %q, part of the password in %q:\n%s", + word, tc.dsn, out.String()) + } } if !strings.Contains(out.String(), "10.0.0.9") { t.Errorf("the report no longer says where the index writes, so an operator cannot tell what "+ diff --git a/cmd/seid/cmd/configmanager/tendermint.go b/cmd/seid/cmd/configmanager/tendermint.go index 12d6e6d895..2da9a1ed34 100644 --- a/cmd/seid/cmd/configmanager/tendermint.go +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -57,11 +57,12 @@ func deliverDecodedSections(ctx *server.Context, bySection map[string]map[string // deliverOneSection decodes one section's resolved values into the node's configuration. // -// Decoded into a copy of that configuration first, and published by replacing it. A decoder gathers errors -// and keeps going, so a value it refuses partway leaves its target holding some of the new values and some -// of the old, with nothing to compare against and no way back. Rehearsing into a copy of the configuration -// the node already has, rather than into a fresh one, is what makes the rehearsal answer the same question: -// what a decoder writes can depend on what the target already holds, and only a copy holds the same things. +// Decoded into a copy of that configuration first, and published into the live one only once the whole +// section has decoded. A decoder gathers errors and keeps going, so a value it refuses partway leaves its +// target holding some of the new values and some of the old, with nothing to compare against and no way +// back. Rehearsing into a copy of the configuration the node already has, rather than into a fresh one, is +// what makes the rehearsal answer the same question: what a decoder writes can depend on what the target +// already holds, and only a copy holds the same things. func deliverOneSection(ctx *server.Context, name string, values map[string]any, log *slog.Logger) { keys := sortedKeys(values) @@ -97,6 +98,20 @@ func deliverOneSection(ctx *server.Context, name string, values map[string]any, return } + // The node's own rules, on the copy, before anything is published. A value can decode cleanly, mean + // what it says, and still be one the node refuses: a negative transaction-size ceiling decodes to + // minus one and then every transaction measures larger than it, so the node accepts none. Around + // thirty such checks live here and nothing above this can see any of them. + // + // Cheap here and nowhere else, because this is the one place a copy exists to test. It also inherits + // the section-scoped refusal, so a bad value costs its own section and not the whole file. + if err := candidate.ValidateBasic(); err != nil { + log.Error("this section's written values leave the node's configuration invalid, so none of the "+ + "section is applied and every one of its keys reads as it always has", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } + if err := publishNodeConfig(ctx.Config, candidate); err != nil { log.Error("cannot publish this node's configuration, so these keys read as they always have", "section", name, "keys", strings.Join(keys, ","), "err", err) @@ -204,8 +219,11 @@ const redacted = "xxxxx" // "?password=", and one keyword of a space-separated list, as in "password=". PostgreSQL accepts both, // and it accepts prefixed spellings such as "sslpassword", which the leading group keeps intact. // -// The value runs to the next separator or the end, so nothing after it is swallowed. -var passwordAssignment = regexp.MustCompile(`(?i)(password|passwd|pwd)(\s*=\s*)[^&\s]*`) +// The value runs to the next separator or the end, so nothing after it is swallowed. A quoted value is +// taken whole, because PostgreSQL accepts a keyword value in quotes and a password may hold spaces: +// stopping at the first one leaves the rest of it in the line. +var passwordAssignment = regexp.MustCompile( + `(?i)(password|passwd|pwd)(\s*=\s*)('[^']*'|"[^"]*"|[^&\s]*)`) // withoutCredentials removes a password from a value that carries one. // diff --git a/cmd/seid/cmd/configmanager/tendermint_copy.go b/cmd/seid/cmd/configmanager/tendermint_copy.go index c7f41a4e1d..292bf3522c 100644 --- a/cmd/seid/cmd/configmanager/tendermint_copy.go +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -6,7 +6,6 @@ import ( "reflect" "sort" "strings" - "testing" "time" "github.com/go-viper/mapstructure/v2" @@ -176,6 +175,16 @@ func flatten(prefix string, in map[string]any, out map[string]any) { } } +// TestReporter is the part of a test's own type this file needs. +// +// Named as an interface rather than taking *testing.T, so this file does not pull the testing package into +// a binary the boot path links. The behaviour is the point: the helper below has to be able to end the +// test, because a caller that could carry on would compare two answers produced by reading nothing. +type TestReporter interface { + Helper() + Fatalf(format string, args ...any) +} + // DescribeForTest reads what a node's configuration holds for each key, as text. // // Exported for the tests that measure a booted node's configuration, which live beside the boot because @@ -184,7 +193,7 @@ func flatten(prefix string, in map[string]any, out map[string]any) { // It fails the test rather than answering partially. A caller comparing two of these answers over a hundred // keys finds every value equal when both are empty, so an answer produced by reading nothing is // indistinguishable from a node where nothing moved. -func DescribeForTest(t *testing.T, cfg *tmcfg.Config, keys []string) map[string]string { +func DescribeForTest(t TestReporter, cfg *tmcfg.Config, keys []string) map[string]string { t.Helper() values, unread, err := describe(cfg, keys) if err != nil { @@ -368,41 +377,6 @@ func reachesTheFieldAsItself(n float64, ft reflect.Type) bool { return true } -// referencePathsIn returns every path in a type that a copy has to detach, for the test that holds the -// detach to the type it walks. -func referencePathsIn(t reflect.Type, path string, seen map[reflect.Type]bool) []string { - if seen[t] { - return nil - } - seen[t] = true - defer delete(seen, t) - - var out []string - switch t.Kind() { - case reflect.Pointer, reflect.Slice, reflect.Map, reflect.Interface: - if path != "" { - out = append(out, path) - } - if t.Kind() != reflect.Interface { - out = append(out, referencePathsIn(t.Elem(), path, seen)...) - } - case reflect.Array: - // The array itself is not a reference, so it is not a path of its own. What it holds can be, and - // leaving this case out gives this walk the same blind spot as the copy it holds to account. - out = append(out, referencePathsIn(t.Elem(), path, seen)...) - case reflect.Struct: - for i := 0; i < t.NumField(); i++ { - f := t.Field(i) - if f.PkgPath != "" { - continue - } - out = append(out, referencePathsIn(f.Type, join(path, f.Name), seen)...) - } - } - sort.Strings(out) - return out -} - // publishNodeConfig makes a node's configuration hold what the candidate holds, without replacing it. // // Field by field rather than by assigning the whole struct. The configuration is one struct behind one diff --git a/cmd/seid/cmd/configmanager/tendermint_copy_test.go b/cmd/seid/cmd/configmanager/tendermint_copy_test.go index c9e2458969..cfc620be62 100644 --- a/cmd/seid/cmd/configmanager/tendermint_copy_test.go +++ b/cmd/seid/cmd/configmanager/tendermint_copy_test.go @@ -2,6 +2,7 @@ package configmanager import ( "reflect" + "sort" "testing" tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" @@ -9,7 +10,7 @@ import ( // TestTheCopyShareNothingWithWhatItCopied is the property the rollback rests on. // -// The delivery decodes into a copy and publishes by replacing, so a refused value leaves the node's +// The delivery decodes into a copy and publishes into the live one, so a refused value leaves the node's // configuration untouched. That holds only if the copy shares nothing the decode can write through, and a // decoder writes a list into the array its target already holds. One shared section, list or map and the // rehearsal edits the original. @@ -138,16 +139,16 @@ func TestPublishingKeepsThePointerEverySectionIsHeldBy(t *testing.T) { } held := map[string]uintptr{} - held4321 := reflect.ValueOf(target).Elem() - for i := 0; i < held4321.NumField(); i++ { - f := held4321.Type().Field(i) + live := reflect.ValueOf(target).Elem() + for i := 0; i < live.NumField(); i++ { + f := live.Type().Field(i) if f.Type.Kind() != reflect.Pointer || f.Type.Elem().Kind() != reflect.Struct { continue } - if held4321.Field(i).IsNil() { + if live.Field(i).IsNil() { continue } - held[f.Name] = held4321.Field(i).Pointer() + held[f.Name] = live.Field(i).Pointer() } if len(held) == 0 { t.Fatal("this configuration holds no section behind a pointer of its own, so there is no identity " + @@ -171,3 +172,38 @@ func TestPublishingKeepsThePointerEverySectionIsHeldBy(t *testing.T) { "the value arrives through it", got) } } + +// referencePathsIn returns every path in a type that a copy has to detach, for the test that holds the +// detach to the type it walks. +func referencePathsIn(t reflect.Type, path string, seen map[reflect.Type]bool) []string { + if seen[t] { + return nil + } + seen[t] = true + defer delete(seen, t) + + var out []string + switch t.Kind() { + case reflect.Pointer, reflect.Slice, reflect.Map, reflect.Interface: + if path != "" { + out = append(out, path) + } + if t.Kind() != reflect.Interface { + out = append(out, referencePathsIn(t.Elem(), path, seen)...) + } + case reflect.Array: + // The array itself is not a reference, so it is not a path of its own. What it holds can be, and + // leaving this case out gives this walk the same blind spot as the copy it holds to account. + out = append(out, referencePathsIn(t.Elem(), path, seen)...) + case reflect.Struct: + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + if f.PkgPath != "" { + continue + } + out = append(out, referencePathsIn(f.Type, join(path, f.Name), seen)...) + } + } + sort.Strings(out) + return out +} diff --git a/cmd/seid/cmd/node_delivery_test.go b/cmd/seid/cmd/node_delivery_test.go index 709cf984df..77ca89c0a0 100644 --- a/cmd/seid/cmd/node_delivery_test.go +++ b/cmd/seid/cmd/node_delivery_test.go @@ -111,7 +111,8 @@ func TestAnUnwrittenKeyTakesTheDeclaredValue(t *testing.T) { // // A decoder gathers errors and keeps going, so a value it refuses partway leaves its target holding some // new values and some old, with nothing to compare against. The delivery decodes into a copy and publishes -// by replacing, so a refused value leaves the section exactly as the node had it. +// into the live one only once the whole section decodes, so a refused value leaves the section exactly as +// the node had it. func TestARefusedValueLeavesItsSectionAlone(t *testing.T) { configtest.Isolate(t) ctx := bootWithNodeFile(t, nodeFileHeader+ @@ -437,3 +438,25 @@ func keysADecodeDelivers() []string { _, keys := registry.ResolvedAndOwnedByDecodedSections(registry.Resolved{}) return keys } + +// TestAValueTheNodesOwnRulesRejectIsRefused covers what decodes cleanly, means what it says, and still +// breaks the node. +// +// A negative transaction-size ceiling is a valid int, so nothing about its shape is wrong. The node then +// measures every transaction against it and finds all of them larger, so it accepts none. The node's own +// rules refuse exactly this, and they are checked on the rehearsal copy because that is the one place a +// copy exists to check. +func TestAValueTheNodesOwnRulesRejectIsRefused(t *testing.T) { + configtest.Isolate(t) + was := tmcfg.DefaultConfig().Mempool.MaxTxBytes + + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nmax-tx-bytes = -1\nsize = 4321\n", nil) + if got := ctx.Config.Mempool.MaxTxBytes; got != was { + t.Errorf("the node runs a transaction-size ceiling of %d after -1 was written, want the %d it "+ + "had. Every transaction measures larger than a negative ceiling, so the node would accept "+ + "none of them", got, was) + } + if got := ctx.Config.Mempool.Size; got == 4321 { + t.Error("the value beside the refused one was applied, so the section was published in part") + } +} From 9e2d240a76bcabb0bbc0f9f40341e445bfa94b60 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 31 Aug 2026 10:43:38 -0700 Subject: [PATCH 07/10] fix(config): read the decoded key set through the one accessor Two tests here called an accessor the install PR removed once a single read replaced it. They go through the same one read now, so a test cannot describe a registry the boot would not. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/node_delivery_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cmd/seid/cmd/node_delivery_test.go b/cmd/seid/cmd/node_delivery_test.go index 77ca89c0a0..4cbe00c0cc 100644 --- a/cmd/seid/cmd/node_delivery_test.go +++ b/cmd/seid/cmd/node_delivery_test.go @@ -460,3 +460,12 @@ func TestAValueTheNodesOwnRulesRejectIsRefused(t *testing.T) { t.Error("the value beside the refused one was applied, so the section was published in part") } } + +// keysADecodeDelivers returns every key the decoded sections own, sorted. +// +// Through the one accessor that answers both halves from a single read of the registry, so a test cannot +// see a registry the boot did not. +func keysADecodeDelivers() []string { + _, keys := registry.SuppliedAndOwnedByDecodedSections(registry.Resolved{}) + return keys +} From 9fe3053501b56ba413fcebcb4d0911cec08003bc Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 31 Aug 2026 10:56:31 -0700 Subject: [PATCH 08/10] fix(config): make the redaction fail closed, and render what a pointer leaf holds The redaction failed open on three inputs. A backslash escape inside a quoted keyword value ended the match at the escaped quote, so the tail of the password stayed in the line, and the same held for an escaped space in an unquoted value. Separately, a password holding a byte a URL parser refuses in userinfo made the parse fail, and a connection string in that form carries no named field either, so the whole credential was logged. Escapes are consumed with what follows them now, and a value that does not parse as a URL has its userinfo removed by pattern. The test could not see any of it, so it grew the three cases. Removing either half fails it on two and three leaked words. A pointer to anything other than a struct is left as a pointer by the decoder, so rendering one gives an address, and publishing assigns a fresh pointer for a leaf that is not a struct. The report would have named such a key as moved on every boot and printed two addresses. Nothing reaches it today because every pointer leaf is left undeclared, but the walk around it is driven from the type so that a field added later is covered, and this keeps that true for one more shape. Co-Authored-By: Claude Opus 5 (1M context) --- .../cmd/configmanager/decode_report_test.go | 6 ++++ cmd/seid/cmd/configmanager/tendermint.go | 30 +++++++++++++---- cmd/seid/cmd/configmanager/tendermint_copy.go | 23 ++++++++++++- .../cmd/configmanager/tendermint_copy_test.go | 33 +++++++++++++++++++ 4 files changed, 85 insertions(+), 7 deletions(-) diff --git a/cmd/seid/cmd/configmanager/decode_report_test.go b/cmd/seid/cmd/configmanager/decode_report_test.go index 997e1ac22a..21fff0753e 100644 --- a/cmd/seid/cmd/configmanager/decode_report_test.go +++ b/cmd/seid/cmd/configmanager/decode_report_test.go @@ -78,6 +78,12 @@ func TestAPasswordInASettingDoesNotReachTheReport(t *testing.T) { // PostgreSQL accepts a quoted keyword value, and a password may hold spaces. {"alpha bravo charlie", "host=10.0.0.9 user=seid password='alpha bravo charlie' dbname=idx"}, {"alpha bravo charlie", `host=10.0.0.9 user=seid password="alpha bravo charlie" dbname=idx`}, + // A backslash escape inside a quoted value. Ending the match at the escaped quote leaks the tail. + {`alpha' bravo charlie`, `host=10.0.0.9 user=seid password='alpha\' bravo charlie' dbname=idx`}, + // The same unquoted, where the space itself is escaped. + {`alpha bravo`, `host=10.0.0.9 user=seid password=alpha\ bravo dbname=idx`}, + // A password a URL parser refuses in userinfo, so the parse fails and no named field exists. + {"alpha bravo", "postgres://seid:alpha bravo@10.0.0.9:5432/idx"}, } { var out bytes.Buffer log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug})) diff --git a/cmd/seid/cmd/configmanager/tendermint.go b/cmd/seid/cmd/configmanager/tendermint.go index 2da9a1ed34..3e2278143c 100644 --- a/cmd/seid/cmd/configmanager/tendermint.go +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -168,9 +168,11 @@ func copyNodeConfig(from *tmcfg.Config) (*tmcfg.Config, error) { // Keys that did not move are not reported. An operator who writes the value their file already held has // changed nothing, and a line saying so buries the ones that did. // -// A value carrying a password has the password taken out. This is the only place the running configuration -// is written down, so it is also the only place one would reach a log file, a journal, and whatever ships -// them onward. The node's own configuration file holds the same string and nothing reads it out to a log. +// A value carrying a password has the password taken out, in the forms a connection string writes one: the +// userinfo of a URL, a named field, quoted or not, with or without backslash escapes. This is the only +// place the running configuration is written down, so it is also the only place one would reach a log file, +// a journal, and whatever ships them onward. The node's own configuration file holds the same string and +// nothing reads it out to a log. // // The rendered list is capped for the reason every other one here is: the count is what an operator alerts // on, and one line per key of a large section buries whichever of them mattered. @@ -220,10 +222,21 @@ const redacted = "xxxxx" // and it accepts prefixed spellings such as "sslpassword", which the leading group keeps intact. // // The value runs to the next separator or the end, so nothing after it is swallowed. A quoted value is -// taken whole, because PostgreSQL accepts a keyword value in quotes and a password may hold spaces: -// stopping at the first one leaves the rest of it in the line. +// taken whole, because a keyword value may be quoted and a password may hold spaces: stopping at the first +// one leaves the rest of it in the line. +// +// A backslash escape is consumed with what follows it. Quoted and unquoted keyword values both escape a +// quote and a backslash that way, so a password holding one ends the match early and leaks its tail. var passwordAssignment = regexp.MustCompile( - `(?i)(password|passwd|pwd)(\s*=\s*)('[^']*'|"[^"]*"|[^&\s]*)`) + `(?i)(password|passwd|pwd)(\s*=\s*)('(?:\\.|[^'\\])*'|"(?:\\.|[^"\\])*"|(?:\\.|[^&\s\\])*)`) + +// credentialsInAURI matches the userinfo of a connection string that does not parse as a URL. +// +// A password holding a raw space, or any other byte a URL parser rejects in userinfo, makes the parse fail, +// and a connection string in that form carries no named field for the pattern above to find. Failing open +// there logs the whole credential, so this is the fallback: whatever sits between the scheme and the host, +// after the first colon, is removed. +var credentialsInAURI = regexp.MustCompile(`(://[^/@\s]*:)[^@\n]*(@)`) // withoutCredentials removes a password from a value that carries one. // @@ -240,6 +253,11 @@ func withoutCredentials(value string) string { u.User = url.UserPassword(u.User.Username(), redacted) value = u.String() } + } else { + // The parse failed, so the userinfo above was never reached and a string in that form carries no + // named field either. Failing open here would log the whole credential, which is the one outcome + // this function exists to prevent. + value = credentialsInAURI.ReplaceAllString(value, "${1}"+redacted+"${2}") } return passwordAssignment.ReplaceAllString(value, "${1}${2}"+redacted) } diff --git a/cmd/seid/cmd/configmanager/tendermint_copy.go b/cmd/seid/cmd/configmanager/tendermint_copy.go index 292bf3522c..87315db908 100644 --- a/cmd/seid/cmd/configmanager/tendermint_copy.go +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -171,10 +171,31 @@ func flatten(prefix string, in map[string]any, out map[string]any) { flatten(path, inner, out) continue } - out[path] = value + out[path] = whatAPointerHolds(value) } } +// whatAPointerHolds returns the value behind a pointer, or the value itself. +// +// The decoder flattens a pointer to a struct by following it and leaves a pointer to anything else as a +// pointer. Rendering one of those gives an address, and an address is a different one on each side of a +// delivery, because publishing assigns a fresh pointer for a leaf that is not a struct. So the report would +// name the key as moved on every boot and print two addresses for it. +// +// Nothing reaches this today: every pointer leaf the node's configuration carries is deliberately left +// undeclared. The walk around it is driven from the type so that a field added later is covered, and this +// keeps that true for one more shape. +func whatAPointerHolds(value any) any { + v := reflect.ValueOf(value) + if v.Kind() != reflect.Pointer { + return value + } + if v.IsNil() { + return nil + } + return v.Elem().Interface() +} + // TestReporter is the part of a test's own type this file needs. // // Named as an interface rather than taking *testing.T, so this file does not pull the testing package into diff --git a/cmd/seid/cmd/configmanager/tendermint_copy_test.go b/cmd/seid/cmd/configmanager/tendermint_copy_test.go index cfc620be62..0033634747 100644 --- a/cmd/seid/cmd/configmanager/tendermint_copy_test.go +++ b/cmd/seid/cmd/configmanager/tendermint_copy_test.go @@ -1,6 +1,7 @@ package configmanager import ( + "fmt" "reflect" "sort" "testing" @@ -207,3 +208,35 @@ func referencePathsIn(t reflect.Type, path string, seen map[reflect.Type]bool) [ sort.Strings(out) return out } + +// TestAPointerLeafRendersWhatItHolds covers a shape the report would name as moved on every boot. +// +// The decoder follows a pointer to a struct and leaves a pointer to anything else alone. Rendered, that is +// an address, and publishing assigns a fresh pointer for a leaf that is not a struct, so the two sides of a +// delivery hold different addresses for a value nobody changed. The report would say the key moved and +// print two of them. +// +// No pointer leaf is declared today. This holds the rendering to being right for one anyway, because the +// walk around it is driven from the type so that a field added later is covered. +func TestAPointerLeafRendersWhatItHolds(t *testing.T) { + seven := uint(7) + for _, tc := range []struct { + name string + value any + want string + }{ + {"a pointer to a number", &seven, "7"}, + {"a nil pointer", (*uint)(nil), ""}, + {"a plain number", uint(7), "7"}, + {"a string", "kv", "kv"}, + } { + t.Run(tc.name, func(t *testing.T) { + out := map[string]any{} + flatten("", map[string]any{"probe": tc.value}, out) + if got := fmt.Sprint(out["probe"]); got != tc.want { + t.Errorf("%s renders as %q, want %q. An address differs on each side of a delivery, so "+ + "the report would name the key as moved every boot", tc.name, got, tc.want) + } + }) + } +} From 3e049430d18e7e824b81587d9cb6b075086ae771 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 31 Aug 2026 12:52:41 -0700 Subject: [PATCH 09/10] fix(config): do not blame a section for a node already breaking its own rules The node's rules answer for the whole configuration, and a boot never applies them to an existing config.toml, so a node can already hold a value they reject. Refusing on that blamed the section being delivered for a failure it did not cause and left every later change unable to land on that node. The copy is now compared against what the node already holds: a failure that was already there is reported and the section is applied, and only a failure this section introduced refuses it. The keys neither side could be read for are dropped before the comparison. Naming them and then comparing them anyway let the report say the section matches the node's own file, which is the statement naming them exists to withhold. The filter is its own step so it can be measured, because it had no test when it was inline. Co-Authored-By: Claude Opus 5 (1M context) --- .../cmd/configmanager/decode_report_test.go | 31 ++++++++++++++ cmd/seid/cmd/configmanager/tendermint.go | 42 +++++++++++++++---- cmd/seid/cmd/node_delivery_test.go | 19 +++++++++ 3 files changed, 85 insertions(+), 7 deletions(-) diff --git a/cmd/seid/cmd/configmanager/decode_report_test.go b/cmd/seid/cmd/configmanager/decode_report_test.go index 21fff0753e..a42a498982 100644 --- a/cmd/seid/cmd/configmanager/decode_report_test.go +++ b/cmd/seid/cmd/configmanager/decode_report_test.go @@ -134,3 +134,34 @@ func TestAValueWithNoPasswordIsReportedAsWritten(t *testing.T) { } } } + +// TestAnUnreadKeyIsNotComparedAsUnchanged covers the statement the report must withhold. +// +// A key neither side could be read for is absent from both answers, so a comparison finds it equal and says +// it did not move. That is what a key an operator wrote and got looks like, produced by having read nothing, +// and the line naming the unread keys exists precisely so the report does not claim it. +func TestAnUnreadKeyIsNotComparedAsUnchanged(t *testing.T) { + keys := []string{"mempool.size", "mempool.ttl-duration", "mempool.max-tx-bytes"} + unread := asSet([]string{"mempool.ttl-duration"}) + + got := whatBothSidesCouldBeReadFor(keys, unread) + want := []string{"mempool.size", "mempool.max-tx-bytes"} + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Fatalf("the keys compared are %v, want %v", got, want) + } + + // The report over the surviving keys must still say what moved, so the filter cannot be a way of + // reporting nothing. + var out bytes.Buffer + log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug})) + reportWhatMoved("mempool", got, + map[string]string{"mempool.size": "5000", "mempool.max-tx-bytes": "1048576"}, + map[string]string{"mempool.size": "4321", "mempool.max-tx-bytes": "1048576"}, + log) + if !strings.Contains(out.String(), "mempool.size") { + t.Errorf("the report does not name the key that moved:\n%s", out.String()) + } + if strings.Contains(out.String(), "ttl-duration") { + t.Errorf("the report names a key neither side could be read for:\n%s", out.String()) + } +} diff --git a/cmd/seid/cmd/configmanager/tendermint.go b/cmd/seid/cmd/configmanager/tendermint.go index 3e2278143c..0f6e7fe7af 100644 --- a/cmd/seid/cmd/configmanager/tendermint.go +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -105,11 +105,21 @@ func deliverOneSection(ctx *server.Context, name string, values map[string]any, // // Cheap here and nowhere else, because this is the one place a copy exists to test. It also inherits // the section-scoped refusal, so a bad value costs its own section and not the whole file. + // Against what the node already holds, not against the rules alone. ValidateBasic answers for the whole + // configuration, and the boot never applies it to an existing config.toml, so a node can already be in + // a state these rules reject. Refusing on that would blame this section for a failure it did not cause + // and leave every later change unable to land. if err := candidate.ValidateBasic(); err != nil { - log.Error("this section's written values leave the node's configuration invalid, so none of the "+ - "section is applied and every one of its keys reads as it always has", - "section", name, "keys", strings.Join(keys, ","), "err", err) - return + if already := ctx.Config.ValidateBasic(); already != nil { + log.Warn("this node's configuration already fails its own rules, so this section cannot be "+ + "held to them; it is applied as written", + "section", name, "keys", strings.Join(keys, ","), "already", already) + } else { + log.Error("this section's written values leave the node's configuration invalid, so none of "+ + "the section is applied and every one of its keys reads as it always has", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } } if err := publishNodeConfig(ctx.Config, candidate); err != nil { @@ -128,13 +138,15 @@ func deliverOneSection(ctx *server.Context, name string, values map[string]any, } // The same hazard one key at a time. A key absent from both answers compares equal, so it would be // reported as a setting that did not move, which is what a key an operator wrote and got looks like. - if unread := append(unreadBefore, unreadAfter...); len(unread) > 0 { - shown, omitted := capLoggedItems(sortedKeys(asSet(unread))) + unread := asSet(append(unreadBefore, unreadAfter...)) + if len(unread) > 0 { + shown, omitted := capLoggedItems(sortedKeys(unread)) log.Error("this section was applied and some of its keys cannot be read back, so nothing here "+ "says whether those moved", "section", name, "count", len(shown)+omitted, "keys", strings.Join(shown, ","), "omitted", omitted) } - reportWhatMoved(name, keys, before, after, log) + + reportWhatMoved(name, whatBothSidesCouldBeReadFor(keys, unread), before, after, log) } // copyNodeConfig returns a configuration that holds what this one holds and shares nothing with it. @@ -158,6 +170,22 @@ func copyNodeConfig(from *tmcfg.Config) (*tmcfg.Config, error) { return &out, nil } +// whatBothSidesCouldBeReadFor drops the keys neither side could be read for. +// +// A key missing from both answers compares equal, so leaving it in makes the report say the section matches +// the node's own file, which is the statement the line above it exists to withhold. Dropped here rather +// than inside the comparison, because the comparison's job is to say what moved and this one's is to say +// which keys it can speak for. +func whatBothSidesCouldBeReadFor(keys []string, unread map[string]struct{}) []string { + out := make([]string, 0, len(keys)) + for _, key := range keys { + if _, missing := unread[key]; !missing { + out = append(out, key) + } + } + return out +} + // reportWhatMoved names every key whose value the delivery changed, and what it changed from. // // The node's own configuration file still says what it said, and every tool an operator reaches for reads diff --git a/cmd/seid/cmd/node_delivery_test.go b/cmd/seid/cmd/node_delivery_test.go index 4cbe00c0cc..28559cd36d 100644 --- a/cmd/seid/cmd/node_delivery_test.go +++ b/cmd/seid/cmd/node_delivery_test.go @@ -469,3 +469,22 @@ func keysADecodeDelivers() []string { _, keys := registry.SuppliedAndOwnedByDecodedSections(registry.Resolved{}) return keys } + +// TestASectionLandsOnANodeAlreadyFailingItsOwnRules covers a node the boot never validated. +// +// The node's rules answer for the whole configuration, and a boot does not apply them to an existing +// config.toml, so a node can already hold a value they reject. Refusing on that blames the section being +// delivered for a failure it did not cause, and leaves every later change unable to land on that node. +func TestASectionLandsOnANodeAlreadyFailingItsOwnRules(t *testing.T) { + configtest.Isolate(t) + + // A node whose own file already holds a value its rules reject, in a section nobody is changing. + alreadyInvalid := func(c *tmcfg.Config) { c.Mempool.MaxTxBytes = -1 } + + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[p2p]\nmax-incoming-connection-attempts = 7\n", alreadyInvalid) + if got := ctx.Config.P2P.MaxIncomingConnectionAttempts; got != 7 { + t.Errorf("the written value is %d on a node that already failed its own rules, want 7. A section "+ + "cannot be held to rules the node was already breaking, or nothing can ever be delivered to "+ + "it again", got) + } +} From ed5b3a9e661064498f2d4f5993755efc49552a03 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 31 Aug 2026 14:42:30 -0700 Subject: [PATCH 10/10] feat(config)!: the decode delivers every declared key, not only what a source wrote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A section read by a decode holds what its own file said, and config.toml is not consulted for a declared key under this manager. So every key the resolution answered is handed to the decode, including the ones that took their declared value. A key sei.toml leaves out would otherwise keep whatever config.toml said, which makes the file a patch on the configuration rather than the configuration. That replaces what an operator's config.toml said for a key their sei.toml does not mention, and it is meant to. A path rendering sei.toml from a node's existing files is what makes it safe, and it has to land before this is switched on anywhere. The accessor is named for what it now answers, the delivery no longer returns whether it did anything, and two tests that asserted the old model now assert this one. The one whose failure message said it plainest read "the node's own file turned the metrics listener on, sei.toml said nothing about it, and the node runs with it off" — which is now the correct outcome. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/node_delivery_test.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/cmd/seid/cmd/node_delivery_test.go b/cmd/seid/cmd/node_delivery_test.go index 28559cd36d..3b2e11a42d 100644 --- a/cmd/seid/cmd/node_delivery_test.go +++ b/cmd/seid/cmd/node_delivery_test.go @@ -461,15 +461,6 @@ func TestAValueTheNodesOwnRulesRejectIsRefused(t *testing.T) { } } -// keysADecodeDelivers returns every key the decoded sections own, sorted. -// -// Through the one accessor that answers both halves from a single read of the registry, so a test cannot -// see a registry the boot did not. -func keysADecodeDelivers() []string { - _, keys := registry.SuppliedAndOwnedByDecodedSections(registry.Resolved{}) - return keys -} - // TestASectionLandsOnANodeAlreadyFailingItsOwnRules covers a node the boot never validated. // // The node's rules answer for the whole configuration, and a boot does not apply them to an existing