Add alternative provider retrieval check #132

pyropy · 2025-04-09T15:32:37Z

This pull requests includes a new check that measures retrievability for alternative provider.

Alternative provider retrieval check is performed ONLY when there is no valid advertisement found for the storage provider (miner) we are checking.

If there is no no valid advertisement found we are going to pick one random provider at random that advertises the CID we want to retrieve. Note that providers are ranked by proprity based on their attributes (context ID they advertise, protocol they use) so some providers may have higher chance of getting picked. Pseudo-random number generator is used to pick the retrieval provider from the given list.

In case there is a valid advertisement, standard retrieval result status will be used to calculate alternative-provider retrieval score (see CheckerNetwork/spark-evaluate#518).

Changelog

Introduced a new Provider type and updated the queryTheIndex function to return a list of alternative providers in case no valid advertisement are found.
Added a new method checkRetrievalFromAlternativeProvider in the Spark class to perform alternative provider retrieval check on a random provider when no valid advertisement is found.
Added helper functions pickRandomProvider, to select a provider based on their priority.
Added new tests for the checkRetrievalFromAlternativeProvider method and updated existing tests to include the contextId field in the provider object.
Introduced new prando package to generate pseudo-random numbers.

Closes #130
Relates to:

Copilot

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

lib/spark.js:450

Consider handling the case where filteredProviders is empty so that pickRandomWeightedItem receives a non-empty list, preventing potential errors.

const filteredProviders = providers.filter((provider) => provider.protocol !== 'bitswap')

Copilot

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

lib/spark.js:478

The prefix check for contextId is case-sensitive but the test data uses a different casing (e.g., 'ghA=='). Consider normalizing the case or using a consistent prefix for proper weight assignment.

if (provider.contextId.startsWith('gHa')) weight += 1

lib/ipni-client.js:56

The condition is redundant because the loop already continues for non-matching provider IDs. Removing this check could simplify the code.

if (p.Provider.ID === providerId) {

bajtos · 2025-04-10T12:53:14Z

lib/ipni-client.js

+ *  provider?: Provider;
+ *  providers?: Provider[];


I find it confusing to have two properties called provider and providers, I cannot tell what's the difference.

How about calling the new property alternativeProviders or altProviders?

bajtos

Great start!

lib/spark.js

bajtos · 2025-04-10T13:01:35Z

lib/spark.js

+    // we will try to perform network wide retrieval from other providers
+    if (noValidAdvertisement) {
+      console.log('No valid advertisement found. Performing network-wide retrieval check...')
+      return await this.testNetworkRetrieval(providers, retrieval.cid, stats)


The more I work in this codebase, the more I regret the design pattern using a mutable stats parameter passed around. I would like to eventually refactor the code so that every function returns a stats object with the relevant subset of fields.

Would you mind applying that design for this new function?

stats.networkRetrieval = await this.testNetworkRetrieval(providers, retrieval.cid, stats)

Also, let's find a different name for stats.networkRetrieval, e.g. stats.altProvider or stats.altProviderCheck

I have changed it to alternativeProviderCheck.

lib/spark.js

bajtos · 2025-04-10T13:17:02Z

lib/spark.js

+/**
+ * Picks a random item from an array based on their weight. The higher the weight, the higher the chance of being selected.
+ *
+ * @template T The type of the item in the list.
+ * @param {Array<{weight: number}>} items The list of items, where each item has a `weight`property.
+ * @returns {T} The randomly selected item based on its weight.
+ *
+ */
+function pickRandomWeightedItem(items) {
+  const totalWeight = items.reduce((acc, item) => acc + item.weight, 0)
+  let random = Math.random() * totalWeight
+
+  // Iterate over items, subtracting the item's weight from the random number
+  // until we find the item where the random number is less than the item's weight
+  for (let i = 0; i < items.length; i++) {
+    random -= items[i].weight
+    if (random <= 0) {
+      return items[i]
+    }
+  }
+}


I find this very problematic:

(1)
When using Math.random(), each checker node will pick a different provider. As a result, we cannot create committees to find an honest majority in the data reported by the network.

Please use a DRAND beacon to get deterministic randomness so that all nodes pick the same alternative provider to check.

See how we are using DRAND beacon in other parts of this codebase. The important part is to use the DRAND beacon tied to the time when the Spark round started.

(2)
I am not sure if it's a good idea to do a random selection with weights. Shouldn't we always prefer Filecoin SPs serving HTTP retrievals over everybody else?

I would replace weights with the following algorithm:

Is there a provider with ContextID starting with ghsA?

Yes:
Does any of those providers support HTTP?

Yes -> pick one of those HTTP providers at random

No -> pick any of the Graphsync providers at random

No:
Does any of the providers support HTTP?

Yes -> pick one of those HTTP providers at random

No -> pick any of the Graphsync providers at random

Alternatively, keep using weight, but pick randomly only from the providers with the same highest weight.

Let's discuss what would be the best approach here!

Thanks for your input!

(1)

I wasn't aware that we also need to form committees for the second (alternative) measurement as commitees were formed to evaluate measurement for a single storage provider. I therefore assumed that we can pick any provider at random as this measurement should not affect existing RSR.

In case we're going to use committees to evaluate this measurement I agree with using DRAND.

(2)

Alternative solution to your proposal would be to adjust the weights but I think the algorithm you proposed may be easier to understand. I don't mind changing the algorithm.

Would be nice if we could use committees

I agree with the more explicit algorithm, it's easier to reason about

I added both a pseudo-RNG and more a explicit algorithm.

bajtos · 2025-04-10T13:37:48Z

lib/spark.js

+  return {
+    statusCode: null,
+    timeout: false,
+    endAt: null,
+    carTooLarge: false,
  }


Please include the selected providerId in the new stats object, see CheckerNetwork/roadmap#254 (comment)

This is a bit problematic as for providerId we need to make sure that provider is a Filecoin storage provider and that we have their miner Id.

Are you aware of some way to reverse Peer ID to Miner ID?

Let's use the retrieval provider peer ID found in the IPNI response instead of the Filecoin miner ID.

We are already doing that for the "regular" retrieval checks, see here:

spark-checker/lib/spark.js

Lines 54 to 55 in 9c29967

console.log(`Found peer id: ${peerId}`)

stats.providerId = peerId

Co-authored-by: Miroslav Bajtoš <[email protected]>

lib/ipni-client.js

juliangruber · 2025-04-11T10:22:05Z

lib/ipni-client.js

          }
-        }
+      }


