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 new file mode 100644 index 0000000000..0876a842f5 --- /dev/null +++ b/cmd/seid/cmd/configmanager/decode_report_test.go @@ -0,0 +1,104 @@ +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()) + } +} + +// 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 d411e4a91f..bab0cc4054 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) @@ -136,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 new file mode 100644 index 0000000000..ee5e07a650 --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -0,0 +1,268 @@ +package configmanager + +import ( + "cmp" + "fmt" + "log/slog" + "net/url" + "os" + "strings" + + "github.com/spf13/viper" + + "github.com/sei-protocol/seilog" + + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// deliverDecodedSections puts the resolved values of the decoded sections into the node's own +// configuration. +// +// Putting a value into the source a node reads is the whole delivery for a section whose reader looks its +// keys up one at a time. It is no delivery at all for the sections this covers, which the boot's handler +// reads once into a struct before this runs. Those values are decoded into that struct instead, which is +// the same mechanism the handler used and therefore the same casts, the same tags and the same hooks. +// +// Nothing here can stop a node starting, which is the one promise this manager makes. +func deliverDecodedSections(ctx *server.Context, 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 + } + + // Each section names why its values need decoding rather than installing, and the reason names the + // struct they are decoded into. Reported here because this is the only place that claim is acted on: + // a section whose reason no longer describes what reads it is delivered the wrong way, and there is + // nothing else that would show it. + reasons := registry.DecodedSections() + + // One section at a time. A decode is all or nothing for whatever it is handed, so a single value a + // decoder refuses would otherwise cost every key in the file rather than the keys of the section it + // appeared in. An operator who fixes one setting and mistypes another has to end up with the first + // one applied. + for _, name := range sortedKeys(bySection) { + log.Debug("delivering a section by decoding it rather than by a lookup", + "section", name, "why", reasons[name], "keys", len(bySection[name])) + deliverOneSection(ctx, name, bySection[name], log) + } + 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 := 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 + } + + 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, 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 "+ + "every one of its keys reads as it always has", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } + + if err := publishNodeConfig(ctx.Config, candidate); err != nil { + log.Error("cannot publish this node's configuration, so these keys read as they always have", + "section", name, "keys", strings.Join(keys, ","), "err", err) + 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. + 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 + } + // 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) +} + +// 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. 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 := detachReferences(&out); 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. +// +// 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, + withoutCredentials(before[key]), withoutCredentials(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 + } + shown, omitted := capLoggedItems(moved) + log.Info("this section's settings now differ from what the node's own configuration file says", + "section", name, "count", len(moved), "changed", strings.Join(shown, "; "), "omitted", omitted) +} + +// 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" + +// 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 +// 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..0d282f6257 --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -0,0 +1,460 @@ +package configmanager + +import ( + "fmt" + "math" + "reflect" + "sort" + "strings" + "testing" + "time" + + "github.com/go-viper/mapstructure/v2" + + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// 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. 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(cfg).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.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()) + } + 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, 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. +// +// 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 values, keys, fmt.Errorf("no configuration to read") + } + var nested map[string]any + if err := mapstructure.Decode(cfg, &nested); err != nil { + return values, keys, err + } + flat := map[string]any{} + flatten("", nested, flat) + for _, key := range keys { + v, ok := flat[key] + if !ok { + unread = append(unread, key) + continue + } + values[key] = fmt.Sprint(v) + } + sort.Strings(unread) + return values, unread, 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 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. +// +// 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 +// 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. +// +// 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 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) + 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 +} + +// keyFieldTypes returns every dotted key this type declares and the type of the field it names. +// +// 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 != "" { + 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 + } + switch { + case squash: + for key, kt := range keyFieldTypes(ft, prefix) { + out[key] = kt + } + case ft.Kind() == reflect.Struct && !isDuration(ft): + for key, kt := range keyFieldTypes(ft, path) { + out[key] = kt + } + default: + out[path] = ft + } + } + return out +} + +// 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 + } + seen[t] = true + defer delete(seen, t) + + var out []string + switch t.Kind() { + case reflect.Pointer, reflect.Slice, reflect.Map, reflect.Interface: + if path != "" { + out = append(out, path) + } + if t.Kind() != reflect.Interface { + out = append(out, referencePathsIn(t.Elem(), path, seen)...) + } + case reflect.Array: + // The array itself is not a reference, so it is not a path of its own. What it holds can be, and + // leaving this case out gives this walk the same blind spot as the copy it holds to account. + out = append(out, referencePathsIn(t.Elem(), path, seen)...) + case reflect.Struct: + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + if f.PkgPath != "" { + continue + } + out = append(out, referencePathsIn(f.Type, join(path, f.Name), seen)...) + } + } + sort.Strings(out) + return out +} + +// publishNodeConfig makes a node's configuration hold what the candidate holds, without replacing it. +// +// Field by field rather than by assigning the whole struct. The configuration is one struct behind one +// 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 new file mode 100644 index 0000000000..c9e2458969 --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint_copy_test.go @@ -0,0 +1,173 @@ +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. +// +// 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 + } + 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() + 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 new file mode 100644 index 0000000000..a1fdf68b43 --- /dev/null +++ b/cmd/seid/cmd/node_agreement_test.go @@ -0,0 +1,127 @@ +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") + } + + 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 new file mode 100644 index 0000000000..b684e54f8b --- /dev/null +++ b/cmd/seid/cmd/node_delivery_test.go @@ -0,0 +1,428 @@ +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.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 { + 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() + // 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)) + } + + // 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(t, after.Config, decodedKeys) + for _, key := range decodedKeys { + if key == "mempool.size" { + continue + } + if 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 "+ + "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 +} + +// 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 +}