feat(accounts): give nominator_positions a live refresh lane and stop the confident zero - #9292
Merged
Merged
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
metagraphed-data-api | fb960fb | Aug 03 2026, 01:40 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
metagraphed-registry-sync-api | fb960fb | Aug 03 2026, 01:40 PM |
| const rows = await r2SqlQuery( | ||
| env, | ||
| "SELECT MAX(observed_at) AS latest FROM chain.account_events" + | ||
| ` WHERE coldkey = '${coldkeyLiteral}'` + |
There was a problem hiding this comment.
P2: SQL injection risk via string interpolation when parameterized queries are available
latestStakeEventAt interpolates coldkeyLiteral directly into an R2 SQL query string even though r2SqlQuery supports bound parameters.
Use r2SqlQuery bound parameters with ? placeholders instead of string interpolation for all query values.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="src/nominator-positions-cold-tier.ts">
<violation number="1" location="src/nominator-positions-cold-tier.ts:219">
<priority>P2</priority>
<title>SQL injection risk via string interpolation when parameterized queries are available</title>
<evidence>The latestStakeEventAt function constructs an R2 SQL query by interpolating coldkeyLiteral, STAKE_ADDED_KIND, and STAKE_REMOVED_KIND into the query string:
"SELECT MAX(observed_at) AS latest FROM chain.account_events" +
` WHERE coldkey = '${coldkeyLiteral}'` +
` AND event_kind IN ('${STAKE_ADDED_KIND}', '${STAKE_REMOVED_KIND}')`
While coldkeyLiteral is expected to be pre-sanitized via safeSs58Literal, the parameter name is a convention, not an enforcement. The r2SqlQuery helper used elsewhere in the same file already supports bound parameters via ? placeholders (see neuronStakeByHotkeys). Using string interpolation when parameterized queries are available creates a SQL injection risk if the function is ever called with an unsanitized value, or if the safeSs58Literal helper is bypassed.</evidence>
<recommendation>Replace the string interpolation with parameterized query placeholders:
const rows = await r2SqlQuery(
env,
"SELECT MAX(observed_at) AS latest FROM chain.account_events" +
" WHERE coldkey = ? AND event_kind IN (?, ?)",
[coldkeyLiteral, STAKE_ADDED_KIND, STAKE_REMOVED_KIND],
);
This eliminates the injection surface entirely and aligns with the pattern already used in neuronStakeByHotkeys.</recommendation>
</violation>
</file>
JSONbored
force-pushed
the
fix/nominator-positions-live-lane
branch
2 times, most recently
from
August 3, 2026 13:14
0bec87b to
004e92f
Compare
… the confident zero (#9273) `/api/v1/accounts/{ss58}/positions` served `captured_at: 2026-08-02T01:38:22.670Z` and could never advance: the ledger behind it was written by a lane on the retired box, the lakehouse holds the frozen 153,611-row export, and nothing refreshed it. Worse than the staleness, an account that began delegating after that export got `positions: 0, total_stake_alpha: 0` -- four of five coldkeys sampled from a live /validators/{hotkey}/nominators response came back that way, all of them provably delegating right now. The lane, on the pattern the neurons/hyperparams/account-identity lanes use: - migrations/d1/0011 adds `nominator_positions` on D1, upserted on (coldkey, hotkey, netuid) with the usual `captured_at <= excluded.captured_at` staleness guard. The writer's column list and the table are asserted against each other in both directions, the same anti-drift guarantee 0007 has. - src/nominator-positions-d1-write.ts reuses neurons-d1-write's statement building, so one place owns the Workers-binding budget of 100 bound parameters per statement -- asserted as the platform limit in the tests, not as a constant we picked. The prune is PER COLDKEY, not batch-wide: a full Alpha scan does not fit one request body, so a batch-wide sweep would delete the rows a sibling request just wrote. - workers/data-api.ts's handleNominatorPositionsSync stops answering `503 hyperdrive binding unavailable` and writes that batch to D1. share_fraction is range-checked as well as finite-checked: it multiplies a hotkey's whole stake at serve time, so it is the one field whose garbage would read as a plausible number rather than as an error. - src/nominator-positions-staleness-watchdog.ts + a 8,38 cron is the alarm. This gap existed because nothing noticed the writer was gone; an EMPTY table alerts too, since that is the state in which every read is still answering from the frozen export. Serving is now Postgres -> D1 hot -> lakehouse cold -> labelled empty. The hot leg declines while its table is empty, so the cutover is a property of the data rather than of a deploy. And the correctness fix that does not wait for the lane: - A zero from the lakehouse leg now reads the ledger's own capture stamp and this account's newest on-chain StakeAdded/StakeRemoved. The stamp fills `captured_at` so the age of a rowless answer is visible at all; a stake event NEWER than the ledger contradicts the zero, and the payload says so (`degraded.reason: snapshot_predates_stake_activity`) instead of asserting it. Both reads fire only on the zero path, in parallel, with the ledger stamp memoized per isolate. - When every tier declines, the card is `unavailableAccountPositions` (`degraded.reason: tier_unavailable`) rather than a bare zero -- the same defect class as #9260/#9263, worse here because the payload carried a confident total rather than merely an empty list. `degraded` is optional, so a consumer that ignores it reads exactly what it read before. Closes #9273
markMcpTierDegraded overwrote any `degraded` a handler had already set, so get_account_positions' specific `snapshot_predates_stake_activity` reason -- the position ledger predates a stake event this coldkey has on chain -- was replaced by the generic `tier_unavailable` on every MCP call, making MCP the one surface that could not report WHY a zero is untrustworthy while REST and GraphQL both could. Every one of those answers is degraded either way, so keeping the more specific reason never loses the signal the marker exists for. Refs #9273
…ating it R2 SQL has no bound parameters, so safeSs58Literal is the only thing between a request path and the warehouse -- and this function is exported, so "the one caller already validated it" was a property of today's code, not of the function. Re-validating inside is idempotent on an already-safe literal and refuses anything else rather than escaping it, matching every other predicate builder in src/r2-sql.ts. Refs #9273
JSONbored
force-pushed
the
fix/nominator-positions-live-lane
branch
from
August 3, 2026 13:39
004e92f to
fb960fb
Compare
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
JSONbored
added a commit
that referenced
this pull request
Aug 3, 2026
…advance again (#9302) `nominator_count` on /api/v1/validators and /api/v1/validators/{hotkey} is served from the lakehouse mirror of `chain.validator_nominator_counts` (#9276), and that mirror is frozen. Measured live 2026-08-03: 112,550 rows, newest `captured_at` 2026-08-02T01:38Z -- the retired box's last scan -- covering only 564 of the 1,031 validators the leaderboard serves. Coverage can only fall from here as new validators register. This is a SINK problem, not a producer problem. metagraphed-infra's poller Container already carries the full SubtensorModule::Alpha scan this table is derived from (src/bin/poller/jobs/validator_nominators.rs, 24h tick); it is one of the five lanes Dockerfile.poller leaves disabled "until they have a Cloudflare-native sink", because it writes to a Postgres that no longer exists. The scan needs no rehosting. Its write target does. The lane, on the pattern #9273/#9292 used for `nominator_positions` -- the other output of that same scan: - migrations/d1/0012 adds `validator_nominator_counts` on D1, upserted on (hotkey) with the usual `captured_at <= excluded.captured_at` staleness guard. The writer's column list and the table are asserted against each other in both directions, the same anti-drift guarantee 0007 has. No prune, and for a sharper reason than account_identity's: the producer's pass over Alpha IS exhaustive, but it chunks across requests, so "absent from this batch" and "absent from the scan" are different statements and only the latter would license a delete. - src/validator-nominator-counts-d1-write.ts reuses neurons-d1-write's statement building, so one place owns the Workers-binding budget of 100 bound parameters per statement -- asserted as the platform limit in the tests, not as a constant we picked. At 3 columns this chunks to 30 rows a statement, so a full scan is ~3,752 statements. - workers/data-api.ts's handleValidatorNominatorCountsSync stops answering `503 hyperdrive binding unavailable` and writes that batch to D1. `nominator_count` is required to be a non-negative integer rather than coerced: the read side already discards anything else, and a value the route accepted but every reader silently drops is worse than a 400 the producer can see. - The read is wired into buildGlobalValidators/buildValidatorDetail, which were handed a hardcoded empty map / null count. It joins against `neurons` inside SQLite rather than inlining a key list -- the leaderboard covers ~1,031 hotkeys against a 100-parameter cap, so a correlated subquery costs zero bound parameters and one query where an IN list would cost a dozen round trips. #9276's serving-Worker lakehouse overlay is deliberately left in place. It only collects hotkeys whose count is still null and returns early when there are none, so it covers what D1 has not received yet and stops firing on its own once a full scan lands -- the cutover is a property of the data, not of a deploy. Failure posture is the family's throughout: any failed read degrades to null, which is exactly what this tier served before the table existed. A broken read is a lost enrichment, never a wrong number. Closes #9301 Part of #9146
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
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.
Summary
/api/v1/accounts/{ss58}/positionsservedcaptured_at: 2026-08-02T01:38:22.670Zand could never advance: the ledger behind it was written by a lane on the retired box, the lakehouse holds the frozen 153,611-row export (feat(api): serve account positions and validator nominators from the lakehouse #9266 made it readable again), and nothing refreshed it.positions: 0, total_stake_alpha: 0. Four of five coldkeys sampled from a live/validators/{hotkey}/nominatorsresponse came back that way — all of them provably delegating right now. That is fix(explorer): /blocks/{n}/chain-events has no cold reader — empty for all 8.76M historical blocks #9260/fix(accounts): the account summary serves empty event aggregates while /events serves 100 for the same account #9263's failure shape, and worse here, because the payload carried a confident total rather than merely an empty list.What Changed
The lane (write side)
migrations/d1/0011_nominator_positions.sql—nominator_positionson D1, upserted on(coldkey, hotkey, netuid)with the usualcaptured_at <= excluded.captured_atstaleness guard. The migration's column set and the writer's bound column list are asserted against each other in both directions, the same anti-drift guarantee0007_neurons.sqlhas.src/nominator-positions-d1-write.ts— reusessrc/neurons-d1-write.ts's statement building rather than restating it, so one place owns the bound-parameter arithmetic. The prune is per coldkey, not batch-wide: a fullSubtensorModule::Alphascan does not fit one request body, so a batch-wide sweep would delete the rows a sibling request just wrote. That is the exact analogue of neurons-sync's per-netuid prune, resting on the same poster contract.workers/data-api.ts'shandleNominatorPositionsSyncstops answering503 hyperdrive binding unavailableand writes the batch to D1. The api-Worker proxy and the token gate already existed and are unchanged.share_fractionis range-checked, not merely finite-checked: it multiplies a hotkey's whole stake at serve time, so it is the one field here whose garbage would read as a plausible number instead of as an error.D1's bound-parameter limit. Every statement this lane issues is chunked under the Workers-binding limit of 100 bound parameters per statement — not the 1,200 the
wrangler d1 executeHTTP door allows. The tests assert the platform limit (params.length <= D1_BIND_PARAM_CAP) rather than a chunk count we chose, because the count grows with the network and the limit does not.The alarm
src/nominator-positions-staleness-watchdog.ts+NOMINATOR_POSITIONS_STALENESS_WATCHDOG_CRON = "8,38 * * * *", modelled onsrc/neurons-staleness-watchdog.ts. This gap existed because nothing noticed the writer was gone. An empty table alerts too — that is the state in which every positions read is still answering from the frozen export.workers/config.tsand stay off the*/5raw-capture and*/15probe grids; a test asserts the string is unique across every*_CRONexport and thatwrangler.jsoncdeclares the trigger, since dispatch keys on the literal cron string.Serving
Postgres → D1 hot → lakehouse cold → labelled empty, wired identically in REST, MCP (both call sites), and GraphQL so the three surfaces cannot disagree. The hot leg declines while its table is empty, so the cutover is a property of the data rather than of a deploy — there is no window in which a config value and the table disagree about which tier is real.The correctness fix — it does not wait for the lane
StakeAdded/StakeRemovedfromchain.account_events(which is live). The stamp fillscaptured_at, so the age of a rowless answer is visible at all — previously it wasnull, which told a caller nothing. A stake event newer than the ledger contradicts the zero, and the payload now says so (degraded.reason: "snapshot_predates_stake_activity") instead of asserting it.unavailableAccountPositions(degraded.reason: "tier_unavailable", the same vocabulary the analytics routes' degraded header uses) rather than a bare zero.degradedlabel: a lakehouse that cannot answer says nothing about whether the zero is real.degradedis optional on the artifact, not nullable: its absence is the healthy case, so a consumer that ignores it reads exactly what it read before, and one that checks it can tell "delegates nothing" from "we cannot currently say".Third commit —
latestStakeEventAtsanitizes its own input. R2 SQL has no bound parameters, so every predicate in that module is interpolated andsafeSs58Literalis the only thing between a request path and the warehouse. The function is exported, so "the one caller already validated it" was a property of today's code rather than of the function; it now re-validates internally (idempotent on an already-safe literal) and refuses anything else rather than escaping it, matching every other predicate builder insrc/r2-sql.ts. Superagent flagged this on the first push and it was a fair call.Second commit — the MCP chokepoint.
markMcpTierDegradedoverwrote anydegradeda handler had already set, so the specificsnapshot_predates_stake_activityreason would have been replaced by the generictier_unavailableon every MCP call — making MCP the one surface unable to report why a zero is untrustworthy, while REST and GraphQL both could. Every one of those answers is degraded either way, so keeping the more specific reason never loses the signal that marker exists for.Measured against production while writing this
Sampled coldkeys from live
/validators/{hotkey}/nominatorsresponses and asked the live route what they hold. Every one of them — including5DM2txvjAfp4ASTWmnUvZX4iPNeou8RWPVbXYqtBGtSzvSGc, which has 45,006 stake events and staked as recently as today — comes back:captured_at: nullwith zero rows is the signature of every tier declining, not of an account that holds nothing — and today the route publishes it as a confident total with no header, no field, and nothing else to distinguish it. After this change that exact response carriesdegraded: {"reason": "tier_unavailable", …}.To be explicit about what this PR does and does not do for that account: it does not make it return real positions on merge. The hot leg's table is empty until the poller Container posts its first batch, so the answer stays zero — but it stops being a confident zero, and the watchdog starts saying out loud that the lane has never run. Once the Container posts, the same account resolves through the D1 leg with a current
captured_at, which is the half of #9273 that cannot be closed from this repo alone.Registry Safety
Closes #<n>) — required.npm run build;openapi.json,types.d.ts,packages/contract/index.d.tsregenerated and committed).public/metagraph/r2-manifest.jsonandschemas/index.jsonare not in the diff.degradedobject onAccountPositionsArtifact, declared inschemas-src/routes/account-positions.tswith per-field descriptions.Validation
All run locally against this branch, rebased on
origin/main@b6907b8c5:npm run lint·npm run format:check·npm run typechecknpm run validate— 129 native subnets, 3444 surfaces, 136 providers, 2037 candidatesnpm run validate:schemas·npm run validate:api(185 routes + 8 Postgres-tier) ·npm run validate:openapi(185 + 24 feed + 70 network variants) ·npm run validate:types·npm run validate:contract-driftnpm run validate:mcp— 210 tools, lifecycle + both subscribe→notify round tripsnpm run validate:migrations·npm run validate:artifact-budgets·npm run validate:docs·npm run validate:intake·npm run validate:workflows·npm run validate:private-boundarynpm run scan:public-safetynpm run test:coverage— full suite green; statements 99.18%, branches 98.05%, lines 99.43%git diff --checkapps/ui's three doc generators +packages/client/packages/ui-kitdist rebuilds all produce no driftPatch coverage — diff ∩ v8's uncovered set, not a whole-file percentage:
src/account-nominator-positions.tssrc/nominator-positions-cold-tier.tssrc/nominator-positions-d1-write.tssrc/nominator-positions-hot-tier.tssrc/nominator-positions-staleness-watchdog.tsworkers/api.tsworkers/config.tsworkers/data-api.tssrc/mcp-server.tssrc/r2-sql.tssrc/graphql.ts·workers/request-handlers/entities.tsNew/changed test files:
tests/nominator-positions-d1-write.test.ts,tests/data-api-nominator-positions-d1.test.ts(end-to-end through the real Worker fetch handler against a real SQLite database),tests/nominator-positions-staleness-watchdog.test.ts,tests/nominator-positions-hot-tier.test.ts, plus additions totests/nominator-positions-cold-tier.test.tsand updates to the two existing tests whose premise this change moves (tests/data-api.test.ts's "answers 503" andtests/request-handlers-entities.test.ts's "D1 never queried").Deployment note
The D1 migration is applied out-of-band with
wrangler, as0010was. Until the poller Container starts POSTing to/api/v1/internal/nominator-positions-sync, the hot leg's table is empty, so it declines and the lakehouse leg keeps answering exactly as it does today — with the honest-zero labelling above, which is live immediately. The watchdog will alert on the empty table until the first batch lands, which is the correct signal.Closes #9273