It's confusing to me that in case of HTTP we return immediately, but in case of graph sync we don't. I don't see a reason for this behavior. Could we also directly return in the case of graphsync?

This was the way we use to do it even before introduction of "alternative providers" to the response. As I understood it: we're iterating trough list of providers and in case we find record serving content via graphsync first we'll return it only if there's no HTTP alternative. In case there's a record serving content via HTTP we return it immediately.

lib/spark.js

juliangruber · 2025-04-11T10:28:08Z

lib/spark.js

+/**
+ * Picks a random item from an array based on their weight. The higher the weight, the higher the chance of being selected.
+ *
+ * @template T The type of the item in the list.
+ * @param {Array<{weight: number}>} items The list of items, where each item has a `weight`property.
+ * @returns {T} The randomly selected item based on its weight.
+ *
+ */
+function pickRandomWeightedItem(items) {
+  const totalWeight = items.reduce((acc, item) => acc + item.weight, 0)
+  let random = Math.random() * totalWeight
+
+  // Iterate over items, subtracting the item's weight from the random number
+  // until we find the item where the random number is less than the item's weight
+  for (let i = 0; i < items.length; i++) {
+    random -= items[i].weight
+    if (random <= 0) {
+      return items[i]
+    }
+  }
+}


Would be nice if we could use committees

