diff --git a/cmd/seid/cmd/node_agreement_test.go b/cmd/seid/cmd/node_agreement_test.go index 0da3f12eef..dabc958204 100644 --- a/cmd/seid/cmd/node_agreement_test.go +++ b/cmd/seid/cmd/node_agreement_test.go @@ -39,9 +39,9 @@ import ( // value is four times what a node runs, so a sparse file raises both. // connection policy p2p.max-connections is ten times higher declared than a seed runs, and // p2p.allow-duplicate-ip is declared on where a seed has it off. -// service switches api.enable, grpc.enable, evm.http_enabled, evm.ws_enabled and -// state-store.ss-enable are open on the node and declared closed for a validator -// and a seed, which serve no queries. A sparse file closes them. +// service switches api.enable, grpc.enable, grpc-web.enable, evm.http_enabled, evm.ws_enabled +// and state-store.ss-enable are open on the node and declared closed for a +// validator and a seed, which serve no queries. A sparse file closes them. // retention pruning is the flag-default row, in every kind: the flag prunes and the // declaration keeps all state history. min-retain-blocks and // state-store.ss-keep-recent are the block and state halves of the same choice, @@ -58,6 +58,7 @@ var divergences = map[registry.Mode]map[string]string{ "api.enable": "true", "evm.http_enabled": "true", "evm.ws_enabled": "true", + "grpc-web.enable": "true", "grpc.enable": "true", "p2p.recv-rate": "5120000", "p2p.send-rate": "5120000", @@ -79,6 +80,7 @@ var divergences = map[registry.Mode]map[string]string{ "api.enable": "true", "evm.http_enabled": "true", "evm.ws_enabled": "true", + "grpc-web.enable": "true", "grpc.enable": "true", "p2p.allow-duplicate-ip": "false", "p2p.max-connections": "100", diff --git a/cmd/seid/cmd/undeclared_keys_test.go b/cmd/seid/cmd/undeclared_keys_test.go new file mode 100644 index 0000000000..e1f86e2eac --- /dev/null +++ b/cmd/seid/cmd/undeclared_keys_test.go @@ -0,0 +1,128 @@ +package cmd + +import ( + "path/filepath" + "sort" + "testing" + + "github.com/spf13/viper" + + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// keysAGeneratedFileStatesThatNothingDeclares is every key this binary writes into a node's own +// configuration files and does not declare, with the reason it is left out. +// +// An undeclared key goes on resolving from the file that states it. Under the versioned manager that is +// the one thing sei.toml cannot reach: the resolution has nothing to say about the key, so writing that +// file changes nothing for it and neither does writing it again on every start. A node enrolled in +// automatic configuration management is held to its kind's defaults for every key but these. +// +// So each one is a decision, and the decision is recorded here rather than left as the absence of a +// registration. A key added to a template without a section to declare it fails the measurement below +// instead of quietly answering from app.toml. +// +// Where the reason belongs to a registered section, it lives with that section and the row names the +// constant holding it. Restating it here would let the two drift. +var keysAGeneratedFileStatesThatNothingDeclares = map[string]string{ + // Not a supported feature of this chain. The upstream server still starts one when the key is on, and + // it prints that it is a beta feature not to be used in production. Pending removal. + "rosetta.enable": "rosetta is not a supported feature", + "rosetta.address": "rosetta is not a supported feature", + "rosetta.blockchain": "rosetta is not a supported feature", + "rosetta.network": "rosetta is not a supported feature", + "rosetta.offline": "rosetta is not a supported feature", + "rosetta.retries": "rosetta is not a supported feature", + + // The reader that builds the server configuration from the source fills this whole section from its + // own defaults and never asks the source for any of these keys. Measured: a written page ceiling of + // 500 arrives as the default 1000, and a written allowlist arrives empty. Declaring one would offer a + // setting that changes nothing, and the file's own comment already tells an operator to set one of + // them. + "query.disable-limits": "the server configuration reader never asks the source for this section", + "query.trusted-cidrs": "the server configuration reader never asks the source for this section", + "query.max-limit": "the server configuration reader never asks the source for this section", + "query.max-offset": "the server configuration reader never asks the source for this section", + "query.max-iterations": "the server configuration reader never asks the source for this section", + + // No reader resolves it. The struct the command renders sets one value for this key and the template + // beside it renders another as a bare literal, which is held by a test of its own. + "wasm.lru_size": "no reader resolves it", + + // The template renders this name and the reader looks up contract_state_checks, which is the key this + // binary declares. A value written under this name reaches nothing. + "eth_replay.eth_replay_contract_state_checks": "the reader looks up contract_state_checks", + + // Recorded where the sections that would carry them are registered. + "mode": "config/tendermintbase, statedAtTheTopOfTheFile", + "mempool.max-batch-bytes": "config/tendermintbase, unreadAndUnmarked", + "mempool.pending-ttl-duration": "config/tendermintbase, neverReachTheMempool", + "mempool.pending-ttl-num-blocks": "config/tendermintbase, neverReachTheMempool", + "self-remediation.p2p-no-peers-available-window-seconds": "config/tendermintbase, reachesNoReactor", +} + +// TestEveryKeyAGeneratedFileStatesIsDeclaredOrRecorded closes the gap a missing registration leaves. +// +// Two sides of the same key space, and neither one is checked by anything else. A key this binary writes +// into a file and does not declare resolves from that file forever, and a key recorded here that the +// binary has started declaring makes the record claim a hole that is closed. +// +// The sections a registration excludes are covered too, because an excluded path is still written into +// the file. Excluding a path and declaring the section it sits in are different statements, and only the +// first leaves the key resolving from app.toml. +func TestEveryKeyAGeneratedFileStatesIsDeclaredOrRecorded(t *testing.T) { + configtest.Isolate(t) + home := aNodeRunningAs(t, registry.ModeValidator) + + stated := whatTheFilesState(t, home) + declared := make(map[string]bool, len(registry.Keys())) + for _, key := range registry.Keys() { + declared[key] = true + } + + var undeclared []string + for _, key := range stated { + if declared[key] { + continue + } + undeclared = append(undeclared, key) + if _, recorded := keysAGeneratedFileStatesThatNothingDeclares[key]; !recorded { + t.Errorf("this binary writes %s into a node's own files and does not declare it, and nothing "+ + "records why. It resolves from that file whatever a sei.toml says, so a node enrolled in "+ + "automatic configuration management is not held to its kind's defaults for it", key) + } + } + + for key := range keysAGeneratedFileStatesThatNothingDeclares { + if declared[key] { + t.Errorf("%s is recorded as undeclared and this binary declares it, so the record names a "+ + "hole that is closed", key) + } + } + + sort.Strings(undeclared) + if len(undeclared) != len(keysAGeneratedFileStatesThatNothingDeclares) { + t.Errorf("measured %d undeclared keys and %d are recorded: %v", + len(undeclared), len(keysAGeneratedFileStatesThatNothingDeclares), undeclared) + } +} + +// whatTheFilesState returns every key the node's own configuration files state, by dotted name. +// +// Read the way a lookup reads them, so the names are the ones a reader resolves rather than the shape the +// files nest them in. +func whatTheFilesState(t *testing.T, home string) []string { + t.Helper() + var all []string + for _, name := range []string{"app.toml", "config.toml"} { + v := viper.New() + v.SetConfigFile(filepath.Join(home, "config", name)) + if err := v.ReadInConfig(); err != nil { + t.Fatalf("read %s: %v", name, err) + } + all = append(all, v.AllKeys()...) + } + sort.Strings(all) + return all +} diff --git a/config/cosmosbase/cosmosbase.go b/config/cosmosbase/cosmosbase.go index ba37836a12..01091f0d6c 100644 --- a/config/cosmosbase/cosmosbase.go +++ b/config/cosmosbase/cosmosbase.go @@ -1,6 +1,6 @@ // Package cosmosbase registers the configuration sections whose keys belong to the Cosmos server. // -// These five register here rather than beside the structs they describe, and the reason is an import edge. +// These six register here rather than beside the structs they describe, and the reason is an import edge. // The mode rules their defaults answer through live in app/params, which imports the upstream server // configuration, so that package cannot ask for them without a cycle. A vendored tree is not itself the // obstacle: other sections do register inside one. @@ -25,6 +25,7 @@ const ( GRPCSectionName = "grpc" TelemetrySectionName = "telemetry" StateSyncSectionName = "state-sync" + GRPCWebSectionName = "grpc-web" ) // globalLabelsKey is the metric label set, which is the one key here no environment variable can supply. @@ -32,12 +33,13 @@ const globalLabelsKey = TelemetrySectionName + ".global-labels" // Registration puts the upstream server's configuration sections in the registry. // -// Four of the five register the upstream struct directly, because their mapstructure tags already name the +// Five of the six register the upstream struct directly, because their mapstructure tags already name the // keys their reader resolves. func init() { registry.RegisterRootKeys(BaseSectionName, &srvconfig.BaseConfig{}, baseDefaults) registry.RegisterSection(APISectionName, &srvconfig.APIConfig{}, apiDefaults) registry.RegisterSection(GRPCSectionName, &srvconfig.GRPCConfig{}, grpcDefaults) + registry.RegisterSection(GRPCWebSectionName, &srvconfig.GRPCWebConfig{}, grpcWebDefaults) registry.RegisterSection(TelemetrySectionName, &telemetrySchema{}, telemetryDefaults) registry.RegisterSection(StateSyncSectionName, &srvconfig.StateSyncConfig{}, stateSyncDefaults) } @@ -102,6 +104,17 @@ func apiDefaults(mode registry.Mode) any { return forMode(mode).API } // which is the shape the reader parses back. func grpcDefaults(mode registry.Mode) any { return forMode(mode).GRPC } +// grpcWebDefaults is what the gRPC-web settings resolve to for a node of this kind. +// +// On for a full node and an archive node, off for a validator and a seed, which is the rule the REST +// interface and gRPC follow and for the same reason. The upstream default is on for every kind. +// +// Three of these four keys are read with a casting getter and no check that the key was present, and the +// fourth, the connection ceiling, is read only when the key is set. The three unguarded ones are safe in +// one direction only: whether the interface starts at all is one of them, so an absent key casts to false +// and the interface does not come up rather than coming up on an address cast from nothing. +func grpcWebDefaults(mode registry.Mode) any { return forMode(mode).GRPCWeb } + // stateSyncDefaults is what the snapshot settings resolve to for a node of this kind. // // All three keys are read with a casting getter and no presence check, and the retention is the one that