diff --git a/cmd/seid/cmd/boot_install_test.go b/cmd/seid/cmd/boot_install_test.go index 0d72101b41..a732ce1e14 100644 --- a/cmd/seid/cmd/boot_install_test.go +++ b/cmd/seid/cmd/boot_install_test.go @@ -188,7 +188,8 @@ func TestEveryDeclaredKeyIsInstalled(t *testing.T) { func TestAppTomlDoesNotReachTheFlagChannel(t *testing.T) { const key = "state-sync.snapshot-keep-recent" if _, declared := declaredKey(key); !declared { - t.Skipf("%s is not declared, so this cannot happen through it", key) + t.Fatalf("%s is not declared, so this test cannot reach the inversion it exists for. Skipping "+ + "instead would leave the only guard on it passing while measuring nothing", key) } configtest.Isolate(t) @@ -230,15 +231,18 @@ func TestAppTomlDoesNotReachTheFlagChannel(t *testing.T) { // accepted, which measures the absence of a value rather than the refusal. func TestAFileThisBinaryCannotUseLeavesTheNodeAsItWas(t *testing.T) { supplies := "\n[evm]\nmax_tx_pool_txs = 111\n" - for name, body := range map[string]string{ - "no file at all": "", - "a mode nothing knows": "schema_version = 1\nnode_mode = \"sentry\"\n" + supplies, - "no mode at all": "schema_version = 1\n" + supplies, - "not parseable": "schema_version = 1\nnode_mode = \"validator\"\n[evm\n" + supplies, + for _, tc := range []struct { + name string + body string + }{ + {"no file at all", ""}, + {"a mode nothing knows", "schema_version = 1\nnode_mode = \"sentry\"\n" + supplies}, + {"no mode at all", "schema_version = 1\n" + supplies}, + {"not parseable", "schema_version = 1\nnode_mode = \"validator\"\n[evm\n" + supplies}, } { - t.Run(name, func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) { configtest.Isolate(t) - ctx := bootWith(t, body, nil) + ctx := bootWith(t, tc.body, nil) if got := ctx.Viper.Get(bootProbeKey); got != nil { t.Errorf("%s reads %#v, so a value was installed from a file this binary cannot use. "+ "A node whose file names a mode this binary does not know would run one mode's "+ diff --git a/cmd/seid/cmd/configmanager/decode_report_test.go b/cmd/seid/cmd/configmanager/decode_report_test.go new file mode 100644 index 0000000000..a42a498982 --- /dev/null +++ b/cmd/seid/cmd/configmanager/decode_report_test.go @@ -0,0 +1,167 @@ +package configmanager + +import ( + "bytes" + "fmt" + "log/slog" + "strconv" + "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" +) + +// TestEveryDeclaredKeyOfADecodedSectionIsDelivered is what makes sei.toml the configuration for the +// sections a reader decodes whole. +// +// The install cannot reach them, so a value only arrives through this decode. Every key the resolution +// answered has to arrive, not only the ones a source wrote, or a key sei.toml leaves out would keep +// whatever config.toml said and the file would be a patch rather than the configuration. +func TestEveryDeclaredKeyOfADecodedSectionIsDelivered(t *testing.T) { + var out bytes.Buffer + log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug})) + + // A node holding a value nobody wrote in sei.toml. + live := tmcfg.DefaultConfig() + live.P2P.MaxConnections = 999 + ctx := &server.Context{Config: live} + + resolved, err := registry.Resolve(registry.ModeFull, registry.Sources{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + forADecode, _ := registry.ResolvedAndOwnedByDecodedSections(resolved) + deliverDecodedSections(ctx, forADecode, log) + + declared := resolved.Values["p2p.max-connections"] + if declared == nil { + t.Fatal("p2p.max-connections declares nothing, so this measures nothing") + } + if got := uint64(ctx.Config.P2P.MaxConnections); got != asUint(t, declared) { + t.Errorf("the node runs a peer ceiling of %d after a resolution that declares %v and a sei.toml "+ + "that mentions nothing, want the declared value. A key the file leaves out has to take the "+ + "declaration, or config.toml is still the configuration", got, declared) + } +} + +// asUint reads a declared numeric value as an unsigned number. +func asUint(t *testing.T, v any) uint64 { + t.Helper() + n, err := strconv.ParseUint(fmt.Sprint(v), 10, 64) + if err != nil { + t.Fatalf("the declared value %v is not a number: %v", v, err) + } + return n +} + +// 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) { + // Each case carries its own secret, and the whole secret has to be gone. A shared one-word secret + // hides the failure that matters here: a run that stops at the first space redacts the first word and + // leaves the rest, and the rest is still the password. + for _, tc := range []struct{ secret, dsn string }{ + {"sup3rs3cret", "postgres://seid:sup3rs3cret@10.0.0.9:5432/idx"}, + {"sup3rs3cret", "postgres://seid@10.0.0.9:5432/idx?password=sup3rs3cret"}, + {"sup3rs3cret", "postgres://seid@10.0.0.9:5432/idx?sslpassword=sup3rs3cret"}, + {"sup3rs3cret", "host=10.0.0.9 port=5432 user=seid password=sup3rs3cret dbname=idx"}, + {"sup3rs3cret", "postgresql://seid@10.0.0.9/idx?password=sup3rs3cret&sslmode=require"}, + // PostgreSQL accepts a quoted keyword value, and a password may hold spaces. + {"alpha bravo charlie", "host=10.0.0.9 user=seid password='alpha bravo charlie' dbname=idx"}, + {"alpha bravo charlie", `host=10.0.0.9 user=seid password="alpha bravo charlie" dbname=idx`}, + // A backslash escape inside a quoted value. Ending the match at the escaped quote leaks the tail. + {`alpha' bravo charlie`, `host=10.0.0.9 user=seid password='alpha\' bravo charlie' dbname=idx`}, + // The same unquoted, where the space itself is escaped. + {`alpha bravo`, `host=10.0.0.9 user=seid password=alpha\ bravo dbname=idx`}, + // A password a URL parser refuses in userinfo, so the parse fails and no named field exists. + {"alpha bravo", "postgres://seid:alpha bravo@10.0.0.9:5432/idx"}, + } { + var out bytes.Buffer + log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug})) + reportWhatMoved("tx-index", + []string{"tx-index.psql-conn"}, + map[string]string{"tx-index.psql-conn": ""}, + map[string]string{"tx-index.psql-conn": tc.dsn}, + log) + + // Every word of the secret, not the whole string. A run that stops at the first space redacts the + // first word and leaves the rest, and the rest is still the password: checking only for the whole + // secret cannot see that, because the first word is genuinely gone. + for _, word := range strings.Fields(tc.secret) { + if strings.Contains(out.String(), word) { + t.Errorf("the report carries %q, part of the password in %q:\n%s", + word, tc.dsn, out.String()) + } + } + if !strings.Contains(out.String(), "10.0.0.9") { + t.Errorf("the report no longer says where the index writes, so an operator cannot tell what "+ + "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", + "host=10.0.0.9 port=5432 user=seid dbname=idx", + "postgres://seid@10.0.0.9/idx?sslmode=require", + } { + if got := withoutCredentials(value); got != value { + t.Errorf("%q is reported as %q, and an operator reading it back has to see what they wrote", + value, got) + } + } +} + +// TestAnUnreadKeyIsNotComparedAsUnchanged covers the statement the report must withhold. +// +// A key neither side could be read for is absent from both answers, so a comparison finds it equal and says +// it did not move. That is what a key an operator wrote and got looks like, produced by having read nothing, +// and the line naming the unread keys exists precisely so the report does not claim it. +func TestAnUnreadKeyIsNotComparedAsUnchanged(t *testing.T) { + keys := []string{"mempool.size", "mempool.ttl-duration", "mempool.max-tx-bytes"} + unread := asSet([]string{"mempool.ttl-duration"}) + + got := whatBothSidesCouldBeReadFor(keys, unread) + want := []string{"mempool.size", "mempool.max-tx-bytes"} + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Fatalf("the keys compared are %v, want %v", got, want) + } + + // The report over the surviving keys must still say what moved, so the filter cannot be a way of + // reporting nothing. + var out bytes.Buffer + log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug})) + reportWhatMoved("mempool", got, + map[string]string{"mempool.size": "5000", "mempool.max-tx-bytes": "1048576"}, + map[string]string{"mempool.size": "4321", "mempool.max-tx-bytes": "1048576"}, + log) + if !strings.Contains(out.String(), "mempool.size") { + t.Errorf("the report does not name the key that moved:\n%s", out.String()) + } + if strings.Contains(out.String(), "ttl-duration") { + t.Errorf("the report names a key neither side could be read for:\n%s", out.String()) + } +} diff --git a/cmd/seid/cmd/configmanager/doc.go b/cmd/seid/cmd/configmanager/doc.go index 39af536a17..20d7639a7f 100644 --- a/cmd/seid/cmd/configmanager/doc.go +++ b/cmd/seid/cmd/configmanager/doc.go @@ -28,28 +28,39 @@ // anywhere, and seid init has to write one for a new node. Neither exists yet, which is why the gate // defaults to the legacy manager. // -// # Delivering a value +// # Two deliveries, because a node reads a setting two ways // -// A node reads a setting one of two ways, and only one of them can be delivered from here today. Most -// settings are looked up by name from a source the boot builds, so a resolved value reaches them by being -// installed into that source. The settings the node's own configuration file carries are read once, by +// Most settings are looked up by name from a source the boot builds, so a resolved value reaches them by +// being installed into that source. The settings the node's own configuration file carries are read once, by // decoding that file into a struct before any lookup happens, so a value installed into the source -// afterwards reaches nothing at all. Those sections are identified and deliberately left out of the -// install, because installing a value that changes nothing is worse than not installing it: it reads as -// applied everywhere except in the node. They are reported instead, and delivered by the change after this -// one. +// afterwards reaches nothing at all. Those are decoded into a copy of the struct and published into it. +// +// A section names which of the two it needs, and the registry answers for the name. Nothing can tell from +// the outside: both look like a key with a value. +// +// # Precedence +// +// A value in sei.toml wins over the same key in app.toml or config.toml. The node's own files are read +// first and sei.toml is delivered on top, so for a key both state, the running node uses sei.toml's and the +// other file still says what it said. +// +// That is why the reports name every key that moved. After the delivery, neither of the node's own files +// describes what it is running, and nothing else does either. // // # Refusing nothing // // Nothing here can stop a node starting. A missing sei.toml, an unreadable one, a mode this binary does not -// know, a value the install refuses, or a panic in the delivery itself all leave every key reading as it -// always has, and the node starts. A mistyped line in a hand-edited file must not become an outage on the -// next restart. +// know, a value that decodes to something other than what it says, or a panic in the delivery itself all +// leave every key reading as it always has, and the node starts. A mistyped line in a hand-edited file must +// not become an outage on the next restart. +// +// A refusal is per section, not per file, because a decode is all or nothing for whatever it is handed. An +// operator who fixes one setting and mistypes another gets the first one. // // What that costs is that a value which does not arrive is reported rather than refused, which makes these // reports the only signal an operator has. So they are held at a floor that survives a fleet running its -// nodes quiet, without lowering a level an operator raised; they name the source they are about; and they -// are bounded, because a report that fires on every boot is one nobody reads. +// nodes quiet, without lowering a level an operator raised; they name the source they are about; they carry +// no password; and they are bounded, because a report that fires on every boot is one nobody reads. // // Deferred: a path that writes sei.toml, so a node's configuration can be rendered from its existing files // rather than only read out of a file somebody has to author by hand. diff --git a/cmd/seid/cmd/configmanager/install.go b/cmd/seid/cmd/configmanager/install.go index 0bc2e71b1a..5d9270adf7 100644 --- a/cmd/seid/cmd/configmanager/install.go +++ b/cmd/seid/cmd/configmanager/install.go @@ -82,12 +82,17 @@ func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logg "mode", mode, "err", err) return } - // Before the report below, because a refused registration is what makes that one point at the wrong - // file, and an operator reading in order should meet the cause first. + // First, because every report below is a log line and a refusal is reported at a level an operator + // may have raised the threshold above. Doing this after would mean the one setting somebody changes + // in order to see a refusal is the setting a refusal suppresses. + applyResolvedLogLevel(resolved, typed, log) + + // Before the reports it explains, because a refused registration is what makes the next one point at + // the wrong file, and an operator reading in order should meet the cause first. reportWhatThisBinaryCouldNotUse(resolved, log) - // A key nothing declares is the most common thing an operator gets wrong and the only signal they - // have for it. + // After the level, so a file that raises it can report its own mistakes. A key nothing declares is + // the most common thing an operator gets wrong and the only signal they have for it. reportWhatTheFileDidNotReach(resolved, log) reportWhatTheFileSaysTheNodeIs(ctx, mode, log) @@ -96,27 +101,21 @@ func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logg // boot drop to debug everywhere else. On `seid keys list` nobody asked, and a line held above the // operator's own level buries the reports beside it that are actionable. // - // Holding keys back joins them. It is a problem, but not one an operator can act on, and its trigger - // is any sei.toml carrying a [p2p], [mempool] or root key, which is nearly every file somebody would - // write. It keeps its own level on the boot, because that is the one place holding them back changes - // what the node runs. - // // What a refused registration says, and what a key nothing declares says, report everywhere. Both are // things to fix. - said, warned := log.Info, log.Warn + said := log.Info if !runsANode(cmd) { - said, warned = log.Debug, log.Debug + said = log.Debug } - // One read for both halves. A section arriving between two reads would be absent from what is - // reported and present in what is dropped, which is undelivered and unreported at once. - forADecode, ownedByADecode := registry.SuppliedAndOwnedByDecodedSections(resolved) + // One read, and both halves are used: the values a decode has to deliver, and every key those + // sections own so the install below leaves them out. Two reads would let a section arrive between + // them, absent from the delivery and present in what the install drops. + forADecode, ownedByADecode := registry.ResolvedAndOwnedByDecodedSections(resolved) - // Only what the file itself wrote. The supplied set is filled by every channel, and a flag or a - // variable answering one of these keys does reach the node, so reporting it as read-as-it-always-has - // would be false as well as pointed at the wrong file. - heldFromTheFile := whatTheFileWroteForADecode(forADecode, written) - reportWhatThisInstallHoldsBack(heldFromTheFile, warned) + // The second delivery. Their file is read into a struct before this runs and nothing consults the + // source for them afterwards, so the values are decoded into that struct instead. + deliverDecodedSections(ctx, forADecode, log) // Every declared key a lookup reads, whether sei.toml mentioned it or not. There is no case where this // is empty for a reason an operator caused: the paths above already returned for a file that could not @@ -194,55 +193,6 @@ func everyKeyALookupReads(resolved registry.Resolved, ownedByADecode []string) r return out } -// whatTheFileWroteForADecode narrows the decoded sections' supplied values to the keys the file itself -// wrote, sorted. -// -// The supplied set is filled by the file, the environment and the flags alike, and only the file's keys are -// ones this install holds back in a way an operator can act on. A flag answering one of these keys reaches -// the node through the flag, so reporting it as reading the way it always has would be untrue, and naming -// the file for it would be untrue twice. -// -// Matched lower-cased, which is how the resolution matches a file's keys. -func whatTheFileWroteForADecode(bySection map[string]map[string]any, written map[string]any) []string { - inTheFile := make(map[string]bool, len(written)) - for key := range written { - inTheFile[strings.ToLower(key)] = true - } - var keys []string - for _, values := range bySection { - for key := range values { - if inTheFile[strings.ToLower(key)] { - keys = append(keys, key) - } - } - } - sort.Strings(keys) - return keys -} - -// reportWhatThisInstallHoldsBack names the supplied keys this install cannot deliver. -// -// A section whose reader decodes its file whole was read before this ran, so putting a value into the -// source reaches nothing for it. Those keys are left out on purpose. -// -// Left unreported they are invisible. They are absent from what was installed, and they are declared, so -// they are absent from the undeclared keys too. An operator who supplied only such keys would be told -// nothing was supplied while their node ran the old values, which is the failure this whole surface exists -// to remove. -// -// No source is named, because the resolution does not record which one answered. A file, a variable and a -// flag all arrive here as an override, and naming the file for a value an environment variable supplied is -// the same misattribution the undeclared-key report was split apart to end. -func reportWhatThisInstallHoldsBack(keys []string, say func(string, ...any)) { - if len(keys) == 0 { - return - } - shown, omitted := capLoggedItems(keys) - say("sei.toml writes keys whose reader decodes its file whole; this install cannot deliver them "+ - "and they read as they always have", - "count", len(keys), "keys", strings.Join(shown, ","), "omitted", omitted) -} - // reportWhatThisBinaryCouldNotUse names a registration this binary's own source got wrong. // // Not the operator's mistake and nothing they can fix. It reaches them anyway: a refused registration diff --git a/cmd/seid/cmd/configmanager/tendermint.go b/cmd/seid/cmd/configmanager/tendermint.go new file mode 100644 index 0000000000..0f6e7fe7af --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint.go @@ -0,0 +1,346 @@ +package configmanager + +import ( + "cmp" + "fmt" + "log/slog" + "net/url" + "os" + "regexp" + "strings" + + "github.com/spf13/viper" + + "github.com/sei-protocol/seilog" + + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" +) + +// deliverDecodedSections puts the resolved values of the decoded sections into the node's own +// configuration. +// +// Putting a value into the source a node reads is the whole delivery for a section whose reader looks its +// keys up one at a time. It is no delivery at all for the sections this covers, which the boot's handler +// reads once into a struct before this runs. Those values are decoded into that struct instead, which is +// the same mechanism the handler used and therefore the same casts, the same tags and the same hooks. +// +// Nothing here can stop a node starting, which is the one promise this manager makes. +func deliverDecodedSections(ctx *server.Context, bySection map[string]map[string]any, + log *slog.Logger) { + if len(bySection) == 0 { + return + } + if ctx == nil || ctx.Config == nil { + log.Error("no node configuration to deliver into; every one of these keys reads as it always has", + "sections", len(bySection)) + return + } + + // 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) + } +} + +// deliverOneSection decodes one section's resolved values into the node's configuration. +// +// Decoded into a copy of that configuration first, and published into the live one only once the whole +// section has decoded. A decoder gathers errors and keeps going, so a value it refuses partway leaves its +// target holding some of the new values and some of the old, with nothing to compare against and no way +// back. Rehearsing into a copy of the configuration the node already has, rather than into a fresh one, is +// what makes the rehearsal answer the same question: what a decoder writes can depend on what the target +// already holds, and only a copy holds the same things. +func deliverOneSection(ctx *server.Context, name string, values map[string]any, log *slog.Logger) { + keys := sortedKeys(values) + + 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 + } + + // The node's own rules, on the copy, before anything is published. A value can decode cleanly, mean + // what it says, and still be one the node refuses: a negative transaction-size ceiling decodes to + // minus one and then every transaction measures larger than it, so the node accepts none. Around + // thirty such checks live here and nothing above this can see any of them. + // + // Cheap here and nowhere else, because this is the one place a copy exists to test. It also inherits + // the section-scoped refusal, so a bad value costs its own section and not the whole file. + // Against what the node already holds, not against the rules alone. ValidateBasic answers for the whole + // configuration, and the boot never applies it to an existing config.toml, so a node can already be in + // a state these rules reject. Refusing on that would blame this section for a failure it did not cause + // and leave every later change unable to land. + if err := candidate.ValidateBasic(); err != nil { + if already := ctx.Config.ValidateBasic(); already != nil { + log.Warn("this node's configuration already fails its own rules, so this section cannot be "+ + "held to them; it is applied as written", + "section", name, "keys", strings.Join(keys, ","), "already", already) + } else { + log.Error("this section's written values leave the node's configuration invalid, so none of "+ + "the section is applied and every one of its keys reads as it always has", + "section", name, "keys", strings.Join(keys, ","), "err", err) + return + } + } + + if err := publishNodeConfig(ctx.Config, candidate); err != nil { + 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. + unread := asSet(append(unreadBefore, unreadAfter...)) + if len(unread) > 0 { + shown, omitted := capLoggedItems(sortedKeys(unread)) + log.Error("this section was applied and some of its keys cannot be read back, so nothing here "+ + "says whether those moved", "section", name, "count", len(shown)+omitted, + "keys", strings.Join(shown, ","), "omitted", omitted) + } + + reportWhatMoved(name, whatBothSidesCouldBeReadFor(keys, unread), 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 +} + +// whatBothSidesCouldBeReadFor drops the keys neither side could be read for. +// +// A key missing from both answers compares equal, so leaving it in makes the report say the section matches +// the node's own file, which is the statement the line above it exists to withhold. Dropped here rather +// than inside the comparison, because the comparison's job is to say what moved and this one's is to say +// which keys it can speak for. +func whatBothSidesCouldBeReadFor(keys []string, unread map[string]struct{}) []string { + out := make([]string, 0, len(keys)) + for _, key := range keys { + if _, missing := unread[key]; !missing { + out = append(out, key) + } + } + return out +} + +// reportWhatMoved names every key whose value the delivery changed, and what it changed from. +// +// The node's own configuration file still says what it said, and every tool an operator reaches for reads +// 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, in the forms a connection string writes one: the +// userinfo of a URL, a named field, quoted or not, with or without backslash escapes. This is the only +// place the running configuration is written down, so it is also the only place one would reach a log file, +// a journal, and whatever ships them onward. The node's own configuration file holds the same string and +// nothing reads it out to a log. +// +// The rendered list is capped for the reason every other one here is: the count is what an operator alerts +// on, and one line per key of a large section buries whichever of them mattered. +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 +} + +// redacted stands in for a secret a report would otherwise carry. +const redacted = "xxxxx" + +// passwordAssignment matches a password given as a named field, wherever it appears. +// +// A connection string carries one three ways and this covers two of them: a query parameter, as in +// "?password=", and one keyword of a space-separated list, as in "password=". PostgreSQL accepts both, +// and it accepts prefixed spellings such as "sslpassword", which the leading group keeps intact. +// +// The value runs to the next separator or the end, so nothing after it is swallowed. A quoted value is +// taken whole, because a keyword value may be quoted and a password may hold spaces: stopping at the first +// one leaves the rest of it in the line. +// +// A backslash escape is consumed with what follows it. Quoted and unquoted keyword values both escape a +// quote and a backslash that way, so a password holding one ends the match early and leaks its tail. +var passwordAssignment = regexp.MustCompile( + `(?i)(password|passwd|pwd)(\s*=\s*)('(?:\\.|[^'\\])*'|"(?:\\.|[^"\\])*"|(?:\\.|[^&\s\\])*)`) + +// credentialsInAURI matches the userinfo of a connection string that does not parse as a URL. +// +// A password holding a raw space, or any other byte a URL parser rejects in userinfo, makes the parse fail, +// and a connection string in that form carries no named field for the pattern above to find. Failing open +// there logs the whole credential, so this is the fallback: whatever sits between the scheme and the host, +// after the first colon, is removed. +var credentialsInAURI = regexp.MustCompile(`(://[^/@\s]*:)[^@\n]*(@)`) + +// withoutCredentials removes a password from a value that carries one. +// +// A setting can hold a connection string, and a connection string can carry a password in the userinfo of +// a URL, in a query parameter, or as one keyword of a list. All three are forms an operator can write, so +// all three are removed. +// +// Detected in the value rather than declared per key, because a list of the keys that can hold one is a +// list somebody keeps in step with every section anyone adds, and the first key forgotten is a password in +// a log. +func withoutCredentials(value string) string { + if u, err := url.Parse(value); err == nil && u.User != nil { + if _, set := u.User.Password(); set { + u.User = url.UserPassword(u.User.Username(), redacted) + value = u.String() + } + } else { + // The parse failed, so the userinfo above was never reached and a string in that form carries no + // named field either. Failing open here would log the whole credential, which is the one outcome + // this function exists to prevent. + value = credentialsInAURI.ReplaceAllString(value, "${1}"+redacted+"${2}") + } + return passwordAssignment.ReplaceAllString(value, "${1}${2}"+redacted) +} + +// 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..87315db908 --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint_copy.go @@ -0,0 +1,455 @@ +package configmanager + +import ( + "fmt" + "math" + "reflect" + "sort" + "strings" + "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] = whatAPointerHolds(value) + } +} + +// whatAPointerHolds returns the value behind a pointer, or the value itself. +// +// The decoder flattens a pointer to a struct by following it and leaves a pointer to anything else as a +// pointer. Rendering one of those gives an address, and an address is a different one on each side of a +// delivery, because publishing assigns a fresh pointer for a leaf that is not a struct. So the report would +// name the key as moved on every boot and print two addresses for it. +// +// Nothing reaches this today: every pointer leaf the node's configuration carries is deliberately left +// undeclared. The walk around it is driven from the type so that a field added later is covered, and this +// keeps that true for one more shape. +func whatAPointerHolds(value any) any { + v := reflect.ValueOf(value) + if v.Kind() != reflect.Pointer { + return value + } + if v.IsNil() { + return nil + } + return v.Elem().Interface() +} + +// TestReporter is the part of a test's own type this file needs. +// +// Named as an interface rather than taking *testing.T, so this file does not pull the testing package into +// a binary the boot path links. The behaviour is the point: the helper below has to be able to end the +// test, because a caller that could carry on would compare two answers produced by reading nothing. +type TestReporter interface { + Helper() + Fatalf(format string, args ...any) +} + +// DescribeForTest reads what a node's configuration holds for each key, as text. +// +// Exported for the tests that measure a booted node's configuration, which live beside the boot because +// 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 TestReporter, 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 +} + +// whatDecodesToSomethingElse reports written values the decoder accepts and turns into something the +// operator did not mean, with what they should have written. +// +// Four shapes, and every one of them decodes cleanly, which is why nothing later objects. +// +// 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 +} + +// 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..0033634747 --- /dev/null +++ b/cmd/seid/cmd/configmanager/tendermint_copy_test.go @@ -0,0 +1,242 @@ +package configmanager + +import ( + "fmt" + "reflect" + "sort" + "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 into the live one, so a refused value leaves the node's +// configuration untouched. That holds only if the copy shares nothing the decode can write through, and a +// decoder writes a list into the array its target already holds. One shared section, list or map and the +// rehearsal edits the original. +// +// 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{} + live := reflect.ValueOf(target).Elem() + for i := 0; i < live.NumField(); i++ { + f := live.Type().Field(i) + if f.Type.Kind() != reflect.Pointer || f.Type.Elem().Kind() != reflect.Struct { + continue + } + if live.Field(i).IsNil() { + continue + } + held[f.Name] = live.Field(i).Pointer() + } + if len(held) == 0 { + t.Fatal("this configuration holds no section behind a pointer of its own, so there is no identity " + + "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) + } +} + +// 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 +} + +// TestAPointerLeafRendersWhatItHolds covers a shape the report would name as moved on every boot. +// +// The decoder follows a pointer to a struct and leaves a pointer to anything else alone. Rendered, that is +// an address, and publishing assigns a fresh pointer for a leaf that is not a struct, so the two sides of a +// delivery hold different addresses for a value nobody changed. The report would say the key moved and +// print two of them. +// +// No pointer leaf is declared today. This holds the rendering to being right for one anyway, because the +// walk around it is driven from the type so that a field added later is covered. +func TestAPointerLeafRendersWhatItHolds(t *testing.T) { + seven := uint(7) + for _, tc := range []struct { + name string + value any + want string + }{ + {"a pointer to a number", &seven, "7"}, + {"a nil pointer", (*uint)(nil), ""}, + {"a plain number", uint(7), "7"}, + {"a string", "kv", "kv"}, + } { + t.Run(tc.name, func(t *testing.T) { + out := map[string]any{} + flatten("", map[string]any{"probe": tc.value}, out) + if got := fmt.Sprint(out["probe"]); got != tc.want { + t.Errorf("%s renders as %q, want %q. An address differs on each side of a delivery, so "+ + "the report would name the key as moved every boot", tc.name, got, tc.want) + } + }) + } +} diff --git a/cmd/seid/cmd/node_agreement_test.go b/cmd/seid/cmd/node_agreement_test.go new file mode 100644 index 0000000000..a65ffba43d --- /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, 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..3b2e11a42d --- /dev/null +++ b/cmd/seid/cmd/node_delivery_test.go @@ -0,0 +1,481 @@ +package cmd + +import ( + "fmt" + "os" + "path/filepath" + "sort" + "testing" + "time" + + "go.opentelemetry.io/otel/sdk/trace" + + "github.com/sei-protocol/sei-chain/cmd/seid/cmd/configmanager" + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// The second delivery, driven the way an operator reaches it. +// +// A section whose reader looks its keys up one at a time is delivered by putting the value into the source. +// The node's own configuration file is read once into a struct before any of that, so a value put into the +// source reaches nothing and has to be decoded into the struct instead. These read the setting the node +// runs rather than the source it was resolved into, because a key can be correct in the source and absent +// from the struct. + +// bootWithNodeFile runs a real boot against a sei.toml and a generated node configuration file. +func bootWithNodeFile(t *testing.T, seiToml string, edit func(*tmcfg.Config)) *server.Context { + t.Helper() + home := configtest.NewHome(t) + dir := filepath.Join(home.Root, "config") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + // The node's own file, generated the way the node generates it, so what the delivery writes over is + // what an operator would actually have. + live := tmcfg.DefaultConfig() + if edit != nil { + edit(live) + } + if err := tmcfg.WriteConfigFile(home.Root, live); err != nil { + t.Fatalf("render the node's configuration file: %v", err) + } + if seiToml != "" { + if err := os.WriteFile(filepath.Join(dir, "sei.toml"), []byte(seiToml), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + } + + cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home.Root); err != nil { + t.Fatalf("set --home: %v", err) + } + ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("the boot was refused: %v", err) + } + if ctx.Config == nil { + t.Fatal("the boot produced no node configuration") + } + return ctx +} + +const nodeFileHeader = "schema_version = 1\nnode_mode = \"validator\"\n" + +// TestAWrittenValueReachesTheNodesOwnConfiguration is the property the whole thing rests on. +func TestAWrittenValueReachesTheNodesOwnConfiguration(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\n[instrumentation]\nprometheus = true\nmax-open-connections = 41\n", nil) + + if !ctx.Config.Instrumentation.Prometheus { + t.Error("sei.toml turned the metrics listener on and the node runs with it off. The value was " + + "resolved and put into a source that nothing reading this file ever consults") + } + if got := ctx.Config.Instrumentation.MaxOpenConnections; got != 41 { + t.Errorf("sei.toml set max-open-connections to 41 and the node runs %d", got) + } +} + +// TestAnUnwrittenKeyTakesTheDeclaredValue is what makes sei.toml the configuration rather than a patch. +// +// A section read by a decode holds what its own file said, and that file is not consulted for a declared +// key under this manager. So a key sei.toml does not mention arrives at the value this binary declares for +// the kind of node this is, and what config.toml said about it does not survive. +// +// The fixture turns the key on in the node's own file, where the declared value is off, so the two +// disagree. Without that they agree and there is nothing to observe. +// +// This is the change with the largest consequence for an existing node, which is why a path that renders +// sei.toml from the files a node already has must land before this is switched on anywhere. +func TestAnUnwrittenKeyTakesTheDeclaredValue(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nsize = 4321\n", func(live *tmcfg.Config) { + live.Instrumentation.Prometheus = true + }) + + if got := ctx.Config.Mempool.Size; got != 4321 { + t.Fatalf("the written key arrived as %d, so this test cannot tell the two cases apart", got) + } + if ctx.Config.Instrumentation.Prometheus { + t.Error("the node's own file turned the metrics listener on, sei.toml said nothing about it, and " + + "the node still runs with it on. config.toml is still answering for a declared key, so " + + "sei.toml is a patch on the configuration rather than the configuration") + } +} + +// TestARefusedValueLeavesItsSectionAlone is the promise that makes this safe to enable. +// +// A decoder gathers errors and keeps going, so a value it refuses partway leaves its target holding some +// new values and some old, with nothing to compare against. The delivery decodes into a copy and publishes +// into the live one only once the whole section decodes, so a refused value leaves the section exactly as +// the node had it. +func TestARefusedValueLeavesItsSectionAlone(t *testing.T) { + configtest.Isolate(t) + ctx := bootWithNodeFile(t, nodeFileHeader+ + "\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 := 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) + } +} + +// keysADecodeDelivers returns every key the decoded sections own, sorted. +// +// Through the one accessor that answers both halves from a single read of the registry, so a test cannot +// see a registry the boot did not. +func keysADecodeDelivers() []string { + _, keys := registry.ResolvedAndOwnedByDecodedSections(registry.Resolved{}) + return keys +} + +// TestAValueTheNodesOwnRulesRejectIsRefused covers what decodes cleanly, means what it says, and still +// breaks the node. +// +// A negative transaction-size ceiling is a valid int, so nothing about its shape is wrong. The node then +// measures every transaction against it and finds all of them larger, so it accepts none. The node's own +// rules refuse exactly this, and they are checked on the rehearsal copy because that is the one place a +// copy exists to check. +func TestAValueTheNodesOwnRulesRejectIsRefused(t *testing.T) { + configtest.Isolate(t) + was := tmcfg.DefaultConfig().Mempool.MaxTxBytes + + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[mempool]\nmax-tx-bytes = -1\nsize = 4321\n", nil) + if got := ctx.Config.Mempool.MaxTxBytes; got != was { + t.Errorf("the node runs a transaction-size ceiling of %d after -1 was written, want the %d it "+ + "had. Every transaction measures larger than a negative ceiling, so the node would accept "+ + "none of them", got, was) + } + if got := ctx.Config.Mempool.Size; got == 4321 { + t.Error("the value beside the refused one was applied, so the section was published in part") + } +} + +// TestASectionLandsOnANodeAlreadyFailingItsOwnRules covers a node the boot never validated. +// +// The node's rules answer for the whole configuration, and a boot does not apply them to an existing +// config.toml, so a node can already hold a value they reject. Refusing on that blames the section being +// delivered for a failure it did not cause, and leaves every later change unable to land on that node. +func TestASectionLandsOnANodeAlreadyFailingItsOwnRules(t *testing.T) { + configtest.Isolate(t) + + // A node whose own file already holds a value its rules reject, in a section nobody is changing. + alreadyInvalid := func(c *tmcfg.Config) { c.Mempool.MaxTxBytes = -1 } + + ctx := bootWithNodeFile(t, nodeFileHeader+"\n[p2p]\nmax-incoming-connection-attempts = 7\n", alreadyInvalid) + if got := ctx.Config.P2P.MaxIncomingConnectionAttempts; got != 7 { + t.Errorf("the written value is %d on a node that already failed its own rules, want 7. A section "+ + "cannot be held to rules the node was already breaking, or nothing can ever be delivered to "+ + "it again", got) + } +} diff --git a/config/registry/delivery.go b/config/registry/delivery.go index 69a03b0dc9..7a2f19efb2 100644 --- a/config/registry/delivery.go +++ b/config/registry/delivery.go @@ -50,23 +50,24 @@ func DecodedSections() map[string]string { return out } -// SuppliedAndOwnedByDecodedSections answers both halves of the decoded-delivery question from one read. +// ResolvedAndOwnedByDecodedSections answers both halves of the decoded-delivery question from one read. // -// The values each decoded section was supplied, and every key those sections own whether supplied or not. A -// caller needs both: the first to deliver or report, the second to know what to leave out of a delivery that +// The resolved values each decoded section has to be handed, and every key those sections own. A caller +// needs both: the first to deliver or report, the second to know what to leave out of a delivery that // cannot carry it. // +// Every key the resolution answered, not only the ones a source wrote. A key sei.toml leaves out takes the +// value this binary declares, and that has to reach the struct like any other, because the struct is what +// the node reads and config.toml is not consulted for a declared key. That does replace what an operator's +// config.toml said for a key their sei.toml does not mention, and it is meant to: one file states what a +// node runs. +// // Together rather than from two calls, because a section arriving between two reads is absent from one // answer and present in the other, so its keys are dropped from the install by the second and not reported // by the first. Silently undelivered and unreported is the outcome one read exists to prevent. -func SuppliedAndOwnedByDecodedSections(resolved Resolved) (map[string]map[string]any, []string) { +func ResolvedAndOwnedByDecodedSections(resolved Resolved) (map[string]map[string]any, []string) { registered, _, owning := snapshot() - supplied := make(map[string]bool, len(resolved.Overrides)) - for _, key := range resolved.Overrides { - supplied[key] = true - } - out := map[string]map[string]any{} var everyKey []string for _, section := range registered { @@ -75,13 +76,14 @@ func SuppliedAndOwnedByDecodedSections(resolved Resolved) (map[string]map[string } everyKey = append(everyKey, section.Keys...) for _, key := range section.Keys { - if !supplied[key] { + value, answered := resolved.Values[key] + if !answered { continue } if out[section.Name] == nil { out[section.Name] = map[string]any{} } - out[section.Name][key] = resolved.Values[key] + out[section.Name][key] = value } } sort.Strings(everyKey) diff --git a/config/registry/delivery_test.go b/config/registry/delivery_test.go index dee2eacd16..ac23d01a7f 100644 --- a/config/registry/delivery_test.go +++ b/config/registry/delivery_test.go @@ -117,13 +117,13 @@ func TestResetClearsHowSectionsAreDelivered(t *testing.T) { } } -// TestOnlyASuppliedValueReachesADecodedSection is the difference between delivering a value and -// replacing an operator's file. +// TestEveryResolvedValueReachesADecodedSection is what makes one file the configuration. // -// A section read by a decode already holds what its own file said. Handing it a default would rewrite -// that on every boot for every key the operator's file does not mention, so a key that took its default -// is skipped and a key any other layer answered is delivered. -func TestOnlyASuppliedValueReachesADecodedSection(t *testing.T) { +// A section read by a decode holds what its own file said, and that file is not consulted for a declared +// key under this manager. So every key the resolution answered has to be handed over, including the ones +// that took their declared value, or a key sei.toml leaves out would keep whatever was on disk and the +// file would be a patch rather than the configuration. +func TestEveryResolvedValueReachesADecodedSection(t *testing.T) { registry.Reset() registerProbe(t, "mempool") registerProbe(t, "api") @@ -137,15 +137,16 @@ func TestOnlyASuppliedValueReachesADecodedSection(t *testing.T) { t.Fatalf("Resolve: %v", err) } - got, _ := registry.SuppliedAndOwnedByDecodedSections(resolved) - want := map[string]map[string]any{"mempool": {"mempool.a": "from the file"}} + got, _ := registry.ResolvedAndOwnedByDecodedSections(resolved) + want := map[string]map[string]any{ + "mempool": {"mempool.a": "from the file", "mempool.b": "from the default"}, + } if !reflect.DeepEqual(got, want) { - t.Errorf("the decoded sections are handed %v, want %v. A key nobody wrote arriving here replaces "+ - "an operator's own value, and a section delivered by a lookup arriving here is delivered "+ - "twice", got, want) + t.Errorf("the decoded sections are handed %v, want %v. Every key the resolution answered has to "+ + "arrive, and a section a lookup delivers must not", got, want) } - if _, held := got["mempool"]["mempool.b"]; held { - t.Error("mempool.b took its default and was handed to the decode anyway, which writes a value " + - "nobody chose over whatever the node's own file holds") + if _, wrong := got["api"]; wrong { + t.Error("api is delivered by a lookup and was handed to the decode as well, so its keys would " + + "be delivered twice") } } diff --git a/go.mod b/go.mod index e46ab4e48e..736705c6c1 100644 --- a/go.mod +++ b/go.mod @@ -188,7 +188,7 @@ require ( github.com/go-logr/stdr v1.2.2 // indirect github.com/go-ole/go-ole v1.3.0 // indirect github.com/go-sourcemap/sourcemap v2.1.3+incompatible // indirect - github.com/go-viper/mapstructure/v2 v2.5.0 // indirect + github.com/go-viper/mapstructure/v2 v2.5.0 github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2 // indirect github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 // indirect github.com/golang/snappy v1.0.0 // indirect