feat: use IPNI advertisements from the miner only

[bajtos]

1. Resolve minerId to miner's PeerId, which is the same value as Provider.ID in the IPNI records

2. Ignore IPNI records advertised by different miners

3. Report minerId and providerId as the new measurement fields

Links:

• Link retrievals to miners space-meridian/roadmap#65

[juliangruber]

wouldn't it be safer to push the function calls into an array, and then assert this function was called at all? This is the advice you give me with all these mocks I believe

[bajtos]

In this particular test, if my stubbed function isn't called, the retrieval check fails because Filecoin's RPC API call will reject the fake miner id.

The purpose of this test is not to verify how Spark works under the hood (and whether it calls all other functions we think it should call) but whether it can find the provider and retrieve the data for the CID served by our Frisbii instance.

However, if you think it's worth asserting that both functions were called, then I am fine with adding that. Let me know!

[juliangruber]

This was also my understanding when I wrote tests like this, but through your reviews I understood that it's clearer and safer to assert the function calls. If in the future a condition is introduced which skips lookupMinerPeerId(), then this test will still pass.

However,iIn my previous review I missed that this was an integration test and not a unit test. Please choose the testing method that you see fits best.

[bajtos]

push the function calls into an array, and then assert this function was called

added in 0a48986

[juliangruber]

I don't understand the need for this custom property .reported here.

If we remove the console.error(err) above, then we can let the outer catch handler log it. Also, the outer catch handler can call #activity.onError() itself.

I think the only custom error handling we need here is when err.name === 'FilecoinRpcError', and in that case we can simply add a new log event. I'm proposing to refactor this catch handler like this:

Suggested change

	console.error(err)
	// There are three common error cases:
	// 1. We are offline
	// 2. The JSON RPC provider is down
	// 3. JSON RPC errors like when Miner ID is not a known actor
	// There isn't much we can do in the first two cases. We can notify the user that we are not
	// performing any jobs and wait until the problem is resolved.
	// The third case should not happen unless we made a mistake, so we want to learn about it
	if (err.name === 'FilecoinRpcError') {
	// TODO: report the error to Sentry
	console.error('The error printed above was not expected, please report it on GitHub:')
	console.error('https://github.com/filecoin-station/spark/issues/new')
	} else {
	this.#activity.onError()
	}
	err.reported = true
	// Abort the check, no measurement should be recorded
	throw err
	// There are three common error cases:
	// 1. We are offline
	// 2. The JSON RPC provider is down
	// 3. JSON RPC errors like when Miner ID is not a known actor
	// There isn't much we can do in the first two cases. We can notify the user that we are not
	// performing any jobs and wait until the problem is resolved.
	// The third case should not happen unless we made a mistake, so we want to learn about it
	if (err.name === 'FilecoinRpcError') {
	// TODO: report the error to Sentry
	console.error('Unexpected error, please report it on GitHub:')
	console.error('https://github.com/filecoin-station/spark/issues/new')
	}
	// Abort the check, no measurement should be recorded
	throw err

[bajtos]

Let's discuss.

In my proposed implementation:

• FilecoinRpcError does not trigger this.#activity.onError(), therefore Spark stays in "online" mode
• We print the call to report the issue after the error stack.

In your proposal:

• FilecoinRpcError triggers this.#activity.onError() and puts Spark to offline mode.
• The call to report the issue is printed before the error to report. (I guess that's fine, we just need to tweak the message.)

Here is the important question we need to answer: How should we handle the case when we cannot get a miner's Peer ID because Filecoin RPC returns an error?

• This can be either because we are calling the RPC API incorrectly or because the miner ID is not a valid one. (I suppose we could also receive this error if the RPC API server has an internal problem; it can process the JSON-RPC part, but the called RPC method fails.)
• Do we want to indicate offline mode? The next retrieval check will likely be able fetch the round details from spark-api, which means Spark goes back to online mode, only to go offline again when the JSON RPC call fails.

[juliangruber]

Ah understood! I thought the purpose of err.reported was to prevent double log lines, now I see it's also to prevent reporting it to the activity state handler.

Could we be more explicit about this?

1. Rename to error.setActivityToOffline = false
2. Let the outer error handler handle logging, to be more consistent with other errors

[bajtos]

See db3b2ae

[juliangruber]

Ah understood! I thought the purpose of err.reported was to prevent double log lines, now I see it's also to prevent reporting it to the activity state handler.

Could we be more explicit about this?

1. Rename to error.setActivityToOffline = false
2. Let the outer error handler handle logging, to be more consistent with other errors