Skip to content
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

feat: multiplicity depend on payload size #670

Merged
merged 40 commits into from
Sep 3, 2024
Merged

feat: multiplicity depend on payload size #670

merged 40 commits into from
Sep 3, 2024

Conversation

ggutoski
Copy link
Contributor

closes: #663

This PR:

This PR replaces #667. Thanks to @akonring for getting this PR started.

The fix for issue #663 has many knock-on tasks, most of which are also resolved here.

  • multiplicity arg for Advz constructor is now max_multiplicity. The user no longer specifies the multiplicity that is used for all payloads. Instead, the user specifies the maximum multiplicity that may be used. The actual multiplicity is now selected at disperse-time as a function of payload size. Thus, as per Fix large shares when small payload and large multiplicity #663, a small payload with large multiplicity will no longer have wastefully-large share size.
  • new test max_multiplicity that ensures small multiplicity for small payloads
  • As noted in feat: let multiplicity depend on payload size #667 (comment), it is insufficient merely to set multiplicity at disperse-time. We must also set the size of the FFT domains at disperse-time.
  • Prior to this PR those domains were stored in fields eval_domain, multi_open_domain of the Advz struct. Now that these domains are set at disperse-time there is no need to store them in the Advz struct, so I removed them. They are now computed on-the-fly as needed, just like multiplicity.
  • Implementing these changes was tedious. It became clear to me that we have some tech debt. Thus, I also did some code clean-up in this PR. This clean-up did not cost much extra time because I had to do 90% of the work anyway just to implement the changes needed for this PR.
  • Tech debt:
    • disperse_precompute is mostly copy-pasted code from disperse. I refactored these two functions into disperse_with_polys_and_commits.
    • Lots of low-level helpers were messy: polynomial, polynomial_internal, bytes_to_polys, etc
  • Posted new issue ADVZ: drop the restriction that recovery_threshold, multiplicity must be powers of two #668 and linked to it in several places in code comments.

This PR does not:

Key places to review:


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added relevant changelog entries to the CHANGELOG.md of touched crates.
  • Re-reviewed Files changed in the GitHub PR explorer

akonring and others added 26 commits August 23, 2024 13:41
This commit defines a function `min_multiplicity` which can compute the actual multiplicity that will be used from `max_multiplicity` and `payload_len`. The original argument `multiplicity` has been renamed to `max_multiplicity` to indicate that this is an upper bound.
akonring
akonring previously approved these changes Aug 30, 2024
Copy link
Contributor

@akonring akonring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
Made a few comments (mostly nits so feel free to ignore if they are out of scope or don't apply).

vid/src/advz/payload_prover.rs Show resolved Hide resolved
vid/src/advz/test.rs Outdated Show resolved Hide resolved
vid/src/advz/test.rs Outdated Show resolved Hide resolved
vid/src/advz.rs Outdated Show resolved Hide resolved
vid/src/advz.rs Outdated Show resolved Hide resolved
vid/src/advz.rs Outdated Show resolved Hide resolved
vid/src/advz.rs Show resolved Hide resolved
vid/src/advz.rs Outdated Show resolved Hide resolved
vid/src/advz/payload_prover.rs Outdated Show resolved Hide resolved
vid/src/advz/payload_prover.rs Outdated Show resolved Hide resolved
@ggutoski ggutoski requested a review from akonring August 30, 2024 20:36
Copy link
Contributor

@akonring akonring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ggutoski ggutoski merged commit 3195eb0 into main Sep 3, 2024
5 checks passed
@ggutoski ggutoski deleted the ak/663 branch September 3, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix large shares when small payload and large multiplicity
2 participants