[ConfigManager] Register Sections 4/4 - #3977
Conversation
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 latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3977 +/- ##
==========================================
- Coverage 59.08% 57.60% -1.49%
==========================================
Files 2305 2230 -75
Lines 197017 187543 -9474
==========================================
- Hits 116414 108038 -8376
+ Misses 69842 69656 -186
+ Partials 10761 9849 -912
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
…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.
PR SummaryLow Risk Overview Extends Tests lock declared keys to server readers, mode-specific API/gRPC/retention defaults, env refusal behavior, and a measured set of keys where Reviewed by Cursor Bugbot for commit 299d541. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Registers the five Cosmos-server sections and adds root-key registration plus a per-key environment refusal; the key derivation, value walk, and mode gate all line up and the tests pin the behaviour closely. No blockers — three non-blocking points about a documented gap that is partly already covered downstream, an argument whose documented purpose is dropped, and diagnostics that lose their section for root sections.
Findings: 0 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 3 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] sei-cosmos/server/config/config.go:445 —
GetConfigdoeslabelsRaw[0].(string), labelsRaw[1].(string)on atelemetry.global-labelsrow after only checking the row is a 2-element[]interface{}, so a file writing e.g.global-labels = [[1, 2]]panics at startup instead of returning the "failed to parse global label" error the neighbouring checks produce. Noticed while verifying the telemetry schema rationale in this PR; pre-existing in the vendored tree.
| // - Not a file format. Nothing here reads or writes a configuration file. | ||
| // - Not a validator. A section may state rules about its own values; this package invents none. | ||
| // - Not wired. No section is registered by this package and no reader is migrated onto it. | ||
| // - Not a guard against a key and a table sharing one name. A key at the top of the file that is also |
There was a problem hiding this comment.
[suggestion] The gap is real at registration time, but "nothing says which" overstates it. appopts.refuseUnwritable (config/appopts/appopts.go:149-157) already refuses exactly this pair: for a declared telemetry.enabled, dottedPrefixes yields telemetry, and a declared root key of that name hits note(under, key, true), so Install fails naming both keys as "both declared". So the collision does not silently make a setting unreachable — it surfaces later, as an Install error on a booting node rather than as a Defect a package's own registration test would catch.
Two things worth doing: correct this bullet to say where the collision does surface (otherwise the next reader concludes the failure mode is silent), and consider whether the check belongs in record alongside the other structural refusals, so the failure lands at the same door as the duplicate-key and env-spelling refusals rather than one layer down. Codex raised the same collision independently.
There was a problem hiding this comment.
Correct, and I verified it rather than taking it on the reference: refuseUnwritable builds %q and %q, both declared for exactly this pair, so Install fails naming both keys.
The bullet now says the failure surfaces there, and what that costs: it is late, arriving on a booting node rather than in the registration test of the package that caused it.
On moving the check into record — I have not done that, and it is a decision rather than an omission. The guard existed earlier in this work and was deliberately removed, with the collision recorded as a tracked gap on the grounds that it has no instance today: one section declares root keys and none of its names is a section name. record could certainly do it, and it is where the sibling structural refusals live, so I agree that is the right home if we decide to guard rather than track. Flagging it to the DRI rather than reversing that call in a review reply.
| // start, so the difference is recorded rather than assumed. A value silently doing nothing is the failure | ||
| // this whole surface exists to remove, which is why the reason is required and not optional. | ||
| // | ||
| // section is the section that declares the key, so a refused key is attributable to a registration the |
There was a problem hiding this comment.
[suggestion] "section is the section that declares the key, so a refused key is attributable to a registration the way every other defect is" — but section is only read in the reason == "" branch. On the recording path it is discarded: envCannotDeliver is keyed by key alone and EnvCannotDeliver() returns key→reason, so nothing can attribute a refusal to a section.
That matters in the one place attribution would help: resolve.go:99's "refused from the environment and no section declares it" error names the key but cannot say which registration recorded the refusal, which is the mis-spelled-key case TestARefusalNamingAKeyNothingDeclaresIsRefused covers. Either store the section beside the reason and name it in that error, or drop the parameter and this paragraph.
There was a problem hiding this comment.
Right — it was read only on the empty-reason branch and discarded on the recording path.
Kept now, in a map beside the reason, and named in the one error where attribution is worth anything:
%q is refused from the environment by section %q and no section declares it, so the refusal covers nothing
That is the misspelling case, where what an author needs is which registration to look at. EnvCannotDeliver() keeps its key-to-reason shape so nothing else moved; RefusedBy(key) is the new accessor, and Reset clears it alongside.
|
|
||
| var keys []string | ||
| if err := walk(t, section, &keys, map[reflect.Type]bool{}); err != nil { | ||
| if err := walk(t, prefix, &keys, map[reflect.Type]bool{}); err != nil { |
There was a problem hiding this comment.
[suggestion] prefix is now doing two jobs: it builds the key, and it is the label every diagnostic below formats. For a root section it is "", so a malformed registration reports .HaltHeight has no mapstructure tag ... (registry.go:269, 337, 351, 357, 360, 365) and, at the top level, is struct {...}, which contains itself (registry.go:255) — same for the value walk at resolve.go:258, 287. The section name is right here in deriveKeys but is not threaded through.
Passing the label separately from the key prefix (e.g. walk(t, name, prefix, ...), defaulting the label to the section name when the prefix is empty) keeps these messages self-explanatory. Defect.Section still carries the name, so the impact is bounded — but it is a refusal message in a package whose stated posture is that every refusal explains itself.
There was a problem hiding this comment.
Fixed. The message path is now seeded with the section name and grown by the same tag segments the key prefix is grown by, so a squashed field adds nothing and a nested one adds its own.
Before: .HaltHeight has no mapstructure tag ...
After: node_base.Untagged has no mapstructure tag ...
Threading it was not quite a matter of passing the name through. My first attempt passed the accumulated Go-field path, which made a nested refusal read nest.Mid.Deep.Untagged and a squashed one sq.Base.Untagged — the two existing tests for those paths caught both, since they assert the tag-derived path and that a squashed field adds no segment. The label now grows exactly as the key prefix does.
A refusal built its message from the key prefix, which is empty for a section whose keys sit at the root of the file. Those messages read ".HaltHeight has no mapstructure tag", naming no section at all, in a package whose stated posture is that every refusal explains itself. The message path is now seeded with the section name and grown by the same tag segments the key prefix is grown by, so a squashed field adds nothing and a nested one adds its own. A root section's refusal now reads "node_base.Untagged has no mapstructure tag". The section a refusal from the environment is recorded against was accepted and then discarded, while the comment beside it said a refused key is attributable to a registration. It is kept now, and named in the one error where attribution is worth anything: a refusal whose key no section declares, which is the misspelling case, where what an author needs is which registration to look at. The note about a key sharing a table's name said nothing reports the collision. Something does, one layer down: whatever installs a resolution refuses it, naming both keys, because a source holds one value per path. So the failure is not silent. It is late, arriving on a booting node rather than in the registration test of the package that caused it, and the note now says that instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Registers the five configuration sections whose keys belong to the Cosmos server, and
adds the two registry capabilities they need.
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 nowhereupstream 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 declare keys at the root of the file
The node-wide settings are written at the top of
app.tomland read aspruningandhalt-height, with no segment in front. A section that carried its name into every keywould rename all fourteen, and an operator's existing file would reach none of them. So
a section now has a name it is looked up and reported under, and a prefix its keys
carry, and for a root section the prefix is empty.
Both walks build a key through one function. Reverting either one on its own fails a
test: the type walk through the key comparison, and the value walk through the existing
check that a rendered default states one value per declared key.
One collision is refused, and one is a gap we are tracking
A key two sections both declare has one default rendered over the other, and which one
depends on the order the sections are walked. That is refused — two identical keys answer
to one environment variable, so the check that already refuses a spelling collision
catches it. It now says so in the right terms: a key colliding with itself is not two
spellings of one variable.
The gap. A key at the top of the file that is also a section's name cannot be written
at all, because no file holds both a value for that name and a table under it, so one of
the two settings is unreachable. Nothing here refuses that.
It is not silent, which is a correction to what this said before. Whatever installs a
resolution refuses it: a source holds one value per path, so the install names both keys and
stops rather than choosing which survives. I verified that rather than taking it on
reference. So the failure arrives late, on a booting node, instead of in the registration test
of the package that caused it — and that is the cost of tracking the gap rather than guarding
it, stated where a reader will look for it.
I had a guard for it and removed it, because it has no instance: one section declares keys
at the top of the file and none of its fourteen names is a section's name. It becomes
reachable at the second such section, and the prototype shows that is exactly where it
bites — it named the section holding
config.toml's top-level keys after the file ratherthan after the node, because the client file declares a top-level key
nodeand anodesection could not have coexisted with it. So the guard belongs with the change that adds
the second root section, where the naming decision it forces actually arises.
The fact is recorded in the package contract, among the things the registry does not
guard, with the condition that makes it reachable. It is adjacent to PLT-1039, which
tracks the same shape of problem inside a single section: a leaf and a subtree declared
under one tag.
A refusal names the section that recorded it
Two smaller corrections from review, both in the same posture: this package's stated position
is that every refusal explains itself.
A refusal built its message from the key prefix, which is empty for a section whose keys sit at
the root of the file. Those messages read
.HaltHeight has no mapstructure tag, naming nosection at all. The message path is now seeded with the section name and grown by the same tag
segments the key prefix is grown by, so a squashed field adds nothing and a nested one adds its
own. A root section's refusal reads
node_base.Untagged has no mapstructure tagnow.Threading that was not simply passing the name through. My first attempt used the accumulated
Go field path, which made a nested refusal read
nest.Mid.Deep.Untaggedand a squashed onesq.Base.Untagged; the two existing tests for those paths caught both.And the section a refusal from the environment is recorded against was accepted and then
discarded, while the comment beside it claimed a refused key is attributable to a registration.
It is kept now and named in the one error where attribution is worth anything: a refusal whose
key no section declares, which is the misspelling case, where what an author needs is which
registration to look at.
A section can say the environment cannot supply a key
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. That
assertion is the first statement of the whole server configuration, so a resolved
variable installs 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 one schema, and what holds it
The metric section needs a schema, 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 exactly one. A second divergence fails, and if the upstream type ever comes to
match, the schema is left with nothing to justify it and that fails too.
These five are declared and not yet linked
Nothing imports
config/cosmosbase, so itsinitruns only in its own test binary and these fivesections do not enter a real
seid. That is deliberate for now: adding a blank import somewhere in thecommand tree would put fourteen node-wide keys into the key space a node resolves, which is a behaviour
change and belongs with the work that consumes them rather than with the work that declares them.
Two consequences worth stating rather than leaving to be discovered. A diagnostic run against a real
binary will not see these keys. And the cross-section refusals — the two that depend on what else has
registered — are exercised against probes in the registry's own tests and never against the real
registered set, because that set and these five are never in one binary. Linking them is the first step
of whichever change consumes the registry.
One declared value is not what a running node resolves, and several more are not either
pruningis the one worth naming: the command line registers a flag of the same name defaulting to thestandard schedule, and a bound flag is a source below the file, so a node started with no pruning key
written prunes on that schedule while this declares it keeps everything. It is not the only one — a
command that assembles the server configuration overrides some of these before a node starts, and the
count is measured rather than asserted here.
A caller resolving for a running node therefore has to supply that node's flag values, and only the ones
an operator actually set. A flag nobody typed still reports a default, and this resolution ranks flags
above the file, so passing defaults would put every one of them over an operator's own value.
What a declared value is, and what it is not
A declared value is what
seid initwrites for a kind of node. That is not a judgement: the defaultsthese sections answer through are the upstream defaults with the binary's own mode rules applied, which
is the pipeline that command builds and renders. So the claim is checkable, and a test holds it.
It is deliberately not what a node with nothing written resolves. Twelve keys differ, and the set is
measured rather than described — 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 first. Measuring it corrected an earlier draft twice: seven keys the prose
implied differ do not once the flags are bound, and
grpc.enabledoes, which no paragraph had named. Ofthe two interface toggles it is the only one that diverges.
One more thing worth knowing, and it is pre-existing rather than introduced here. This binary writes an
app.tomltwo ways and they disagree on four keys, because the path a node takes when it starts withouta file applies no mode rules at all and carries overrides of its own: the standard pruning strategy
rather than keeping everything, a metric retention of 60 rather than 7200, the REST interface on for a
validator rather than off, and a pruning interval drawn at random on every run. These sections follow the
provisioning command, which is the one an operator runs deliberately. A node that got its file the other
way already has a file, so a generate path is not its path.
What I broke on purpose
Eight mutations, each caught: porting the key-joining into only one of the two walks
(both directions), rendering a root section's defaults under its lookup name, dropping
the collision refusal, dropping the environment skip, allowing a refusal with no reason,
resolving the label set as the shape its reader refuses, and giving the node-wide keys a
section prefix.
Two things to know
One declared value is not what a running node uses. The pruning strategy is declared
as keeping everything (
DefaultConfig()), while the command line registers a flag ofthe 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. Nothing depends on this yet; I can file it if it should be
tracked.
Three defaults vary by kind of node, and every section here answers through the mode
rules the binary already applies rather than restating them. A full node and an archive node
exist to serve queries, so both interfaces that serve them are on; a validator is meant to
expose as little as it can, so both are off, and the upstream default would have declared
gRPC open on every validator. Block retention follows the same rules. The test writes the
three values out by kind of node, so a change to the rules fails it and gets looked at.
The recorded configuration surface does not move, because nothing consumes the registry
on a boot path yet.
The manifest that states which sections a binary declares is deliberately not here. It
has to name every section, so it can only land once these four registration changes are
all on
main.