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

IPIP-412: Signaling Block Order in CARs on HTTP Gateways #412

Merged
merged 15 commits into from
Aug 4, 2023

Conversation

lidel
Copy link
Member

@lidel lidel commented May 15, 2023

This PR supersedes #330 and continues order discussions from #402

cc @hannahhoward @aschmahmann @Jorropo @willscott @rvagg @ribasushi @John-LittleBearLabs @fabricedesre

TODO

What

This IPIP is based on prior art from IPIP-330, and recent discussions cited in the header front matter, including ones in IPIP-402.

Why

👉 To keep scope in check, I made a conscious decision to only specify a single opt-in ordering that aims to reduce resource usage on light clients like lighter read-only Brave, IPFS in Chromium, Capyloon, RAPIDE, or even bifrost-gateway.

These are the north star use cases for now.

TLDR How

The proposed solution reuses HTTP content type negotiation from RFC9110 and introduces two new parameters for the content type headers
in HTTP requests and responses: order and dups.

  • The order parameter allows the client to indicate its preference for a
    specific block order in the CAR response, for now we only specify the Depth-First Search order.
  • The dups parameter specifies
    whether duplicate blocks are allowed in the response

The proposed spec creates room for other ones to be added in future IPIPs.

cc @hannahhoward @aschmahmann @Jorropo @willscott @rvagg @ribasushi

@lidel lidel force-pushed the ipip-car-order-signaling branch from 250bbe2 to 5818113 Compare May 15, 2023 12:48
@lidel lidel changed the title IPIP: Signaling Block Order in CARs on Gateways IPIP0412: Signaling Block Order in CARs on Gateways May 15, 2023
@lidel lidel changed the title IPIP0412: Signaling Block Order in CARs on Gateways IPIP-412: Signaling Block Order in CARs on Gateways May 15, 2023
@lidel lidel marked this pull request as ready for review May 15, 2023 12:49
@lidel lidel requested a review from a team as a code owner May 15, 2023 12:49
@lidel lidel force-pushed the ipip-car-order-signaling branch 2 times, most recently from 81f42be to 63ea4ff Compare May 15, 2023 12:56
@lidel lidel changed the title IPIP-412: Signaling Block Order in CARs on Gateways IPIP-412: Signaling Block Order in CARs on HTTP Gateways May 15, 2023
src/ipips/ipip-0412.md Outdated Show resolved Hide resolved
src/ipips/ipip-0412.md Outdated Show resolved Hide resolved
src/ipips/ipip-0412.md Outdated Show resolved Hide resolved
src/ipips/ipip-0412.md Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented May 16, 2023

Does identity CID handling belong in here or #402? It feels very closely related to duplicate blocks so might be in scope to be described here.

src/ipips/ipip-0412.md Outdated Show resolved Hide resolved
lidel added a commit that referenced this pull request May 17, 2023
src/ipips/ipip-0412.md Outdated Show resolved Hide resolved
Copy link
Member

@olizilla olizilla left a comment

Choose a reason for hiding this comment

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

From a clients point of view:

  • order=dfs; dups=y means "make it as easy as possible for me to unpack"
  • order=dfs; dups=n means "i will track blocks but verifiably get me to the leaf blocks asap!"

Are there other combos that make sense for a client to set?

Could we encode this as a single param to avoid having combinations tha don't make sense. Something like pack=dfs implying plz dedupe if you can and pack=dfs-stream implying dupes must to be included?

For the server-side, we may struggle to guarantee full dedupe in freeway. A CF worker gets 128MB of ram shared between concurrent requests from that region. In theory we could track ~2 million CIDs, but that number will drop as it gets busy. Stable ordering will eat some more ram too.

Let's have discussion in #412 (comment)

@Jorropo
Copy link
Contributor

Jorropo commented May 17, 2023

Could we encode this as a single param to avoid having combinations tha don't make sense. Something like pack=dfs implying plz dedupe if you can and pack=dfs-stream implying dupes must to be included?

I was already proposing to join dupness and ordering in the previous IPIP but it was apparently really confusing.

Are there other combos that make sense for a client to set?

  • order=weak, dups=n

Allows the gateway to use stream different parallel downloads and the client can still incrementally and stream the data if you are writing it to an io.ReadWriterAt.

(this is being moved to an other IPIP)

@lidel
Copy link
Member Author

lidel commented May 17, 2023

@olizilla in my mind these flags are separate for a reason:
to allow implementations like the one you described to do the best they can.

If gateway can't guarantee anything about duplicates, but can do DFS, it is better if it returns CAR Content-Type with order=dfs but have no dups parameter, than error.
They can also return HTTP 406 Not Acceptable (111fc4f) if anyone asks for one of unsupported combinations.

src/ipips/ipip-0412.md Outdated Show resolved Hide resolved
alanshaw pushed a commit to storacha/dagula that referenced this pull request May 18, 2023
# Goals

This adds support for depth first traversals to Dagula. It's designed to
be in conformance with ipfs/specs#412.

(will update if parameter names change)

# Implementation

- adds order parameter to options struct for getPath and get
- renames breadFirstSearch to blockLinks (it isn't really "bread first
search" so much as "all the links in this block").
- handles ordering change inside of `get`, using recursion for
depth-first-search
- adds tests for both orders.

---------

Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
alanshaw pushed a commit to storacha/gateway-lib that referenced this pull request May 19, 2023
This PR updates dagula and implements block order signaling via HTTP
Accept header as specified in ipfs/specs#412.

It also renames the query param `car-scope` to `dag-scope` and changes
the scope value `file` to `entity` as these were updated in
ipfs/specs#402.
@lidel lidel changed the base branch from main to feat/improving-trustless-gateways July 6, 2023 12:34
lidel and others added 7 commits July 6, 2023 18:57
First draft based on various prior art and recent discussions cited in
the header front matter.
Co-authored-by: Oli Evans <oli.evans@gmail.com>
First draft based on various prior art and recent discussions cited in
the header front matter.
@lidel lidel force-pushed the ipip-car-order-signaling branch 3 times, most recently from c9e231c to e94a390 Compare July 6, 2023 17:40
- moving spec details to trustless-gateway
- rebasing on top of ipip-402
- fixing linter
@lidel lidel force-pushed the ipip-car-order-signaling branch from e94a390 to 02a8465 Compare July 6, 2023 17:52
Copy link
Member Author

@lidel lidel left a comment

Choose a reason for hiding this comment

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

We have conformance tests (ipfs/gateway-conformance#87), reference order=dfs implementation is in boxo/gateway (ipfs/boxo#370) and plan to ship this with Kubo 0.22 (ipfs/kubo#9989).

Thank you to everyone involved, especially asking questions and pointing gaps – it all made the specification better.

I believe this is ready for being ratified and merged after final fixture CIDs are known (comment below)

Will flag this during IPFS-Implementers-Sync-2023-07-20.

src/ipips/ipip-0412.md Outdated Show resolved Hide resolved
Base automatically changed from feat/improving-trustless-gateways to main July 27, 2023 14:16
Copy link
Member Author

@lidel lidel left a comment

Choose a reason for hiding this comment

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

There were no ratification concerns during IPFS Implementers sync, nor during the two weeks after it.
Merging! 🎉

For future reference:

Thank you to everyone involved in this one ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ratified
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

10 participants