I agree with the more explicit algorithm, it's easier to reason about

lib/spark.js

juliangruber · 2025-04-11T10:30:20Z

lib/spark.js

+    }
+
+    stats.networkRetrieval = newNetworkRetrievalStats()
+    const randomProvider = pickRandomProvider(providers)


I'm surprised by this strategy. I might be out of sync with the current product plan, but wasn't it the goal to see if any provider can fetch the data? Here we're marking a failure if the 2nd provider we try fails, but don't try the other ones.

I've posted my opinions on that in this comment and I'm happy to discuss more!

Other motivation behind this was not to put too much pressure on the network serving content and on the checkers. I'm afraid of the worst case situation where we'd have to check every provider serving content.

Co-authored-by: Julian Gruber <[email protected]>

pyropy · 2025-04-11T10:38:29Z

Converting to draft as there's some major refactoring that needs to take place.

…n-station/spark into add/network-wide-retrieval-check

pyropy · 2025-04-15T16:24:42Z

deps.ts

@@ -34,3 +34,6 @@ export {
 export { assertOkResponse } from 'https://cdn.skypack.dev/[email protected]/?dts'
 import pRetry from 'https://cdn.skypack.dev/[email protected]/?dts'
 export { pRetry }
+
+import Prando from 'https://cdn.jsdelivr.net/npm/[email protected]/+esm'
+export { Prando }


I have opted for using package instead of the custom implementation for the pRNG. There's lack of good packages for pRNG so I have settled in the end for Prando. I also wanted to use Deno's random package but from what I realize they have added it to newer versions of the std package which we don't use yet.

This may be a good thing to update in the future.

pyropy · 2025-04-15T16:27:01Z

lib/tasker.js

   */
  async next() {
    await this.#updateCurrentRound()
-    return this.#remainingRoundTasks.pop()
+    const retrievalTask = this.#remainingRoundTasks.pop()
+    return { retrievalTask, randomness: this.#randomness }


We somehow need to export the round randomness so I have opted for returning object with randomness attribute from the next function.

Maybe adding the randomness property to the retrieval task wouldn't be a bad thing either.

pyropy · 2025-04-15T16:28:34Z

lib/spark.js

+ * @param {number} randomness
+ * @returns {Provider | undefined}
+ */
+export function pickRandomProvider(providers, randomness) {


pickRandomProvider now picks random provider based on the priority rather then weight and generated pseudo-random number.

Copilot

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

manual-check.js

lib/spark.js

Co-authored-by: Copilot <[email protected]>

pyropy added 6 commits April 9, 2025 13:06

Add network wide retrieval check

aedec80

Use status code instead of boolean retrieval flag

233cc1f

Simplify name for network wide measurements

83e7f31

Refactor code for picking random provider

afe30dd

Add network retrieval protocol field

23ee203

Add basic test for testing network retrieval

4bc1076

pyropy self-assigned this Apr 9, 2025

pyropy requested review from bajtos and juliangruber as code owners April 9, 2025 15:32

github-project-automation bot added this to Space Meridian Apr 9, 2025

pyropy marked this pull request as draft April 9, 2025 15:35

pyropy added 4 commits April 9, 2025 19:03

Refactor function for picking random providers

63424ff

Only return providers in case of no valid advert

8a94f4e

Convert network stats to object inside stats obj

c4350b6

Format testNetworkRetrieval func

edfdef1

This was referenced Apr 10, 2025

Add alternative provider retrieval measurement CheckerNetwork/spark-api#571

Open

Evaluate alternative provider measurement CheckerNetwork/spark-evaluate#518

Open

Refactor queryTheIndex function

dbf0fd7

pyropy requested a review from Copilot April 10, 2025 10:19

pyropy changed the title ~~WIP: Add network wide retrieval check~~ Add network wide retrieval check Apr 10, 2025

Copilot AI reviewed Apr 10, 2025

View reviewed changes

Handle case when no random provider is picked

d33f276

pyropy requested a review from Copilot April 10, 2025 10:23

Copilot AI reviewed Apr 10, 2025

View reviewed changes

Test function for picking random providers

97bee91

pyropy marked this pull request as ready for review April 10, 2025 10:39

pyropy requested a review from NikolasHaimerl April 10, 2025 10:39

bajtos reviewed Apr 10, 2025

View reviewed changes

bajtos requested changes Apr 10, 2025

View reviewed changes

bajtos reviewed Apr 10, 2025

View reviewed changes

pyropy and others added 6 commits April 11, 2025 11:53

Rename network retrieval to alternative provider check

4b6d0bc

Co-authored-by: Miroslav Bajtoš <[email protected]>

Update logging to reflect metric name change

97fcc28

Co-authored-by: Miroslav Bajtoš <[email protected]>

Update logging to reflect metric name change

5121a49

Co-authored-by: Miroslav Bajtoš <[email protected]>

Rename providers field to alternativeProviders

4065784

Rename testNetworkRetrieval to checkRetrievalFromAlternativeProvider

74f06e9

Return retrieval stats from checkRetrievalFromAlternativeProvider

ea8cce4

juliangruber requested changes Apr 11, 2025

View reviewed changes

pyropy and others added 2 commits April 11, 2025 12:36

Update lib/spark.js

f9afe34

Co-authored-by: Julian Gruber <[email protected]>

Update lib/spark.js

9959b50

Co-authored-by: Julian Gruber <[email protected]>

pyropy marked this pull request as draft April 11, 2025 10:38

pyropy added 4 commits April 11, 2025 13:58

Rename functions to match new metric name

a2da050

Merge branch 'add/network-wide-retrieval-check' of github.com:filecoi…

9759d80

…n-station/spark into add/network-wide-retrieval-check

Pick alternative provider using supplied randomness

820e8a3

Replace custom rng implementation with Prando

5b13287

pyropy commented Apr 15, 2025

View reviewed changes

pyropy marked this pull request as ready for review April 15, 2025 16:31

pyropy changed the title ~~Add network wide retrieval check~~ Add alternative provider retrieval check Apr 15, 2025

pyropy requested review from bajtos, juliangruber and Copilot April 15, 2025 17:27

Copilot AI reviewed Apr 15, 2025

View reviewed changes

manual-check.js Show resolved Hide resolved

lib/spark.js Outdated Show resolved Hide resolved

Fix typos

3c14f84

Co-authored-by: Copilot <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add alternative provider retrieval check #132

Add alternative provider retrieval check #132

pyropy commented Apr 9, 2025 •

edited

Loading

Copilot AI left a comment

Copilot AI left a comment

bajtos Apr 10, 2025

bajtos left a comment

bajtos Apr 10, 2025

pyropy Apr 15, 2025

bajtos Apr 10, 2025

pyropy Apr 10, 2025

juliangruber Apr 11, 2025

pyropy Apr 15, 2025

bajtos Apr 10, 2025

pyropy Apr 11, 2025

bajtos Apr 16, 2025

juliangruber Apr 11, 2025

pyropy Apr 11, 2025

juliangruber Apr 11, 2025

juliangruber Apr 11, 2025

pyropy Apr 11, 2025

pyropy commented Apr 11, 2025

pyropy Apr 15, 2025

pyropy Apr 15, 2025

pyropy Apr 15, 2025

Copilot AI left a comment

	console.log(`Found peer id: ${peerId}`)
	stats.providerId = peerId

Add alternative provider retrieval check #132

Are you sure you want to change the base?

Add alternative provider retrieval check #132

Conversation

pyropy commented Apr 9, 2025 • edited Loading

Changelog

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyropy commented Apr 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

pyropy commented Apr 9, 2025 •

edited

Loading