ipfs / specs

Technical specifications for the IPFS protocol stack
https://specs.ipfs.tech
1.16k stars 232 forks source link

IPIP-431: Opt-in Extensible CAR Metadata on Trustless Gateway #431

Open bajtos opened 1 year ago

bajtos commented 1 year ago

Define an optional enhancement of the CARv1 stream that allows a Gateway server to provide additional metadata about the CARv1 response. Introduce a new content type that allows the client and the server to signal or negotiate the inclusion of extra metadata.

The PR discussing a new multi-codec car-metadata: https://github.com/multiformats/multicodec/pull/334

bajtos commented 1 year ago

@lidel @willscott Here is my proposal for allowing gateway clients to request the response to include a metadata block at the end.

This is my first IPIP. Please let me know what and how to improve, where to add more details, etc. Feel free to edit the text directly if you like (edits by maintainers are allowed).

rvagg commented 1 year ago

I don't think we're going to be able to do a new CID codec code for this (car-metadata), see discussion over in the multicodec thread. I don't really want to go into that here (there's been many keystrokes spilled over this question for a few years now). Unfortunately the limits of CARv1 keep on getting in our way and the focus on the blockstore use-case of CARv2 means it's not great as a transport format. But (aside from just doing CARv3 and fixing all the things) there's a few things we could consider:

  1. CAR with special last block, but not a special codec

If you supply ?meta then you're opting in to a special format. Clients that do this, like Lassie, would then get to assume that the last block likely has special meaning—we just need to attempt to pass it through a well-defined schema (see the one I proposed in the multicodec thread) and if it matches then strip it from the CARv1 payload and do <special thing> with the contents.

Unfortunately this option mixes up the metadata in the payload it's trying to describe, but at least we have mechanisms to signal the special nature of it.

  1. CAR with 0x00 end and metadata block following.

If we want to avoid the metadata being within the CARv1 payload. The ZeroLengthSectionAsEOF option that we have for go-car (I don't think we did it yet for js-car but it's straightforward) is already used inside Filecoin to delimit the end of a CAR body. We could make ?meta mean that you parse the CAR stream looking for the 0x00 || EOF to signal the end. Then any bytes beyond that can be whatever we need them to be—ideally a well-formed IPLD block conforming to a strict schema so we don't have opportunities for abuse.

One problem is that if you curl -o such a file with ?meta then you end up with a CAR that won't play nicely with most CAR tooling because ZeroLengthSectionAsEOF is a special library option (currently, we could add it to the go-car CLI).

  1. CARv2 with "metadata" index

CARv2 isn't really designed as a transport format, that's why we're using CARv1, but we may be able to squish it into a usable form to help with this problem. We have a "characteristics" field we can play with, and there's an index section we can also play with.

We can define a new index format very easily, and the CAR decoders will just complain if they can't read the index, which is fine for this purpose. We could define an index as a "trustless gateway metadata index"; which doesn't get the location data, but we can use it to put whatever we want into the trailer of the CARv2 stream—we could just encode well-defined IPLD block strictly conforming to a schema, to present this metadata and anything else we want.

The main problem is that CARv2 requires a "DataSize" in the header to tell us the length of the CARv1 payload, which we don't have up-front, and an "IndexOffset" to tell us where the index starts, which we don't have for the same reasons. We've used IndexOffset==0 to signal no-index to date. But we could use the characteristics field to deal with these problems. We just need to parse the CARv1 payload with the ZeroLengthSectionAsEOF option turned on, which we can use as the signal for the end of the CARv1 data. We could also make that characteristic bit tell us that if IndexOffset==0 but there are more bytes after the payload, to interpret it as an index. Then the CAR decoder would fine our new index format, containing our metadata.

We get to leave the CARv1 intact, in the same form that you would get it if you didn't turn meta parameter on, and we get a ~valid CARv2 format file that could be read with a standard CAR decoder .. with the caveat that there's a couple of things here that will probably make existing parsers bork at it until we update the code and ship new versions. Arguably an extension of CARv2 rather than hard break to the format. Current CAR tooling may have problems, but we could produce new versions that are fine with these things in a way that doesn't have to compromise on anything else.


