chore: retarget IPNI verification from advertised to synced, single cid.contact check - #690
chore: retarget IPNI verification from advertised to synced, single cid.contact check#690BigLep wants to merge 2 commits into
Conversation
….contact check Temporary checkpoint commit for review, not final. - monitorPieceStatus now gates on synced (Curio-confirmed indexer sync) instead of advertised; advertised is logged but no longer returns early - collapse the old two-step filecoinpin.contact -> cid.contact verify() into a single authoritative cid.contact check - add IpniStatus.SP_SYNCED plus ipniSyncedAt/ipniTimeToSyncMs tracking (Postgres migration, Deal entity, ClickHouse mirror) - switch checkIpniIndexer (was waitForIpniProviderResults) and default indexer to cid.contact everywhere (data storage, retrieval, sampled retrieval checks) - remove now-dead cidContactVerification metrics - filecoin-pin dependency temporarily pinned to a pkg.pr.new preview build pending a real npm release - update docs and pnpm-lock.yaml (regenerated with pnpm 9.15.2 to match CI)
|
@beck-8 started this. I know it's WIP. The title, PR description, should get updated. I created the PR so had a good place to comment and so had a durable link |
There was a problem hiding this comment.
Pull request overview
Retargets IPNI verification away from filecoinpin.contact to a single authoritative cid.contact check, and changes the gating signal from SP “advertised” to SP “synced” (Curio-confirmed). It also adds new “synced” tracking fields to the Deal lifecycle and updates docs/metrics to match.
Changes:
- Switch IPNI verification to
checkIpniIndexeragainsthttps://cid.contactacross data storage, retrieval, and sampled retrieval flows; remove the old cid.contact cross-check metrics. - Add
IpniStatus.SP_SYNCEDplusipniSyncedAt/ipniTimeToSyncMstracking (Postgres migration + Deal entity updates; partial ClickHouse mirror). - Update documentation and lockfile; temporarily pin
filecoin-pinto apkg.pr.newpreview build.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates dependency graph/lockfile; introduces pkg.pr.new tarball resolution for filecoin-pin. |
| apps/backend/package.json | Pins filecoin-pin dependency to https://pkg.pr.new/filecoin-pin@689. |
| apps/backend/src/ipni/ipni-verification.service.ts | Replaces waitForIpniProviderResults with checkIpniIndexer and defaults to cid.contact. |
| apps/backend/src/ipni/ipni-verification.service.spec.ts | Updates mocks/assertions for the new checkIpniIndexer integration. |
| apps/backend/src/deal-addons/strategies/ipni.types.ts | Extends SP piece-status schema/types with synced / syncedAt. |
| apps/backend/src/deal-addons/strategies/ipni.strategy.ts | Gates IPNI verification on SP synced, updates IPNI status progression, removes cid.contact cross-check flow. |
| apps/backend/src/deal-addons/strategies/ipni.strategy.spec.ts | Adds/updates tests for synced gating and SP_SYNCED handling. |
| apps/backend/src/retrieval/retrieval.service.ts | Aligns retrieval IPNI verification metric labels with cid.contact. |
| apps/backend/src/metrics-prometheus/metrics-prometheus.module.ts | Removes cidContactVerification metric provider. |
| apps/backend/src/metrics-prometheus/check-metrics.service.ts | Removes cid.contact cross-check counter/type surface; narrows indexer label to cid.contact. |
| apps/backend/src/database/types.ts | Adds IpniStatus.SP_SYNCED. |
| apps/backend/src/database/migrations/1787920828000-AddIpniSyncedFields.ts | Adds enum value + DB columns ipni_synced_at and ipni_time_to_sync_ms. |
| apps/backend/src/database/entities/deal.entity.ts | Adds ipniSyncedAt and ipniTimeToSyncMs columns to the Deal entity. |
| apps/backend/src/deal/deal.service.ts | Adds ClickHouse payload field ipni_synced_at. |
| apps/backend/src/clickhouse/clickhouse.schema.ts | Adds ClickHouse column ipni_synced_at (via ALTER). |
| docs/infra.md | Updates IPNI indexer documentation to cid.contact. |
| docs/environment-variables.md | Updates env var docs to reference cid.contact instead of filecoinpin.contact. |
| docs/checks/sampled-retrievals.md | Updates sampled retrieval docs to reference cid.contact. |
| docs/checks/retrievals.md | Updates retrieval docs to reference cid.contact. |
| docs/checks/events-and-metrics.md | Updates event/metric documentation for the single-indexer approach and new spSyncedToIpni. |
| docs/checks/data-storage.md | Updates Data Storage check flow: introduces sp_synced gate and removes cid.contact cross-check. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| filecoin-pin@https://pkg.pr.new/filecoin-pin@689: | ||
| resolution: {tarball: https://pkg.pr.new/filecoin-pin@689} | ||
| version: 1.3.1 | ||
| hasBin: true |
| "cors": "^2.8.6", | ||
| "cron": "4.4.0", | ||
| "filecoin-pin": "^1.2.0", | ||
| "filecoin-pin": "https://pkg.pr.new/filecoin-pin@689", |
…vertised Error/log text still said "piece retrieval" timeout, which was misleading once monitorPieceStatus started waiting for synced instead of advertised. Caught by Copilot review on PR #690.
| "@filoz/synapse-core": "0.7.1", | ||
| "@filoz/synapse-sdk": "1.1.1", |
There was a problem hiding this comment.
we need to update these as well
| } else if (finalStatus.synced) { | ||
| deal.ipniStatus = IpniStatus.SP_SYNCED; |
There was a problem hiding this comment.
if (finalStatus.synced && !ipniResult.rootCIDVerified && !result.skipped) we should log an error and emit a metric for the cid.contact and SP out of sync case.
From the tracking issue:
Handle the "Curio says synced but cid.contact confirmation failed" case explicitly: when filecoin-pin's polling surfaces that mismatch error/event, dealbot must log an error and emit a metric for it (not optional) — this is a real Curio/cid.contact disagreement, not routine unverified/timeout noise, and should be distinguishable in dashboards from ordinary ipniVerifyMs failures.
| const ipniVerifyStartTimestamp = deal.ipniSyncedAt ?? deal.ipniAdvertisedAt; | ||
| const ipniVerifyMs = ipniVerifyStartTimestamp | ||
| ? (calculateDuration(verificationEndTimestamp, "ipniVerify", ipniVerifyStartTimestamp) ?? ipniResult.durationMs) |
There was a problem hiding this comment.
this changes metrics meaning
OLD: ipniVerifyMs{indexer="filecoinpin.contact"} === verifiedAt - advertisedAt
NEW: ipniVerifyMs{indexer="cid.contact"} === verifiedAt - syncedAt
and I don't think that's what we want? ipniVerifyMs is supposed to measure time from telling IPNI indexer about a new advertisement, to actually responding with providers for a given CID, right?
Retargets dealbot's IPNI indexing check away from filecoinpin.contact (being retired). Not final yet — see the pkg.pr.new dependency note below before merging.
monitorPieceStatusnow gates onsynced(Curio-confirmed indexer sync) instead ofadvertised;advertisedis still logged but no longer returns earlyverify()into a single authoritative cid.contact checkIpniStatus.SP_SYNCEDplusipniSyncedAt/ipniTimeToSyncMstracking (Postgres migration, Deal entity). OnlyipniSyncedAtis mirrored to ClickHouse'sdata_storage_checkstable, matching how the other threeipni_time_to_*_msfields are Postgres-only todaycheckIpniIndexer(waswaitForIpniProviderResults) and default indexer to cid.contact everywhere (data storage, retrieval, sampled retrieval checks)cidContactVerificationmetricspnpm-lock.yaml(regenerated with pnpm 9.15.2 to match CI)Known blocker before merge:
filecoin-pinis temporarily pinned to a pkg.pr.new preview build (https://pkg.pr.new/filecoin-pin@689) pending a real npm release. The lockfile entry for that tarball URL has no integrity hash, and the URL itself tracks the PR's latest push rather than a fixed commit — flagged by Copilot review. Needs to move to a published version before this ships for real.