Skip to content

Conversation

@rvagg
Copy link
Member

@rvagg rvagg commented Sep 19, 2023

Fix is in ipldutil/traverser.go; without this, an identity CID in a DAG that contains a link to more blocks won't be properly traversed. Unsure how common this is in the real world, most identity CIDs are for chunks that are small enough that they can't fit a CID in but there are instances where people feel it worthwhile to inline chunks much larger than this (e.g.. The Filecoin chain itself has such inline CIDs too.

@rvagg rvagg requested a review from LexLuthr September 19, 2023 10:19
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@975c0f7). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main       #4   +/-   ##
=======================================
  Coverage        ?   64.67%           
=======================================
  Files           ?       69           
  Lines           ?     7519           
  Branches        ?        0           
=======================================
  Hits            ?     4863           
  Misses          ?     2512           
  Partials        ?      144           

Copy link
Collaborator

@LexLuthr LexLuthr left a comment

Choose a reason for hiding this comment

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

Apart from chain data and toor identity CIDs, do we have any other use case where we will find identity CIDs in the DAG?

@rvagg
Copy link
Member Author

rvagg commented Sep 19, 2023

I wish I could be confident in suggesting how common they are but I just don't know. I try to avoid them but some people see them as an efficiency hack so it wouldn't surprise me if they were not uncommon and made up some portion of our "could not load link" errors (there's no clue in error messages currently that would give us a hint, we'd have to rewind the DAG to see).

🤷 So urgency is unknown, I'm just treating this as a TODO I finally got around to in Lassie and found that all 3 protocols had some form of problem with them!

In terms of risk, this PR is very low risk. Doesn't change the happy path and only fixes the identity CID path.

@rvagg rvagg merged commit b1a0d73 into main Sep 19, 2023
@rvagg rvagg deleted the rvagg/identity-traverse branch September 19, 2023 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants