[ConfigManager] Register Sections 3/4 - #3976
Conversation
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.
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3976 +/- ##
==========================================
- Coverage 59.08% 57.59% -1.50%
==========================================
Files 2305 2232 -73
Lines 197017 187492 -9525
==========================================
- Hits 116414 107981 -8433
+ Misses 69842 69661 -181
+ Partials 10761 9850 -911
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…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.
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.
PR SummaryMedium Risk Overview Centralizes fullnode vs validator/seed classification in Fixes a process-wide mutation bug in Notes eth_replay’s existing template/reader key mismatch: the registry declares only keys the reader actually resolves. Reviewed by Cursor Bugbot for commit d7a8614. 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.
Registration-only change: four EVM sections declare their reader's own struct so keys are derived from mapstructure tags, plus a detach copy for resolved slice/map values and a shared IsFullnodeMode rule. The derivation, key sets and per-mode values are well covered by the new tests; the remaining notes are about the default/reader divergence for the EVM interface toggles and the scope of the new copy guarantee, neither of which is live yet since nothing calls Resolve outside tests.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] No test links all four sections together and asserts
registry.Defects()is empty.RegisterSectiondeliberately records a refusal instead of panicking, and eachregister_test.goonly inspects defects for its own section — in a binary where the other three sections aren't linked. A cross-section refusal (duplicate section name, or an env-spelling collision caught byenvNamesAreDistinct, which compares against already-registered sections) would silently drop a whole section from the registry with no test failing. A single test in a package that imports all four (e.g. underapp/, which already links them transitively) assertingDefects()is empty and that every expected section name is present would close it. - [suggestion]
evmrpc/config/register.go— thedefaultsdoc says the worker pool "re-measures whenever the value it is given is not positive, so a file carrying zero lets every node size itself, and a caller rendering into a file should write that rather than this", butdefaultsreturnsDefaultConfig.WorkerPoolSize=min(64, NumCPU*2), i.e. the resolving host's measurement. Nothing in this PR supplies the zero the comment says a renderer should write, so a future caller rendering the resolved section intoapp.tomlbakes in the generating host's CPU count (which is what the current template already does). Either state that the renderer is responsible for substituting zero for this one key, or drop the instruction — as written the doc and the code disagree about what reaches the file. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| func defaults(mode registry.Mode) any { | ||
| cfg := DefaultConfig | ||
| serves := registry.IsFullnodeMode(mode) | ||
| cfg.HTTPEnabled = serves |
There was a problem hiding this comment.
[suggestion] This makes the registry's default for evm.http_enabled / evm.ws_enabled mode-varying (false on validator/seed), which matches what SetEVMConfigByMode writes at seid init — but the reader's own fallback is unconditional: DefaultConfig has both true, and ReadConfig keeps that value for every mode when the key is absent from app.toml.
That divergence is invisible today because nothing outside tests calls Resolve (appopts.Install has no callers). But Resolve's contract is that an absent key carries its mode's default, and Install writes at override precedence — so the moment the registry is wired into boot, a validator or seed whose app.toml omits these keys flips from EVM HTTP/WS enabled to disabled. That's a silent behaviour change on upgrade for hand-written or pre-mode-aware config files.
Worth recording the divergence now rather than at wiring time: either a note here that the registry default intentionally differs from ReadConfig's fallback for these two keys, or a characterization row (per testutil/configtest/AGENTS.md) so the flip lands in a diff instead of arriving with the wiring PR.
There was a problem hiding this comment.
The divergence is real and I have recorded it (d7a8614). The consequence is not reachable, and I had that checked independently rather than asserting it — including three ways my own reasoning was too strong.
Why the flip cannot happen. The consumer built since installs only keys a source actually supplied, never a declared default. Resolve records an override only inside the loop over the file, environment and flag sources; the defaults copy above it records nothing. Booted end to end with an empty app.toml, mode = "validator", and a sei.toml writing a sibling key of the same section so the install genuinely runs: it carried exactly evm.http_port, and ReadConfig still answered true for both toggles. The control, writing http_enabled = false, carried both keys and flipped it — so the harness can see the flip; it does not occur.
Three caveats I would rather state than have you find.
- That invariant is a convention, not a choke point. "Install only
Overrides" is implemented independently in three functions and no test names it. By this repo's own doctrine — guard at the choke point, never at each caller — that wants a named test, and your finding is the argument for one. I am adding it. - I am arguing from unmerged code.
installResolvedis not on this branch, so nothing you can read here supports the claim. It is onplt-775-node-install. - This registration's own godoc over-claims. It says the declared values are what
seid initwrites.seid initnever readsdefaults(mode)— it doesDefaultConfigthenSetEVMConfigByMode. Only the rule is shared. They agree because two hand-written tests say so, not because one reads the other.
A channel you did not name that does flip it: SEID_EVM_HTTP_ENABLED=false reaches the node. That is a source supplying a value rather than a default arriving, so it is intended — but it is a new way to close a public interface and worth knowing about.
And a larger divergence next door, which predates this PR. cmd/seid/cmd/root.go:424 builds the boot handler's app.toml from NewCustomAppConfig(srvCfg, evmrpcconfig.DefaultConfig) — the mode-blind default — so a validator that boots without an app.toml gets http_enabled = true written to disk, while seid init writes false. The binary already generates two disagreeing app.tomls for a validator, and the registry default matches one of them.
| // resolves values and does not convert them, so the reader that owns a key remains the thing that | ||
| // turns any of the three into what that key means. | ||
| // | ||
| // A value is the caller's to write into. Nothing here shares storage with a section's own default. |
There was a problem hiding this comment.
[suggestion] "A value is the caller's to write into" is broader than what detach actually guarantees. Only default-sourced values pass through walkValues/detach; file- and flag-sourced values are stored straight from the caller's maps (fileValues only re-keys to lower case, and from.Flags is ranged over directly). A caller that sorts or de-duplicates a resolved list in place — the exact scenario the second sentence and detach_test.go are about — still reaches the caller's own Sources.File map when the value came from the file.
That's the operationally common case for the motivating example: evm.deny_list is a deny list an operator sets in app.toml, so it arrives via File, not via the section default.
Secondly, detach is shallow — a map[string][]string or [][]string field would still share its nested backing storage. No registered section has one today, so this is about the stated invariant rather than a live bug.
Either narrow the sentence to say the guarantee covers values that came from a section's defaults, or detach file/flag values on the way in too.
There was a problem hiding this comment.
Both points confirmed by running them, and the second is worse than stated. Fixed in 484a614.
Your mechanism, measured: the resolved value and the caller's own Sources.File map shared a backing array at the same address, and sorting the resolved value rewrote the caller's map.
On shallowness — I registered a probe with a map[string][]string default held in a package-level variable and sorted the inner list through a value detach had returned. It rewrote the package variable, and the next resolution carried the sorted version. So narrowing the sentence to "values that came from a section's defaults" would still have been false; it was true only one level down. detach now copies all the way down, including through an interface, and a test drives the nested case through a second resolution rather than by inspecting the variable.
I had planned to argue that detaching file values on the way in was too expensive. That reason does not survive measurement — a full resolution of 218 keys across 17 sections is ~109 µs and 820 allocations, once per boot, so eleven extra copies are noise.
The reason that does hold is ownership, and half of it already exists: config/seitoml/values.go's handedOut recursively copies every list on the way out of the file cache, with a doc comment stating the same guarantee. So the production file source is already detached before Resolve sees it, and TypedFlags stores only f.Value.String(). Detaching again in Resolve would be a copy of a copy, placed in the layer that does not know what else holds the storage. The doc now says which source the guarantee covers and how deep, and points at the source that keeps its own half.
One thing your "no registered section has one today" is right about on the defaults side and not on the file side: telemetry.global-labels is declared []any and its reader takes untyped rows, so an operator writing it supplies nested storage through a channel nothing copies. It is also refused from the environment, so the file is the only way in. That is where this lands first if it lands.
A copy one level deep still shares the storage inside it. A list of lists, or a map of lists, hands a caller the inner storage through a copy of the outer, so sorting an inner list rewrites the section's own default exactly as directly as sorting the outer one would, and every resolution after it carries the sorted version. That is the defect this copy exists to prevent, reached through the value it returned. Nothing declared today has that shape. The copy runs once per boot over a hundred or so keys, so being thorough costs nothing measurable and not being costs a defect nobody finds twice. The doc claimed more than the copy delivered. It covers what a section's defaults answer, and not a value a source supplied: the file source hands out its own copies where it reads them, and the flag source carries only text. A source that grew a channel handing out live storage would have to do the same at its own edge, because only the source knows what else holds it. That is what it says now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ader's fallback The two interface toggles are declared per kind of node, following the rule the provisioning command applies, while this package's own reader falls back to both open for every kind. That reads like a behaviour change waiting to happen. It is not one. Only keys a source supplied are ever delivered, so a node whose file omits these has nothing written for them and the reader's fallback still answers. What would change that is a writer rendering every declared key into a file, and that is the case the note exists to be found by. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Registers the four EVM configuration sections.
All four register the struct their reader fills, and none needs a schema. 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 flag constants, which
are the second statement of the same set a few lines away in the same file, so a rename
that moves one and not the other compiles today and fails the test now.
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 declaring both open for every kind puts a public request
surface on the node that holds a signing key.
The rule already exists in this binary, and it could not be read from this package: the
package that owns the node mode imports this one, so reading it back would be a cycle. The
rule therefore moved into 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 fails a test.
A resolved value is the caller's to write into
A section's default is usually a package-level variable, so handing out a slice field handed
out the array that variable holds. 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 copied a section's keys for exactly this reason; values now get the same
guarantee, at the one function both walks pass through.
The copy goes all the way down. A copy one level deep still shares the storage inside it, so a
list of lists or a map of lists hands a caller the inner storage through a copy of the outer,
and sorting an inner list rewrites the default exactly as directly. I checked that rather than
reasoned about it: a probe registering a
map[string][]stringdefault, sorted one level in,rewrote the package variable and poisoned the next resolution. Nothing declared today has that
shape, and the copy runs once per boot over a hundred or so keys, so being thorough costs
nothing measurable.
What the guarantee covers is now stated exactly: what a section's defaults answer, all the way
down. It is not the guarantee for a value a source supplied. The file source hands out its own
copies where it reads them and the flag source carries only text, so a source growing a channel
that hands out live storage has to do the same at its own edge, because only the source knows
what else holds it.
telemetry.global-labelsis where that lands first if it lands: declared[]any, refused from the environment, so a file is the only way in.The EVM interface defaults differ from the reader's own fallback
This declares the two toggles per kind of node, following the rule the provisioning command
applies. The reader's own fallback has both open for every kind and keeps that when the key is
absent from a file. That reads like a behaviour change waiting to happen.
It is not one, and I had the reasoning checked rather than asserting it. Only keys a source
supplied are ever delivered, so a node whose file omits these has nothing written for them and
the reader's fallback still answers. Booted end to end with an empty app.toml and a file
supplying a sibling key of the same section: the install carried exactly that sibling and both
toggles still read open. Writing
http_enabled = falseinstead carries it and closes them, sothe check can see the flip.
What would change that is a writer rendering every declared key into a file. Then these two are
supplied and the node closes them, visibly, in a file the operator was handed. The note in the
source says so, because that is the moment to remember it.
Worth knowing separately:
SEID_EVM_HTTP_ENABLED=falsedoes reach the node. That is a sourcesupplying a value rather than a default arriving, so it is intended, but it is a new way to
close a public interface.
Two machine-derived values, which are not one case
The worker pool has a portable answer: it re-measures whenever the value it is given is not
positive, so a file carrying zero lets every node size itself, and a caller rendering into a
file should write that rather than the resolved number. The simulation call limit has none,
because zero there is not a request to measure but the absence of a limit, and that limit is
the only bound on how many simulations a node runs at once. Both describe the host that
resolved them, so neither travels.
Replay is the one with a name mismatch already in the tree. The template renders
eth_replay_contract_state_checksand the reader looks upcontract_state_checks, soevery generated
app.tomlcarries a name nothing resolves. The declared key is the onea value reaches a reader through, and a test refuses the other, because declaring it
would add a key an operator can set and no reader answers. That is worse than the
mismatch on its own: the value would look as though it applied.
I broke each reader on purpose and checked the tests noticed. Renaming one of the
fifty-seven tags, removing a tag entirely, renaming the one-key section, declaring
replay's written-but-unread name, closing an interface for a single mode, and replacing
a host measurement with a fixed number each fail a test.
What a resolved value's type depends on
A key's Go type depends on which source answered it, and more of them than I first wrote.
A default arrives as its field's own type, a file as whatever the format decodes to, and an
environment variable as one string, always. Across a generated
app.tomlthat is 35 of thedeclared keys and five type families, not the eleven durations I claimed: durations become
text,
intbecomesint64,uint64becomesint64,float64becomesint64where thetemplate renders a whole number, and a list of strings becomes a list of untyped elements.
The registry now states that contract where
Resolvedis declared, because it resolves valuesand does not convert them: the reader that owns a key stays the thing that turns any of the
three into what that key means. A consumer that installs resolved values without going
through that reader also skips two range checks, a cross-key invariant, and a rewrite that
de-duplicates tracer names. PLT-1019 tracks the convergence.
The recorded configuration surface does not move.