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

add provide messages in reframe #285

Merged
merged 4 commits into from Aug 16, 2022
Merged

add provide messages in reframe #285

merged 4 commits into from Aug 16, 2022

Conversation

willscott
Copy link
Contributor

@willscott willscott commented Jun 7, 2022

Add a Provide request/response message for the provide side of delegated routing

@willscott willscott requested a review from aschmahmann Jun 7, 2022
@willscott
Copy link
Contributor Author

willscott commented Jun 7, 2022

One design point that is implicit here but that may be worth calling out specifically, is that this format implies that providing follows the same behavior as in current IPFS implementations - that a provide has an implicit 24hr ttl.

Should we have a provide / retract model like in network indexing instead?
should we support an explicit ttl?

REFRAME.md Outdated
type ProvideRequest struct
Key &Any
Providers [Provider]
}
Copy link
Contributor

@aschmahmann aschmahmann Jun 8, 2022

Choose a reason for hiding this comment

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

From @willscott

One design point that is implicit here but that may be worth calling out specifically, is that this format implies that providing follows the same behavior as in current IPFS implementations - that a provide has an implicit 24hr ttl.

Should we have a provide / retract model like in network indexing instead?
should we support an explicit ttl?

Good questions, a few thoughts:

  1. This interface cannot be used to proxy requests to the current IPFS Public DHT. This is simply a result of the current RPCs that only accept messages like "I am providing foo" rather than "Alice is providing foo" Remove special cases for Peer and Provider records libp2p/go-libp2p-kad-dht#584. This means the systems most likely to use this in the shorter term are newer systems (e.g. network indexers, things that look like BitTorrent trackers, new DHTs, etc.) and so we can look toward what those systems want to use.
  2. While people can use transport level protections like bearer tokens , IP censoring, etc. to protect requests like Provide from being spammed, there's nothing in the message really helping a server prevent abuse. Perhaps we want something here to help.
  3. Do we want to design for these to come in bulk? I guess we could just send multiple messages after each other and rely on transport compression to reduce the overhead, so it might be fine as is.
  4. Providing a recommended TTL seems pretty reasonable
  5. Allowing for a retract also seems fairly reasonable, although probably there should either be no expectation from the client that this actually happens or that expectation should be communicated via the response or something (e.g. a reframe server which advertises data to two systems one it can retract from and one it cannot won't be able to fully retract it).
    • Note; Retract seems even more problematic than the rest without some authentication/abuse protection that prevents me from retracting your content just because I feel like it (e.g. I ask for providers then retract them all)

Copy link
Contributor Author

@willscott willscott Jun 13, 2022

Choose a reason for hiding this comment

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

  1. I'm happy to add a signature field. I believe we have some prior work here in thinking about signed peer records. is there a documentation to look at to make sure the proposal matches what we ended up with there?
  2. lets do that later if we find it's a performance issue.
  3. I think I'll propose an 'advisoryTTL' on requests, and responses - the response may indicate that it's stored for either longer or shorter than the request indicates.
  4. This seems like a pretty reasonable argument to not include retraction, and save the complexity until we're really sure we need it.

@willscott
Copy link
Contributor Author

willscott commented Jul 4, 2022

@aschmahmann i pushed a proposal for 2 and 4. I wonder if the mechanics around expectations for provides is worth a session at the thing next week?

@willscott
Copy link
Contributor Author

willscott commented Jul 13, 2022

This is re-based, and ready for a next look

@willscott
Copy link
Contributor Author

willscott commented Jul 28, 2022

ping @ajnavarro / @aschmahmann / @lidel

@aschmahmann
Copy link
Contributor

aschmahmann commented Jul 29, 2022

Aside from the PR failing this seems fine to me. Ultimately, this is being designed to fit the storetheindex use case and ideally it works for others but if not then 🤷 we can make new methods or version older ones.

If there are no objections or obvious flags to raise then IMO @willscott your team should do some prototyping with this and see if it works for you and we can merge, alternatively we could merge it and mark as "draft" and switch it to non-draft once you're happy with how it works in production.

@willscott
Copy link
Contributor Author

willscott commented Jul 29, 2022

The linting here does not appear to be a result of this PR fwiw:

/github/workspace/reframe/REFRAME_KNOWN_METHODS.md:1 MD045/no-alt-text Images should have alternate text (alt text)
/github/workspace/reframe/REFRAME_KNOWN_METHODS.md:15 MD025/single-title/single-h1 Multiple top-level headings in the same document [Context: "# Organization of this documen..."]
/github/workspace/reframe/REFRAME_KNOWN_METHODS.md:32 MD025/single-title/single-h1 Multiple top-level headings in the same document [Context: "# Known Methods"]
/github/workspace/reframe/REFRAME_KNOWN_METHODS.md:78 MD025/single-title/single-h1 Multiple top-level headings in the same document [Context: "# Message Specs"]

@aschmahmann
Copy link
Contributor

aschmahmann commented Jul 29, 2022

Thanks for the flag this seems to have been a result of #292.

willscott added a commit to willscott/go-delegated-routing that referenced this issue Jul 29, 2022
willscott added a commit to ipfs/go-delegated-routing that referenced this issue Aug 10, 2022
* Add Provide RPC per ipfs/specs#285
@ajnavarro
Copy link
Member

ajnavarro commented Aug 11, 2022

@willscott do you know if rebasing we can fix the CI errors? Thx!

@willscott
Copy link
Contributor Author

willscott commented Aug 12, 2022

@ajnavarro re-basing did fix the CI errors

Copy link
Member

@ajnavarro ajnavarro left a comment

LGTM. Let's wait for @aschmahmann for the final validation.

Copy link
Contributor

@aschmahmann aschmahmann left a comment

LGTM and merging. Generally I'd wait until I had running code and got things going before merging, but @willscott's been the one doing the implementation work so if he's happy with merging then I'm happy 😄.

@aschmahmann aschmahmann merged commit e4d0123 into main Aug 16, 2022
2 checks passed
@aschmahmann aschmahmann deleted the reframe-provide branch Aug 16, 2022
@aschmahmann aschmahmann changed the title propose provide messages in reframe add provide messages in reframe Aug 16, 2022
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.

None yet

3 participants