diff --git a/config/tendermintbase/tendermintbase.go b/config/tendermintbase/tendermintbase.go index 56f0316602..4d164ff656 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 @@ -26,6 +56,10 @@ func init() { filledFromTheCommandLine, "max-outbound-connections") registry.RegisterSectionExcluding(RPCSectionName, &tmcfg.RPCConfig{}, rpcDefaults, filledFromTheCommandLine) + registry.RegisterSectionExcluding(ConsensusSectionName, &tmcfg.ConsensusConfig{}, consensusDefaults, + append([]string{filledFromTheCommandLine}, removedSettings...)...) + registry.RegisterSectionExcluding(MempoolSectionName, &tmcfg.MempoolConfig{}, mempoolDefaults, + filledFromTheCommandLine) } // forMode is the configuration the seid init command writes for a kind of node. @@ -70,3 +104,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 a6f0d9ab0d..264c79f52a 100644 --- a/config/tendermintbase/tendermintbase_test.go +++ b/config/tendermintbase/tendermintbase_test.go @@ -10,6 +10,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. @@ -108,6 +109,8 @@ func TestTheDeclaredKeysAreTheOnesTheReaderDecodes(t *testing.T) { }{ {P2PSectionName, &tmcfg.P2PConfig{}, 2}, {RPCSectionName, &tmcfg.RPCConfig{}, 1}, + {ConsensusSectionName, &tmcfg.ConsensusConfig{}, len(removedSettings) + 1}, + {MempoolSectionName, &tmcfg.MempoolConfig{}, 1}, } { registered, ok := registry.Lookup(tc.section) if !ok { @@ -186,6 +189,138 @@ 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 want := len(removedSettings) + 1; len(excluded) != want { + t.Errorf("the section excludes %d paths and %d were expected, being the removed settings and the "+ + "root directory", len(excluded), want) + } + 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" + } +} + // 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