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

[kad] Store addresses of provider records. #1708

Merged
merged 5 commits into from
Aug 18, 2020

Conversation

romanb
Copy link
Contributor

@romanb romanb commented Aug 17, 2020

So far, provider records are stored without their addresses and the addresses of providers are obtained from the routing table on demand. I don't remember whether this was ever done on purpose or just an oversight as part of a larger amount of work. In any case, this approach has two shortcomings:

  1. We can only return provider records whose provider peers happen to currently be in the local routing table.

  2. The local node never returns itself as a provider for a key, even if it is indeed a provider.

These issues are addressed here by storing the addresses together with the provider records, falling back to
addresses from the routing table only for backward-compatibility with existing implementations of RecordStore that use persistent storage and may thus have records that persist across restarts.

Closes #1526.

So far, provider records are stored without their
addresses and the addresses of provider records are
obtained from the routing table on demand. This has
two shortcomings:

  1. We can only return provider records whose provider
  peers happen to currently be in the local routing table.

  2. The local node never returns itself as a provider for
  a key, even if it is indeed a provider.

These issues are addressed here by storing the addresses
together with the provider records, falling back to
addresses from the routing table only for backward-compatibility
with existing implementations of `RecordStore` using persistent
storage.

Resolves libp2p#1526.
@romanb romanb requested a review from mxinden August 17, 2020 14:19
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

We can only return provider records whose provider peers happen to currently be in the local routing table.

Good catch. I have missed this to be honest.

Wouldn't the ProviderRecords need to be updated in inject_addr_failure and inject_address_change?

protocols/kad/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/kad/src/behaviour.rs Show resolved Hide resolved
protocols/kad/src/behaviour.rs Outdated Show resolved Hide resolved
@romanb
Copy link
Contributor Author

romanb commented Aug 18, 2020

Good catch. I have missed this to be honest.

Wouldn't the ProviderRecords need to be updated in inject_addr_failure and inject_address_change?

I don't think that is necessary as provider records are regularly re-published. It may not even be desirable.

As for inject_addr_failure, just because one node cannot reach a certain address does not mean others cannot either. A provider may publish a bunch of addresses, different subsets being reachable by different nodes, depending on where they are located. This is not dissimilar to the addresses in the routing table where we do remove such addresses, except for the last one, which is a trade-off in favour of faster routing lookups in the general case, because these addresses are used so much more frequently in the iterative routing. In general, however, it is the job of the provider to tell others which addresses to store and advertise to others.

As for inject_address_change, even if a provider is generally ok with its addresses being updated without his direct instruction in this manner, I think a push-based approach would be more desirable. Meaning, a provider could re-publish its own provider records whenever his listen addresses change, but given that this is already done in regular intervals anyway, it is at best a nice-to-have.

Does that sound reasonable or do you have more serious concerns?

Co-authored-by: Max Inden <mail@max-inden.de>
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Following up on the discussion above (#1708 (comment)):

it is the job of the provider to tell others which addresses to store and advertise to others.

That sounds plausible. Thank you for the detailed explanation.

protocols/kad/src/behaviour.rs Outdated Show resolved Hide resolved
@romanb romanb merged commit e1df920 into libp2p:master Aug 18, 2020
@romanb romanb deleted the kad-provider-addresses branch August 18, 2020 12:14
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.

Kad-GetProviders doesn't work on two nodes
2 participants