feat: peer id smart contract support #117

NikolasHaimerl · 2025-03-10T10:45:49Z

This PR implements support for resolving miner PeerIDs from both traditional on-chain sources and the new Curio smart contract. It addresses the issue of Curio storage providers having their Spark scores drop to zero due to using different PeerIDs for on-chain state and index provider advertisements.

Context

Closes subtask of Support smart-contract-based mapping of MinerID to IndexProviderPeerID roadmap#250
Related to Verify that spark-api eligible_deals include deals from Curio SPs spark-deal-observer#117 - Verify that spark-api eligible_deals include deals from Curio SPs
Implements the approach detailed in Support smart-contract-based mapping of MinerID to IndexProviderPeerID roadmap#250 (comment)

Changes

Refactors getMinerPeerId to query both sources concurrently using Promise.allSettled
Prioritizes the smart contract result, falling back to the RPC node result if necessary
Adds new getMinerPeerIdFromRpcNode function with the original implementation
Implements getMinerPeerIdFromSmartContract to query the Curio contract at 0x14183aD016Ddc83D638425D6328009aa390339Ce
Adds comprehensive tests for all resolution paths

deps.ts

bajtos

The implementation looks good at the high level, let's improve the details now.

lib/miner-info.js

lib/smart-contract-client.js

lib/miner-info.js

test/spark.js

bajtos · 2025-03-10T12:35:03Z

lib/miner-info.js