As an aside, we could use any of these options to do our error signalling, which I'm pretty keen on having. A schema for this metadata block could be a union type of the metadata presented here or an error string, so we can log and possibly pass on that information up the chain (still likely having to do the bad-chunk ending thing for cache reasons).

willscott commented 1 year ago

the keys / schema presented here, i think, should be considered an example. I would hope that it would be treated as an arbitrary key-value map of metadata objects, and that these key-values could be used to signal an error, could be used to signal an 'eof' signal, and/or could be used to provide additional check-sum attestation as described in the current text

rvagg commented 1 year ago

I'm prototyping a form of option 1 above with Frisbii and Lassie, will let you know how it goes.

rvagg commented 1 year ago

https://github.com/filecoin-project/lassie/pull/378 and https://github.com/ipld/frisbii/pull/15 demonstrate an approximation of the option 1 I presented above.

<stuff> that I've chosen to do at the moment is validate the car body according to the stats in the metadata and print out a "checksum" multihash that was provided by the server—this could be a signed checksum, or whatever it needs to be. There's also affordance for an error property in the metadata to signal that an error occurred and a message about it, so we get to see messages such as "block not found" now.

rvagg commented 1 year ago

I'll acknowledge that it may be better to just go with the plain map approach, without a schema, as Will's suggested. That would even let us do novel things for specific situations like having Lassie tell Saturn about retrieval clients and their timings (currently can only use Server-Timings header which ends when the data starts). But there's a bit of a can of worms that we open up that I wondered if we could avoid by having a strong schema, at least for the first version. All of the things that http headers have to deal with - like what to do with duplicate keys, what limits we need to put on the sizes of things to avoid abuse, etc. Constraining within the bounds of dag-json, which itself is a bit strict, and having a schema, let's us be very clear about rules and avoid abuse. But, maybe it's better to basically replicate the Trailers section but in the payload. 🤷

lidel commented 1 year ago

@rvagg prefixing metadata with NUL changes the scope of this IPIP, effectively moves metadata outside CARv1 serialization format that we have specs and compatible implementations, which makes it harder to argue reuse of the same content type as CAR.

Existing CARv1 implementations will error without explicit support for ZeroLengthSectionAsEOF, right?

This thing starts looking like a new content type, changing the scope of this IPIP to something similar to application/vnd.ipld.car-stream from "Alternatives" section.

Not saying it is bad, maybe a separate content type for streaming CARs is the right call here. It mitigates risks around mixing regular CAR responses with ones that include metadata trailer and causing issues on clients that don't support ZeroLengthSectionAsEOF, and poisoning HTTP caches in scenarios when a rogue Saturn L1 sends a fishy CAR response.

But i'm worried about duplicated effort across teams and project in light of CARv3, which (iiuc) also needs to happen some time in the next ~12-24 months and might have overlapping scope, solving similar problems.


@willscott @bajtos is this IPIP something we intend to expose on all gateways and support forever in the IPFS ecosystem, even when we have CARv3? Would this be intended for wrapping CARv2 and v3 too? Or is this just a stop-gap for Rhea/Boost internally until we have CARv3 with built-in metadata/eof support?

rvagg commented 1 year ago

Yeah you're right @lidel, this content type is "CARv1 but terminated with NUL byte", you need to tell the parser to handle it in that mode to see the content properly.

Arguably it could also be seen as "CARv1 within another container that has additional content at the end", which is also a different content type.

I think there is a strong desire here not to block metadata sending on a CARv3 because of how long that's likely to take to happen, whereas the work that needs this metadata has some time pressure associated with it.

willscott commented 1 year ago

is this IPIP something we intend to expose on all gateways and support forever in the IPFS ecosystem, even when we have CARv3?

it would probably need to support the retroactive validation of commitments.

rvagg commented 1 year ago

Here's another option to throw into the mix: https://github.com/ipld/ipld/pull/295

The big catch with this is that it's not compatible with existing CAR implementations; they'll read a CAR created with this and bork at the data-size=0 and think there's nothing in there. But we can easily upgrade implementations to understand these.

I think I consider this less breaky than my CARv1 zero-length-termination version because with that one there's not really an upgrade path for CAR implementations other than to do things like provide --zero-length-termination flags for the user to opt in to when consuming those CARs. At least with this one the answer is "oh, you're using an older CAR implementation, upgrade and it'll load just fine".

