-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add node ID to index mapping to DKG data model #441
Conversation
I'm also leaning towards option 2. We actually can remove fields in upgrades. It doesn't delete the state, but it removes the ability to access it anywhere, so that is an option. Might be good to keep the field though for the small chance that we might actually need it again in the future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good start!
return false | ||
} | ||
// TODO: validate equality check on list is safe | ||
if self.pubKeys != other.pubKeys { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect these to be in the same exact order or just contain the same keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Equivalent ResultSubmission
would need to have them in the same order, because the index is meaningful in the DKG.
contracts/epochs/FlowDKG.cdc
Outdated
@@ -357,6 +426,7 @@ access(all) contract FlowDKG { | |||
} | |||
|
|||
/// Get the count of the final submissions array | |||
// TODO: Should this be a view function? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, probably should be view
Sounds good. We'll move ahead in that direction then - thanks for the review! I agree keeping the deprecated fields (at least at first) is the better move. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding some notes for reviewers
// By convention, all keys are encoded as lowercase hex strings, though it is not enforced here. | ||
// | ||
// INVARIANTS: | ||
// (1) either all fields are nil (empty submission) or no fields are nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, the data model for submissions was [String?]
, so some keys could be nil and some could be non-nil. If any keys were nil, we considered the submission invalid ("empty" in the terminology used in this PR).
This PR changes the data model so that either all fields are nil, or none are, and enforces this in the constructor.
contracts/epochs/FlowDKG.cdc
Outdated
@@ -119,6 +333,7 @@ access(all) contract FlowDKG { | |||
|
|||
/// Posts a whiteboard message to the contract | |||
access(all) fun postMessage(_ content: String) { | |||
// TODO: DKG enabled? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the smart contract allows posting whiteboard messages and sending final submissions even when the DKG is disabled 🤔. I don't think this matters functionally because:
- the DKG cannot be ended (winning final submission retrieved) if it is disabled
- when enabled/started, all state including whiteboard messages and submissions are wiped
For clarity though, I think we should disallow interacting with the contract while it is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is fine with me. Feel free to add those pre-conditions to the functions if you want!
@@ -279,36 +442,9 @@ access(all) contract FlowDKG { | |||
} | |||
} | |||
|
|||
/// Checks if two final submissions are equal by comparing each element | |||
/// Each element has to be exactly the same and in the same order | |||
access(account) fun submissionsEqual(_ existingSubmission: [String?], _ submission: [String?]): Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with ResultSubmission.equals
@@ -139,57 +354,9 @@ access(all) contract FlowDKG { | |||
/// Sends the final key vector submission. | |||
/// Can only be called by consensus nodes that are registered | |||
/// and can only be called once per consensus node per epoch | |||
access(all) fun sendFinalSubmission(_ submission: [String?]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with SubmissionTracker
methods
} | ||
|
||
// Borrows the singleton SubmissionTracker from storage; panics if none exists. | ||
access(contract) view fun mustBorrowSubmissionTracker(): &FlowDKG.SubmissionTracker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be used to read from SubmissionTracker
in a view-only context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! I just had a few comments. I appreciate converting some of the tests to the Cadence testing framework. Still will need to make sure all the important transactions are tested in Go though because we need to make sure the templates package is still working properly and returning valid transactions.
contracts/epochs/FlowDKG.cdc
Outdated
@@ -119,6 +333,7 @@ access(all) contract FlowDKG { | |||
|
|||
/// Posts a whiteboard message to the contract | |||
access(all) fun postMessage(_ content: String) { | |||
// TODO: DKG enabled? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is fine with me. Feel free to add those pre-conditions to the functions if you want!
since it is specifically enforcing the named Invariant (1), and checking both empty and non-empty submissions.
Co-authored-by: Tarak Ben Youssef <50252200+tarakby@users.noreply.github.com> Co-authored-by: Joshua Hannan <hannanjoshua19@gmail.com>
…ontracts into jord/6213-dkg-mapping
Ref: onflow/flow-go#6213
General Context
This PR modifies the DKG result data model to include an explicit mapping from node ID to DKG index. Currently the submission is an ordered list of keys, with the mapping being implicit based on the ordering. To support EFM Recovery we need to explicitly encode the index mapping and include it in the
EpochCommit
event. See onflow/flow-go#6213 for details.📝 Notes about implementation options from original draft
I broadly see two approaches to implement this:
finalSubmissionByNodeID
anduniqueFinalSubmissions
.The contract would use the new and existing fields together, and would need to maintain consistency between them.
finalSubmissionByNodeID
anduniqueFinalSubmissions
.The existing fields would be deprecated (we can't remove them because of upgrade limitations, but they would not be referenced by any code in the contract).
After noodling around a bit with both, I'm inclined to go with Option 2, because it seems easier to work with and reason about the submission state when it's encapsulated in one field (and type).
Changes
ResultSubmission
that encapsulates one complete DKG submission, (replaces[String?]
)SubmissionTracker
that encapsulates the state for all DKG submissions for one DKG instance[String]
dkgGroupKey
anddkgIdMapping
fields toEpochCommit
(previously the group key was by convention the first entry in the singledkgPubKeys
entry