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
Create witness and blob preloader #31
Create witness and blob preloader #31
Changes from 18 commits
bdf0f34
bff6d13
298793d
1c3f2c0
a1c9374
755a5c8
be6e64e
14aad44
4ec8403
99c1406
bb54f9d
967332c
221d829
362cfb5
53bf291
d7f6ace
90e93ff
4cdf6ca
38103d4
7e03e25
c0e96d2
24bd4f9
4ffa71f
641fb41
067b4c4
e847a16
27670ae
d90799b
e066250
fd01cfa
6eac168
5c0be2d
384ec0b
462285d
7793fb6
62c2668
108cea0
7382e45
b6609ff
4854613
ab4b9ec
f98c543
97dab19
0ee38dc
bcfe850
e5b0f67
5bfbdf1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
wdym by "inside the blob header"?
Also is there anything preventing this other implementation right now? Or is it just that its harder and you didn't want to do it now?
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 believe @litt3 is working on it in proxy
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.
@bxue-l2 Are you referring to the addition of
pi
to theBlobCommitment
?That's on my list of TODOs, but not at the top, FYI
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.
don't think it is urgent
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 an issue tracking that? best to link to that issue than to leave vague comments. Then people know exactly what's up and how to track the progress.
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 is an issue, but I suppose the question is whether it's appropriate to link private linear issues in a public forum. Seems a bit odd
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.
Definitely, which is why we should always be creating issues on github first, and linear will import them. Isn't this the issue? Layr-Labs/eigenda#1037
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.
Why are you loading 256MiB?
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.
Also you should try to get into the habbit of not .unwrap()ing in functions that return a Result.
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.
268435456 is the number of SRS, see issue here, Layr-Labs/rust-kzg-bn254#46
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.
then you have to convert your error into one of kzg error. Simpler just abort, and let user fix the proof issue, as there is no point to continnue
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.
Then why return an error at all? just panic everywhere
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.
also comented on that issue. Still don't understand how we are loading 256MiB from a file that is supposed to be 32MiB large.
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.
Because some error can be handled by the upper layer and is acceptable by the application, whereas in some other cases, it just has to break. I guess your point is that it should only breaks at the highest level for clean shutdown
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.
we only load 1024 points in the code above
268435456 is not the number of point we are going to load, it is just some number we make sure 1024 is not greater than 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.
ahhhhh I see I totally misunderstood SRS::new()'s signature then.
My question remains then though, why are you setting 268435456 and not 1024?
And if order is not needed, can we just get rid of 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.
we should just get rid of 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.
don't we have a constructor that doesn't requiring making kzg mutable and then modifying it in place?
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 don't think so
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.
would be good to add