From a1f9d49cb4e6c1f95176a21b871566cf77dbf299 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 24 Aug 2026 11:54:02 -0700 Subject: [PATCH 1/3] feat(config): declare the consensus and mempool sections Neither varies by node kind. How long a node waits at each step of a round has to agree across the validator set for the set to reach a decision, and what a node holds before a transaction is decided is a limit on its own memory. The consensus struct carries fifteen fields the node removed as settings, and it marks each one deprecated. They are excluded: declaring one would offer a key that changes nothing about how the node runs. So the section declares nine of its twenty-four paths. The reader has a check that names the removed settings an operator wrote, and it reaches eight of the fifteen. Six are durations or booleans, where a written zero and an unwritten field hold the same value, so no check can tell them apart. One more it omits. Nothing calls the check in any case. A test holds which eight it reaches, so making it complete fails rather than leaving the count stale, and a second test holds every exclusion to the struct's own deprecated marking rather than to that check. Co-Authored-By: Claude Opus 5 (1M context) --- config/tendermintbase/tendermintbase.go | 50 ++++++- config/tendermintbase/tendermintbase_test.go | 134 +++++++++++++++++++ 2 files changed, 182 insertions(+), 2 deletions(-) diff --git a/config/tendermintbase/tendermintbase.go b/config/tendermintbase/tendermintbase.go index 60f39177a6..43a69876b4 100644 --- a/config/tendermintbase/tendermintbase.go +++ b/config/tendermintbase/tendermintbase.go @@ -8,10 +8,40 @@ import ( // The names these sections have in the configuration key space. const ( - P2PSectionName = "p2p" - RPCSectionName = "rpc" + P2PSectionName = "p2p" + RPCSectionName = "rpc" + ConsensusSectionName = "consensus" + MempoolSectionName = "mempool" ) +// 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 +// so a decode can tell that an operator set one, and declaring any of them would offer a key that changes +// nothing about how the node runs. +// +// The reader has a check that names the removed settings an operator wrote, and it reaches eight of these +// fifteen. Six are durations or booleans, where a written zero and an unwritten field are the same value, +// so no check can tell them apart. One more the check simply omits. Nothing calls the check in any case, so +// leaving these out of the file is what an operator actually gets. +var removedSettings = []string{ + "unsafe-overrides-enabled", + "unsafe-propose-timeout-override", + "unsafe-propose-timeout-delta-override", + "unsafe-vote-timeout-override", + "unsafe-vote-timeout-delta-override", + "unsafe-commit-timeout-override", + "unsafe-bypass-commit-timeout-override", + "timeout-propose", + "timeout-propose-delta", + "timeout-prevote", + "timeout-prevote-delta", + "timeout-precommit", + "timeout-precommit-delta", + "timeout-commit", + "skip-timeout-commit", +} + // Registration puts these sections in the configuration registry. // // Neither the package that defines these settings nor the package that decides them can register them. The @@ -25,6 +55,9 @@ func init() { registry.RegisterSectionExcluding(P2PSectionName, &tmcfg.P2PConfig{}, p2pDefaults, "max-outbound-connections") registry.RegisterSection(RPCSectionName, &tmcfg.RPCConfig{}, rpcDefaults) + registry.RegisterSectionExcluding(ConsensusSectionName, &tmcfg.ConsensusConfig{}, consensusDefaults, + removedSettings...) + registry.RegisterSection(MempoolSectionName, &tmcfg.MempoolConfig{}, mempoolDefaults) } // forMode is the configuration the seid init command writes for a kind of node. @@ -60,3 +93,16 @@ func p2pDefaults(mode registry.Mode) any { return *forMode(mode).P2P } // Answered per mode, for the listen address alone: a node that serves queries binds one and a validator // does not. func rpcDefaults(mode registry.Mode) any { return *forMode(mode).RPC } + +// consensusDefaults is what a generated file carries for the consensus section. +// +// The same values for every mode. How long a node waits at each step of a round has to agree across the +// validator set for the set to reach a decision, so a value that followed from the kind of node asking +// would be this package proposing that they disagree. +func consensusDefaults(mode registry.Mode) any { return *forMode(mode).Consensus } + +// mempoolDefaults is what a generated file carries for the mempool section. +// +// The same values for every mode. What a node holds before a transaction is decided is a limit on its own +// memory and bandwidth, and nothing in the binary makes one follow from what kind of node is asking. +func mempoolDefaults(mode registry.Mode) any { return *forMode(mode).Mempool } diff --git a/config/tendermintbase/tendermintbase_test.go b/config/tendermintbase/tendermintbase_test.go index 974e557109..546ea98ca1 100644 --- a/config/tendermintbase/tendermintbase_test.go +++ b/config/tendermintbase/tendermintbase_test.go @@ -9,6 +9,7 @@ import ( "github.com/sei-protocol/sei-chain/config/registry" tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" + "github.com/spf13/viper" ) // whatVariesByNodeKind is every key these sections answer differently depending on the kind of node. @@ -107,6 +108,8 @@ func TestTheDeclaredKeysAreTheOnesTheReaderDecodes(t *testing.T) { }{ {P2PSectionName, &tmcfg.P2PConfig{}, 1}, {RPCSectionName, &tmcfg.RPCConfig{}, 0}, + {ConsensusSectionName, &tmcfg.ConsensusConfig{}, len(removedSettings)}, + {MempoolSectionName, &tmcfg.MempoolConfig{}, 0}, } { registered, ok := registry.Lookup(tc.section) if !ok { @@ -184,3 +187,134 @@ func hasOpt(opts []string, want string) bool { } return false } + +// warningCannotName are the removed settings the reader's own deprecation check does not report. +// +// Six are durations or booleans, where a written zero and an unwritten field hold the same value, so the +// check has nothing to test. The seventh is a pointer the check could name and does not. Recorded so that +// making the check complete fails here rather than leaving a sentence quietly stale. +var warningCannotName = map[string]bool{ + "unsafe-overrides-enabled": true, + "unsafe-propose-timeout-override": true, + "unsafe-propose-timeout-delta-override": true, + "unsafe-vote-timeout-override": true, + "unsafe-vote-timeout-delta-override": true, + "unsafe-commit-timeout-override": true, + "unsafe-bypass-commit-timeout-override": true, +} + +// TestTheExcludedConsensusPathsAreTheRemovedOnes ties the exclusion list to the struct's own marking. +// +// Each excluded path has to name a field the struct itself marks as deprecated, and no declared path may. +// The struct is the authority rather than the deprecation warning, because that warning is incomplete: it +// names eight of these fifteen. +// +// Nothing in the binary calls the warning either, so an operator who still has one of these in their file +// gets no error and no warning and the value is quietly ignored. That is what the exclusion is carrying. It +// keeps the key out of the new format rather than relying on a diagnostic that never runs. +func TestTheExcludedConsensusPathsAreTheRemovedOnes(t *testing.T) { + registered, ok := registry.Lookup(ConsensusSectionName) + if !ok { + t.Fatalf("%s is not registered; Defects: %v", ConsensusSectionName, registry.Defects()) + } + marked := deprecatedPaths(reflect.TypeOf(tmcfg.ConsensusConfig{})) + + excluded := map[string]bool{} + for _, key := range registered.Excluded { + excluded[key] = true + } + if len(excluded) != len(removedSettings) { + t.Errorf("the section excludes %d paths and %d are listed", len(excluded), len(removedSettings)) + } + for _, rel := range removedSettings { + key := ConsensusSectionName + "." + rel + if !excluded[key] { + t.Errorf("%s is listed as removed and the section does not exclude it", key) + } + if !marked[rel] { + t.Errorf("%s is excluded as a removed setting and the struct does not mark its field "+ + "deprecated, so it is a setting an operator can use and belongs declared", key) + } + delete(marked, rel) + } + for rel := range marked { + t.Errorf("%s.%s names a field the struct marks deprecated and the section declares it", + ConsensusSectionName, rel) + } + + for _, key := range registered.Keys { + rel := strings.TrimPrefix(key, ConsensusSectionName+".") + if err := writtenThenChecked(t, rel); err != nil { + t.Errorf("%s is declared and the deprecation warning names it: %v", key, err) + } + } +} + +// TestTheDeprecationWarningReachesTheRecordedSubset measures the gap in the reader's own check. +// +// Eight of the fifteen removed settings make the warning name them and seven cannot, so an operator who +// wrote one of those seven would get nothing back even from a caller that ran the check. Held so that +// making the check complete shows up as a failure rather than as a sentence going quietly stale. +func TestTheDeprecationWarningReachesTheRecordedSubset(t *testing.T) { + for _, rel := range removedSettings { + err := writtenThenChecked(t, rel) + switch { + case warningCannotName[rel] && err != nil: + t.Errorf("the warning now names %s, so it reaches one more removed setting and the row "+ + "should go", rel) + case !warningCannotName[rel] && err == nil: + t.Errorf("the warning no longer names %s, so one more removed setting is now silent", rel) + } + } +} + +// deprecatedPaths returns the mapstructure names of the fields a struct marks deprecated. +func deprecatedPaths(t reflect.Type) map[string]bool { + out := map[string]bool{} + for i := 0; i < t.NumField(); i++ { + f := t.Field(i) + if !strings.HasPrefix(f.Name, "Deprecated") { + continue + } + if tag, ok := f.Tag.Lookup("mapstructure"); ok { + out[strings.Split(tag, ",")[0]] = true + } + } + return out +} + +// writtenThenChecked writes one consensus path into a configuration and returns what the reader's +// deprecation warning says about it. +// +// Written and decoded rather than assigned, because a removed setting is detected by its field being +// non-nil after a decode, and assigning it directly would skip the step under test. +func writtenThenChecked(t *testing.T, rel string) error { + t.Helper() + conf := tmcfg.DefaultConfig() + v := viper.New() + v.SetConfigType("toml") + body := "[consensus]\n" + rel + " = " + probeValueFor(rel) + "\n" + if err := v.ReadConfig(strings.NewReader(body)); err != nil { + t.Fatalf("compose a file setting %s: %v", rel, err) + } + if err := v.Unmarshal(conf); err != nil { + t.Skipf("%s does not decode from the probe value: %v", rel, err) + } + return conf.DeprecatedFieldWarning() +} + +// probeValueFor returns a written value of the right shape for a consensus path. +// +// Three shapes appear: a duration written as a string, a boolean, and a whole number. +func probeValueFor(rel string) string { + switch { + case strings.HasPrefix(rel, "skip-") || strings.HasPrefix(rel, "unsafe-") || + strings.HasPrefix(rel, "double-sign-") || strings.HasSuffix(rel, "-enabled"): + return "true" + case strings.Contains(rel, "timeout") || strings.Contains(rel, "-delta") || + strings.Contains(rel, "interval") || strings.Contains(rel, "period"): + return "\"1s\"" + default: + return "1" + } +} From 44f76d291af6fcbe86dd0ac2afe6efc86acca9f8 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Mon, 24 Aug 2026 12:15:29 -0700 Subject: [PATCH 2/3] fix(config): the consensus and mempool sections leave the root directory out Both carry the root directory field the node fills from the command line after the file is read, so both stated the empty string for it. Co-Authored-By: Claude Opus 5 (1M context) From 5fad4c52e9b823629f5d49f69aec0081ab75133b Mon Sep 17 00:00:00 2001 From: bdchatham Date: Tue, 25 Aug 2026 15:36:40 -0700 Subject: [PATCH 3/3] fix(config): a deprecated field is found by its marking, not by its name The consensus section's exclusion list was held against a predicate that read the prefix on a field's name. The node marks a deprecated field two ways: fifteen consensus fields carry that prefix, and seven more across three other structs carry the standard comment above the field instead. Reflection cannot see a comment, so those seven read as live settings. The predicate now reads the source. That is what a deprecation marking is: a sentence in the code, not a naming convention. With it, one list per section still serves both directions of the check, so no second list is needed for a field the old predicate could not see. Three keys were declared as a result of the gap. The leader election setting is documented as retained for parsing compatibility and ignored, and its declared value was the affirmative one, so an operator reading a generated file would find the behaviour named and switchable and neither is true. Two mempool pending-lifetime settings are marked dead at their destination. A third mempool key joins them for a stronger reason than a marking: the conversion into the running mempool carries thirteen of that struct's fields and none of these three, so no written value arrives anywhere. That reason names the function which would have to change for the key to matter. The check is now applied to every section rather than to consensus alone, through one table the count check reads too, so a section added to it is covered by both and the next such key fails here rather than the one already found. The probe that drives the deprecation-warning rows composed its value by matching the key's name. A name says nothing about a shape: the double-sign height is an integer whose name begins like the boolean overrides do, and it decoded only because the decoder accepts a boolean where an integer belongs. A refused shape was reported as a skip, which ends the whole loop rather than one row, so every remaining row went unmeasured while the run read as a pass. The value now comes from the field's own type and a refusal is fatal. Verified by mutation: feeding an integer field a list now raises a failure where it previously raised a skip. Co-Authored-By: Claude Opus 5 (1M context) --- config/tendermintbase/tendermintbase.go | 21 +- config/tendermintbase/tendermintbase_test.go | 237 ++++++++++++++++--- 2 files changed, 223 insertions(+), 35 deletions(-) diff --git a/config/tendermintbase/tendermintbase.go b/config/tendermintbase/tendermintbase.go index e5c66bac46..882d74bade 100644 --- a/config/tendermintbase/tendermintbase.go +++ b/config/tendermintbase/tendermintbase.go @@ -20,6 +20,11 @@ const ( // so a decode can tell that an operator set one, and declaring any of them would offer a key that changes // nothing about how the node runs. // +// The node marks them two ways. Most carry the prefix on the field name; the leader election one carries +// the standard comment instead, which is why it went on being declared as a settable key. Its declared +// value was the affirmative one, so an operator reading a generated file would find the behaviour named +// and switchable and neither is true. +// // The reader has a check that names the removed settings an operator wrote, and it reaches eight of these // fifteen. Six are durations or booleans, where a written zero and an unwritten field are the same value, // so no check can tell them apart. One more the check simply omits. Nothing calls the check in any case, so @@ -40,6 +45,20 @@ var removedSettings = []string{ "timeout-precommit-delta", "timeout-commit", "skip-timeout-commit", + "stateless-leader-election", +} + +// neverReachTheMempool are the mempool paths this section does not declare. +// +// The conversion into the running mempool carries thirteen of this struct's fields and none of these +// three, so no value an operator writes for them arrives anywhere. That is a stronger reason than the +// marking on the fields: it names the function that would have to change for the key to matter, where +// two of the three are also marked dead at the destination and the third carries only a note about an +// upstream issue. Declaring any of them would offer a key that changes nothing about how the node runs. +var neverReachTheMempool = []string{ + "max-batch-bytes", + "pending-ttl-duration", + "pending-ttl-num-blocks", } // Registration puts these sections in the configuration registry. @@ -59,7 +78,7 @@ func init() { registry.RegisterSectionExcluding(ConsensusSectionName, &tmcfg.ConsensusConfig{}, consensusDefaults, append([]string{filledFromTheCommandLine}, removedSettings...)...) registry.RegisterSectionExcluding(MempoolSectionName, &tmcfg.MempoolConfig{}, mempoolDefaults, - filledFromTheCommandLine) + append([]string{filledFromTheCommandLine}, neverReachTheMempool...)...) } // forMode is the configuration the seid init command writes for a kind of node. diff --git a/config/tendermintbase/tendermintbase_test.go b/config/tendermintbase/tendermintbase_test.go index e4b5ce6196..4bec01f23d 100644 --- a/config/tendermintbase/tendermintbase_test.go +++ b/config/tendermintbase/tendermintbase_test.go @@ -2,11 +2,16 @@ package tendermintbase import ( "fmt" + "go/ast" + "go/parser" + "go/token" "reflect" "slices" "sort" "strings" + "sync" "testing" + "time" "github.com/sei-protocol/sei-chain/app/seeds" "github.com/sei-protocol/sei-chain/config/registry" @@ -98,21 +103,56 @@ 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. +var declaredAgainst = []struct { + section string + proto any + exclude int +}{ + {P2PSectionName, &tmcfg.P2PConfig{}, 3}, + {RPCSectionName, &tmcfg.RPCConfig{}, 1}, + {ConsensusSectionName, &tmcfg.ConsensusConfig{}, len(removedSettings) + 1}, + {MempoolSectionName, &tmcfg.MempoolConfig{}, len(neverReachTheMempool) + 1}, +} + +// TestNoDeclaredKeyNamesADeprecatedField holds every section against its struct's own marking. +// +// A field the node marks deprecated is one a written value cannot change, so declaring it offers a key +// that reads as a setting and is not one. The consensus section had this check from the start against a +// hand-kept list; this is the same rule for every section, which is what catches the next one rather than +// the one already found. +// +// 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) + if !ok { + t.Errorf("%s is not registered; Defects: %v", tc.section, registry.Defects()) + continue + } + marked := deprecatedPaths(t, reflect.TypeOf(tc.proto).Elem()) + for _, key := range registered.Keys { + rel := strings.TrimPrefix(key, tc.section+".") + if marked[rel] { + t.Errorf("%s names a field the node marks deprecated, so it offers a setting a written "+ + "value cannot change", key) + } + } + } +} + // 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 // moves the reader and the declaration together, and there is no third statement to drift from. func TestTheDeclaredKeysAreTheOnesTheReaderDecodes(t *testing.T) { - for _, tc := range []struct { - section string - proto any - exclude int - }{ - {P2PSectionName, &tmcfg.P2PConfig{}, 3}, - {RPCSectionName, &tmcfg.RPCConfig{}, 1}, - {ConsensusSectionName, &tmcfg.ConsensusConfig{}, len(removedSettings) + 1}, - {MempoolSectionName, &tmcfg.MempoolConfig{}, 1}, - } { + for _, tc := range declaredAgainst { registered, ok := registry.Lookup(tc.section) if !ok { t.Errorf("%s is not registered; Defects: %v", tc.section, registry.Defects()) @@ -192,9 +232,9 @@ func hasOpt(opts []string, want string) bool { // warningCannotName are the removed settings the reader's own deprecation check does not report. // -// Six are durations or booleans, where a written zero and an unwritten field hold the same value, so the -// check has nothing to test. The seventh is a pointer the check could name and does not. Recorded so that -// making the check complete fails here rather than leaving a sentence quietly stale. +// Seven are durations or booleans, where a written zero and an unwritten field hold the same value, so +// the check has nothing to test. The eighth is a pointer the check could name and does not. Recorded so +// that making the check complete fails here rather than leaving a sentence quietly stale. var warningCannotName = map[string]bool{ "unsafe-overrides-enabled": true, "unsafe-propose-timeout-override": true, @@ -203,6 +243,7 @@ var warningCannotName = map[string]bool{ "unsafe-vote-timeout-delta-override": true, "unsafe-commit-timeout-override": true, "unsafe-bypass-commit-timeout-override": true, + "stateless-leader-election": true, } // TestTheExcludedConsensusPathsAreTheRemovedOnes ties the exclusion list to the struct's own marking. @@ -219,7 +260,7 @@ func TestTheExcludedConsensusPathsAreTheRemovedOnes(t *testing.T) { if !ok { t.Fatalf("%s is not registered; Defects: %v", ConsensusSectionName, registry.Defects()) } - marked := deprecatedPaths(reflect.TypeOf(tmcfg.ConsensusConfig{})) + marked := deprecatedPaths(t, reflect.TypeOf(tmcfg.ConsensusConfig{})) excluded := map[string]bool{} for _, key := range registered.Excluded { @@ -272,18 +313,103 @@ func TestTheDeprecationWarningReachesTheRecordedSubset(t *testing.T) { } // deprecatedPaths returns the mapstructure names of the fields a struct marks deprecated. -func deprecatedPaths(t reflect.Type) map[string]bool { - out := map[string]bool{} - for i := 0; i < t.NumField(); i++ { - f := t.Field(i) - if !strings.HasPrefix(f.Name, "Deprecated") { - continue - } - if tag, ok := f.Tag.Lookup("mapstructure"); ok { - out[strings.Split(tag, ",")[0]] = true +func deprecatedPaths(t *testing.T, typ reflect.Type) map[string]bool { + t.Helper() + marked, ok := deprecatedFields(t)[typ.Name()] + if !ok { + t.Fatalf("%s was not found in the node's configuration source, so nothing measures which of "+ + "its fields are deprecated", typ.Name()) + } + return marked +} + +// deprecatedFields reports, per struct name, the mapstructure key of every field the node marks +// deprecated. +// +// Read from the source rather than through reflection, because the node marks a field two ways and +// reflection can only see one of them. Fifteen consensus fields carry a Deprecated prefix on the name, +// which a tag walk finds. Seven more across three other structs are marked the standard way, by a +// comment above the field, which no tag carries. A predicate that saw only the names reported those +// seven as live settings, so each was declared as a key an operator could set and none of them does +// anything. +// +// Parsed once and cached, because every section that asks pays for the whole package otherwise. +func deprecatedFields(t *testing.T) map[string]map[string]bool { + t.Helper() + deprecatedOnce.Do(func() { + deprecatedByStruct, deprecatedErr = parseDeprecatedFields(nodeConfigSourceDir) + }) + if deprecatedErr != nil { + t.Fatalf("read the node's configuration source: %v", deprecatedErr) + } + return deprecatedByStruct +} + +// nodeConfigSourceDir is the package whose structs these sections declare against. +// +// A relative path because it is the same module, so it is checked out beside this one and moving either +// is a change to both. +const nodeConfigSourceDir = "../../sei-tendermint/config" + +var ( + deprecatedOnce sync.Once + deprecatedByStruct map[string]map[string]bool + deprecatedErr error +) + +// parseDeprecatedFields walks a package's source for struct fields marked deprecated. +// +// A field counts as marked if its name carries the prefix the consensus settings use, or if the comment +// above it is the standard deprecation note. Only a field with a mapstructure tag is reported, since a +// field with no tag declares no key for a section to exclude. +func parseDeprecatedFields(dir string) (map[string]map[string]bool, error) { + parsed, err := parser.ParseDir(token.NewFileSet(), dir, nil, parser.ParseComments) + if err != nil { + return nil, err + } + out := map[string]map[string]bool{} + for _, pkg := range parsed { + ast.Inspect(pkg, func(n ast.Node) bool { + spec, ok := n.(*ast.TypeSpec) + if !ok { + return true + } + structType, ok := spec.Type.(*ast.StructType) + if !ok { + return true + } + marked := map[string]bool{} + for _, field := range structType.Fields.List { + if len(field.Names) != 1 || field.Tag == nil { + continue + } + name := field.Names[0].Name + if !strings.HasPrefix(name, "Deprecated") && !isDeprecationNote(field.Doc) { + continue + } + tag := reflect.StructTag(strings.Trim(field.Tag.Value, "`")) + if key, ok := tag.Lookup("mapstructure"); ok { + marked[strings.Split(key, ",")[0]] = true + } + } + out[spec.Name.Name] = marked + return true + }) + } + return out, nil +} + +// isDeprecationNote reports whether a field's comment is the standard deprecation note. +func isDeprecationNote(doc *ast.CommentGroup) bool { + if doc == nil { + return false + } + for _, line := range doc.List { + if strings.Contains(line.Text, "Deprecated:") { + return true } } - return out + return false } // writtenThenChecked writes one consensus path into a configuration and returns what the reader's @@ -296,30 +422,73 @@ func writtenThenChecked(t *testing.T, rel string) error { conf := tmcfg.DefaultConfig() v := viper.New() v.SetConfigType("toml") - body := "[consensus]\n" + rel + " = " + probeValueFor(rel) + "\n" + body := "[consensus]\n" + rel + " = " + probeValueFor(t, reflect.TypeOf(tmcfg.ConsensusConfig{}), rel) + "\n" if err := v.ReadConfig(strings.NewReader(body)); err != nil { t.Fatalf("compose a file setting %s: %v", rel, err) } + // Fatal rather than skipped. The probe value comes from the field's own type, so a refusal here means + // the derivation is wrong, and a skip inside this loop ends every remaining row while reporting a + // pass. if err := v.Unmarshal(conf); err != nil { - t.Skipf("%s does not decode from the probe value: %v", rel, err) + t.Fatalf("%s does not decode from a value derived from its own field: %v", rel, err) } return conf.DeprecatedFieldWarning() } +// probeValueFor renders a value the field behind a key accepts, taken from that field's own type. +// +// Derived rather than matched on the name, because a name says nothing about a shape. The double-sign +// height is an integer whose name begins like the boolean overrides do, and it decoded only because the +// decoder accepts a boolean where an integer belongs. A shape the decoder refuses used to end the loop +// through a skip, so every row after it went unmeasured and the run still read as a pass. +// // probeValueFor returns a written value of the right shape for a consensus path. // // Three shapes appear: a duration written as a string, a boolean, and a whole number. -func probeValueFor(rel string) string { - switch { - case strings.HasPrefix(rel, "skip-") || strings.HasPrefix(rel, "unsafe-") || - strings.HasPrefix(rel, "double-sign-") || strings.HasSuffix(rel, "-enabled"): - return "true" - case strings.Contains(rel, "timeout") || strings.Contains(rel, "-delta") || - strings.Contains(rel, "interval") || strings.Contains(rel, "period"): +func probeValueFor(t *testing.T, typ reflect.Type, rel string) string { + t.Helper() + field, ok := fieldTagged(typ, rel) + if !ok { + t.Fatalf("%s names no field of %s, so no value can be derived for it", rel, typ.Name()) + } + ft := field.Type + for ft.Kind() == reflect.Pointer { + ft = ft.Elem() + } + if ft == reflect.TypeOf(time.Duration(0)) { return "\"1s\"" - default: + } + switch ft.Kind() { + case reflect.Interface: + // The removed settings are each a pointer to an empty interface, which is how the reader tells a + // written value from an absent one. Any shape decodes, so the value only has to be present. + return "\"1s\"" + case reflect.Bool: + return "true" + case reflect.String: + return "\"probe\"" + case reflect.Float32, reflect.Float64: + return "1.0" + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: return "1" + case reflect.Slice: + return "[]" + default: + t.Fatalf("%s is a %s and no probe value is derived for that shape", rel, ft.Kind()) + return "" + } +} + +// fieldTagged returns the field of a struct whose mapstructure tag names a key. +func fieldTagged(typ reflect.Type, rel string) (reflect.StructField, bool) { + for i := 0; i < typ.NumField(); i++ { + f := typ.Field(i) + if tag, ok := f.Tag.Lookup("mapstructure"); ok && strings.Split(tag, ",")[0] == rel { + return f, true + } } + return reflect.StructField{}, false } // TestNoSectionDeclaresTheRootDirectory covers a field five of these sections carry.