From 59a387ad0994b59bd6888faac7bdd2a161527b66 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 20 Aug 2026 15:04:18 -0700 Subject: [PATCH 1/7] config: the upstream server sections enter the registry Five sections whose keys belong to the Cosmos server, and the two registry capabilities they need. api 8 keys base 14 keys grpc 11 keys state-sync 3 keys telemetry 7 keys These sections have no owning package here. Their structs and their readers live in sei-cosmos, which this repository vendors rather than authors, so there is nowhere upstream to put a registration this registry would see. Four of the five register the upstream struct directly, because its mapstructure tags already name the keys the reader resolves. A section can now declare keys at the root of the file. The node-wide settings are written at the top of app.toml and read as pruning and halt-height, with no segment in front, so a section carrying a name into every key would rename all fourteen and an operator's existing file would reach none of them. A section therefore has a name it is looked up by and a prefix its keys carry, and for a root section the prefix is empty. Both walks build a key through one function, so a root key gains no separator on either side; reverting either one on its own fails a test, the value walk through the check that a rendered default states one value per declared key. Two keys can now collide where two prefixes never could. A key two sections both declare has one default rendered over the other, and which one depends on the order the sections are walked. And a root key that is also a section's name cannot be written at all, because a file holding both a value for that name and a table under it is not valid TOML, so one of the two is unreachable and nothing says which. Both are refused, in either registration order. A section can now say that an environment variable cannot supply one of its keys. The metric label set is a list of name and value rows and its reader asserts that exact shape rather than casting what it finds, so no single string satisfies it, and the assertion is the first statement of the whole server configuration. A resolved variable would install a value that stops the node; leaving the channel out means the file's value applies and the node runs. The reason is required rather than optional, because an operator whose variable is ignored has to be told why, and a refusal with no reason is itself refused. The metric section is the one here that needs a schema, and for one field's shape rather than for a spelling. Its label set is declared as untyped rows to match what the reader takes. A test holds every other field to the upstream field's name, tag and type, and holds the count of differing types at one, so a second divergence is a failure and a converged upstream type leaves the schema with nothing to justify it. Nothing here varies a default by mode. seid init writes the two interface toggles and the block retention per mode, so a node it provisioned carries those as written values, and these are what a node with nothing written runs. One declared value is not what a running node uses, and it is worth knowing which. The pruning strategy is declared as keeping everything, while the command line registers a flag of the same name defaulting to the standard strategy, and a bound flag is a source of its own below the file. A node started with no pruning key written prunes on the standard schedule. Whoever resolves for a running node has to supply the flag values to get the answer that node uses. The recorded configuration surface does not move, because nothing consumes the registry on a boot path yet. --- config/cosmosbase/cosmosbase.go | 129 +++++++++++++++ config/cosmosbase/cosmosbase_test.go | 224 +++++++++++++++++++++++++++ config/registry/environment.go | 42 +++++ config/registry/registry.go | 118 ++++++++++++-- config/registry/resolve.go | 16 +- config/registry/rootkeys_test.go | 211 +++++++++++++++++++++++++ 6 files changed, 723 insertions(+), 17 deletions(-) create mode 100644 config/cosmosbase/cosmosbase.go create mode 100644 config/cosmosbase/cosmosbase_test.go create mode 100644 config/registry/environment.go create mode 100644 config/registry/rootkeys_test.go diff --git a/config/cosmosbase/cosmosbase.go b/config/cosmosbase/cosmosbase.go new file mode 100644 index 0000000000..9b2312b6e4 --- /dev/null +++ b/config/cosmosbase/cosmosbase.go @@ -0,0 +1,129 @@ +// Package cosmosbase registers the configuration sections whose keys belong to the Cosmos server. +// +// These sections have no owning package inside this repository. Their structs and their readers live in +// sei-cosmos, which this repository vendors rather than authors, so there is nowhere upstream to put a +// registration that this repository's registry would see. A section belongs here only when its keys are +// upstream's; a section this repository owns registers in the package that owns its struct. +package cosmosbase + +import ( + "github.com/sei-protocol/sei-chain/config/registry" + srvconfig "github.com/sei-protocol/sei-chain/sei-cosmos/server/config" +) + +// The names these sections have in the configuration key space. +// +// BaseSectionName names a section whose keys carry no prefix at all. The name is for lookups and reports +// and is not part of any key, because giving those settings a section would rename every one of them. +const ( + BaseSectionName = "base" + APISectionName = "api" + GRPCSectionName = "grpc" + TelemetrySectionName = "telemetry" + StateSyncSectionName = "state-sync" +) + +// GlobalLabelsKey is the metric label set, which is the one key here no environment variable can supply. +const GlobalLabelsKey = TelemetrySectionName + ".global-labels" + +// Registration puts the upstream server's configuration sections in the registry. +// +// Four of the five register the upstream struct directly, because their mapstructure tags already name the +// keys their reader resolves. That is worth stating rather than assuming: the two SeiDB sections needed a +// schema precisely because their tags name something else. +func init() { + registry.RegisterRootKeys(BaseSectionName, &srvconfig.BaseConfig{}, baseDefaults) + registry.RegisterSection(APISectionName, &srvconfig.APIConfig{}, apiDefaults) + registry.RegisterSection(GRPCSectionName, &srvconfig.GRPCConfig{}, grpcDefaults) + registry.RegisterSection(TelemetrySectionName, &telemetrySchema{}, telemetryDefaults) + registry.RegisterSection(StateSyncSectionName, &srvconfig.StateSyncConfig{}, stateSyncDefaults) + + registry.RefuseFromEnvironment(GlobalLabelsKey, + "the metric label set is a list of name and value rows, and its reader takes that exact shape "+ + "rather than casting what it finds, so no single environment string can supply it. Write it "+ + "in the configuration file instead") +} + +// baseDefaults is what the node-wide settings resolve to for a node that has written nothing. +// +// The upstream defaults, unchanged by mode. Every one of these keys is read with a casting getter and no +// check that the key was present, so an absent key casts to a zero and clobbers the default beside it. +// Five of the fourteen have a non-zero default, and the pruning strategy is the one that matters, because +// an empty strategy is not a strategy. +// +// Three keys elsewhere in this package vary by node mode, and none of them varies here. seid init writes +// the interface toggles and the block retention per mode, so a node it provisioned carries those as +// written values, and a written value is what resolves. These are what a node with nothing written runs. +// +// One value here is not what a running node uses today, and it is worth knowing which. The pruning +// strategy is declared as keeping everything, while the command line registers a flag of the same name +// defaulting to the standard strategy, and a bound flag is a source of its own below the file. So a node +// started with no pruning key written prunes on the standard schedule and this states that it would keep +// everything. Whoever resolves for a running node has to supply the flag values to get the answer that +// node uses. +func baseDefaults(registry.Mode) any { return srvconfig.DefaultConfig().BaseConfig } + +// apiDefaults is what the REST interface settings resolve to for a node that has written nothing. +// +// The interface is off, for every mode. seid init turns it on for a full node and an archive node, so +// those carry it written, and a node whose file lacks the key does not serve REST whatever kind it is. +func apiDefaults(registry.Mode) any { return srvconfig.DefaultConfig().API } + +// grpcDefaults is what the gRPC settings resolve to for a node that has written nothing. +// +// The interface is on, which is the upstream default, and seid init writes it off for a validator and a +// seed. Six of these eleven keys are read only when the key is present, so for those the declared default +// is also what an absent key resolves to today. +// +// The six durations are declared as durations and written into a file as text, which is the shape the +// reader parses back. +func grpcDefaults(registry.Mode) any { return srvconfig.DefaultConfig().GRPC } + +// stateSyncDefaults is what the snapshot settings resolve to for a node that has written nothing. +// +// All three keys are read with a casting getter and no presence check, and the retention is the one that +// inverts: it is declared as keeping two snapshots and an absent key casts to zero, which the file format +// documents as keeping every snapshot. +func stateSyncDefaults(registry.Mode) any { return srvconfig.DefaultConfig().StateSync } + +// telemetrySchema declares the keys the metric settings reader resolves. +// +// A schema rather than the upstream type, and the only one of these five that needs one. The difference is +// a single field's type. The upstream struct declares the label set as a list of string pairs, and the +// reader takes a list of untyped rows: it asserts that exact shape rather than casting what it finds, and +// the struct's own type does not satisfy it, including that type's empty value. Registering the upstream +// type would resolve a default the reader refuses, and it refuses by returning an error that is the first +// statement of the whole server configuration, so the node stops. Every node, not only one that wrote the +// key. +// +// Every other field matches the upstream type, so this is one field's shape and not the section's. +type telemetrySchema struct { + ServiceName string `mapstructure:"service-name"` + Enabled bool `mapstructure:"enabled"` + EnableHostname bool `mapstructure:"enable-hostname"` + EnableHostnameLabel bool `mapstructure:"enable-hostname-label"` + EnableServiceLabel bool `mapstructure:"enable-service-label"` + PrometheusRetentionTime int64 `mapstructure:"prometheus-retention-time"` + GlobalLabels []any `mapstructure:"global-labels"` +} + +// telemetryDefaults is what the metric settings resolve to for a node that has written nothing. +// +// Read out of the upstream defaults rather than written again here, so a changed default moves both at +// once and this states only which key carries which setting. +// +// The label set is empty, which is what the upstream default holds, so there is nothing to convert into +// the untyped rows the reader takes. A test holds that emptiness, because a default that gained rows would +// need converting and would otherwise reach the reader as the shape it refuses. +func telemetryDefaults(registry.Mode) any { + live := srvconfig.DefaultConfig().Telemetry + return telemetrySchema{ + ServiceName: live.ServiceName, + Enabled: live.Enabled, + EnableHostname: live.EnableHostname, + EnableHostnameLabel: live.EnableHostnameLabel, + EnableServiceLabel: live.EnableServiceLabel, + PrometheusRetentionTime: live.PrometheusRetentionTime, + GlobalLabels: []any{}, + } +} diff --git a/config/cosmosbase/cosmosbase_test.go b/config/cosmosbase/cosmosbase_test.go new file mode 100644 index 0000000000..1220db25bd --- /dev/null +++ b/config/cosmosbase/cosmosbase_test.go @@ -0,0 +1,224 @@ +package cosmosbase + +import ( + "reflect" + "sort" + "testing" + + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/baseapp" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + srvconfig "github.com/sei-protocol/sei-chain/sei-cosmos/server/config" + "github.com/sei-protocol/sei-chain/sei-cosmos/telemetry" +) + +// requireDeclares holds one section's declared keys against the keys named for it. +func requireDeclares(t *testing.T, section string, reads []string) registry.Section { + t.Helper() + registered, ok := registry.Lookup(section) + if !ok { + t.Fatalf("%s is not registered, so nothing resolves its keys", section) + } + want := append([]string(nil), reads...) + sort.Strings(want) + if !reflect.DeepEqual(registered.Keys, want) { + t.Errorf("%s declares\n %v\nand its reader resolves\n %v", section, registered.Keys, want) + } + return registered +} + +// TestTheNodeWideKeysAreTheOnesTheirReaderResolves holds the root section against the server's constants. +// +// Fourteen keys and not one of them carries a segment in front. The reader looks these up by the constants +// below, so a prefix here would declare fourteen keys no operator writes and leave the real ones +// undeclared. +func TestTheNodeWideKeysAreTheOnesTheirReaderResolves(t *testing.T) { + section := requireDeclares(t, BaseSectionName, []string{ + server.FlagMinGasPrices, server.FlagPruning, server.FlagPruningKeepRecent, + server.FlagPruningKeepEvery, server.FlagPruningInterval, server.FlagHaltHeight, + server.FlagFreezeHeight, server.FlagHaltTime, server.FlagMinRetainBlocks, + server.FlagInterBlockCache, server.FlagIndexEvents, server.FlagCompactionInterval, + server.FlagConcurrencyWorkers, baseapp.FlagOccEnabled, + }) + if section.Prefix != "" { + t.Errorf("the section carries prefix %q, and one here renames every key it declares", section.Prefix) + } +} + +// TestTheSnapshotKeysAreTheOnesTheirReaderResolves holds the snapshot section against the server's +// constants. +func TestTheSnapshotKeysAreTheOnesTheirReaderResolves(t *testing.T) { + requireDeclares(t, StateSyncSectionName, []string{ + server.FlagStateSyncSnapshotInterval, + server.FlagStateSyncSnapshotKeepRecent, + server.FlagStateSyncSnapshotDir, + }) +} + +// TestTheRESTKeysAreTheOnesItsReaderResolves holds the REST section against the keys its reader looks up. +// +// Written out rather than taken from constants, because this reader has none: it looks each key up as a +// literal string where it reads it. That is the whole reason a comparison is worth making here. +func TestTheRESTKeysAreTheOnesItsReaderResolves(t *testing.T) { + requireDeclares(t, APISectionName, []string{ + "api.enable", "api.swagger", "api.enabled-unsafe-cors", "api.address", + "api.max-open-connections", "api.rpc-read-timeout", "api.rpc-write-timeout", + "api.rpc-max-body-bytes", + }) +} + +// TestTheGRPCKeysAreTheOnesItsReaderResolves holds the gRPC section against the keys its reader looks up. +// +// Written out for the same reason as the REST section: the reader has no constants for these. +func TestTheGRPCKeysAreTheOnesItsReaderResolves(t *testing.T) { + requireDeclares(t, GRPCSectionName, []string{ + "grpc.enable", "grpc.address", "grpc.max-recv-msg-size", "grpc.max-open-connections", + "grpc.max-connection-idle", "grpc.max-connection-age", "grpc.max-connection-age-grace", + "grpc.keepalive-time", "grpc.keepalive-timeout", "grpc.keepalive-min-time", + "grpc.keepalive-permit-without-stream", + }) +} + +// TestTheMetricKeysAreTheOnesItsReaderResolves holds the metric section against the keys its reader looks +// up, the label set among them. +func TestTheMetricKeysAreTheOnesItsReaderResolves(t *testing.T) { + requireDeclares(t, TelemetrySectionName, []string{ + "telemetry.service-name", "telemetry.enabled", "telemetry.enable-hostname", + "telemetry.enable-hostname-label", "telemetry.enable-service-label", + "telemetry.prometheus-retention-time", GlobalLabelsKey, + }) +} + +// TestTheMetricSchemaRestatesTheUpstreamTypeExactlyOnceOver is what a schema costs. +// +// The schema exists for one field's shape, so every other field has to be the upstream field: same name, +// same tag, same type. A field that drifted would declare a key under a spelling the reader does not look +// up, or resolve a value of a type it cannot take, and the section would go on registering cleanly either +// way. +func TestTheMetricSchemaRestatesTheUpstreamTypeExactlyOnceOver(t *testing.T) { + upstream := reflect.TypeOf(telemetry.Config{}) + schema := reflect.TypeOf(telemetrySchema{}) + if schema.NumField() != upstream.NumField() { + t.Fatalf("the schema has %d fields and the upstream type has %d; a field on one side only is "+ + "either a key nothing reads or a setting nothing declares", + schema.NumField(), upstream.NumField()) + } + + differing := 0 + for i := range schema.NumField() { + got, want := schema.Field(i), upstream.Field(i) + if got.Name != want.Name { + t.Errorf("field %d is %s here and %s upstream", i, got.Name, want.Name) + continue + } + if got.Tag != want.Tag { + t.Errorf("%s is tagged %q here and %q upstream, so it declares a key the reader does not "+ + "look up", got.Name, got.Tag, want.Tag) + } + if got.Type == want.Type { + continue + } + differing++ + if got.Name != "GlobalLabels" { + t.Errorf("%s is %s here and %s upstream. The label set is the only field whose shape this "+ + "schema changes, so a second one is a divergence nothing decided", + got.Name, got.Type, want.Type) + } + } + if differing != 1 { + t.Errorf("%d fields differ in type, want exactly one. If the upstream type came to match, this "+ + "schema is a restatement with nothing left to justify it", differing) + } +} + +// TestTheUpstreamDefaultCarriesNoLabels holds the assumption the declared label set is built on. +// +// The declared default is an empty list of rows, which is right only while the upstream default holds no +// labels. A default that gained a pair would need converting into the untyped rows the reader takes, and +// without that it reaches the reader as the shape it refuses. +func TestTheUpstreamDefaultCarriesNoLabels(t *testing.T) { + if got := srvconfig.DefaultConfig().Telemetry.GlobalLabels; len(got) != 0 { + t.Errorf("the upstream default carries %d label rows: %v. They need converting into untyped rows "+ + "here, because the reader asserts that shape rather than casting what it finds", len(got), got) + } +} + +// TestTheLabelSetIsRefusedFromTheEnvironment covers the one key no variable here can supply. +// +// Its reader asserts a list of untyped rows and an environment carries one string, so resolving the +// variable installs a value the reader refuses, and it refuses in the first statement of the whole server +// configuration. The node stops. Leaving the channel out means the file's value applies and the node runs. +func TestTheLabelSetIsRefusedFromTheEnvironment(t *testing.T) { + reason, refused := registry.EnvCannotDeliver()[GlobalLabelsKey] + if !refused { + t.Fatalf("%s is not refused from the environment, so a variable naming it resolves to a string "+ + "and installing that stops the node", GlobalLabelsKey) + } + if reason == "" { + t.Error("the refusal carries no reason, so an operator whose variable is ignored cannot be told why") + } + + resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{ + LookupEnv: func(name string) (string, bool) { + if name == registry.EnvName(GlobalLabelsKey) { + return "chain_id=pacific-1", true + } + return "", false + }, + }) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if got := resolved.Values[GlobalLabelsKey]; !reflect.DeepEqual(got, []any{}) { + t.Errorf("%s resolved to %#v (%T), want the declared default it was left to", + GlobalLabelsKey, got, got) + } + for _, key := range resolved.Overrides { + if key == GlobalLabelsKey { + t.Errorf("%s is reported as a value an operator supplied, and the variable did nothing", + GlobalLabelsKey) + } + } +} + +// TestDefaultsAreTheUpstreamOnesForEveryMode covers the value side of all five registrations. +// +// Unchanged by mode, which is the decision worth pinning. seid init writes three of these keys per mode, +// so a node it provisioned carries them as written values; these are what a node with nothing written +// runs. +func TestDefaultsAreTheUpstreamOnesForEveryMode(t *testing.T) { + for _, mode := range registry.Modes() { + live := srvconfig.DefaultConfig() + for _, c := range []struct { + section string + got any + want any + }{ + {BaseSectionName, baseDefaults(mode), live.BaseConfig}, + {APISectionName, apiDefaults(mode), live.API}, + {GRPCSectionName, grpcDefaults(mode), live.GRPC}, + {StateSyncSectionName, stateSyncDefaults(mode), live.StateSync}, + } { + if !reflect.DeepEqual(c.got, c.want) { + t.Errorf("mode %q: %s resolves to something other than the upstream default", mode, c.section) + } + } + + metrics, ok := telemetryDefaults(mode).(telemetrySchema) + if !ok { + t.Fatalf("mode %q: the metric defaults returned %T, want the schema", mode, telemetryDefaults(mode)) + } + if metrics.Enabled != live.Telemetry.Enabled || + metrics.PrometheusRetentionTime != live.Telemetry.PrometheusRetentionTime || + metrics.ServiceName != live.Telemetry.ServiceName { + t.Errorf("mode %q: the metric defaults are not the upstream ones: %+v", mode, metrics) + } + } +} + +// TestEverySectionHereRegistersCleanly covers what the registry itself refuses. +func TestEverySectionHereRegistersCleanly(t *testing.T) { + for _, defect := range registry.Defects() { + t.Errorf("%s is registered and defective: %v", defect.Section, defect.Err) + } +} diff --git a/config/registry/environment.go b/config/registry/environment.go new file mode 100644 index 0000000000..ef2e013ed3 --- /dev/null +++ b/config/registry/environment.go @@ -0,0 +1,42 @@ +package registry + +import "fmt" + +// envCannotDeliver holds the keys an environment variable cannot supply, with the reason. +var envCannotDeliver = map[string]string{} + +// RefuseFromEnvironment records that an environment variable cannot supply a key. +// +// An environment carries one string per name. Most readers cast that string into whatever the setting +// needs, so the environment works for them. A reader that takes its value's exact type instead cannot be +// handed a string at all, and no spelling of the variable would satisfy it. +// +// Resolving such a key from the environment puts an unusable value at the top of the order, and installing +// it stops the node. Leaving the channel out means the file's value applies and the node runs. That is +// deliberately not what the machinery this replaces does, which resolves the variable and refuses to +// start, so the difference is recorded rather than assumed. A value silently doing nothing is the failure +// this whole surface exists to remove, which is why the reason is required and not optional. +// +// Called from the owning package, beside its registration, so the reason sits with the code that knows it. +func RefuseFromEnvironment(key, reason string) { + mu.Lock() + defer mu.Unlock() + if reason == "" { + defects = append(defects, Defect{Section: key, Err: fmt.Errorf( + "refusing %q from the environment with no reason; an operator whose variable is ignored has "+ + "to be told why", key)}) + return + } + envCannotDeliver[key] = reason +} + +// EnvCannotDeliver returns the keys an environment variable cannot supply, and why. +func EnvCannotDeliver() map[string]string { + mu.RLock() + defer mu.RUnlock() + out := make(map[string]string, len(envCannotDeliver)) + for key, reason := range envCannotDeliver { + out[key] = reason + } + return out +} diff --git a/config/registry/registry.go b/config/registry/registry.go index 008738680c..a9753d270d 100644 --- a/config/registry/registry.go +++ b/config/registry/registry.go @@ -28,8 +28,16 @@ func Modes() []Mode { return []Mode{ModeValidator, ModeFull, ModeSeed, ModeArchi // Section is one registered configuration section. type Section struct { - // Name is the section's own segment, and the first segment of every key it declares. + // Name identifies the section. A lookup, a report and a defect are keyed by it, and for most + // sections it is also the first segment of every key. Name string + // Prefix is the first segment of every key this section declares, and is empty for a section whose + // keys sit at the root of the file with no section of their own. + // + // Separate from Name because the two do different jobs. A node-wide setting such as the pruning + // strategy is written at the top of app.toml and read as "pruning", so it has no segment to take a + // name from, and it still needs one to be looked up and reported under. + Prefix string // Keys are the dotted paths this section declares, sorted. Keys []string // Defaults returns the section's default for a mode. @@ -68,7 +76,23 @@ var ( // It never panics. A registration this package cannot use is recorded as a Defect and the // section is not registered. func RegisterSection(name string, prototype any, defaults func(Mode) any) { - keys, err := deriveKeys(name, prototype) + record(name, name, prototype, defaults) +} + +// RegisterRootKeys records a section whose keys sit at the root of the file, with no section of their own. +// +// name identifies the section for lookups and reports and is not part of any key. Everything else matches +// RegisterSection: the keys come from the mapstructure tags, and the tags are the only spelling. +// +// Some settings are node-wide and are written at the top of a file rather than inside a table. Giving them +// a section would rename them, and a renamed key is one an operator's existing file no longer reaches. +func RegisterRootKeys(name string, prototype any, defaults func(Mode) any) { + record(name, "", prototype, defaults) +} + +// record is the one path both registrations take. +func record(name, prefix string, prototype any, defaults func(Mode) any) { + keys, err := deriveKeys(name, prefix, prototype) mu.Lock() defer mu.Unlock() @@ -82,14 +106,73 @@ func RegisterSection(name string, prototype any, defaults func(Mode) any) { defects = append(defects, Defect{Section: name, Err: fmt.Errorf("section registered twice")}) return } + if err := refuseOverlap(name, prefix, keys); err != nil { + defects = append(defects, Defect{Section: name, Err: err}) + return + } if err := envNamesAreDistinct(keys); err != nil { defects = append(defects, Defect{Section: name, Err: err}) return } - sections[name] = Section{Name: name, Keys: keys, Defaults: defaults} + sections[name] = Section{Name: name, Prefix: prefix, Keys: keys, Defaults: defaults} } } +// refuseOverlap rejects a registration whose keys cannot coexist with what is already registered. +// Callers hold mu. +// +// Two shapes of overlap, and neither could happen while every key carried its section's name. A key two +// sections both declare has one default rendered over the other, and which one depends on the order the +// sections are walked. And a root key that is also a section's name cannot be written at all: a file +// holding both a value for that name and a table under it is not valid TOML, so one of the two is +// unreachable and nothing says which. +// +// The first shape reaches the environment check below as well, which would refuse it for the wrong +// reason: two spellings of one variable, when the keys are in fact the same key. This names it as itself. +func refuseOverlap(name, prefix string, keys []string) error { + declaredBy := map[string]string{} + sectionNamed := map[string]string{} + for _, s := range sections { + for _, key := range s.Keys { + declaredBy[key] = s.Name + } + if s.Prefix != "" { + sectionNamed[s.Prefix] = s.Name + } + } + + for _, key := range keys { + if owner, taken := declaredBy[key]; taken { + return fmt.Errorf("%s declares %q and so does %s; one default renders over the other and "+ + "which one wins depends on the order the sections are walked", name, key, owner) + } + if prefix != "" { + continue + } + if owner, taken := sectionNamed[key]; taken { + return fmt.Errorf("%s declares %q at the root of the file and %s is a section of that name; "+ + "a file cannot hold both a value for %q and a table under it, so one of them is "+ + "unreachable", name, key, owner, key) + } + } + + if prefix == "" { + return nil + } + for _, s := range sections { + if s.Prefix != "" { + continue + } + for _, key := range s.Keys { + if key == prefix { + return fmt.Errorf("%s is a section named %q and %s declares %q at the root of the file; "+ + "a file cannot hold both a table and a value under that name", name, prefix, s.Name, key) + } + } + } + return nil +} + // envNamesAreDistinct refuses keys that share one environment spelling. Callers hold mu. // // Dots and hyphens both become underscores, so two keys differing only in that punctuation answer to @@ -166,19 +249,19 @@ func Keys() []string { // outside state-commit.flatkv.*. Ninety-two operator-facing keys reach their field only through a // spelling the tags do not produce, and a silent fallback is what made that invisible. Refusing to // guess is what keeps the tag authoritative. -func deriveKeys(section string, prototype any) ([]string, error) { - if section == "" { +func deriveKeys(name, prefix string, prototype any) ([]string, error) { + if name == "" { return nil, fmt.Errorf("section name is empty") } - if section != strings.ToLower(section) { + if name != strings.ToLower(name) { return nil, fmt.Errorf("section name %q is not lower case; configuration sources "+ - "enumerate lower-cased, so a key under it would never match a written one", section) + "enumerate lower-cased, so a key under it would never match a written one", name) } - if bad, found := unaddressableChar(section); found { + if bad, found := unaddressableChar(name); found { return nil, fmt.Errorf("section name %q carries %q, and a section is one segment. A dotted name "+ "declares keys inside another section's subtree, where the two sections' defaults land in "+ "one map and whichever renders last silently wins; a space cannot be written in an "+ - "environment variable name at all", section, bad) + "environment variable name at all", name, bad) } if prototype == nil { return nil, fmt.Errorf("no struct") @@ -192,7 +275,7 @@ func deriveKeys(section string, prototype any) ([]string, error) { } var keys []string - if err := walk(t, section, &keys, map[reflect.Type]bool{}); err != nil { + if err := walk(t, prefix, &keys, map[reflect.Type]bool{}); err != nil { return nil, err } if len(keys) == 0 { @@ -254,15 +337,15 @@ func walk(t reflect.Type, prefix string, keys *[]string, open map[reflect.Type]b if ft.Kind() != reflect.Struct { return fmt.Errorf("%s.%s is squashed but is a %s, not a struct", prefix, f.Name, ft.Kind()) } - if err := walkSubtree(ft, prefix, prefix+"."+f.Name, keys, open); err != nil { + if err := walkSubtree(ft, prefix, join(prefix, f.Name), keys, open); err != nil { return err } continue } - path := prefix + "." + tag + path := join(prefix, tag) if ft.Kind() == reflect.Struct && !isLeaf(ft) { - if err := walkSubtree(ft, path, prefix+"."+f.Name, keys, open); err != nil { + if err := walkSubtree(ft, path, join(prefix, f.Name), keys, open); err != nil { return err } continue @@ -272,6 +355,14 @@ func walk(t reflect.Type, prefix string, keys *[]string, open map[reflect.Type]b return nil } +// join appends a key segment to a prefix, and returns the segment alone when there is no prefix. +func join(prefix, segment string) string { + if prefix == "" { + return segment + } + return prefix + "." + segment +} + // walkSubtree appends the keys a struct-typed field declares, and refuses one that declares none. // // A struct configuration cannot reach is a setting an operator writes into nothing. A defined type @@ -362,6 +453,7 @@ func Reset() { defer mu.Unlock() sections = map[string]Section{} defects = nil + envCannotDeliver = map[string]string{} } // envPrefix is the environment namespace for every derived key. diff --git a/config/registry/resolve.go b/config/registry/resolve.go index dfcca8f9e3..c47281a293 100644 --- a/config/registry/resolve.go +++ b/config/registry/resolve.go @@ -63,6 +63,7 @@ func Resolve(mode Mode, from Sources) (Resolved, error) { return out, err } declared := declaredKeys(registered) + undeliverable := EnvCannotDeliver() out.Values = make(map[string]any, len(declared)) for key, v := range defaults { @@ -75,7 +76,7 @@ func Resolve(mode Mode, from Sources) (Resolved, error) { // order, which is why nothing exports it. for _, values := range []map[string]any{ fileValues(from.File), - envValues(declared, from.LookupEnv), + envValues(declared, undeliverable, from.LookupEnv), from.Flags, } { for key, v := range values { @@ -128,7 +129,7 @@ func declaredKeys(registered []Section) map[string]bool { func defaultValues(mode Mode, registered []Section) (map[string]any, error) { out := map[string]any{} for _, s := range registered { - values, err := sectionValues(s.Name, s.Defaults(mode)) + values, err := sectionValues(s.Prefix, s.Defaults(mode)) if err != nil { return out, fmt.Errorf("section %q default for mode %q: %w", s.Name, mode, err) } @@ -252,7 +253,7 @@ func walkValues(v reflect.Value, prefix string, out map[string]any) error { } continue } - path := prefix + "." + tag + path := join(prefix, tag) if fv.Kind() == reflect.Struct && !isLeaf(fv.Type()) { if err := walkValues(fv, path, out); err != nil { return err @@ -272,12 +273,19 @@ func walkValues(v reflect.Value, prefix string, out map[string]any) error { // declared is passed in rather than read here, so this shares Resolve's snapshot. Reading the registry // again would ask for a key the caller's declared set does not hold, and the answer would come back // only to be reported as one no section declares. -func envValues(declared map[string]bool, lookup func(string) (string, bool)) map[string]any { +func envValues(declared map[string]bool, undeliverable map[string]string, + lookup func(string) (string, bool)) map[string]any { if lookup == nil { return nil } out := map[string]any{} for key := range declared { + // A key no variable can carry is left to the sources that can. Resolving it would put a string + // at the top of the order for a reader that takes the exact type, and installing that stops the + // node. What an operator loses is the channel; what they keep is a node that boots. + if _, refused := undeliverable[key]; refused { + continue + } // An empty value is treated as unset. A variable exported empty is far more often a shell // artefact than a deliberate empty string, and the two are indistinguishable here. The cost is // that clearing a key by exporting it empty reads as touching nothing, and Overrides will not diff --git a/config/registry/rootkeys_test.go b/config/registry/rootkeys_test.go new file mode 100644 index 0000000000..6bcc1a2a41 --- /dev/null +++ b/config/registry/rootkeys_test.go @@ -0,0 +1,211 @@ +package registry_test + +import ( + "reflect" + "sort" + "strings" + "testing" + + "github.com/sei-protocol/sei-chain/config/registry" +) + +// nodeWide is a probe for the settings written at the top of a file rather than inside a table. +type nodeWide struct { + Pruning string `mapstructure:"pruning"` + HaltHeight uint64 `mapstructure:"halt-height"` + Concurrency int `mapstructure:"concurrency-workers"` +} + +// TestARootSectionDeclaresKeysWithNoPrefix is the whole of what registering root keys adds. +// +// Some settings are node-wide and are written at the top of a file. Giving them a section would rename +// them, and a renamed key is one an operator's existing file no longer reaches. +func TestARootSectionDeclaresKeysWithNoPrefix(t *testing.T) { + registry.Reset() + registry.RegisterRootKeys("base", &nodeWide{}, func(registry.Mode) any { + return nodeWide{Pruning: "nothing", Concurrency: 4} + }) + for _, d := range registry.Defects() { + t.Fatalf("registering root keys was refused: %v", d.Err) + } + + section, ok := registry.Lookup("base") + if !ok { + t.Fatal("the section did not register under its name, so nothing can look it up or report on it") + } + if section.Prefix != "" { + t.Errorf("the section carries prefix %q, and one here renames every key it declares", section.Prefix) + } + if got := strings.Join(section.Keys, ","); got != "concurrency-workers,halt-height,pruning" { + t.Errorf("derived %q, want the three keys with no prefix. A leading segment is a key no operator "+ + "writes", got) + } + + // The default has to render under the same prefix-free names, or a declared key states no value and + // the resolution is refused rather than short. + resolved, err := registry.Resolve(registry.ModeFull, registry.Sources{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if got := resolved.Values["pruning"]; got != "nothing" { + t.Errorf("pruning resolved to %#v, want %q", got, "nothing") + } +} + +// TestARootKeyAndASectionCannotShareAName holds a limit of the file format, not a matter of taste. +// +// TOML cannot express a value for pruning and a table under pruning in one file, so one of the two is +// unwritable and which one an operator lost would depend on where in the file they wrote it. Registration +// order is not something an operator can see, so the refusal cannot depend on it either. +func TestARootKeyAndASectionCannotShareAName(t *testing.T) { + nested := func() (string, any, func(registry.Mode) any) { + return "pruning", &struct { + Mode string `mapstructure:"mode"` + }{}, func(registry.Mode) any { + return struct { + Mode string `mapstructure:"mode"` + }{Mode: "nothing"} + } + } + root := func() (string, any, func(registry.Mode) any) { + return "base", &struct { + Pruning string `mapstructure:"pruning"` + }{}, func(registry.Mode) any { + return struct { + Pruning string `mapstructure:"pruning"` + }{Pruning: "nothing"} + } + } + + t.Run("the section registers first", func(t *testing.T) { + registry.Reset() + registry.RegisterSection(nested()) + registry.RegisterRootKeys(root()) + if _, ok := registry.Lookup("base"); ok { + t.Error("the root section registered a key that is also a section name. A file cannot hold " + + "both, so one of them is unreachable and nothing says which") + } + if len(registry.Defects()) != 1 { + t.Errorf("recorded %d defects, want one naming the collision", len(registry.Defects())) + } + }) + + t.Run("the root key registers first", func(t *testing.T) { + registry.Reset() + registry.RegisterRootKeys(root()) + registry.RegisterSection(nested()) + if _, ok := registry.Lookup("pruning"); ok { + t.Error("a section registered under a name a root key already holds") + } + if len(registry.Defects()) != 1 { + t.Errorf("recorded %d defects, want one naming the collision", len(registry.Defects())) + } + }) +} + +// TestTwoSectionsCannotDeclareTheSameKey was impossible while every key carried its section's name. +// +// Two prefixes cannot collide. Two root sections can, and the default rendered for such a key would be +// whichever section the walk reached last. +func TestTwoSectionsCannotDeclareTheSameKey(t *testing.T) { + registry.Reset() + same := func(name string) { + registry.RegisterRootKeys(name, &struct { + Pruning string `mapstructure:"pruning"` + }{}, func(registry.Mode) any { + return struct { + Pruning string `mapstructure:"pruning"` + }{Pruning: "nothing"} + }) + } + same("base") + same("other") + + if _, ok := registry.Lookup("other"); ok { + t.Fatal("both sections declared the same key. One default renders over the other and which one " + + "wins depends on the order the sections are walked, so the value a node runs is not decided " + + "by anything an operator or a reviewer can see") + } + defects := registry.Defects() + if len(defects) != 1 { + t.Fatalf("recorded %d defects, want one", len(defects)) + } + // Named as one key two sections declare, rather than as two spellings of one variable, which is what + // the environment check would have called it. + if got := defects[0].Err.Error(); !strings.Contains(got, "and so does") { + t.Errorf("the refusal reads %q, and an identical key is not an environment spelling collision", got) + } +} + +// TestAKeyTheEnvironmentCannotDeliverIsLeftToTheOtherSources is what refusing a channel buys. +// +// The environment carries one string per name. A reader taking its value's exact type cannot be handed +// one, so resolving the variable installs a value that stops the node. Skipping it means the file's value +// applies and the node runs. +func TestAKeyTheEnvironmentCannotDeliverIsLeftToTheOtherSources(t *testing.T) { + registry.Reset() + registry.RegisterSection("probe", &struct { + Rows []any `mapstructure:"rows"` + Plain string `mapstructure:"plain"` + }{}, func(registry.Mode) any { + return struct { + Rows []any `mapstructure:"rows"` + Plain string `mapstructure:"plain"` + }{Rows: []any{}, Plain: "from the default"} + }) + registry.RefuseFromEnvironment("probe.rows", "its reader takes the exact type rather than casting") + for _, d := range registry.Defects() { + t.Fatalf("the registration was refused: %v", d.Err) + } + + // Both variables are set. Only the one the environment can carry is allowed to answer. + resolved, err := registry.Resolve(registry.ModeFull, registry.Sources{ + LookupEnv: func(name string) (string, bool) { + switch name { + case "SEID_PROBE_ROWS": + return "chain_id=pacific-1", true + case "SEID_PROBE_PLAIN": + return "from the environment", true + } + return "", false + }, + }) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + + if got := resolved.Values["probe.rows"]; !reflect.DeepEqual(got, []any{}) { + t.Errorf("probe.rows resolved to %#v (%T), want the default it was left to. Its reader takes a "+ + "list of rows, so installing the environment's string stops the node", got, got) + } + if got := resolved.Values["probe.plain"]; got != "from the environment" { + t.Errorf("probe.plain resolved to %#v; refusing one key's channel closed another's", got) + } + sort.Strings(resolved.Overrides) + if got := strings.Join(resolved.Overrides, ","); got != "probe.plain" { + t.Errorf("overrides are %q, want only probe.plain. A key nothing supplied is not one an operator "+ + "has taken responsibility for", got) + } +} + +// TestRefusingAChannelWithoutAReasonIsItselfRefused keeps the exemption from being unexplainable. +// +// A key left out of the environment layer is one whose variable does nothing, and an operator told that +// has to be told why. A refusal with no reason gives a diagnostic nothing to print. +func TestRefusingAChannelWithoutAReasonIsItselfRefused(t *testing.T) { + registry.Reset() + registry.RefuseFromEnvironment("probe.rows", "") + if len(registry.Defects()) != 1 { + t.Fatalf("recorded %d defects, want one naming the key with no reason", len(registry.Defects())) + } + if _, refused := registry.EnvCannotDeliver()["probe.rows"]; refused { + t.Error("the key was refused from the environment anyway. Its variable would then be ignored " + + "with nothing able to say why, which is worse than either resolving it or not") + } + + registry.Reset() + registry.RefuseFromEnvironment("probe.rows", "its reader takes the exact type") + if _, refused := registry.EnvCannotDeliver()["probe.rows"]; !refused { + t.Error("a refusal carrying a reason was not recorded") + } +} From d87787d0279011ed81301ecd42415b9f78c0be98 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Fri, 21 Aug 2026 08:14:51 -0700 Subject: [PATCH 2/7] config: answer the upstream sections per kind of node Three of these settings mean something different depending on what kind of node asks, and all five sections answered the same for every one. The binary already states the rules, in what it applies when it writes a file, and every section here now answers through them. Each of the three matters in a different direction. A full node and an archive node exist to serve queries, and both interfaces that serve them were declared closed. A validator is meant to expose as little as it can, and gRPC was declared open on every one of them, which is the opposite of what the rule beside it says it is for. And the number of blocks a node retains was declared as keeping everything for a full node, where the rule prunes at a hundred thousand. The rules are read rather than restated, so one added later moves these sections with nothing here changing, and the test writes the three values out by kind of node so a change to the rules fails and gets looked at. Resolving every mode as a validator, opening gRPC on a validator, and changing the retention each fail it. The two sections no rule touches answer through the same function, so there is one place a mode is applied rather than a decision per section about whether to apply it. --- config/cosmosbase/cosmosbase.go | 43 +++++++++++++++------- config/cosmosbase/cosmosbase_test.go | 54 ++++++++++++++++++++++++---- 2 files changed, 78 insertions(+), 19 deletions(-) diff --git a/config/cosmosbase/cosmosbase.go b/config/cosmosbase/cosmosbase.go index 9b2312b6e4..ad64b5eff7 100644 --- a/config/cosmosbase/cosmosbase.go +++ b/config/cosmosbase/cosmosbase.go @@ -7,6 +7,7 @@ package cosmosbase import ( + "github.com/sei-protocol/sei-chain/app/params" "github.com/sei-protocol/sei-chain/config/registry" srvconfig "github.com/sei-protocol/sei-chain/sei-cosmos/server/config" ) @@ -44,6 +45,21 @@ func init() { "in the configuration file instead") } +// forMode is the server configuration a node of this kind is meant to run. +// +// The upstream defaults with the binary's own mode rules applied. Every section here answers through this, +// so a section states what a kind of node is meant to run rather than what the type holds before any mode +// is considered, and a rule added to those rules later moves these sections with nothing here changing. +// +// Three settings differ by mode today and each of them matters in a different direction. A node that +// serves queries needs the interfaces that serve them; a validator is meant to expose as little as it can; +// and how many blocks a node retains is a decision about its disk. +func forMode(mode registry.Mode) *srvconfig.Config { + out := srvconfig.DefaultConfig() + params.SetAppConfigByMode(out, params.NodeMode(mode)) + return out +} + // baseDefaults is what the node-wide settings resolve to for a node that has written nothing. // // The upstream defaults, unchanged by mode. Every one of these keys is read with a casting getter and no @@ -61,30 +77,31 @@ func init() { // started with no pruning key written prunes on the standard schedule and this states that it would keep // everything. Whoever resolves for a running node has to supply the flag values to get the answer that // node uses. -func baseDefaults(registry.Mode) any { return srvconfig.DefaultConfig().BaseConfig } +func baseDefaults(mode registry.Mode) any { return forMode(mode).BaseConfig } // apiDefaults is what the REST interface settings resolve to for a node that has written nothing. // -// The interface is off, for every mode. seid init turns it on for a full node and an archive node, so -// those carry it written, and a node whose file lacks the key does not serve REST whatever kind it is. -func apiDefaults(registry.Mode) any { return srvconfig.DefaultConfig().API } +// On for a full node and an archive node, off for a validator and a seed. Serving queries is what the +// first two are for, and the second two are meant to expose as little as they can. +func apiDefaults(mode registry.Mode) any { return forMode(mode).API } // grpcDefaults is what the gRPC settings resolve to for a node that has written nothing. // -// The interface is on, which is the upstream default, and seid init writes it off for a validator and a -// seed. Six of these eleven keys are read only when the key is present, so for those the declared default -// is also what an absent key resolves to today. +// On for a full node and an archive node, off for a validator and a seed, which is the same rule the REST +// interface follows and for the same reason. The upstream default is on for every kind, so declaring that +// would state an open interface on the nodes meant to expose the least. // -// The six durations are declared as durations and written into a file as text, which is the shape the -// reader parses back. -func grpcDefaults(registry.Mode) any { return srvconfig.DefaultConfig().GRPC } +// Six of these eleven keys are read only when the key is present, so for those the declared value is also +// what an absent key resolves to today. The six durations are declared as durations and written into a +// file as text, which is the shape the reader parses back. +func grpcDefaults(mode registry.Mode) any { return forMode(mode).GRPC } // stateSyncDefaults is what the snapshot settings resolve to for a node that has written nothing. // // All three keys are read with a casting getter and no presence check, and the retention is the one that // inverts: it is declared as keeping two snapshots and an absent key casts to zero, which the file format // documents as keeping every snapshot. -func stateSyncDefaults(registry.Mode) any { return srvconfig.DefaultConfig().StateSync } +func stateSyncDefaults(mode registry.Mode) any { return forMode(mode).StateSync } // telemetrySchema declares the keys the metric settings reader resolves. // @@ -115,8 +132,8 @@ type telemetrySchema struct { // The label set is empty, which is what the upstream default holds, so there is nothing to convert into // the untyped rows the reader takes. A test holds that emptiness, because a default that gained rows would // need converting and would otherwise reach the reader as the shape it refuses. -func telemetryDefaults(registry.Mode) any { - live := srvconfig.DefaultConfig().Telemetry +func telemetryDefaults(mode registry.Mode) any { + live := forMode(mode).Telemetry return telemetrySchema{ ServiceName: live.ServiceName, Enabled: live.Enabled, diff --git a/config/cosmosbase/cosmosbase_test.go b/config/cosmosbase/cosmosbase_test.go index 1220db25bd..baf2bef75f 100644 --- a/config/cosmosbase/cosmosbase_test.go +++ b/config/cosmosbase/cosmosbase_test.go @@ -5,6 +5,7 @@ import ( "sort" "testing" + "github.com/sei-protocol/sei-chain/app/params" "github.com/sei-protocol/sei-chain/config/registry" "github.com/sei-protocol/sei-chain/sei-cosmos/baseapp" "github.com/sei-protocol/sei-chain/sei-cosmos/server" @@ -181,14 +182,54 @@ func TestTheLabelSetIsRefusedFromTheEnvironment(t *testing.T) { } } -// TestDefaultsAreTheUpstreamOnesForEveryMode covers the value side of all five registrations. +// TestEachKindOfNodeResolvesTheInterfacesItIsFor is the mode-varying part of these sections. // -// Unchanged by mode, which is the decision worth pinning. seid init writes three of these keys per mode, -// so a node it provisioned carries them as written values; these are what a node with nothing written -// runs. -func TestDefaultsAreTheUpstreamOnesForEveryMode(t *testing.T) { +// Three settings differ by kind of node, and the values are written out here rather than taken from the +// same rules the sections read, so a change to those rules fails this and gets looked at. Each matters in a +// different direction. A node that serves queries needs the two interfaces that serve them, and declaring +// them closed would take a service away from one. A validator is meant to expose as little as it can, and +// declaring gRPC open would state the opposite of that on every validator. And how many blocks a node +// keeps is a decision about its disk. +func TestEachKindOfNodeResolvesTheInterfacesItIsFor(t *testing.T) { + byMode := map[registry.Mode]struct { + api, grpc bool + retain uint64 + }{ + registry.ModeValidator: {api: false, grpc: false, retain: 0}, + registry.ModeSeed: {api: false, grpc: false, retain: 0}, + registry.ModeFull: {api: true, grpc: true, retain: 100000}, + registry.ModeArchive: {api: true, grpc: true, retain: 0}, + } + for _, mode := range registry.Modes() { + want, named := byMode[mode] + if !named { + t.Fatalf("mode %q has no expectation here, so a mode was added and this was not revisited", mode) + } + resolved, err := registry.Resolve(mode, registry.Sources{}) + if err != nil { + t.Fatalf("mode %q: %v", mode, err) + } + for key, expected := range map[string]any{ + "api.enable": want.api, + "grpc.enable": want.grpc, + "min-retain-blocks": want.retain, + } { + if got := resolved.Values[key]; !reflect.DeepEqual(got, expected) { + t.Errorf("mode %q: %s resolves to %#v, want %#v", mode, key, got, expected) + } + } + } +} + +// TestDefaultsAreTheUpstreamOnesApartFromTheModeRules covers everything a mode does not change. +// +// Compared against the upstream defaults with the same mode rules applied, so this holds the sections to +// carrying the whole of that configuration rather than a subset of it, and the three settings the rules +// touch are pinned by name above. +func TestDefaultsAreTheUpstreamOnesApartFromTheModeRules(t *testing.T) { for _, mode := range registry.Modes() { live := srvconfig.DefaultConfig() + params.SetAppConfigByMode(live, params.NodeMode(mode)) for _, c := range []struct { section string got any @@ -200,7 +241,8 @@ func TestDefaultsAreTheUpstreamOnesForEveryMode(t *testing.T) { {StateSyncSectionName, stateSyncDefaults(mode), live.StateSync}, } { if !reflect.DeepEqual(c.got, c.want) { - t.Errorf("mode %q: %s resolves to something other than the upstream default", mode, c.section) + t.Errorf("mode %q: %s resolves to something other than that mode's upstream configuration", + mode, c.section) } } From db12097f433b7e316ac8ab2a733dc6b31a26e09b Mon Sep 17 00:00:00 2001 From: bdchatham Date: Fri, 21 Aug 2026 11:27:46 -0700 Subject: [PATCH 3/7] config: refuse a mode nothing declares, and report a variable that did nothing A resolution answered for any string. A section's defaults answer per mode, and a mode this package does not know reached whatever each section does with an argument it cannot match, which for these five is the rules answering as though it were a full node. So an empty string, a capitalised name, or one with a trailing space resolved the interfaces a full node serves onto whichever node asked, with no error. It is refused now, naming the four. A refusal of the environment channel is recorded by a key, so a slip in the spelling named a key no section declares. The channel would never have offered that key, so the refusal covered nothing while reading as though it did, and the key it was written for went on resolving from a variable. Both sets exist for the first time when something resolves, because a refusal may be recorded before the section declaring its key registers, so that is where they are compared. The reason a refusal carries is required and had no consumer. The channel was skipped before the variable was read, so the one fact a diagnostic needs, that an operator set it, was discarded at the cheapest possible point. The variable is read now and its value still thrown away, and the key comes back named, so a required reason is one somebody can be told. A refused key nobody set is not reported, because a value nobody chose is not news. A refusal also names the section that declares the key, so a refused key is attributable the way every other defect is. It was putting the key where the section belongs, which made a defect read as though a key had registered and made a scoped sweep skip it. Four of the metric section's seven hand-copied values were held against nothing. That is the one section here that has to restate its values, so it is the one where a field can be assigned from its neighbour, and assigning the hostname toggle from the enabled toggle survived the suite. Every one of the seven is now held as the key it resolves to rather than as a struct field, because a struct compared with itself agrees while two values sit on the wrong fields. Five comments said things the code does not. The node-wide settings claimed to be unchanged by mode while one of their own keys answers per mode. A count of non-zero defaults was wrong. Two different counts of six read as one, and the pair the sentence lost is read through a clamp that does nothing for an absent key. The package's reason for existing named a vendored tree, when other sections register inside one and the real obstacle is an import edge. And a paragraph named two sections that belong to another change. --- config/cosmosbase/cosmosbase.go | 62 +++++++------- config/cosmosbase/cosmosbase_test.go | 64 ++++++++++---- config/registry/environment.go | 8 +- config/registry/resolve.go | 55 +++++++++++- config/registry/rootkeys_test.go | 120 ++++++++++++++++++++++++++- 5 files changed, 254 insertions(+), 55 deletions(-) diff --git a/config/cosmosbase/cosmosbase.go b/config/cosmosbase/cosmosbase.go index ad64b5eff7..7def33ed90 100644 --- a/config/cosmosbase/cosmosbase.go +++ b/config/cosmosbase/cosmosbase.go @@ -1,9 +1,12 @@ // Package cosmosbase registers the configuration sections whose keys belong to the Cosmos server. // -// These sections have no owning package inside this repository. Their structs and their readers live in -// sei-cosmos, which this repository vendors rather than authors, so there is nowhere upstream to put a -// registration that this repository's registry would see. A section belongs here only when its keys are -// upstream's; a section this repository owns registers in the package that owns its struct. +// These five register here rather than beside the structs they describe, and the reason is an import edge. +// The mode rules their defaults answer through live in app/params, which imports the upstream server +// configuration, so that package cannot ask for them without a cycle. A vendored tree is not itself the +// obstacle: other sections do register inside one. +// +// A section belongs here only when its keys are upstream's and that edge is in the way. Everything else +// registers in the package that owns its struct, so the struct, the values and the keys stay together. package cosmosbase import ( @@ -24,14 +27,13 @@ const ( StateSyncSectionName = "state-sync" ) -// GlobalLabelsKey is the metric label set, which is the one key here no environment variable can supply. -const GlobalLabelsKey = TelemetrySectionName + ".global-labels" +// globalLabelsKey is the metric label set, which is the one key here no environment variable can supply. +const globalLabelsKey = TelemetrySectionName + ".global-labels" // Registration puts the upstream server's configuration sections in the registry. // // Four of the five register the upstream struct directly, because their mapstructure tags already name the -// keys their reader resolves. That is worth stating rather than assuming: the two SeiDB sections needed a -// schema precisely because their tags name something else. +// keys their reader resolves. func init() { registry.RegisterRootKeys(BaseSectionName, &srvconfig.BaseConfig{}, baseDefaults) registry.RegisterSection(APISectionName, &srvconfig.APIConfig{}, apiDefaults) @@ -39,7 +41,7 @@ func init() { registry.RegisterSection(TelemetrySectionName, &telemetrySchema{}, telemetryDefaults) registry.RegisterSection(StateSyncSectionName, &srvconfig.StateSyncConfig{}, stateSyncDefaults) - registry.RefuseFromEnvironment(GlobalLabelsKey, + registry.RefuseFromEnvironment(TelemetrySectionName, globalLabelsKey, "the metric label set is a list of name and value rows, and its reader takes that exact shape "+ "rather than casting what it finds, so no single environment string can supply it. Write it "+ "in the configuration file instead") @@ -60,23 +62,24 @@ func forMode(mode registry.Mode) *srvconfig.Config { return out } -// baseDefaults is what the node-wide settings resolve to for a node that has written nothing. -// -// The upstream defaults, unchanged by mode. Every one of these keys is read with a casting getter and no -// check that the key was present, so an absent key casts to a zero and clobbers the default beside it. -// Five of the fourteen have a non-zero default, and the pruning strategy is the one that matters, because -// an empty strategy is not a strategy. -// -// Three keys elsewhere in this package vary by node mode, and none of them varies here. seid init writes -// the interface toggles and the block retention per mode, so a node it provisioned carries those as -// written values, and a written value is what resolves. These are what a node with nothing written runs. -// -// One value here is not what a running node uses today, and it is worth knowing which. The pruning -// strategy is declared as keeping everything, while the command line registers a flag of the same name -// defaulting to the standard strategy, and a bound flag is a source of its own below the file. So a node -// started with no pruning key written prunes on the standard schedule and this states that it would keep -// everything. Whoever resolves for a running node has to supply the flag values to get the answer that -// node uses. +// baseDefaults is what the node-wide settings resolve to for a node of this kind. +// +// One of these keys answers per mode: how many blocks a node retains, which is a hundred thousand for a +// full node and everything for the rest. The other two mode-varying keys in this package are the interface +// toggles, which belong to the sections that own them. +// +// Every one of these keys is read with a casting getter and no check that the key was present, so an +// absent key casts to a zero and clobbers the default beside it. Which keys those are, and what a node +// resolves for each instead, belongs in a measurement rather than in a count here. +// +// Several of these are not what a running node resolves today, and the causes differ: a bound command flag +// of the same name carries its own default below the file, and the command that assembles the server +// configuration overrides some of them before a node starts. The pruning strategy is the one worth naming, +// because the flag defaults it to the standard schedule while this declares it keeps everything. +// +// A caller resolving for a running node therefore has to supply that node's flag values, and only the ones +// an operator actually set. A flag nobody typed still reports a default, and this resolution ranks flags +// above the file, so passing defaults would put every one of them over an operator's own value. func baseDefaults(mode registry.Mode) any { return forMode(mode).BaseConfig } // apiDefaults is what the REST interface settings resolve to for a node that has written nothing. @@ -91,9 +94,10 @@ func apiDefaults(mode registry.Mode) any { return forMode(mode).API } // interface follows and for the same reason. The upstream default is on for every kind, so declaring that // would state an open interface on the nodes meant to expose the least. // -// Six of these eleven keys are read only when the key is present, so for those the declared value is also -// what an absent key resolves to today. The six durations are declared as durations and written into a -// file as text, which is the shape the reader parses back. +// Six of these eleven keys are read only when the key is present. Two more are durations read through a +// clamp that rescues a negative value and does nothing for an absent one, so those two are unguarded and +// their clobber leaves no trace. The durations are declared as durations and written into a file as text, +// which is the shape the reader parses back. func grpcDefaults(mode registry.Mode) any { return forMode(mode).GRPC } // stateSyncDefaults is what the snapshot settings resolve to for a node that has written nothing. diff --git a/config/cosmosbase/cosmosbase_test.go b/config/cosmosbase/cosmosbase_test.go index baf2bef75f..e038e15435 100644 --- a/config/cosmosbase/cosmosbase_test.go +++ b/config/cosmosbase/cosmosbase_test.go @@ -86,7 +86,7 @@ func TestTheMetricKeysAreTheOnesItsReaderResolves(t *testing.T) { requireDeclares(t, TelemetrySectionName, []string{ "telemetry.service-name", "telemetry.enabled", "telemetry.enable-hostname", "telemetry.enable-hostname-label", "telemetry.enable-service-label", - "telemetry.prometheus-retention-time", GlobalLabelsKey, + "telemetry.prometheus-retention-time", globalLabelsKey, }) } @@ -150,10 +150,10 @@ func TestTheUpstreamDefaultCarriesNoLabels(t *testing.T) { // variable installs a value the reader refuses, and it refuses in the first statement of the whole server // configuration. The node stops. Leaving the channel out means the file's value applies and the node runs. func TestTheLabelSetIsRefusedFromTheEnvironment(t *testing.T) { - reason, refused := registry.EnvCannotDeliver()[GlobalLabelsKey] + reason, refused := registry.EnvCannotDeliver()[globalLabelsKey] if !refused { t.Fatalf("%s is not refused from the environment, so a variable naming it resolves to a string "+ - "and installing that stops the node", GlobalLabelsKey) + "and installing that stops the node", globalLabelsKey) } if reason == "" { t.Error("the refusal carries no reason, so an operator whose variable is ignored cannot be told why") @@ -161,7 +161,7 @@ func TestTheLabelSetIsRefusedFromTheEnvironment(t *testing.T) { resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{ LookupEnv: func(name string) (string, bool) { - if name == registry.EnvName(GlobalLabelsKey) { + if name == registry.EnvName(globalLabelsKey) { return "chain_id=pacific-1", true } return "", false @@ -170,14 +170,14 @@ func TestTheLabelSetIsRefusedFromTheEnvironment(t *testing.T) { if err != nil { t.Fatalf("Resolve: %v", err) } - if got := resolved.Values[GlobalLabelsKey]; !reflect.DeepEqual(got, []any{}) { + if got := resolved.Values[globalLabelsKey]; !reflect.DeepEqual(got, []any{}) { t.Errorf("%s resolved to %#v (%T), want the declared default it was left to", - GlobalLabelsKey, got, got) + globalLabelsKey, got, got) } for _, key := range resolved.Overrides { - if key == GlobalLabelsKey { + if key == globalLabelsKey { t.Errorf("%s is reported as a value an operator supplied, and the variable did nothing", - GlobalLabelsKey) + globalLabelsKey) } } } @@ -246,21 +246,51 @@ func TestDefaultsAreTheUpstreamOnesApartFromTheModeRules(t *testing.T) { } } - metrics, ok := telemetryDefaults(mode).(telemetrySchema) - if !ok { + if _, ok := telemetryDefaults(mode).(telemetrySchema); !ok { t.Fatalf("mode %q: the metric defaults returned %T, want the schema", mode, telemetryDefaults(mode)) } - if metrics.Enabled != live.Telemetry.Enabled || - metrics.PrometheusRetentionTime != live.Telemetry.PrometheusRetentionTime || - metrics.ServiceName != live.Telemetry.ServiceName { - t.Errorf("mode %q: the metric defaults are not the upstream ones: %+v", mode, metrics) + // Every field the schema copies by hand, held against the upstream value, and held as the + // resolved key rather than as a struct field. The section that has to restate its values is the + // one where a field can be assigned from the wrong neighbour, and a struct comparison would not + // see it: each field still holds a value, and the count still matches. + requireResolvesTelemetry(t, mode, live.Telemetry) + } +} + +// requireResolvesTelemetry holds every key the metric schema declares against the upstream value. +func requireResolvesTelemetry(t *testing.T, mode registry.Mode, live telemetry.Config) { + t.Helper() + resolved, err := registry.Resolve(mode, registry.Sources{}) + if err != nil { + t.Fatalf("mode %q: %v", mode, err) + } + for key, want := range map[string]any{ + "telemetry.service-name": live.ServiceName, + "telemetry.enabled": live.Enabled, + "telemetry.enable-hostname": live.EnableHostname, + "telemetry.enable-hostname-label": live.EnableHostnameLabel, + "telemetry.enable-service-label": live.EnableServiceLabel, + "telemetry.prometheus-retention-time": live.PrometheusRetentionTime, + globalLabelsKey: []any{}, + } { + if got := resolved.Values[key]; !reflect.DeepEqual(got, want) { + t.Errorf("mode %q: %s resolves to %#v (%T), want %#v (%T)", mode, key, got, got, want, want) } } } -// TestEverySectionHereRegistersCleanly covers what the registry itself refuses. -func TestEverySectionHereRegistersCleanly(t *testing.T) { +// TestTheSectionsThisPackageRegistersAreUsable covers what the registry refuses. +// +// Scoped to the five names this file registers. A refusal that depends on what else has registered is +// not this package's to answer for, and the sweep that covers it belongs where every section is linked. +func TestTheSectionsThisPackageRegistersAreUsable(t *testing.T) { + mine := map[string]bool{ + BaseSectionName: true, APISectionName: true, GRPCSectionName: true, + TelemetrySectionName: true, StateSyncSectionName: true, + } for _, defect := range registry.Defects() { - t.Errorf("%s is registered and defective: %v", defect.Section, defect.Err) + if mine[defect.Section] { + t.Errorf("%s was refused, so none of its keys is declared: %v", defect.Section, defect.Err) + } } } diff --git a/config/registry/environment.go b/config/registry/environment.go index ef2e013ed3..d3f440b178 100644 --- a/config/registry/environment.go +++ b/config/registry/environment.go @@ -17,12 +17,16 @@ var envCannotDeliver = map[string]string{} // start, so the difference is recorded rather than assumed. A value silently doing nothing is the failure // this whole surface exists to remove, which is why the reason is required and not optional. // +// section is the section that declares the key, so a refused key is attributable to a registration the +// way every other defect is. Whether the key is one that section declares is answered when something +// resolves, because a refusal may be recorded before the registration it belongs to. +// // Called from the owning package, beside its registration, so the reason sits with the code that knows it. -func RefuseFromEnvironment(key, reason string) { +func RefuseFromEnvironment(section, key, reason string) { mu.Lock() defer mu.Unlock() if reason == "" { - defects = append(defects, Defect{Section: key, Err: fmt.Errorf( + defects = append(defects, Defect{Section: section, Err: fmt.Errorf( "refusing %q from the environment with no reason; an operator whose variable is ignored has "+ "to be told why", key)}) return diff --git a/config/registry/resolve.go b/config/registry/resolve.go index c47281a293..5ea1c2bbbe 100644 --- a/config/registry/resolve.go +++ b/config/registry/resolve.go @@ -16,6 +16,12 @@ type Resolved struct { // The keys an operator has taken responsibility for, as distinct from the ones tracking the // binary's judgement. This is what a diff renders. Overrides []string + // Ignored are declared keys an environment variable was set for and could not supply, sorted. + // + // Separate from Unknown because the two are different mistakes. An unknown key is one nothing reads. + // An ignored one is read, and the operator reached for the one channel that cannot carry it, so the + // value they wrote elsewhere is what applies. EnvCannotDeliver says why, per key. + Ignored []string // Unknown are keys a source carried that no section declares, sorted. // // Reported rather than an error, because what to do about one is the caller's decision: a @@ -37,6 +43,16 @@ type Sources struct { Flags map[string]any } +// known reports whether this package declares defaults for a mode. +func known(mode Mode) bool { + for _, m := range Modes() { + if m == mode { + return true + } + } + return false +} + // Resolve reduces a node's configuration sources to one value per declared key. // // The precedence is stated once, in this function, and a caller cannot reorder its way to a different @@ -54,6 +70,16 @@ type Sources struct { func Resolve(mode Mode, from Sources) (Resolved, error) { var out Resolved + // Refused before anything is resolved, because a section's defaults answer per mode and a mode this + // package does not know reaches whatever each section does with an argument it cannot match. What that + // is varies by section and none of them is a decision anyone made: the upstream mode rules answer for + // an unrecognised mode as though it were a full node, so an empty string, a capitalised name or one + // with a trailing space resolves the interfaces a full node serves onto whatever asked. + if !known(mode) { + return out, fmt.Errorf("%q is not a mode this binary declares defaults for; the modes are %v", + mode, Modes()) + } + // One snapshot, read once and passed everywhere below. Every part of the answer has to describe the // same registry: asking again leaves a window a concurrent registration fits through, and a section // arriving in that window is declared by one part of the answer and not by another. @@ -64,6 +90,16 @@ func Resolve(mode Mode, from Sources) (Resolved, error) { } declared := declaredKeys(registered) undeliverable := EnvCannotDeliver() + // A refusal is recorded by a key, and a key that no section declares is one the environment layer + // would never have offered anyway, so the refusal protects nothing and reads as though it did. Held + // here because a refusal may be recorded before the section that declares its key registers, so this + // is the first point both sets exist. + for key := range undeliverable { + if !declared[key] { + return out, fmt.Errorf("%q is refused from the environment and no section declares it, so the "+ + "refusal covers nothing", key) + } + } out.Values = make(map[string]any, len(declared)) for key, v := range defaults { @@ -74,9 +110,11 @@ func Resolve(mode Mode, from Sources) (Resolved, error) { unknown := map[string]bool{} // Lowest precedence first, so a later source overwrites an earlier one. The one statement of the // order, which is why nothing exports it. + fromEnv, ignored := envValues(declared, undeliverable, from.LookupEnv) + out.Ignored = ignored for _, values := range []map[string]any{ fileValues(from.File), - envValues(declared, undeliverable, from.LookupEnv), + fromEnv, from.Flags, } { for key, v := range values { @@ -274,16 +312,24 @@ func walkValues(v reflect.Value, prefix string, out map[string]any) error { // again would ask for a key the caller's declared set does not hold, and the answer would come back // only to be reported as one no section declares. func envValues(declared map[string]bool, undeliverable map[string]string, - lookup func(string) (string, bool)) map[string]any { + lookup func(string) (string, bool)) (map[string]any, []string) { if lookup == nil { - return nil + return nil, nil } out := map[string]any{} + var ignored []string for key := range declared { // A key no variable can carry is left to the sources that can. Resolving it would put a string // at the top of the order for a reader that takes the exact type, and installing that stops the // node. What an operator loses is the channel; what they keep is a node that boots. + // + // The variable is still read, and the value still discarded. Asking is what turns this from a + // silent skip into something a caller can report: a reason nothing can attach to an operator's + // own action is a reason nobody is ever told. if _, refused := undeliverable[key]; refused { + if v, set := lookup(EnvName(key)); set && v != "" { + ignored = append(ignored, key) + } continue } // An empty value is treated as unset. A variable exported empty is far more often a shell @@ -294,7 +340,8 @@ func envValues(declared map[string]bool, undeliverable map[string]string, out[key] = v } } - return out + sort.Strings(ignored) + return out, ignored } // fileValues normalises a configuration file's keys to lower case. diff --git a/config/registry/rootkeys_test.go b/config/registry/rootkeys_test.go index 6bcc1a2a41..a8c4083c87 100644 --- a/config/registry/rootkeys_test.go +++ b/config/registry/rootkeys_test.go @@ -153,7 +153,7 @@ func TestAKeyTheEnvironmentCannotDeliverIsLeftToTheOtherSources(t *testing.T) { Plain string `mapstructure:"plain"` }{Rows: []any{}, Plain: "from the default"} }) - registry.RefuseFromEnvironment("probe.rows", "its reader takes the exact type rather than casting") + registry.RefuseFromEnvironment("probe", "probe.rows", "its reader takes the exact type rather than casting") for _, d := range registry.Defects() { t.Fatalf("the registration was refused: %v", d.Err) } @@ -194,7 +194,7 @@ func TestAKeyTheEnvironmentCannotDeliverIsLeftToTheOtherSources(t *testing.T) { // has to be told why. A refusal with no reason gives a diagnostic nothing to print. func TestRefusingAChannelWithoutAReasonIsItselfRefused(t *testing.T) { registry.Reset() - registry.RefuseFromEnvironment("probe.rows", "") + registry.RefuseFromEnvironment("probe", "probe.rows", "") if len(registry.Defects()) != 1 { t.Fatalf("recorded %d defects, want one naming the key with no reason", len(registry.Defects())) } @@ -204,8 +204,122 @@ func TestRefusingAChannelWithoutAReasonIsItselfRefused(t *testing.T) { } registry.Reset() - registry.RefuseFromEnvironment("probe.rows", "its reader takes the exact type") + registry.RefuseFromEnvironment("probe", "probe.rows", "its reader takes the exact type") if _, refused := registry.EnvCannotDeliver()["probe.rows"]; !refused { t.Error("a refusal carrying a reason was not recorded") } } + +// TestAModeThisBinaryDoesNotDeclareIsRefused closes a resolution that answered for anything. +// +// A section's defaults answer per mode, and a mode this package does not know reaches whatever each +// section does with an argument it cannot match. Nothing about that is a decision anyone made: the mode +// rules these sections read answer for an unrecognised mode as though it were a full node, so an empty +// string, a capitalised name or one with a trailing space resolved the interfaces a full node serves onto +// whichever node asked. +func TestAModeThisBinaryDoesNotDeclareIsRefused(t *testing.T) { + registry.Reset() + registry.RegisterSection("probe", &struct { + Serves bool `mapstructure:"serves"` + }{}, func(mode registry.Mode) any { + return struct { + Serves bool `mapstructure:"serves"` + }{Serves: mode == registry.ModeFull || mode == registry.ModeArchive} + }) + for _, d := range registry.Defects() { + t.Fatalf("the probe was refused: %v", d.Err) + } + + for _, mode := range registry.Modes() { + if _, err := registry.Resolve(mode, registry.Sources{}); err != nil { + t.Errorf("mode %q is declared and did not resolve: %v", mode, err) + } + } + for _, mode := range []registry.Mode{"", "Validator", "validator ", "VALIDATOR", "sentry"} { + resolved, err := registry.Resolve(mode, registry.Sources{}) + if err == nil { + t.Errorf("mode %q resolved, to serves=%v. A mode nothing declares has no answer, and the one "+ + "it reached is whatever the rules do with an argument they cannot match", + mode, resolved.Values["probe.serves"]) + } + } +} + +// TestARefusalNamingAKeyNothingDeclaresIsRefused keeps a refusal from covering nothing. +// +// A refusal is recorded by a key, so a slip in the spelling names a key no section declares. The +// environment layer would never have offered that key, so the refusal protects nothing while reading as +// though it did, and the key it was meant to cover resolves from the environment as before. +// +// Answered when something resolves rather than when the refusal is recorded, because a refusal may be +// recorded before the section declaring its key registers. Resolving is the first point both sets exist. +func TestARefusalNamingAKeyNothingDeclaresIsRefused(t *testing.T) { + registry.Reset() + registry.RegisterSection("probe", &struct { + Rows []any `mapstructure:"rows"` + }{}, func(registry.Mode) any { + return struct { + Rows []any `mapstructure:"rows"` + }{Rows: []any{}} + }) + registry.RefuseFromEnvironment("probe", "probe.rowz", "a slip in the spelling") + + if _, err := registry.Resolve(registry.ModeFull, registry.Sources{}); err == nil { + t.Error("a refusal naming a key nothing declares was accepted, so it covers nothing and the key " + + "it was written for still resolves from the environment") + } +} + +// TestAVariableSetForARefusedKeyIsReported is what makes the required reason worth requiring. +// +// The channel is skipped and the value discarded, which is the point. But an operator who set the variable +// believes otherwise, and a reason nothing can attach to their own action is a reason nobody is told. So +// the variable is still read, and the key comes back named. +func TestAVariableSetForARefusedKeyIsReported(t *testing.T) { + registry.Reset() + registry.RegisterSection("probe", &struct { + Rows []any `mapstructure:"rows"` + Plain string `mapstructure:"plain"` + }{}, func(registry.Mode) any { + return struct { + Rows []any `mapstructure:"rows"` + Plain string `mapstructure:"plain"` + }{Rows: []any{}, Plain: "from the default"} + }) + registry.RefuseFromEnvironment("probe", "probe.rows", "its reader takes the exact type") + for _, d := range registry.Defects() { + t.Fatalf("the probe was refused: %v", d.Err) + } + + resolved, err := registry.Resolve(registry.ModeFull, registry.Sources{ + LookupEnv: func(name string) (string, bool) { + if name == registry.EnvName("probe.rows") { + return "chain_id=pacific-1", true + } + return "", false + }, + }) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + + if got := strings.Join(resolved.Ignored, ","); got != "probe.rows" { + t.Errorf("the ignored variables are %q, want probe.rows. An operator set it and nothing here can "+ + "tell them it did nothing", got) + } + if !reflect.DeepEqual(resolved.Values["probe.rows"], []any{}) { + t.Errorf("probe.rows resolved to %#v, and the channel was supposed to be skipped", + resolved.Values["probe.rows"]) + } + + // A refused key nobody set is not news, so it is not reported. + quiet, err := registry.Resolve(registry.ModeFull, registry.Sources{ + LookupEnv: func(string) (string, bool) { return "", false }, + }) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if len(quiet.Ignored) != 0 { + t.Errorf("a refused key nobody set is reported as ignored: %v", quiet.Ignored) + } +} From eb53ea6e3ba2599b14793cc9bbb9bfcda969e208 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Sat, 22 Aug 2026 16:57:24 -0700 Subject: [PATCH 4/7] config: the contract says what the registry now does Four statements in the package contract described the previous shape. A second entry point exists, for the settings written at the top of a file rather than inside a table, and the contract showed one. The list of what makes a registration unusable no longer enumerated: two sections declaring one key and a top-level key sharing a section's name both became possible once a key could sit at the root, and a refusal of the environment carrying no reason is refused too. The resolution order had gained a per-key hole in one channel and did not say so. And the first step of adding a section told an author to use the name as the first segment of every key, which is false for a section whose keys have none. A mode this package does not declare is also refused now, and the contract says that where it says a default answers per mode. --- config/registry/doc.go | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/config/registry/doc.go b/config/registry/doc.go index d1955c08f3..ed2177d788 100644 --- a/config/registry/doc.go +++ b/config/registry/doc.go @@ -36,7 +36,13 @@ // serve. // // The third argument answers per node mode, because a validator and a seed node do not default -// alike. +// alike. A mode this package does not declare is refused rather than answered for: what a section +// does with an argument it cannot match is not a decision anybody made. +// +// Some settings are node-wide and are written at the top of a file rather than inside a table. +// RegisterRootKeys declares those: the name it takes is what a lookup and a report are keyed by and is +// not part of any key, so the keys are the tags alone. Giving such a section a segment would rename +// every key it declares, and a renamed key is one an operator's existing file no longer reaches. // // # Defaults // @@ -56,6 +62,14 @@ // value and a default are otherwise indistinguishable once merged. The second is why a typo in an // operator's file is visible rather than silently dropped. // +// One channel has a per-key hole. A reader that takes its value's exact type cannot be handed the one +// string an environment carries, so a section may refuse that channel for such a key, and the file's +// value applies instead of a value that would stop the node. The variable is still read and its value +// still discarded, and the key is reported as ignored, because a channel that quietly does nothing is +// the failure this package exists to remove. A refusal carries the reason an operator is owed, and one +// naming a key no section declares is refused in turn: it would cover nothing while reading as though +// it covered something. +// // Resolve either answers for every declared key or returns an error naming what it could not answer // for. A caller is never handed a resolution with a hole in it. // @@ -71,6 +85,18 @@ // tag, an unexported field carrying a tag, two fields declaring one path, a struct that declares no // key, a struct that contains itself, and two keys that collapse onto one environment variable. // +// Two more become possible once a key can sit at the top of a file, and neither could happen while +// every key carried its section's name. Two sections declaring one key have one default rendered over +// the other, and which one depends on the order the sections are walked. And a key at the top of the +// file that is also a section's name cannot be written at all, because a file holding both a value for +// that name and a table under it is not valid TOML, so one of the two is unreachable and nothing says +// which. Both are refused in either registration order, since registration order is not something an +// operator can see. +// +// Refusing the environment for a key is itself refused when it carries no reason. An operator told +// their variable does nothing has to be told why, and a refusal with nothing to print is worse than +// resolving the variable or leaving it alone. +// // A key segment is also refused if it is upper-case, or if it carries a dot or a space. That rule // holds for the section name and for a field's tag alike, since both become segments of the same // dotted key and answer to the same sources. @@ -85,7 +111,9 @@ // // # Adding a Section // -// 1. Give the section a name, and use it as the first segment of every key it declares. +// 1. Give the section a name, and use it as the first segment of every key it declares. A section +// whose settings sit at the top of the file instead declares root keys, and its name is then a +// handle for lookups and reports rather than part of any key. // 2. Register the struct the reader already uses, with a per-mode default. // 3. Assert the registration produced no Defect. // 4. Hold the derived key names against the reader, so a key that reaches nothing fails. From 02878d03e23113462e19338c361e6fbbede020d0 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Sat, 22 Aug 2026 17:08:29 -0700 Subject: [PATCH 5/7] config: drop a guard with no instance, and name the gap it leaves refuseOverlap refused two collisions and only one of them could happen. Two sections declaring one key is already refused by the environment check, which two identical keys reach by answering to one variable, so that arm was a second guard on a case already covered. The other arm, a key at the top of the file sharing a section's name, was the only one it alone caught, and it has no instance: one section declares keys at the top of the file and none of its fourteen names is a section's. So the code goes and the fact stays. The contract names the collision among the things this package does not guard, with what makes it reachable, because a second such section is where it starts to matter. The prototype found that out by hand: it named the section holding config.toml's top-level keys after the file rather than after the node, because the client file declares a top-level key called node and a node section could not have coexisted with it. The one case the removed guard described better is named better now where it is still refused. Two identical keys were being reported as two spellings of one environment variable, and the reason a dot and a hyphen are the same character to the environment is not the reason a key collides with itself. --- config/registry/doc.go | 16 +++---- config/registry/registry.go | 71 +++++--------------------------- config/registry/rootkeys_test.go | 60 +++------------------------ 3 files changed, 25 insertions(+), 122 deletions(-) diff --git a/config/registry/doc.go b/config/registry/doc.go index ed2177d788..bc1659ecec 100644 --- a/config/registry/doc.go +++ b/config/registry/doc.go @@ -85,13 +85,10 @@ // tag, an unexported field carrying a tag, two fields declaring one path, a struct that declares no // key, a struct that contains itself, and two keys that collapse onto one environment variable. // -// Two more become possible once a key can sit at the top of a file, and neither could happen while -// every key carried its section's name. Two sections declaring one key have one default rendered over -// the other, and which one depends on the order the sections are walked. And a key at the top of the -// file that is also a section's name cannot be written at all, because a file holding both a value for -// that name and a table under it is not valid TOML, so one of the two is unreachable and nothing says -// which. Both are refused in either registration order, since registration order is not something an -// operator can see. +// One more becomes possible once a key can sit at the top of a file, and it could not happen while every +// key carried its section's name: two sections declaring one key, where one default renders over the +// other and which one depends on the order the sections are walked. The environment check refuses it, +// because two identical keys answer to one variable. // // Refusing the environment for a key is itself refused when it carries no reason. An operator told // their variable does nothing has to be told why, and a refusal with nothing to print is worse than @@ -108,6 +105,11 @@ // - Not a file format. Nothing here reads or writes a configuration file. // - Not a validator. A section may state rules about its own values; this package invents none. // - Not wired. No section is registered by this package and no reader is migrated onto it. +// - Not a guard against a key and a table sharing one name. A key at the top of the file that is also +// a section's name cannot be written at all, because no file holds both a value for that name and a +// table under it, so one of the two settings is unreachable and nothing says which. One section +// declares keys at the top of the file today and none of its names is a section's, so the collision +// has no instance; a second such section is where it becomes reachable. // // # Adding a Section // diff --git a/config/registry/registry.go b/config/registry/registry.go index a9753d270d..06d472a15b 100644 --- a/config/registry/registry.go +++ b/config/registry/registry.go @@ -106,10 +106,6 @@ func record(name, prefix string, prototype any, defaults func(Mode) any) { defects = append(defects, Defect{Section: name, Err: fmt.Errorf("section registered twice")}) return } - if err := refuseOverlap(name, prefix, keys); err != nil { - defects = append(defects, Defect{Section: name, Err: err}) - return - } if err := envNamesAreDistinct(keys); err != nil { defects = append(defects, Defect{Section: name, Err: err}) return @@ -118,61 +114,6 @@ func record(name, prefix string, prototype any, defaults func(Mode) any) { } } -// refuseOverlap rejects a registration whose keys cannot coexist with what is already registered. -// Callers hold mu. -// -// Two shapes of overlap, and neither could happen while every key carried its section's name. A key two -// sections both declare has one default rendered over the other, and which one depends on the order the -// sections are walked. And a root key that is also a section's name cannot be written at all: a file -// holding both a value for that name and a table under it is not valid TOML, so one of the two is -// unreachable and nothing says which. -// -// The first shape reaches the environment check below as well, which would refuse it for the wrong -// reason: two spellings of one variable, when the keys are in fact the same key. This names it as itself. -func refuseOverlap(name, prefix string, keys []string) error { - declaredBy := map[string]string{} - sectionNamed := map[string]string{} - for _, s := range sections { - for _, key := range s.Keys { - declaredBy[key] = s.Name - } - if s.Prefix != "" { - sectionNamed[s.Prefix] = s.Name - } - } - - for _, key := range keys { - if owner, taken := declaredBy[key]; taken { - return fmt.Errorf("%s declares %q and so does %s; one default renders over the other and "+ - "which one wins depends on the order the sections are walked", name, key, owner) - } - if prefix != "" { - continue - } - if owner, taken := sectionNamed[key]; taken { - return fmt.Errorf("%s declares %q at the root of the file and %s is a section of that name; "+ - "a file cannot hold both a value for %q and a table under it, so one of them is "+ - "unreachable", name, key, owner, key) - } - } - - if prefix == "" { - return nil - } - for _, s := range sections { - if s.Prefix != "" { - continue - } - for _, key := range s.Keys { - if key == prefix { - return fmt.Errorf("%s is a section named %q and %s declares %q at the root of the file; "+ - "a file cannot hold both a table and a value under that name", name, prefix, s.Name, key) - } - } - } - return nil -} - // envNamesAreDistinct refuses keys that share one environment spelling. Callers hold mu. // // Dots and hyphens both become underscores, so two keys differing only in that punctuation answer to @@ -189,7 +130,17 @@ func envNamesAreDistinct(adding []string) error { } for _, key := range adding { env := EnvName(key) - if other, taken := spellings[env]; taken { + other, taken := spellings[env] + switch { + case taken && other == key: + // Two sections declaring one key, which a prefix made impossible and a key at the root of the + // file does not. One section's default renders over the other's and which one depends on the + // order the sections are walked, so the value a node runs is decided by nothing an operator + // or a reviewer can see. Named as the one key it is, because the spelling reason below is not + // the reason here. + return fmt.Errorf("%q is declared by two sections; one default renders over the other and "+ + "which one wins depends on the order the sections are walked", key) + case taken: return fmt.Errorf("%q and %q both answer to %s, because a dot and a hyphen are the same "+ "character to the environment, so one of them can never be set from it", other, key, env) } diff --git a/config/registry/rootkeys_test.go b/config/registry/rootkeys_test.go index a8c4083c87..d0a698d135 100644 --- a/config/registry/rootkeys_test.go +++ b/config/registry/rootkeys_test.go @@ -52,61 +52,11 @@ func TestARootSectionDeclaresKeysWithNoPrefix(t *testing.T) { } } -// TestARootKeyAndASectionCannotShareAName holds a limit of the file format, not a matter of taste. -// -// TOML cannot express a value for pruning and a table under pruning in one file, so one of the two is -// unwritable and which one an operator lost would depend on where in the file they wrote it. Registration -// order is not something an operator can see, so the refusal cannot depend on it either. -func TestARootKeyAndASectionCannotShareAName(t *testing.T) { - nested := func() (string, any, func(registry.Mode) any) { - return "pruning", &struct { - Mode string `mapstructure:"mode"` - }{}, func(registry.Mode) any { - return struct { - Mode string `mapstructure:"mode"` - }{Mode: "nothing"} - } - } - root := func() (string, any, func(registry.Mode) any) { - return "base", &struct { - Pruning string `mapstructure:"pruning"` - }{}, func(registry.Mode) any { - return struct { - Pruning string `mapstructure:"pruning"` - }{Pruning: "nothing"} - } - } - - t.Run("the section registers first", func(t *testing.T) { - registry.Reset() - registry.RegisterSection(nested()) - registry.RegisterRootKeys(root()) - if _, ok := registry.Lookup("base"); ok { - t.Error("the root section registered a key that is also a section name. A file cannot hold " + - "both, so one of them is unreachable and nothing says which") - } - if len(registry.Defects()) != 1 { - t.Errorf("recorded %d defects, want one naming the collision", len(registry.Defects())) - } - }) - - t.Run("the root key registers first", func(t *testing.T) { - registry.Reset() - registry.RegisterRootKeys(root()) - registry.RegisterSection(nested()) - if _, ok := registry.Lookup("pruning"); ok { - t.Error("a section registered under a name a root key already holds") - } - if len(registry.Defects()) != 1 { - t.Errorf("recorded %d defects, want one naming the collision", len(registry.Defects())) - } - }) -} - // TestTwoSectionsCannotDeclareTheSameKey was impossible while every key carried its section's name. // // Two prefixes cannot collide. Two root sections can, and the default rendered for such a key would be -// whichever section the walk reached last. +// whichever section the walk reached last. Refused by the environment check, which two identical keys +// reach by answering to one variable, and named as the one key it is rather than as two spellings. func TestTwoSectionsCannotDeclareTheSameKey(t *testing.T) { registry.Reset() same := func(name string) { @@ -130,9 +80,9 @@ func TestTwoSectionsCannotDeclareTheSameKey(t *testing.T) { if len(defects) != 1 { t.Fatalf("recorded %d defects, want one", len(defects)) } - // Named as one key two sections declare, rather than as two spellings of one variable, which is what - // the environment check would have called it. - if got := defects[0].Err.Error(); !strings.Contains(got, "and so does") { + // Named as one key two sections declare rather than as two spellings of one variable, which is the + // reason the same check gives for the collision it was written for. + if got := defects[0].Err.Error(); !strings.Contains(got, "is declared by two sections") { t.Errorf("the refusal reads %q, and an identical key is not an environment spelling collision", got) } } From 9b2d183a163b9dada421d4784aaf390a86ad79d1 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Sun, 23 Aug 2026 10:01:57 -0700 Subject: [PATCH 6/7] config: a declared value is what seid init writes, and the record measures where a node differs These sections said their values were what a node with nothing written resolves. They are not, and the difference was carried in four paragraphs of prose with one of the counts wrong. What they are is what seid init writes for a kind of node: the upstream defaults with the binary's own mode rules applied is exactly the pipeline that renders a generated app.toml, so a declared value is what that file would have held. That is a claim about a real pipeline in this binary rather than a judgement, so it can be held, and it is what a caller writing a configuration file wants. Where a node with nothing written resolves something else is now measured. The reader is driven with the start command's flags bound, the way a booting node binds them, because seventeen of these keys are also flags and a flag's registration default is what an absent key reaches before the lookup comes back empty. Twelve keys differ, and the measurement corrected the prose twice over: seven keys the paragraphs implied differ do not once the flags are bound, and the gRPC toggle does, which no paragraph named. Of the two interface toggles it is the only one that diverges, because its flag defaults the interface on while a generated validator file writes it off. A key that starts diverging fails, and so does one that stops, so guarding a read has to account for its row. Dropping the flag binding fails it too, which is what keeps the record measuring what a node gets rather than what the reader says in isolation. --- config/cosmosbase/agreement_test.go | 200 ++++++++++++++++++++++++++++ config/cosmosbase/cosmosbase.go | 33 ++--- 2 files changed, 217 insertions(+), 16 deletions(-) create mode 100644 config/cosmosbase/agreement_test.go diff --git a/config/cosmosbase/agreement_test.go b/config/cosmosbase/agreement_test.go new file mode 100644 index 0000000000..a13982ecca --- /dev/null +++ b/config/cosmosbase/agreement_test.go @@ -0,0 +1,200 @@ +package cosmosbase + +import ( + "fmt" + "sort" + "testing" + + "github.com/spf13/viper" + + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + srvconfig "github.com/sei-protocol/sei-chain/sei-cosmos/server/config" +) + +// whatANodeRunsToday is what each diverging key resolves to for a configuration carrying no keys. +// +// A declared value is what seid init writes for a kind of node. That is not what a node with nothing +// written resolves, and these are the keys where the two differ. Most are reads that take no account of +// whether the key was present, so an absent key casts to a zero and the default beside it is lost. +// +// 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 here is which keys disagree and what a node gets instead. +var whatANodeRunsToday = map[string]string{ + "api.address": "", + "api.max-open-connections": "0", + "api.rpc-max-body-bytes": "0", + "api.rpc-read-timeout": "0", + "api.swagger": "false", + "grpc.enable": "true", + "minimum-gas-prices": "", + "occ-enabled": "false", + "pruning": "default", + "pruning-keep-every": "", + "telemetry.enabled": "false", + "telemetry.prometheus-retention-time": "0", +} + +// whyItMatters says what a node gets today, for the keys where that is worth stating. +var whyItMatters = map[string]string{ + "pruning": "a command flag of this name carries the standard schedule below the file, so a node with " + + "nothing written prunes on that schedule where a generated file would have said keep everything", + "grpc.enable": "a command flag of this name defaults the interface on, so a validator with nothing " + + "written serves gRPC where a generated file would have written it off. This is the one interface " + + "toggle of the two that diverges; the REST one agrees", + "api.max-open-connections": "zero is unlimited, so the ceiling a generated file states is simply " + + "absent from a node that never wrote it, and the same holds for the body-size ceiling beside it", + "minimum-gas-prices": "an empty price refuses to start, so this key is one no running node can " + + "actually have unwritten", + "occ-enabled": "the transaction execution path, and no command flag carries it, so an absent key " + + "reads as off where a generated file says on", +} + +// readerValues is what a node resolves for a configuration carrying none of these keys. +// +// Driven through the reader rather than reasoned about, because the reader is the authority on what an +// absent key resolves to and its answer differs per key: some reads check that the key was present, most +// do not, and two are rescued by a clamp that does nothing for an absent value. +// +// The start command's flags are bound first, the way a booting node binds them, and that is what makes +// this the answer a node gets rather than the answer the reader gives in isolation. Seventeen of these keys +// are also command flags, so a flag's registration default is what an absent key reaches before the lookup +// comes back empty. Without the binding, a key like the gRPC toggle reads as its type's zero and the +// comparison would report agreement where a node disagrees. +// +// One key has to be supplied. The metric label set is the first thing the reader asks for and it refuses a +// configuration without it, so a reader handed nothing at all answers for no key at all. +func readerValues(t *testing.T) map[string]string { + t.Helper() + v := viper.New() + start := server.StartCmd(nil, t.TempDir(), nil) + if err := v.BindPFlags(start.Flags()); err != nil { + t.Fatalf("bind the start flags: %v", err) + } + v.Set(globalLabelsKey, []any{}) + cfg, err := srvconfig.GetConfig(v) + if err != nil { + t.Fatalf("the reader refused a configuration carrying only the label set: %v", err) + } + + return map[string]string{ + "minimum-gas-prices": fmt.Sprint(cfg.MinGasPrices), + "pruning": fmt.Sprint(cfg.Pruning), + "pruning-keep-recent": fmt.Sprint(cfg.PruningKeepRecent), + "pruning-keep-every": fmt.Sprint(cfg.PruningKeepEvery), + "pruning-interval": fmt.Sprint(cfg.PruningInterval), + "halt-height": fmt.Sprint(cfg.HaltHeight), + "halt-time": fmt.Sprint(cfg.HaltTime), + "freeze-height": fmt.Sprint(cfg.FreezeHeight), + "min-retain-blocks": fmt.Sprint(cfg.MinRetainBlocks), + "inter-block-cache": fmt.Sprint(cfg.InterBlockCache), + "compaction-interval": fmt.Sprint(cfg.CompactionInterval), + "concurrency-workers": fmt.Sprint(cfg.ConcurrencyWorkers), + "occ-enabled": fmt.Sprint(cfg.OccEnabled), + "api.enable": fmt.Sprint(cfg.API.Enable), + "api.swagger": fmt.Sprint(cfg.API.Swagger), + "api.address": fmt.Sprint(cfg.API.Address), + "api.enabled-unsafe-cors": fmt.Sprint(cfg.API.EnableUnsafeCORS), + "api.max-open-connections": fmt.Sprint(cfg.API.MaxOpenConnections), + "api.rpc-read-timeout": fmt.Sprint(cfg.API.RPCReadTimeout), + "api.rpc-write-timeout": fmt.Sprint(cfg.API.RPCWriteTimeout), + "api.rpc-max-body-bytes": fmt.Sprint(cfg.API.RPCMaxBodyBytes), + "grpc.enable": fmt.Sprint(cfg.GRPC.Enable), + "grpc.address": fmt.Sprint(cfg.GRPC.Address), + "grpc.max-recv-msg-size": fmt.Sprint(cfg.GRPC.MaxRecvMsgSize), + "grpc.max-open-connections": fmt.Sprint(cfg.GRPC.MaxOpenConnections), + "grpc.max-connection-idle": fmt.Sprint(cfg.GRPC.MaxConnectionIdle), + "grpc.max-connection-age": fmt.Sprint(cfg.GRPC.MaxConnectionAge), + "grpc.max-connection-age-grace": fmt.Sprint(cfg.GRPC.MaxConnectionAgeGrace), + "grpc.keepalive-time": fmt.Sprint(cfg.GRPC.KeepaliveTime), + "grpc.keepalive-timeout": fmt.Sprint(cfg.GRPC.KeepaliveTimeout), + "grpc.keepalive-min-time": fmt.Sprint(cfg.GRPC.KeepaliveMinTime), + "grpc.keepalive-permit-without-stream": fmt.Sprint(cfg.GRPC.KeepalivePermitWithoutStream), + "telemetry.service-name": fmt.Sprint(cfg.Telemetry.ServiceName), + "telemetry.enabled": fmt.Sprint(cfg.Telemetry.Enabled), + "telemetry.enable-hostname": fmt.Sprint(cfg.Telemetry.EnableHostname), + "telemetry.enable-hostname-label": fmt.Sprint(cfg.Telemetry.EnableHostnameLabel), + "telemetry.enable-service-label": fmt.Sprint(cfg.Telemetry.EnableServiceLabel), + "telemetry.prometheus-retention-time": fmt.Sprint(cfg.Telemetry.PrometheusRetentionTime), + "state-sync.snapshot-interval": fmt.Sprint(cfg.StateSync.SnapshotInterval), + "state-sync.snapshot-keep-recent": fmt.Sprint(cfg.StateSync.SnapshotKeepRecent), + "state-sync.snapshot-directory": fmt.Sprint(cfg.StateSync.SnapshotDirectory), + "index-events": fmt.Sprint(cfg.IndexEvents), + globalLabelsKey: fmt.Sprint(cfg.Telemetry.GlobalLabels), + } +} + +// TestTheDivergencesFromTheReaderAreTheRecordedOnes measures what a comment used to count. +// +// A declared value is what seid init writes for a kind of node, and for a good number of these keys that is +// not what a node with nothing written resolves. Which keys those are was carried in prose, in four +// paragraphs, and one of the counts was wrong. Prose cannot fail when it is wrong. +// +// So the set is measured. A key that starts diverging fails, and so does one that stops, which means +// guarding a read has to account for its row rather than quietly making a sentence stale. +// +// Run for the mode whose declared values match the reader's own mode-blind answer most closely, because +// the reader takes no mode and comparing every mode against it would report the mode rules as divergences. +// The mode-varying keys are held by name in the test beside this one. +func TestTheDivergencesFromTheReaderAreTheRecordedOnes(t *testing.T) { + reader := readerValues(t) + resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + + var measured []string + for key, got := range reader { + declared, declares := resolved.Values[key] + if !declares { + t.Errorf("%s is read by the upstream reader and no section here declares it", key) + continue + } + if fmt.Sprint(declared) == got { + if _, listed := whatANodeRunsToday[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 a generated file states differently from a node that "+ + "never wrote them", key, declared) + } + continue + } + measured = append(measured, key) + want, listed := whatANodeRunsToday[key] + switch { + case !listed: + t.Errorf("%s is declared as %v and a node with nothing written resolves %q, and nothing "+ + "records that. %s", key, declared, got, whyItMatters[key]) + case want != got: + t.Errorf("%s is recorded as resolving %q and resolves %q", key, want, got) + } + } + + sort.Strings(measured) + if len(measured) != len(whatANodeRunsToday) { + t.Errorf("measured %d divergences and %d are recorded: %v", + len(measured), len(whatANodeRunsToday), measured) + } +} + +// TestEveryKeyTheseSectionsDeclareIsOneTheReaderResolves holds the two lists against each other. +// +// The reader's side is written out above, which is a second statement of the same key set. It is the only +// statement available: this reader looks its keys up as inline strings rather than through constants, so +// there is nothing to compare a tag against. A key on one side only is either a setting an operator writes +// that no reader fills, or one the reader fills that no section here declares. +func TestEveryKeyTheseSectionsDeclareIsOneTheReaderResolves(t *testing.T) { + reader := readerValues(t) + for _, section := range []string{ + BaseSectionName, APISectionName, GRPCSectionName, TelemetrySectionName, StateSyncSectionName, + } { + registered, ok := registry.Lookup(section) + if !ok { + t.Fatalf("%s is not registered", section) + } + for _, key := range registered.Keys { + if _, filled := reader[key]; !filled { + t.Errorf("%s declares %s and no field above is paired with it", section, key) + } + } + } +} diff --git a/config/cosmosbase/cosmosbase.go b/config/cosmosbase/cosmosbase.go index 7def33ed90..e4588a4800 100644 --- a/config/cosmosbase/cosmosbase.go +++ b/config/cosmosbase/cosmosbase.go @@ -47,11 +47,17 @@ func init() { "in the configuration file instead") } -// forMode is the server configuration a node of this kind is meant to run. +// forMode is the server configuration seid init writes for a node of this kind. // -// The upstream defaults with the binary's own mode rules applied. Every section here answers through this, -// so a section states what a kind of node is meant to run rather than what the type holds before any mode -// is considered, and a rule added to those rules later moves these sections with nothing here changing. +// The upstream defaults with the binary's own mode rules applied, which is exactly the pipeline that +// produces a generated app.toml: seid init builds this and renders it through the template. So a declared +// value here is what that file would have held, and a caller writing a configuration file writes what the +// binary would have written. +// +// That is what a declared value states, and it is deliberately not what a node with nothing written +// resolves. Those differ for a good number of these keys, because most are read with no check that the key +// was present and several are bound to a command flag carrying its own default below the file. The set is +// measured rather than counted, in the agreement test beside this one. // // Three settings differ by mode today and each of them matters in a different direction. A node that // serves queries needs the interfaces that serve them; a validator is meant to expose as little as it can; @@ -72,23 +78,18 @@ func forMode(mode registry.Mode) *srvconfig.Config { // absent key casts to a zero and clobbers the default beside it. Which keys those are, and what a node // resolves for each instead, belongs in a measurement rather than in a count here. // -// Several of these are not what a running node resolves today, and the causes differ: a bound command flag -// of the same name carries its own default below the file, and the command that assembles the server -// configuration overrides some of them before a node starts. The pruning strategy is the one worth naming, -// because the flag defaults it to the standard schedule while this declares it keeps everything. -// -// A caller resolving for a running node therefore has to supply that node's flag values, and only the ones -// an operator actually set. A flag nobody typed still reports a default, and this resolution ranks flags -// above the file, so passing defaults would put every one of them over an operator's own value. +// A caller resolving for a running node has to supply that node's flag values, and only the ones an +// operator actually set. A flag nobody typed still reports a default, and this resolution ranks flags above +// the file, so passing defaults would put every one of them over an operator's own value. func baseDefaults(mode registry.Mode) any { return forMode(mode).BaseConfig } -// apiDefaults is what the REST interface settings resolve to for a node that has written nothing. +// apiDefaults is what the REST interface settings resolve to for a node of this kind. // // On for a full node and an archive node, off for a validator and a seed. Serving queries is what the // first two are for, and the second two are meant to expose as little as they can. func apiDefaults(mode registry.Mode) any { return forMode(mode).API } -// grpcDefaults is what the gRPC settings resolve to for a node that has written nothing. +// grpcDefaults is what the gRPC settings resolve to for a node of this kind. // // On for a full node and an archive node, off for a validator and a seed, which is the same rule the REST // interface follows and for the same reason. The upstream default is on for every kind, so declaring that @@ -100,7 +101,7 @@ func apiDefaults(mode registry.Mode) any { return forMode(mode).API } // which is the shape the reader parses back. func grpcDefaults(mode registry.Mode) any { return forMode(mode).GRPC } -// stateSyncDefaults is what the snapshot settings resolve to for a node that has written nothing. +// stateSyncDefaults is what the snapshot settings resolve to for a node of this kind. // // All three keys are read with a casting getter and no presence check, and the retention is the one that // inverts: it is declared as keeping two snapshots and an absent key casts to zero, which the file format @@ -128,7 +129,7 @@ type telemetrySchema struct { GlobalLabels []any `mapstructure:"global-labels"` } -// telemetryDefaults is what the metric settings resolve to for a node that has written nothing. +// telemetryDefaults is what the metric settings resolve to for a node of this kind. // // Read out of the upstream defaults rather than written again here, so a changed default moves both at // once and this states only which key carries which setting. From 3f9d618db415f951e48d0e9b4ec40478c3802703 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Sun, 23 Aug 2026 10:22:45 -0700 Subject: [PATCH 7/7] config: name which generator a declared value follows This binary writes an app.toml two ways and they disagree on four keys. The provisioning command applies the mode rules and renders the result; a node starting without a file runs a second pipeline that applies no mode rules at all and carries overrides of its own. So it writes the standard pruning strategy where the command writes keeping everything, a metric retention of sixty against seven thousand two hundred, the REST interface on for a validator against off, and a pruning interval drawn at random on every run. A declared value follows the command an operator runs to provision a node. That was already true and the comment said only that seid writes it, which is ninety per cent of a fact. --- config/cosmosbase/cosmosbase.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/config/cosmosbase/cosmosbase.go b/config/cosmosbase/cosmosbase.go index e4588a4800..cc38c75030 100644 --- a/config/cosmosbase/cosmosbase.go +++ b/config/cosmosbase/cosmosbase.go @@ -47,12 +47,18 @@ func init() { "in the configuration file instead") } -// forMode is the server configuration seid init writes for a node of this kind. -// -// The upstream defaults with the binary's own mode rules applied, which is exactly the pipeline that -// produces a generated app.toml: seid init builds this and renders it through the template. So a declared -// value here is what that file would have held, and a caller writing a configuration file writes what the -// binary would have written. +// forMode is the server configuration the seid init command writes for a node of this kind. +// +// The upstream defaults with the binary's own mode rules applied, which is the pipeline that command +// builds and renders through the template. So a declared value here is what that file would have held, and +// a caller writing a configuration file writes what that command would have written. +// +// Named by the command, because this binary generates a file two ways and they do not agree. A node +// starting without one gets a file from a second pipeline that applies no mode rules at all and carries +// overrides of its own, so it writes the standard pruning strategy where this writes keeping everything, +// a metric retention of sixty where this writes seven thousand two hundred, the REST interface on for a +// validator where this writes it off, and a pruning interval drawn at random each time it runs. This +// follows the command an operator runs to provision a node, not the file a node writes for itself. // // That is what a declared value states, and it is deliberately not what a node with nothing written // resolves. Those differ for a good number of these keys, because most are read with no check that the key