[ConfigManager] Register Node Sections 2/4 - #3993
Merged
Merged
Conversation
Four sections, each registered by the package that owns its struct, so the struct, the values and the keys come from one place and cannot drift apart. The keys derive from the mapstructure tags, which is what makes the registry's spelling and each reader's own constants the same strings, and each package's test holds the two against each other rather than against a written-out list. admin_server 2 keys giga_executor 2 keys receipt-store 6 keys wasm 3 keys Three register their own struct. wasm needs a schema, because the upstream type carries no mapstructure tags at all, so keys derived from it would be field names rather than the ones the module reads. Its simulation gas limit is text because the field it stands for is an optional number and absent is a meaning of its own: unset means the consensus block gas limit applies, which no number can say. Two of that type's settings declare nothing, one having no key any reader resolves and the other written into app.toml by the template and read by nothing. Registering the receipt store needed a distinction the registry did not draw. mapstructure reads a tag of "-" as skip this field, and a configuration struct uses it for a field something else assigns: KeepRecent comes from the global min-retain-blocks flag at the app layer, ExternalPruning from whatever constructs the garbage collector. The registry read that as a missing name and refused the whole section. Such a field now declares no key, which is narrower than declaring one that resolves to a default and safer for the same reason: a declared key is written at override precedence, so a default would land on top of the value that code assigned, and a node with min-retain-blocks set would silently keep nothing. A field with no tag at all stays a defect, because that is the opposite intent, a key nothing names reaching no field. The skip lives in tagOf, which both walks already share, so the declared keys and the rendered defaults describe the same fields. Reverting either walk's skip on its own fails a test. The recorded configuration surface does not move: nothing consumes the registry yet, and no golden changed. 100% of statements in config/registry, race clean. Three mutations each fail a named test: refusing a dash again, skipping an untagged field, and letting the two walks disagree.
Four sections owned by the app package, each registered where its struct lives. genesis 2 keys light_invariance 1 key state-commit 20 keys state-store 12 keys light_invariance registers the type its reader fills. The other three declare a schema, because keys derived from the type the reader fills are not the keys the reader looks up. The genesis import type carries no mapstructure tags at all. State store and state commit both tag their fields with something other than the name resolved from configuration, and state commit nests its settings under three inner structs while the keys are flat names on the section, apart from the one flat key-value setting that has a segment of its own. State commit's write mode is text rather than the reader's named type, because the reader parses a written name into that type itself. Declaring the named type would have one key answer as a named string from these defaults and as a plain one from an operator's file. Each section's test holds its resolved keys and values against the reader's own constants and its own defaults. Resolving is what it compares, rather than the registered struct, because the resolved map carries the key a tag produced and the value that tag's field held: a comparison of struct to struct agrees with itself while two tags sit on the wrong fields, since each field still holds the value the test names for it. Putting the genesis tags on each other's fields leaves the key set identical and fails the test. State store's declared defaults are not what its reader produces for a file missing those keys. It starts from the declared defaults, then assigns eleven of its twelve fields straight from a lookup with no check that the key was present, so an absent key casts to a zero and clobbers the default beside it: the store reads as disabled, with no backend, keeping every version, and committing synchronously. Only the snapshot key is guarded, and its own comment at the read says why. A node whose app.toml predates one of the other keys therefore runs the clobbered value today and the declared default once something installs this section, and guarding the remaining reads is what makes those the same thing. The recorded configuration surface does not move, because nothing consumes the registry on a boot path yet.
Four sections, each registered by the package that owns its struct. eth_blocktest 2 keys eth_replay 4 keys evm 57 keys evm_query 1 key All four register the struct their reader fills. None needs a schema, because in each of them the mapstructure tags already spell the keys the reader looks up, so the registry derives what a node reads and nothing restates a list of fifty-seven keys. Each package's test holds the derived keys against the reader's own constants, which are the second statement of the same set in the same file: a rename that moves one and not the other compiles. The EVM section resolves the same values for every mode. Nothing consults a node's kind while reading these keys, so a file missing them serves both interfaces whatever kind of node it is, and that is what these resolve to. A node seid init provisioned is the other case and needs nothing here, since that path writes the two interface toggles per mode and a written value is what resolves. Two of that section's values come from the machine rather than from a decision. The simulation call limit is the processor count and the worker pool is twice it, capped, so they describe whichever host resolved them. That is stated where they are declared, because a caller rendering them into a file carries one host's sizing to whatever reads that file next. Replay declares three of its four keys under the name the template writes and one under a different one. The template renders eth_replay_contract_state_checks and the reader looks up contract_state_checks, so every generated file already carries a name nothing resolves. The declared key is the one a value reaches a reader through, and a test refuses the other: declaring it would add a key an operator can set and no reader answers, which is worse than the mismatch, because the value would look as though it applied. The recorded configuration surface does not move, because nothing consumes the registry on a boot path yet.
Five sections whose keys belong to the Cosmos server, and the two registry capabilities they need. api 8 keys base 14 keys grpc 11 keys state-sync 3 keys telemetry 7 keys These sections have no owning package here. Their structs and their readers live in sei-cosmos, which this repository vendors rather than authors, so there is nowhere upstream to put a registration this registry would see. Four of the five register the upstream struct directly, because its mapstructure tags already name the keys the reader resolves. A section can now declare keys at the root of the file. The node-wide settings are written at the top of app.toml and read as pruning and halt-height, with no segment in front, so a section carrying a name into every key would rename all fourteen and an operator's existing file would reach none of them. A section therefore has a name it is looked up by and a prefix its keys carry, and for a root section the prefix is empty. Both walks build a key through one function, so a root key gains no separator on either side; reverting either one on its own fails a test, the value walk through the check that a rendered default states one value per declared key. Two keys can now collide where two prefixes never could. A key two sections both declare has one default rendered over the other, and which one depends on the order the sections are walked. And a root key that is also a section's name cannot be written at all, because a file holding both a value for that name and a table under it is not valid TOML, so one of the two is unreachable and nothing says which. Both are refused, in either registration order. A section can now say that an environment variable cannot supply one of its keys. The metric label set is a list of name and value rows and its reader asserts that exact shape rather than casting what it finds, so no single string satisfies it, and the assertion is the first statement of the whole server configuration. A resolved variable would install a value that stops the node; leaving the channel out means the file's value applies and the node runs. The reason is required rather than optional, because an operator whose variable is ignored has to be told why, and a refusal with no reason is itself refused. The metric section is the one here that needs a schema, and for one field's shape rather than for a spelling. Its label set is declared as untyped rows to match what the reader takes. A test holds every other field to the upstream field's name, tag and type, and holds the count of differing types at one, so a second divergence is a failure and a converged upstream type leaves the schema with nothing to justify it. Nothing here varies a default by mode. seid init writes the two interface toggles and the block retention per mode, so a node it provisioned carries those as written values, and these are what a node with nothing written runs. One declared value is not what a running node uses, and it is worth knowing which. The pruning strategy is declared as keeping everything, while the command line registers a flag of the same name defaulting to the standard strategy, and a bound flag is a source of its own below the file. A node started with no pruning key written prunes on the standard schedule. Whoever resolves for a running node has to supply the flag values to get the answer that node uses. The recorded configuration surface does not move, because nothing consumes the registry on a boot path yet.
The rule was right and the reason recorded for it was not, which matters because it is the reason the remaining sections will cite. It said a declared key would be written at override precedence and land on top of the value that code assigned. That cannot happen for either field it named: the app layer assigns the receipt store's retention after the reader has returned, so the assignment is last and wins. It also said such a node would silently keep nothing, and the field's own comment says the opposite, that keeping zero versions means keeping everything. An operator handed that sentence during an incident looks for missing receipts and finds a full disk. The reproducible reason is the one every other refusal here rests on. A key for such a field is one an operator can write that the assignment then discards, so it reaches no field. A field with no tag stays a defect for the same reason read from the other end, because it would declare a key derived from a field name and no operator writes that. The two look alike in a diff and mean opposite things, which is why the package's own contract now states the distinction rather than leaving it in a comment beside one branch. Two of the four sections held their declared keys against a written-out list of the same strings. That is a second statement of the key set, which is what a section exists to remove: a tag and the list move together and the reader keeps asking for the old spelling. Both now hold against the constants their reader passes to Get. The admin server's reader was spelling its two keys inline, so it has constants for them now, and its registration no longer recites the derived keys in prose that drifts the moment a tag moves. One assertion is gone because it could not fail, the exact key set having been compared three lines above it. Two declared values are also stated elsewhere in the binary, and each now says so where it is declared. The wasm query gas limit resolves to ten times what the template writes into a generated file, so a node provisioned by the binary runs the smaller number and a node whose file predates the section runs the declared one; whoever renders declared values into a file has to decide which survives, and that limit bounds the work one smart query can ask of a node serving queries to anyone. The receipt store's database directory resolves to an empty string, and the emptiness carries the meaning: the app layer fills it from the host, keeping the former path for a node that already holds the store there. A path written into a file is one host's answer and names an empty directory on another. One comment said the contract debug switch has no key any reader resolves. It is read from the node-wide trace flag, so its key belongs to the root of the file rather than to that section, which also means the section's three keys do not determine the configuration the module ends up with. A new test asks the whole set at once. Two refusals depend on what else has registered, neither is visible from inside either section, and the section that loses is dropped whole with every key it declared. Registering a section whose key collides with the receipt store's leaves all four section suites green and fails only this one.
Two of the state store's settings mean something different depending on what kind of node asks, and this section answered the same for all four. An archive node exists to keep history, and it was declaring a retention of a hundred thousand versions. The binary already says otherwise, in the mode rules it applies when it writes a file: an archive node keeps everything, and a validator and a seed run with the store off. Those rules are now where this section's answer comes from, so a change to them moves this too. That is the correction the pinned defect asks for. The record of it says to pin how configuration resolves today and correct it in the versioned manager, and this registry is the versioned manager, so declaring the rendered value would have pinned the defect a second time in the place meant to fix it. Nothing runs differently yet, because nothing consumes the registry on a boot path, and the direction matters more than the timing: state store pruning deletes inside the store on a timer, the archive volumes are protected against being deleted rather than against being emptied, and pruning frees disk so nothing that watches disk would fire. The rest of this change replaces prose with measurement. The declared values for the two storage sections are not what their readers produce for a file missing the keys, and the comment describing which keys those are was wrong three ways: it named four of the six store settings, missed two, named a commitment setting that does not in fact differ, and missed the one that selects how a node commits. The write mode is read through a presence check and then rewritten unconditionally, so a node with nothing written commits in the derived mode rather than the one that key carries. A comment cannot fail when it is wrong. So the set is measured against the readers now, per mode, and recorded as data: a key that starts diverging fails, and so does one that stops, which means guarding a read has to account for its row rather than quietly making a sentence stale. Each key set is also held against this package's own read-site record, which is kept for another purpose and held against a golden file, rather than against a list written beside it in the same commit. The record spells its keys with the reader's constants and the section derives them from tags, so a rename on either side alone fails. One comment said the other keys under the commitment section's flat key-value name have no reader. Four of them are read by the Cosmos server's own reader, so they belong to whoever registers that section, and saying they reach nothing would have closed the door on declaring them. The whole-registry defect sweep here is now scoped to the four sections this file registers. A refusal that depends on what else has registered is not this package's to answer for, and the sweep that covers it lives where every section is linked.
Four comments said more than their subject. The light-invariance default now states the mode decision and leaves what the check compares to the check. The genesis schema says why it exists and leaves what holds it to the test that holds it. The helper that resolves says once that it renders every section, so a failure naming another one is read correctly. And the write-mode default is asked for every mode rather than one, which is how the tests beside it ask.
The failure messages for an excluded field still claimed the mechanism the comment above them no longer does: that a declared key would land over whatever assigns the field. They now say what is true, that such a key is one an operator can write and the assignment discards. The half of that test covering an untagged field asserted only that some refusal was recorded, so it passed on a refusal raised for any other reason. It now requires the message to name the field, and changing the name it looks for fails it. The registry's contract adds the one thing its new rule leaves unsaid: the exclusion tag is meaningful only on an exported field, because an unexported one carrying any tag is refused before the tag is read. The giga executor's default says why this section does not vary by mode without stating it as a rule for every section, since another section in this work does vary and the binary is what decides which. The fixture in the registry's own spec no longer describes that package as one that would register, because it now does. The wasm section's defaults function takes a name that does not collide with a local of the same name elsewhere in that package, and its test asks every mode rather than one.
Three of these settings mean something different depending on what kind of node asks, and all five sections answered the same for every one. The binary already states the rules, in what it applies when it writes a file, and every section here now answers through them. Each of the three matters in a different direction. A full node and an archive node exist to serve queries, and both interfaces that serve them were declared closed. A validator is meant to expose as little as it can, and gRPC was declared open on every one of them, which is the opposite of what the rule beside it says it is for. And the number of blocks a node retains was declared as keeping everything for a full node, where the rule prunes at a hundred thousand. The rules are read rather than restated, so one added later moves these sections with nothing here changing, and the test writes the three values out by kind of node so a change to the rules fails and gets looked at. Resolving every mode as a validator, opening gRPC on a validator, and changing the retention each fail it. The two sections no rule touches answer through the same function, so there is one place a mode is applied rather than a decision per section about whether to apply it.
…aces per mode A resolved list was the section's own list. A section's default is usually a package-level variable, so a slice field handed out the array that variable holds, and one in-place write by a caller rewrote it for the whole process: every later resolution, and every reader that copies the same struct. Two of the five lists this reaches are deny lists, so the rewrite is silent and it is a security control. The registry already copies a section's keys for exactly this reason and said so in a comment; values now get the same guarantee, at the one function both walks pass through. Removing it fails a test that writes into a resolved list and asks the section's default what it holds. The EVM interfaces answer per kind of node. A full node and an archive node serve queries, which is what those interfaces are for. A validator and a seed serve none, and this section declared both open for every kind, which puts a public request surface on the node that holds a signing key. The rule already exists in this binary. It could not be read from here, because the package that owns the node mode imports this one, so the rule moved to the registry, which is a leaf both sides reach, and the node-mode type now delegates to it rather than stating it twice. Forgetting archive in that one statement now fails a test. Each section also holds its keys to the values their fields hold, not to its own defaults struct compared with itself. That comparison agreed with itself while two tags sat on the wrong fields: the key set stays identical and every field still holds the value it always did, so an endpoint and a directory, or a deny list and an origin list, change places unnoticed. Both of those swaps now fail. Each section reports its own refusal. A registration the registry cannot use is recorded rather than raised, and these tests inferred it from a lookup coming back empty, which threw away the sentence saying why. Two tests are gone because they restated checks that already exist a few files away, where the message is better. Two comments are corrected: enabling replay opens a client without reaching the endpoint, so an unreachable one surfaces during replay rather than at startup, and the two machine-derived values are not one case, because the worker pool re-measures when it is given a value that is not positive and the simulation limit reads zero as no limit at all. The registry now states what a resolved value's type depends on, because it resolves values and does not convert them: a default arrives as its field's type, a file as whatever the format decodes to, and an environment variable as one string.
…d nothing A resolution answered for any string. A section's defaults answer per mode, and a mode this package does not know reached whatever each section does with an argument it cannot match, which for these five is the rules answering as though it were a full node. So an empty string, a capitalised name, or one with a trailing space resolved the interfaces a full node serves onto whichever node asked, with no error. It is refused now, naming the four. A refusal of the environment channel is recorded by a key, so a slip in the spelling named a key no section declares. The channel would never have offered that key, so the refusal covered nothing while reading as though it did, and the key it was written for went on resolving from a variable. Both sets exist for the first time when something resolves, because a refusal may be recorded before the section declaring its key registers, so that is where they are compared. The reason a refusal carries is required and had no consumer. The channel was skipped before the variable was read, so the one fact a diagnostic needs, that an operator set it, was discarded at the cheapest possible point. The variable is read now and its value still thrown away, and the key comes back named, so a required reason is one somebody can be told. A refused key nobody set is not reported, because a value nobody chose is not news. A refusal also names the section that declares the key, so a refused key is attributable the way every other defect is. It was putting the key where the section belongs, which made a defect read as though a key had registered and made a scoped sweep skip it. Four of the metric section's seven hand-copied values were held against nothing. That is the one section here that has to restate its values, so it is the one where a field can be assigned from its neighbour, and assigning the hostname toggle from the enabled toggle survived the suite. Every one of the seven is now held as the key it resolves to rather than as a struct field, because a struct compared with itself agrees while two values sit on the wrong fields. Five comments said things the code does not. The node-wide settings claimed to be unchanged by mode while one of their own keys answers per mode. A count of non-zero defaults was wrong. Two different counts of six read as one, and the pair the sentence lost is read through a clamp that does nothing for an absent key. The package's reason for existing named a vendored tree, when other sections register inside one and the real obstacle is an import edge. And a paragraph named two sections that belong to another change.
Four statements in the package contract described the previous shape. A second entry point exists, for the settings written at the top of a file rather than inside a table, and the contract showed one. The list of what makes a registration unusable no longer enumerated: two sections declaring one key and a top-level key sharing a section's name both became possible once a key could sit at the root, and a refusal of the environment carrying no reason is refused too. The resolution order had gained a per-key hole in one channel and did not say so. And the first step of adding a section told an author to use the name as the first segment of every key, which is false for a section whose keys have none. A mode this package does not declare is also refused now, and the contract says that where it says a default answers per mode.
refuseOverlap refused two collisions and only one of them could happen. Two sections declaring one key is already refused by the environment check, which two identical keys reach by answering to one variable, so that arm was a second guard on a case already covered. The other arm, a key at the top of the file sharing a section's name, was the only one it alone caught, and it has no instance: one section declares keys at the top of the file and none of its fourteen names is a section's. So the code goes and the fact stays. The contract names the collision among the things this package does not guard, with what makes it reachable, because a second such section is where it starts to matter. The prototype found that out by hand: it named the section holding config.toml's top-level keys after the file rather than after the node, because the client file declares a top-level key called node and a node section could not have coexisted with it. The one case the removed guard described better is named better now where it is still refused. Two identical keys were being reported as two spellings of one environment variable, and the reason a dot and a hyphen are the same character to the environment is not the reason a key collides with itself.
…sures where a node differs These sections said their values were what a node with nothing written resolves. They are not, and the difference was carried in four paragraphs of prose with one of the counts wrong. What they are is what seid init writes for a kind of node: the upstream defaults with the binary's own mode rules applied is exactly the pipeline that renders a generated app.toml, so a declared value is what that file would have held. That is a claim about a real pipeline in this binary rather than a judgement, so it can be held, and it is what a caller writing a configuration file wants. Where a node with nothing written resolves something else is now measured. The reader is driven with the start command's flags bound, the way a booting node binds them, because seventeen of these keys are also flags and a flag's registration default is what an absent key reaches before the lookup comes back empty. Twelve keys differ, and the measurement corrected the prose twice over: seven keys the paragraphs implied differ do not once the flags are bound, and the gRPC toggle does, which no paragraph named. Of the two interface toggles it is the only one that diverges, because its flag defaults the interface on while a generated validator file writes it off. A key that starts diverging fails, and so does one that stops, so guarding a read has to account for its row. Dropping the flag binding fails it too, which is what keeps the record measuring what a node gets rather than what the reader says in isolation.
This binary writes an app.toml two ways and they disagree on four keys. The provisioning command applies the mode rules and renders the result; a node starting without a file runs a second pipeline that applies no mode rules at all and carries overrides of its own. So it writes the standard pruning strategy where the command writes keeping everything, a metric retention of sixty against seven thousand two hundred, the REST interface on for a validator against off, and a pruning interval drawn at random on every run. A declared value follows the command an operator runs to provision a node. That was already true and the comment said only that seid writes it, which is ninety per cent of a fact.
A declared value is what the seid init command writes for a kind of node. This section departs from that once: the retention an archive node keeps. The mode rules set it to keep everything and the command does not write that, because the type it renders declares a state store field of its own and fills it from the mode-blind default, so the rule is applied and then discarded. PLT-955 records that and records the decision, which is to pin what a node resolves today and correct it in the versioned declaration rather than at the point that loses it. So the departure is intended, and it is now held rather than asserted. It fails if the command starts carrying the rule, which is the day the departure should be deleted. It fails if this section stops departing, which would declare a retention on the one kind of node whose purpose is keeping what it would prune. Both directions are checked, because a departure nothing measures cannot be told from an oversight.
These four said their values are what a node with nothing written resolves. They are what the seid init command writes for a kind of node, which is a different statement and a checkable one: that command applies the same mode rule to this section's own defaults and renders the result, and for the EVM section it passes what it applied through rather than refilling it from a mode-blind copy, so a declared value here is the value that reaches the file.
…de runs The section declared this module's own default and the command writes a tenth of it, so the two disagreed by a factor of ten on the only bound on the work one smart query can ask of a node that serves queries to anyone. Declaring the larger one meant a caller rendering a file from these values would loosen that bound, and a test named for the module's default invited exactly that reading. The number now lives once, beside the section, and the command reads it. So a declared value is what reaches an operator's file, and the generated file is unchanged, which the command's own characterization suite confirms. Raising it to the module's default fails a test rather than silently widening a public surface. The module's default is still what a node whose file carries no wasm section resolves. That is a different question, and it is held as one. Three other sections say what their declared values are rather than what a node with nothing written resolves.
# Conflicts: # config/registry/doc.go
tagOf carried its rationale inline, which meant the steps were never named. Each condition that needed explaining is now a predicate whose name says why it is there, and the rationale moved to that predicate's doc comment. The tag excluding a field from configuration and the spelling no written key can match are the two that carried comments. Splitting the tag into a name and the squash option is a step of its own, so the body reads as a sequence rather than parsing in place. Behaviour is unchanged and no test moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ions The node's own configuration file carries 141 keys and no section declared any of them. These two are the first, and the registry needed two things before they could be. A section can now leave out a path the struct carries. Two kinds of field earn it. One a reader refuses outright, where writing the key stops the node, so declaring it would put a setting in the space whose only effect is an outage. And one whose absence is itself the setting, where a default would be this package inventing one. An exclusion naming no field the struct carries is refused, because the field it named can be renamed away and leave the exclusion reading as a deliberate omission while excluding nothing. A field that collects what the decode matched no field for now declares no key. What lands in it is what an operator misspelled, so giving it a key would offer the collector itself as a setting to write. Neither the package defining these settings nor the package deciding them can register them: the struct belongs to the node's configuration package and the rules that vary it by node kind live in the parameters package, which imports that struct. So a third package does it, the same shape the upstream server sections already use. Four keys vary by node kind and a test holds all four by value. Two are listen addresses, so a rule that stopped varying would leave a validator binding the address a query-serving node binds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neither varies by node kind. How long a node waits at each step of a round has to agree across the validator set for the set to reach a decision, and what a node holds before a transaction is decided is a limit on its own memory. The consensus struct carries fifteen fields the node removed as settings, and it marks each one deprecated. They are excluded: declaring one would offer a key that changes nothing about how the node runs. So the section declares nine of its twenty-four paths. The reader has a check that names the removed settings an operator wrote, and it reaches eight of the fifteen. Six are durations or booleans, where a written zero and an unwritten field hold the same value, so no check can tell them apart. One more it omits. Nothing calls the check in any case. A test holds which eight it reaches, so making it complete fails rather than leaving the count stale, and a second test holds every exclusion to the struct's own deprecated marking rather than to that check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Five of these sections carry a root directory field tagged the same as the key at the top of the file, and the node fills every one of them from the command line after the file is read. So the file never carries the value and each section states the empty string for it. Declaring it hands whatever delivers these values an empty root to write over a running node's, and a node that cannot find its data directory, its genesis file or its signing key does not start. Two of the five are here. The test checks every registered section rather than the five, so a section added later that carries the same field fails rather than shipping the same hole. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3993 +/- ##
==========================================
- Coverage 61.22% 60.31% -0.92%
==========================================
Files 2153 2065 -88
Lines 188393 176947 -11446
==========================================
- Hits 115351 106717 -8634
+ Misses 62298 60469 -1829
+ Partials 10744 9761 -983
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
# Conflicts: # config/tendermintbase/tendermintbase.go # config/tendermintbase/tendermintbase_test.go
Contributor
Author
|
@seidroid review |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The consensus and mempool registrations are derived cleanly from the reader's own structs, and the four findings from the last review are all addressed — the mempool doc no longer overclaims what the test drives, the seven/eight drift is gone from the test file, the two reasons behind the warning gap are separated, and the second section list is collapsed into declaredAgainst. Two things remain: one new off-by-one in the rewritten paragraph, and the mempool exclusion criterion is narrower than the one this package already measures for p2p, in a way that reaches three keys every generated config.toml carries today.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The godocs this PR adds record design history, which
AGENTS.md("Godoc") rules out: "Never record design history" and "Explain WHAT, not WHY or HOW". Concretely —probeValueFor("A shape the decoder refuses used to end the loop through a skip, so every row after it went unmeasured"),parseDeprecatedFields("Without these the same input gave four different answers across forty calls"),deprecatedFields("A predicate that saw only the names reported those seven as live settings"),removedSettings("which is why it went on being declared as a settable key"), andTestNoDeclaredKeyNamesADeprecatedField("The consensus section had this check from the start against a hand-kept list"). Each narrates what a previous revision did wrong; the diff and git log hold that. The surrounding package is already written in this register, so this is a direction for the series rather than a rewrite demand — but the new prose is where the rule is being extended, and prose describing a defect already fixed is the part that goes stale first. Codex flags the same thing. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
(*ConsensusConfig).DeprecatedFieldWarning(sei-tendermint/config/config.go:1345) testsDeprecatedSkipTimeoutCommittwice (lines 1347 and 1371), so a file settingskip-timeout-commitnames it twice in the error, and it never testsDeprecatedUnsafeBypassCommitTimeoutOverrideeven though that field is a*boolit could detect exactly as it detects the other eight pointers. Combined with having no caller anywhere in the binary, an operator with a removed timeout key gets nothing. Codex flags the same thing.
Leaving those three out is decided by the reader and a generated file writes all three, which is where this section departs from the other two that leave paths out. The dial hook and the sixteen removed consensus settings are each unread and unwritten, so for them the two facts agree and either would serve as the rule. Here they disagree, and the doc resolved it by quietly picking one. Stated now: a key nothing reads is not a setting, whoever writes it into a file, and what that costs is an operator who has one of these lines being told it reaches nothing. That is the true answer, and it is also why the peer seeds go the other way on the same shape. They are read, so refusing them would take a live setting out of the space. One count corrected. The reader's check tests eight pointer fields, not seven; the seven are the timeout paths alone and dropping the eighth is what makes the comparison the sentence draws fail to check out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t-775-node-sections-2
Contributor
Author
|
@seidroid review |
There was a problem hiding this comment.
The consensus and mempool registrations are correct and well measured — counts reconcile against the structs (24→7, 22→18), the exclusion lists tie to the source-parsed deprecation marking, and both findings from the last review are addressed. What remains is one entry in the recorded reason for the warning gap that is inverted for the field it matters most for, plus the godoc design-history pattern that is still unchanged.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The godocs this PR adds still record design history, which
AGENTS.md("Godoc") rules out — unchanged since the last review, and Codex flags the same thing. Concretely:probeValueFor("A shape the decoder refuses used to end the loop through a skip, so every row after it went unmeasured"),parseDeprecatedFields("Without these the same input gave four different answers across forty calls"),deprecatedFields("A predicate that saw only the names reported those seven as live settings"),removedSettings("which is why it went on being declared as a settable key"),TestNoDeclaredKeyNamesADeprecatedField("The consensus section had this check from the start against a hand-kept list"),deprecatedPaths' inline note ("Sharing the map left a later caller reading an emptied set") andisDeprecationNote("a field whose documentation merely points at another field's deprecation came back marked"). Each narrates what a previous revision of this same PR did wrong; the diff and git log hold that. The invariant in each case survives the rewrite — "only a declaration at the top level is read, so a struct declared inside a function cannot answer" says everything the forty-calls sentence does, without going stale the moment the defect is forgotten. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
(*ConsensusConfig).DeprecatedFieldWarning(sei-tendermint/config/config.go:1345) testsDeprecatedSkipTimeoutCommittwice (lines 1347 and 1371), so a file settingskip-timeout-commitnames it twice in the error, and it never testsDeprecatedUnsafeBypassCommitTimeoutOverrideeven though that field is a*boolit could detect exactly as it detects the other eight pointers. Combined with having no caller anywhere in the binary, an operator with a removed timeout key gets nothing. Codex flags the same thing.
…he zero The record of what the reader's deprecation check cannot name said a written zero cannot be told from an unwritten field. That is backwards for one of the eight, and it is the one an operator is most likely to write: the stateless leader election setting defaults to true, so an unwritten field decodes to true, a written true is what cannot be told apart, and a written false is detectable. False is exactly what an operator writes after reading a generated file and trying to turn the behaviour off. Measured across the eight: six default to the zero, one to true, and one is a pointer where any written value is distinguishable. The rule that covers all three is the field's own default, so that is what both copies now say. The point of stating the split is that someone closing the gap knows which keys need non-zero-write detection and which need four lines in a check that already tests eight pointers. As written it told them to skip the field whose detectable case is the dangerous one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…wrong Almost every comment this branch adds narrated what an earlier revision of it did wrong: a loop that used to end in a skip, a parse that gave four answers across forty calls, a predicate that reported seven live settings as dead. The repository's own guide rules that out, and the git log holds it. Rewritten from each declaration rather than edited. Where a sentence was protecting a property a future editor could break, the property is stated in the present tense and the story around it is gone: that only a top-level declaration is read, so a struct inside a function cannot answer; that the deprecation note has to open a line, because a comment mentioning another field's deprecation is not a marking; that the returned marking map belongs to the caller, because the parse behind it is shared. Rationale that was doing no work in a godoc moved to the line that needs it or went. Two docs shed a policy argument that is not a property of the variable it sat on. One kept its second paragraph, because a coverage limit is the kind of second paragraph that earns itself. Two accuracy fixes fell out of the rewrite. The consensus list said its fields are kept so a decode can tell that an operator set one, which is true of the nine held as pointers and false of the six held as values. The interface note said the removed settings are each a pointer to an empty interface, which is true of eight of sixteen. One guard removed as dead. Filtering parsed files by a _test package name cannot fire: Go requires an external test package to live in a _test.go file, and the filename filter above it already excludes those. The invariant it claimed to keep is real and is kept by the filename. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t-775-node-sections-2
Contributor
Author
|
The godoc finding is addressed. Every comment this branch adds is rewritten from its declaration rather than edited, with the design history removed and the load-bearing property restated in the present tense. Three things fell out of the rewrite that were not wording problems:
Also fixed the inverted reason on the warning gap, on its own commit: the undetectable write is the one equal to the field's own default, not the zero, which puts Two notes on scope. The same pattern runs through 3994 and 3995 and I will apply it there next. |
Contributor
Author
|
@seidroid review |
There was a problem hiding this comment.
The consensus and mempool registrations are correct and the counts reconcile against the structs (24→7, 22→18); the exclusion lists tie to a source-parsed deprecation marking, the mode-invariance claim is now measured through the registry, and the previous review's finding is addressed. One residual: the marking walk does not follow the same mapstructure tag rules the key walk does.
Findings: 0 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
(*ConsensusConfig).DeprecatedFieldWarning(sei-tendermint/config/config.go:1345) testsDeprecatedSkipTimeoutCommittwice (lines 1347 and 1371), so a file settingskip-timeout-commitnames it twice in the error, and it never testsDeprecatedUnsafeBypassCommitTimeoutOverrideeven though that field is a*boolit could detect exactly as it detects the other eight pointers. Combined with having no caller anywhere in the binary, an operator with a removed timeout key gets nothing. Codex flags the same thing.
…y walk The walk that finds deprecated fields took the first tag segment unconditionally, where the walk that derives keys drops a field naming no key and one collecting what the decode matched nothing for. So the two read the same tags by different rules. The reachable case is the ordinary next step for a dead field: deprecate a consensus setting and take it out of the reader by retagging it to name no key. The marking set then gains a row for that name, and the check reports that the section declares a key which does not exist. Adding it to the removed list makes it worse, since an exclusion matching no derived path is refused and the whole section leaves the registry. Reproduced before fixing and confirmed after: the retagged field now passes, a live declared key marked deprecated is still caught, and reverting the alignment brings the false failure back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@seidroid review |
There was a problem hiding this comment.
The consensus and mempool registrations are correct and well measured — the counts reconcile against the structs (24→7, 22→18), the exclusion lists tie to a source-parsed deprecation marking, the mode-invariance walk reads the registry, and every finding from the previous review is addressed. One residual: the reason consensusDefaults gives for answering the same for every mode describes keys this section excludes rather than the ones it declares.
Findings: 0 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
(*ConsensusConfig).DeprecatedFieldWarning(sei-tendermint/config/config.go:1345) testsDeprecatedSkipTimeoutCommittwice (lines 1347 and 1371), so a file settingskip-timeout-commitnames it twice in the error, and it never testsDeprecatedUnsafeBypassCommitTimeoutOverrideeven though that field is a*boolit could detect exactly as it detects the other eight pointers. Combined with having no caller anywhere in the binary, an operator with a removed timeout key gets nothing. Codex flags the same thing.
Contributor
Author
|
@seidroid review |
There was a problem hiding this comment.
The consensus and mempool registrations are correct and well measured: the key counts derive from the structs (24→7, 22→18), the exclusion lists are tied to a source-parsed deprecation marking rather than a name prefix, the mode-invariance walk now reads the registry, and ToMempoolConfig is held by a test. Two prose claims remain unmeasured or wrong — the consensus mode-invariance rationale describes keys the section excludes, and the recorded reason for the warning gap does not explain six of its eight rows.
Findings: 0 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
(*ConsensusConfig).DeprecatedFieldWarning(sei-tendermint/config/config.go:1345) testsDeprecatedSkipTimeoutCommittwice (lines 1347 and 1371), so a file settingskip-timeout-commitnames it twice in the error, and it never testsDeprecatedUnsafeBypassCommitTimeoutOverrideeven though that field is a*boolit could detect exactly as it detects the other eight pointers. Combined with having no caller anywhere in the binary —grepfinds only this PR's new test — an operator with a removed timeout key gets nothing. Codex flags the same thing.
The consensus defaults said the same values hold for every mode because the timings have to agree across the validator set. That is true of the paths this section leaves out, which moved to the consensus parameters, and not of the seven it declares: a log path, a proposer's own empty-block policy, a gossip mode, two reactor sleeps and a local restart check. Nodes running different values for those still reach the same decision. The reason is the one the mempool defaults already give, that nothing in the binary derives one from a node kind, which is also what the mode-variance check measures. The record of what the reader's deprecation check cannot name gave one reason for eight rows, and measured against the value the test writes it fits one of them. Seven of the eight probes differ from the field's default, so the ambiguity does not arise for them; they are silent because the check never tests those fields, and it would fire on exactly these writes. Only the leader election setting has a probe equal to its default. Stated as the three reasons it is, because the split is what tells someone closing the gap which rows are cheap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
yzang2019
reviewed
Aug 27, 2026
Comment on lines
+38
to
+46
| // neverReachTheMempool are the mempool paths this section does not declare. No code reads them, so | ||
| // declaring one would offer a setting that changes nothing, even though a generated file writes all | ||
| // three. The similarly spelled ttl-duration and ttl-num-blocks are live and stay declared. | ||
| var neverReachTheMempool = []string{ | ||
| "max-batch-bytes", | ||
| "pending-ttl-duration", | ||
| "pending-ttl-num-blocks", | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
If no code reaches them, shall we just delete them?
Contributor
Author
There was a problem hiding this comment.
Probably yes, eventually, and deliberately not in this PR.
Two reasons. The fields live in sei-tendermint, which is the same Go module, so deleting one is an upstream change to a struct an operator's existing config.toml decodes into. And this PR declares keys without changing anything about what a node runs, which is the property that has let it absorb a lot of review without any risk to a running node. A field deletion would end that.
There is also a live use for keeping them: a decode can tell that an operator set one, which is what the node's deprecation warning is for. That warning is itself incomplete and nothing in the binary calls it, so the value of keeping the fields is smaller than it looks. Worth its own change with that cleaned up at the same time.
|
|
||
| // removedSettings are the consensus paths this section does not declare. Each names a field the node's | ||
| // struct marks deprecated, so the key would offer a setting that a written value cannot change. | ||
| var removedSettings = []string{ |
Contributor
There was a problem hiding this comment.
These settings could actually impact block time and consensus time, if we deprecate them, what default value are we gonna replace them with?
Contributor
Author
There was a problem hiding this comment.
Nothing is being deprecated here. The node already did that, and it moved these settings to consensus params rather than dropping them.
ResolveTimeouts is the whole of it:
func (*ConsensusConfig) ResolveTimeouts(t types.TimeoutParams) types.TimeoutParams {
return t.Or(types.DefaultTimeoutParams())
}
The receiver is unnamed, so the config fields feed nothing. state.go:2792 calls it with cs.state.ConsensusParams.Timeout, and where the chain states nothing it falls back to DefaultTimeoutParams(): propose 1s, propose delta 500ms, vote 50ms, vote delta 500ms, commit 50ms, bypass commit false.
So the answer to what replaces them is the chain's consensus params, which is the right home for a value that has to agree across the validator set. A per node config.toml could not be, since two validators with different values would disagree about how long a round step lasts.
What this PR does is not declare them in the new key space, so an operator writing one is told it reaches nothing instead of believing it changed block time. Today they get no error and no warning and the value is quietly ignored.
amir-deris
reviewed
Aug 27, 2026
amir-deris
approved these changes
Aug 27, 2026
declaredHere did not say where "here" was: the package, the file or the repository. Named for what it returns instead, which is the keys of the sections this package registers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Declares the consensus and mempool sections: 25 keys. Nothing consumes them.
What is left out, and why
The consensus table carries 24 paths and 7 are declared. Sixteen name settings the node
removed and one is the root directory. The fields are kept so a decode can tell that an
operator set one, and declaring any would offer a key that changes nothing about how the node
runs.
The mempool table declares 18 of 22. Three are left out because no code reads them. A
generated file writes all three, which is where this section departs from the other two that
leave paths out: for the dial hook and the removed consensus settings the two facts agree, and
here they disagree. A key nothing reads is not a setting whoever writes it into a file, and
what that costs is an operator with one of those lines being told it reaches nothing. That is
also what separates them from the peer seeds, which are read and stay declared.
A deprecated field is found by its marking, not by its name
The exclusion list was held against a predicate that read the prefix on a field's name. The
node marks a deprecated field two ways: most carry that prefix, and seven more across four
structs carry the standard comment above the field instead. Reflection cannot see a comment, so
those seven read as live settings.
The predicate now reads the source, which is what a deprecation marking is. One list per
section still serves both directions of the check, and it is applied to every section rather
than to consensus alone.
That found the leader election setting, documented as retained for parsing compatibility and
ignored. Its declared value was the affirmative one, so an operator reading a generated file
would find the behaviour named and switchable and neither is true.
The marking walk also read a tag by different rules than the key walk: it took the first
segment unconditionally, where the key walk drops a field naming no key and one collecting what
the decode matched nothing for. Retagging a dead field to name no key — the ordinary next step
— then produced a failure naming a key that does not exist, and adding that phantom to the
removed list would have taken the whole section out of the registry. Both walks now read a tag
the same way.
A probe that reported a pass while measuring nothing
The rows checking which removed settings the reader's own warning names composed their probe
value by matching the key's name. A name says nothing about a shape: the double-sign height is
an integer whose name begins like the boolean overrides do, and it decoded only because the
decoder accepts a boolean where an integer belongs. A refused shape was reported as a skip,
which ends the whole loop rather than one row, so every remaining row went unmeasured while the
run read as a pass. The value now comes from the field's own type and a refusal is fatal.
Two recorded reasons corrected
The consensus defaults said the same values hold for every mode because the timings have to
agree across the validator set. That is true of the paths this section leaves out, not of the
seven it declares: a log path, a proposer's own empty-block policy, a gossip mode, two reactor
sleeps and a local restart check. The reason is the one the mempool defaults give, that nothing
in the binary derives one from a node kind.
The record of what the reader's deprecation check cannot name gave one reason for eight rows.
Measured against the value the test writes, seven of the eight probes differ from the field's
default, so the ambiguity does not arise for them; they are silent because the check never
tests those fields. Stated as the three reasons it is, since the split is what tells someone
closing the gap which rows are cheap.
Notes for review
The godocs this branch adds were rewritten from their declarations rather than edited. Several
recorded what an earlier revision of this pull request got wrong, which the repository's guide
rules out; where a sentence was protecting a property a future editor could break, the property
is stated in the present tense and the story around it is gone.
Formatters,
go vetandgolangci-lintclean; race detector clean. Mutation-verified:feeding an integer field a list raises a failure where it previously raised a skip, and
retagging a dead field to name no key passes where it previously produced a phantom failure.