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

[BFT] Epoch Recovery integration test #6823

Conversation

durkmurder
Copy link
Member

#6645

Context

This PR implements the most general case where Random beacon committee and consensus committee form a symmetric difference with cardinality 1. More details in the attached issue.

@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 6.25000% with 30 lines in your changes missing coverage. Please review.

Project coverage is 41.72%. Comparing base (a35632f) to head (4603a98).

Files with missing lines Patch % Lines
cmd/bootstrap/run/epochs.go 0.00% 30 Missing ⚠️
Additional details and impacted files
@@                   Coverage Diff                    @@
##           feature/efm-recovery    #6823      +/-   ##
========================================================
- Coverage                 41.81%   41.72%   -0.09%     
========================================================
  Files                      1588     2033     +445     
  Lines                    144098   181223   +37125     
========================================================
+ Hits                      60251    75613   +15362     
- Misses                    78887    99381   +20494     
- Partials                   4960     6229    +1269     
Flag Coverage Δ
unittests 41.72% <6.25%> (-0.09%) ⬇️

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.

3000,
false,
dkgIndexMap,
dkg.KeyShares()[:nConsensusNodes],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dkg.KeyShares()[:nConsensusNodes],
dkgKeyShares,

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.

Thanks. Amazing work. My suggestions are mostly focused on clarifying and documenting the intricacies of this test.

cmd/bootstrap/run/epochs.go Outdated Show resolved Hide resolved
cmd/bootstrap/run/epochs.go Outdated Show resolved Hide resolved
Comment on lines 365 to 367
// At this point we have a node which is part of the consensus committee but not part of the Random Beacon committee and
// another node which is part of the Random Beacon committee but not part of the consensus committee.
txArgs, err := run.GenerateRecoverTxArgsWithDKG(
Copy link
Member

Choose a reason for hiding this comment

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

If we move step 2 to here, the test would be easier for me to understand as it would more closely emulate a real-world scenario.

If that is possible, we could summarize the test as follows in the documentation (changes highlighted in bold font)

This test will do the following:

  1. Triggers EFM by turning off the sole collection node before the end of the DKG forcing the DKG to fail.
  2. Eject the first consensus node by modifying the epoch snapshot.
  3. Drop the last consensus node from the Random Beacon committee. This hack works only for threshold systems with an even number of participants,
    without changing the threshold - hence we need to start this test with 4 consensus nodes.
  4. Generates epoch recover transaction args using the tooling [run.GenerateRecoverTxArgsWithDKG] provided for the governance committee.
  5. Submit recover epoch transaction.
  6. Ensure expected EpochRecover event is emitted.
  7. Ensure the network transitions into the recovery epoch and finalizes the first view of the recovery epoch.

durkmurder and others added 2 commits January 6, 2025 13:03
@durkmurder durkmurder merged commit 2e44aa0 into feature/efm-recovery Jan 6, 2025
54 of 55 checks passed
@durkmurder durkmurder deleted the yurii/6645-consensus-dkg-symmetric-difference-test branch January 6, 2025 13:02
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