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

DKG Result Submission Backward Compatibility #6827

Open
wants to merge 3 commits into
base: feature/efm-recovery
Choose a base branch
from

Conversation

jordanschalm
Copy link
Member

@jordanschalm jordanschalm commented Dec 19, 2024

This PR implements backward-compatibility for DKG result submission. Addresses #6792

The implementation here uses differences in the smart contract to decide which transaction to use for result submission. Compared to the protocol-version-based approach described in the issue, this has the benefit that the smart contract upgrade does not need to be coordinated with the Protocol HCU.

@jordanschalm jordanschalm marked this pull request as ready for review December 19, 2024 20:01
@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.22%. Comparing base (a35632f) to head (d9604c7).

Additional details and impacted files
@@                    Coverage Diff                    @@
##           feature/efm-recovery    #6827       +/-   ##
=========================================================
- Coverage                 41.81%   41.22%    -0.59%     
=========================================================
  Files                      1588      400     -1188     
  Lines                    144098    39367   -104731     
=========================================================
- Hits                      60251    16231    -44020     
+ Misses                    78887    21807    -57080     
+ Partials                   4960     1329     -3631     
Flag Coverage Δ
unittests 41.22% <ø> (-0.59%) ⬇️

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.

👍

// submitResult_ProtocolV1 submits this node's locally computed DKG result (public key vector)
// to the FlowDKG smart contract, using the submission API associated with Protocol Version 1.
// Input public keys are expected to be either all nil or all non-nil.
// Deprecated: This function is a temporary measure to provide backward compatibility between Protocol Versions 1 and 2.
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
// Deprecated: This function is a temporary measure to provide backward compatibility between Protocol Versions 1 and 2.
// Deprecated: This is a temporary function to provide backward compatibility between Protocol Versions 1 and 2.

Comment on lines +329 to +333
// CASE 2: we aren't sure whether FlowDKG is v2 compatible - fallback to v1 submission
if err != nil {
c.Log.Warn().Err(err).Msg("unable to determine FlowDKG compatibility - falling back to v1 result submission")
return c.submitResult_ProtocolV1(groupPublicKey, publicKeys)
}
Copy link
Member

Choose a reason for hiding this comment

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

This is probably not a question that we can address as part of this PR. When reading these lines, I am wondering what would happen if the protocol was on version 2 already, but script execution failed and we would submit with version 1 ... not sure how unlikely that is and what the consequences could be. Probably something best to be discussed in person.

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.

3 participants