From 467ceec26edd5a3e068d5f33c3f52300b042bf23 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 20 Aug 2026 14:19:11 -0700 Subject: [PATCH 1/4] 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/4] 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/4] 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/4] 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) + } +}