From 0d27a3424b60272aa24baab2b1da29d39304b661 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 27 Aug 2026 13:04:52 -0700 Subject: [PATCH 1/2] 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 | 50 +++ cmd/seid/cmd/configmanager/install.go | 19 +- cmd/seid/cmd/configmanager/tendermint.go | 229 +++++++++++ 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 | 380 ++++++++++++++++++ 7 files changed, 1260 insertions(+), 3 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..1c5f56c01e --- /dev/null +++ b/cmd/seid/cmd/configmanager/decode_report_test.go @@ -0,0 +1,50 @@ +package configmanager + +import ( + "bytes" + "log/slog" + "strings" + "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" +) + +// TestTheNothingSuppliedLineIsNotSaidWhenADecodeDelivered holds the two reports against each other. +// +// The two deliveries run in one pass and only the second one's emptiness is what that line describes. An +// operator whose file moved a setting through the first was told the file supplied nothing, in the same +// boot and a few lines later, which is the one report they have to go on. +func TestTheNothingSuppliedLineIsNotSaidWhenADecodeDelivered(t *testing.T) { + var out bytes.Buffer + log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug})) + + ctx := &server.Context{Config: tmcfg.DefaultConfig()} + resolved := registry.Resolved{ + Values: map[string]any{"p2p.max-connections": 77}, + Overrides: []string{"p2p.max-connections"}, + } + + delivered := deliverDecodedSections(ctx, resolved, log) + if !delivered { + t.Fatalf("a declared key of a decoded section was not delivered, so this measures nothing:\n%s", + out.String()) + } + if got := ctx.Config.P2P.MaxConnections; got != 77 { + t.Errorf("the peer ceiling is %v after a delivery of 77, so nothing was decoded", got) + } + + // The line the caller would emit for an empty lookup delivery. It must not be said in this boot. + supplied := onlyWhatALookupSourceSupplied(resolved) + if len(supplied.Values) != 0 { + t.Fatalf("this key reached the lookup delivery too, so the contradiction under test cannot arise") + } + if !delivered { + log.Info("sei.toml supplies no declared value; every key reads as it always has", "mode", "full") + } + if strings.Contains(out.String(), "supplies no declared value") { + t.Errorf("the boot reported that the file supplied nothing, having just moved a setting:\n%s", + out.String()) + } +} diff --git a/cmd/seid/cmd/configmanager/install.go b/cmd/seid/cmd/configmanager/install.go index d411e4a91f..556cfa20fd 100644 --- a/cmd/seid/cmd/configmanager/install.go +++ b/cmd/seid/cmd/configmanager/install.go @@ -81,15 +81,28 @@ func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logg "mode", mode, "err", err) return } - // A key nothing declares is the most common thing an operator gets wrong and the only signal they - // have for it. + // First, because every report below is a log line and a refusal is reported at a level an operator + // may have raised the threshold above. Doing this after would mean the one setting somebody changes + // in order to see a refusal is the setting a refusal suppresses. + applyResolvedLogLevel(resolved, typed, log) + + // After the level, so a file that raises it can report its own mistakes. A key nothing declares is + // the most common thing an operator gets wrong and the only signal they have for it. reportWhatTheFileDidNotReach(resolved, log) reportWhatTheFileSaysTheNodeIs(ctx, mode, log) + // The 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. + delivered := deliverDecodedSections(ctx, resolved, log) + supplied := onlyWhatALookupSourceSupplied(resolved) if len(supplied.Values) == 0 { - log.Info("sei.toml supplies no declared value; every key reads as it always has", "mode", mode) + // Scoped to this delivery, because the other one runs first: an operator whose file moved a + // setting through a decode was otherwise told the file supplied nothing, in the same boot. + if !delivered { + log.Info("sei.toml supplies no declared value; every key reads as it always has", "mode", mode) + } return } report, err := appopts.Install(ctx.Viper, supplied) diff --git a/cmd/seid/cmd/configmanager/tendermint.go b/cmd/seid/cmd/configmanager/tendermint.go new file mode 100644 index 0000000000..54998c04bb --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -0,0 +1,229 @@ +package configmanager + +import ( + "cmp" + "fmt" + "log/slog" + "os" + "sort" + "strings" + + "github.com/spf13/viper" + + "github.com/sei-protocol/seilog" + + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// deliverDecodedSections puts the resolved values of the decoded sections into the node's own +// configuration. +// +// Putting a value into the source a node reads is the whole delivery for a section whose reader looks its +// keys up one at a time. It is no delivery at all for the sections this covers, which the boot's handler +// reads once into a struct before this runs. Those values are decoded into that struct instead, which is +// the same mechanism the handler used and therefore the same casts, the same tags and the same hooks. +// +// Nothing here can stop a node starting, which is the one promise this manager makes. +func deliverDecodedSections(ctx *server.Context, resolved registry.Resolved, log *slog.Logger) bool { + bySection := registry.SuppliedByDecodedSection(resolved) + if len(bySection) == 0 { + return false + } + 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 false + } + + // One section at a time. A decode is all or nothing for whatever it is handed, so a single value a + // decoder refuses would otherwise cost every key in the file rather than the keys of the section it + // appeared in. An operator who fixes one setting and mistypes another has to end up with the first + // one applied. + for _, name := range sortedSectionNames(bySection) { + deliverOneSection(ctx, name, bySection[name], log) + } + return true +} + +// deliverOneSection decodes one section's resolved values into the node's configuration. +// +// Decoded into a copy of that configuration first, and published by replacing it. A decoder gathers errors +// and keeps going, so a value it refuses partway leaves its target holding some of the new values and some +// of the old, with nothing to compare against and no way back. Rehearsing into a copy of the configuration +// the node already has, rather than into a fresh one, is what makes the rehearsal answer the same question: +// what a decoder writes can depend on what the target already holds, and only a copy holds the same things. +func deliverOneSection(ctx *server.Context, name string, values map[string]any, log *slog.Logger) { + keys := sortedKeys(values) + + source := viper.New() + for key, value := range values { + source.Set(key, value) + } + + // Refused before the decode, because a plain number where a length of time belongs decodes cleanly + // and means nanoseconds. Nothing after this can tell that apart from a value somebody meant. + if bad := refuseWhatDecodesToSomethingElse(ctx.Config, values); len(bad) > 0 { + log.Error("a length of time in this section is written as a plain number, which reads as "+ + "nanoseconds; none of the section is applied and every one of its keys reads as it always has", + "section", name, "written", strings.Join(bad, "; ")) + return + } + + candidate, err := copyNodeConfig(ctx.Config) + if err != nil { + log.Error("cannot copy this node's configuration, so nothing can be delivered into it without "+ + "risking a half-written one; these keys read as they always have", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } + before, readErr := describe(ctx.Config, keys) + + if err := source.Unmarshal(candidate); err != nil { + log.Error("a written value in this section was refused, so none of the section is applied and "+ + "every one of its keys reads as it always has", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } + + *ctx.Config = *candidate + after, afterErr := describe(ctx.Config, keys) + if readErr != nil || afterErr != nil { + // Reported rather than compared. Two unreadable sides look identical, so comparing them would + // say every value matched, which is a statement about nothing produced by reading nothing. + log.Error("this section was applied and what moved cannot be read, so nothing here says which "+ + "settings now differ from the node's own file", "section", name, + "keys", strings.Join(keys, ","), "err", cmp.Or(readErr, afterErr)) + return + } + reportWhatMoved(name, keys, before, after, log) +} + +// copyNodeConfig returns a configuration that holds what this one holds and shares nothing with it. +// +// A shallow copy would share every section, so a decode into the copy would write through to the original +// and a refused value would leave exactly the half-written configuration the copy exists to prevent. This +// copies the top level and every section under it. +// +// Written against the type rather than field by field, so a section added to it is copied without this +// function changing. A field this cannot copy is an error rather than a silent share. +func copyNodeConfig(from *tmcfg.Config) (*tmcfg.Config, error) { + if from == nil { + return nil, fmt.Errorf("no configuration to copy") + } + out := *from + if err := detachSections(&out, from); err != nil { + return nil, err + } + return &out, nil +} + +// reportWhatMoved names every key whose value the delivery changed, and what it changed from. +// +// The node's own configuration file still says what it said, and every tool an operator reaches for reads +// that file: a patch command, a validator, an audit, somebody reading it over their shoulder at three in +// the morning. None of them describes the running node after this. This log line is the only place the two +// can be told apart, so it names the key, what the file gave it and what the node now runs. +// +// Keys that did not move are not reported. An operator who writes the value their file already held has +// changed nothing, and a line saying so buries the ones that did. +func reportWhatMoved(name string, keys []string, before, after map[string]string, log *slog.Logger) { + var moved []string + for _, key := range keys { + if before[key] != after[key] { + moved = append(moved, fmt.Sprintf("%s: %s -> %s", key, before[key], after[key])) + } + } + if len(moved) == 0 { + log.Info("this section's written values match what the node's own file already gave it", + "section", name, "keys", len(keys)) + return + } + log.Info("this section's settings now differ from what the node's own configuration file says", + "section", name, "changed", strings.Join(moved, "; ")) +} + +// sortedKeys returns a map's keys in a fixed order, so a log line does not vary between runs. +func sortedKeys(values map[string]any) []string { + out := make([]string, 0, len(values)) + for key := range values { + out = append(out, key) + } + sort.Strings(out) + return out +} + +// sortedSectionNames returns the sections to deliver in a fixed order. +func sortedSectionNames(bySection map[string]map[string]any) []string { + out := make([]string, 0, len(bySection)) + for name := range bySection { + out = append(out, name) + } + sort.Strings(out) + return out +} + +// logLevelKey is the one delivered setting the struct is not the end of. +const logLevelKey = "log-level" + +// loggerOwnVariable is the environment variable the logger itself reads when it starts. +// +// Not the variable this key answers to in the resolution, which carries the binary's own prefix. Two names +// for one setting, and the older one is read before any of this runs. +const loggerOwnVariable = "SEI_LOG_LEVEL" + +// applyResolvedLogLevel hands a resolved log level to the logger, which the struct alone does not reach. +// +// The boot's handler reads the level off the struct and sets it before any of this runs, so a value that +// only reaches the struct moves a field and changes no logging. A setting that appears to take and does not +// is what this key space exists to remove. +// +// Applied from the resolution rather than after the decode, and before it. Every failure this manager can +// have is a log line, and a refusal is reported at a level an operator may have raised the threshold above. +// Waiting for a successful decode would mean the one setting somebody changes in order to see a refusal is +// the setting a refusal suppresses. +// +// Which value arrives is already decided: the resolution ranks a flag over the environment over the file. +// A level that cannot be read is reported and skipped, and the node keeps the level it had. +func applyResolvedLogLevel(resolved registry.Resolved, typed map[string]string, log *slog.Logger) { + supplied := false + for _, key := range resolved.Overrides { + if key == logLevelKey { + supplied = true + } + } + if !supplied { + return + } + + // The logger reads a variable of its own at start-up, under a name that is not the one this key + // answers to, and the boot's own handler steps aside when it is set: a flag beats it and a file does + // not. Applying here regardless would put the file above it, so an operator who exported a level and + // then adopted this file would find the level they exported ignored. A typed flag still wins, which is + // the order that was already there. + if _, fromFlag := flagValues(typed)[logLevelKey]; !fromFlag { + if os.Getenv(loggerOwnVariable) != "" { + log.Info("a log level is set in the environment under the logger's own variable, which the "+ + "node already applied; the level this file supplies is not used", + "variable", loggerOwnVariable, "ignored", resolved.Values[logLevelKey]) + return + } + } + text, isText := resolved.Values[logLevelKey].(string) + if !isText { + log.Error("the resolved log level is not text; the node keeps the level it already had", + "value", resolved.Values[logLevelKey]) + return + } + var level slog.Level + if err := level.UnmarshalText([]byte(text)); err != nil { + log.Error("the resolved log level cannot be read; the node keeps the level it already had", + "level", text, "err", err) + return + } + seilog.SetDefaultLevel(level, true) + // That set every logger in the process, this one included, so the floor goes back on. + keepOwnReportingVisible() + log.Info("resolved log level applied", "level", text) +} diff --git a/cmd/seid/cmd/configmanager/tendermint_copy.go b/cmd/seid/cmd/configmanager/tendermint_copy.go new file mode 100644 index 0000000000..e878e80649 --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -0,0 +1,335 @@ +package configmanager + +import ( + "fmt" + "reflect" + "sort" + "strings" + "time" + + "github.com/go-viper/mapstructure/v2" + + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// detachSections makes a copy hold what the original holds without sharing anything a decode can write +// through. +// +// A copy of the struct alone shares every section, every list and every map it points at. A decoder writes +// a list into the array its target already holds, so a shared one means the rehearsal edits the original +// and a refused value leaves exactly the half-written configuration the copy exists to prevent. +// +// Walked over the type rather than field by field, so a section or a list added to the node's configuration +// is detached without this changing. A field it cannot detach is an error rather than a silent share, and +// the test beside this holds every reference in the type against that promise. +func detachSections(out, from *tmcfg.Config) error { + if out == nil || from == nil { + return fmt.Errorf("no configuration to detach") + } + return detachValue(reflect.ValueOf(out).Elem(), "") +} + +// detachValue replaces every reference under v with one nothing else holds. +// +// An unexported field is skipped rather than refused. The copy this walks was made by assigning the struct, +// which copies unexported fields by value, and a decoder cannot write to one either. +func detachValue(v reflect.Value, path string) error { + switch v.Kind() { + case reflect.Pointer: + if v.IsNil() || !v.CanSet() { + return nil + } + fresh := reflect.New(v.Type().Elem()) + fresh.Elem().Set(v.Elem()) + if err := detachValue(fresh.Elem(), path); err != nil { + return err + } + v.Set(fresh) + + case reflect.Struct: + for i := 0; i < v.NumField(); i++ { + f := v.Type().Field(i) + if !v.Field(i).CanSet() { + continue + } + if err := detachValue(v.Field(i), join(path, f.Name)); err != nil { + return err + } + } + + case reflect.Slice: + if v.IsNil() || !v.CanSet() { + return nil + } + fresh := reflect.MakeSlice(v.Type(), v.Len(), v.Len()) + reflect.Copy(fresh, v) + for i := 0; i < fresh.Len(); i++ { + if err := detachValue(fresh.Index(i), path); err != nil { + return err + } + } + v.Set(fresh) + + case reflect.Map: + if v.IsNil() || !v.CanSet() { + return nil + } + fresh := reflect.MakeMapWithSize(v.Type(), v.Len()) + for _, key := range v.MapKeys() { + elem := reflect.New(v.Type().Elem()).Elem() + elem.Set(v.MapIndex(key)) + if err := detachValue(elem, path); err != nil { + return err + } + fresh.SetMapIndex(key, elem) + } + v.Set(fresh) + + case reflect.Interface: + if v.IsNil() || !v.CanSet() { + return nil + } + inner := v.Elem() + fresh := reflect.New(inner.Type()).Elem() + fresh.Set(inner) + if err := detachValue(fresh, path); err != nil { + return err + } + v.Set(fresh) + + case reflect.Chan, reflect.Func, reflect.UnsafePointer: + return fmt.Errorf("%s is a %s, which cannot be copied", path, v.Kind()) + } + return nil +} + +// join builds a field path for a message. +func join(path, field string) string { + if path == "" { + return field + } + return path + "." + field +} + +// describe reads the value the node's configuration currently holds for each key, as text. +// +// Read through the same tags the decode writes through, so a key names the same field in both directions. +// Held as text because what a report needs is whether two values differ and what they are, and comparing +// the shapes a decode produced against the shapes a struct holds would answer a different question. +func describe(cfg *tmcfg.Config, keys []string) (map[string]string, error) { + out := map[string]string{} + if cfg == nil { + return out, fmt.Errorf("no configuration to read") + } + var nested map[string]any + if err := mapstructure.Decode(cfg, &nested); err != nil { + return out, err + } + flat := map[string]any{} + flatten("", nested, flat) + for _, key := range keys { + if v, ok := flat[key]; ok { + out[key] = fmt.Sprint(v) + } + } + return out, nil +} + +// flatten turns a nested map into one keyed by dotted path. +func flatten(prefix string, in map[string]any, out map[string]any) { + for name, value := range in { + path := name + if prefix != "" { + path = prefix + "." + name + } + if inner, nested := value.(map[string]any); nested { + flatten(path, inner, out) + continue + } + out[path] = value + } +} + +// DescribeForTest reads what a node's configuration holds for each key, as text. +// +// Exported for the test that measures the two generators against each other, which lives beside the boot +// because only a boot produces a generated file. +func DescribeForTest(cfg *tmcfg.Config, keys []string) map[string]string { + out, _ := describe(cfg, keys) + return out +} + +// refuseWhatDecodesToSomethingElse reports written values the decoder accepts and turns into something the +// operator did not mean, with what they should have written. +// +// Two shapes, and both decode cleanly, which is why nothing later objects. +// +// A length of time has no form of its own in the file, so it is written as text with a unit. A plain number +// is read as nanoseconds, the shortest unit there is, so sixty means sixty billionths of a second. Zero is +// the exception and is allowed: nanoseconds and seconds are the same at zero, and zero is the documented way +// to turn several of these settings off. +// +// A negative number written where the field cannot hold one wraps to the largest value that field has. So +// minus one, which is how an operator says "no limit" in most software they have used, becomes a limit of +// eighteen million million million: the ceiling on connected peers stops bounding anything, and a window +// measured in seconds becomes six centuries. +// +// This is the one place either can be caught. The resolution sees a number and a key; only the struct says +// what the key is. +func refuseWhatDecodesToSomethingElse(cfg *tmcfg.Config, values map[string]any) []string { + t := reflect.TypeOf(*cfg) + durations := durationKeys(t, "") + unsigned := unsignedKeys(t, "") + + var bad []string + for key, value := range values { + n, numeric := asNumber(value) + if !numeric { + continue + } + switch { + case durations[key] && n != 0: + bad = append(bad, fmt.Sprintf("%s = %v is a length of time, so write a unit, as %q", + key, value, fmt.Sprintf("%vs", value))) + case unsigned[key] && n < 0: + bad = append(bad, fmt.Sprintf("%s = %v cannot be negative, and decodes to the largest value "+ + "this setting can hold rather than to no limit", key, value)) + } + } + sort.Strings(bad) + return bad +} + +// asNumber reports whether a written value arrived as a number, and what it was. +// +// Held as a float because what the checks above ask is whether it is zero and whether it is negative, and +// every numeric shape a file, a variable or a flag can carry answers both. +func asNumber(value any) (float64, bool) { + switch v := value.(type) { + case int: + return float64(v), true + case int8: + return float64(v), true + case int16: + return float64(v), true + case int32: + return float64(v), true + case int64: + return float64(v), true + case uint: + return float64(v), true + case uint8: + return float64(v), true + case uint16: + return float64(v), true + case uint32: + return float64(v), true + case uint64: + return float64(v), true + case float32: + return float64(v), true + case float64: + return v, true + } + return 0, false +} + +// unsignedKeys returns the dotted keys whose field cannot hold a negative number. +func unsignedKeys(t reflect.Type, prefix string) map[string]bool { + return keysWhoseFieldIs(t, prefix, func(ft reflect.Type) bool { + switch ft.Kind() { + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + return true + } + return false + }) +} + +// durationKeys returns the dotted keys whose field is a length of time. +// +// Matched by conversion rather than by identity, so a named type over the same underlying number is a length +// of time too. +func durationKeys(t reflect.Type, prefix string) map[string]bool { + durationType := reflect.TypeOf(time.Duration(0)) + return keysWhoseFieldIs(t, prefix, func(ft reflect.Type) bool { + return ft.Kind() == reflect.Int64 && ft.ConvertibleTo(durationType) && ft != reflect.TypeOf(int64(0)) + }) +} + +// keysWhoseFieldIs returns the dotted keys whose field answers a question about its type. +// +// One walk for every such question, over the same tag rules the declaration derives keys by, so a key found +// here is a key that can be written. +func keysWhoseFieldIs(t reflect.Type, prefix string, is func(reflect.Type) bool) map[string]bool { + out := map[string]bool{} + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + if f.PkgPath != "" { + continue + } + tag, ok := f.Tag.Lookup("mapstructure") + if !ok { + continue + } + name := strings.Split(tag, ",")[0] + squash := strings.Contains(tag, ",squash") + ft := f.Type + for ft.Kind() == reflect.Pointer { + ft = ft.Elem() + } + path := name + if prefix != "" && name != "" { + path = prefix + "." + name + } + if squash { + for key := range keysWhoseFieldIs(ft, prefix, is) { + out[key] = true + } + continue + } + if is(ft) { + out[path] = true + continue + } + if ft.Kind() == reflect.Struct { + for key := range keysWhoseFieldIs(ft, path, is) { + out[key] = true + } + } + } + return out +} + +// referencePathsIn returns every path in a type that a copy has to detach, for the test that holds +// detachSections to the type it copies. +func referencePathsIn(t reflect.Type, path string, seen map[reflect.Type]bool) []string { + if seen[t] { + return nil + } + seen[t] = true + defer delete(seen, t) + + var out []string + switch t.Kind() { + case reflect.Pointer, reflect.Slice, reflect.Map, reflect.Interface: + if path != "" { + out = append(out, path) + } + if t.Kind() != reflect.Interface { + out = append(out, referencePathsIn(t.Elem(), path, seen)...) + } + case reflect.Struct: + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + if f.PkgPath != "" { + continue + } + out = append(out, referencePathsIn(f.Type, join(path, f.Name), seen)...) + } + } + sort.Strings(out) + return out +} + +// samePath reports whether two paths name the same field, ignoring repeats a pointer produces. +func samePath(a, b string) bool { return strings.TrimSuffix(a, ".") == strings.TrimSuffix(b, ".") } diff --git a/cmd/seid/cmd/configmanager/tendermint_copy_test.go b/cmd/seid/cmd/configmanager/tendermint_copy_test.go new file mode 100644 index 0000000000..65c8220a1f --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint_copy_test.go @@ -0,0 +1,115 @@ +package configmanager + +import ( + "reflect" + "testing" + + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// TestTheCopyShareNothingWithWhatItCopied is the property the rollback rests on. +// +// The delivery decodes into a copy and publishes by replacing, so a refused value leaves the node's +// configuration untouched. That holds only if the copy shares nothing the decode can write through, and a +// decoder writes a list into the array its target already holds. One shared section, list or map and the +// rehearsal edits the original. +// +// Walked over the whole type rather than the fields anyone thought of, so a reference added to the node's +// configuration fails here rather than quietly sharing. +func TestTheCopyShareNothingWithWhatItCopied(t *testing.T) { + from := tmcfg.DefaultConfig() + // Give every list something in it, so a shared backing array is observable. + from.RPC.CORSAllowedOrigins = []string{"a", "b", "c"} + from.StateSync.RPCServers = []string{"one:1", "two:2"} + from.TxIndex.Indexer = []string{"kv"} + from.Other = map[string]any{"left": "over"} + + out, err := copyNodeConfig(from) + if err != nil { + t.Fatalf("copyNodeConfig: %v", err) + } + + for _, path := range referencePathsIn(reflect.TypeOf(tmcfg.Config{}), "", map[reflect.Type]bool{}) { + a, okA := fieldByPath(reflect.ValueOf(from).Elem(), path) + b, okB := fieldByPath(reflect.ValueOf(out).Elem(), path) + if !okA || !okB { + continue + } + if shares(a, b) { + t.Errorf("%s is shared between the node's configuration and the copy, so a decode into the "+ + "copy writes through to the node and a refused value cannot be rolled back", path) + } + } +} + +// TestTheCopyHoldsWhatItCopied is the other half: detaching must not lose a value. +// +// A copy that shares nothing and holds nothing would pass the test above and deliver a configuration of +// zeroes over a running node. +func TestTheCopyHoldsWhatItCopied(t *testing.T) { + from := tmcfg.DefaultConfig() + from.RPC.CORSAllowedOrigins = []string{"a", "b", "c"} + from.Mempool.Size = 4321 + from.Instrumentation.Prometheus = true + from.Other = map[string]any{"left": "over"} + + out, err := copyNodeConfig(from) + if err != nil { + t.Fatalf("copyNodeConfig: %v", err) + } + if !reflect.DeepEqual(from, out) { + t.Error("the copy does not hold what it copied; a delivery would publish a configuration that " + + "differs from the node's in ways nobody wrote") + } +} + +// fieldByPath walks a dotted field path, following pointers. +func fieldByPath(v reflect.Value, path string) (reflect.Value, bool) { + for _, name := range splitPath(path) { + for v.Kind() == reflect.Pointer { + if v.IsNil() { + return reflect.Value{}, false + } + v = v.Elem() + } + if v.Kind() != reflect.Struct { + return reflect.Value{}, false + } + f := v.FieldByName(name) + if !f.IsValid() { + return reflect.Value{}, false + } + v = f + } + return v, true +} + +// splitPath breaks a dotted field path into its names. +func splitPath(path string) []string { + if path == "" { + return nil + } + var out []string + start := 0 + for i := 0; i < len(path); i++ { + if path[i] == '.' { + out = append(out, path[start:i]) + start = i + 1 + } + } + return append(out, path[start:]) +} + +// shares reports whether two values point at the same memory. +func shares(a, b reflect.Value) bool { + if a.Kind() != b.Kind() { + return false + } + switch a.Kind() { + case reflect.Pointer, reflect.Map: + return !a.IsNil() && !b.IsNil() && a.Pointer() == b.Pointer() + case reflect.Slice: + return a.Len() > 0 && b.Len() > 0 && a.Pointer() == b.Pointer() + } + return false +} diff --git a/cmd/seid/cmd/node_agreement_test.go b/cmd/seid/cmd/node_agreement_test.go new file mode 100644 index 0000000000..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..abae1b238c --- /dev/null +++ b/cmd/seid/cmd/node_delivery_test.go @@ -0,0 +1,380 @@ +package cmd + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "testing" + "time" + + "go.opentelemetry.io/otel/sdk/trace" + + "github.com/sei-protocol/sei-chain/cmd/seid/cmd/configmanager" + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// The second delivery, driven the way an operator reaches it. +// +// A section whose reader looks its keys up one at a time is delivered by putting the value into the source. +// The node's own configuration file is read once into a struct before any of that, so a value put into the +// source reaches nothing and has to be decoded into the struct instead. These read the setting the node +// runs rather than the source it was resolved into, because a key can be correct in the source and absent +// from the struct. + +// bootWithNodeFile runs a real boot against a sei.toml and a generated node configuration file. +func bootWithNodeFile(t *testing.T, seiToml string, edit func(*tmcfg.Config)) *server.Context { + t.Helper() + home := configtest.NewHome(t) + dir := filepath.Join(home.Root, "config") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + // The node's own file, generated the way the node generates it, so what the delivery writes over is + // what an operator would actually have. + live := tmcfg.DefaultConfig() + if edit != nil { + edit(live) + } + if err := tmcfg.WriteConfigFile(home.Root, live); err != nil { + t.Fatalf("render the node's configuration file: %v", err) + } + if seiToml != "" { + if err := os.WriteFile(filepath.Join(dir, "sei.toml"), []byte(seiToml), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + } + + cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home.Root); err != nil { + t.Fatalf("set --home: %v", err) + } + ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("the boot was refused: %v", err) + } + if ctx.Config == nil { + t.Fatal("the boot produced no node configuration") + } + return ctx +} + +const nodeFileHeader = "schema_version = 1\nnode_mode = \"validator\"\n" + +// TestAWrittenValueReachesTheNodesOwnConfiguration is the property the whole thing rests on. +func TestAWrittenValueReachesTheNodesOwnConfiguration(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[instrumentation]\nprometheus = true\nmax-open-connections = 41\n", nil) + + if !ctx.Config.Instrumentation.Prometheus { + t.Error("sei.toml turned the metrics listener on and the node runs with it off. The value was " + + "resolved and put into a source that nothing reading this file ever consults") + } + if got := ctx.Config.Instrumentation.MaxOpenConnections; got != 41 { + t.Errorf("sei.toml set max-open-connections to 41 and the node runs %d", got) + } +} + +// TestAnUnwrittenKeyKeepsWhatTheNodesOwnFileSaid separates delivering a value from overwriting one. +// +// A section read by a lookup can be delivered whole, because its reader has nowhere else to get a value +// from. A section read by a decode already holds what its own file said, put there before this ran. So a +// key the operator's sei.toml does not mention has to arrive at whatever that file gave it, and delivering +// a default instead replaces their file with one nobody chose, on every boot. +// +// The fixture turns the key on in the node's own file, where the default is off, so the two disagree. +// Without that they agree and the overwrite is invisible. +func TestAnUnwrittenKeyKeepsWhatTheNodesOwnFileSaid(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nsize = 4321\n", func(live *tmcfg.Config) { + live.Instrumentation.Prometheus = true + }) + + if got := ctx.Config.Mempool.Size; got != 4321 { + t.Fatalf("the written key arrived as %d, so this test cannot tell the two cases apart", got) + } + if !ctx.Config.Instrumentation.Prometheus { + t.Error("the node's own file turned the metrics listener on, sei.toml said nothing about it, and " + + "the node runs with it off. A default was delivered over the operator's own file, which " + + "happens on every boot for every key their sei.toml does not mention") + } +} + +// TestARefusedValueLeavesItsSectionAlone is the promise that makes this safe to enable. +// +// A decoder gathers errors and keeps going, so a value it refuses partway leaves its target holding some +// new values and some old, with nothing to compare against. The delivery decodes into a copy and publishes +// by replacing, so a refused value leaves the section exactly as the node had it. +func TestARefusedValueLeavesItsSectionAlone(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[instrumentation]\nprometheus = true\nmax-open-connections = \"not a number\"\n", nil) + + if got := ctx.Config.Instrumentation.MaxOpenConnections; got != 3 { + t.Errorf("max-open-connections is %d after a refused value, want the 3 the node had. A "+ + "partly applied decode leaves settings nobody chose", got) + } + if ctx.Config.Instrumentation.Prometheus { + t.Error("the value beside the refused one was applied, so a partial decode was published. " + + "Either all of a section's values arrive or none do") + } +} + +// TestARefusedValueCostsOnlyItsOwnSection is why the delivery is per section. +// +// One decode for the whole file would mean an operator who fixed one setting and mistyped another boots +// with neither applied. The mistyped section is lost; the one beside it is not. +func TestARefusedValueCostsOnlyItsOwnSection(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[instrumentation]\nmax-open-connections = \"not a number\"\n"+ + "\n[mempool]\nsize = 4321\n", nil) + + if got := ctx.Config.Instrumentation.MaxOpenConnections; got != 3 { + t.Errorf("the refused section was applied anyway, reading %d", got) + } + if got := ctx.Config.Mempool.Size; got != 4321 { + t.Errorf("mempool.size is %d and sei.toml set it to 4321. A value refused in one section took "+ + "another section's settings down with it", got) + } +} + +// TestEachChannelWinsForADecodedKeyToo is precedence, asserted where a decoded value lands. +func TestEachChannelWinsForADecodedKeyToo(t *testing.T) { + const key = "rpc.max-open-connections" + body := nodeFileHeader + "\n[rpc]\nmax-open-connections = 111\n" + + t.Run("the file beats what the node's own file said", func(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, body, nil) + if got := ctx.Config.RPC.MaxOpenConnections; got != 111 { + t.Errorf("the node runs %d with 111 in sei.toml; the value resolved and never reached the "+ + "struct the node reads", got) + } + }) + + t.Run("the environment beats the file", func(t *testing.T) { + configtest.Isolate(t) + t.Setenv(registry.EnvName(key), "222") + ctx := bootWithNodeFile(t, body, nil) + if got := ctx.Config.RPC.MaxOpenConnections; got != 222 { + t.Errorf("the node runs %d with 222 in the environment and 111 in the file", got) + } + }) +} + +// TestTheDeliveryLeavesTheRootDirectoryAlone is what the root-directory exclusions buy. +func TestTheDeliveryLeavesTheRootDirectoryAlone(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[instrumentation]\nprometheus = true\n", nil) + + if !ctx.Config.Instrumentation.Prometheus { + t.Fatal("the delivery did not run, so this test would pass with the root directory declared") + } + if ctx.Config.RootDir == "" { + t.Error("the node's root directory is empty after the delivery") + } + if ctx.Config.PrivValidator.RootDir == "" { + t.Error("the signing key's root directory is empty after the delivery. A node that cannot find " + + "its key does not sign") + } +} + +// TestATypedFlagReachesTheKeyItCarries covers the one channel an operator reaches for under pressure. +// +// A flag's name and the key it carries are not always spelled the same: the node's own flags separate words +// with an underscore where the tag they decode through uses a hyphen. Compared as strings such a flag looks +// like a name nothing declares, so it is dropped, and the file wins over the command line. +// +// Driven with the file and the flag disagreeing, and read off the struct the node runs from, because this +// key belongs to a section delivered by a decode. +func TestATypedFlagReachesTheKeyItCarries(t *testing.T) { + const key = "p2p.unconditional-peer-ids" + const flag = "p2p.unconditional_peer_ids" + configtest.Isolate(t) + + home := configtest.NewHome(t) + dir := filepath.Join(home.Root, "config") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatal(err) + } + if err := tmcfg.WriteConfigFile(home.Root, tmcfg.DefaultConfig()); err != nil { + t.Fatalf("render the node's configuration file: %v", err) + } + body := nodeFileHeader + "\n[p2p]\nunconditional-peer-ids = \"from-the-file\"\n" + if err := os.WriteFile(filepath.Join(dir, "sei.toml"), []byte(body), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + + cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home.Root); err != nil { + t.Fatal(err) + } + if err := cmd.Flags().Set(flag, "from-the-command-line"); err != nil { + t.Skipf("--%s is not on this command, so nothing here can carry the key: %v", flag, err) + } + ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("the boot was refused: %v", err) + } + + if got := ctx.Config.P2P.UnconditionalPeerIDs; got != "from-the-command-line" { + t.Errorf("the node runs %q with --%s typed and a different value in the file, want the typed "+ + "one. The flag's name and the key it carries are spelled differently, so comparing them as "+ + "strings drops the flag and the file wins over the command line", got, flag) + } +} + +// TestALengthOfTimeWrittenAsAPlainNumberIsRefused covers a value that decodes cleanly and is wrong by a +// factor of a billion. +// +// The file format has no way to say how long something is, so a length of time is written as text with a +// unit. A plain number decodes as nanoseconds, the shortest unit there is, so sixty means sixty billionths +// of a second and the node starts. Nothing later objects, because nothing later can tell. +func TestALengthOfTimeWrittenAsAPlainNumberIsRefused(t *testing.T) { + configtest.Isolate(t) + was := tmcfg.DefaultConfig().Mempool.TTLDuration + + t.Run("a plain number is refused and the section is left alone", func(t *testing.T) { + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nttl-duration = 60\nsize = 4321\n", nil) + if got := ctx.Config.Mempool.TTLDuration; got != was { + t.Errorf("the node runs a time-to-live of %v after a plain 60 was written, want the %v it "+ + "had. Sixty read as nanoseconds is sixty billionths of a second", got, was) + } + if got := ctx.Config.Mempool.Size; got == 4321 { + t.Error("the value beside the refused one was applied, so the section was published in part") + } + }) + + t.Run("zero is applied, because zero is the same in every unit", func(t *testing.T) { + // Several of these settings document zero as the way to turn them off, and three declare it as + // their value, so an operator writing it is doing the ordinary thing. Refusing it would cost them + // every other key in the section. + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[rpc]\ntimeout-read-header = 0\nmax-open-connections = 41\n", nil) + if got := ctx.Config.RPC.TimeoutReadHeader; got != 0 { + t.Errorf("the node runs a read-header timeout of %v with 0 written, want 0", got) + } + if got := ctx.Config.RPC.MaxOpenConnections; got != 41 { + t.Errorf("max-open-connections is %d, so writing a zero length of time cost the section. "+ + "Zero nanoseconds and zero seconds are the same value, so there is nothing to refuse", got) + } + }) + + t.Run("the same number with a unit is applied", func(t *testing.T) { + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nttl-duration = \"60s\"\n", nil) + if got := ctx.Config.Mempool.TTLDuration; got != 60*time.Second { + t.Errorf("the node runs %v with \"60s\" written, want 60s. Refusing a plain number must not "+ + "refuse the written form an operator is being asked for", got) + } + }) +} + +// TestTheReportSurvivesAQuietNode is what a fleet running its nodes quiet needs. +// +// One log level covers every logger in the process and an operator writes it. A fleet that sets it above the +// level these reports use turns this manager into a component that changes what a node runs and says nothing +// about it, and the report is the only place the node's own file and the running settings can be told apart. +// +// The level is what is asserted rather than a message, because a message can be absent for reasons that have +// nothing to do with whether it would have been printed. +func TestTheReportSurvivesAQuietNode(t *testing.T) { + configtest.Isolate(t) + + ctx := bootWithNodeFile(t, nodeFileHeader+"log-level = \"error\"\n\n[mempool]\nsize = 4321\n", nil) + if got := ctx.Config.Mempool.Size; got != 4321 { + t.Fatalf("the value was not delivered (%d), so this test cannot show a report being kept", got) + } + if !configmanager.OwnReportingEnabledForTest() { + t.Error("a node whose file sets the level to error delivered a value and this manager's own " + + "reporting is switched off. The report is the only signal it has, and the node's own file " + + "and its running settings can be told apart nowhere else") + } +} + +// TestANegativeNumberWhereTheSettingCannotHoldOneIsRefused covers the habit of writing minus one for +// "no limit". +// +// Most software an operator has used takes minus one that way. Here the field cannot hold a negative number, +// so the decoder wraps it to the largest value the field has: the ceiling on connected peers stops bounding +// anything, and a window measured in seconds becomes centuries. The value decodes cleanly, so nothing later +// objects. +func TestANegativeNumberWhereTheSettingCannotHoldOneIsRefused(t *testing.T) { + configtest.Isolate(t) + was := tmcfg.DefaultConfig().P2P.MaxConnections + + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[p2p]\nmax-connections = -1\nsend-rate = 1234567\n", nil) + + if got := ctx.Config.P2P.MaxConnections; got != was { + t.Errorf("the node allows %d connected peers after minus one was written, want the %d it had. "+ + "Minus one wraps to the largest value this setting can hold, which is no bound at all", got, was) + } + if got := ctx.Config.P2P.SendRate; got == 1234567 { + t.Error("the value beside the refused one was applied, so the section was published in part") + } +} + +// TestNoDeliveryCarriesADeclaredDefault is the one rule both deliveries depend on, named. +// +// A resolution answers for every declared key, and a declared value is what a provisioning command writes +// for a kind of node rather than what any particular node runs. Delivering one would replace a setting an +// operator never mentioned, on every boot, for every key their file omits. Both deliveries avoid that by +// narrowing to the keys a source supplied, and each does it in its own function. +// +// That makes it a rule three call sites remember rather than one a single function enforces, which is the +// shape this repository's own guidance says to guard. Until the narrowing has one home, this is the guard: +// it boots with a file that supplies one key and asserts that nothing else moved anywhere, across both +// deliveries and every mode. +func TestNoDeliveryCarriesADeclaredDefault(t *testing.T) { + for _, mode := range registry.Modes() { + t.Run(string(mode), func(t *testing.T) { + configtest.Isolate(t) + + // What the node holds before any file supplies anything. + bare := bootWithNodeFile(t, "schema_version = 1\nnode_mode = \""+string(mode)+"\"\n", nil) + keys := everyDeclaredKey() + before := configmanager.DescribeForTest(bare.Config, keys) + beforeSource := map[string]string{} + for _, key := range keys { + beforeSource[key] = fmt.Sprint(bare.Viper.Get(key)) + } + + // The same node, with a file supplying exactly one key. + after := bootWithNodeFile(t, "schema_version = 1\nnode_mode = \""+string(mode)+"\"\n"+ + "\n[mempool]\nsize = 4321\n", nil) + if got := after.Config.Mempool.Size; got != 4321 { + t.Fatalf("the one supplied key arrived as %d, so nothing was delivered and this test "+ + "would pass for a delivery that does nothing", got) + } + + afterDescribed := configmanager.DescribeForTest(after.Config, keys) + for _, key := range keys { + if key == "mempool.size" { + continue + } + if afterDescribed[key] != before[key] { + t.Errorf("%s reads %q after a file that supplies only mempool.size, and %q before. A "+ + "declared default was delivered over a setting nobody wrote", + key, afterDescribed[key], before[key]) + } + if got := fmt.Sprint(after.Viper.Get(key)); got != beforeSource[key] { + t.Errorf("%s reads %q in the source and %q before it. A declared default was "+ + "installed for a key nobody wrote", key, got, beforeSource[key]) + } + } + }) + } +} + +// everyDeclaredKey returns every key any registered section declares, sorted. +func everyDeclaredKey() []string { + keys := registry.Keys() + sort.Strings(keys) + return keys +} From ae60b140e07d178e82286027739c819ec3584a73 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 27 Aug 2026 14:24:31 -0700 Subject: [PATCH 2/2] 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 | 54 ++++ cmd/seid/cmd/configmanager/doc.go | 45 ++- cmd/seid/cmd/configmanager/install.go | 10 +- cmd/seid/cmd/configmanager/tendermint.go | 103 +++++-- 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 | 62 +++- config/registry/delivery.go | 19 ++ 10 files changed, 500 insertions(+), 154 deletions(-) diff --git a/cmd/seid/cmd/boot_install_test.go b/cmd/seid/cmd/boot_install_test.go index 8e12d33f73..5f0dd23971 100644 --- a/cmd/seid/cmd/boot_install_test.go +++ b/cmd/seid/cmd/boot_install_test.go @@ -182,7 +182,8 @@ func TestOnlyWhatASourceSuppliedIsInstalled(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) @@ -224,15 +225,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 1c5f56c01e..0876a842f5 100644 --- a/cmd/seid/cmd/configmanager/decode_report_test.go +++ b/cmd/seid/cmd/configmanager/decode_report_test.go @@ -48,3 +48,57 @@ func TestTheNothingSuppliedLineIsNotSaidWhenADecodeDelivered(t *testing.T) { out.String()) } } + +// 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 00063e50bd..21d9e19b10 100644 --- a/cmd/seid/cmd/configmanager/doc.go +++ b/cmd/seid/cmd/configmanager/doc.go @@ -8,31 +8,44 @@ // always been answered, and then delivers whatever sei.toml supplies on top of that. It also runs an // advisory validation pass, which never rewrites a file and never refuses a boot. // -// # 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. +// afterwards reaches nothing at all. Those are decoded into a copy of the struct and published into it. // -// Only what a source supplied is installed. A resolution answers for every declared key, so installing all +// 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. +// +// Only what a source supplied is delivered. A resolution answers for every declared key, so delivering all // of it would write a default over whatever an operator's own file holds for every key their sei.toml does // not mention. // +// # 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. Selecting this manager is a switch rather than a configuration change, -// and a mistyped line in a hand-edited file must not become an outage on the next restart. -// -// 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 level that survives a fleet running its -// nodes quiet, they name the source they are about, and they are bounded, because a report that fires on -// every boot is one nobody reads. +// 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. Selecting this manager is a switch rather +// than a configuration change, and 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 all 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 level that survives a fleet running +// its nodes quiet, 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 it rather than only // read into it. diff --git a/cmd/seid/cmd/configmanager/install.go b/cmd/seid/cmd/configmanager/install.go index 556cfa20fd..bab0cc4054 100644 --- a/cmd/seid/cmd/configmanager/install.go +++ b/cmd/seid/cmd/configmanager/install.go @@ -149,15 +149,9 @@ func everyChannelAnOperatorCanUse(written map[string]any, typed map[string]strin // It also means a declared default never reaches a running node, which is what lets a default state what // the provisioning command writes rather than having to state what each node already runs. func onlyWhatALookupSourceSupplied(resolved registry.Resolved) registry.Resolved { - decoded := registry.DecodedSections() owning := map[string]bool{} - for _, section := range registry.Sections() { - if _, ok := decoded[section.Name]; !ok { - continue - } - for _, key := range section.Keys { - owning[key] = true - } + for _, key := range registry.KeysADecodeDelivers() { + owning[key] = true } out := registry.Resolved{Values: make(map[string]any, len(resolved.Overrides))} diff --git a/cmd/seid/cmd/configmanager/tendermint.go b/cmd/seid/cmd/configmanager/tendermint.go index 54998c04bb..ee5e07a650 100644 --- a/cmd/seid/cmd/configmanager/tendermint.go +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -4,8 +4,8 @@ import ( "cmp" "fmt" "log/slog" + "net/url" "os" - "sort" "strings" "github.com/spf13/viper" @@ -37,11 +37,19 @@ func deliverDecodedSections(ctx *server.Context, resolved registry.Resolved, log return false } + // 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 sortedSectionNames(bySection) { + 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) } return true @@ -64,9 +72,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 } @@ -78,7 +88,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 "+ @@ -87,8 +97,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. @@ -97,6 +111,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) } @@ -107,13 +129,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 @@ -128,11 +152,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 { @@ -140,28 +172,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, "; ")) -} - -// sortedKeys returns a map's keys in a fixed order, so a log line does not vary between runs. -func sortedKeys(values map[string]any) []string { - out := make([]string, 0, len(values)) - for key := range values { - out = append(out, key) - } - sort.Strings(out) - return out -} - -// sortedSectionNames returns the sections to deliver in a fixed order. -func sortedSectionNames(bySection map[string]map[string]any) []string { - out := make([]string, 0, len(bySection)) - for name := range bySection { - out = append(out, name) - } - sort.Strings(out) - return out + "section", name, "count", len(moved), "changed", strings.Join(shown, "; "), "omitted", omitted) } // logLevelKey is the one delivered setting the struct is not the end of. @@ -173,6 +186,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..a1fdf68b43 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, registry.KeysADecodeDelivers()) } diff --git a/cmd/seid/cmd/node_delivery_test.go b/cmd/seid/cmd/node_delivery_test.go index abae1b238c..b684e54f8b 100644 --- a/cmd/seid/cmd/node_delivery_test.go +++ b/cmd/seid/cmd/node_delivery_test.go @@ -216,7 +216,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 { @@ -339,7 +341,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 := registry.KeysADecodeDelivers() + before := configmanager.DescribeForTest(t, bare.Config, decodedKeys) beforeSource := map[string]string{} for _, key := range keys { beforeSource[key] = fmt.Sprint(bare.Viper.Get(key)) @@ -353,15 +359,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 "+ @@ -378,3 +389,40 @@ 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) + } +} diff --git a/config/registry/delivery.go b/config/registry/delivery.go index 4049fccac5..0e7fa84b09 100644 --- a/config/registry/delivery.go +++ b/config/registry/delivery.go @@ -2,6 +2,7 @@ package registry import ( "fmt" + "sort" ) // decodedNotLookedUp holds the sections whose values reach their reader by a decode, and why. @@ -94,3 +95,21 @@ func SuppliedByDecodedSection(resolved Resolved) map[string]map[string]any { } return out } + +// KeysADecodeDelivers returns the keys of every section whose values reach their reader by a decode, sorted. +// +// One read for the sections and the declarations together, for the reason SuppliedByDecodedSection takes +// one: asked separately, a section arriving between the two reads is named by one answer and absent from +// the other, so its keys are attributed to the wrong delivery. +func KeysADecodeDelivers() []string { + registered, _, decoded := snapshot() + var out []string + for _, section := range registered { + if _, owned := decoded[section.Name]; !owned { + continue + } + out = append(out, section.Keys...) + } + sort.Strings(out) + return out +}