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

[Dynamic Protocol State] Replace dynamic weight and ejection flag with 'epoch participation status' #5039

Merged

Conversation

durkmurder
Copy link
Member

Context

This PR is part of initiate of addressing TODOs in #4649.
Specifically it removes dynamic weight and re-introduces concept of Ejected flag. Instead of using dynamic weight and Ejected flag we introduce a enum epoch participation status which could be one of the next values: active, joining, leaving, ejected.

💡
One interesting details is how we store ActiveIdentities in EpochStateContainer, previously DynamicIdentityEntry was encoding the dynamic part of flow.Identity, specifically flow.DynamicIdentity. I have taken another approach and instead of storing flow.DynamicIdentity and flow.EpochParticipantStatus subsequently I am storing only the Ejected flag. Reasoning for that is from point of view of ActiveIdentities we only care about ejection, not about the 'joining', 'leaving', 'active' states which are derived on the epoch itself not the identity.

@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2023

Codecov Report

Attention: 61 lines in your changes are missing coverage. Please review.

Comparison is base (3984803) 79.71% compared to head (cdfffe9) 56.06%.

Files Patch % Lines
model/flow/identity.go 47.22% 18 Missing and 1 partial ⚠️
utils/unittest/fixtures.go 0.00% 13 Missing ⚠️
cmd/execution_builder.go 0.00% 6 Missing ⚠️
cmd/bootstrap/utils/node_info.go 0.00% 5 Missing ⚠️
state/protocol/util.go 0.00% 4 Missing ⚠️
consensus/hotstuff/committees/leader/cluster.go 0.00% 3 Missing ⚠️
model/flow/protocol_state.go 86.95% 2 Missing and 1 partial ⚠️
cmd/bootstrap/cmd/constraints.go 33.33% 1 Missing and 1 partial ⚠️
consensus/hotstuff/committees/cluster_committee.go 80.00% 1 Missing and 1 partial ⚠️
model/bootstrap/node_info.go 66.66% 1 Missing ⚠️
... and 3 more
Additional details and impacted files
@@                         Coverage Diff                         @@
##           feature/dynamic-protocol-state    #5039       +/-   ##
===================================================================
- Coverage                           79.71%   56.06%   -23.65%     
===================================================================
  Files                                   1      955      +954     
  Lines                                  69    88552    +88483     
===================================================================
+ Hits                                   55    49650    +49595     
- Misses                                 13    35196    +35183     
- Partials                                1     3706     +3705     
Flag Coverage Δ
unittests 56.06% <60.64%> (-23.65%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@AlexHentschel AlexHentschel left a comment

Choose a reason for hiding this comment

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

I think we have some ambiguity currently how we handle the degenerate case of the EpochSetup event containing nodes with InitialWeight=0. I think we allow that, don't we? Previously, those nodes would be filtered out in most cases, due to the requirement to have a positive dynamic weight. But that dynamic weight was replaced by the EpochParticipationStatus. So no, EpochParticipationStatus=EpochParticipationStatusActive with InitialWeight=0 is something to consider.

We could either categorically exclude those nodes from the consensus committees. After all, their InitialWeight is fixed for the epoch and a zero-weighted node cannot contribute to protocol progress. If we included them in the committee, we would essentially add dead weight to the signatures. So conceptually, I think it would be more intuitive to not consider InitialWeight=0 nodes as part of the committee.

But that also introduces extra edge cases. All implementations need to be consistent about filtering out zero-weighted nodes. If we mess that up, we might halt the network. Therefore, I would consider it an acceptable alternative to allow zero-weighted nodes as part of the consensus committee. They would receive an index where their signature could be included in QCs and TCs, despite it being completely useless.

please see my last two comments for further details

model/flow/identity.go Outdated Show resolved Hide resolved
model/flow/identity.go Outdated Show resolved Hide resolved
model/flow/identity.go Outdated Show resolved Hide resolved
model/flow/identity.go Show resolved Hide resolved
cmd/collection/main.go Outdated Show resolved Hide resolved
consensus/hotstuff/committees/cluster_committee.go Outdated Show resolved Hide resolved
consensus/hotstuff/committees/consensus_committee_test.go Outdated Show resolved Hide resolved
durkmurder and others added 15 commits November 23, 2023 17:21
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
• renamed function `constructInitialClusterIdentities` to `constructContributingClusterParticipants`
• moved documentation about zero weight and canonical ordering into the functions main goDoc for better visibility.
…r` I renamed the function that lists the voting cluster participants to `votingClusterParticipants`
@onflow onflow deleted a comment from jordanschalm Nov 25, 2023
• updated documentation
• minor broadening of test coverage
Copy link
Member

@AlexHentschel AlexHentschel left a comment

Choose a reason for hiding this comment

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

This revision had a large blast radius. Very clean changes. Great work.

consensus/hotstuff/committees/leader/cluster.go Outdated Show resolved Hide resolved
cmd/execution_builder.go Outdated Show resolved Hide resolved
consensus/hotstuff/committees/consensus_committee_test.go Outdated Show resolved Hide resolved
model/flow/filter/identity.go Outdated Show resolved Hide resolved
state/protocol/util.go Show resolved Hide resolved
state/protocol/util.go Show resolved Hide resolved
storage/badger/protocol_state_test.go Outdated Show resolved Hide resolved
storage/badger/protocol_state_test.go Outdated Show resolved Hide resolved
storage/badger/protocol_state_test.go Outdated Show resolved Hide resolved
durkmurder and others added 7 commits November 27, 2023 11:03
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
@durkmurder durkmurder merged commit a7b22f6 into feature/dynamic-protocol-state Nov 28, 2023
54 checks passed
@durkmurder durkmurder deleted the yurii/4649-remove-dynamic-weight branch November 28, 2023 10:36
This pull request was closed.
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