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

feat: allow disabling value and provider storage/messages #400

Merged
merged 5 commits into from Dec 12, 2019

Conversation

Stebalien
Copy link
Member

fixes #274

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

That's a little funky.

One question wrt correctness is that the local option affects the propagation of messages to the network.
Ie if we disable values/providers, then GetValue and FindProviders will short-circuit and not send any messages to the network.
However, we might have peers that are able to answer these queries because they have these options enabled.
Is this intended?

@@ -110,6 +114,10 @@ type RecvdVal struct {

// GetValue searches for the value corresponding to given Key.
func (dht *IpfsDHT) GetValue(ctx context.Context, key string, opts ...routing.Option) (_ []byte, err error) {
if !dht.enableValues {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct behaviour? We might know a peer that supports GetValue and so on.

@@ -477,6 +495,9 @@ func (dht *IpfsDHT) makeProvRecord(skey cid.Cid) (*pb.Message, error) {

// FindProviders searches until the context expires.
func (dht *IpfsDHT) FindProviders(ctx context.Context, c cid.Cid) ([]peer.AddrInfo, error) {
if !dht.enableProviders {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here.

@Stebalien
Copy link
Member Author

Is this intended?

Yes. This feature is intended to be used in private/forked DHTs with custom protocol names (e.g., the Filecoin DHT).

@lanzafame
Copy link
Contributor

Yes. This feature is intended to be used in private/forked DHTs with custom protocol names (e.g., the Filecoin DHT).

@Stebalien Could this get documented in the code? Maybe on the struct values:
https://github.com/libp2p/go-libp2p-kad-dht/pull/400/files#diff-5d01a71abe5aa561a232951a0eb79b0cR74

@Stebalien
Copy link
Member Author

@lanzafame I'm documenting the options.

@Stebalien
Copy link
Member Author

(eh, I did it in both places)

@Stebalien Stebalien requested a review from raulk December 6, 2019 00:15
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Besides inline comments, two overall notes:

  1. Let's document that GetPublicKey() will also stop to operate properly if values are disabled. Do we expect any major impact?
  2. How will the routing helpers behave when we start returning errors?

routing.go Outdated Show resolved Hide resolved
opts/options.go Outdated Show resolved Hide resolved
@Stebalien
Copy link
Member Author

Let's document that GetPublicKey() will also stop to operate properly if values are disabled. Do we expect any major impact?

None. It was only ever used for validating IPNS records and:

  1. IPNS needs the valuestore system as well.
  2. IPNS records now include the public key.

How will the routing helpers behave when we start returning errors?

They'll continue to work:

  • On put, as long as one router works, ErrNotSupported will be ignored.
  • On get, as long as one router returns a value, errors will be ignored.

@Stebalien Stebalien requested a review from raulk December 7, 2019 02:43
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@Stebalien Stebalien merged commit 2e6adb8 into master Dec 12, 2019
@Stebalien Stebalien deleted the feat/disable-providers branch December 12, 2019 17:05
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.

add option to run DHT without providers
4 participants