Trustless Gateway implementations can still provide a CARv1 when we don't need extra stuff, we evolve it with a new Accept format that lets them switch to this format for additional signalling and optional metadata.

bajtos commented 1 year ago

Hi folks, thank you for a constructive discussion and lots of new ideas for improving my proposal. :pray:

IIUC, we all agree with the following aspects:

Then there are two areas where haven't reached a consensus yet:

  1. How to design the new container format "CAR + metadata".
  2. What is the schema of the metadata block, what is required vs. optional, and so on?

Let me also explain more about our context:


Let's dive into the container format now.

Considering the strong push-back in https://github.com/multiformats/multicodec/pull/334, I think we should look for options that don't require a new multicodec.

I quite like Rod's proposal to use dag-cbor and car-metadata/v1 as the top-level key. It makes it easy to detect the metadata block by inspecting the first 17 bytes of the block payload and comparing them to the magic string \xA1\x6Fcar-metadata/v1 (A1 signals map(1), 6F signals text(15)).

I am reluctant to introduce a zero-length EOF block:

What do you think about the following proposal?

rvagg commented 1 year ago

I'm not opposed to just tacking on a trailing block; it does make validating/consuming the data slightly harder than having a clean termination of the payload and the metadata sitting outside of it, but not terrible since we should know what the "end" is if we're validating while streaming. My main discomfort, and reason for suggesting other paths, is that you're mixing metadata in with the data that the metadata is describing. <a thing> <something about the thing> should be separate items.

You also get a CAR that's not quite what you asked for. I doubt there are handlers out there that are going to have problems with this (although Kubo dag import will find 2 separate DAGs in there to pin). As has been said already, content-type is the way to differentiate that and it'd have to be caveat emptor.

It is the path of least resistance if that's what we're going for with this.

