object patch add-link overwrites root node data when child is too small

[ellttBen]

Version information:

go-ipfs version: 0.4.23-
Repo version: 7
System version: amd64/linux
Golang version: go1.13.7

Description:

Hello,

I was trying to add links to other versions of an image to the original's dag node. Though the links are working, accessing the root node yields a child version of the original image instead of the original.

To reproduce, we use two versions of the same image :

• original : QmcFDihqvEwzgahFora3TU1BRZ84UJRSd2PW43j4buSs6n
• smaller version : Qmapcp7ey9eUyk98eEK8iDbu7VdUHVtdd1RVsP5Wvqf4Cp

Then, we run

ipfs object patch add-link QmcFDihqvEwzgahFora3TU1BRZ84UJRSd2PW43j4buSs6n "small.jpg" Qmapcp7ey9eUyk98eEK8iDbu7VdUHVtdd1RVsP5Wvqf4Cp

On my machine, this yields QmQbr75jzhPXWFi59uRUrig6U2paGwfKSe8MLuDwC5s8VL.

We can then observe that the above cid doesn't display the original, but the small.jpg link works and yields the same image. In testing, I have noticed that when the small image is larger (my guess is larger than chunk size), this behavior disappears.

In case the images aren't available :

• original
• small

[Stebalien]

Sorry for the delay, this got lost in the noise.

The short version is that UnixFSv1 (the current go-ipfs filesystem format) simply doesn't support this. Unfortunately, adding support for this is rather difficult without introducing compatibility concerns.

The plan has always been to switch to a "v2" UnixFS based on CBOR objects but work on this has stalled for now.

[ellttBen]

@Stebalien Thanks for taking the time to respond. I've opened another related issue on the go-unixfs repo (ipfs/boxo#390) in which I specify what I think would go into adding support for this feature. If my assumptions are correct, I have a fix ready and can open a PR right away. The proposed solution passes the tests from both repos and seems to fix the bug, if there's any interest.

[Stebalien]

What's your proposal, specifically? The first step here would be to open a PR against the unixfs spec in https://github.com/ipfs/specs/.

[Stebalien]

Ah, I see your proposal. That won't work, unfortunately. It'll work for tiny files that fit into one block, but how would you handle alternative versions for large files?

[ellttBen]

Reading back my explanation, I've failed to highlight the most important aspect : we should stop descending through the DAG when a node has only named links. This condition is expressed by the snippet len(node.Links()[0].Name) != 0, since I've noticed that empty links always come first, but this might be too much of a hack and it might be worth it to go through the entire node.Links() array.
I have tested the solution on root files that fit into one chunk as well as chunked files, all generated by go-ipfs, and it does seem to work. Sorry for the lack of clarity!

Also, I don't know how to translate this into a change of spec, since I've gone about it in the spirit of fixing an implementation bug.

[schomatis]

Per @lidel:

I think if you see a way to throw an error instead of malforming the data, then its fine to fix it with one-liner PR. (Error could say "this api is deprecated, use MFS for now")

If it is more work, I'd say close the issue and pick up and redesign patching under ipfs dag patch (#4782) instead.

Will be working on dag patch and close this issue when it lands.

[BigLep]

dag patch work is happening here: #4782