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: fr32: Make UnpadReader be a correct Reader #12491

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Sep 19, 2024

Related Issues

Fixes #9324

Proposed Changes

The fr32 unpad reader currently requires that the output is power-of-two-unpadded size, which is an extremely sketchy assumption and seems to cause lots of pain in retrievals.

There is already quite a lot of code in piece-provider which ensures that reads are initialized at correct offsets (multiple of 127-unpadded-bytes), but the read sizes are only ensured by the fact that the output reader is wrapped in a bufio.Reader, which won't always buffer writes with the required size, leading to hard to debug panics.

This PR aims to make the UnpadReader behave more like any normal reader, removing a giant footgun in the retrieval code.

Additional Info

I realize that this code should probably eventually be external to lotus, but right now both boost and curio depend on this for decoding fr32 data. Should be extracted but let's fix the panics first.

Checklist

Before you mark the PR ready for review, please make sure that:

@magik6k magik6k added the skip/changelog This change does not require CHANGELOG.md update label Sep 19, 2024
@rjan90
Copy link
Contributor

rjan90 commented Sep 19, 2024

Looks like there are a couple of lint errors related to not checking the error return value of the rand.Read() that needs to be addressed.

@rvagg
Copy link
Member

rvagg commented Sep 27, 2024

Would be good to also address @ribasushi's findings in https://filecoinproject.slack.com/archives/C06GD1SS56Y/p1727046562422669 - set numcpus low enough and this reliably borks; although it's the same before and after this patch. Something odd going on when you get below 8 CPUs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does not require CHANGELOG.md update
Projects
Status: ✔️ Approved by reviewer
Development

Successfully merging this pull request may close these issues.

boostd dagstore initialize-all crashes Boost - error coming from reader in lotus storage
4 participants