diff --git a/cmd/seid/cmd/registry_sections_test.go b/cmd/seid/cmd/registry_sections_test.go index e0dc016ae4..277f1a048d 100644 --- a/cmd/seid/cmd/registry_sections_test.go +++ b/cmd/seid/cmd/registry_sections_test.go @@ -1,9 +1,18 @@ package cmd import ( + "strings" "testing" "github.com/sei-protocol/sei-chain/config/registry" + + // Linked so the checks below see the whole key space rather than the part this binary already + // reached. A section registers during its own package's initialisation, and these two register the + // node's and the application's configuration files; nothing else imports either yet, so without + // these the cross-section refusals are evaluated over a set neither file is in. A test import + // rather than a production one, because nothing consumes these sections yet. + _ "github.com/sei-protocol/sei-chain/config/cosmosbase" + _ "github.com/sei-protocol/sei-chain/config/tendermintbase" ) // TestEverySectionThisBinaryDeclaresIsUsable is the check no single section can make. @@ -16,7 +25,8 @@ import ( // // This package links every section a node's configuration reaches, so asking here is asking about the set // a node actually gets. Nothing is enumerated, so a section added later is covered without this file -// changing. +// changing, provided its package is linked: a registrar nothing imports registers in no binary, and the +// blank imports above are what keep that claim true. func TestEverySectionThisBinaryDeclaresIsUsable(t *testing.T) { for _, defect := range registry.Defects() { t.Errorf("%s was refused, so none of its keys is declared: %v", defect.Section, defect.Err) @@ -49,3 +59,42 @@ func TestEveryDeclaredKeyResolvesForEveryMode(t *testing.T) { } } } + +// TestNoRootKeyCollidesWithAnotherSectionsName covers the collision the registry does not refuse. +// +// A key at the top of a file that is also a section's name cannot be written: no file holds both a value +// for that name and a table under it, so one of the two settings is unreachable and nothing says which. +// +// Asked here because it is a property of the whole key space and of no single section. Two packages +// declare keys at a root, one per configuration file, and a check inside either sees only its own. +// +// The two files are the reason a match is reported rather than failed outright: the key space spans them +// both, and a root key in one file against a table name in the other is two settings an operator can +// write, in two places, with no collision at all. So this names the pair and which file each came from, +// and the judgement is a human's. +func TestNoRootKeyCollidesWithAnotherSectionsName(t *testing.T) { + tables := map[string]bool{} + var roots []registry.Section + for _, s := range registry.Sections() { + if s.Prefix != "" { + tables[s.Prefix] = true + continue + } + roots = append(roots, s) + } + if len(roots) == 0 { + t.Fatal("no section declares keys at the root of a file, so this check holds for an empty set") + } + for _, root := range roots { + for _, key := range root.Keys { + if strings.Contains(key, ".") { + continue + } + if tables[key] { + t.Errorf("%s declares %q at the root of its file and a section takes that same name for "+ + "a table. Within one file only one of the two is writable; across the two files "+ + "both are, and which this is has to be decided rather than assumed", root.Name, key) + } + } + } +} diff --git a/config/tendermintbase/tendermintbase.go b/config/tendermintbase/tendermintbase.go index 2e9350c9a2..6d74c3e513 100644 --- a/config/tendermintbase/tendermintbase.go +++ b/config/tendermintbase/tendermintbase.go @@ -18,8 +18,43 @@ const ( InstrumentationSectionName = "instrumentation" PrivValidatorSectionName = "priv-validator" SelfRemediationSectionName = "self-remediation" + + // RootSectionName identifies the keys that sit at the top of the file with no table of their own. The + // name is for lookups and reports and is not part of any key. + RootSectionName = "node_base" ) +// notWritableInThisFile are root paths this section does not declare. +// +// Neither is a setting an operator can usefully write here. The home directory is where this file is found, +// so a value inside it would be the file naming its own location, and the command line already carries it. +// The node mode is the same fact the file states at the top under its own name, and declaring a second +// spelling would let the two disagree, with the resolution answering for one and the node reading the +// other. +var notWritableInThisFile = []string{"home", "mode"} + +// removedFromTheNode are the root paths this section does not declare because nothing reads them. +// +// The node marks each field deprecated and nothing in the tree reads any of them: out-of-process ABCI +// was removed, and so was peer filtering through it. The generated file writes twelve root keys and none +// of these three. Two of them state an affirmative value in the node's own defaults, an address and a +// transport name, so declaring them put settings for a transport the binary no longer has into the key +// space a writer would render from. +var removedFromTheNode = []string{"proxy-app", "abci", "filter-peers"} + +// nodeRootSchema declares the keys that sit at the root of the node's configuration file. +// +// The node's own top-level type carries these and the nine tables both, so declaring against it directly +// would declare every table's keys a second time. This squashes the same base group that type squashes, so +// fourteen spellings still come from the node's own tags, and restates only the two fields it holds beside +// that group. A test holds those two against it. +type nodeRootSchema struct { + tmcfg.BaseConfig `mapstructure:",squash"` + + AutobahnConfigFile string `mapstructure:"autobahn-config-file"` + HashVaultDisabledUnsafe bool `mapstructure:"hash-vault-disabled-unsafe"` +} + // removedSettings are the consensus paths this section does not declare. // // Every one is a setting the node removed, and the struct marks each field deprecated. The fields are kept @@ -84,24 +119,49 @@ const fixedForEveryNode = "namespace" // The keys derive from the struct's mapstructure tags, which is what the node's reader decodes through, so // a key here is a key that reader resolves rather than a second spelling of it. func init() { - registry.RegisterSectionExcluding(P2PSectionName, &tmcfg.P2PConfig{}, p2pDefaults, + declareSection(P2PSectionName, &tmcfg.P2PConfig{}, p2pDefaults, filledFromTheCommandLine, derivedFromTheConnectionLimit, readByNothing) - registry.RegisterSectionExcluding(RPCSectionName, &tmcfg.RPCConfig{}, rpcDefaults, + declareSection(RPCSectionName, &tmcfg.RPCConfig{}, rpcDefaults, filledFromTheCommandLine) - registry.RegisterSectionExcluding(ConsensusSectionName, &tmcfg.ConsensusConfig{}, consensusDefaults, + declareSection(ConsensusSectionName, &tmcfg.ConsensusConfig{}, consensusDefaults, append([]string{filledFromTheCommandLine}, removedSettings...)...) - registry.RegisterSectionExcluding(MempoolSectionName, &tmcfg.MempoolConfig{}, mempoolDefaults, + declareSection(MempoolSectionName, &tmcfg.MempoolConfig{}, mempoolDefaults, append([]string{filledFromTheCommandLine}, neverReachTheMempool...)...) - registry.RegisterSection(StateSyncSectionName, &tmcfg.StateSyncConfig{}, stateSyncDefaults) - registry.RegisterSection(TxIndexSectionName, &tmcfg.TxIndexConfig{}, txIndexDefaults) - registry.RegisterSectionExcluding(InstrumentationSectionName, &tmcfg.InstrumentationConfig{}, + declareSection(StateSyncSectionName, &tmcfg.StateSyncConfig{}, stateSyncDefaults) + declareSection(TxIndexSectionName, &tmcfg.TxIndexConfig{}, txIndexDefaults) + declareSection(InstrumentationSectionName, &tmcfg.InstrumentationConfig{}, instrumentationDefaults, fixedForEveryNode) - registry.RegisterSectionExcluding(PrivValidatorSectionName, &tmcfg.PrivValidatorConfig{}, + declareSection(PrivValidatorSectionName, &tmcfg.PrivValidatorConfig{}, privValidatorDefaults, filledFromTheCommandLine) - registry.RegisterSection(SelfRemediationSectionName, &tmcfg.SelfRemediationConfig{}, + declareSection(SelfRemediationSectionName, &tmcfg.SelfRemediationConfig{}, selfRemediationDefaults) + declareRootKeys(RootSectionName, &nodeRootSchema{}, rootDefaults, + append(append([]string{}, notWritableInThisFile...), removedFromTheNode...)...) } +// registeredHere are the sections this package put in the registry, recorded as each one is registered. +// +// A test needs to know which sections are this package's, and a list written beside the registrations is a +// second statement of the same fact: a section registered and left off the list is one nothing here checks, +// which is the case the list exists to prevent. Recorded by the registration itself instead, so the two +// cannot disagree. +var registeredHere []string + +// declareSection registers a section and records that it belongs to this package. +func declareSection(name string, prototype any, defaults func(registry.Mode) any, excluding ...string) { + registry.RegisterSectionExcluding(name, prototype, defaults, excluding...) + registeredHere = append(registeredHere, name) +} + +// declareRootKeys registers a section whose keys sit at the root of the file, and records it the same way. +func declareRootKeys(name string, prototype any, defaults func(registry.Mode) any, excluding ...string) { + registry.RegisterRootKeysExcluding(name, prototype, defaults, excluding...) + registeredHere = append(registeredHere, name) +} + +// SectionsRegisteredHere returns the sections this package registered, for the tests that hold the set. +func SectionsRegisteredHere() []string { return append([]string(nil), registeredHere...) } + // forMode is the configuration the seid init command writes for a kind of node. // // Pinned to that command's own pipeline rather than restated here: the defaults the node's package @@ -151,9 +211,11 @@ const readByNothing = "test-dial-fail" // Declared rather than excluded, because the generated file writes each one into every node's // configuration, and a key this space refuses is a key an operator's own file reports as unknown. What // they cost is the invariant on forMode: for these, a declared value is not what a generated file -// carries. The peer seeds come from the chain identifier, which is a runtime input and not a node kind, -// so no answer keyed on mode can be right. A writer takes the value from the generator instead. -var filledByTheGenerator = []string{P2PSectionName + ".bootstrap-peers"} +// carries. The peer seeds come from the chain identifier and the node name is a required argument to the +// command, both runtime inputs and neither a node kind, so no answer keyed on mode can be right. The node +// name is the sharper of the two: its declared value is the hostname of whatever machine is asking, so +// this key resolves differently on every host. A writer takes both from the generator instead. +var filledByTheGenerator = []string{P2PSectionName + ".bootstrap-peers", "moniker"} // p2pDefaults is what a generated file carries for the peer-to-peer section. // @@ -212,6 +274,19 @@ func instrumentationDefaults(mode registry.Mode) any { return *forMode(mode).Ins // not make. func privValidatorDefaults(mode registry.Mode) any { return *forMode(mode).PrivValidator } +// rootDefaults is what a generated file carries at the top of the node's configuration file. +// +// The same values for every mode. These name where a node keeps its data and how it logs, and nothing in +// the binary makes either follow from what kind of node is asking. +func rootDefaults(mode registry.Mode) any { + live := forMode(mode) + return nodeRootSchema{ + BaseConfig: live.BaseConfig, + AutobahnConfigFile: live.AutobahnConfigFile, + HashVaultDisabledUnsafe: live.HashVaultDisabledUnsafe, + } +} + // selfRemediationDefaults is what a generated file carries for the self remediation section. // // The same values for every mode. These are the thresholds at which a node restarts itself, and each one diff --git a/config/tendermintbase/tendermintbase_test.go b/config/tendermintbase/tendermintbase_test.go index c975118ee6..e1521a8735 100644 --- a/config/tendermintbase/tendermintbase_test.go +++ b/config/tendermintbase/tendermintbase_test.go @@ -5,6 +5,7 @@ import ( "go/ast" "go/parser" "go/token" + "os" "reflect" "slices" "sort" @@ -59,22 +60,26 @@ var whatVariesByNodeKind = map[string]map[registry.Mode]string{ // declaredSections are the sections this package registers, so a test walks the set rather than a list that // has to be extended alongside it. -func declaredSections() []string { - return []string{ - P2PSectionName, RPCSectionName, ConsensusSectionName, MempoolSectionName, - StateSyncSectionName, TxIndexSectionName, InstrumentationSectionName, - PrivValidatorSectionName, SelfRemediationSectionName, - } -} +func declaredSections() []string { return SectionsRegisteredHere() } -// ours reports whether a key belongs to a section this package registers. -func ours(key string) bool { +// declaredKeys are the keys the sections this package registers declare. +// +// Read out of the registry rather than matched against the section names as prefixes, because a section +// whose keys sit at the root of the file has no prefix for a match to find. The prefix form covered a +// hundred and ten of the hundred and twenty-four keys and was blind to the whole root section, so the +// claim that those fourteen answer the same for every node kind was measured by nothing. It also assumed +// a section's name and its prefix are the same string, which is a thing the registry keeps separate. +func declaredKeys(t *testing.T) []string { + t.Helper() + var out []string for _, name := range declaredSections() { - if strings.HasPrefix(key, name+".") { - return true + registered, ok := registry.Lookup(name) + if !ok { + t.Fatalf("%s is not registered; Defects: %v", name, registry.Defects()) } + out = append(out, registered.Keys...) } - return false + return out } // TestWhatVariesByNodeKindIsTheRecordedSet measures the mode rules through the declared values. @@ -93,10 +98,7 @@ func TestWhatVariesByNodeKindIsTheRecordedSet(t *testing.T) { } var measured []string - for key := range byMode[registry.ModeValidator] { - if !ours(key) { - continue - } + for _, key := range declaredKeys(t) { seen := map[string]bool{} for _, mode := range registry.Modes() { seen[fmt.Sprint(byMode[mode][key])] = true @@ -132,9 +134,9 @@ func TestWhatVariesByNodeKindIsTheRecordedSet(t *testing.T) { // declaredAgainst pairs each section with the struct it declares against and how many of that struct's // paths it leaves out. // -// One table, read by the count check and by the deprecation check alike, so a section added here is -// covered by both without either being extended on its own. A section absent from it is a section whose -// keys nothing measures. +// A section absent from it is a section whose key count nothing measures. The root section is not here +// because it declares against a local schema whose squashed group counts as one tagged field, so the +// arithmetic this table does would be wrong for it; a test of its own holds that section. var declaredAgainst = []struct { section string proto any @@ -161,15 +163,21 @@ var declaredAgainst = []struct { // Read from the source, because the node marks a field two ways and a tag walk sees only the prefix on // the field name. The standard comment above a field is how most of them are marked. func TestNoDeclaredKeyNamesADeprecatedField(t *testing.T) { - for _, tc := range declaredAgainst { - registered, ok := registry.Lookup(tc.section) + for _, name := range declaredSections() { + registered, ok := registry.Lookup(name) if !ok { - t.Errorf("%s is not registered; Defects: %v", tc.section, registry.Defects()) + t.Errorf("%s is not registered; Defects: %v", name, registry.Defects()) continue } - marked := deprecatedPaths(t, reflect.TypeOf(tc.proto).Elem()) + proto, named := markedIn[name] + if !named { + t.Errorf("%s declares keys and no struct is named as carrying their markings, so nothing "+ + "measures whether any of them is a setting the node removed", name) + continue + } + marked := deprecatedPaths(t, reflect.TypeOf(proto).Elem()) for _, key := range registered.Keys { - rel := strings.TrimPrefix(key, tc.section+".") + rel := strings.TrimPrefix(key, name+".") if marked[rel] { t.Errorf("%s names a field the node marks deprecated, so it offers a setting a written "+ "value cannot change", key) @@ -178,6 +186,27 @@ func TestNoDeclaredKeyNamesADeprecatedField(t *testing.T) { } } +// markedIn pairs each section with the node's own struct whose deprecation markings apply to its keys. +// +// Named per section rather than taken from what the section declares against, because the root one +// declares against a local schema that squashes the node's base group. The markings that apply to those +// keys live on that group, and a schema this package wrote carries none of them. +// +// Walked from the registered set rather than from this map, so a section added without a row here fails +// instead of skipping the check. +var markedIn = map[string]any{ + P2PSectionName: &tmcfg.P2PConfig{}, + RPCSectionName: &tmcfg.RPCConfig{}, + ConsensusSectionName: &tmcfg.ConsensusConfig{}, + MempoolSectionName: &tmcfg.MempoolConfig{}, + StateSyncSectionName: &tmcfg.StateSyncConfig{}, + TxIndexSectionName: &tmcfg.TxIndexConfig{}, + InstrumentationSectionName: &tmcfg.InstrumentationConfig{}, + PrivValidatorSectionName: &tmcfg.PrivValidatorConfig{}, + SelfRemediationSectionName: &tmcfg.SelfRemediationConfig{}, + RootSectionName: &tmcfg.BaseConfig{}, +} + // TestTheDeclaredKeysAreTheOnesTheReaderDecodes holds the declaration to the struct the node decodes into. // // Derived from that struct's own tags, so this asserts the count rather than the spelling: a renamed tag @@ -578,6 +607,94 @@ func TestEverySectionThisPackageRegistersIsUsable(t *testing.T) { } } +// TestTheRootSchemaCarriesWhatTheNodesOwnTypeCarries closes the one place a spelling is restated. +// +// The root section declares against a schema rather than the node's top-level type, because that type +// carries the nine tables as well and declaring against it would declare their keys twice. The schema +// squashes the same base group, so fourteen keys still derive from the node's own tags, and it restates two +// fields by hand. This holds those two to the type they came from: name, tag and type each, and the count of +// non-table fields, so a third one appearing there fails here rather than going undeclared. +func TestTheRootSchemaCarriesWhatTheNodesOwnTypeCarries(t *testing.T) { + live := reflect.TypeOf(tmcfg.Config{}) + schema := reflect.TypeOf(nodeRootSchema{}) + + var restated int + for i := 0; i < live.NumField(); i++ { + f := live.Field(i) + tag, ok := f.Tag.Lookup("mapstructure") + if !ok { + continue + } + // The squashed base group and the nine tables are not restated; everything else is. + if strings.Contains(tag, "squash") { + continue + } + if f.Type.Kind() == reflect.Pointer && f.Type.Elem().Kind() == reflect.Struct { + continue + } + restated++ + + got, found := schema.FieldByName(f.Name) + if !found { + t.Errorf("the node's type carries %s (%s) at its root and the schema does not, so the key is "+ + "not declared at all", f.Name, tag) + continue + } + if want := got.Tag.Get("mapstructure"); want != tag { + t.Errorf("%s is tagged %q on the node's type and %q here, so the declared key is not the one "+ + "the reader decodes", f.Name, tag, want) + } + if got.Type != f.Type { + t.Errorf("%s is a %s on the node's type and a %s here, so the declared value has a shape the "+ + "reader does not read", f.Name, f.Type, got.Type) + } + } + + // The schema holds the squashed group plus exactly the restated fields, so a field added here that the + // node's type does not carry fails too. + if want := restated + 1; schema.NumField() != want { + t.Errorf("the schema carries %d fields and the node's type has %d root fields beside the squashed "+ + "group, so %d were expected", schema.NumField(), restated, want) + } +} + +// TestTheRootPathsLeftOutAreTheOnesTheFileAlreadyStates names why two root paths are not declared. +// +// The home directory is where this file is found, so a value inside it would be the file naming its own +// location, and the command line already carries it. The node mode is the fact the file states at the top +// under its own name, and a second spelling would let the two disagree: the resolution answers for one and +// the node reads the other. +// +// Written out here rather than read from the list the registration uses. Comparing that list against itself +// agrees however it changes, so a path dropped from it would leave this passing while the key became +// declared. +func TestTheRootPathsLeftOutAreTheOnesTheFileAlreadyStates(t *testing.T) { + registered, ok := registry.Lookup(RootSectionName) + if !ok { + t.Fatalf("%s is not registered; Defects: %v", RootSectionName, registry.Defects()) + } + declared := map[string]bool{} + for _, key := range registered.Keys { + declared[key] = true + } + for key, why := range map[string]string{ + "home": "the file's own location, which the command line carries", + "mode": "the fact the file states at the top under its own name", + "proxy-app": "the address of an out-of-process application the node no longer runs", + "abci": "the transport to that application", + "filter-peers": "peer filtering through that application", + } { + if declared[key] { + t.Errorf("%q is declared at the root and it is %s, so the key space offers a setting that "+ + "is either already settled or reaches nothing", key, why) + } + } + if want := len(notWritableInThisFile) + len(removedFromTheNode); len(registered.Excluded) != want { + t.Errorf("the root section excludes %v and %d paths were expected, being the two the file settles "+ + "elsewhere and the three the node removed", registered.Excluded, want) + } +} + // TestNoSectionDeclaresTheRootDirectory covers a field five of these sections carry. // // Each holds a root directory tagged the same as the key at the top of the file, and the node fills every @@ -609,11 +726,36 @@ func TestNoSectionDeclaresTheRootDirectory(t *testing.T) { // generator supplies is something the declaration does not state. A key that stopped diverging fails // too, which is what makes this a record of the class rather than a note about two keys. func TestWhatTheGeneratorFillsIsNotWhatTheDeclarationStates(t *testing.T) { - // The generator's own source for each key, read from it rather than restated, so a change there - // moves this. A chain identifier is needed for the peer seeds because that is the input the mode - // rules do not carry, and the public networks are the ones an operator runs. - supplied := map[string]string{ - P2PSectionName + ".bootstrap-peers": seeds.BootstrapPeers("pacific-1"), + // What makes each declaration something other than what a generated file carries, checked rather + // than described. A row that stops holding means the key no longer belongs on the list. + wrongBecause := map[string]func(*testing.T, any){ + P2PSectionName + ".bootstrap-peers": func(t *testing.T, declared any) { + // The command fills these from the chain identifier, after the pipeline forMode mirrors. + // Read from the generator's own source, so a change there moves this. + supplied := seeds.BootstrapPeers("pacific-1") + if supplied == "" { + t.Error("the generator ships no seeds for a public network, so nothing fills this key " + + "and the declared empty value is what a file carries after all") + return + } + if fmt.Sprint(declared) == supplied { + t.Errorf("declared as %q, which is what the generator supplies, so this no longer "+ + "diverges", declared) + } + }, + "moniker": func(t *testing.T, declared any) { + // The command takes the node name as a required argument, so a generated file always + // carries an operator's own. What the declaration answers instead is a fact about the + // machine that asked, which is why no value keyed on mode could be right. + host, err := os.Hostname() + if err != nil { + t.Skipf("this host has no name to compare against: %v", err) + } + if fmt.Sprint(declared) != host { + t.Errorf("declared as %q where this host is %q, so the declaration no longer answers a "+ + "host fact and this key may belong off the list", declared, host) + } + }, } resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{}) @@ -621,10 +763,10 @@ func TestWhatTheGeneratorFillsIsNotWhatTheDeclarationStates(t *testing.T) { t.Fatalf("Resolve: %v", err) } for _, key := range filledByTheGenerator { - want, named := supplied[key] + check, named := wrongBecause[key] if !named { - t.Errorf("%s is on the list and no writer is named for it, so nothing measures that the "+ - "generator fills it", key) + t.Errorf("%s is on the list and nothing says what makes its declared value wrong, so the "+ + "row records no divergence", key) continue } declared, declares := resolved.Values[key] @@ -633,16 +775,9 @@ func TestWhatTheGeneratorFillsIsNotWhatTheDeclarationStates(t *testing.T) { "belongs in the key space", key) continue } - if want == "" { - t.Errorf("%s names a writer that supplies nothing, so the row records no divergence", key) - continue - } - if fmt.Sprint(declared) == want { - t.Errorf("%s is declared as %q and the generator supplies the same, so it no longer "+ - "diverges. Take it off the list, and off forMode's exception", declared, want) - } + t.Run(key, func(t *testing.T) { check(t, declared) }) } - if len(supplied) != len(filledByTheGenerator) { - t.Errorf("%d writers are named and %d keys are listed", len(supplied), len(filledByTheGenerator)) + if len(wrongBecause) != len(filledByTheGenerator) { + t.Errorf("%d reasons are stated and %d keys are listed", len(wrongBecause), len(filledByTheGenerator)) } }