Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(content): update filecoin piece content #1016
fix(content): update filecoin piece content #1016
Changes from 6 commits
9d32675
079b8c4
ba67553
c6cb3e9
13e33f3
51171eb
49a025b
7db9051
972763d
7877dd3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this para has gone through some iterations and lost the initial ordering in an attempt to make it easier to explain - which I understand, but it's now not quite accurate. The final size needs to be a ^2 but the fr32 padding gets in there before it goes to ^2. So to calculate how much zero padding to add, you have to account for the 254/256 inflation that's going to occur.
I think in an earlier iteration it was suggested to switch the order of the steps and say that fr32 happens first and then inflation to ^2 after that. I think this is the ideal framing because it makes it easier to understand (even though in code it happens the other way around, it doesn't actually matter so it's worth doing what's best for descriptive purposes). So that would mean pulling the last sentence to be first, "A process called
fr32 padding
which adds ... is applied. Extra padding is applied to the end of the data, adding zero bytes until the resulting blob is a power of two sie.". Something like that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @rvagg! I've tried to explain it both ways, but indeed it becomes tricky, you're right. I've updated in commit 49a025b and included only the easier to grasp process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to mention that I've removed the definition of
fr32
from here and included it in the Glossary (outstanding PR atm).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just noticing this now. I'm commenting here without an entirely clear point, but so it doesn't get lost in the context shift:
Fr32
is well-defined in therust-fil-proofs
implementation. As far as I can tell from the glossary, it's not given a substantive definition there. It's use here as an adjective was never particularly well-defined (from what I saw), although some true statements were made justifying the adjective.Without making a specific suggestion for how to untangle this, let me offer the definition underlying the type called
Fr32
inrust-fil-proofs
.Fr32
is a 32-bit representation of a field element (which, in our case, is now, is the arithmetic field of BLS12-381). To be well-formed, a value of typeFr32
must actually fit within that field, but this is not enforced by the type system. It is an invariant which must be perserved by correct usage.In the case of so-called
Fr32
padding, two zero bits are inserted 'after' a number requiring at most 254 bits to represent. This guarantees that the result will beFr32
, regardless of the value of the initial 254 bits. This is a 'conservative' technique, since for some initial values, only one bit of zero-padding would actually be required.It's possible that none of this needs to be spelled out explicitly here. It's not clear that an 'official' definition of the term makes much sense without at least some of this context, though. I've put this comment here rather than there because it's in the context of this conversation that the added detail is obviously relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should note that
Fr32
is not obviously a concept which belongs in the spec, even though it seems to have made its way into the preferred name for what I historically called 'bit padding'. I don't mind if it does stay in the spec as a glossary term. I mention this just to clarify that its primary definition is as an implementation type with relatively elaborate characteristics.In particular, it's important to understand that
Fr32
-padded data represents a smaller set of values than inhabit theFr32
type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@porcuquine this is useful input. I have added your definition above as an extra "important note" in this section (see commit 972763d) and will also add this as a definition in the glossary too, if you don't mind.
I generally tend to agree. However, if we don't mention anything specific to
Fr32
, then we should just mention what is in one of the notes: "any implementation has to make sure that the initial IPLD DAG is serialised and padded so that it gives a clean binary tree, and therefore, calculating the Merkle root out of the resulting blob of data gives the same Piece CID", and leave it to that, without any mention of padding, .car etc.I suggest leaving the text as is for now, given we mention that all this is Lotus-specific and the alternative would be slightly confusing to describe. Let me know if you're ok with this.
This file was deleted.
This file was deleted.
This file was deleted.