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

VID share distribution algorithm (2) #2163

Merged
merged 11 commits into from
Dec 8, 2023

Conversation

rob-maron
Copy link
Collaborator

@rob-maron rob-maron commented Dec 5, 2023

Closes #2104

This PR:

  • Changes the number of shares and chunk size to:
    • Conform to VID changes (power of 2)
    • Use the quorum membership
  • Adds a constructor for VidDisperse that takes a membership for share distribution
  • Implements an algorithm for the constructor to deterministically disperse VID shares based on index
  • Changes the testing function vid_init to use the constructor so it's easy to use during tests
  • Edits the tests to conform to this

This PR does not:

  • Change any other VID functionality
  • Address networking or actual distribution of shares

Key places to review:

vid.rs - where we do the actual dispersal
data.rs - where we specify what algorithm to use for dispersal given membership and the shares

This PR augments and fixes #2149 by modifying the vid_commitment function. All the relevant (unapproved) changes are in the second commit "second set of changes" and the commit "remove quorum membership from DA".

@rob-maron rob-maron changed the title VID share distribution algorithm: take 2 VID share distribution algorithm (2) Dec 5, 2023
@rob-maron rob-maron marked this pull request as ready for review December 5, 2023 23:16
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.

There's a couple design questions we need to discuss sometime. Not sure whether we need to resolve these questions before merging this PR.

crates/task-impls/src/events.rs Outdated Show resolved Hide resolved
crates/task-impls/src/network.rs Outdated Show resolved Hide resolved
crates/types/src/traits/block_contents.rs Outdated Show resolved Hide resolved
crates/types/src/traits/block_contents.rs Show resolved Hide resolved
ggutoski
ggutoski previously approved these changes Dec 8, 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.

It's a thing of beauty!

crates/types/src/traits/block_contents.rs Show resolved Hide resolved
@rob-maron rob-maron changed the base branch from develop to main December 8, 2023 15:27
@rob-maron rob-maron dismissed ggutoski’s stale review December 8, 2023 15:27

The base branch was changed.

@rob-maron rob-maron requested a review from ggutoski December 8, 2023 16:06
Copy link
Member

@shenkeyao shenkeyao left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the patience for being back and forth regarding the quorum membership.

@rob-maron rob-maron merged commit 81722f1 into main Dec 8, 2023
7 of 8 checks passed
@rob-maron rob-maron deleted the rm/vid-share-distribution-algo-take2 branch December 8, 2023 21:52
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.

[T4.0] - VID share distribution
4 participants