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

fix: BlockMetadata#Offset should be for section, not block data #497

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Sep 6, 2023

@willscott while the CARv2 offsets were completely broken prior to #491 and I made it match for both CARv1 and CARv2; I changed the meaning of "Offset" in the process, making it the data offset, not section offset. Unfortunately because there were no tests or docs in there the intention wasn't clear so I made an assumption. I just discovered this after updating go-car in vanilla Boost and getting the section read errors ("bad cid version").

So this is a bugfix, to change it back to section offset but also make both CARv1 and CARv2 work (+ docs + tests).

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.

it's interesting that this is actually added complexity

  • since boost metadata is storing the size information as well so that it can make an appropriately sized reader it actually could make use of a data rather than section offset.

this is probably the right way to go for compatibility

@rvagg
Copy link
Member Author

rvagg commented Sep 6, 2023

09d23db#diff-fc451b22af005ef3ae74c133e1c434be409f228975d20ade449c92eeb3341c3aL219 Size appears to be as intended, the length of the block data, not section; which is helpful because with Size and Cid you can derive the length of the section too. So I think we're good with this.

@rvagg
Copy link
Member Author

rvagg commented Sep 6, 2023

Just tried this commit in boost and what I was tinkering with is passing now.

In the current boost, Size is only used for BlockstoreGetSize which is the right number. There's no LimitReader at the moment for reading the section, it just seeks to the offset and does a ReadNode to slurp in the section.

I'm thinking that a follow-up could add a bunch more information - rather than having to derive the block data offset from cid length + varint(cid length + block length) you could just have it.

But maybe see what we need in Boost first? This might make it simpler over there because there's already some calculation going on - in one of my changes to your branch I introduced a section offset calculation based on the offset, that can go away now.

@willscott
Copy link
Member

agree - happy to wait and see if we need more from the boost perspective

@rvagg rvagg merged commit 86d6af2 into master Sep 6, 2023
17 checks passed
@rvagg rvagg deleted the rvagg/fix-block-offsets branch September 6, 2023 12:06
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