perf(ensindexer): unblock Ponder prefetch on hot tables#2016
Conversation
🦋 Changeset detectedLatest commit: 01f2fa3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
b9fdd7a to
3de1e2c
Compare
Two changes that together push the indexing-cache prefetch share from 0 to 35–50% on the hottest ENSv1 read paths: 1. Switch composite ids to dash-delimited tuples (`enssdk/src/lib/ids.ts`). Replace CAIP-style mixed `:` / `/` delimiters with a single `-` so Ponder's profile pattern matcher can string-split the id and match each segment against `event.chain.id` / `event.event.log.address` / `event.event.args.*`. Drops the ERC1155 namespace literal from makeENSv2DomainId since the registry contract already namespaces it. Run D measurements showed this lifts hot tables (Domain, Registry, accounts, etc.) to 80–100% prefetch share, up from 0% on the prior CAIP form. 2. Re-key migrated_nodes by (parentNode, labelHash) plus a sibling migrated_nodes_by_node index keyed on `node`. The five Registry handlers that consult migration status split between two payload shapes: ENSv1RegistryOld#NewOwner has parentNode + labelHash; the Transfer/NewTTL/NewResolver trio has only the post-namehash `node`. Ponder's matcher can't invert keccak, so a single-table layout surrenders prefetch on whichever shape it doesn't key. The two-table layout lets every read site address whichever key matches its event args, both stay on the prefetch hot-path. Cost is one extra conflict-do-nothing insert per migration write. See the block comment in migrated-node-db-helpers.ts for the full rationale. Schema split: pulled migratedNode + migratedNodeByNode into a sibling migrated-nodes.schema.ts re-exported from the ensindexer-abstract index, so the migration-status tables are co-located with their helpers and the protocol-acceleration schema stays focused on resolver/permissions data. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3de1e2c to
a88a715
Compare
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughSplit ENS migration tracking into two onchain tables keyed by (parentNode,labelHash) and node; add helpers to read/write both tables; remove legacy registry-migration-status; update ENSv1 handlers to use parent+label lookups; change composite ID constructors to dash-delimited format. Changes
Sequence Diagram(s)sequenceDiagram
participant Event as ENSv1RegistryOld Event
participant Handler as ENS Indexer Handler
participant Helper as MigratedNode DB Helper
participant DB as ENSIndexer Onchain Tables
Event->>Handler: NewOwner(parentNode, labelHash, ...)
Handler->>Helper: nodeIsMigratedByParentAndLabel(context,parentNode,labelHash)
Helper->>DB: SELECT FROM migrated_nodes_by_parent WHERE (parentNode,labelHash)
alt found
DB-->>Helper: exists
Helper-->>Handler: true
Handler-->>Event: return (ignore event)
else not found
DB-->>Helper: not found
Helper-->>Handler: false
Handler->>Handler: proceed -> handleNewOwner(...)
Handler->>Helper: migrateNode(context,parentNode,labelHash) [when migration recorded]
Helper->>DB: INSERT into migrated_nodes_by_parent & migrated_nodes_by_node (onConflictDoNothing)
DB-->>Helper: ack
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 46 seconds.Comment |
Greptile SummaryThis PR addresses two independent Ponder prefetch failures: (1) all composite ids switch from CAIP-style mixed Confidence Score: 5/5Safe to merge — no P0 or P1 issues found; all logic is correct and well-documented. The implementation correctly solves both prefetch problems: dash-delimited ids remove the CAIP delimiter ambiguity, and the two-table migration layout gives each event handler an addressable key. The two-table write is idempotent and consistent within Ponder's transactional flush model. The ETHRegistrar address-normalization fix is correct. The only finding is a P2 suggestion to align the testcontainers wait strategy with the newly-added healthcheck. packages/integration-test-env/src/orchestrator.ts — minor: wait strategy could leverage the new healthcheck. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["ENSv1Registry(Old)#NewOwner\n(parentNode, labelHash)"] -->|"migrateNode(parentNode, labelHash)"| B["migratedNodeByParent\nPK: (parentNode, labelHash)"]
A -->|"makeSubdomainNode → node\nmigrateNode writes both"| C["migratedNodeByNode\nPK: node"]
D["ENSv1RegistryOld#NewOwner\n(parentNode, labelHash)"] -->|"nodeIsMigratedByParentAndLabel"| B
E["ENSv1RegistryOld#Transfer\n(node)"] -->|"nodeIsMigrated"| C
F["ENSv1RegistryOld#NewTTL\n(node)"] -->|"nodeIsMigrated"| C
G["ENSv1RegistryOld#NewResolver\n(node)"] -->|"nodeIsMigrated"| C
style B fill:#d4edda
style C fill:#d4edda
Reviews (8): Last reviewed commit: "fix(enssdk): drop AccountIdString brand ..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR targets a Ponder indexing performance bottleneck by making composite IDs and migration-status lookups decomposable by Ponder’s profile matcher, enabling prefetch on hot tables in ensindexer.
Changes:
- Switched
enssdkID constructors from CAIP-style mixed delimiters to single--delimited tuple IDs. - Split ENSv1 registry-migration tracking into two tables keyed by
(parentNode,labelHash)and bynode, with paired writes and payload-matching reads. - Extracted migrated-node schema into its own
ensdb-sdkschema module and re-exported it viaensindexer-abstract.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/enssdk/src/lib/ids.ts | Changes all exported ID constructors to --joined tuples to make keys prefetch-profileable. |
| packages/ensdb-sdk/src/ensindexer-abstract/protocol-acceleration.schema.ts | Removes the legacy single-table migrated_nodes schema from this module. |
| packages/ensdb-sdk/src/ensindexer-abstract/migrated-nodes.schema.ts | Adds new two-table migrated-node schema (*_by_parent, *_by_node). |
| packages/ensdb-sdk/src/ensindexer-abstract/index.ts | Re-exports the new migrated-nodes schema module. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.ts | Updates migration writes to the new (parentNode,labelHash) helper API. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts | Updates migration reads to use payload-matching lookup helpers. |
| apps/ensindexer/src/lib/protocol-acceleration/registry-migration-status.ts | Removes the old single-table migration status helper. |
| apps/ensindexer/src/lib/protocol-acceleration/migrated-node-db-helpers.ts | Introduces new two-table read/write helpers with rationale documentation. |
| .changeset/enssdk-dash-delimited-ids.md | Changeset documenting the public enssdk ID wire-format change. |
| .changeset/ensdb-sdk-migrated-nodes-split.md | Changeset documenting the ensdb-sdk migrated-nodes table split/rekey. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@greptile review |
54aa7e5 to
0dfa9af
Compare
…plit Replace the single `migrated_nodes` section with the two-table layout: `migrated_nodes_by_parent` (composite PK on parentNode + labelHash) and `migrated_nodes_by_node` (sibling PK on node). Notes the rationale — the three RegistryOld handlers that emit only `node` need their own keyed access to stay on Ponder's prefetch hot-path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The shape of each id (CAIP-10/CAIP-19/dash-tuple etc.) is a ids.ts-implementation detail that drifts when the format changes (just did, in #2016). The brand-only doc comments now describe the entity identity without leaking the encoding, and reference ids.ts as the canonical source for the current shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The testcontainers DockerComposeEnvironment generates a unique project name per up() call (`testcontainers-<uuid>`), and each project gets its own volume namespace. A stale volume from a prior run is scoped under the prior project's name and is invisible to a new run's project — so there's no path for state to leak between runs. The original schema-collision error this was guarding against came from a host-native postgres on localhost:5432, fixed separately by the ephemeral host port mapping. With that in place, no scenario justifies the pre-up wipe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related changes: - docker-compose.orchestrator.yml: drop `container_name: ensdb` from the inlined ensdb override. Without an explicit container name the testcontainers project prefix (testcontainers-<uuid>) scopes the container globally, so concurrent orchestrators or co-resident dev stacks can't collide. - orchestrator.ts: the testcontainers wait-strategy + getContainer lookups parse the container name as `<project>-<service>-<index>` and return everything after the project prefix. Without a container_name that means `ensdb-1` (single-replica suffix), so update both call sites accordingly. Tried `extends + !override` to preserve DRY-ness against services/ensdb.yml, but standard YAML language servers don't recognize the compose-spec !override / !reset tags — kept the inline form for editor cleanliness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ENSv1RegistryId / ENSv2RegistryId / PermissionsId / ResolverId were
typed as `AccountIdString & { __brand: ... }`, advertising a CAIP-10
shape. After the dash-delimited id refactor (#2016) the runtime values
no longer satisfy that contract — they're tuple-joined strings, not
CAIP-10. Anything that took these ids and fed them into a CAIP-10
parser would silently fail.
Switch to plain `string & { __brand: ... }` so the type doesn't lie
about its shape. AccountIdString itself is still `string` under the
hood, so this is a documentation/intent change rather than a structural
one — no callers needed updating.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer Focus (Read This First)
packages/enssdk/src/lib/ids.ts— every id constructor switches from CAIP-style mixed://delimiters to dash-joined tuples. This is a wire-format change for every public id type. External consumers that string-parse or pattern-match against ids will see different shapes; the in-tree code treats them as opaque strings.apps/ensindexer/src/lib/protocol-acceleration/migrated-node-db-helpers.tsblock comment + the two-table layoutmigrated-nodes.schema.tsis mechanical, but perhaps should still live in protocol acceleration because that's the plugin that manages the tables?Problem & Motivation
:and/, which the matcher won't decompose, so every prefetch attempt on Domain / Registry / Resolver / etc. silently failed.migrated_nodeswas keyed by post-namehashnode, which the matcher also can't recover from event args. Run D measured 0% prefetch share on the table even though every ENSv1RegistryOld read consults it.What Changed (Concrete)
enssdk/src/lib/ids.ts: every id constructor now joins its components with-via_stringifyAccountIdmigrated_nodestable renamed tomigrated_nodes_by_parentand re-keyed from(node)to composite(parentNode, labelHash).migrated_nodes_by_nodekeyed bynodefor the four ENSv1RegistryOld handlers (Transfer / NewTTL / NewResolver) that emitnodeonly.migrateNodehelper writes both tables at once;nodeIsMigratedByParentAndLabelandnodeIsMigratedread from whichever table matches the calling handler's event payload.migratedNodeByParent+migratedNodeByNodemoved intopackages/ensdb-sdk/src/ensindexer-abstract/migrated-nodes.schema.ts, re-exported fromensindexer-abstract/index.ts. Removed fromprotocol-acceleration.schema.ts.Design & Planning
The id format change came out of investigating Ponder's prefetch path in
ponder/dist/esm/indexing-store/profile.js— specifically the delimiter splitter that only splits on a single delimiter character per call. Confirmed empirically in Run D (2026-04-27): dash-delimited ids pushed hot tables to 80–100% prefetch share, up from 0% under CAIP. The migration table re-key followed once the dominantmigrated_nodesoutlier was identified in Run D's per-table breakdown.Alternatives considered for
migrated_nodes:node→ 0% prefetch on the hot read path.(parentNode, labelHash)→ flips the floor but breaks the fournode-only read sites.INSERT ON CONFLICT DO NOTHINGper migration write; storage roughly doubles for this table (~1GB → ~2GB on mainnet today).Self-Review
nodeIsMigrated(node)vsnodeIsMigratedByParentAndLabel(parentNode, labelHash)— names match what the call site actually passes; tables followmigrated_nodes_by_*patternCross-Codebase Alignment
domain_eventschild writes — flagged in debugensv2indexing slowness #1982 as suspected-but-likely-not-worth.Downstream & Consumer Impact
migrated-node-db-helpers.tsis the canonical reference for the two-table design; ids.ts will carry a block comment explaining the prefetch trade-off.migratedNodeByParent/migratedNodeByNode— symmetric naming makes the keyed-by intent explicit.Testing Evidence
ensindexer,ensdb-sdk,enssdk,ensapi.pnpm lintclean.Scope Reductions
domain_events/resolver_events, flagged in debugensv2indexing slowness #1982 as marginal headroom vs. material code complexity.Risk Analysis
Storage cost. Migration-status footprint roughly doubles on mainnet (~1GB → ~2GB). Acceptable for the prefetch win.
External id consumers. Callers outside this monorepo that pattern-match against the CAIP id shape will see a different format. None known.
Mitigations or rollback options: the change is a single commit so revert is mechanical
Named owner if this causes problems: @shrugs
Pre-Review Checklist (Blocking)