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: VID take iterator instead of slice #381

Merged
merged 30 commits into from Oct 9, 2023
Merged

feat: VID take iterator instead of slice #381

merged 30 commits into from Oct 9, 2023

Conversation

ggutoski
Copy link
Contributor

@ggutoski ggutoski commented Oct 5, 2023

Description

closes: #375
closes: #300

  • conversion.rs: add new bytes_to_field and field_to_bytes that act on iterators instead of slices. The code is more complex than I like. But if you trust the tests then you don't need to get too deep into it. The cause of this complexity is that we need to append an extra field element at the end to encode some length information in order to enable inversion of this process.
    • These methods would be much simpler if we did not need to encode this metadata. But that would make these functions useless because the caller must know in advance how many bytes are in the iterator so he can truncate the decoded bytes accordingly. Since we're now using iterators, not slices, it seems unreasonable for the caller to know this.
    • These methods work only on PrimeField, not Field. We could extend it to Field but I don't want to take extra time for that now.
  • I did not delete the existing functions bytes_to_field_elements, bytes_from_field_elements that act on slices. But we could delete them if needed. (Currently they're still used in plonk.)
  • Update VID API to take iterators instead of slices, use the new conversion methods.
  • Add a new elems_len field to Common struct in VID. This data is needed because recovery adds extra zero elements via RS decoding. Previously we did not need this because the existing bytes_to_field_elements encodes the total byte length in the output. Thus, bytes_from_field_elements automatically drops any extra zero elements added by RS decoding. By contrast, the new bytes_to_field encodes only the byte length of the final element (not the total byte length). So now VID needs to keep track of how many zero elems were added by RS decoding.

Other stuff snuck into this PR

  • clean itertools dependency
  • add new derivations for VidDisperse struct that are useful downstream

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 a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@ggutoski ggutoski requested a review from mrain October 5, 2023 21:45
impl<P, T, H, V> VidScheme for GenericAdvz<P, T, H, V>
where
P: UnivariatePCS<Point = <P as PolynomialCommitmentScheme>::Evaluation>,
P::Evaluation: FftField,
Copy link
Contributor

@mrain mrain Oct 6, 2023

Choose a reason for hiding this comment

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

Any specific reason for making it PrimeField?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was in a rush. 😊 I'm happy to extend it to Field if you request it. That gives me an excuse to allocate the time to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The answer to your question is here:

// `PrimeField` needed only because `bytes_to_field` needs it.
// Otherwise we could relax to `FftField`.

This #381 (comment) is an explanation for why I restricted bytes_to_field to PrimeField.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. We can settle with PrimeField for now, and add a follow-up issue for the future improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

primitives/src/vid/advz.rs Outdated Show resolved Hide resolved
mrain
mrain previously approved these changes Oct 6, 2023
Copy link
Contributor

@mrain mrain left a comment

Choose a reason for hiding this comment

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

LGTM. We can merge it after some follow-up issues and CI fixes.

@mrain mrain merged commit c0b8842 into main Oct 9, 2023
5 checks passed
@mrain mrain deleted the gg/conversion-iter branch October 9, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants