From 467ceec26edd5a3e068d5f33c3f52300b042bf23 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 20 Aug 2026 14:19:11 -0700 Subject: [PATCH 1/5] config: four app sections enter the registry Four sections owned by the app package, each registered where its struct lives. genesis 2 keys light_invariance 1 key state-commit 20 keys state-store 12 keys light_invariance registers the type its reader fills. The other three declare a schema, because keys derived from the type the reader fills are not the keys the reader looks up. The genesis import type carries no mapstructure tags at all. State store and state commit both tag their fields with something other than the name resolved from configuration, and state commit nests its settings under three inner structs while the keys are flat names on the section, apart from the one flat key-value setting that has a segment of its own. State commit's write mode is text rather than the reader's named type, because the reader parses a written name into that type itself. Declaring the named type would have one key answer as a named string from these defaults and as a plain one from an operator's file. Each section's test holds its resolved keys and values against the reader's own constants and its own defaults. Resolving is what it compares, rather than the registered struct, because the resolved map carries the key a tag produced and the value that tag's field held: a comparison of struct to struct agrees with itself while two tags sit on the wrong fields, since each field still holds the value the test names for it. Putting the genesis tags on each other's fields leaves the key set identical and fails the test. State store's declared defaults are not what its reader produces for a file missing those keys. It starts from the declared defaults, then assigns eleven of its twelve fields straight from a lookup with no check that the key was present, so an absent key casts to a zero and clobbers the default beside it: the store reads as disabled, with no backend, keeping every version, and committing synchronously. Only the snapshot key is guarded, and its own comment at the read says why. A node whose app.toml predates one of the other keys therefore runs the clobbered value today and the declared default once something installs this section, and guarding the remaining reads is what makes those the same thing. The recorded configuration surface does not move, because nothing consumes the registry on a boot path yet. --- app/config_register.go | 190 ++++++++++++++++++++++++++++++++++++ app/config_register_test.go | 166 +++++++++++++++++++++++++++++++ 2 files changed, 356 insertions(+) create mode 100644 app/config_register.go create mode 100644 app/config_register_test.go diff --git a/app/config_register.go b/app/config_register.go new file mode 100644 index 0000000000..8d17c6a21d --- /dev/null +++ b/app/config_register.go @@ -0,0 +1,190 @@ +package app + +import ( + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-db/config" +) + +// The names these sections have in the configuration key space. +const ( + LightInvarianceSectionName = "light_invariance" + GenesisSectionName = "genesis" + StateStoreSectionName = "state-store" + StateCommitSectionName = "state-commit" +) + +// Registration puts this package's configuration sections in the registry. +// +// The owning package registers its own sections, so the struct, the values and the keys come from one +// place and cannot drift apart. Three of the four declare a schema rather than the type their reader +// fills, and each says why on the schema itself; the keys still derive from mapstructure tags, so a +// section's spelling and its reader's own constants stay the same strings. +func init() { + registry.RegisterSection(LightInvarianceSectionName, &LightInvarianceConfig{}, lightInvarianceDefaults) + registry.RegisterSection(GenesisSectionName, &genesisSchema{}, genesisDefaults) + registry.RegisterSection(StateStoreSectionName, &stateStoreSchema{}, stateStoreDefaults) + registry.RegisterSection(StateCommitSectionName, &stateCommitSchema{}, stateCommitDefaults) +} + +// lightInvarianceDefaults is what this section resolves to for a node that has written nothing. +// +// The same value for every mode, and on. The check compares the bank module's recorded total supply +// against what the store holds, which is a correctness property of every node rather than of one kind, +// so a mode-varying default would stop some nodes noticing that they had diverged. +func lightInvarianceDefaults(registry.Mode) any { return DefaultLightInvarianceConfig } + +// genesisSchema declares the keys the genesis import reader resolves. +// +// A schema and not a transport: nothing decodes into it. The type the reader fills is +// genesistypes.GenesisImportConfig, which carries no mapstructure tags at all, so no key can be derived +// from it. Declaring the spelling here is what lets the registry name the keys the reader looks up, and +// the test holds these tags against the reader's own constants because nothing keeps them together by +// construction. +type genesisSchema struct { + StreamImport bool `mapstructure:"stream-import"` + ImportFile string `mapstructure:"import-file"` +} + +// genesisDefaults is what this section resolves to for a node that has written nothing. +// +// Read out of the reader's own default rather than written again here, so a changed default moves both at +// once and this states only which key carries which setting. The same values for every mode: streaming a +// genesis file is what an operator does to import a chain's existing state, and no node mode implies it. +func genesisDefaults(registry.Mode) any { + return genesisSchema{ + StreamImport: DefaultGenesisConfig.StreamGenesisImport, + ImportFile: DefaultGenesisConfig.GenesisStreamFile, + } +} + +// stateStoreSchema declares the keys parseSSConfigs resolves. +// +// A schema and not a transport: nothing decodes into it. config.StateStoreConfig carries mapstructure +// tags of its own and every one names something other than the key the reader looks up, so deriving from +// that type would declare a set of keys no operator writes. It also holds settings no key reaches, which +// stay at whatever the defaults struct holds; giving them keys would declare settings a written value +// could not change. +type stateStoreSchema struct { + Enable bool `mapstructure:"ss-enable"` + DBDirectory string `mapstructure:"ss-db-directory"` + Backend string `mapstructure:"ss-backend"` + AsyncWriteBuffer int `mapstructure:"ss-async-write-buffer"` + KeepRecent int `mapstructure:"ss-keep-recent"` + PruneIntervalSeconds int `mapstructure:"ss-prune-interval"` + ImportNumWorkers int `mapstructure:"ss-import-num-workers"` + EnableReadWriteMetrics bool `mapstructure:"ss-enable-read-write-metrics"` + SnapshotEnable bool `mapstructure:"ss-snapshot-enable"` + EVMDBDirectory string `mapstructure:"evm-ss-db-directory"` + SeparateEVMSubDBs bool `mapstructure:"evm-ss-separate-dbs"` + EVMSplit bool `mapstructure:"evm-ss-split"` +} + +// stateStoreDefaults is what this section resolves to for a node that has written nothing. +// +// The declared defaults, which is what seid init renders into app.toml, so a generated file reproduces a +// freshly initialised node. +// +// That is not what parseSSConfigs produces for a file missing these keys. It starts from the declared +// defaults and then assigns eleven of its twelve fields straight from a lookup with no check that the key +// was present, so an absent key casts to a zero and clobbers the default beside it: the store reads as +// disabled, with no backend, keeping every version, and committing synchronously. Only ss-snapshot-enable +// is guarded, and its own comment at the read says why. So a node whose app.toml predates one of the other +// keys runs the clobbered value today and the declared default once something installs this section, and +// guarding the remaining reads is what makes those the same thing. +func stateStoreDefaults(registry.Mode) any { + live := config.DefaultStateStoreConfig() + return stateStoreSchema{ + Enable: live.Enable, + DBDirectory: live.DBDirectory, + Backend: live.Backend, + AsyncWriteBuffer: live.AsyncWriteBuffer, + KeepRecent: live.KeepRecent, + PruneIntervalSeconds: live.PruneIntervalSeconds, + ImportNumWorkers: live.ImportNumWorkers, + EnableReadWriteMetrics: live.EnableReadWriteMetrics, + SnapshotEnable: live.SnapshotEnable, + EVMDBDirectory: live.EVMDBDirectory, + SeparateEVMSubDBs: live.SeparateEVMSubDBs, + EVMSplit: live.EVMSplit, + } +} + +// stateCommitFlatKVSchema declares the one flat key-value setting that has a key of its own. +// +// A nested segment, because the key is state-commit.flatkv.enable-read-write-metrics. The rest of the +// flat key-value configuration has no keys: nothing reads them from configuration, so declaring them +// would give an operator settings a written value could not change. +type stateCommitFlatKVSchema struct { + EnableReadWriteMetrics bool `mapstructure:"enable-read-write-metrics"` +} + +// stateCommitSchema declares the keys parseSCConfigs resolves. +// +// A schema and not a transport: nothing decodes into it. config.StateCommitConfig nests its settings +// under MemIAVLConfig, FlatKVConfig and HashLogger, and the keys the reader looks up are flat names on the +// section itself, so no derivation from that type produces them. +// +// The write mode is a plain string rather than the reader's own named type, because the reader parses a +// written name into that type itself. Declaring the named type would have one key answer as a named string +// from these defaults and as a plain one from an operator's file, which is a difference a caller can trip +// over and nothing here needs. +type stateCommitSchema struct { + Enable bool `mapstructure:"sc-enable"` + Directory string `mapstructure:"sc-directory"` + AsyncCommitBuffer int `mapstructure:"sc-async-commit-buffer"` + SnapshotKeepRecent uint32 `mapstructure:"sc-keep-recent"` + SnapshotInterval uint32 `mapstructure:"sc-snapshot-interval"` + SnapshotMinTimeInterval uint32 `mapstructure:"sc-snapshot-min-time-interval"` + SnapshotWriterLimit int `mapstructure:"sc-snapshot-writer-limit"` + SnapshotPrefetchThreshold float64 `mapstructure:"sc-snapshot-prefetch-threshold"` + SnapshotWriteRateMBps int `mapstructure:"sc-snapshot-write-rate-mbps"` + HistoricalProofMaxInFlight int `mapstructure:"sc-historical-proof-max-inflight"` + HistoricalProofRateLimit float64 `mapstructure:"sc-historical-proof-rate-limit"` + HistoricalProofBurst int `mapstructure:"sc-historical-proof-burst"` + WriteMode string `mapstructure:"sc-write-mode"` + WriteModeEnableAuto bool `mapstructure:"sc-write-mode-enable-auto"` + HashLoggerEnable bool `mapstructure:"sc-hash-logger-enable"` + HashLoggerDirectory string `mapstructure:"sc-hash-logger-directory"` + HashLoggerBlocksToRetain uint `mapstructure:"sc-hash-logger-blocks-to-retain"` + HashLoggerTargetFileSize uint `mapstructure:"sc-hash-logger-target-file-size"` + HashLoggerMaxDiskSize uint `mapstructure:"sc-hash-logger-max-disk-size"` + FlatKV stateCommitFlatKVSchema `mapstructure:"flatkv"` +} + +// stateCommitDefaults is what this section resolves to for a node that has written nothing. +// +// The declared defaults, which is what seid init renders into app.toml. Eighteen of parseSCConfigs' twenty +// reads already check that the key was present, so for those the declared default is also what an absent +// key resolves to today. The two that do not are sc-enable and sc-directory, and sc-enable is the one that +// matters: an absent key reads as false, and SetupSeiDB stops a node with state commitment off, so no +// running node has that key missing. Resolving it to true is what every working node already has written. +// +// The same values for every mode. How often a node snapshots and how much proof history it serves are +// decisions about disk and load that an operator writes down. +func stateCommitDefaults(registry.Mode) any { + live := config.DefaultStateCommitConfig() + return stateCommitSchema{ + Enable: live.Enable, + Directory: live.Directory, + AsyncCommitBuffer: live.MemIAVLConfig.AsyncCommitBuffer, + SnapshotKeepRecent: live.MemIAVLConfig.SnapshotKeepRecent, + SnapshotInterval: live.MemIAVLConfig.SnapshotInterval, + SnapshotMinTimeInterval: live.MemIAVLConfig.SnapshotMinTimeInterval, + SnapshotWriterLimit: live.MemIAVLConfig.SnapshotWriterLimit, + SnapshotPrefetchThreshold: live.MemIAVLConfig.SnapshotPrefetchThreshold, + SnapshotWriteRateMBps: live.MemIAVLConfig.SnapshotWriteRateMBps, + HistoricalProofMaxInFlight: live.HistoricalProofMaxInFlight, + HistoricalProofRateLimit: live.HistoricalProofRateLimit, + HistoricalProofBurst: live.HistoricalProofBurst, + WriteMode: string(live.WriteMode), + WriteModeEnableAuto: live.WriteModeEnableAuto, + HashLoggerEnable: live.HashLogger.Enable, + HashLoggerDirectory: live.HashLogger.Directory, + HashLoggerBlocksToRetain: live.HashLogger.BlocksToRetain, + HashLoggerTargetFileSize: live.HashLogger.TargetFileSize, + HashLoggerMaxDiskSize: live.HashLogger.MaxDiskSize, + FlatKV: stateCommitFlatKVSchema{ + EnableReadWriteMetrics: live.FlatKVConfig.EnableReadWriteMetrics, + }, + } +} diff --git a/app/config_register_test.go b/app/config_register_test.go new file mode 100644 index 0000000000..bcac7b8940 --- /dev/null +++ b/app/config_register_test.go @@ -0,0 +1,166 @@ +package app + +import ( + "reflect" + "testing" + + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-db/config" +) + +// requireSectionResolves holds one section's resolved keys and values against what its reader asks for. +// +// Resolving is what to compare against rather than the registered struct, because the resolved map is +// what a caller reads: it carries the key a tag produced and the value that tag's field held. A +// comparison of struct to struct agrees with itself while two tags are on the wrong fields, since each +// field still holds the value the test names for it. This one does not, because the swap moves the value +// to the other key. +// +// The values come from the reader's own constants and its own defaults, so a renamed key or a changed +// default fails here rather than being restated correctly in two places and wrongly in a third. +func requireSectionResolves(t *testing.T, mode registry.Mode, section string, want map[string]any) { + t.Helper() + registered, ok := registry.Lookup(section) + if !ok { + t.Fatalf("%s is not registered, so nothing resolves its keys", section) + } + resolved, err := registry.Resolve(mode, registry.Sources{}) + if err != nil { + t.Fatalf("mode %q: %v", mode, err) + } + + declared := make(map[string]bool, len(registered.Keys)) + for _, key := range registered.Keys { + declared[key] = true + expected, named := want[key] + if !named { + t.Errorf("mode %q: %s declares %s and nothing here names a value for it, so either its reader "+ + "resolves the key and this list is short, or no reader does and an operator has a setting "+ + "that changes nothing", mode, section, key) + continue + } + if got := resolved.Values[key]; !reflect.DeepEqual(got, expected) { + t.Errorf("mode %q: %s resolves to %#v (%T), want %#v (%T)", mode, key, got, got, expected, expected) + } + } + for key := range want { + if !declared[key] { + t.Errorf("mode %q: %s does not declare %s, which its reader resolves, so that setting stays "+ + "answered by whatever answers it today and nothing reports it", mode, section, key) + } + } +} + +// TestLightInvarianceResolves covers the one section registered as the type its reader fills. +// +// Every mode, because this section's defaults are the same for all of them: the check compares the bank +// module's recorded total supply against what the store holds, which is a property of every node. +func TestLightInvarianceResolves(t *testing.T) { + for _, mode := range registry.Modes() { + requireSectionResolves(t, mode, LightInvarianceSectionName, map[string]any{ + flagSupplyEnabled: DefaultLightInvarianceConfig.SupplyEnabled, + }) + } +} + +// TestGenesisResolves holds the genesis schema against the reader's own constants. +// +// The two keys carry different types, which is what makes the pairing worth asserting: the schema states +// the tags in one place and the reader looks them up in another, and nothing but this holds the two +// together. +func TestGenesisResolves(t *testing.T) { + for _, mode := range registry.Modes() { + requireSectionResolves(t, mode, GenesisSectionName, map[string]any{ + flagGenesisStreamImport: DefaultGenesisConfig.StreamGenesisImport, + flagGenesisImportFile: DefaultGenesisConfig.GenesisStreamFile, + }) + } +} + +// TestStateStoreResolves holds the state store schema against every key parseSSConfigs resolves. +// +// Twelve keys, including ss-snapshot-enable, which is the one read that checks whether the key was +// present. A schema short of a key its reader resolves leaves that setting undeclared, so it keeps +// whatever answers it today and no diagnostic names it. +func TestStateStoreResolves(t *testing.T) { + live := config.DefaultStateStoreConfig() + for _, mode := range registry.Modes() { + requireSectionResolves(t, mode, StateStoreSectionName, map[string]any{ + FlagSSEnable: live.Enable, + FlagSSDirectory: live.DBDirectory, + FlagSSBackend: live.Backend, + FlagSSAsyncWriterBuffer: live.AsyncWriteBuffer, + FlagSSKeepRecent: live.KeepRecent, + FlagSSPruneInterval: live.PruneIntervalSeconds, + FlagSSImportNumWorkers: live.ImportNumWorkers, + FlagSSReadWriteMetrics: live.EnableReadWriteMetrics, + FlagSSSnapshotEnable: live.SnapshotEnable, + FlagEVMSSDirectory: live.EVMDBDirectory, + FlagEVMSSSeparateDBs: live.SeparateEVMSubDBs, + FlagEVMSSSplit: live.EVMSplit, + }) + } +} + +// TestStateCommitResolves holds the state commit schema against every key parseSCConfigs resolves. +// +// Twenty keys, one of them a segment below the section, since the flat key-value read is +// state-commit.flatkv.enable-read-write-metrics. The write mode is a plain string here because the +// reader parses a written name into its own type, and comparing values is what holds it to that: the +// named type carries the same text and is not the same value. +func TestStateCommitResolves(t *testing.T) { + live := config.DefaultStateCommitConfig() + for _, mode := range registry.Modes() { + requireSectionResolves(t, mode, StateCommitSectionName, map[string]any{ + FlagSCEnable: live.Enable, + FlagSCDirectory: live.Directory, + FlagSCAsyncCommitBuffer: live.MemIAVLConfig.AsyncCommitBuffer, + FlagSCSnapshotKeepRecent: live.MemIAVLConfig.SnapshotKeepRecent, + FlagSCSnapshotInterval: live.MemIAVLConfig.SnapshotInterval, + FlagSCSnapshotMinTimeInterval: live.MemIAVLConfig.SnapshotMinTimeInterval, + FlagSCSnapshotWriterLimit: live.MemIAVLConfig.SnapshotWriterLimit, + FlagSCSnapshotPrefetchThreshold: live.MemIAVLConfig.SnapshotPrefetchThreshold, + FlagSCSnapshotWriteRateMBps: live.MemIAVLConfig.SnapshotWriteRateMBps, + FlagSCHistoricalProofMaxInFlight: live.HistoricalProofMaxInFlight, + FlagSCHistoricalProofRateLimit: live.HistoricalProofRateLimit, + FlagSCHistoricalProofBurst: live.HistoricalProofBurst, + FlagSCWriteMode: string(live.WriteMode), + FlagSCWriteModeEnableAuto: live.WriteModeEnableAuto, + FlagSCHashLoggerEnable: live.HashLogger.Enable, + FlagSCHashLoggerDirectory: live.HashLogger.Directory, + FlagSCHashLoggerBlocksToRetain: live.HashLogger.BlocksToRetain, + FlagSCHashLoggerTargetFileSize: live.HashLogger.TargetFileSize, + FlagSCHashLoggerMaxDiskSize: live.HashLogger.MaxDiskSize, + FlagSCFlatKVReadWriteMetrics: live.FlatKVConfig.EnableReadWriteMetrics, + }) + } +} + +// TestStateCommitWriteModeDefaultIsOneTheReaderAccepts covers the one declared value that is parsed text. +// +// Every other declared default is a value its reader uses as it stands. This one is a name the reader +// turns into a mode, so a default nothing parses would put a value in a generated file that stops the +// node it was generated for. +func TestStateCommitWriteModeDefaultIsOneTheReaderAccepts(t *testing.T) { + resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{}) + if err != nil { + t.Fatalf("%v", err) + } + declared, ok := resolved.Values[FlagSCWriteMode].(string) + if !ok { + t.Fatalf("%s resolves to %T, and the reader parses text", FlagSCWriteMode, resolved.Values[FlagSCWriteMode]) + } + if _, err := config.ParseSCWriteMode(declared); err != nil { + t.Errorf("%s resolves to %q, which this binary's own reader refuses: %v", FlagSCWriteMode, declared, err) + } +} + +// TestEverySectionThisPackageRegistersIsWellFormed covers what the registry itself refuses. +// +// A section with a tag the registry cannot read is reported rather than returned, so a defect here is a +// section that registered and declares nothing a caller can resolve. +func TestEverySectionThisPackageRegistersIsWellFormed(t *testing.T) { + for _, defect := range registry.Defects() { + t.Errorf("%s is registered and defective: %v", defect.Section, defect.Err) + } +} From 84660827a5aeb8befba57d7c145824f146e516ed Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 20 Aug 2026 15:58:21 -0700 Subject: [PATCH 2/5] config: answer the store section per mode, and measure the divergences Two of the state store's settings mean something different depending on what kind of node asks, and this section answered the same for all four. An archive node exists to keep history, and it was declaring a retention of a hundred thousand versions. The binary already says otherwise, in the mode rules it applies when it writes a file: an archive node keeps everything, and a validator and a seed run with the store off. Those rules are now where this section's answer comes from, so a change to them moves this too. That is the correction the pinned defect asks for. The record of it says to pin how configuration resolves today and correct it in the versioned manager, and this registry is the versioned manager, so declaring the rendered value would have pinned the defect a second time in the place meant to fix it. Nothing runs differently yet, because nothing consumes the registry on a boot path, and the direction matters more than the timing: state store pruning deletes inside the store on a timer, the archive volumes are protected against being deleted rather than against being emptied, and pruning frees disk so nothing that watches disk would fire. The rest of this change replaces prose with measurement. The declared values for the two storage sections are not what their readers produce for a file missing the keys, and the comment describing which keys those are was wrong three ways: it named four of the six store settings, missed two, named a commitment setting that does not in fact differ, and missed the one that selects how a node commits. The write mode is read through a presence check and then rewritten unconditionally, so a node with nothing written commits in the derived mode rather than the one that key carries. A comment cannot fail when it is wrong. So the set is measured against the readers now, per mode, and recorded as data: 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. Each key set is also held against this package's own read-site record, which is kept for another purpose and held against a golden file, rather than against a list written beside it in the same commit. The record spells its keys with the reader's constants and the section derives them from tags, so a rename on either side alone fails. One comment said the other keys under the commitment section's flat key-value name have no reader. Four of them are read by the Cosmos server's own reader, so they belong to whoever registers that section, and saying they reach nothing would have closed the door on declaring them. The whole-registry defect sweep here is now scoped to the four sections 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 lives where every section is linked. --- app/config_register.go | 47 ++++--- app/config_register_agreement_test.go | 179 +++++++++++++++++++++++++ app/config_register_test.go | 182 +++++++++++++++++--------- 3 files changed, 321 insertions(+), 87 deletions(-) create mode 100644 app/config_register_agreement_test.go diff --git a/app/config_register.go b/app/config_register.go index 8d17c6a21d..855aa53a1e 100644 --- a/app/config_register.go +++ b/app/config_register.go @@ -1,7 +1,9 @@ package app 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" "github.com/sei-protocol/sei-chain/sei-db/config" ) @@ -16,9 +18,8 @@ const ( // Registration puts this package's configuration sections in the registry. // // The owning package registers its own sections, so the struct, the values and the keys come from one -// place and cannot drift apart. Three of the four declare a schema rather than the type their reader -// fills, and each says why on the schema itself; the keys still derive from mapstructure tags, so a -// section's spelling and its reader's own constants stay the same strings. +// place and cannot drift apart. The keys derive from mapstructure tags, so a section's spelling and its +// reader's own constants stay the same strings. func init() { registry.RegisterSection(LightInvarianceSectionName, &LightInvarianceConfig{}, lightInvarianceDefaults) registry.RegisterSection(GenesisSectionName, &genesisSchema{}, genesisDefaults) @@ -81,18 +82,18 @@ type stateStoreSchema struct { // stateStoreDefaults is what this section resolves to for a node that has written nothing. // -// The declared defaults, which is what seid init renders into app.toml, so a generated file reproduces a -// freshly initialised node. +// Answered per mode, because two of these settings mean something different depending on what kind of node +// asks. An archive node exists to keep history, so it keeps every version; a validator and a seed serve no +// queries, so the store is off for them. Both come from the mode rules the binary already states rather +// than being written again here, so a change to those rules moves this too. // -// That is not what parseSSConfigs produces for a file missing these keys. It starts from the declared -// defaults and then assigns eleven of its twelve fields straight from a lookup with no check that the key -// was present, so an absent key casts to a zero and clobbers the default beside it: the store reads as -// disabled, with no backend, keeping every version, and committing synchronously. Only ss-snapshot-enable -// is guarded, and its own comment at the read says why. So a node whose app.toml predates one of the other -// keys runs the clobbered value today and the declared default once something installs this section, and -// guarding the remaining reads is what makes those the same thing. -func stateStoreDefaults(registry.Mode) any { - live := config.DefaultStateStoreConfig() +// This is the one section here whose declared values are not what its reader produces for a file missing +// the keys, and the divergences are measured rather than described. A test names each one and what a node +// runs today, so a read that gains a presence check has to account for it. +func stateStoreDefaults(mode registry.Mode) any { + server := srvconfig.DefaultConfig() + params.SetAppConfigByMode(server, params.NodeMode(mode)) + live := server.StateStore return stateStoreSchema{ Enable: live.Enable, DBDirectory: live.DBDirectory, @@ -109,11 +110,11 @@ func stateStoreDefaults(registry.Mode) any { } } -// stateCommitFlatKVSchema declares the one flat key-value setting that has a key of its own. +// stateCommitFlatKVSchema declares the one flat key-value key this package's reader resolves. // -// A nested segment, because the key is state-commit.flatkv.enable-read-write-metrics. The rest of the -// flat key-value configuration has no keys: nothing reads them from configuration, so declaring them -// would give an operator settings a written value could not change. +// A nested segment, because the key is state-commit.flatkv.enable-read-write-metrics. Four further keys +// under that name are read by the Cosmos server's own configuration reader and not by this one, so they +// belong to whoever registers that reader's section rather than to this one. type stateCommitFlatKVSchema struct { EnableReadWriteMetrics bool `mapstructure:"enable-read-write-metrics"` } @@ -153,14 +154,12 @@ type stateCommitSchema struct { // stateCommitDefaults is what this section resolves to for a node that has written nothing. // -// The declared defaults, which is what seid init renders into app.toml. Eighteen of parseSCConfigs' twenty -// reads already check that the key was present, so for those the declared default is also what an absent -// key resolves to today. The two that do not are sc-enable and sc-directory, and sc-enable is the one that -// matters: an absent key reads as false, and SetupSeiDB stops a node with state commitment off, so no -// running node has that key missing. Resolving it to true is what every working node already has written. +// The declared defaults. Two of them are not what this section's reader produces for a file missing the +// key, and a test names which two and what a node runs instead. // // The same values for every mode. How often a node snapshots and how much proof history it serves are -// decisions about disk and load that an operator writes down. +// decisions about disk and load that an operator writes down, and nothing in the binary makes either +// follow from what kind of node is asking. func stateCommitDefaults(registry.Mode) any { live := config.DefaultStateCommitConfig() return stateCommitSchema{ diff --git a/app/config_register_agreement_test.go b/app/config_register_agreement_test.go new file mode 100644 index 0000000000..ca84b68182 --- /dev/null +++ b/app/config_register_agreement_test.go @@ -0,0 +1,179 @@ +package app + +import ( + "fmt" + "sort" + "testing" + + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// whatANodeRunsToday is what each diverging key resolves to for a file carrying no keys at all. +// +// Every entry is a read that takes no account of whether the key was present, or a value another key +// transforms afterwards. Held separately from the modes because the reader takes no mode: it produces one +// answer, and which modes disagree with it depends on what the section declares. +var whatANodeRunsToday = map[string]string{ + FlagSSEnable: "false", + FlagSSBackend: "", + FlagSSAsyncWriterBuffer: "0", + FlagSSKeepRecent: "0", + FlagSSPruneInterval: "0", + FlagSSImportNumWorkers: "0", + FlagSCEnable: "false", + FlagSCWriteMode: "auto", +} + +// whyItMatters says what a node gets today, for the keys where that is worth stating. +var whyItMatters = map[string]string{ + FlagSSPruneInterval: "pruning is off, in the store and in the write-ahead log, so installing the " + + "declared value starts deleting what the node was retaining", + FlagSSKeepRecent: "every version is kept, so for an archive node what is declared and what runs " + + "agree about keeping history and for the others they do not", + FlagSCEnable: "state commitment reads as disabled, and a node started that way stops, which is why " + + "no running node has this key missing", + FlagSCWriteMode: "another key transforms this one after it is read, so the mode a node commits " + + "through is derived rather than carried by this key", +} + +// theDivergences is which keys disagree with the reader, per mode. +// +// Per mode because the section answers per mode for two of these settings and the reader does not answer +// per mode at all. An archive node declares the retention the reader also produces, so that key agrees for +// archive and disagrees everywhere else; the store toggle is the reverse. +var theDivergences = map[registry.Mode][]string{ + registry.ModeValidator: {FlagSSBackend, FlagSSAsyncWriterBuffer, FlagSSKeepRecent, + FlagSSPruneInterval, FlagSSImportNumWorkers, FlagSCEnable, FlagSCWriteMode}, + registry.ModeSeed: {FlagSSBackend, FlagSSAsyncWriterBuffer, FlagSSKeepRecent, + FlagSSPruneInterval, FlagSSImportNumWorkers, FlagSCEnable, FlagSCWriteMode}, + registry.ModeFull: {FlagSSEnable, FlagSSBackend, FlagSSAsyncWriterBuffer, FlagSSKeepRecent, + FlagSSPruneInterval, FlagSSImportNumWorkers, FlagSCEnable, FlagSCWriteMode}, + registry.ModeArchive: {FlagSSEnable, FlagSSBackend, FlagSSAsyncWriterBuffer, + FlagSSPruneInterval, FlagSSImportNumWorkers, FlagSCEnable, FlagSCWriteMode}, +} + +// readerValues is what each section's reader produces for a file carrying no keys at all. +// +// Written as a map from key to the field that key fills, because that pairing is what the comparison +// needs and neither the reader nor the section states it: the reader takes a key and assigns a field, and +// the section declares a key and a value. +func readerValues(t *testing.T) map[string]string { + t.Helper() + ss := parseSSConfigs(configtest.AppOpts{}) + sc := parseSCConfigs(configtest.AppOpts{}) + return map[string]string{ + FlagSSEnable: fmt.Sprint(ss.Enable), + FlagSSDirectory: fmt.Sprint(ss.DBDirectory), + FlagSSBackend: fmt.Sprint(ss.Backend), + FlagSSAsyncWriterBuffer: fmt.Sprint(ss.AsyncWriteBuffer), + FlagSSKeepRecent: fmt.Sprint(ss.KeepRecent), + FlagSSPruneInterval: fmt.Sprint(ss.PruneIntervalSeconds), + FlagSSImportNumWorkers: fmt.Sprint(ss.ImportNumWorkers), + FlagSSReadWriteMetrics: fmt.Sprint(ss.EnableReadWriteMetrics), + FlagSSSnapshotEnable: fmt.Sprint(ss.SnapshotEnable), + FlagEVMSSDirectory: fmt.Sprint(ss.EVMDBDirectory), + FlagEVMSSSeparateDBs: fmt.Sprint(ss.SeparateEVMSubDBs), + FlagEVMSSSplit: fmt.Sprint(ss.EVMSplit), + FlagSCEnable: fmt.Sprint(sc.Enable), + FlagSCDirectory: fmt.Sprint(sc.Directory), + FlagSCAsyncCommitBuffer: fmt.Sprint(sc.MemIAVLConfig.AsyncCommitBuffer), + FlagSCSnapshotKeepRecent: fmt.Sprint(sc.MemIAVLConfig.SnapshotKeepRecent), + FlagSCSnapshotInterval: fmt.Sprint(sc.MemIAVLConfig.SnapshotInterval), + FlagSCSnapshotMinTimeInterval: fmt.Sprint(sc.MemIAVLConfig.SnapshotMinTimeInterval), + FlagSCSnapshotWriterLimit: fmt.Sprint(sc.MemIAVLConfig.SnapshotWriterLimit), + FlagSCSnapshotPrefetchThreshold: fmt.Sprint(sc.MemIAVLConfig.SnapshotPrefetchThreshold), + FlagSCSnapshotWriteRateMBps: fmt.Sprint(sc.MemIAVLConfig.SnapshotWriteRateMBps), + FlagSCHistoricalProofMaxInFlight: fmt.Sprint(sc.HistoricalProofMaxInFlight), + FlagSCHistoricalProofRateLimit: fmt.Sprint(sc.HistoricalProofRateLimit), + FlagSCHistoricalProofBurst: fmt.Sprint(sc.HistoricalProofBurst), + FlagSCWriteMode: fmt.Sprint(sc.WriteMode), + FlagSCWriteModeEnableAuto: fmt.Sprint(sc.WriteModeEnableAuto), + FlagSCHashLoggerEnable: fmt.Sprint(sc.HashLogger.Enable), + FlagSCHashLoggerDirectory: fmt.Sprint(sc.HashLogger.Directory), + FlagSCHashLoggerBlocksToRetain: fmt.Sprint(sc.HashLogger.BlocksToRetain), + FlagSCHashLoggerTargetFileSize: fmt.Sprint(sc.HashLogger.TargetFileSize), + FlagSCHashLoggerMaxDiskSize: fmt.Sprint(sc.HashLogger.MaxDiskSize), + FlagSCFlatKVReadWriteMetrics: fmt.Sprint(sc.FlatKVConfig.EnableReadWriteMetrics), + } +} + +// TestTheDivergencesFromTheReaderAreTheRecordedOnes measures what the doc comments describe. +// +// The two storage sections declare defaults their readers do not produce for a file missing the keys, +// because most of those reads take no account of whether the key was present. Prose describing which keys +// those are cannot fail when it is wrong, and it was: it named four of the six store settings and one +// commitment setting that does not in fact differ, and missed the setting that selects how a node commits. +// +// So the set is measured here rather than described. A key that starts diverging fails this test, and so +// does one that stops: guarding a read means deleting its row, which is what makes the reconciliation +// something a change has to account for rather than something a comment claims. +func TestTheDivergencesFromTheReaderAreTheRecordedOnes(t *testing.T) { + reader := readerValues(t) + for _, mode := range registry.Modes() { + resolved, err := registry.Resolve(mode, registry.Sources{}) + if err != nil { + t.Fatalf("mode %q: %v", mode, err) + } + recorded, named := theDivergences[mode] + if !named { + t.Fatalf("mode %q has no record here, so a mode was added and this was not revisited", mode) + } + listed := make(map[string]bool, len(recorded)) + for _, key := range recorded { + listed[key] = true + } + + var measured []string + for key, got := range reader { + declared, declares := resolved.Values[key] + if !declares { + t.Errorf("mode %q: %s is read by this package and no section declares it", mode, key) + continue + } + if fmt.Sprint(declared) == got { + if listed[key] { + t.Errorf("mode %q: %s no longer diverges, both sides being %v. Take it off that "+ + "mode's list, so the list stays the set of keys installing this section changes", + mode, key, declared) + } + continue + } + measured = append(measured, key) + if !listed[key] { + t.Errorf("mode %q: %s declares %v and its reader produces %q for a file with no keys, and "+ + "nothing records that. Installing this section changes what such a node runs. %s", + mode, key, declared, got, whyItMatters[key]) + } + if want, stated := whatANodeRunsToday[key]; stated && want != got { + t.Errorf("mode %q: %s is recorded as producing %q and produces %q", mode, key, want, got) + } + } + + sort.Strings(measured) + if len(measured) != len(recorded) { + t.Errorf("mode %q: measured %d divergences and %d are recorded: %v", + mode, len(measured), len(recorded), measured) + } + } +} + +// TestEveryKeyThisPackageDeclaresIsOneItsReadersFill holds the two lists against each other. +// +// The declared keys come from the schemas and the read keys from the map above, so a key on one side only +// is either a setting an operator writes that no reader fills, or one this package reads and nothing +// declares. +func TestEveryKeyThisPackageDeclaresIsOneItsReadersFill(t *testing.T) { + reader := readerValues(t) + for _, section := range []string{StateStoreSectionName, StateCommitSectionName} { + 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/app/config_register_test.go b/app/config_register_test.go index bcac7b8940..5aa6a24fe0 100644 --- a/app/config_register_test.go +++ b/app/config_register_test.go @@ -2,95 +2,135 @@ package app import ( "reflect" + "sort" "testing" "github.com/sei-protocol/sei-chain/config/registry" "github.com/sei-protocol/sei-chain/sei-db/config" + "github.com/sei-protocol/sei-chain/testutil/configtest" ) -// requireSectionResolves holds one section's resolved keys and values against what its reader asks for. +// manifestKeys returns the keys a section's read-site record names, plus any named here. // -// Resolving is what to compare against rather than the registered struct, because the resolved map is -// what a caller reads: it carries the key a tag produced and the value that tag's field held. A -// comparison of struct to struct agrees with itself while two tags are on the wrong fields, since each -// field still holds the value the test names for it. This one does not, because the swap moves the value -// to the other key. -// -// The values come from the reader's own constants and its own defaults, so a renamed key or a changed -// default fails here rather than being restated correctly in two places and wrongly in a third. -func requireSectionResolves(t *testing.T, mode registry.Mode, section string, want map[string]any) { +// The record is this package's own statement of which keys each reader looks up, kept for another purpose +// and held against a golden file. Taking the key set from it means a section's declaration is compared +// against something maintained under a different discipline, rather than against a list written beside it +// by the same hand in the same commit. +func manifestKeys(specs []configtest.KeySpec, also ...string) []string { + out := make([]string, 0, len(specs)+len(also)) + for _, spec := range specs { + out = append(out, spec.Key) + } + out = append(out, also...) + sort.Strings(out) + return out +} + +// requireDeclares holds a section's declared keys against the record of what its reader looks up. +func requireDeclares(t *testing.T, section string, want []string) { t.Helper() + for _, defect := range registry.Defects() { + if defect.Section == section { + t.Fatalf("%s was refused, so none of its keys is declared: %v", section, defect.Err) + } + } registered, ok := registry.Lookup(section) if !ok { t.Fatalf("%s is not registered, so nothing resolves its keys", section) } + if !reflect.DeepEqual(registered.Keys, want) { + t.Errorf("%s declares\n %v\nand its read-site record names\n %v\nA key on one side only is either "+ + "a setting an operator writes that no reader fills, or one this package reads and nothing "+ + "declares", section, registered.Keys, want) + } +} + +// requireResolves holds a section's resolved values against what its reader's own defaults hold. +// +// Resolving is what to compare against rather than the registered struct, because the resolved map carries +// the key a tag produced and the value that tag's field held. A comparison of struct to struct agrees with +// itself while two tags sit on the wrong fields, since each field still holds the value the test names for +// it. The swap moves the value to the other key, and this notices. +func requireResolves(t *testing.T, mode registry.Mode, section string, want map[string]any) { + t.Helper() resolved, err := registry.Resolve(mode, registry.Sources{}) if err != nil { t.Fatalf("mode %q: %v", mode, err) } - - declared := make(map[string]bool, len(registered.Keys)) - for _, key := range registered.Keys { - declared[key] = true - expected, named := want[key] - if !named { - t.Errorf("mode %q: %s declares %s and nothing here names a value for it, so either its reader "+ - "resolves the key and this list is short, or no reader does and an operator has a setting "+ - "that changes nothing", mode, section, key) - continue - } + for key, expected := range want { if got := resolved.Values[key]; !reflect.DeepEqual(got, expected) { - t.Errorf("mode %q: %s resolves to %#v (%T), want %#v (%T)", mode, key, got, got, expected, expected) - } - } - for key := range want { - if !declared[key] { - t.Errorf("mode %q: %s does not declare %s, which its reader resolves, so that setting stays "+ - "answered by whatever answers it today and nothing reports it", mode, section, key) + t.Errorf("mode %q: %s resolves to %#v (%T), want %#v (%T)", + mode, key, got, got, expected, expected) } } } -// TestLightInvarianceResolves covers the one section registered as the type its reader fills. -// -// Every mode, because this section's defaults are the same for all of them: the check compares the bank -// module's recorded total supply against what the store holds, which is a property of every node. -func TestLightInvarianceResolves(t *testing.T) { +// TestLightInvarianceDeclaresAndResolves covers the one section registered as the type its reader fills. +func TestLightInvarianceDeclaresAndResolves(t *testing.T) { + requireDeclares(t, LightInvarianceSectionName, manifestKeys(lightInvarianceKeys)) for _, mode := range registry.Modes() { - requireSectionResolves(t, mode, LightInvarianceSectionName, map[string]any{ + requireResolves(t, mode, LightInvarianceSectionName, map[string]any{ flagSupplyEnabled: DefaultLightInvarianceConfig.SupplyEnabled, }) } } -// TestGenesisResolves holds the genesis schema against the reader's own constants. +// TestGenesisDeclaresAndResolves holds the genesis schema against the record and the reader's defaults. // -// The two keys carry different types, which is what makes the pairing worth asserting: the schema states -// the tags in one place and the reader looks them up in another, and nothing but this holds the two -// together. -func TestGenesisResolves(t *testing.T) { +// The record names one of the two keys as a row and the other beside it, because that one is read as a type +// assertion rather than a guarded cast and a row would predict the wrong resolution. Both are this +// package's, so both are declared. +func TestGenesisDeclaresAndResolves(t *testing.T) { + requireDeclares(t, GenesisSectionName, manifestKeys(genesisKeys, flagGenesisImportFile)) for _, mode := range registry.Modes() { - requireSectionResolves(t, mode, GenesisSectionName, map[string]any{ + requireResolves(t, mode, GenesisSectionName, map[string]any{ flagGenesisStreamImport: DefaultGenesisConfig.StreamGenesisImport, flagGenesisImportFile: DefaultGenesisConfig.GenesisStreamFile, }) } } -// TestStateStoreResolves holds the state store schema against every key parseSSConfigs resolves. +// TestStateStoreDeclaresEveryKeyItsReaderResolves holds the schema against the read-site record. +func TestStateStoreDeclaresEveryKeyItsReaderResolves(t *testing.T) { + requireDeclares(t, StateStoreSectionName, manifestKeys(ssKeys)) +} + +// TestStateStoreResolvesWhatEachKindOfNodeNeeds is the mode-varying half of this section. // -// Twelve keys, including ss-snapshot-enable, which is the one read that checks whether the key was -// present. A schema short of a key its reader resolves leaves that setting undeclared, so it keeps -// whatever answers it today and no diagnostic names it. -func TestStateStoreResolves(t *testing.T) { +// Two of these settings mean something different depending on what kind of node asks, and the values are +// written out here rather than taken from the same rules the section reads. An archive node exists to keep +// history, so a retention that pruned it would be the one declaration here that destroys data, and it +// would do so with nothing to alert on, because pruning frees disk rather than filling it. +func TestStateStoreResolvesWhatEachKindOfNodeNeeds(t *testing.T) { + byMode := map[registry.Mode]struct { + enable bool + keepRecent int + }{ + registry.ModeValidator: {enable: false, keepRecent: 100000}, + registry.ModeSeed: {enable: false, keepRecent: 100000}, + registry.ModeFull: {enable: true, keepRecent: 100000}, + registry.ModeArchive: {enable: true, keepRecent: 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) + } + requireResolves(t, mode, StateStoreSectionName, map[string]any{ + FlagSSEnable: want.enable, + FlagSSKeepRecent: want.keepRecent, + }) + } +} + +// TestStateStoreResolvesItsOtherValuesTheSameForEveryMode covers the ten settings a mode does not change. +func TestStateStoreResolvesItsOtherValuesTheSameForEveryMode(t *testing.T) { live := config.DefaultStateStoreConfig() for _, mode := range registry.Modes() { - requireSectionResolves(t, mode, StateStoreSectionName, map[string]any{ - FlagSSEnable: live.Enable, + requireResolves(t, mode, StateStoreSectionName, map[string]any{ FlagSSDirectory: live.DBDirectory, FlagSSBackend: live.Backend, FlagSSAsyncWriterBuffer: live.AsyncWriteBuffer, - FlagSSKeepRecent: live.KeepRecent, FlagSSPruneInterval: live.PruneIntervalSeconds, FlagSSImportNumWorkers: live.ImportNumWorkers, FlagSSReadWriteMetrics: live.EnableReadWriteMetrics, @@ -102,16 +142,25 @@ func TestStateStoreResolves(t *testing.T) { } } -// TestStateCommitResolves holds the state commit schema against every key parseSCConfigs resolves. +// TestStateCommitDeclaresEveryKeyItsReaderResolves holds the schema against the read-site record. +// +// Twenty keys: the seventeen the record holds as rows, and three it names beside them because each has a +// target of its own. The four keys under this section's flat key-value name that only the Cosmos server's +// reader resolves are not among them, and are not this section's to declare. +func TestStateCommitDeclaresEveryKeyItsReaderResolves(t *testing.T) { + requireDeclares(t, StateCommitSectionName, manifestKeys(scKeys, + FlagSCWriteMode, FlagSCWriteModeEnableAuto, FlagSCHashLoggerTargetFileSize)) +} + +// TestStateCommitResolvesTheModuleDeclaredValues covers the value side of the same registration. // -// Twenty keys, one of them a segment below the section, since the flat key-value read is -// state-commit.flatkv.enable-read-write-metrics. The write mode is a plain string here because the -// reader parses a written name into its own type, and comparing values is what holds it to that: the -// named type carries the same text and is not the same value. -func TestStateCommitResolves(t *testing.T) { +// The write mode is a plain string here because the reader parses a written name into its own type, and +// comparing values is what holds it to that: the named type carries the same text and is not the same +// value. +func TestStateCommitResolvesTheModuleDeclaredValues(t *testing.T) { live := config.DefaultStateCommitConfig() for _, mode := range registry.Modes() { - requireSectionResolves(t, mode, StateCommitSectionName, map[string]any{ + requireResolves(t, mode, StateCommitSectionName, map[string]any{ FlagSCEnable: live.Enable, FlagSCDirectory: live.Directory, FlagSCAsyncCommitBuffer: live.MemIAVLConfig.AsyncCommitBuffer, @@ -138,9 +187,8 @@ func TestStateCommitResolves(t *testing.T) { // TestStateCommitWriteModeDefaultIsOneTheReaderAccepts covers the one declared value that is parsed text. // -// Every other declared default is a value its reader uses as it stands. This one is a name the reader -// turns into a mode, so a default nothing parses would put a value in a generated file that stops the -// node it was generated for. +// Every other declared value is used as it stands. This one is a name the reader turns into a mode, so a +// default nothing parses would put a value in a generated file that stops the node it was generated for. func TestStateCommitWriteModeDefaultIsOneTheReaderAccepts(t *testing.T) { resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{}) if err != nil { @@ -155,12 +203,20 @@ func TestStateCommitWriteModeDefaultIsOneTheReaderAccepts(t *testing.T) { } } -// TestEverySectionThisPackageRegistersIsWellFormed covers what the registry itself refuses. +// TestTheSectionsThisPackageRegistersAreUsable covers what the registry refuses. // -// A section with a tag the registry cannot read is reported rather than returned, so a defect here is a -// section that registered and declares nothing a caller can resolve. -func TestEverySectionThisPackageRegistersIsWellFormed(t *testing.T) { +// Scoped to the four names this file registers. The whole-registry sweep belongs where every section is +// linked, because a refusal that depends on what else registered is not this package's to answer for. +func TestTheSectionsThisPackageRegistersAreUsable(t *testing.T) { + mine := map[string]bool{ + LightInvarianceSectionName: true, + GenesisSectionName: true, + StateStoreSectionName: true, + StateCommitSectionName: 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) + } } } From 0e1d87072ac92404824d64af6c1c5e052ce59ede Mon Sep 17 00:00:00 2001 From: bdchatham Date: Fri, 21 Aug 2026 07:25:11 -0700 Subject: [PATCH 3/5] config: state each comment's own subject Four comments said more than their subject. The light-invariance default now states the mode decision and leaves what the check compares to the check. The genesis schema says why it exists and leaves what holds it to the test that holds it. The helper that resolves says once that it renders every section, so a failure naming another one is read correctly. And the write-mode default is asked for every mode rather than one, which is how the tests beside it ask. --- app/config_register.go | 9 +++------ app/config_register_test.go | 28 ++++++++++++++++++---------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/app/config_register.go b/app/config_register.go index 855aa53a1e..aca7c71889 100644 --- a/app/config_register.go +++ b/app/config_register.go @@ -29,18 +29,15 @@ func init() { // lightInvarianceDefaults is what this section resolves to for a node that has written nothing. // -// The same value for every mode, and on. The check compares the bank module's recorded total supply -// against what the store holds, which is a correctness property of every node rather than of one kind, -// so a mode-varying default would stop some nodes noticing that they had diverged. +// The same value for every mode, and on. What the check compares is a property of every node rather than +// of one kind, so a mode that resolved it off would stop those nodes noticing they had diverged. func lightInvarianceDefaults(registry.Mode) any { return DefaultLightInvarianceConfig } // genesisSchema declares the keys the genesis import reader resolves. // // A schema and not a transport: nothing decodes into it. The type the reader fills is // genesistypes.GenesisImportConfig, which carries no mapstructure tags at all, so no key can be derived -// from it. Declaring the spelling here is what lets the registry name the keys the reader looks up, and -// the test holds these tags against the reader's own constants because nothing keeps them together by -// construction. +// from it. Declaring the spelling here is what lets the registry name the keys the reader looks up. type genesisSchema struct { StreamImport bool `mapstructure:"stream-import"` ImportFile string `mapstructure:"import-file"` diff --git a/app/config_register_test.go b/app/config_register_test.go index 5aa6a24fe0..35547461ea 100644 --- a/app/config_register_test.go +++ b/app/config_register_test.go @@ -47,6 +47,10 @@ func requireDeclares(t *testing.T, section string, want []string) { // requireResolves holds a section's resolved values against what its reader's own defaults hold. // +// Resolving renders every registered section, so a section elsewhere whose defaults cannot state a value +// for a key it declares fails here too. The registry names that section in the error, so the message +// points at the real one rather than at whichever test asked. +// // Resolving is what to compare against rather than the registered struct, because the resolved map carries // the key a tag produced and the value that tag's field held. A comparison of struct to struct agrees with // itself while two tags sit on the wrong fields, since each field still holds the value the test names for @@ -190,16 +194,20 @@ func TestStateCommitResolvesTheModuleDeclaredValues(t *testing.T) { // Every other declared value is used as it stands. This one is a name the reader turns into a mode, so a // default nothing parses would put a value in a generated file that stops the node it was generated for. func TestStateCommitWriteModeDefaultIsOneTheReaderAccepts(t *testing.T) { - resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{}) - if err != nil { - t.Fatalf("%v", err) - } - declared, ok := resolved.Values[FlagSCWriteMode].(string) - if !ok { - t.Fatalf("%s resolves to %T, and the reader parses text", FlagSCWriteMode, resolved.Values[FlagSCWriteMode]) - } - if _, err := config.ParseSCWriteMode(declared); err != nil { - t.Errorf("%s resolves to %q, which this binary's own reader refuses: %v", FlagSCWriteMode, declared, err) + for _, mode := range registry.Modes() { + resolved, err := registry.Resolve(mode, registry.Sources{}) + if err != nil { + t.Fatalf("mode %q: %v", mode, err) + } + declared, ok := resolved.Values[FlagSCWriteMode].(string) + if !ok { + t.Fatalf("mode %q: %s resolves to %T, and the reader parses text", + mode, FlagSCWriteMode, resolved.Values[FlagSCWriteMode]) + } + if _, err := config.ParseSCWriteMode(declared); err != nil { + t.Errorf("mode %q: %s resolves to %q, which this binary's own reader refuses: %v", + mode, FlagSCWriteMode, declared, err) + } } } From e244418bfe985b1c27fde56fa075839e01806136 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Sun, 23 Aug 2026 17:33:53 -0700 Subject: [PATCH 4/5] config: the archive retention departs on purpose, and says so A declared value is what the seid init command writes for a kind of node. This section departs from that once: the retention an archive node keeps. The mode rules set it to keep everything and the command does not write that, because the type it renders declares a state store field of its own and fills it from the mode-blind default, so the rule is applied and then discarded. PLT-955 records that and records the decision, which is to pin what a node resolves today and correct it in the versioned declaration rather than at the point that loses it. So the departure is intended, and it is now held rather than asserted. It fails if the command starts carrying the rule, which is the day the departure should be deleted. It fails if this section stops departing, which would declare a retention on the one kind of node whose purpose is keeping what it would prune. Both directions are checked, because a departure nothing measures cannot be told from an oversight. --- app/config_register.go | 16 ++++++++++---- app/config_register_test.go | 43 +++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/app/config_register.go b/app/config_register.go index aca7c71889..e7ab04ba8f 100644 --- a/app/config_register.go +++ b/app/config_register.go @@ -77,16 +77,24 @@ type stateStoreSchema struct { EVMSplit bool `mapstructure:"evm-ss-split"` } -// stateStoreDefaults is what this section resolves to for a node that has written nothing. +// stateStoreDefaults is what the seid init command writes for a node of this kind, with one deliberate +// departure. // // Answered per mode, because two of these settings mean something different depending on what kind of node // asks. An archive node exists to keep history, so it keeps every version; a validator and a seed serve no // queries, so the store is off for them. Both come from the mode rules the binary already states rather // than being written again here, so a change to those rules moves this too. // -// This is the one section here whose declared values are not what its reader produces for a file missing -// the keys, and the divergences are measured rather than described. A test names each one and what a node -// runs today, so a read that gains a presence check has to account for it. +// The departure is the retention an archive node keeps. The mode rules set it to keep everything and the +// command does not write that, because the type it renders declares a state store field of its own and +// fills it from the mode-blind default, so the rule is applied and then discarded. PLT-955 records that, +// and records the decision: pin what a node resolves today and correct it here, in the versioned +// declaration, rather than at the point that loses it. So this states the rule and the command states the +// value the rule was overwritten by, and the test beside this holds both, because a departure nothing +// measures is indistinguishable from an oversight. +// +// The declared values are also not what this section's reader produces for a file missing the keys, which +// is a different comparison and measured separately. func stateStoreDefaults(mode registry.Mode) any { server := srvconfig.DefaultConfig() params.SetAppConfigByMode(server, params.NodeMode(mode)) diff --git a/app/config_register_test.go b/app/config_register_test.go index 35547461ea..7fe25c5c86 100644 --- a/app/config_register_test.go +++ b/app/config_register_test.go @@ -228,3 +228,46 @@ func TestTheSectionsThisPackageRegistersAreUsable(t *testing.T) { } } } + +// TestTheArchiveRetentionDepartsFromWhatTheCommandWrites measures the one deliberate departure. +// +// A declared value is what the seid init command writes for a kind of node. This section departs from that +// in exactly one place: the retention an archive node keeps. The mode rules set it to keep everything, and +// the command does not write that, because the type it renders declares a state store field of its own and +// fills it from the mode-blind default, so the rule is applied and then thrown away. +// +// PLT-955 records the defect and the decision to correct it in the versioned declaration rather than at the +// point that loses it. So the departure is intended, and it is held here for two reasons. It fails if the +// command starts writing the rule, which is the day this departure should be deleted. And it fails if this +// section stops departing, which would put a retention on the one kind of node whose purpose is keeping +// what it would prune. +func TestTheArchiveRetentionDepartsFromWhatTheCommandWrites(t *testing.T) { + live := config.DefaultStateStoreConfig() + + // What the command renders for an archive node: the mode rules are applied to the server + // configuration, and then the type it renders fills its own state store field from the mode-blind + // default, which is what reaches the file. + written := live.KeepRecent + if written == 0 { + t.Fatalf("the mode-blind default retention is already zero, so this departure measures nothing " + + "and the comparison below holds for any declaration") + } + + resolved, err := registry.Resolve(registry.ModeArchive, registry.Sources{}) + if err != nil { + t.Fatalf("%v", err) + } + declared := resolved.Values[FlagSSKeepRecent] + + if declared == written { + t.Errorf("%s resolves to %v for an archive node, which is what the command writes. Either the "+ + "command now carries the mode rule, in which case this departure and its note should go, or "+ + "this section stopped departing and an archive node is declared to prune the history it "+ + "exists to keep", FlagSSKeepRecent, declared) + } + if declared != 0 { + t.Errorf("%s resolves to %v for an archive node, want zero. The mode rule keeps every version, "+ + "and departing from the command is only defensible while this states that rule", + FlagSSKeepRecent, declared) + } +} From cc81247cd320f5d26a691708ba47ec539974c6d3 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 24 Aug 2026 15:24:58 -0700 Subject: [PATCH 5/5] fix(config): name every departure, declare the third genesis key Four findings from review, all measured rather than argued. The state store section says it departs from what the command writes in one place and it departs in three, across two keys and from one cause. A validator and a seed keep the store off where the command writes it on, which is the more consequential of the two: it leaves a request surface on the nodes the rules close it for. The record now holds all three rows, so one that stops departing fails and one that starts departing fails too. That record could not see the command. This package cannot import the one that renders the file, so the mode-blind side was read from the same default the command reads, which meant a change making the command keep the rules would have left the record green and stale. The half that fails then now lives beside the command, and reverting the discard fails it three times by name. The genesis section declares a third key. The upstream server reads it into a configuration of its own and a generated file renders it, and a section name is the whole of what a registration owns, so a key under genesis is this registration's or nobody's. The record already named it; the declaration did not. The test now reads that record rather than a list written again beside it. The comment on the nested flat key-value segment said four further keys belong to whoever registers that reader's section. No such registration can exist, for the same reason as the genesis key. It now says what is true: they are deliberately not declared, the template renders none of them, and an operator who writes one is told it reached nothing rather than having a value applied for a setting this section does not state. Co-Authored-By: Claude Opus 5 (1M context) --- app/config_register.go | 39 +++++-- app/config_register_test.go | 125 +++++++++++++++------ cmd/seid/cmd/state_store_departure_test.go | 65 +++++++++++ 3 files changed, 181 insertions(+), 48 deletions(-) create mode 100644 cmd/seid/cmd/state_store_departure_test.go diff --git a/app/config_register.go b/app/config_register.go index e7ab04ba8f..dcfab7a493 100644 --- a/app/config_register.go +++ b/app/config_register.go @@ -41,6 +41,12 @@ func lightInvarianceDefaults(registry.Mode) any { return DefaultLightInvarianceC type genesisSchema struct { StreamImport bool `mapstructure:"stream-import"` ImportFile string `mapstructure:"import-file"` + + // The third key under this name, which a different reader takes: the upstream server resolves it into + // its own configuration and a generated app.toml renders it. Declared here because a section name is + // the whole of what a registration owns, and a dotted one is refused, so nothing else can ever declare + // a key under genesis. Left out, it would be a key every node's file carries that no section knows. + StreamFile string `mapstructure:"genesis-stream-file"` } // genesisDefaults is what this section resolves to for a node that has written nothing. @@ -52,6 +58,7 @@ func genesisDefaults(registry.Mode) any { return genesisSchema{ StreamImport: DefaultGenesisConfig.StreamGenesisImport, ImportFile: DefaultGenesisConfig.GenesisStreamFile, + StreamFile: srvconfig.DefaultConfig().Genesis.GenesisStreamFile, } } @@ -77,21 +84,24 @@ type stateStoreSchema struct { EVMSplit bool `mapstructure:"evm-ss-split"` } -// stateStoreDefaults is what the seid init command writes for a node of this kind, with one deliberate -// departure. +// stateStoreDefaults is what the seid init command writes for a node of this kind, departing from it on two +// keys and for one reason. // // Answered per mode, because two of these settings mean something different depending on what kind of node // asks. An archive node exists to keep history, so it keeps every version; a validator and a seed serve no // queries, so the store is off for them. Both come from the mode rules the binary already states rather // than being written again here, so a change to those rules moves this too. // -// The departure is the retention an archive node keeps. The mode rules set it to keep everything and the -// command does not write that, because the type it renders declares a state store field of its own and -// fills it from the mode-blind default, so the rule is applied and then discarded. PLT-955 records that, -// and records the decision: pin what a node resolves today and correct it here, in the versioned -// declaration, rather than at the point that loses it. So this states the rule and the command states the -// value the rule was overwritten by, and the test beside this holds both, because a departure nothing -// measures is indistinguishable from an oversight. +// One cause. The type the command renders declares a state store field of its own and fills it from the +// mode-blind default, so every value the mode rules put on this section is applied and then discarded. +// PLT-955 records that, and records the decision: pin what a node resolves today and correct it here, in the +// versioned declaration, rather than at the point that loses it. +// +// So this follows the rules and departs from the command wherever the rules moved a value, which is three +// rows across two keys. A validator and a seed keep the store off, where the command writes it on; that is +// the more consequential of the two, because it leaves a request surface on nodes the rules close it for. An +// archive node keeps every version, where the command writes a retention of a hundred thousand. The test +// beside this holds all three, because a departure nothing measures is indistinguishable from an oversight. // // The declared values are also not what this section's reader produces for a file missing the keys, which // is a different comparison and measured separately. @@ -117,9 +127,14 @@ func stateStoreDefaults(mode registry.Mode) any { // stateCommitFlatKVSchema declares the one flat key-value key this package's reader resolves. // -// A nested segment, because the key is state-commit.flatkv.enable-read-write-metrics. Four further keys -// under that name are read by the Cosmos server's own configuration reader and not by this one, so they -// belong to whoever registers that reader's section rather than to this one. +// A nested segment, because the key is state-commit.flatkv.enable-read-write-metrics. +// +// Four further keys under that name are read by the upstream server's own configuration reader and not by +// this one, and they are deliberately not declared. Nothing else could declare them: a section name is the +// whole of what a registration owns and a dotted one is refused, so every key under state-commit is this +// registration's or nobody's. They are left out because the template renders none of them, so a node only +// has one if somebody added it by hand, and an operator who writes one is told it reached nothing rather +// than having a value applied for a setting this section does not state. type stateCommitFlatKVSchema struct { EnableReadWriteMetrics bool `mapstructure:"enable-read-write-metrics"` } diff --git a/app/config_register_test.go b/app/config_register_test.go index 7fe25c5c86..fab0305984 100644 --- a/app/config_register_test.go +++ b/app/config_register_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/sei-protocol/sei-chain/config/registry" + srvconfig "github.com/sei-protocol/sei-chain/sei-cosmos/server/config" "github.com/sei-protocol/sei-chain/sei-db/config" "github.com/sei-protocol/sei-chain/testutil/configtest" ) @@ -81,15 +82,26 @@ func TestLightInvarianceDeclaresAndResolves(t *testing.T) { // TestGenesisDeclaresAndResolves holds the genesis schema against the record and the reader's defaults. // -// The record names one of the two keys as a row and the other beside it, because that one is read as a type -// assertion rather than a guarded cast and a row would predict the wrong resolution. Both are this -// package's, so both are declared. +// Three keys, and the record is what says so rather than a list written again here. One is a row. The other +// two have targets of their own: one is read as a type assertion rather than a guarded cast, so a row would +// predict the wrong resolution, and one is read by the upstream server into its own configuration and not by +// this package at all. +// +// All three are declared, because a section name is the whole of what a registration owns and a dotted one +// is refused, so a key under genesis is this registration's or nobody's. func TestGenesisDeclaresAndResolves(t *testing.T) { - requireDeclares(t, GenesisSectionName, manifestKeys(genesisKeys, flagGenesisImportFile)) + want := manifestKeys(genesisKeys) + for _, key := range genesisKeysWithTargetsOfTheirOwn { + want = append(want, string(key)) + } + sort.Strings(want) + requireDeclares(t, GenesisSectionName, want) + for _, mode := range registry.Modes() { requireResolves(t, mode, GenesisSectionName, map[string]any{ - flagGenesisStreamImport: DefaultGenesisConfig.StreamGenesisImport, - flagGenesisImportFile: DefaultGenesisConfig.GenesisStreamFile, + flagGenesisStreamImport: DefaultGenesisConfig.StreamGenesisImport, + flagGenesisImportFile: DefaultGenesisConfig.GenesisStreamFile, + "genesis.genesis-stream-file": srvconfig.DefaultConfig().Genesis.GenesisStreamFile, }) } } @@ -229,45 +241,86 @@ func TestTheSectionsThisPackageRegistersAreUsable(t *testing.T) { } } -// TestTheArchiveRetentionDepartsFromWhatTheCommandWrites measures the one deliberate departure. +// TestTheseDeparturesFromWhatTheCommandWritesAreTheRecordedOnes measures every one of them. // // A declared value is what the seid init command writes for a kind of node. This section departs from that -// in exactly one place: the retention an archive node keeps. The mode rules set it to keep everything, and -// the command does not write that, because the type it renders declares a state store field of its own and -// fills it from the mode-blind default, so the rule is applied and then thrown away. +// wherever the mode rules moved a value, and for one cause: the type the command renders declares a state +// store field of its own and fills it from the mode-blind default, so every rule this section applies is +// applied and then thrown away. // // PLT-955 records the defect and the decision to correct it in the versioned declaration rather than at the -// point that loses it. So the departure is intended, and it is held here for two reasons. It fails if the -// command starts writing the rule, which is the day this departure should be deleted. And it fails if this -// section stops departing, which would put a retention on the one kind of node whose purpose is keeping -// what it would prune. -func TestTheArchiveRetentionDepartsFromWhatTheCommandWrites(t *testing.T) { - live := config.DefaultStateStoreConfig() +// point that loses it. So the departures are intended, and the set is held rather than described. A row that +// stops departing fails, which is the day that row should be deleted. A row that starts departing fails too, +// so a rule added to this section has to account for what the command does with it. +// +// What this cannot see is the command. This package cannot import the one that renders the file, because +// that direction is the cycle, so the mode-blind side here is read from the same default the command reads +// rather than from the command's output. A change that made the command keep the rules would leave this +// green, and the test under cmd/seid/cmd is the half that fails then. +func TestTheseDeparturesFromWhatTheCommandWritesAreTheRecordedOnes(t *testing.T) { + // The mode-blind values the command's own field carries, which is what reaches a generated file. + blind := config.DefaultStateStoreConfig() - // What the command renders for an archive node: the mode rules are applied to the server - // configuration, and then the type it renders fills its own state store field from the mode-blind - // default, which is what reaches the file. - written := live.KeepRecent - if written == 0 { - t.Fatalf("the mode-blind default retention is already zero, so this departure measures nothing " + - "and the comparison below holds for any declaration") + // Every departure, by key and mode, with what each side says. + recorded := map[string]map[registry.Mode][2]any{ + FlagSSKeepRecent: { + registry.ModeArchive: {0, blind.KeepRecent}, + }, + FlagSSEnable: { + registry.ModeValidator: {false, blind.Enable}, + registry.ModeSeed: {false, blind.Enable}, + }, } - resolved, err := registry.Resolve(registry.ModeArchive, registry.Sources{}) - if err != nil { - t.Fatalf("%v", err) + measured := map[string]map[registry.Mode]bool{} + for _, mode := range registry.Modes() { + resolved, err := registry.Resolve(mode, registry.Sources{}) + if err != nil { + t.Fatalf("Resolve(%s): %v", mode, err) + } + for _, key := range []string{FlagSSKeepRecent, FlagSSEnable} { + declared := resolved.Values[key] + written := blindValueFor(key, blind) + if declared == written { + if _, listed := recorded[key][mode]; listed { + t.Errorf("%s for %s no longer departs, both sides being %v. Take the row off, so "+ + "the record stays the set of values this section states differently from the "+ + "file the command writes", key, mode, declared) + } + continue + } + if measured[key] == nil { + measured[key] = map[registry.Mode]bool{} + } + measured[key][mode] = true + + want, listed := recorded[key][mode] + switch { + case !listed: + t.Errorf("%s for %s is declared as %v and the command writes %v, and nothing records "+ + "that", key, mode, declared, written) + case declared != want[0] || written != want[1]: + t.Errorf("%s for %s is recorded as declaring %v against a written %v, and declares %v "+ + "against %v", key, mode, want[0], want[1], declared, written) + } + } } - declared := resolved.Values[FlagSSKeepRecent] - if declared == written { - t.Errorf("%s resolves to %v for an archive node, which is what the command writes. Either the "+ - "command now carries the mode rule, in which case this departure and its note should go, or "+ - "this section stopped departing and an archive node is declared to prune the history it "+ - "exists to keep", FlagSSKeepRecent, declared) + for key, modes := range recorded { + if len(measured[key]) != len(modes) { + t.Errorf("%s is recorded as departing for %d modes and departs for %d", key, + len(modes), len(measured[key])) + } } - if declared != 0 { - t.Errorf("%s resolves to %v for an archive node, want zero. The mode rule keeps every version, "+ - "and departing from the command is only defensible while this states that rule", - FlagSSKeepRecent, declared) +} + +// blindValueFor returns the value the command's own state store field carries for a key. +func blindValueFor(key string, blind config.StateStoreConfig) any { + switch key { + case FlagSSKeepRecent: + return blind.KeepRecent + case FlagSSEnable: + return blind.Enable } + return nil } diff --git a/cmd/seid/cmd/state_store_departure_test.go b/cmd/seid/cmd/state_store_departure_test.go new file mode 100644 index 0000000000..86c1d882bf --- /dev/null +++ b/cmd/seid/cmd/state_store_departure_test.go @@ -0,0 +1,65 @@ +package cmd + +import ( + "testing" + + "github.com/sei-protocol/sei-chain/app/params" + evmrpcconfig "github.com/sei-protocol/sei-chain/evmrpc/config" + srvconfig "github.com/sei-protocol/sei-chain/sei-cosmos/server/config" + seidbconfig "github.com/sei-protocol/sei-chain/sei-db/config" +) + +// TestTheRenderedConfigStillDiscardsTheStateStoreRules is the half of a departure that lives here. +// +// A section elsewhere declares what this command writes for a kind of node, and it departs from this command +// on three values because of one thing this file does: the type rendered here declares a state store field +// of its own and fills it from the mode-blind default, so every rule applied to the server configuration is +// applied and then thrown away. +// +// That section cannot measure this. It cannot import this package, because that direction is the cycle, so +// it compares against the same default this file reads rather than against what this file produces. A change +// that made this keep the rules would leave it green and its record stale. This is the test that fails then. +func TestTheRenderedConfigStillDiscardsTheStateStoreRules(t *testing.T) { + blind := seidbconfig.DefaultStateStoreConfig() + + for _, tc := range []struct { + mode params.NodeMode + named string + ruled any + got func(CustomAppConfig) any + blind any + }{ + {params.NodeModeValidator, "ss-enable", false, + func(c CustomAppConfig) any { return c.StateStore.Enable }, blind.Enable}, + {params.NodeModeSeed, "ss-enable", false, + func(c CustomAppConfig) any { return c.StateStore.Enable }, blind.Enable}, + {params.NodeModeArchive, "ss-keep-recent", 0, + func(c CustomAppConfig) any { return c.StateStore.KeepRecent }, blind.KeepRecent}, + } { + base := srvconfig.DefaultConfig() + params.SetAppConfigByMode(base, tc.mode) + + // What the rules put on the server configuration, before this file renders anything. + switch tc.named { + case "ss-enable": + if base.StateStore.Enable != tc.ruled { + t.Errorf("the rules for %s no longer set %s to %v, so this row measures nothing", + tc.mode, tc.named, tc.ruled) + continue + } + case "ss-keep-recent": + if base.StateStore.KeepRecent != tc.ruled { + t.Errorf("the rules for %s no longer set %s to %v, so this row measures nothing", + tc.mode, tc.named, tc.ruled) + continue + } + } + + rendered := NewCustomAppConfig(base, evmrpcconfig.DefaultConfig) + if got := tc.got(rendered); got != tc.blind { + t.Errorf("%s for %s renders %v, and the mode-blind default is %v. This command has stopped "+ + "discarding the rules, so the section that declares these values no longer departs from "+ + "it and its record should go", tc.named, tc.mode, got, tc.blind) + } + } +}