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

Reed solomon erasure code #225

Merged
merged 14 commits into from
Apr 12, 2023
Merged

Reed solomon erasure code #225

merged 14 commits into from
Apr 12, 2023

Conversation

mrain
Copy link
Contributor

@mrain mrain commented Mar 31, 2023

Description

closes: #215

A very naive implementation of Reed Solomon erasure code.

  • Encoding is a very naive polynomial evaluation. It could be accelerated by NTT/FFT which may be already in arkworks repo (PrimeField is also a FftField in ark_ff).
  • Decoding is also a very naive Lagrange interpolation. I don't find an easy way to optimize it yet.

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

@mrain mrain requested a review from ggutoski March 31, 2023 22:56
ggutoski
ggutoski previously approved these changes Apr 3, 2023
Copy link
Contributor

@ggutoski ggutoski left a comment

Choose a reason for hiding this comment

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

API doc could be improved but otherwise ok for now.

primitives/src/erasure_code/mod.rs Outdated Show resolved Hide resolved
primitives/src/erasure_code/mod.rs Outdated Show resolved Hide resolved
primitives/src/erasure_code/reed_solomon_erasure.rs Outdated Show resolved Hide resolved
primitives/src/erasure_code/reed_solomon_erasure.rs Outdated Show resolved Hide resolved
primitives/src/erasure_code/reed_solomon_erasure.rs Outdated Show resolved Hide resolved
@ggutoski ggutoski self-requested a review April 3, 2023 18:26
Copy link
Contributor

@ggutoski ggutoski left a comment

Choose a reason for hiding this comment

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

a couple more questions

primitives/src/erasure_code/reed_solomon_erasure.rs Outdated Show resolved Hide resolved
primitives/src/erasure_code/reed_solomon_erasure.rs Outdated Show resolved Hide resolved
@ggutoski
Copy link
Contributor

ggutoski commented Apr 6, 2023

Now that I've had a chance to think about this PR wrt the VID use case: The level of abstraction here is too high. Encoding/decoding should not automatically split/recombine data into chunks of length reconstruction_size. (I need to do this again anyway in VID.)

I have a good idea of the API I want. Let's chat next week.

@ggutoski
Copy link
Contributor

Ready for review @mrain @chancharles92 .

TODO/Questions:

  1. Proposal: change new() args to be data_size and parity_size so that num_shards = data_size + parity_size and it's impossible to give invalid args to new(). Any thoughts?
  2. Shall we encode in coordinate form or coefficient form as per discussion?

@ggutoski
Copy link
Contributor

ggutoski commented Apr 11, 2023

Current erasure code API looks like this:

fn encode(&self, data: &[F])
fn decode(&self, shards: &[Self::Shard])

where data size and codeword size are implicit and set by the constructor new().

Problem: This API is stateful. Also, there's nothing to stop a user from taking shards from one erasure code and passing them to another (with different data and codeword sizes), so we don't get much argument safety anyway.

Proposal: make encode and decode associated functions (ie. don't take &self):

fn encode(codeword_size: usize, data: &[F]) // data size is data.len()
fn decode(data_size: usize, shards: &[Self::Shard]) // need data_size <= shards.len()

@mrain @chancharles92 any opinions?

ggutoski
ggutoski previously approved these changes Apr 12, 2023
@ggutoski ggutoski merged commit 26a84c0 into main Apr 12, 2023
@ggutoski ggutoski deleted the erasure-code branch April 12, 2023 14:09
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.

Erasure code, RS code
2 participants