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-330: ordered car responses draft #330

Closed
wants to merge 2 commits into from

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Oct 10, 2022

No description provided.

@Jorropo Jorropo requested a review from a team as a code owner October 10, 2022 10:28
@Jorropo Jorropo marked this pull request as draft October 10, 2022 12:45
@Jorropo Jorropo changed the title REWORD IPIP ordered car responses draft IPIP ordered car responses draft Oct 10, 2022
@Jorropo Jorropo changed the title IPIP ordered car responses draft IPIP: ordered car responses draft Oct 10, 2022
@rvagg
Copy link
Member

rvagg commented Oct 11, 2022

Having "include identity CIDs" would be a helpful permutation to add to the list here. Ideally we could use this to describe the Filecoin online deal CAR creation for the purpose of generating an agreed CommP (essentially a hash of the CAR on both client and server). Aside from DAG ordering problems, the other problem we've run in to is where one side includes identity CIDs and the other doesn't. For historical reasons, inclusion of identity CIDs got baked in to the online deal flow. But, perhaps with a mechanism to describe what an "ordered CAR" looks like for setting up a Filecoin deal we could have a way of agreeing up-front on whether it should include them or not (previously I've thought about having some kind of boolean flag to set up agreement, but the descriptors here might be useful for this too).

@Jorropo
Copy link
Contributor Author

Jorropo commented Oct 11, 2022

@rvagg I meant (but forgot) to precise that inline CIDs MUST NOT be transmited (but MUST be traversed).

I was thinking suffixes could actually be made a list of flags if needed, sounds like legacy Filecoin code would need it so a -WithInline option seems good to me.

Then Filecoin deals could be either one of:

  • ForwardDFSWithSkippingOfDuplicates-OnlyUsefull
  • BackwardDFSWithSkippingOfDuplicates-OnlyUsefull
  • ForwardDFSWithDuplicates-OnlyUsefull
  • BackwardDFSWithDuplicates-OnlyUsefull
  • ForwardDFSWithSkippingOfDuplicates-OnlyUsefull-WithInline
  • BackwardDFSWithSkippingOfDuplicates-OnlyUsefull-WithInline
  • ForwardDFSWithDuplicates-OnlyUsefull-WithInline
  • BackwardDFSWithDuplicates-OnlyUsefull-WithInline

(other ones aren't acceptable because they are not fully deterministic)

Does that sounds good ?

@Jorropo
Copy link
Contributor Author

Jorropo commented Oct 11, 2022

Also if this would be usefull for filecoin, the ordering part of this spec should be moved into IPLD.

@ribasushi
Copy link
Contributor

@Jorropo filecoin is stedily moving away from considering the ordering of blocks within a .car file and/or whether they form an actual DAG. What you are discussing here is strictly of interest to gateways and out of scope for bulk transport systems ( like Fil ).

@Jorropo
Copy link
Contributor Author

Jorropo commented Oct 11, 2022

@ribasushi ok I was responding to:

Ideally we could use this to describe the Filecoin online deal CAR creation for the purpose of generating an agreed CommP

But if Filecoin isn't then we can keep it here.

Copy link
Member

@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.

Thank you @Jorropo, did a quick first pass, details inline but quick feedback:

  • This IPIP introduces many orderings without making it clear why we need all of them.
    • Either simplify (remove magical fallbacks – maybe we can remove some types too?),
    • or document when a client would want to use each of them and not the other ones.
  • Cosmetic: lowercase and shorten ordering parameter and its values (some ideas below)
  • Reuse preexisting RFC HTTP semantics for things like accepting multiple response types and listing supported ones (details around Accept in comments below)

@@ -1,68 +0,0 @@
# IPIP 0000: InterPlanetary Improvement Proposal Template
Copy link
Member

Choose a reason for hiding this comment

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

@Jorropo please restore the template 🙏 :trollface:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this happened when an IPIP number was attribued (so before merging).

Anyway ok I'll

IPIP/0000-ordered-car-responses.md Outdated Show resolved Hide resolved
IPIP/0000-ordered-car-responses.md Outdated Show resolved Hide resolved
1. Blocks can arrive out of order, meaning when a block is consumed (data is red and returned to the consumer) and when it's received might not match.
1. Blocks can be reused multiple times, this is handy for cases when you plan to cache on disk but not at all when you want to process a stream with use & forget policy.

What we really want is for the gateway to help us a bit, and give us blocks in a useful order.
Copy link
Member

Choose a reason for hiding this comment

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

@Jorropo Is this the proper characterization of the desired end goal? Or do we want to support other use cases (not clear from the document):

Suggested change
What we really want is for the gateway to help us a bit, and give us blocks in a useful order.
What we really want is for the gateway to help us a bit, and give us blocks in an order that removes the requirement of managing a block cache.

The block order correspond to someone reading the car file `Data` section (see car specification) from low index to high indexes.
In other words, blocks are sent first when streaming the car file over a fifo byte stream such as TCP connection, unix pipe, ... they are readable first.

`BlockOrder`, `enum (string)`, defaults to `Unordered`
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ @Jorropo I know this is tedious, but trust me, it will save us time ;)

This change will most likely be requested by IANA anyway, if we ever submit this as an update to existing https://www.iana.org/assignments/media-types/application/vnd.ipld.car, so better to clean this up from the beginning:

  • Media type's optional parameters are lowercase, and shorter is better.
    • 👉 Rename BlockOrder to order, use lowercase for all values too (e.g. FooBarBuzfoo-bar-buz)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I thought we could put anything there. I'll

Comment on lines +160 to +178
For example:

- `Accept: application/vnd.ipld.car; BlockOrder=ForwardDFSWithSkippingOfDuplicates` means it is intrested in either one of:
- `ForwardDFSWithSkippingOfDuplicates`
- `ForwardDFSWithSkippingOfDuplicates-OnlyUsefull`
- `ForwardDFSWithDuplicates`
- `ForwardDFSWithDuplicates-OnlyUsefull`
- `Accept: application/vnd.ipld.car; BlockOrder=Unordered` would accept anything.

Clients may qualify it multiple times, for example
`Accept: application/vnd.ipld.car; BlockOrder=ForwardDFSWithSkippingOfDuplicates-OnlyUsefull; BlockOrder=Unordered`
in this case this is a list of preferences (lower indexes is better), the client would really like `ForwardDFSWithSkippingOfDuplicates-OnlyUsefull`
however if this isn't available it would accept `Unordered` (so anything) instead.

Clients SHOULD NOT ask for an Ordering that already covered by a previous ask, for example:
`Accept: application/vnd.ipld.car; BlockOrder=ForwardDFSWithSkippingOfDuplicates; BlockOrder=ForwardDFSWithDuplicates`
is illegal because `ForwardDFSWithDuplicates` is already compatible with `ForwardDFSWithSkippingOfDuplicates`.

In case a gateway sees this it should read this left to right and ignore already covered orderings (it can try matching it multiple times, but this would do nothing).
Copy link
Member

Choose a reason for hiding this comment

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

⚠️

HTTP RFC 9110, 12.5.1. Accept already defines how to specify an HTTP request is accepting multiple response types:

Accept: application/vnd.ipld.car;order=foo;q=0.9, application/vnd.ipld.car;order=bar;q=0.8, application/vnd.ipld.car

We should simplify this spec, remove magical implicit fallbacks: format decision belongs to the client, server should not try to be smart and give "next best thing" – only fulfill request as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't knew Accept worked this way.
Yeah definitely better.


#### Ordering Incompatiblity

If none of the ordering requested by the client is supported the gateway MUST answer with `415 Unsupported Media Type`.
Copy link
Member

Choose a reason for hiding this comment

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

nit: add this code to https://github.com/ipfs/specs/blob/main/http-gateways/PATH_GATEWAY.md#response-status-codes + link to it.

I suggest we include note from https://httpwg.org/specs/rfc9110.html#status.415:

The Accept response header field can be used to indicate which media types would have been accepted in the request.

Sidenote: i believe this is how what you wanted to do with order=query should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is, and if it's how HTTP is supposed to work then it's better to follow this.

Comment on lines +142 to +147
### `Query`

This order MUST NOT be suppported by the gateway.

This is the canonical way to trigger a failure and thus query which orderings are supported.

Copy link
Member

Choose a reason for hiding this comment

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

I quess if you want to have dedicated query convention, it could be 415 Unsupported Media Type HTTP error and Accept header:

Suggested change
### `Query`
This order MUST NOT be suppported by the gateway.
This is the canonical way to trigger a failure and thus query which orderings are supported.
### `Query`
This order MUST NOT be suppported by the gateway.
This is the canonical way to trigger a failure and thus query which orderings are supported.
Returned HTTP Error MUST have supported orderings listed in the `Accept` response header field.
Lack of error, or a response with `Content-Type` without `order` means `order` parameter is not supported by the gateway.

That being said, having a dedicated fake-order type is an unnecessary spec complication, but against how content negotiation works in HTTP.

HTTP client can send multiple orderings in Accept with the request already (comma-separated), and even attach q weight to each one of them:

Accept: text/html, application/xhtml+xml, application/xml;q=0.9, image/webp, */*;q=0.8
Accept: application/vnd.ipld.car;order=foo;q=0.9, application/vnd.ipld.car;order=bar;q=0.8, application/vnd.ipld.car

Comment on lines +196 to +208
This indicates the start of the list of supported orderings which is a `, ` list of ordering up until the next newline, which indicates the end of the list (anything past this line MUST be ignored by clients).

The list doesn't need to include implicitly compatible orderings.

Regex:
```
.*: ((((Forward|Backward)DFSWith(SkippingOf|out)Duplicates)|Unordered|ProvenUsefullFirst)(-OnlyUsefull)?)(, ((((Forward|Backward)DFSWith(SkippingOf|out)Duplicates)|Unordered|ProvenUsefullFirst)(-OnlyUsefull)?))*$
```

Example:
```
BackwardDFSWithSkippingOfDuplicates is not a supported ordering, supported orderings are: ForwardDFSWithSkippingOfDuplicates-OnlyUsefull, ForwardDFSWithDuplicates-OnlyUsefull
```
Copy link
Member

Choose a reason for hiding this comment

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

⚠️

The magical Regex should be removed.

It is fine to have an example of human-readable HTTP error body, but the actual list of supported orderings (parsable by clients) should live in Accept header returned with the error response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I'll move to use 415 and Accept headers, this parsing of the error body is not needed anymore.

Comment on lines +72 to +85
For gateways:

`Unordered` is the only MUST implement ordering.

- `ProvenUsefullFirst`
- `ForwardDFSWithSkippingOfDuplicates`
- `ForwardDFSWithDuplicates`

are SHOULD implement orderings.

- `BackwardDFSWithSkippingOfDuplicates`
- `BackwardDFSWithDuplicates`

are MAY implement orderings.
Copy link
Member

Choose a reason for hiding this comment

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

I see 5 types of ordering, but no rationale why we need them.

Which one will be used by a low memory IoT device which is fetching firmware update as a CAR?
Are there other use cases that require different orders and handling of duplicates?

@lidel lidel changed the title IPIP: ordered car responses draft IPIP-330: ordered car responses draft Oct 26, 2022
@Jorropo
Copy link
Contributor Author

Jorropo commented May 15, 2023

Way overrenginered and confusing, closing in favor of #412.

@Jorropo Jorropo closed this May 15, 2023
@Jorropo Jorropo deleted the ordered-car-responses branch May 15, 2023 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Deferred
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants