CARv2 imports broken when using the HTTP API

aschmahmann commented 1 year ago

Checklist

[X] This is a bug report, not a question. Ask questions on discuss.ipfs.io.
[X] I have searched on the issue tracker for my bug.
[X] I am running the latest kubo version or have an issue updating.

Installation method

built from source

Version

Kubo version: 0.16.0
Repo version: 12
System version: amd64/linux
Golang version: go1.18.1

Config

Nothing of interest

Description

Run this command while the daemon is running

kubo/test/sharness/t0054-dag-car-import-export-data$ ipfs dag import lotus_testnet_export_128_v2.car
Error: operation not supported

This is the CAR imported in this test https://github.com/ipfs/kubo/blob/master/test/sharness/t0054-dag-car-import-export.sh#L257.

However, the test doesn't run with the daemon online (which is the most common way in which kubo is run).

IIUC the reason why this is happening is:

The wrapper used to process files inbound over the HTTP API implements the ReaderSeeker interface, but errors if the underlying Reader does not support seeking (as is true for multipart). https://github.com/ipfs/go-ipfs-files/blob/30b08cadfb701548b99f14c5b92dda95e735de61/readerfile.go#L72-L78
The CARv2 reader operates on ByteReaderSeeker so tries to convert/upgrade any Reader not implementing ByteReader or Seeker https://github.com/ipld/go-car/blob/50e029b9990efc2a9f70df098445fe5a5cf10df6/v2/block_reader.go#L79-L82
This is done using interface casting https://github.com/ipld/go-car/blob/50e029b9990efc2a9f70df098445fe5a5cf10df6/v2/internal/io/converter.go#L54-L62
The interface cast says the wrapper implements ReaderSeeker even though any calls to Seek will result in an ErrUnsupported
This results in the first call to Seek failing and the import failing

There's are a number of ways to handle this since it's basically an API mismatch. I suspect @guseggert, @Jorropo and @rvagg might have some opinions on the right way to do this since it's a similar issue to https://github.com/ipfs/go-datastore/pull/188, https://github.com/golang/go/issues/41198 and involves the go-car v2 internals.

Note for the kubo maintainers: IMO sharness tests should either test when the daemon is running or both when running and not running since having a daemon running is the most commonly use for kubo and we have run into issues like this before where there's success when no HTTP API is used, but failures over the HTTP API.

rvagg commented 1 year ago

Not directly related but has a similar smell to https://github.com/ipld/go-car/issues/336; although this one seems like it might be best to fix outside of go-car?

It turns out that feature sniffing in Go isn't as smooth as it could be.

My (likely very naive) take is that perhaps github.com/ipfs/go-ipfs-files/ReaderFile shouldn't implement Seeker, but calls to construct a new ReaderFile could branch on detecting whether the incoming Reader is also a Seeker and return a different type if it does so that Seek() doesn't appear if it can't be used.

endersonmaia commented 8 months ago

I'm having a similar error when using a CARv2 file created with go-car CLI.

I can use ipfs dag import my-file.car when the CAR file that was created using ipfs dag export.

:heavy_check_mark:

❯ echo "my content" > my-file.txt
❯ cid=$(ipfs add -Q my-file.txt)
❯ echo $cid
QmWgVariV9nUzCWGBXfmXvTyTevUmDksycyPv3DAN9BP3A
❯ ipfs dag export $cid > my-file.car
 0s  111 B / ? [--------------------------------------------=--------------------------------------------------------------------------------------------------------------] 519.17 KiB/s 0s
❯ car inspect my-file.car
Version: 1
Roots: QmWgVariV9nUzCWGBXfmXvTyTevUmDksycyPv3DAN9BP3A
Root blocks present in data: Yes
Block count: 1
Min / average / max block length (bytes): 19 / 19 / 19
Min / average / max CID length (bytes): 34 / 34 / 34
Block count per codec:
        dag-pb: 1
CID count per multihash:
        sha2-256: 1
❯ ipfs dag import my-file.car
Pinned root     QmWgVariV9nUzCWGBXfmXvTyTevUmDksycyPv3DAN9BP3A  success

I CAN'T use ipfs dag import my-file.car when I created the file using car create.

:x:

❯ car create -f my-file.car my-file.txt
❯ car inspect my-file.car
Version: 2
Characteristics: 00000000000000000000000000000000
Data offset: 51
Data (payload) length: 203
Index offset: 254
Index type: car-multihash-index-sorted
Roots: bafybeigepy5cjyxbeccayvysewdlbjfy5u3ti4c5fjt47pyae3uzj7jwbu
Root blocks present in data: Yes
Block count: 2
Min / average / max block length (bytes): 11 / 35 / 59
Min / average / max CID length (bytes): 36 / 36 / 36
Block count per codec:
    raw: 1
    dag-pb: 1
CID count per multihash:
    sha2-256: 2
❯ ipfs dag import my-file.car
Error: operation not supported

EDIT: It work's if I use CARv1 when creating the file with go-car CLI

:heavy_check_mark:

❯ car create --version 1 -f my-file.car my-file.txt
❯ car inspect my-file.car
Version: 1
Roots: bafybeigepy5cjyxbeccayvysewdlbjfy5u3ti4c5fjt47pyae3uzj7jwbu
Root blocks present in data: Yes
Block count: 2
Min / average / max block length (bytes): 11 / 35 / 59
Min / average / max CID length (bytes): 36 / 36 / 36
Block count per codec:
    raw: 1
    dag-pb: 1
CID count per multihash:
    sha2-256: 2
❯ ipfs dag import my-file.car
Pinned root bafybeigepy5cjyxbeccayvysewdlbjfy5u3ti4c5fjt47pyae3uzj7jwbu success

rvagg commented 8 months ago

the fact that create results in a v2 is an annoyance to me that's bitten me personally a number of times, I'd like to switch that to v1 by default and save many footguns, it's just a breaking change, so will also annoy another set of users that may depend on it!

ipfs / kubo