diff --git a/app/receipt_store_config_test.go b/app/receipt_store_config_test.go new file mode 100644 index 0000000000..8cb5006f2a --- /dev/null +++ b/app/receipt_store_config_test.go @@ -0,0 +1,254 @@ +package app + +import ( + "fmt" + "math" + "testing" + + "github.com/spf13/cast" + + "github.com/sei-protocol/sei-chain/app/params" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + srvconfig "github.com/sei-protocol/sei-chain/sei-cosmos/server/config" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// Tests for min-retain-blocks, the only key in this tree that two live consumers read. One +// operator value becomes both a Tendermint block-retention height and an EVM receipt retention +// window, through different casts. +// +// testutil/configtest/AGENTS.md holds the fan-out's architecture and the two gaps it leaves open, +// including which receipt-store backend survives a saturated retention window by an ordinary bound +// and which survives by an accident nothing here pins. + +// minRetainBlocksFanOut is what one operator value becomes on each side. +var minRetainBlocksFanOut = []struct { + // raw is an any because the input type is part of the case. Both readers cast whatever + // appOpts.Get returns, and which Go type that is depends on the layer that set the key: the env + // layer and a quoted app.toml entry hand them a string, an unquoted integer decodes to int64, + // and an unquoted float literal decodes to float64. Only the float case makes the two casts + // disagree with the receipt side positive, so a table of strings could not express it. + raw any + receipt int // app/receipt_store_config.go:27, through cast.ToInt. Not read when saturates. + // block records what cast.ToUint64 produces, held against a number rather than a second call to + // the same function, since an assertion comparing a call to itself passes for any reader. + // + // It and the saturates rows assert spf13/cast and Go's float-to-int conversion rather than + // anything in this tree, so a cast bump can redden this package for a reason unrelated to + // configuration resolution, reported as a message about receipt retention. + block uint64 // cmd/seid/cmd/root.go:297, through cast.ToUint64 + // saturates marks a row whose receipt cast Go leaves implementation-defined, so no literal is a + // correct prediction and the property is asserted in place of the value. Same reason a + // NumCPU-derived default is recorded as a DerivedDefault rather than a number. + saturates bool +}{ + {"0", 0, 0, false}, // keep everything, both sides + {"100000", 100000, 100000, false}, // the ordinary case: one value, two policies + {"200000", 200000, 200000, false}, // a second ordinary value, same class + {"-5", -5, 0, false}, // ToInt keeps the negative, ToUint64 floors + {"9223372036854775808", 0, 9223372036854775808, false}, // past int64: ToInt floors, ToUint64 keeps + {"18446744073709551615", 0, 18446744073709551615, false}, // the uint64 ceiling, same shape + {"not-a-number", 0, 0, false}, // both floor, so both keep everything + // A leading sign is where the two string casts could most plausibly part company, because ParseInt + // takes a sign prefix. cast strips a leading + before ParseUint sees it, so they agree, and the row + // exists so that stops being something a reader takes on trust. + {"+5", 5, 5, false}, + // Unquoted in app.toml, so the decode hands both readers the operator's own type. + {int64(100000), 100000, 100000, false}, + // The one shape where the two casts can disagree with the receipt side positive. Reachable as + // min-retain-blocks = 1e19, which decodes to float64. Go leaves a float-to-int conversion whose + // result the target cannot represent implementation-defined, and the two architectures the fleet + // ships take it differently: amd64 lowers it to a bare CVTTSD2SQ and gets the x86 indefinite value, + // MinInt64, while arm64 lowers it to FCVTZS and saturates to MaxInt64. So the receipt column is a + // property here rather than a number. The block column is not affected: uint64 of a float below + // 2^64 is the exact magnitude on both. + {float64(1e19), 0, 10000000000000000000, true}, + // Just above 2^63, so the same conversion from a value an operator could plausibly mistype. + {float64(9.3e18), 0, 9300000000000000000, true}, +} + +// TestMinRetainBlocksFanOutNeverResolvesReceiptsToAPruningWindow asserts that the receipt side never +// resolves to a window that would prune. It says the config layer resolves to a safe value, not that +// what happens to that value downstream is safe. +// +// The name says resolves rather than prunes because whether a resolved value goes on to expire +// anything is a sei-db property this layer cannot reach. +func TestMinRetainBlocksFanOutNeverResolvesReceiptsToAPruningWindow(t *testing.T) { + for _, row := range minRetainBlocksFanOut { + t.Run(fmt.Sprintf("%v(%T)", row.raw, row.raw), func(t *testing.T) { + receiptConfig, err := readReceiptStoreConfig(t.TempDir(), mapAppOpts{ + server.FlagMinRetainBlocks: row.raw, + }) + if err != nil { + t.Fatalf("readReceiptStoreConfig(%v): %v", row.raw, err) + } + + // Where Go leaves the conversion implementation-defined, the property stands in for the + // value: the receipt side must be at or below zero, or the saturating positive extreme. + // Anything else positive is a real retention window and would prune. + if row.saturates { + if kr := receiptConfig.KeepRecent; kr > 0 && kr != math.MaxInt64 { + t.Errorf("min-retain-blocks=%v resolved the receipt store's KeepRecent to %d. Go "+ + "leaves this conversion implementation-defined, so the two outcomes this suite "+ + "accepts are at-or-below zero, which the KeepRecent>0 guard refuses, and MaxInt64, "+ + "which sei-db is relied on to render harmless by overflow and which nothing here "+ + "pins. A positive window that is neither is a real retention schedule an operator "+ + "never set", row.raw, kr) + } + } else if receiptConfig.KeepRecent != row.receipt { + t.Errorf("min-retain-blocks=%v resolves the receipt store's KeepRecent to %d where this "+ + "row predicts %d. The prediction describes app/receipt_store_config.go, so a reader "+ + "you changed deliberately means updating the row and saying what a node now retains "+ + "for receipts; a reader you did not change means the cast moved underneath it", + row.raw, receiptConfig.KeepRecent, row.receipt) + } + // The recording, against the cast the block side applies. + if got := cast.ToUint64(row.raw); got != row.block { + t.Errorf("min-retain-blocks=%v casts to %d through cast.ToUint64, recorded as %d. The "+ + "block column is a recording of the cast rather than a pin on root.go:297, so "+ + "update it and say what a node now retains", row.raw, got, row.block) + } + + // A saturating row is done here, and this sits above the agreement check rather than below + // it so that check never reads a receipt column holding no prediction. It stays below the + // block check, because the block column is a real prediction on these rows. + if row.saturates { + return + } + + // Same number on both sides means the fan-out is a plain coupling and there is nothing to + // check. The sign test is part of that question rather than belt-and-braces: converting a + // negative receipt value to uint64 wraps it to the top of the range, so a row pairing a + // negative with a large block value would read as agreement and skip the check below. + if row.receipt >= 0 && uint64(row.receipt) == row.block { + return + } + // Positive is the whole test, because a positive KeepRecent is the one state that arms + // receipt expiry on either backend: pebbledb starts a pruner + // (sei-db/ledger_db/receipt/receipt_store.go:363) and litt sets a TTL + // (sei-db/ledger_db/receipt/litt_receipt_store.go:138). At or below zero, neither does. + if row.receipt > 0 { + t.Errorf("min-retain-blocks=%v gives %d for receipt retention and %d for block "+ + "retention. The two disagree and the receipt side is a positive window that is not "+ + "the saturation case, so a value the block side handled one way starts pruning EVM "+ + "receipts on a schedule the operator never set", row.raw, row.receipt, row.block) + } + }) + } +} + +// minRetainBlocksKeyName records the operator-facing spelling of the key both retention readers share. +// +// A KeyName rather than a KeySpec, because it resolves into two different structs through two +// different casts and no single row could name a Path for it. The record exists because nothing else +// in the tree pins this constant's value: sei-cosmos/server/config's base_config record belongs to +// GetConfig, which reads the literal independently, so renaming server.FlagMinRetainBlocks moves the +// key for both live readers this file exists to hold still. That rename does fail today, where a test +// asks the start command to set a flag it no longer has, but it fails as "no such flag" rather than as +// an operator-facing key having moved, and it fails in another package. +// +// Spelled through the reader's own constant, which is the position the record exists for: a rename +// lands in this golden as a diff rather than only as a set failure somewhere else. +var minRetainBlocksKeyName = []configtest.KeyName{configtest.KeyName(server.FlagMinRetainBlocks)} + +// TestMinRetainBlocksKeyNameMatchesTheRecordedName pins the spelling of the key above. +func TestMinRetainBlocksKeyNameMatchesTheRecordedName(t *testing.T) { + configtest.CheckKeyNames(t, "min_retain_blocks", nil, minRetainBlocksKeyName...) +} + +// TestMinRetainBlocksFullNodeModeCapsReceiptRetention records the case where the fan-out bites, which +// is the default one. +// +// seid init defaults --mode to full, and setFullnodeTypeAppConfig sets min-retain-blocks to 100000 for +// Tendermint block pruning. The same key reaches the receipt store as KeepRecent, so a positive value +// arms receipt pruning: the default pebbledb backend starts a pruner from it +// (sei-db/ledger_db/receipt/receipt_store.go:176) and the litt backend applies a TTL and a read floor +// from it. So a default full node caps EVM receipt retention at a block count nobody set for that +// purpose, and the key's own documentation describes only block retention. +// +// An operator has no setting that keeps both. Block pruning at 100000 caps receipts, and keeping every +// receipt means min-retain-blocks of 0, which also stops pruning blocks. That is the coupling recorded +// here, and it is tracked as PLT-976. +// +// The value is read out of SetAppConfigByMode rather than written here, for the reason the archive test +// gives: a transcribed 100000 would hold whatever the mode did, so moving the mode off 100000 would +// change receipt retention on every full node with nothing reporting it. +func TestMinRetainBlocksFullNodeModeCapsReceiptRetention(t *testing.T) { + configtest.Isolate(t) + + full := srvconfig.DefaultConfig() + params.SetAppConfigByMode(full, params.NodeModeFull) + + if full.MinRetainBlocks != 100000 { + t.Fatalf("full mode now sets min-retain-blocks to %d rather than 100000. That value is also the "+ + "receipt store's KeepRecent, so this moves how much EVM receipt history every default full "+ + "node keeps, not only how many Tendermint blocks it retains. If the change is deliberate, "+ + "say what receipt history a full node is now expected to serve", full.MinRetainBlocks) + } + + receiptConfig, err := readReceiptStoreConfig(t.TempDir(), mapAppOpts{ + server.FlagMinRetainBlocks: full.MinRetainBlocks, + }) + if err != nil { + t.Fatalf("readReceiptStoreConfig: %v", err) + } + // The state that arms pruning, stated on its own so the name of this test stays true, and + // established before the comparison below so that comparison needs no unguarded conversion. + if receiptConfig.KeepRecent <= 0 { + t.Errorf("full mode now leaves the receipt store's KeepRecent at %d, which is the no-pruning "+ + "state. That is a better end state for receipt history and it changes what a full node "+ + "keeps, so record what block retention it now gets instead", receiptConfig.KeepRecent) + } + + // The mode's value reaching the reader intact. Compared as uint64 rather than converting the + // reader's int down, because the sign is already established above and widening cannot overflow + // where narrowing could. + if uint64(receiptConfig.KeepRecent) != full.MinRetainBlocks { + t.Errorf("full mode's min-retain-blocks of %d reached the receipt store as KeepRecent=%d. The "+ + "fan-out is what this file records, so the two stopping agreeing is the thing to explain", + full.MinRetainBlocks, receiptConfig.KeepRecent) + } +} + +// TestMinRetainBlocksArchiveModeKeepsBothRetentionsOpen records that the archive path is aligned. +// +// PLT-955 is an archive node pruning state history because its mode's state-store settings are +// discarded. This is the neighbouring question, asked and answered so nobody re-investigates it: the +// archive mode leaves min-retain-blocks at 0, which is keep-all for Tendermint blocks, and 0 through +// the receipt cast leaves KeepRecent at 0, which is no pruning. So the fan-out does not give archive +// a second history-loss path. +// +// Two things are checked and they catch different changes. The first assertion reads archive's value +// out of SetAppConfigByMode, so a mode that stops keeping every block fails there; that is the one +// that would have caught the change this test exists to notice, and it is why the value is sourced +// rather than transcribed. The second hands that value to the reader, which with the first assertion +// standing is always 0, so what it adds is narrower: it fails if readReceiptStoreConfig starts +// substituting a non-zero default for a zero key instead of passing it through. +func TestMinRetainBlocksArchiveModeKeepsBothRetentionsOpen(t *testing.T) { + configtest.Isolate(t) + + archive := srvconfig.DefaultConfig() + params.SetAppConfigByMode(archive, params.NodeModeArchive) + + // Stated as its own assertion so a mode that stops keeping every block says so here, rather than + // through the receipt-side comparison below. + if archive.MinRetainBlocks != 0 { + t.Fatalf("archive mode now sets min-retain-blocks to %d rather than 0. Tendermint prunes "+ + "blocks on that schedule and the receipt store takes the same key, so an archive node "+ + "keeps neither all blocks nor all receipts. If the mode changed deliberately, say what an "+ + "archive node is now expected to retain", archive.MinRetainBlocks) + } + + receiptConfig, err := readReceiptStoreConfig(t.TempDir(), mapAppOpts{ + server.FlagMinRetainBlocks: archive.MinRetainBlocks, + }) + if err != nil { + t.Fatalf("readReceiptStoreConfig: %v", err) + } + if receiptConfig.KeepRecent != 0 { + t.Errorf("archive's min-retain-blocks of %d gave the receipt store KeepRecent=%d, where 0 is "+ + "what leaves receipts unpruned. An archive node would now discard EVM receipts, which is "+ + "the same class of loss as PLT-955 through a different key", + archive.MinRetainBlocks, receiptConfig.KeepRecent) + } +} diff --git a/app/testdata/min_retain_blocks.keys.golden b/app/testdata/min_retain_blocks.keys.golden new file mode 100644 index 0000000000..b04c255bc0 --- /dev/null +++ b/app/testdata/min_retain_blocks.keys.golden @@ -0,0 +1,2 @@ +# keys with a target of their own +"min-retain-blocks" diff --git a/app/testdata/wiring_coverage.txt b/app/testdata/wiring_coverage.txt index 6a1fc4b061..75d1f3d2d6 100644 --- a/app/testdata/wiring_coverage.txt +++ b/app/testdata/wiring_coverage.txt @@ -18,6 +18,7 @@ light_invariance CheckEveryRowHasADiscriminatingSeed light_invariance CheckKeyNames light_invariance CheckManifestCoversEveryField light_invariance CheckRow +min_retain_blocks CheckKeyNames state-commit CheckDefaults state-commit CheckEveryRowHasADiscriminatingSeed state-commit CheckKeyNames diff --git a/cmd/seid/cmd/mingasprices_config_fuzz_test.go b/cmd/seid/cmd/mingasprices_config_fuzz_test.go new file mode 100644 index 0000000000..739ae812b5 --- /dev/null +++ b/cmd/seid/cmd/mingasprices_config_fuzz_test.go @@ -0,0 +1,159 @@ +package cmd + +import ( + "fmt" + "strings" + "testing" + + "github.com/spf13/cast" + "github.com/spf13/viper" + + "github.com/sei-protocol/sei-chain/sei-cosmos/baseapp" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + serverconfig "github.com/sei-protocol/sei-chain/sei-cosmos/server/config" +) + +// Tests for minimum-gas-prices, the one key in this tree whose accepted syntax is contradicted by +// its own documentation. One reader governs a node, and two artifacts show a separator it panics on. +// +// testutil/configtest/AGENTS.md holds the contradiction, why the two halves of a repair carry +// different risk, and the call-site gap this file cannot close. + +// resolveMinGasPrices runs the expression root.go:296 runs, through the viper type production hands +// it, and reports whether it panicked rather than the option, because the panic is the behavior +// under test. +// +// Whether it panicked and what it said are returned separately for the same reason getterReads +// below keeps them apart: collapsed into one string, a boot and a panic carrying an empty message +// are the same answer, and the fuzz target's only test for "booted" is that string being empty. +func resolveMinGasPrices(raw string) (panicked bool, panicMessage string) { + defer func() { + if r := recover(); r != nil { + panicked, panicMessage = true, fmt.Sprint(r) + } + }() + appOpts := viper.New() + appOpts.Set(server.FlagMinGasPrices, raw) + _ = baseapp.SetMinGasPrices(cast.ToString(appOpts.Get(server.FlagMinGasPrices))) + return false, "" +} + +// FuzzMinGasPricesLiveReaderTakesCommasAndRejectsSemicolons pins which separator boots a node. +func FuzzMinGasPricesLiveReaderTakesCommasAndRejectsSemicolons(f *testing.F) { + f.Add(serverconfig.DefaultMinGasPrices) // the shipped default, one denomination + f.Add("") // absent, and accepted: ParseDecCoins reads it as no floor + f.Add("0.01usei") + f.Add("0.01usei,0.02uatom") // the separator the live reader accepts + f.Add("0.01usei;0.02uatom") // the separator both documents show, and the live reader panics on + f.Add("0.01photino;0.0001stake") + f.Add("abc") + f.Add("1") + f.Add("usei") + f.Add("-1usei") + + f.Fuzz(func(t *testing.T, raw string) { + panicked, panicMessage := resolveMinGasPrices(raw) + + // A semicolon is only reachable as a separator, so any value carrying one has to panic for + // the disjointness this file records to hold. + if strings.Contains(raw, ";") && !panicked { + t.Errorf("minimum-gas-prices=%q carries a semicolon and booted anyway. ParseDecCoins now "+ + "accepts the separator the flag's help text and Config.GetMinGasPrices already use, so "+ + "the two syntaxes have stopped being disjoint. That is a fine end state and it changes "+ + "what a node accepts, so update this file in the PR that widens the parser", raw) + } + // Where the rejection happens, not just that it happens. Every rejection reachable from a + // string arrives as an error ParseDecCoins returns and SetMinGasPrices wraps, so the wrap is + // the marker that the refusal is still baseapp's. + if panicked && !strings.Contains(panicMessage, "invalid minimum gas prices") { + t.Errorf("minimum-gas-prices=%q panicked with %q rather than carrying baseapp's "+ + "\"invalid minimum gas prices\" wrap. Read it one of two ways, and both are worth a look "+ + "rather than a loosened assertion: the rejection moved to another reader, or this value "+ + "found a path that panics inside ParseDecCoins instead of returning an error for "+ + "SetMinGasPrices to wrap. Seeded runs cover neither; a fuzz run reaching here has found "+ + "something", raw, panicMessage) + } + }) +} + +// TestMinGasPricesFlagHelpShowsASeparatorTheLiveReaderPanicsOn reads the help text off the real +// command rather than a copy of it, so correcting either side lands in a diff. +func TestMinGasPricesFlagHelpShowsASeparatorTheLiveReaderPanicsOn(t *testing.T) { + cmd := shippedStartCmd(t) + flag := cmd.Flags().Lookup(server.FlagMinGasPrices) + if flag == nil { + t.Fatalf("start no longer registers %s, so root.go:296 reads a key with no flag behind it "+ + "and an absent value stops being the empty string this file records", server.FlagMinGasPrices) + } + + // The example in the help text, held against the reader that would have to accept it. + const documented = "0.01photino;0.0001stake" + if !strings.Contains(flag.Usage, documented) { + t.Fatalf("the %s help text no longer offers %q as its example, so the contradiction this file "+ + "records may be closed. Confirm the new example parses, then delete this test rather than "+ + "loosening it. Usage is now %q", server.FlagMinGasPrices, documented, flag.Usage) + } + if panicked, _ := resolveMinGasPrices(documented); !panicked { + t.Fatalf("the documented example %q now boots, so the help text and the reader agree and this "+ + "recording is stale", documented) + } + + // The flag's default is what an operator gets by writing nothing, and it is not the package + // default: an absent key resolves empty, which ParseDecCoins reads as no fee floor at all. + if flag.DefValue != "" { + t.Fatalf("the %s flag now defaults to %q rather than empty. An empty default is why a silent "+ + "app.toml yields a node with no fee floor, which is the behavior the [base_config] rows "+ + "record; a real default changes it", server.FlagMinGasPrices, flag.DefValue) + } + // And the declared default is not empty, which is what makes the flag's empty default a gap rather + // than the same value stated twice. Asserted on the declared default directly, since the check + // above has already established what the flag carries. + if serverconfig.DefaultMinGasPrices == "" { + t.Fatalf("DefaultMinGasPrices is now empty too, so the declared default and the resolved one " + + "agree and there is no longer a gap here to record. That also means nothing in-code states " + + "a fee floor, so say what a node is expected to accept") + } +} + +// TestMinGasPricesGetterAcceptsOnlyWhatTheLiveReaderRejects pins the inversion itself, which is the +// part a reader would otherwise have to discover twice. +func TestMinGasPricesGetterAcceptsOnlyWhatTheLiveReaderRejects(t *testing.T) { + const ( + commaSeparated = "0.01usei,0.02uatom" + semicolonSeparated = "0.01usei;0.02uatom" + ) + + // The dead getter, reporting how many denominations it read and whether it panicked. Those are + // kept apart because they are different facts: a getter that returned one coin and a getter that + // panicked would otherwise be the same answer, and the panic is what this records. + getterReads := func(raw string) (coins int, panicked bool) { + defer func() { + if r := recover(); r != nil { + panicked = true + } + }() + cfg := serverconfig.Config{BaseConfig: serverconfig.BaseConfig{MinGasPrices: raw}} + return len(cfg.GetMinGasPrices()), false + } + + if coins, panicked := getterReads(semicolonSeparated); panicked || coins != 2 { + t.Errorf("GetMinGasPrices read %q as %d denominations (panicked=%v) rather than 2, so "+ + "config.go:323 has stopped splitting on the separator no other reader accepts", + semicolonSeparated, coins, panicked) + } + if coins, panicked := getterReads(commaSeparated); !panicked { + t.Errorf("GetMinGasPrices no longer panics on %q and read %d denominations instead. Its split "+ + "leaves the whole value in one token, so the panic is how it refuses the live reader's "+ + "syntax; if it now accepts that syntax the two readers have stopped being disjoint", + commaSeparated, coins) + } + if panicked, _ := resolveMinGasPrices(commaSeparated); panicked { + t.Errorf("the live reader now rejects %q, which was the one multi-denomination spelling that "+ + "booted a node. If the parser changed, say what an operator should write instead", + commaSeparated) + } + if panicked, _ := resolveMinGasPrices(semicolonSeparated); !panicked { + t.Errorf("the live reader now accepts %q, so the documented separator boots and this "+ + "inversion is closed", semicolonSeparated) + } +} diff --git a/cmd/seid/cmd/startprerun_config_fuzz_test.go b/cmd/seid/cmd/startprerun_config_fuzz_test.go index 488344241d..e4c3e24483 100644 --- a/cmd/seid/cmd/startprerun_config_fuzz_test.go +++ b/cmd/seid/cmd/startprerun_config_fuzz_test.go @@ -19,22 +19,144 @@ import ( "github.com/sei-protocol/sei-chain/testutil/configtest" ) -// start layers two more config behaviors on top of Apply, in its own PreRunE and at -// the head of its RunE. Both are reachable without launching a node; everything after -// them is not. +// Tests for the two config behaviors start layers on top of Apply, in its own PreRunE and at the +// head of its RunE. Both are reachable without launching a node; everything after them is not. // -// PreRunE re-binds the command's flags into the viper Apply already populated, then -// resolves pruning purely to fail fast — the returned options are discarded. So a bad -// pruning strategy is refused before the node touches disk, and refused a second time -// later by newApp, where the same helper panics instead of returning. +// PreRunE re-binds the command's flags into the viper Apply already populated, then resolves +// pruning purely to fail fast, discarding the options it returns. RunE re-reads client.toml, +// compares its chain-id against --chain-id, and panics on a mismatch before any app is built. // -// RunE then re-reads client.toml, compares its chain-id against --chain-id, and -// panics on a mismatch before any app is constructed. +// testutil/configtest/AGENTS.md holds which reads resolve where this suite can reach them and are +// consumed where it cannot, and why the effects of those reads are deliberately left unpinned. + +// TestStartPreRunResolvesTheKeysOnlyStartInProcessReads pins what cpu-profile, trace-store and +// grpc-only resolve to, which is the whole of the live-node domain that resolves at all. +// +// These three are the only keys startInProcess reads that no other section pins. Everything else it +// touches is a field of the struct GetConfig produces, and that reader has a manifest of its own. +// Before this, cpu-profile and trace-store appeared in no test and no record anywhere in the tree, +// so a rename of either flag moved the read site with nothing reporting it. +// +// Driven through PreRunE rather than a booted node because that is where the values arrive. start.go +// registers all three as flags on the start command and PreRunE binds the flag set into the viper +// Apply populated, so startInProcess later reads this same viper. What a node would add is the +// effect, not the value. +// +// Both directions per key. An absent flag has to resolve to the zero the read site branches on, +// since cpu-profile and trace-store are each compared against "" to decide whether to enable +// profiling or tracing at all, and a set flag has to arrive intact. A test that only checked the set +// case would hold for a read that ignored the key and returned the operator's value from somewhere +// else. +func TestStartPreRunResolvesTheKeysOnlyStartInProcessReads(t *testing.T) { + configtest.Isolate(t) + + t.Run("absent", func(t *testing.T) { + home := configtest.NewHome(t) + cmd, serverCtx, _ := newStartCmd(t, home, map[string]string{ + server.FlagPruning: "nothing", + }) + if err := cmd.PreRunE(cmd, nil); err != nil { + t.Fatalf("PreRunE: %v", err) + } + + // The empty string is what start.go compares against to decide whether to profile or trace, + // so this is the value that keeps both features off rather than an incidental zero. + if got := serverCtx.Viper.GetString("cpu-profile"); got != "" { + t.Errorf("an absent cpu-profile resolved to %q, want empty. start.go enables the profiler "+ + "for any non-empty value, so a non-empty resolution here starts profiling and writes "+ + "to that path on a node whose operator never asked for it", got) + } + if got := serverCtx.Viper.GetString("trace-store"); got != "" { + t.Errorf("an absent trace-store resolved to %q, want empty. start.go opens a KVStore trace "+ + "writer for any non-empty value, so a non-empty resolution here writes a trace file on "+ + "a node whose operator never asked for one", got) + } + if serverCtx.Viper.GetBool("grpc-only") { + t.Error("an absent grpc-only resolved true, which would start the node with Tendermint " + + "disabled and GRPC.Enable forced on") + } + }) + + t.Run("set", func(t *testing.T) { + home := configtest.NewHome(t) + cmd, serverCtx, _ := newStartCmd(t, home, map[string]string{ + server.FlagPruning: "nothing", + "cpu-profile": "/var/lib/sei/cpu.pprof", + "trace-store": "/var/lib/sei/trace.log", + "grpc-only": "true", + }) + if err := cmd.PreRunE(cmd, nil); err != nil { + t.Fatalf("PreRunE: %v", err) + } + + for _, c := range []struct{ key, want string }{ + {"cpu-profile", "/var/lib/sei/cpu.pprof"}, + {"trace-store", "/var/lib/sei/trace.log"}, + } { + if got := serverCtx.Viper.GetString(c.key); got != c.want { + t.Errorf("%s resolved to %q, want %q. startInProcess reads this viper, so the path a "+ + "node profiles or traces to is whatever arrives here", c.key, got, c.want) + } + } + if !serverCtx.Viper.GetBool("grpc-only") { + t.Error("grpc-only set to true resolved false, so a node asked to run in gRPC-only mode " + + "would start Tendermint anyway") + } + }) +} + +// startFlagKeysWithTargetsOfTheirOwn are the start command's own flags that startInProcess reads, +// recorded for their operator-facing spelling. +// +// A KeyName rather than a KeySpec, because none of these resolves into a config struct a row could +// name a Path in: startInProcess reads them straight off the viper and branches. +// +// The names are literals because server's flagCPUProfile, flagTraceStore and flagGRPCOnly are +// unexported, so this package cannot spell them through the reader's own constant the way a KeyName +// target normally does. +// +// Exporting them is a real option rather than an impossibility, and it was considered: sei-cosmos is +// vendored here, so those are three lines in this repository and exporting them would let this record +// spell the keys through the reader itself, closing the hazard below instead of describing it. It is +// deferred because this branch is test-only and that is a change to shipped code, small as it is. +// Whoever picks it up gets to delete the paragraph after this one. +// +// The literal spellings bound what the record catches, and the bound is worth stating. The record +// holds the spelling this suite asserts against, so renaming a name here without renaming it in the +// assertions fails. It does not catch a rename in production, because the record and the +// assertions would then both still carry the old name. That case is caught one step over, where +// setting the flag fails once the flag no longer answers to the name it is given, and again by +// TestStartFlagNamesAreRegistered below, which says so in those terms rather than as a set failure. +var startFlagKeysWithTargetsOfTheirOwn = []configtest.KeyName{ + "cpu-profile", // TestStartPreRunResolvesTheKeysOnlyStartInProcessReads + "trace-store", // same + "grpc-only", // same, plus TestStartPreRunRebindsFlagsIntoTheApplyViper +} + +// TestStartFlagKeyNamesMatchTheRecordedNames pins the spelling of the three keys above. +func TestStartFlagKeyNamesMatchTheRecordedNames(t *testing.T) { + configtest.CheckKeyNames(t, "start_flags", nil, startFlagKeysWithTargetsOfTheirOwn...) +} + +// TestStartFlagNamesAreRegistered holds the three names against the command that has to answer to +// them, which is what the record above cannot do. // -// What is NOT reachable here, and is stated rather than silently skipped: cpu-profile, -// trace-store, the second GetConfig, grpc-only forcing GRPC.Enable, and the -// api/grpc-web gating all live inside unexported startInProcess, which opens listeners -// and starts a node. Pinning those needs an integration harness. +// A production rename does already fail, where setFlags asks the command to set a flag it no longer +// has. What it fails with is "set --cpu-profile: no such flag", which reads as a broken test rather +// than as an operator-facing flag having moved. This says the latter, and it removes the record's +// dependence on some other test happening to set all three. +func TestStartFlagNamesAreRegistered(t *testing.T) { + cmd := shippedStartCmd(t) + for _, name := range startFlagKeysWithTargetsOfTheirOwn { + if cmd.Flags().Lookup(string(name)) == nil { + t.Errorf("start no longer registers a flag named %q, so an operator's --%s stops being "+ + "accepted and startInProcess reads a key nothing populates. The three keys this file "+ + "records are spelled here as literals because server's constants are unexported, so a "+ + "rename there has to be carried into startFlagKeysWithTargetsOfTheirOwn and its "+ + "record by hand", string(name), string(name)) + } + } +} // nodeEscapedMarker is a fixed token so CI triage can grep one string for this failure, and // nodeEscaped carries it from the row that detected it to TestMain. @@ -77,14 +199,7 @@ func TestMain(m *testing.M) { func newStartCmd(t *testing.T, home *configtest.Home, flagValues map[string]string) (*cobra.Command, *server.Context, context.CancelFunc) { t.Helper() - root, _ := NewRootCmd() - root.SetOut(io.Discard) - root.SetErr(io.Discard) - - cmd, _, err := root.Find([]string{"start"}) - if err != nil { - t.Fatalf("find start: %v", err) - } + root, cmd := shippedRootAndStartCmd(t) if err := cmd.Flags().Set("home", home.Root); err != nil { t.Fatalf("set --home: %v", err) } @@ -103,6 +218,40 @@ func newStartCmd(t *testing.T, home *configtest.Home, flagValues map[string]stri return cmd, serverCtx, cancel } +// shippedRootAndStartCmd resolves the start command seid ships, and is the only place in this file +// that does. +// +// It goes through the real root rather than calling server.StartCmd directly, because AddCommands +// applies addStartFlags on top of StartCmd (sei-cosmos/server/util.go:365-366) and a second +// construction would be a copy: identical today, since addModuleInitFlags is a no-op, and silently +// divergent the moment a module registers or overrides a start flag. +// +// Both callers come through here rather than repeating it, for the same reason: two resolutions of the +// command seid ships drift the same way, which is the argument above turned on this file instead of on +// server.StartCmd. The root comes back with the command because newStartCmd drives the root's +// PersistentPreRunE, and a caller that only reads flags takes shippedStartCmd below. +func shippedRootAndStartCmd(t *testing.T) (root, start *cobra.Command) { + t.Helper() + + root, _ = NewRootCmd() + root.SetOut(io.Discard) + root.SetErr(io.Discard) + + start, _, err := root.Find([]string{"start"}) + if err != nil { + t.Fatalf("find start: %v", err) + } + return root, start +} + +// shippedStartCmd is shippedRootAndStartCmd for the tests that only read the flag set, where nothing +// runs and the root is not needed. +func shippedStartCmd(t *testing.T) *cobra.Command { + t.Helper() + _, start := shippedRootAndStartCmd(t) + return start +} + // FuzzStartPreRunPruningFailsFast pins the fail-fast: an unresolvable pruning // configuration stops start in PreRunE, before anything is opened. // @@ -288,10 +437,10 @@ func TestStartAfterChainIDAgreementHitsTheGenesisNilDeref(t *testing.T) { // a request rather than a guarantee, which is why the second wait is bounded too and the // message distinguishes a node that stopped from one that ignored the cancel. // -// The bounds are deliberately generous, because the terminal branch inverted the cost of -// being wrong. A bound that is too short no longer just files an early report: it aborts the -// whole package on a loaded -race shard, destroying results for every other test on nothing -// more than wall-clock evidence. A bound that is too long costs only a delayed report on a +// The bounds are deliberately generous, because the two ways of being wrong cost very +// different amounts. A bound that is too short aborts the whole package on a loaded -race +// shard, destroying results for every other test on nothing more than wall-clock evidence. +// A bound that is too long costs only a delayed report on a // path where something is already broken, since the happy path returns as soon as the panic // fires, in well under a second, and never waits at all. So the timeout is sized to outlast // any plausible shard rather than to fail fast, and the terminal branch is reached only after diff --git a/cmd/seid/cmd/testdata/start_flags.keys.golden b/cmd/seid/cmd/testdata/start_flags.keys.golden new file mode 100644 index 0000000000..b9718737fd --- /dev/null +++ b/cmd/seid/cmd/testdata/start_flags.keys.golden @@ -0,0 +1,4 @@ +# keys with a target of their own +"cpu-profile" +"trace-store" +"grpc-only" diff --git a/cmd/seid/cmd/testdata/wiring_coverage.txt b/cmd/seid/cmd/testdata/wiring_coverage.txt index 82dce79489..bad00274bb 100644 --- a/cmd/seid/cmd/testdata/wiring_coverage.txt +++ b/cmd/seid/cmd/testdata/wiring_coverage.txt @@ -6,5 +6,6 @@ # so this records which checks cover a section rather than how many times. # Regenerate with `go test .// -run TestWiringMatchesTheRecord -update` and read the diff. +start_flags CheckKeyNames state-sync CheckKeyNames tendermint CheckKeyNames diff --git a/sei-cosmos/server/config/config_fuzz_test.go b/sei-cosmos/server/config/config_fuzz_test.go index d6ed918444..c4ecbae259 100644 --- a/sei-cosmos/server/config/config_fuzz_test.go +++ b/sei-cosmos/server/config/config_fuzz_test.go @@ -31,19 +31,194 @@ import ( // deliberately excludes flag binding and env resolution — those belong to Apply and // are pinned in cmd/seid/cmd. What is left is the parse itself. -// newAppViper returns a viper holding the one key GetConfig unconditionally -// requires, plus whatever the caller adds. telemetry.global-labels has no default -// and no presence guard, so nothing can be parsed without it. +// globalLabelsKey is the one key GetConfig unconditionally requires, and newAppViper supplies it when +// the caller does not. +const globalLabelsKey = "telemetry.global-labels" + +// newAppViper returns a viper holding the one key GetConfig unconditionally requires, plus whatever +// the caller adds. telemetry.global-labels has no default and no presence guard, so nothing can be +// parsed without it. +// +// The caller's spelling wins if it supplies the key itself, which is set before the loop rather than +// inside it so that overriding the stub is a plain override rather than two writes racing. func newAppViper(t testing.TB, keys map[string]any) *viper.Viper { t.Helper() + requireViperCanHoldEveryKey(t, keys) v := viper.New() - v.Set("telemetry.global-labels", []any{}) + if !callerSupplies(keys, globalLabelsKey) { + v.Set(globalLabelsKey, []any{}) + } for k, val := range keys { v.Set(k, val) } return v } +// callerSupplies reports whether the caller already passed a key, normalized the way viper normalizes +// it. That is strings.ToLower and not EqualFold: EqualFold is Unicode case folding, which relates pairs +// viper's lowercasing does not, and the two sibling scans in this file already use ToLower. Contrived +// for ASCII configuration keys, but one relation modelled three ways is how the three drift apart. +func callerSupplies(keys map[string]any, key string) bool { + normalized := strings.ToLower(key) + for k := range keys { + if strings.ToLower(k) == normalized { + return true + } + } + return false +} + +// requireViperCanHoldEveryKey refuses a key set viper cannot hold every value of. +// +// There are two ways a value goes missing and they are checked separately, because they fail for +// different reasons and a reader chasing one should not be handed the other's message. Two keys can +// normalize to the same key, where the later Set overwrites the earlier, or one key can nest inside +// another, where the later Set re-nests around it. +// +// The two scans see different key sets, and the difference is the point. Only the caller's keys race: +// they are Set inside a loop over a Go map, so their order is randomized per run. globalLabelsKey is +// Set outside that loop and only when the caller did not supply it, so it can never be the party that +// loses a race, and folding it into the normalization scan would reject a caller overriding the stub +// deliberately. It stays in the nesting scan, where a caller passing telemetry or +// telemetry.global-labels.x destroys the key GetConfig requires however the order falls. +func requireViperCanHoldEveryKey(t testing.TB, keys map[string]any) { + t.Helper() + callerKeys := make([]string, 0, len(keys)) + for k := range keys { + callerKeys = append(callerKeys, k) + } + // Sorted where the slice leaves map order, not inside either scan. Both scans report the first + // pair they find, so without this the failure names a different pair run to run and stops being + // the stable string CI triage greps for. + sort.Strings(callerKeys) + + requireNoTwoKeysNormalizeAlike(t, callerKeys) + requireNoKeyNestsInsideAnother(t, append(callerKeys, globalLabelsKey)) +} + +// requireNoTwoKeysNormalizeAlike refuses two keys that are the same key once viper has seen them. +// +// Set lowercases before storing (viper.go:1503), so "Telemetry.Global-Labels" and +// "telemetry.global-labels" are one key there while being two in a Go map. Whichever is Set second +// wins and map order picks which, so the loss is the one nesting produces arriving by a different +// route. nestsInside cannot catch it: normalizing makes the pair equal, and equal keys are +// deliberately not a nesting relationship. +// +// Given caller keys only. A caller key that normalizes onto globalLabelsKey is an override rather +// than a race, because newAppViper supplies that stub only when the caller did not. +func requireNoTwoKeysNormalizeAlike(t testing.TB, callerKeys []string) { + t.Helper() + if first, second, found := firstNormalizationCollision(callerKeys); found { + t.Fatalf("%q and %q are two keys in this map and one key to viper, which lowercases before "+ + "storing, so whichever this helper happens to Set second wins and Go map order picks "+ + "which. Pass the key once, with the spelling the reader uses", first, second) + } +} + +// firstNormalizationCollision returns the first two keys that are one key once lowercased, so the +// scan is testable without driving a failure through testing.TB. +func firstNormalizationCollision(all []string) (first, second string, found bool) { + seen := make(map[string]string, len(all)) + for _, key := range all { + normalized := strings.ToLower(key) + if earlier, ok := seen[normalized]; ok { + return earlier, key, true + } + seen[normalized] = key + } + return "", "", false +} + +// TestNormalizationCollisionIsCaseOnly holds the scan to the pairs it exists for, alongside the ones +// it must leave alone. +func TestNormalizationCollisionIsCaseOnly(t *testing.T) { + for _, c := range []struct { + name string + keys []string + want bool + }{ + {"the corpus as it stands", []string{globalLabelsKey, "pruning", "pruning-keep-every"}, false}, + {"case variants of one key", []string{"Telemetry.Global-Labels", globalLabelsKey}, true}, + {"a nesting pair is not this", []string{"telemetry", globalLabelsKey}, false}, + {"distinct keys sharing a prefix", []string{"pruning", "pruning-keep-every"}, false}, + {"the same key written once", []string{globalLabelsKey}, false}, + } { + t.Run(c.name, func(t *testing.T) { + if _, _, got := firstNormalizationCollision(c.keys); got != c.want { + t.Errorf("firstNormalizationCollision(%q) found=%v, want %v", c.keys, got, c.want) + } + }) + } +} + +// requireNoKeyNestsInsideAnother refuses a key set viper.Set cannot hold both halves of. +// +// viper.Set re-nests around the dots. Setting a.b after a.b.c replaces the sub-tree, and setting +// a.b.c after a.b replaces the scalar with a fresh map, so one of the two values is always destroyed. +// Between two caller keys, which one survives is decided by the map order this helper ranges in and +// so varies per run. Against globalLabelsKey the loss is not a race but is still a loss: the key +// GetConfig requires goes away. +// +// The reason that is worth a guard rather than a comment is where the loss shows up. It is visible +// through the per-key Get that GetConfig uses, not only through AllSettings, so a colliding pair +// would not fail loudly here. It would resolve a wrong value on roughly half of all runs, and the +// test would read as flaky rather than as wrong. +// +// Nesting is a dotted-segment relationship, never a textual one. pruning and pruning-keep-every are +// separate keys that share a prefix, as are state-commit.sc-write-mode and its -enable-auto sibling, +// and a plain HasPrefix would reject the corpus as it stands. +func requireNoKeyNestsInsideAnother(t testing.TB, all []string) { + t.Helper() + for _, outer := range all { + for _, inner := range all { + if !nestsInside(outer, inner) { + continue + } + t.Fatalf("%q nests inside %q, so viper.Set can hold one of them but not both, and which "+ + "one survives depends on Go map order. Set them on separate vipers, or give the outer "+ + "key a leaf of its own, rather than passing both to newAppViper", inner, outer) + } + } +} + +// nestsInside reports whether inner is a dotted child of outer, which is the only relationship +// viper.Set collapses. A shared textual prefix is not one. +// +// The relationship is on viper's normalized key rather than the written one. Set lowercases before it +// nests (viper.go:1503), so "Telemetry" and "telemetry.global-labels" collide there while comparing as +// written would not, and a guard that missed the pair would let the map-order loss back in wearing the +// hardest shape to diagnose. +func nestsInside(outer, inner string) bool { + outer, inner = strings.ToLower(outer), strings.ToLower(inner) + return outer != inner && strings.HasPrefix(inner, outer+".") +} + +// TestNestsInsideIsADottedSegmentRelationship holds the predicate to the boundary that matters, +// using the pairs this file actually passes. +func TestNestsInsideIsADottedSegmentRelationship(t *testing.T) { + for _, c := range []struct { + outer, inner string + want bool + }{ + {"pruning", "pruning-keep-every", false}, + {"state-commit.sc-write-mode", "state-commit.sc-write-mode-enable-auto", false}, + {"a.bc", "a.b.c", false}, + {"a.b", "a.b", false}, // identical keys overwrite rather than nest + {"a.b", "a", false}, // the relationship has a direction + {"a", "a.b", true}, + {"a.b", "a.b.c", true}, + {globalLabelsKey, globalLabelsKey + ".x", true}, + // viper lowercases before nesting, so case is not what separates two keys. + {"Telemetry", "telemetry.global-labels", true}, + {"telemetry", "TELEMETRY.GLOBAL-LABELS", true}, + {"Telemetry", "Telemetry", false}, + } { + if got := nestsInside(c.outer, c.inner); got != c.want { + t.Errorf("nestsInside(%q, %q) = %v, want %v", c.outer, c.inner, got, c.want) + } + } +} + // FuzzGetConfigGlobalLabels pins the one key that can stop a node booting by being // absent rather than wrong. // @@ -82,25 +257,87 @@ func FuzzGetConfigGlobalLabels(f *testing.F) { } v := viper.New() - v.Set("telemetry.global-labels", labels) + v.Set(globalLabelsKey, labels) cfg, err := GetConfig(v) if err != nil { t.Fatalf("a well-typed global-labels list must parse, got %v", err) } - // Only two-element labels survive; the rest vanish without a diagnostic. - wantKept := 0 + // Only two-element labels survive; the rest vanish without a diagnostic. Each survivor is + // held to its own key and value in the order written, because an arity comparison passes + // just as well with a pair reversed or with one label's key against another's value. + want := make([][]string, 0, labelCount) if elemsPerLabel == 2 { - wantKept = labelCount + for i := range labelCount { + want = append(want, []string{ + fmt.Sprintf("%s-%d-0", content, i), + fmt.Sprintf("%s-%d-1", content, i), + }) + } } - if len(cfg.Telemetry.GlobalLabels) != wantKept { - t.Fatalf("%d labels of %d elements resolved to %d kept, want %d "+ - "(a label whose length is not exactly 2 is dropped silently)", - labelCount, elemsPerLabel, len(cfg.Telemetry.GlobalLabels), wantKept) + if !reflect.DeepEqual(cfg.Telemetry.GlobalLabels, want) { + t.Fatalf("%d labels of %d elements resolved to %v, want %v (a label whose length is not "+ + "exactly 2 is dropped silently, and the survivors keep their written order)", + labelCount, elemsPerLabel, cfg.Telemetry.GlobalLabels, want) } }) } +// TestGetConfigPanicsOnANonStringGlobalLabel records the one malformed [telemetry] shape this reader +// does not report. +// +// Every other bad shape returns an error naming what it could not parse: a global-labels value that +// is not a list (config.go:421), and an entry within it that is not a list (config.go:429). A +// two-element entry holding a non-string reaches an unchecked assertion instead (config.go:432), so +// global-labels = [["chain", 42]] in app.toml takes the node down with an interface-conversion panic +// rather than the diagnostic its siblings produce. +// +// Recorded rather than repaired, and owned by PLT-976 item 4 rather than left to whoever reads this +// next. Converting it to an error is the change to make, and it belongs with +// the reader that replaces this one, since an operator whose app.toml currently panics would start +// booting into a node that logs a parse failure instead. +func TestGetConfigPanicsOnANonStringGlobalLabel(t *testing.T) { + for _, label := range [][]any{ + {"chain", 42}, // a non-string value + {7, "chain"}, // a non-string key + {1, 2}, // neither is a string + } { + t.Run(fmt.Sprintf("%v", label), func(t *testing.T) { + v := viper.New() + v.Set(globalLabelsKey, []any{label}) + + // Catching is kept apart from judging so exactly one message can reach the reader. A + // t.Fatalf in the body calls Goexit, a deferred recover sees nil during that unwind + // because Goexit is not a panic, and a second t.Fatalf in the defer would then report + // "no longer panics" as the last thing printed, which is an artifact of the unwind + // rather than the failure. + recovered, err := getConfigCatchingPanic(v) + switch { + case recovered == nil && err != nil: + t.Fatalf("global-labels %v returned %v instead of panicking, so the assertion at "+ + "config.go:432 is now guarded and this recording is stale", label, err) + case recovered == nil: + t.Fatalf("global-labels %v no longer panics. If it now returns an error, that is "+ + "the better behavior and it changes which app.toml files start a node, so "+ + "replace this recording with the error the reader reports", label) + case !strings.Contains(fmt.Sprint(recovered), "interface conversion"): + t.Fatalf("global-labels %v panicked with %v rather than the unchecked assertion at "+ + "config.go:432, so the failure moved and this recording no longer describes it", + label, recovered) + } + }) + } +} + +// getConfigCatchingPanic runs GetConfig and reports what it did rather than judging it. It touches no +// testing.TB, which is the point: a t.Fatalf inside a deferred recover cannot then fire during the +// Goexit that another t.Fatalf started. +func getConfigCatchingPanic(v *viper.Viper) (recovered any, err error) { + defer func() { recovered = recover() }() + _, err = GetConfig(v) + return nil, err +} + // TestGetConfigRequiresGlobalLabels pins the absent-key failure on its own. This is // the row that turns a missing telemetry section into a node that will not start. func TestGetConfigRequiresGlobalLabels(t *testing.T) { @@ -114,23 +351,6 @@ func TestGetConfigRequiresGlobalLabels(t *testing.T) { } } -// TestGetConfigPanicsOnNonStringLabel records that the inner element assertions are -// unchecked. A label list of the right shape but the wrong element type takes the -// node down with a raw interface-conversion panic rather than an error naming -// telemetry. -func TestGetConfigPanicsOnNonStringLabel(t *testing.T) { - v := viper.New() - v.Set("telemetry.global-labels", []any{[]any{1, 2}}) - - defer func() { - if r := recover(); r == nil { - t.Fatal("a non-string label must panic; if it is now an error, the diagnostic " + - "improved and this row should say so") - } - }() - _, _ = GetConfig(v) -} - // grpcClamp is a duration key GetConfig clamps rather than accepts verbatim. type grpcClamp struct { Key string @@ -690,14 +910,25 @@ func TestManifestNamesEveryField(t *testing.T) { configtest.CheckManifestCoversEveryField(t, "state-sync", DefaultConfig().StateSync, stateSyncKeys) } -// FuzzConfigValidateBasic pins the two conditions that reject an otherwise -// parseable app.toml. +// FuzzConfigValidateBasic pins the two conditions under which an otherwise parseable app.toml is +// reported as invalid, and the two error strings that distinguish them. +// +// An empty minimum-gas-prices fails, because a validator accepting zero-fee transactions is a +// misconfiguration rather than a choice. And pruning "everything" with state-sync snapshots enabled +// fails, because a node cannot serve a snapshot of state it has already pruned. // -// An empty minimum-gas-prices fails, because a validator accepting zero-fee -// transactions is a misconfiguration rather than a choice. And pruning -// "everything" with state-sync snapshots enabled fails, because a node cannot -// serve a snapshot of state it has already pruned. Both are the rare case in this -// surface where a bad combination is refused rather than absorbed. +// Neither stops a node. start.go:308 is the only caller on the boot path, and it logs the error and +// carries on to build the app, so both conditions are reported rather than refused. The one other +// caller, sei-cosmos/testutil/network/util.go:31, does return it, but that is the in-process test +// network and is never linked into seid. +// +// The message it logs is a fixed string naming an empty minimum-gas-prices, which is one of the two +// causes: an operator whose pruning and snapshot settings conflict is told their fee floor is empty, +// and then boots into a node that cannot serve the snapshots it advertises. +// +// So the distinctness of these two errors is load-bearing in a way the caller does not currently use, +// and that is what the assertions below hold. Recorded rather than repaired, because deciding whether +// this should abort a boot changes which existing configurations still start. func FuzzConfigValidateBasic(f *testing.F) { f.Add("0.01usei", "default", uint64(0)) f.Add("", "default", uint64(0)) @@ -726,6 +957,27 @@ func FuzzConfigValidateBasic(f *testing.F) { t.Fatalf("min-gas-prices=%q pruning=%q snapshot-interval=%d must pass ValidateBasic, got %v", minGasPrices, pruning, snapshotInterval, got) } + + // The pruning conflict has to identify itself, because its caller logs a fixed fee-floor + // message for either cause and this string is the only thing left that tells the two apart. + // Both directions are asserted: it must name the conflict, and it must not read as a fee-floor + // problem. Asserting only the second would pass for any error at all, including one that named + // neither cause. + if minGasPrices != "" && got != nil { + if !strings.Contains(got.Error(), "state sync snapshots") { + t.Fatalf("pruning=%q snapshot-interval=%d was rejected as %q, which no longer names the "+ + "snapshot conflict. start.go:308-311 discards this error and logs a fixed fee-floor "+ + "string for either cause, so nothing surfaces this text to an operator; it is the only "+ + "thing in code that tells the two causes apart", + pruning, snapshotInterval, got) + } + if strings.Contains(got.Error(), "min gas price") { + t.Fatalf("pruning=%q snapshot-interval=%d was rejected as %q while minimum-gas-prices "+ + "was set to %q. The two rejection causes now report the same way, so nothing in code "+ + "tells them apart and the fixed string start.go logs names the wrong one", + pruning, snapshotInterval, got, minGasPrices) + } + } }) } @@ -922,7 +1174,8 @@ var grpcWebKeys = []configtest.KeySpec{ // // global-labels is not a row. It is read as a bare type assertion whose absence fails GetConfig // outright and whose shape rules are their own subject, so it has dedicated targets above -// (FuzzGetConfigGlobalLabels, TestGetConfigRequiresGlobalLabels, TestGetConfigPanicsOnNonStringLabel) +// (FuzzGetConfigGlobalLabels, TestGetConfigRequiresGlobalLabels, +// TestGetConfigPanicsOnANonStringGlobalLabel) // and is recorded by name rather than driven as a row. var telemetryKeys = []configtest.KeySpec{ { @@ -960,7 +1213,7 @@ var telemetryKeys = []configtest.KeySpec{ // telemetryKeysWithTargetsOfTheirOwn is global-labels, recorded for its name because its behaviour // is driven by targets rather than by a row. -var telemetryKeysWithTargetsOfTheirOwn = []configtest.KeyName{"telemetry.global-labels"} +var telemetryKeysWithTargetsOfTheirOwn = []configtest.KeyName{globalLabelsKey} func readRosetta(t testing.TB) func(configtest.AppOpts) (any, error) { return sectionOfGetConfig(t, func(c Config) any { return c.Rosetta }) @@ -1149,6 +1402,14 @@ func TestGetConfigAbsentSectionDivergences(t *testing.T) { {"pruning", cfg.Pruning, def.Pruning, true}, {"pruning-keep-recent", cfg.PruningKeepRecent, def.PruningKeepRecent, true}, {"pruning-interval", cfg.PruningInterval, def.PruningInterval, true}, + // Diverges here and is rescued twice downstream, and which rescue a booted node relies on is + // the distinction to carry off this row. concurrency-workers is a registered start flag + // defaulting to DefaultConcurrencyWorkers (start.go:224) and bound in PreRunE (start.go:117) + // ahead of both production calls (start.go:168 and :303), so there this same read takes the + // flag's default whenever app.toml is silent and never resolves 0. The 0 recorded here belongs + // to this file's flag-less viper. baseapp.New substituting DefaultConcurrencyWorkers for a + // resolved 0 (baseapp.go:316-320) is a second net behind that one, reached only when appOpts + // carries no flags or an operator writes 0 outright. {"concurrency-workers", cfg.ConcurrencyWorkers, def.ConcurrencyWorkers, true}, {"occ-enabled", cfg.OccEnabled, def.OccEnabled, true}, {"halt-height", cfg.HaltHeight, def.HaltHeight, false}, @@ -1335,13 +1596,17 @@ func TestBaseConfigKeyNamesMatchTheRecordedNames(t *testing.T) { configtest.CheckKeyNames(t, "base_config", baseConfigKeys) } -// TestBaseConfigManifestNamesEveryField enforces the manifest's claim, and records the one field -// that has no key. +// TestBaseConfigManifestNamesEveryField enforces the manifest's claim, and records the one field this +// reader leaves alone. // // PruningKeepEvery carries a mapstructure tag of pruning-keep-every and a declared default of "0", -// and GetConfig never reads it. So no app.toml value reaches it through this reader, and the -// exemption below is the record of that rather than a gap in the manifest. It is the shape of thing -// a replacement manager would otherwise try to map a key onto. +// and GetConfig never reads it, so the exemption below records that rather than a gap in the manifest. +// +// It is not an unreachable field, and the difference matters to anything reproducing this surface. +// server/pruning.go reads pruning-keep-every through appOpts and feeds it to the custom pruning +// strategy, pinned by pruning_test.go and pruning_fuzz_test.go. So the key has a reader and a +// consumer; what it does not have is a path through this struct. A replacement manager has to carry +// the key and must not expect this field to be where it lands. func TestBaseConfigManifestNamesEveryField(t *testing.T) { configtest.CheckManifestCoversEveryField(t, "base_config", DefaultConfig().BaseConfig, baseConfigKeys, @@ -1349,6 +1614,55 @@ func TestBaseConfigManifestNamesEveryField(t *testing.T) { ) } +// TestGetConfigLeavesPruningKeepEveryEmpty holds the exemption in +// TestBaseConfigManifestNamesEveryField to being true, and records what the field actually carries. +// +// The exemption says GetConfig does not read pruning-keep-every, which is what lets the manifest omit a +// row. Nothing checked it, so GetConfig could start reading the key and the exemption would quietly +// become a false claim about a field that now resolves. +// +// What the field carries is worth stating, because it is not the declared default. GetConfig builds +// BaseConfig as a struct literal and never assigns PruningKeepEvery, so it stays Go's zero value, the +// empty string, where DefaultConfig declares "0". So this is a divergence as well as an omission, and +// an empty string is not a number the custom pruning strategy can use. It is excluded from the +// divergence table only because it has no manifest row to anchor there. +// +// The other half of the split is pinned elsewhere: server/pruning.go reads the key through appOpts and +// feeds the custom strategy, held by pruning_fuzz_test.go. Between them the two readers' disagreement +// about one key is recorded from both sides. +// +// Asserted with the key set to a value nothing else produces, so a green run means the field is +// genuinely untouched by this reader rather than coincidentally equal to something. +func TestGetConfigLeavesPruningKeepEveryEmpty(t *testing.T) { + const nothingElseProduces = "4321" + + cfg, err := GetConfig(newAppViper(t, configtest.AppOpts{ + "pruning-keep-every": nothingElseProduces, + })) + if err != nil { + t.Fatalf("GetConfig: %v", err) + } + + if cfg.PruningKeepEvery != "" { + t.Errorf("GetConfig resolved PruningKeepEvery to %q with pruning-keep-every set to %q, where it "+ + "leaves the field empty today. This reader now reads the key, so the exemption in "+ + "TestBaseConfigManifestNamesEveryField is false and the field needs a manifest row", + cfg.PruningKeepEvery, nothingElseProduces) + } + // The divergence half: the reader's empty string has to differ from what DefaultConfig declares. + // Compared against the declared default rather than against "" so it reads as the property, and + // carrying no interpolated value because both sides are the empty string whenever it fires. What + // the declared default actually is belongs to server_config.golden, which records it; asserting a + // particular value here would fire on a default moving to another non-empty string, where the + // divergence this test owns is still intact. + if cfg.PruningKeepEvery == DefaultConfig().PruningKeepEvery { + t.Error("GetConfig leaves PruningKeepEvery empty and DefaultConfig now declares it empty too, " + + "so the two agree and the divergence this row records is gone. This half exists only to " + + "catch that collapse; a default that changed to some other value lands in the " + + "server_config.golden diff instead") + } +} + // grpcKeys covers the three [grpc] keys read as plain casts. // // The section is where the guarding in this reader is most complete, which is why only three keys diff --git a/testutil/configtest/AGENTS.md b/testutil/configtest/AGENTS.md index e82817a173..546a32203c 100644 --- a/testutil/configtest/AGENTS.md +++ b/testutil/configtest/AGENTS.md @@ -140,8 +140,10 @@ go test ./evmrpc/config/ -run TestKeyNames -update why a row that reaches its key through the reader's own flag constant needs it. Write one row per key, including when two keys land in the same struct field. The -manifest is what the differential enumerates, so a key with no row is a key the -comparison never makes. +manifest is what the per-key checks iterate, so a key with no row is a key `CheckRow`, +`CheckAbsent` and the seed check never look at. The differential is not a second net +for that. It compares whole resolved vipers and reads no manifest, so it can report +that two readers agree without either one being pinned to anything. `CheckManifestCoversEveryField` covers the weaker half of that automatically: every resolved field must be named by some row's `Path` or `AlsoWrites`, or exempted at the @@ -177,6 +179,39 @@ five flatkv keys as rows, and the 53 exemptions left would each say truthfully t field carries no configuration key. Unbuilt. Meanwhile, wire the check where the section has one reader; a demotion is caught for every section by the record's marker regardless. +`StateStoreConfig` is split the same way, with eleven keys of its own that both readers read. +Each count is per struct, so the pair of them is twenty-two. Neither half can resolve to +different values, and the reason is simpler here than in `[state-commit]`: neither reader guards +these keys at all. `parseSSConfigs` assigns `cast.ToX(appOpts.Get(k))` unconditionally +(`app/seidb.go:198-210`) and `GetConfig` uses plain typed getters with no `IsSet` +(`config.go:629-641`), so both resolve an absent key to the Go zero rather than to the declared +default. There is no guard to differ over. What the split costs is a second copy rather than a +disagreement, and the reason is specific to which copy: nothing reads these fields **on the +`Config` `GetConfig` returns**, so the store is built from `parseSCConfigs` and `parseSSConfigs` +alone. The fields themselves are read elsewhere, and conflating the two is the mistake to avoid. +`SetAppConfigByMode` writes `StateStore.Enable` and `StateStore.KeepRecent` per node mode +(`app/params/config.go`), and `sei-db/config/toml.go` renders eleven of `StateStore`'s thirteen +fields into the app.toml template. + +Those two remaining fields are worth knowing about because they are not the shape they look +like. `keep-last-version` and `use-default-comparer` are read by neither `parseSSConfigs` nor +`GetConfig`, so no operator key reaches them at all: they hold their in-code defaults on every +node and are flipped only in code, by the receipt store and by the EVM state store. `app`'s +manifest already exempts both by name for that reason, and two tests assert the template does +not carry them. That is a different case from a key with a reader and no template line, which +`state-commit`'s `sc-write-mode-enable-auto` and `flatkv.*` keys are: those an operator can set +by hand, they are simply not rendered. A replacement manager needs both classes, because one is +a field configuration cannot address and the other is a key the generated file never mentions. + +The three EVM fields are a third trap and the sharpest of them. Their `mapstructure` tags are +`evm-split`, `evm-db-directory` and `evm-separate-dbs`, while the template and both readers use +`evm-ss-split`, `evm-ss-db-directory` and `evm-ss-separate-dbs`. So a replacement that reads this +struct the obvious way, by unmarshalling the `state-store` subtree onto it, binds three keys +nothing has ever written and picks up the two fields the legacy node never reads. + +PLT-955 is what that distinction looks like when it goes wrong, an archive +node pruning history because the mode's writes to those fields do not reach the store. + ## Renaming a Key Key names are recorded in `testdata/
.keys.golden`, one quoted key per line, and @@ -440,6 +475,129 @@ process environment, `$HOME`, and the executable basename all feed the result. `EnvValueIsSettable` decline the values with no faithful spelling, which keeps a parse failure from being attributed to the layer under test. +## Reads Whose Call Site Cannot Be Pinned + +Four live reads resolve their value where this suite can reach it and consume it where it +cannot. For each, a rename of the key fails somewhere, and a change to the read itself does not, +so the tests would keep describing a reader that had moved. + +| Read | Pinned | Not pinned | +|---|---|---| +| `root.go:296`, `minimum-gas-prices` into `baseapp.SetMinGasPrices` | that the flag is registered, and what the expression resolves to | the call site, an inline argument to `newApp`'s `app.New` | +| `root.go:297`, `min-retain-blocks` into `baseapp.SetMinRetainBlocks` | the recorded cast result | the same inline argument | +| `startInProcess`'s `cpu-profile`, `trace-store` and `grpc-only` | what each resolves to in the viper `startInProcess` reads | the read sites, inside an unexported function needing a booted node | + +**Two of the three start keys would fail quietly.** `cpu-profile` and `trace-store` would accept +an operator's value and write no profile and no trace file, with nothing to notice. `grpc-only` +is visible instead: a node asked to serve gRPC only would start Tendermint anyway. + +**Effects are deliberately not pinned.** Whether the profiler starts, whether a trace file +appears, and whether `grpc-only` changes which listeners bind all need a running node. The +differential the PLT-775 cutover rests on compares the two resolved channels after `Apply`, so +where resolution is identical `startInProcess` reads identical values and its effects follow. +Pinning them with a booted node re-derives what channel equality already gives. + +The repo's `inprocess` package is not the tool for closing this. It calls `tmnode.New` directly +and injects its own `AppOptions`, so it never executes `startInProcess` or the legacy resolver, +and a green assertion through it would characterise that harness rather than seid. It is also +capped at one node boot per test binary, because `app.New` wires process-global singletons that +never re-initialise. + +## The minimum-gas-prices Separator + +One reader governs a node, and two artifacts document a syntax it rejects. + +`root.go:296` hands `cast.ToString` of the key to `baseapp.SetMinGasPrices`, which calls +`sdk.ParseDecCoins` and panics on anything it cannot parse (`options.go:24-28`). That panic is +the whole boot, and `ParseDecCoins` separates denominations with a comma. + +The start flag's own help text offers `0.01photino;0.0001stake` as its example +(`start.go:208`), and `Config.GetMinGasPrices` splits on `";"` (`config.go:323`). The two +syntaxes are disjoint rather than merely different: no multi-denomination value is accepted by +both, and the spelling an operator is shown is the spelling that panics. Both agree on one +denomination, which is the shape of the default and of nearly every deployment, and that is why +this has never been reported. It surfaces the first time an operator prices a second fee token +by following the example. + +`GetMinGasPrices` has no caller outside itself, so it documents an intent rather than being a +second live resolution. There is one answer at runtime and two artifacts describing another. + +Recorded rather than repaired, and the halves of a repair carry different risk. Correcting the +help text and the getter is prose and dead code, and what operators are told today is a value +that takes the node down, so nothing is preserved by leaving it. Teaching `ParseDecCoins` the +semicolon widens the fee-floor grammar for every node, and once operators write semicolons, +narrowing back breaks them. Aligning the documentation down to the comma the parser already +accepts is the cheaper direction and does not spend that door. PLT-976 item 1. + +## The min-retain-blocks Fan-Out + +`min-retain-blocks` is the only key in this tree that two live consumers read, and +`app/receipt_store_config_test.go` holds it still. Every other twice-read key has a dead +second reader: `sei-cosmos/server/config.GetConfig` parses `[state-commit]` and +`[state-store]` into a `Config` nobody hands to the store, so a disagreement there cannot +reach a node. Both of this key's readers run. + +| Reader | Cast | Becomes | +|---|---|---| +| `cmd/seid/cmd/root.go:297` | `cast.ToUint64` | the Tendermint block-retention height, through `baseapp.SetMinRetainBlocks` | +| `app/receipt_store_config.go:27` | `cast.ToInt` | EVM receipt retention, through the receipt store's `KeepRecent` | + +So one number an operator sets for block retention silently also sets receipt retention, and +the two go through different casts. + +**The two halves are not covered equally.** The receipt half is pinned against its reader, +since the table drives `readReceiptStoreConfig` and changing that cast fails. The block half +cannot be pinned from a test: `root.go` builds that argument inline inside `newApp`'s +`app.New` call, so reaching it needs a booted node. The block column is a recorded literal, +and nothing fails if `root.go:297` changes its cast. + +**Where the casts disagree, receipts survive by one of two mechanisms.** For every value an +operator would sensibly write they agree, and the fan-out is then just a coupling. + +*The guard.* A `KeepRecent` at or below zero never arms a pruner on either backend. pebbledb +returns before starting one (`sei-db/ledger_db/receipt/receipt_store.go:363`) and litt skips +its TTL branch (`sei-db/ledger_db/receipt/litt_receipt_store.go:138`). Every disagreement +reachable as a string takes this route, since a negative is kept by `ToInt` and floored by +`ToUint64`, and a decimal past int64 is floored by `ToInt` and kept by `ToUint64`. + +*Saturation.* Reachable only where a value arrives as a `float64` at or above 2^63. There +`cast.ToInt` saturates to `MaxInt64` rather than flooring, so `KeepRecent` is positive and the +guard does not apply. Here the two backends diverge: + +- **pebbledb**, the shipped default, is safe by an ordinary bound. Its pruner computes + `pruneVersion := latestVersion - keepRecent` and prunes only where that is above zero + (`receipt_store.go:379-380`), so `MaxInt64` puts the target far below zero. No change to a + TTL multiplier can undo that. +- **litt** is safe by accident. It multiplies `KeepRecent` by an unexported per-block TTL, and + `MaxInt64` times that multiplier overflows to a negative `Duration`, which + `sei-db/db_engine/litt/disktable/gc_manager.go:273` reads as no expiry. A different + multiplier could wrap the product to a small positive TTL that prunes on a schedule nobody + chose. + +The litt multiply is the third gap in the table below. Nodes on the shipped default are +unaffected either way, which is what scopes that gap rather than closing it. + +Both readers are recorded rather than repaired. Making the two casts one would change what a +node retains for any operator currently relying on the out-of-range behaviour, which is the +kind of change this suite pins instead of making. PLT-976 tracks it. + +## Disclosed Gaps + +Separate from the classes below, which the suite cannot reach, these are gaps it could close +and has not. Each is disclosed where it bites as well, so a reader meeting one in a test file +is not relying on finding this list; the list exists because a reader deciding what to improve +should not have to grep five files for it. + +Each entry names the production change that would close it. None is made here, because a +characterization branch stays test-only, and each is small enough that the reason it is open is +scope rather than difficulty. + +| Gap | What would close it | +|---|---| +| Nothing fails if `cmd/seid/cmd/root.go:296` or `:297` changes the cast or key it hands `baseapp`. Both are inline arguments inside `newApp`'s `app.New` call, so reaching them needs a booted node. | Extract each into a named constructor taking only `AppOpts`, which the suite can then drive. An AST assertion over the call site is not the answer, since it pins spelling rather than behaviour and would redden on a refactor that changed nothing. | +| The `start_flags` record holds `cpu-profile`, `trace-store` and `grpc-only` as literals, because `sei-cosmos/server`'s constants for them are unexported. A rename in production is caught only where setting the flag fails, which reports as a missing flag rather than as an operator-facing key having moved. | Export those three constants, or accessors for them. `sei-cosmos` is vendored here, so this is three lines in this repository. | +| Nothing pins the multiply that makes a saturated receipt `KeepRecent` harmless on the **litt** backend. It happens in `sei-db` against an unexported per-block TTL multiplier, so a change there that landed the product small and positive would prune receipts on a litt-backed arm64 node with this suite green. Nodes on the shipped default backend are bounded instead by `pruneVersion > 0` (`sei-db/ledger_db/receipt/receipt_store.go:379-380`), which no multiplier change can undo. | `sei-db` exports the multiplier, or better a helper returning the TTL for a given `KeepRecent`. Pinning a copy of the constant against another copy, which this suite did briefly, checks nothing. | + ## Out of Scope The suite covers the viper resolution and the keys `app.New` reads back out of the