Manual labels - #314
Conversation
|
|
||
| func archiveLogsRule() string { | ||
| return `[{"method":"eth_getLogs","result":[]}]` | ||
| return `[{"method":"eth_capabilities","result":{"blocks":{"oldestBlock":"0x0"},"logs":{"oldestBlock":"0x0"},"state":{"oldestBlock":"0x0"},"tx":{"oldestBlock":"0x0"},"receiptOldestBlock":"0x0"},"stateproofs":{"oldestBlock":"0x0"}}]` |
There was a problem hiding this comment.
This change looks unrelated to manual labels: this test runs with disable-labels-detection: true, so nothing in it exercises the new code path. It is also not mentioned in the PR description.
Beyond scope, the stub does not match evmCapabilitiesResponse (internal/upstreams/lower_bounds/evm_bounds/capabilities.go:224) in two places:
"receiptOldestBlock":"0x0"— the expected shape isreceipts: {oldestBlock: ...}, soReceiptsstays nil."stateproofs"sits after the closing brace ofresult, i.e. it is a sibling of"method"/"result"and therefore a rule field rather than a capability. The Hardhat hook (test/e2e/internal/hardhat/hardhat.config.cjs:38) ignores unknown rule fields silently, soStateProofsstays nil too.
With ReceiptsBound/ProofBound uncovered, detectFromCapabilities returns false (capabilities.go:314) and falls back to the search path — whose eth_getLogs stub is removed by this same diff. The assertion on line 44 (result must be an empty array) then depends on the real fork provider answering eth_getLogs for 0x1000..0x1001.
Suggest moving this to a separate PR and running it against Docker.
There was a problem hiding this comment.
Fixed the JSON. Let's leave it here, this test is super flaky, don't want to create a separate PR
| DisableLabelsDetection *bool `yaml:"disable-labels-detection"` | ||
| DisableLogIndexValidation *bool `yaml:"disable-log-index-validation"` | ||
| DisableLivenessSubscriptionValidation *bool `yaml:"disable-liveness-subscription-validation"` | ||
| ArchiveCapability *bool `yaml:"archive"` |
There was a problem hiding this comment.
Config parsing is non-strict (internal/config/config.go:57 uses yaml.Unmarshal), so removing this field means a leftover options.archive: false is silently ignored: the archive detector starts running again and overwrites the label with whatever it detects. For clients using the gRPC archive label selector that is a routing change with no log line.
A cheap migration path is to keep the field as deprecated and translate it in Upstream.setDefaults (internal/config/defaults.go:320, right after setOptionsDefaults, so chain-defaults are already merged):
if u.Options.ArchiveCapability != nil {
log.Warn().Msgf("upstream '%s': options.archive is deprecated, use labels.archive instead", u.Id)
if _, set := u.Labels["archive"]; !set {
if u.Labels == nil {
u.Labels = UpstreamLabels{}
}
u.Labels["archive"] = strconv.FormatBool(*u.Options.ArchiveCapability)
}
}This also restores the chain-defaults capability the PR description lists as lost. If a shim is not wanted, a validator that fails startup with an explicit message would still be preferable to a silent routing change.
| RateLimit *RateLimiterConfig `yaml:"rate-limit"` | ||
| RateLimitAutoTune *RateLimitAutoTuneConfig `yaml:"rate-limit-auto-tune"` | ||
| GroupLabels []string `yaml:"group-labels"` | ||
| Labels UpstreamLabels `yaml:"labels"` |
There was a problem hiding this comment.
ChainDefaults (line 213) is a plain struct and Upstream.setDefaults already receives it, so adding Labels UpstreamLabels \yaml:"labels"`there plus a per-key merge (upstream value wins) would cover the chain-defaults case in a few lines. Without it, an operator running 30 upstreams on one chain has to repeat the same label 30 times, which is the one thingoptions.archive` could do and this cannot.
There was a problem hiding this comment.
Let's skip it for now. With such setting it's very easy to make a mess applying labels to all the upstreams
| type UpstreamLabels map[string]string | ||
|
|
||
| func (u *UpstreamLabels) UnmarshalYAML(node *yaml.Node) error { | ||
| node = resolveYamlAlias(node) |
There was a problem hiding this comment.
This call is unreachable. yaml.v3 resolves an alias node before invoking a custom unmarshaller — AliasNode is handled in d.unmarshal ahead of d.prepare, which is what dispatches to UnmarshalYAML. For labels: *shared this method receives Kind == MappingNode (4), never AliasNode (16).
I verified this by deleting the line: TestUpstreamLabelsAliasForWholeMapping and the rest of the package still pass, so the line is also not covered by the new tests. Only the per-value call on line 152 is load-bearing. The PR description and the design doc state that aliases are resolved "both for the labels node and for each value", which holds only for the value case.
| labels := make(UpstreamLabels, len(node.Content)/2) | ||
| for i := 0; i+1 < len(node.Content); i += 2 { | ||
| key, value := node.Content[i], node.Content[i+1] | ||
| if key.Value == "<<" { |
There was a problem hiding this comment.
This matches on the key's text, so a quoted "<<": x — a literal label named <<, not a merge key — is rejected with the merge-key message. yaml.v3 distinguishes the two by tag, so key.Tag == "!!merge" would be the accurate check.
| return fmt.Errorf("label '%s' must have a scalar value", key.Value) | ||
| } | ||
| if _, exists := labels[key.Value]; exists { | ||
| return fmt.Errorf("duplicate label '%s'", key.Value) |
There was a problem hiding this comment.
Errors raised from a custom unmarshaller carry no position information, so this surfaces as duplicate label 'archive' with no line number and no upstream id — awkward to locate in a config with many upstreams. Same for the messages on lines 144 and 154.
Decoding into map[string]yaml.Node keeps the literal-text behaviour and gets the diagnostics from yaml.v3 instead:
func (u *UpstreamLabels) UnmarshalYAML(node *yaml.Node) error {
if node.Kind != yaml.MappingNode {
return errors.New("labels must be a mapping of label names to scalar values")
}
var raw map[string]yaml.Node
if err := node.Decode(&raw); err != nil {
return err
}
labels := make(UpstreamLabels, len(raw))
for key, value := range raw {
if value.Kind != yaml.ScalarNode {
return fmt.Errorf("label '%s' must have a scalar value", key)
}
labels[key] = value.Value
}
*u = labels
return nil
}I checked the behaviour of this variant: false/0x10/1e3/"false" all keep their literal text; a duplicate key reports line 3: mapping key "archive" already defined at line 2; a non-scalar value still hits the message above; an alias for the whole mapping still works. Merge keys also start working (<<: *shared plus archive: "false" yields both labels) rather than being rejected, which removes the need for both the << branch and resolveYamlAlias.
| } | ||
| } | ||
|
|
||
| for label, value := range u.Labels { |
There was a problem hiding this comment.
Map iteration order is random, so with several offending labels the reported error varies between runs. Iterating over sorted keys would make the message deterministic.
| // the process lifetime. Any other value (including "true") lets the detector run and | ||
| // publish what it finds. | ||
| func archiveDetectionSuppressed(manualLabels map[string]string) bool { | ||
| return manualLabels["archive"] == "false" |
There was a problem hiding this comment.
The "archive" key is now spelled out in two packages — here and in internal/upstreams/labels/eth_labels/eth_archive_detector.go:33. Exporting a single constant (for example eth_labels.ArchiveLabel) would keep the detector's output key and the suppression check from drifting apart on a rename.
|
|
||
| b.upstreams.Store(up.GetId(), up) | ||
|
|
||
| // to not lost the first events |
There was a problem hiding this comment.
The reordering is correct — Start() pushes InitUpstreamStateEvent onto stateChan and only then launches processStateEvents (upstream.go:236-237), so the publish happens after Start() returns and the old Start → Subscribe order did have a real window. Subscribing first is also safe from a blocking standpoint, since Publish is non-blocking with a 100-slot buffer (pkg/utils/subscriptions.go:131). Moving Store up likewise removes the nil GetUpstream skip at upstream_supervisor.go:177-185.
Two notes:
- Typo:
to not lost→to not lose. It would also help to state what is being fixed (theInitUpstreamStateEventpublished fromStart()was droppable) and whySubscribeWithReplay(pkg/utils/subscriptions.go:88), which exists for exactly this "late subscriber must learn current state" case, was not used. - This is the only behavioural change in the PR without a test.
| - `failsafe-config` - Upstream-level failsafe configuration. Only the `retry` policy can be specified at this level (hedging and timeouts are configured globally on `upstream-config.failsafe-config`) | ||
| - `group-labels` - List of priority-group labels this upstream belongs to, used by [label-balancing](#label-balancing). These are **config-defined** labels, independent of the runtime labels produced by label detectors. An upstream may belong to several groups but is still selected at most once per request | ||
| - `labels` - Map of manual labels published for this upstream. Values are strings; unquoted YAML scalars are accepted and stored as their literal text (`archive: false` is the same as `archive: "false"`). Keys and values must both be non-empty. Manual labels are **seeds**: they are published to the upstream's state at startup - so they are visible to [gRPC](12-grpc-server.md) label selectors and label matchers even when `disable-labels-detection` is `true` - but a runtime label detector that owns the same key overwrites them on its first round. The one exception is `archive: false`, which skips the EVM archive detector entirely so the configured value stands - the match is an exact, case-sensitive comparison against the literal text `false`, so `archive: False` or `archive: "FALSE"` does **not** suppress the detector and silently leaves auto-detection running. This is distinct from `group-labels`, which is config-only input to [label-balancing](#label-balancing) and is never published to upstream state; manual labels take no part in label-balancing | ||
| > **Migration**: the old `options.archive` flag has been removed. Set `labels: {archive: false}` on the upstream instead. A leftover `options.archive` key is silently ignored (config parsing does not reject unknown keys), which means archive auto-detection starts running again for that upstream. Note that `options.archive` used to also be settable at `chain-defaults.<chain>.options.archive`, but `labels` has no chain-defaults equivalent - it is per-upstream only. A `chain-defaults.<chain>.labels` key is likewise silently ignored, so the migration must be repeated on every upstream of the chain individually |
There was a problem hiding this comment.
If the deprecation shim suggested on pkg/chains/options.go is added, this note becomes "options.archive is deprecated, still honoured, and warns at startup", and the chain-defaults paragraph can go away. Worth revisiting once that is decided.
Manual upstream labels, replacing
options.archiveSummary
An upstream can now declare labels statically in the config, the way dshackle does:
Those labels are seeds: they are written into
protocol.UpstreamState.Labelswhen the upstream isconstructed, so label matchers and the gRPC label selectors see them immediately — including when
disable-labels-detectionistrueand no detector ever runs. A runtime detector that owns the same keyoverwrites the manual value on its first round, exactly as it would overwrite any earlier value.
One exception, and it is the mechanism that replaces the old flag: when the manual
archivelabel is exactly"false", the EVM archive detector is never started, so the configured value stands for the processlifetime.
chains.Options.ArchiveCapability(options.archive) is deleted.Motivation
Labels could only be discovered at runtime. Operators who already know what a node is — an archive node, a
particular provider, a tier — had no way to say so, and the one escape hatch that existed (
options.archive)was a single-purpose boolean bolted onto the options struct. It could pin exactly one label, only on EVM
chains, and only to
false.The old flag also expressed its intent awkwardly:
archive: falsemeant "publisharchive=false, skipdetection", while
archive: truemeant "ignore me, run detection anyway" — identical to leaving it unset.Expressing the same thing as a label makes the mechanism general (any key, any value) and drops a special
case from
chains.Options.What changed
The config field (
internal/config/upstream_config.go)UpstreamLabelsismap[string]stringunderneath, with its ownUnmarshalYAML. A plainmap[string]stringwould reject
archive: falseoutright (cannot unmarshal !!bool into string), and unquoted scalars are howdshackle configs are written, so the unmarshaller walks the mapping node and stores each value's literal text:
false→"false",3→"3","false"→"false".It rejects, at parse time: a non-mapping
labels, a sequence or nested map as a value, and a duplicate key(node-walking bypasses yaml.v3's own duplicate check, so we do it ourselves). YAML aliases are resolved before
the kind checks — both for the
labelsnode and for each value — so a shared labels block can be anchored andreused across upstreams. Merge keys (
<<) stay unsupported but say so instead of blaming a label named<<.Upstream.validaterejects an empty key and an empty value.The type is named
UpstreamLabels, notLabels, becauseinternal/upstreams/upstream.goimports both thispackage and
protocol, which already has aLabelstype.Seeding the state (
internal/upstreams/upstream.go)NewBaseUpstreamfills theLabelsobjectDefaultUpstreamStatealready allocated, before the state ispublished:
No new
protocolconstructor, no secondLabelsobject, andDefaultUpstreamState's signature is unchanged.Mutating the state before
Storeis safe because nothing else holds a reference yet, andatomic.Value.Storegives the happens-before for every later
Load.NewBaseUpstreamWithParamsis deliberately untouched — it receives an already-built state from its caller, andseeding there would stomp the caller's intent.
The archive detector (
chains_specific/evm_specific/evm_chain_specific.go)archiveLabelsDetectorand its static-falsedetector are gone.LabelsProcessor()now delegates to aprivate
labelsDetectors(), which appends the archive detector only when the label doesn't pin it:The match is exact-string.
archive: False,"FALSE"and"0"do not suppress detection — documented,because it fails silently rather than loudly.
internal/upstreams/labels/static_labels_detector.gois deleted; the archive path was its only caller, and aseeded value needs no detector republishing a constant every interval.
Tron shares the EVM detectors
NewTronSpecific'sjson-rpcbranch delegates toNewEvmChainSpecific, and the oldoptions.archivewashonoured on that path. It therefore takes the same final
manualLabelsparameter and forwards it, withgetChainSpecificpassingconf.Labels. Without this,archive: falsewould have silently stopped workingfor Tron json-rpc upstreams — a regression, not a no-op. The
restbranch needs nothing:TronRestSpecific.LabelsProcessorhas no archive detector.Removing
options.archiveThe field is deleted from
pkg/chains/options.go. Config parsing is non-strict (yaml.Unmarshal), so aleftover
options.archivekey is silently ignored — no shim, no deprecation validator. The docs call thisout, because the practical effect is that archive auto-detection starts running again for that upstream.
Notes / impact
upstream.options.archive: falseorchain-defaults.<chain>.options.archive: falsemust move it to the upstream'slabels. The old key parsesfine and does nothing.
labelsis per-upstream only. There is nochain-defaults.<chain>.labels; that key is also silentlyignored. This is the one capability the old flag had that the replacement doesn't — it could be set for a
whole chain at once.
false,3,1.5are allfine.
archive: nullbecomes the literal string"null", which suppresses nothing; barearchive:isrejected as an empty value.
group-labels.group-labelsremains config-only input tolabel-balancingand is neverpublished into upstream state; manual labels take no part in label-balancing. Both fields exist on the
upstream and the docs now contrast them explicitly.
UpstreamState.Labelsthat detectors write,so
LabelMatcher,LabelExistsMatcher,chain_supervisor_state.go's aggregation and the dshackleNodeDetailsmapper consume them unchanged.Adjacent fix: the seeded state could be dropped at startup
StartUpstreamscalledup.Start()beforeup.Subscribe(...), andSubscriptionManager.Publishdropsevents when there is no subscriber. The
InitUpstreamStateEventthat carries the seeded labels is publishedinside
Start(), so under an unlucky schedule it was lost — and unlike the old static detector, a seed doesnot re-publish itself every interval to heal.
internal/upstreams/upstream_supervisor.gonow runs Subscribe →b.upstreams.Store→Start. Moving theStoreup as well closes the second half: an event emitted duringStart()now finds the upstream registered,so
processEvents'GetUpstream(event.Id)can't return nil and silently skipResume()/PartialStop().Docs
docs/nodecore/05-upstream-config.md:labelsentry in the per-upstream Fields list: seed semantics, the exact/case-sensitivefalsematch,bare-scalar values, and the contrast with
group-labels.options.archiveandchain-defaults.<chain>.labels).options.archivebullet is gone; the top-level example's upstream carries an inertlabels: {provider: hetzner}rather thanarchive: false, so copy-pasting the canonical example can'tdisable archive detection by accident.
Testing
go test -race -p 8 ./internal/... ./pkg/...— 80 packages, no failures, no races.go vet -tags e2e ./test/e2e/http/clean.make lint— 0 issues.false/3/1.5and quoted"false"), null value, non-scalar value,non-mapping
labels, duplicate key (exact message), alias-to-scalar, alias-as-whole-mapping, merge key, andfixture-driven empty-key / empty-value validation through
NewAppConfig.NewBaseUpstreamproduces, including withdisable-labels-detection: true; a detector event for the same key replaces the seed. The overwrite testasserts the seeded value is in place before applying the event, so it fails if seeding is removed —
verified by deleting the loop.
"false"/"true"/"False"/""/ unrelated / nil, plus a testover
labelsDetectors()asserting the archive detector is absent forarchive: "false"and present for nil,"true"and"False". Presence/absence rather than a count, so an unrelated new detector won't break it.Verified load-bearing by forcing the condition true.
archive_selector_e2e_test.go's generated config moved tolabels. Note that with seeding thistest no longer exercises the archive detector — both upstreams carry their label before any probe runs — so
it now validates manual label → gRPC selector → routing, with the detector contract covered at unit level.
Compile-checked here (
go vet -tags e2e); the Docker run needsNODECORE_E2E_DRPC_KEY.Known gap: the two lines in
upstream_factory.gothat passconf.Labelsinto the constructors have no testthat fails if they're mutated to
nil— the detector tests call the constructors directly.getChainSpecificis package-private and already covered by a white-box test, so this is closable there.