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

replace all bytes amount-related usage of usize with u64 (and indexing into u64-sized files) #622

Closed
laser opened this issue Apr 18, 2019 · 8 comments
Labels
A-filecoin-proofs bug Something isn't working good first issue Good for newcomers

Comments

@laser
Copy link
Contributor

laser commented Apr 18, 2019

Description

We should definitely use u64 any place where referring to a quantity of bytes. We should definitely use u64 when referring to an index into a structure that could have up to u64 indices.

usize is limited to 4294967295 (in decimal) on 32-bit architectures, which means ~4GiB if we're interpreting that number of as quantity of bytes. We know that sectors will eventually be much larger than 4GiB. In retrospect, the From<UnpaddedBytesAmount> for usize and From<PaddedBytesAmount> for usize impls were a mistake and should be removed.

Acceptance criteria

  • remove From<*BytesAmount> for usize impls
  • modify any place where we use usize for an "amount of bytes" such that it instead uses u64
  • modify any place where we index into a u64-sized container (e.g. the call to sector_base::io::fr32::write_unpadded from get_unsealed_range) to use u64 for the type of the index
@laser laser added bug Something isn't working good first issue Good for newcomers labels Apr 18, 2019
@steven004
Copy link
Contributor

I'm taking this. It is expected to have a version mid. of next week.

@steven004
Copy link
Contributor

@laser , when I look into the system, I found that I need to understand the fr32.rs file first. In the fr32.rs file, there are lots of functions using usize as parameters' type instead of u64.

Is there an introduction of this file, regarding what it is for? I am thinking some of functions should be changed, but others may not be necessary.

@laser
Copy link
Contributor Author

laser commented Apr 29, 2019

@steven004 - The fr32.rs file contains functions used to preprocess (bit pad) client data and to manipulate and inspect the resulting data.

What is preprocessing?

I'll quote the spec:

Preprocessing adds two zero bits after every 254 bits of original data, yielding a sequence of 32-byte blocks, each of which contains two zeroes in the most-significant bits, when interpreted as a little-endian number. That is, for each block, 0x11000000 & block[31] == 0.

Why preprocess?

We do this in order to produce 32-byte aligned sectors in which no 32-byte chunk overflows the field used for the proofs.

@porcuquine
Copy link
Collaborator

Although not directly applicable, I just want to point out that efficient replication of sectors larger than addressable RAM is not a design goal of the system. So, in practice, while we can avoid a class of problems by changing many usize types to u64 — this will be a somewhat illusory gain, since a system which requires that distinction won't be capable of replicating sectors large enough to exercise it.

It might be better just to detect this condition, report it as unsupported, and fail instead.

@steven004
Copy link
Contributor

PR #641 is submitted for this issue.
This pr is not trying to solve all the conflict, but only some to avoid introducing new issues.

I did not touch fr32.rs since it is related to some low-level changes, and lots of changes should be done if we do that. Please advise if we should continue to change more places, or we just limit the program to run on 64-bit system.

@porcuquine
Copy link
Collaborator

I think #652 replaces this. Alternately, we can update this issue to reflect the plan.

Or, we can discuss here whether this is indeed the right approach.

@laser Thoughts?

@steven004 Thanks for all your efforts here: this is a complex and subtle issue which took some time to work through, and we wouldn't have if you had not pushed it forward.

cc: @dignifiedquire

@laser
Copy link
Contributor Author

laser commented May 14, 2019 via email

@dignifiedquire
Copy link
Contributor

@laser can we close this then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-filecoin-proofs bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants