Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd: allow to set the retrieval peerID, not just the retrieval multiaddr #430

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MichaelMure
Copy link

The config would allow us to specify a data retrieval multiaddr, but that is actually not enough as the default peerID (aka, the index-provider itself) is still used in the advertisements.

This PR add the necessary to also override the default peerID, which makes advertisements works as expected.

// WithRetrievalPeerID sets the peerID of where to get the content corresponding to an indexing advertisement.
// If unspecified, the libp2p host peer ID are used.
// See: WithHost
func WithRetrievalPeerID(peerID string) Option {
Copy link
Member

Choose a reason for hiding this comment

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

This is bound to provider libp2p host, since the identity is needed for signing of advertisements.

Have you considered setting the engine host option instead?

Copy link
Author

Choose a reason for hiding this comment

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

You mean just using WithHost and set the full retrieval host? Wouldn't that cause problems?

Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't. The identity of a provider is tied to what it uses to sign advertisements and where chain is retrievable from.
The reason for RetrievalAddrs option existing is that a libp2p node may have any public addr through DNS for example, that may not directly be known by it. This is similar to setting public hostname in a traditional HTTP server.

But the peer ID (or identity by extension) must be known to the host itself and must match the ID other peers expect to encounter when fetching ad chain from it.

Copy link

@EnchanterIO EnchanterIO Feb 14, 2024

Choose a reason for hiding this comment

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

For a bite more ctx, we have this PR deployed and it generates the following advertisements:

CID:          baguqeera7miiexnupzmlrusq2s5u2xd4ngqrsqormpjdtl6hkq73w2xqtkca
...
Signature: ✅ valid
Signed by: advertisement publisher

Copy link
Member

Choose a reason for hiding this comment

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

@EnchanterIO What other options are set in the engine that is deployed? I suspect the reason that works is that the peer ID set via option introduced by this PR matches the host identity?

Choose a reason for hiding this comment

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

It does not match the identity, contrary, the problem we try to address with this PR is that if retrievalAddr is not empty, the host id (identity of index provider) is inserted into adv - but we want our separate p2p-server bitswap provider peer id coming from this option to be inserted into adv instead.

Copy link
Member

Choose a reason for hiding this comment

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

Got it thank you

In IPNI terminology they are referred to as "Provider" and "Publisher". Provider provides the actual downloadable content , and Publisher tells the network of what is available for download from where i.e. expose the advertisement chain.

IIUC what you'd like to be able to do is to use the engine as a publisher for providers that may reside elsewhere. Right?

Related note: I recommend looking at Extended Providers in IPNI spec.

Copy link
Author

@MichaelMure MichaelMure Feb 14, 2024

Choose a reason for hiding this comment

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

IIUC what you'd like to be able to do is to use the engine as a publisher for providers that may reside elsewhere. Right?

That is correct.

@masih masih requested a review from gammazero February 14, 2024 16:39
o.provider.ID = pId
}
return nil
}
Copy link
Collaborator

@gammazero gammazero Feb 19, 2024

Choose a reason for hiding this comment

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

This functionality is already available by using the WithProvider and WithPrivateKey options. The WithProvider option sets the peer and addresses for the provider to put in indexing advertisements. This is different than WithHost which sets the host that is publishing the advertisements (libp2p server).

Setting the provider ID should also be accompanied by a call to WithPrivateKey, to set the private key associated with the provider peer ID being set. This way the advertisements will be signed with the provider's key.

The only reason this PR currently works is because the indexer is currently configured to allow any ID to publish on behalf of a provider. This means the advertisement will be accepted if it has a valid signature from anyone. This configuration is long overdue for being locked down so that the indexer only accepts advertisements signed by the provider or signed by a peer from a specific list of delegated publishers for that provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants