Skip to content

Conversation

@SgtPooki
Copy link
Collaborator

@SgtPooki SgtPooki commented Nov 11, 2025

previously, validateIpniAdvertisement did not validate that the ipni advertisement existed for
the given provider. Now, we validate that the response includes the expected provider,
and allow consumers to pass extra providers (to support future multiple provider uploads)

some code inspired by #140

previously, validateIpniAdvertisement did not validate that the ipni advertisement existed for
the given provider. Now, we validate that the response includes the expected provider,
and allow consumers to pass extra providers (to support future multiple provider uploads)
@FilOzzy FilOzzy added team/filecoin-pin "Filecoin Pin" project is a stakeholder for this work. team/fs-wg FOC working group is a stakeholder for this work, and thus wants to track it on their project board. labels Nov 11, 2025
@FilOzzy FilOzzy added this to FS Nov 11, 2025
@github-project-automation github-project-automation bot moved this to 📌 Triage in FS Nov 11, 2025
@SgtPooki SgtPooki requested a review from Copilot November 11, 2025 21:48
Copilot finished reviewing on behalf of SgtPooki November 11, 2025 21:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances IPNI (InterPlanetary Network Indexer) advertisement validation to verify that specific storage providers have announced content, addressing a gap where the validation previously only checked if any provider advertised the CID. The implementation now validates that expected providers' multiaddrs are present in IPNI responses, with fallback to generic validation when provider information cannot be derived.

Key changes:

  • Added provider-specific validation with multiaddr matching against expected providers
  • Automatic provider detection from upload context with support for explicit provider specification
  • Enhanced error messages showing which provider multiaddrs are missing from advertisements

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/core/utils/validate-ipni-advertisement.ts Implements provider-specific IPNI validation with multiaddr conversion and matching logic
src/test/unit/validate-ipni-advertisement.test.ts Adds comprehensive test coverage for provider validation scenarios and edge cases
src/core/upload/index.ts Integrates provider validation by passing current provider info to IPNI validation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@BigLep BigLep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I need to look again with a fresh head. This generally feels like a lot of code has gotten added and I'm wondering if there's ways to simplify (but not able to think clearly currently)

@BigLep BigLep moved this from 📌 Triage to 🔎 Awaiting review in FS Nov 12, 2025
@SgtPooki
Copy link
Collaborator Author

@BigLep

I think I need to look again with a fresh head. This generally feels like a lot of code has gotten added and I'm wondering if there's ways to simplify (but not able to think clearly currently)

I simplified code a bit and inlined some things.. still +500 lines

Copy link
Collaborator Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self review.. much better now..

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@BigLep BigLep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most comments are around terminology. We're validating the advertisement flow (not the advertisement itself). We do this by inspecting the ProviderResults/provider records.

I also was confused by why we have the ability for passing extra multiaddrs to validate.

Comment on lines +248 to +253
// Note: If expectedProviders is explicitly [], we respect that (no provider expectations)
if (expectedProviders != null) {
validationOptions.expectedProviders = expectedProviders
} else if (synapseService.providerInfo != null) {
validationOptions.expectedProviders = [synapseService.providerInfo]
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BigLep most folks will call through this method, which will set expectedProviders to the correct thing (and to the multi-providers for multi-provider uploads once synapse-sdk is updated)

@SgtPooki
Copy link
Collaborator Author

FYI: CI was failing when I migrated to set operations because we only had tsconfig set to ES2022:

Error: src/core/utils/validate-ipni-advertisement.ts(195,60): error TS2550: Property 'intersection' does not exist on type 'Set<string>'. Do you need to change your target library? Try changing the 'lib' compiler option to 'esnext' or later.
Error: src/core/utils/validate-ipni-advertisement.ts(200,63): error TS2550: Property 'difference' does not exist on type 'Set<string>'. Do you need to change your target library? Try changing the 'lib' compiler option to 'esnext' or later.

NodeJS Active LTS is 24 as of Oct 28 2025, and nodejs 22+ and all major browsers support Set.intersection and other set operations, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set/intersection

We are already testing on CI for lts/* (24) and current (25), see https://nodejs.org/en/about/previous-releases#looking-for-the-latest-release-of-a-version-branch

I updated tsconfig to add ESNext.Collection, not wanting to take on engines and other changes in this PR

@SgtPooki SgtPooki changed the title fix: validateIpniAdvertisement checks provider fix: IPNI validation confirms provider in indexer response Nov 13, 2025
Copy link
Member

@BigLep BigLep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments. Key thing is to get build passing. Approved otherwise.

@github-project-automation github-project-automation bot moved this from 🔎 Awaiting review to ✔️ Approved by reviewer in FS Nov 13, 2025
@SgtPooki SgtPooki merged commit 71f157d into master Nov 13, 2025
12 checks passed
@SgtPooki SgtPooki deleted the 217-give-validateipniadvertisement-the-ability-to-confirm-a-given-sp-is-showing-up-as-an-ipni-provider branch November 13, 2025 02:38
@github-project-automation github-project-automation bot moved this from ✔️ Approved by reviewer to 🎉 Done in FS Nov 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team/filecoin-pin "Filecoin Pin" project is a stakeholder for this work. team/fs-wg FOC working group is a stakeholder for this work, and thus wants to track it on their project board.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Give validateIPNIAdvertisement the ability to confirm a given SP is showing up as an IPNI provider

4 participants