[ConfigManager] Deliver sei.toml for Decoded Sections 2/3

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 "+

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())
+	}
+}

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.

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