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

Create new Certificate and Vote types and use them for Quorum #1967

Merged
merged 18 commits into from
Nov 3, 2023

Conversation

bfish713
Copy link
Collaborator

@bfish713 bfish713 commented Nov 1, 2023

I created mainly 2 new structures: SimpleCertificate and SimpleVote. These implement the most basic vote and certificate pattern.

SimpleVote is some voteable (something we can vote on) data, a signature and a view. It's really important to note that the voteable data is very similar to the old VoteData type except it is no longer an enum. Basically the data member of SimpleVote is the thing that the vote actually votes on (e.g. it is a leaf commitment for the quorum vote) and an implementation of commit. All the voteable implementations of commit add in a tag which commits to which type of vote it is. This prevents forming a QC with different vote types over the same data. (one example would be if we had no votes, both a yes and no vote would be over the same leaf commitment, but the votes would commit to different values).

SimpleCertificate is the basic implementation of a certificate. Like SimpleVote we should be able to use it for all our certificate types (threshould() is the only thing that differs). A certificate is over some voteable data, and commitment to that data.

For votes I created type alias for all the vote types so other code doesn't need to specify the correct Voteable type everywhere in the code.

For certificates I only created the Quorum type because not all the types will use the implementation of threshold in SimpleCertificate I want to follow up to make this function more generic to prevent copypasta of the rest of the Certificate1 trait impl.

I also only implemented the genesis function for the Quorum certificate.

Closes #1965

Copy link
Member

@elliedavidson elliedavidson left a comment

Choose a reason for hiding this comment

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

This really is SO much cleaner than what we had before!

@bfish713 bfish713 marked this pull request as ready for review November 2, 2023 18:27
@bfish713 bfish713 changed the title Bf/quorum vote cert Create new Certificate and Vote types and use them for Quorum Nov 2, 2023
crates/hotshot/src/traits/storage/memory_storage.rs Outdated Show resolved Hide resolved
crates/types/src/simple_certificate.rs Outdated Show resolved Hide resolved
crates/types/src/simple_certificate.rs Show resolved Hide resolved
crates/types/src/simple_vote.rs Show resolved Hide resolved
crates/types/src/simple_vote.rs Outdated Show resolved Hide resolved
crates/types/src/vote2.rs Outdated Show resolved Hide resolved
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.

Thanks for making these changes!

crates/task-impls/src/vid.rs Outdated Show resolved Hide resolved
crates/testing/tests/consensus_task.rs Outdated Show resolved Hide resolved
crates/types/src/simple_vote.rs Outdated Show resolved Hide resolved
crates/types/src/simple_vote.rs Show resolved Hide resolved
crates/types/src/simple_vote.rs Outdated Show resolved Hide resolved
shenkeyao
shenkeyao previously approved these changes Nov 3, 2023
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.

LGTM!

shenkeyao
shenkeyao previously approved these changes Nov 3, 2023
shenkeyao
shenkeyao previously approved these changes Nov 3, 2023
Copy link
Member

@elliedavidson elliedavidson left a comment

Choose a reason for hiding this comment

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

Sorry to be a nit, but a minor change request.

crates/types/src/simple_certificate.rs Outdated Show resolved Hide resolved
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.

LGTM

@bfish713 bfish713 merged commit b071638 into develop Nov 3, 2023
3 of 5 checks passed
@bfish713 bfish713 deleted the bf/quorum-vote-cert branch November 3, 2023 19:25
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.

Create New QC Vote and Certificate
3 participants