With any of these approaches, if we're able to build in generally useful metadata (such as error messages or other information), then what we'd do in Lassie is attempt to opt-in all of our HTTP requests for this, and make allowance for both getting and not getting the metadata in whatever form it is, and not directly passing it on to the user, but separating it out appropriately (on the CLI it might just be extra printed information if that makes sense, like what https://github.com/filecoin-project/lassie/pull/378 does, in our API maybe we provide a separate result packet containing the information).

willscott commented 1 year ago
bajtos commented 1 year ago

I'm not opposed to just tacking on a trailing block; it does make validating/consuming the data slightly harder than having a clean termination of the payload and the metadata sitting outside of it, but not terrible since we should know what the "end" is if we're validating while streaming. My main discomfort, and reason for suggesting other paths, is that you're mixing metadata in with the data that the metadata is describing. <a thing> <something about the thing> should be separate items.

I see your point, @rvagg. As was thinking about how to update your Lassie PR to my new proposal, I realised that the condition "the last block" is indeed more difficult to implement.

I quite like how the zero-byte proposal allows us to think about the payload as a container containing two compartments: CARv1 and metadata.

<CARv1 blocks> <0x00 byte> <metadata>

We can prevent insertion attacks by requiring the party producing this container (Frisbee, Boost) to remove any zero-length blocks from the CARv1 payload. I think this is a reasonable requirement, considering that 0x00 is not allowed in CARv1 anyway.

Because we are inventing a new container-based format here, I am arguing that we don't necessarily have to adjust CAR parsers. Instead, we can add a simple middleware-like filter between the source of CARv1 blocks (the upstream HTTP service) and the CARv1 parser, and let this middleware stop sending bytes to the parser when it reaches the first zero-length block. In practice, enabling ZeroLengthSectionAsEOF is the faster option to a working version in our Go stack. I think it's important that ZeroLengthSectionAsEOF is one of many options for how to implement this, a kind of implementation detail.

When we look at the proposal as a new container format, we are not changing the current "end of transport stream" semantics. The stream contains the entire container, and the zero-length block is a part of that container.

The biggest remaining item is the content type for this new container format.

In https://github.com/ipfs/specs/pull/431#issuecomment-1683041576, @lidel proposed a new content type application/vnd.ipld.car-stream.

Thoughts?

With any of these approaches, if we're able to build in generally useful metadata (such as error messages or other information), then what we'd do in Lassie is attempt to opt-in all of our HTTP requests for this, and make allowance for both getting and not getting the metadata in whatever form it is, and not directly passing it on to the user, but separating it out appropriately (on the CLI it might just be extra printed information if that makes sense, like what https://github.com/filecoin-project/lassie/pull/378 does, in our API maybe we provide a separate result packet containing the information).

+1 to separate out the metadata for the CLI and Go API users.

I think Lassie's HTTP server should produce its own metadata block if the client asks for it via content-type negotiation. It allows Lassie to add extra metadata to the response, which is one of the goals of this IPIP.

Where it becomes tricky: In SPARK, we need to obtain the original metadata block that Lassie received from the storage provider. I was initially thinking that Lassie could simply forward the metadata block received from the storage provider.

Now that I think about this again, I can see how that can be problematic in the case when Lassie modifies the CAR stream received from the SP, e.g. to re-order blocks in DFS order and include duplicate blocks. In such cases, SP-provided metadata fields like "car length" and "Blake3 hash of CARv1 payload" have no meaning for Lassie clients, as they will receive a different CAR file.

Is there a way to ask Lassie's HTTP server to respond with the exact CAR file it received from the upstream (the storage provider)? If there isn't an easy solution for this (existing or easy to implement), then I guess SPARK will have to retrieve content directly from SPs, crafting the request to be indistinguishable from Lassie's requests.

rvagg commented 1 year ago

At this point I'm easy; let me know how I can help iterate forward here.

bajtos commented 1 year ago
  • with that in mind, given this is effectively a surgical stop-gap until CARv3, even if you want to go the <0x00 byte> route, my suggestion would be to reuse the CAR content type and introduce this via optional param
  • seems reasonable that we don't introduce a new one that we're shouldered with forever that only does this one thing and not all the other things we want to do for a CARv3

👍🏻

Let's go with application/vnd.ipld.car then.

I've already expressed a mild preference for dag-json rather than cbor for this trailing chunk--it's human readable which might help in the case that someone downloads this as a CAR that doesn't parse like a normal CAR so they go inspecting it.

Makes sense, I don't have a strong opinion here.

My only concern is about the overhead of dag-json when compared to dag-cbor - we don't want to inflate response sizes too much.

lidel commented 1 year ago

Mind, you don't save much with dag-cbor if your keys and values are strings. Readability of dag-json is more valuable.

lidel commented 1 year ago

@bajtos wiil you have time to update this IPIP to reflect the latest changes based on discussions so far? iiuc they are:

patrickwoodhead commented 1 year ago

@bajtos wiil you have time to update this IPIP to reflect the latest changes based on discussions so far? iiuc they are:

  • [ ] dropping custom codec requirement and moving metadata to the trailer after 0x00 (<CARv1 stream> <0x00 byte> <metadata>)
  • [ ] switching metadata encoding from dag-cbor to dag-json
  • [ ] decision? documenting JSON keys that may be useful outside of SPARK (car_bytes data_bytes, block_count, car_cid?)
  • [ ] decision? if you want to rename from meta=eof to meta=eof+json, or leave as-is

@lidel I will be taking on updating this based on discussion so far. Agreed on first two bullets. I think meta=eof+json works well. the last remaining point of ambiguity is how we document the useful JSON keys

rvagg commented 1 year ago

At this stage I'm not sure I see much of real use other than a field for an error message. I mocked up a schema in my prototype version of this in https://github.com/filecoin-project/lassie/pull/378/files#diff-c0f5012f54c2bfa71efce83a8f158229509132b9a8548158e105df4847fa9f18 which I then went on to specify that the "properties" and "error" fields were both optional with the intention that it would be one or the other (perhaps I should have used some type of union to specify this).

I did make a "request" field mandatory though—echo back what the original request was, so you end up with a block that says "here's what I was asked for, and here's what I was able to provide for it"—in perfect circumstances, a provider given the same request and having access to the full DAG should produce identical information for both of these fields. The uniqueness would come from SPARK stuff.

bajtos commented 11 months ago

Hello folks; thank you for your patience! Together with @patrickwoodhead, we incorporated your feedback and updated both the proposal and the spec.

We are ready for the next round of reviews. 🙏🏻

bajtos commented 11 months ago

Previews of the current version:

bajtos commented 10 months ago

@lidel @rvagg @willscott Ping 👋🏻 What's the best way to move this proposal forward?