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

Empty identity CID should be indexed when options are set #316

Merged
merged 1 commit into from
Jul 6, 2022

Conversation

masih
Copy link
Member

@masih masih commented Jul 6, 2022

There is an edge-case where if the storing identity CIDs are enabled in
a CARv2, one could technically store an empty identity CID which ends up
with a singlewidthindex width of 8.

Such CAR files indeed exist out there and the validation changes
introduced in 2.4.0 means such CAR file indices are no longer readable
, even if regenerated.

The question is should this be considered a valid index/readable index?

@masih
Copy link
Member Author

masih commented Jul 6, 2022

Cc @nonsense @dirkmc

@masih masih requested a review from willscott July 6, 2022 15:35
There is an edge-case where if the storing identity CIDs are enabled in
a CARv2, one could technically store an empty identity CID which ends up
with a `singlewidthindex` width of 8.

Such CAR files indeed exist out there and the validation changes
introduced in `2.4.0` means such CAR file indices are no longer readable
, even if regenerated.

The question is should this be considered a valid index/readable index?
@masih masih force-pushed the masih/v2-idx-id-empty-digest branch from 44f424a to 853cb36 Compare July 6, 2022 15:37
Copy link
Member

@willscott willscott left a comment

Choose a reason for hiding this comment

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

seems fine. not the index's problem to enforce constraints to not do silly things at other layers of the stack

@masih masih merged commit b2e14f0 into master Jul 6, 2022
@masih masih deleted the masih/v2-idx-id-empty-digest branch July 6, 2022 16:31
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.

2 participants