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

[TECH DEBT] Refactor memberships, remove non-staked nodes #3653

Merged
merged 10 commits into from
Sep 10, 2024
Merged

Conversation

rob-maron
Copy link
Collaborator

@rob-maron rob-maron commented Sep 5, 2024

This PR:

Refactors memberships to:

  • Provide a better API
  • Be more verbose (particularly when it comes to the quorum leader == DA leader hack we are doing)
  • Remove the presence of non_staked nodes

It also removes non-staked node support from HotShot as a whole. We don't use the permissioned builder anymore, so it is increasing complexity on HS and the sequencer

Why do we need these changes?

While removing support for non-staked nodes, I remembered a few issues we had with memberships in the past; the primary one being that they are easy to get wrong. One time in the sequencer we constructed the DA membership with DA nodes (as would make sense), but this lead to an issue where DA votes were going to the wrong leaders. This makes things much more verbose

crates/example-types/src/node_types.rs Show resolved Hide resolved
crates/hotshot/src/lib.rs Show resolved Hide resolved
crates/hotshot/src/traits/election/static_committee.rs Outdated Show resolved Hide resolved
crates/hotshot/src/traits/election/static_committee.rs Outdated Show resolved Hide resolved
crates/hotshot/src/traits/election/static_committee.rs Outdated Show resolved Hide resolved
crates/hotshot/src/traits/election/static_committee.rs Outdated Show resolved Hide resolved
crates/testing/src/helpers.rs Show resolved Hide resolved
crates/testing/src/test_runner.rs Show resolved Hide resolved
@rob-maron rob-maron changed the title Refactor memberships Refactor memberships, remove non-staked nodes Sep 6, 2024
@rob-maron rob-maron marked this pull request as ready for review September 6, 2024 16:27
@rob-maron rob-maron requested a review from bfish713 as a code owner September 6, 2024 16:27
@rob-maron rob-maron changed the title Refactor memberships, remove non-staked nodes [TECH DEBT] Refactor memberships, remove non-staked nodes Sep 6, 2024
Copy link
Contributor

@jparr721 jparr721 left a comment

Choose a reason for hiding this comment

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

couple of things

@bfish713
Copy link
Collaborator

bfish713 commented Sep 6, 2024

Since you are already in here I want to pose some questions, why do we even have a DA membership at all? Why do we even have a DA leader concept at all, the leader is just the leader of a view. For DA even we theoretically could just send votes to whoever sent the proposal regardless of view/leadership. So can we just combine the memberships to just be 1 singular membership that is used everywhere and a DA committee is just a part of it

@rob-maron
Copy link
Collaborator Author

rob-maron commented Sep 6, 2024

Since you are already in here I want to pose some questions, why do we even have a DA membership at all? Why do we even have a DA leader concept at all, the leader is just the leader of a view. For DA even we theoretically could just send votes to whoever sent the proposal regardless of view/leadership. So can we just combine the memberships to just be 1 singular membership that is used everywhere and a DA committee is just a part of it

Good question, I also thought this. I actually had gotten it to be almost removed in this PR, but didn't want to touch voting logic too much. I am 100% in favor of combining them though

EDIT: We have decided to keep them this way for now, making the necessary changes for the above later. It comes down to a high coupling with voting/membership types

@rob-maron rob-maron requested a review from jparr721 September 9, 2024 14:24
@rob-maron rob-maron merged commit 8d0fca6 into main Sep 10, 2024
33 of 35 checks passed
@rob-maron rob-maron deleted the rm/election branch September 10, 2024 19:16
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.

4 participants