fix(storage-provider): registration extrinsic fix

[cernicc]

Description

The bug happens because we are trying to convert 64-bit value to the 32-bit. Code example is in the comment. The solution bounds the wpost_proving_period to the u32, so we know the maximal wpost_proving_period passed. The proving period is later used to mod the offset. Because we mod the offset with the u32 number, we know that the offset can later be saturated into the BlockNumber.

The solution can also be that we change the configured BlockNumber to the u64 and fix any config related issues. Hmm. This might be even better. If I understand the substrate, we choose the primitive types on which we depend on

Checklist

• Make sure that you described what this change does.
• Have you tested this solution?
• Were there any alternative implementations considered?
  Yes. Many. It gets complicated fast :D This was the simplest aproach

[th7nder]

I don't think that's a good idea. We express time units in terms of blocks that passed, so the types must match the BlockNumber from the conceptual point of view.

[cernicc]

Hmm. I don't have this feeling. This is something similar as

type PlanetAgeYears = u64;
type UserAgeYears  = u8;

Both represent the distance on the same scale. Only the maximum boundary of that distance is different.

Suppose we implemented BlockDelta representing the distance between two blocks. In that case, The BlockDelta would have to be the same type as BlockNumber. These numbers are not BlockDelta because they can't contain an arbitrary distance between blocks. They are specified by us.

[th7nder]

https://github.com/search?q=repo%3Aparitytech%2Fpolkadot-sdk%20DAYS&type=code -> when you look for it in the SDK code, they all use BlockNumber for this stuff.

[cernicc]

Hmm. We can also change the BlockNumber to u64 and fix all config related issues. That would also solve this issue.

[jmg-duarte]

Seems to me that moving to u64 would also avoid the casts

[aidan46]

I don't think saturated_into is good to use. See the comment on this post. You can do it with saturated_into(), but as soon as your block number is past 255, the value returned would be stuck at 255, and probably not very useful.

[cernicc]

I think this comment was written as the answer for Has anyone been able to convert substrate blocknumber to let's say a u8?

[jmg-duarte]

I don't understand why this fixes the issue, it seems to me that this implementation may hide another bug in the worst case.

#136 doesn't explain the test situation under which the bug was found and this PR doesn't provide a test ensuring it works either.

It's lacking a lot of context, please cover the context in the PR description explaining the issue and why this actually fixes it, as well as add tests.

[th7nder]

The original bug happens, because:

let digest = blake2_64(&addr);
// We're creating a 64-bit hash of Account address and BlockNumber
// To create a pseudo-random proving period.
let offset = u64::from_be_bytes(digest);
// We're trying to convert 64-bit bytes into 32-bit value (BlockNumber is defined as u32 in runtime)
let mut offset = TryInto::<BlockNumber>::try_into(offset).map_err(|_| ProofError::Conversion)?;
// So the try_into fails.

[cernicc]

@jmg-duarte Added a better description. If you have any more questions let me know.

[jmg-duarte]

LGTM but I wonder if we should move to BlockNumber = u64

[jmg-duarte]

Seems to me that moving to u64 would also avoid the casts

[cernicc]

LGTM but I wonder if we should move to BlockNumber = u64

I'll move it to u64 tomorrow. I have a feeling that the solution will be better that way.