+  // Check contract result first
+  if (contractResult.status === 'fulfilled' && contractResult.value) {


What if the smart contract state query failed? Then we don't know whether the miner is setting a custom peer ID in the smart contract or whether it's ok to fall back to the MinerInfo peer ID.

I propose to fail the entire getMinerPeerId in case that any of the RPC API requests fails.

That should also simplify the implementation, as you can use Promise.all and don't have to deal with status === 'fulfilled'.

What do you think?

Are there no cases where a miner's peer ID is recorded on IPNI but not in the smart contract?

+1 on using Promise.all

Let me explain:

Every miner has a PeerID recorded in MinerInfo state. This PeerID is used for peer-to-peer communication between Filecoin nodes building a consensus about the blockchain ledger.

It happens that both Venus and Boost use this same PeerID for signing IPNI advertisements.

Venus and Boost don't have any record in the smart contract mapping.

Curio uses a different PeerID for p2p communication with other Filecoin nodes and a different one for signing IPNI advertisements. You can learn more about this topic in the Curio doc page I linked in one of my comments.

Curio miners will have one PeerID in MinerInfo state and another PeerID in the smart contract mapping. We want to use the value from the smart contract.

Are there no cases where a miner's peer ID is recorded on IPNI but not in the smart contract?

Yes, there are many such cases - all Venus and Boost miners.

Your question describes a different topic from what I meant in my comment, though.

I am talking about the situation when the RPC API call fails, and thus we don't know the answers to the questions "what's miner's PeerID in MinerInfo state" and "what's miner's PeerID in the smart contract mapping".

As I understand your code:

When the RPC API call succeeds, but no mapping is found, the function returns null.

When the RPC API fails, the function throws an error.

I am asking for a more robust handling of the second option.

An example case - can you encode it in a new test?

A miner is running Curio and has an entry in the smart contract mapping with PeerID ID1.

The miner has PeerID ID2 in the MinerInfo state.

On the Spark side, the RPC API call to read smart contract state fails, but the RPC API call to read MinerInfo state succeeds.

In this case, we must not return ID2 as the miner's PeerID, because that's not correct.

Thanks for the explaination. If a miner has peer ids on the smart contract as well as in MinerInfo, I don't think we can tell the difference between The call to the smart contract failed and There is no peer id on the smart contract and the call failed.

In the first case, I expect you get back the "empty string" response.

In the second case, I expect the call to throw a JavaScript-level error.

a real smart contract client connected to Glif but with no authorization header set

This is just a quick way to simulate an outage of the RPC API service. In practice, we will be seeing different errors, but the mechanism and response format should be the same.

I am not 100% sure I understood your concerns with the current design. I added some tests in 5b63a20 to illustrate the expected behaviour of my approach.

Using a real smart contract client connected to Glif but with no authorization header set

This is unfortunately not enough to trigger an error response. Here is an alternative you can use for a one-time verification of how Ethers.js handle transport-level errors:

const fetchRequest = new ethers.FetchRequest('https://api.node.glif.io/rpc/vx')

I am not 100% sure I understood your concerns with the current design.

Here is the code demonstrating the problem - I placed the code to debug.js and executed zinnia run debug.js:

import { getMinerPeerId } from './lib/miner-info.js' const peerId = await getMinerPeerId('f03303347', { smartContract: { getPeerData: async () => { throw new Error('RPC CALL ERROR') } } }) console.log('PeerID: ', peerId) const realPeerId = await getMinerPeerId('f03303347') console.log('Real PeerID:', realPeerId)

Output - notice that the first getMinerPeerId invocation returns a wrong PeerID value.

Using PeerID from FilecoinMinerInfo. PeerID: 12D3KooWCtiN7tAjeLKL4mashteXdH4htUrzWu8bWN9kDU3qbKjQ Using PeerID from the smart contract. Real PeerID: 12D3KooWJ91c6xQshrNe7QAXPFAaeRrHWq2UrgXGPf8UmMZMwyZ5

Expected behaviour: The first call of getMinerPeerId(), where the RPC API call querying the smart contract fails, must not return an incorrect PeerID value. I think it should throw an error, but I am open to discussing other ways to achieve that.

We suggested Promise.all() because it gives us the behaviour I asked for - if any of the RPC API calls fails, then the entire getMinerPeerId() call fails.

const [minerInfoResult, contractResult] = await Promise.all([ getMinerPeerIdFromFilecoinMinerInfo(minerId, { maxAttempts, rpcFn }), getMinerPeerIdFromSmartContract(minerId, { smartContract }) ]) if (contractResult) { console.log('Using PeerID from the smart contract.') return contractResult } if (minerInfoResult) { console.log('Using PeerID from FilecoinMinerInfo.') return minerInfoResult }

I suppose that's suboptimal in case the smart-contract query succeeds (and finds a PeerID value) and the MinerInfo query fails (which does not matter because we will use the value found in the smart contract anyways). Personally, I think this is such a minor edge case that it's not worth fixing, considering the additional complexity required.

However, if you prefer to keep your current Promise.allSettled version, then I am fine with that.

Just make sure the test case I shown above is handled correctly.

Co-authored-by: Miroslav Bajtoš <oss@bajtos.net>

pyropy

Good job @NikolasHaimerl 👏🏻

lib/smart-contract-client.js

pyropy · 2025-03-11T08:39:13Z

lib/miner-info.js

+  // Check contract result first
+  if (contractResult.status === 'fulfilled' && contractResult.value) {


+1 on using Promise.all

deps.ts

bajtos

You are making great progress!

The biggest item to resolve is handling of RPC API errors + building a better understanding what response to expect for miners with no PeerID mapping set in the smart contract.

lib/constants.js

lib/smart-contract-client.js

test/smart-contract-client.test.js

bajtos · 2025-03-11T15:37:35Z

lib/miner-info.js

+  // Check contract result first
+  if (contractResult.status === 'fulfilled' && contractResult.value) {


If a miner has peer ids on the smart contract as well as in MinerInfo, I don't think we can tell the difference between The call to the smart contract failed and There is no peer id on the smart contract and the call failed.

I don't know the answer, but what you described does not match my mental model of how these things work.

Here is what you can do to verify our hypotheses:

Using a real smart contract client connected to Glif, call await contractClient.getPeerData(3501639). (This is a client ID; I don't expect it to have a PeerID mapping in the smart contract.) What response do you get back?

Using a real smart contract client connected to Glif but with no authorization header set, call await contractClient.getPeerData(3303347). (This is a miner ID from your integration test that has a PeerID mapping entry.) What happens?

bajtos · 2025-03-11T15:38:39Z

lib/miner-info.js

+  // Check contract result first
+  if (contractResult.status === 'fulfilled' && contractResult.value) {


In the first case, I expect you get back the "empty string" response.

In the second case, I expect the call to throw a JavaScript-level error.

bajtos · 2025-03-11T15:39:46Z

lib/miner-info.js

+  // Check contract result first
+  if (contractResult.status === 'fulfilled' && contractResult.value) {


a real smart contract client connected to Glif but with no authorization header set

This is just a quick way to simulate an outage of the RPC API service. In practice, we will be seeing different errors, but the mechanism and response format should be the same.

lib/smart-contract-client.js

Co-authored-by: Miroslav Bajtoš <oss@bajtos.net>

lib/miner-info.js

bajtos

The updated implementation (lib files) looks pretty good to me now 👍🏻

Now we need to address this discussion about handling RPC API errors:
#117 (comment)

I'll review the tests updated since my previous review after that discussion is resolved.

bajtos · 2025-03-12T14:20:48Z

test/smart-contract-client.test.js

+import { getMinerPeerId } from '../lib/miner-info.js'
+import { test } from 'zinnia:test'
+
+const mockPeerIdResponse = {


The name mockPeerIdResponse is very similar to mockEmptyPeerIdResponse, I had troubles spotting the difference when reading the tests below. Let's use more distinct names please.

For example:

Suggested change

const mockPeerIdResponse = {

const mockValidPeerIdResponse = {

Also, the prefix "mock" is not suitable here - mocks are objects/functions with active behaviour. See https://martinfowler.com/bliki/TestDouble.html

Can we simply drop the mock prefix and use the names validPeerIdResponse and emptyPeerIdResponse instead?

Addressed in 9a8a972

Co-authored-by: Miroslav Bajtoš <oss@bajtos.net>

NikolasHaimerl added 7 commits March 10, 2025 10:24

add support for smart contracts

6bb3726

fmt

68359b6

reanbled tests

62be1e6

add client file

2bbbeca

fmt

9697f26

comments

1262327

comments

544d4c4

bajtos reviewed Mar 10, 2025

View reviewed changes

deps.ts Outdated Show resolved Hide resolved

bajtos requested changes Mar 10, 2025

View reviewed changes

NikolasHaimerl marked this pull request as ready for review March 10, 2025 15:25

NikolasHaimerl requested a review from juliangruber as a code owner March 10, 2025 15:25

NikolasHaimerl requested a review from pyropy March 10, 2025 15:25

NikolasHaimerl self-assigned this Mar 10, 2025

NikolasHaimerl and others added 9 commits March 10, 2025 17:22

Update lib/miner-info.js

87e6ec0

Co-authored-by: Miroslav Bajtoš <oss@bajtos.net>

Update lib/miner-info.js

e16ae0c

Co-authored-by: Miroslav Bajtoš <oss@bajtos.net>

resolve PR threads

3a60176

merged with main

c3894a0

resolve PR threads

66a2d4a

auth

3eb9eff

fix ethers import

3e827ce

fix cid

1b93104

fix cid

e95b9e7

NikolasHaimerl requested a review from bajtos March 11, 2025 05:24

pyropy reviewed Mar 11, 2025

View reviewed changes

NikolasHaimerl mentioned this pull request Mar 11, 2025

Use human-readable ABI format in smart contract client #118

Open

move constant

c26916a

bajtos requested changes Mar 11, 2025

View reviewed changes

NikolasHaimerl and others added 3 commits March 11, 2025 17:59

Update lib/smart-contract-client.js

08dacb9

Co-authored-by: Miroslav Bajtoš <oss@bajtos.net>

address pr threads

5b63a20

address pr threads

baafc99

NikolasHaimerl mentioned this pull request Mar 12, 2025

Optimize RPC API Calls to Reduce Glif Bill #120

Open

NikolasHaimerl requested a review from bajtos March 12, 2025 10:24

bajtos reviewed Mar 12, 2025

View reviewed changes

lib/miner-info.js Outdated Show resolved Hide resolved

bajtos requested changes Mar 12, 2025

View reviewed changes

NikolasHaimerl and others added 2 commits March 12, 2025 16:32

Update lib/miner-info.js

ec2a53b

Co-authored-by: Miroslav Bajtoš <oss@bajtos.net>

fmt

9a8a972

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: peer id smart contract support #117

feat: peer id smart contract support #117

NikolasHaimerl commented Mar 10, 2025 •

edited

Loading

bajtos left a comment

bajtos Mar 10, 2025

NikolasHaimerl Mar 10, 2025

pyropy Mar 11, 2025

bajtos Mar 11, 2025 •

edited

Loading

NikolasHaimerl Mar 11, 2025 •

edited

Loading

bajtos Mar 11, 2025

bajtos Mar 11, 2025

NikolasHaimerl Mar 12, 2025

bajtos Mar 12, 2025

NikolasHaimerl Mar 12, 2025 •

edited

Loading

pyropy left a comment

pyropy Mar 11, 2025

bajtos left a comment

bajtos Mar 11, 2025

bajtos Mar 11, 2025

bajtos Mar 11, 2025

bajtos left a comment

bajtos Mar 12, 2025

NikolasHaimerl Mar 12, 2025

		// Check contract result first
		if (contractResult.status === 'fulfilled' && contractResult.value) {

	const mockPeerIdResponse = {
	const mockValidPeerIdResponse = {

feat: peer id smart contract support #117

Are you sure you want to change the base?

feat: peer id smart contract support #117

Conversation

NikolasHaimerl commented Mar 10, 2025 • edited Loading

Context

Changes

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

bajtos Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

NikolasHaimerl Mar 11, 2025 • edited Loading

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

NikolasHaimerl Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

pyropy 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

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

NikolasHaimerl commented Mar 10, 2025 •

edited

Loading

bajtos Mar 11, 2025 •

edited

Loading

NikolasHaimerl Mar 11, 2025 •

edited

Loading

NikolasHaimerl Mar 12, 2025 •

edited

Loading