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
Simplify client codec #1105
Simplify client codec #1105
Changes from 13 commits
dbd8ea5
b98b07a
f5e1731
f4bd98c
077e73f
6e2c4f2
d2d7bea
416906a
bfe08cd
98b78df
85c558a
366627c
9c6e23a
23f14c9
12cb664
908c518
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.
Let's be super precise in what "encode" means. What's the length of the encoded thing? Is it multiple of 32, or can the last element not be? etc
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.
They could also be interpreted in coeff form right? Like we had in the optimal disperse route at the top of
Also is it really at expanded roots of unity? This part I'm not sure. cc @bxue-l2
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.
This is a tricky question, let me explain my reasoning for wording it this way.
From the perspective of the client, blob polynomials may either be dispersed in
Eval
form, orCoeff
form. Regardless of the form the client chooses, however, the disperser chooses to interpret what it receives as a polynomial inCoeff
form.client: "
EncodePayload
returns a polynomial inEval
form. Depending on my configuration, I may choose to directly disperse the blob inEval
form, or I may convert the polynomial toCoeff
form" first.disperser: "No matter what the client sends me, I will interpret it as
Coeff
form"One argument for this set of definitions is that it yields meaningful names for the enum which determines pre-dispersal processing:
Eval
means don't IFFT, andCoeff
means do IFFT. If we were to defineEncodePayload
, such that what it returns is considered to beCoeff
form in the case of "optimal dispersal", that means the polynomial sent from the client to the disperser is always inCoeff
form. In this case, we would need to think of alternate names for the enum values.Essentially, the question boils down to this: in the alternate "optimal" dispersal pathway, at what point in time do we declare that the non-iffted bytes are in
Coeff
form? Does the client make this declaration, or does the disperser? I chose for the disperser to make this declaration in my descriptions, but I think it's a pretty arbitrary decision. We just need to come to consensus, and stick to it.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 think so, but confidence is lacking. Waiting for clarification from @bxue-l2
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.
Awesome summary, let's definitely chat about this over standup with everyone, there's a lot going on which I need to think about more deeply.
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.
Regarding the roots_of_unity, having fought in our rust PRs over the meaning of
primitive_roots_of_unity
,roots_of_unity
,expanded_roots_of_unity
, I think the semantics is something like:expanded
, each generate a group of order 2^index of that PROU
(2^0, 2^1, 2^2, etc.)So if my current understand is correct (cc @bxue-l2 @anupsv), then we should not refer to primitive/expanded in these kinds of docs, just
roots_of_unity
.expanded
just refers to the procedure we have to take because of the form we decided to cache the roots in. We could also just have stored every single root of unity for every power directly in the binary and used those directly without needing toexpand
a primitive root.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.
yes that is right.
Having roots_of_unity and expanded_roots_of_unity are sufficient.
See wiki.
which are 1, 2^(2^1), 2^(2^2), 2^(2^3)....2^(2^28). The exponent is power of 2 because it needs to work with FFT
Expanded ROU based on 2^(2^1) is [1, w, w^2, w^3]
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 think I have a slight different idea on the following statement
The client can decide which form it wants the EncodedPayload to be interpreted. If CoeffForm, then the EncodedPayload will go to the optimal path, otherwise in the EvalForm, the EncodedPayload needs to undergo IFFT
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.
If we're going to be using
blob
here, then we have to be 100% precise and link to its definition somewhere in the codebase.Also what does "system is configured mean"? Isn't the system eigenda? And eigenda always interprets blobs as being in coeff form?
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.
Do we have any precise definitions of a blob in the codebase yet?
I intended this to mean "however the various clients are configured to disperse and retrieve blobs". Do you think it is better if I say:
"If clients are configured to distribute blobs in Coeff form,..."?
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.
Yes, I think just using "clients are configured" is cleaner.
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.
Might not have precise definition of blob in the codebase. Ideally we'll have a spec soon enough that we can point to though perhaps
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.
My definition of a blob is the non blobHeader part that is received by the disperser. If the length of the received part is not power of 2, we artificially pad it so. So blob is always power of 2
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'd prefer if this was in a self-contained function. Maybe we can change RemoveEmptyByteFromPaddedBytes to take a second
len
argument?This file was deleted.
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.
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.
this is a bit weird. Why not just always test the same number of iterations?
This file was deleted.
This file was deleted.