Minor change for sector_bytes to use u64 type

[steven004]

No description provided.

[dignifiedquire]

Did you run into an actual issue with this, or what is the motivation behind this?

[dignifiedquire]

Side Note: in general we should move all external visible number types to u64 rather than using usize, to ensure consistency.

[steven004]

Side Note: in general we should move all external visible number types to u64 rather than using usize, to ensure consistency.

I think I mean the same thing as you mentioned. Here is some reasons simiar as what I put into the commit notes:

1. the original definition of sector size in sector-base\src\api\disk_backed_storage.rs is u64 type;
2. In the internal.rs seal function it convert to usize first, and then re-convert to u64 when using it. I would think it's better to use u64 to keep consistency.
3. the change in setup_params is to keep the consistency too.

Currently, we do not see an issue since most of our system are 64-bit system, the usize is the same as u64, and the sector-size is defined as 256M (1<<28). No issues seen. However, there could be potential issue in the future, if (very low possibility) the sector size is getting much larger in some system, info might loss when convert u64 to usize and reconvert back (in a 32-bit system).

I might think too much of it since this may never happen. The real motivation of this proposal is to keep the consistency, I would agree.

[steven004]

Did you run into an actual issue with this, or what is the motivation behind this?

No real issues found. You may ignore and close this PR since you are polishing the code as planned.

[dignifiedquire]

@laser I believe you wrote this code, was there any intent here that we should know about?

[laser]

@laser I believe you wrote this code, was there any intent here that we should know about?

This code was written mostly by @porcuquine. That said, @steven004's rationale is sound and your recommendation to favor u64 over usize is a good one as well.

I will approve this PR once the build turns green. Please ping me, @steven004, when you've resolved the build issue.

[porcuquine]

I think the underlying issue is that we use usize to index into sectors internally. If we want to fully propagate this change, we would switch to u64 everywhere.

[steven004]

@laser I believe you wrote this code, was there any intent here that we should know about?

This code was written mostly by @porcuquine. That said, @steven004's rationale is sound and your recommendation to favor u64 over usize is a good one as well.

I will approve this PR once the build turns green. Please ping me, @steven004, when you've resolved the build issue.

It should be solved already, please check. thanks.

[laser]

@porcuquine

If we want to fully propagate this change, we would switch to u64 everywhere.

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.

So, I agree with @porcuquine. We should fix this issue. @steven004 - are you up for the challenge?

---

Places where we use usize in internal.rs and its direct dependencies:

• internal.rs public_params expects usize for partitions
• internal.rs generate_post_fixed_sectors_count uses a usize to create a Vec (don't need to change this)
• MultiProof#new_from_reader expects usize for partitions (don't need to change this)
• internal.rs lookup_verifying_key formats a usize created from bytes amount for VK file name (this should be a u64)
• sector_base::io::fr32::write_unpadded expects a usize for offset (this should be a u64)

[laser]

I've added an issue for this here.

[steven004]

@laser

So, I agree with @porcuquine. We should fix this issue. @steven004 - are you up for the challenge?

Sure, I could take care of this. I would suggest to merge this PR first, which is quite independent, and next, I will go through all other places related regarding this and do corresponding change, there would be more test done before submit.

[dignifiedquire]

@steven004 thanks for the work, but this got pretty stale, so I am closing this. We are planning to add a compile time check for 64-bit at some point, as the current construction is far away from being portable to a 32-bit system.

[steven004]

Understood. that is better option.