Add node ID to index mapping to DKG data model #441

jordanschalm · 2024-08-07T16:47:20Z

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:

Add contract fields (storage slots, because of upgrade limitations) which includes only the new index mapping, conceptually extending the existing fields finalSubmissionByNodeID and uniqueFinalSubmissions.
The contract would use the new and existing fields together, and would need to maintain consistency between them.
Add one contract field which tracks the entire submission structure (now including both a key list and index mapping), conceptually replacing the existing fields finalSubmissionByNodeID and uniqueFinalSubmissions.
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

Add type ResultSubmission that encapsulates one complete DKG submission, (replaces [String?])
Add type SubmissionTracker that encapsulates the state for all DKG submissions for one DKG instance
Deprecate existing contract fields which track final submissions as [String]
Adds dkgGroupKey and dkgIdMapping fields to EpochCommit (previously the group key was by convention the first entry in the single dkgPubKeys entry

joshuahannan · 2024-08-16T18:47:36Z

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

joshuahannan

This looks like a good start!

joshuahannan · 2024-08-16T18:50:37Z

contracts/epochs/FlowDKG.cdc

+                return false
+            }
+            // TODO: validate equality check on list is safe
+            if self.pubKeys != other.pubKeys {


Do we expect these to be in the same exact order or just contain the same keys?

Equivalent ResultSubmission would need to have them in the same order, because the index is meaningful in the DKG.

joshuahannan · 2024-08-16T19:03:35Z

contracts/epochs/FlowDKG.cdc

@@ -357,6 +426,7 @@ access(all) contract FlowDKG {
    }

    /// Get the count of the final submissions array
+    // TODO: Should this be a view function?


yes, probably should be view

jordanschalm · 2024-08-16T21:34:00Z

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

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.

- make funs view - improve docs - remove commented-out code

was called "publish participant" but was actually publishing admin

jordanschalm

Adding some notes for reviewers

jordanschalm · 2024-09-17T18:28:43Z

contracts/epochs/FlowDKG.cdc

+    // 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


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.

jordanschalm · 2024-09-17T18:33:42Z

contracts/epochs/FlowDKG.cdc

@@ -119,6 +333,7 @@ access(all) contract FlowDKG {

        /// Posts a whiteboard message to the contract
        access(all) fun postMessage(_ content: String) {
+            // TODO: DKG enabled?


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.

That is fine with me. Feel free to add those pre-conditions to the functions if you want!

jordanschalm · 2024-09-17T18:34:26Z

contracts/epochs/FlowDKG.cdc

@@ -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 {


Replaced with ResultSubmission.equals

jordanschalm · 2024-09-17T18:34:39Z

contracts/epochs/FlowDKG.cdc

@@ -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?]) {


Replaced with SubmissionTracker methods

jordanschalm · 2024-09-17T18:38:29Z

contracts/epochs/FlowDKG.cdc

+    }
+
+    // Borrows the singleton SubmissionTracker from storage; panics if none exists.
+    access(contract) view fun mustBorrowSubmissionTracker(): &FlowDKG.SubmissionTracker {


This can be used to read from SubmissionTracker in a view-only context.

jordanschalm · 2024-09-17T18:41:57Z

lib/go/test/flow_dkg_test.go

@@ -19,10 +19,6 @@ import (
 	"github.com/onflow/flow-core-contracts/lib/go/templates"


These tests are updated to the point where they are passing, however there are still several places (noted as TODO comments) where additional updates should be made prior to merging. In particular cases where the test is expecting a failure, but the failure occurs for the wrong reason (like passing in the old transaction arguments).

joshuahannan

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.

joshuahannan · 2024-09-24T16:06:14Z

contracts/epochs/FlowDKG.cdc

+        if groupPubKey == nil && pubKeys == nil && idMapping == nil {
+            return true
+        }
+        // Otherwise, all fields must be non-nil
+        return groupPubKey != nil && pubKeys != nil && idMapping != nil 


I'm a little confused. The comments make it seem like this function is checking if any submission is valid, but the function name is isValidNilSubmission, which makes me think that this should only verify that the all-nil case is valid. Maybe it should be renamed to isValidSubmission? Also, we can simplify the conditional I think:

return (groupPubKey == nil && pubKeys == nil && idMapping == nil) || (groupPubKey != nil && pubKeys != nil && idMapping != nil)

joshuahannan · 2024-09-24T16:16:25Z

contracts/epochs/FlowDKG.cdc

+
+    // Checks that an id mapping (part of ResultSubmission) contains one entry per public key.
+    access(all) view fun isValidIDMapping(pubKeys: [String]?, idMapping: {String: Int}?): Bool {
+        if pubKeys == nil {


Should this be pubKeys == nil && idMapping == nil?

joshuahannan · 2024-09-24T16:26:18Z

contracts/epochs/FlowDKG.cdc

+
+        init(groupPubKey: String?, pubKeys: [String]?, idMapping: {String:Int}?) {
+            pre {
+                FlowDKG.isValidNilSubmission(groupPubKey: groupPubKey, pubKeys: pubKeys, idMapping: idMapping): "invalid empty submission"


Suggested change

FlowDKG.isValidNilSubmission(groupPubKey: groupPubKey, pubKeys: pubKeys, idMapping: idMapping): "invalid empty submission"

FlowDKG.isValidNilSubmission(groupPubKey: groupPubKey, pubKeys: pubKeys, idMapping: idMapping):

"FlowDKG.ResultSubmission.init: Invalid DKG Result Submission! "

.concat("DKG fields must all be `nil` or all be non-`nil`")

I'm going through all the core contracts repos and updating the error messages to be more helpful, so it would be good to start doing this for all our PRs from now on. Basically going off the guidelines in this issue. Can you do something similar for the other panics in this PR?

joshuahannan · 2024-09-24T16:35:05Z

contracts/epochs/FlowDKG.cdc

+        // CAUTION: This method should only be called by Participant, which enforces that Participant.nodeID is passed in.
+        access(all) fun addSubmission(nodeID: String, submission: ResultSubmission) {
+            pre {
+                self.authorized[nodeID] != nil: "must be authorized for this DKG instance"


Suggested change

self.authorized[nodeID] != nil: "must be authorized for this DKG instance"

self.authorized[nodeID] != nil:

"FlowDKG.addSubmission: Cannot add Result Submission for DKG for epoch "

.concat(FlowEpoch.proposedEpochCounter().toString())

.concat(". The node with id ").concat(nodeID)

.concat(" that is submitting the Result Submission is not authorized for this DKG instance."

Same comment about error messages here.

joshuahannan · 2024-09-24T16:38:02Z

contracts/epochs/FlowDKG.cdc

@@ -119,6 +333,7 @@ access(all) contract FlowDKG {

        /// Posts a whiteboard message to the contract
        access(all) fun postMessage(_ content: String) {
+            // TODO: DKG enabled?


That is fine with me. Feel free to add those pre-conditions to the functions if you want!

joshuahannan · 2024-09-24T16:40:05Z

contracts/epochs/FlowDKG.cdc

+
+    // Borrows the singleton SubmissionTracker from storage; panics if none exists.
+    access(contract) view fun mustBorrowSubmissionTracker(): &FlowDKG.SubmissionTracker {
+        return self.account.storage.borrow<&SubmissionTracker>(from: /storage/flowDKGFinalSubmissionTracker)!


Should we add a ?? panic() here?

joshuahannan · 2024-09-24T16:48:02Z

transactions/dkg/send_empty_final_submission.cdc

+
+    prepare(signer: auth(BorrowValue) &Account) {
+        self.dkgParticipant = signer.storage.borrow<&FlowDKG.Participant>(from: FlowDKG.ParticipantStoragePath)
+            ?? panic("Cannot borrow dkg participant reference")


Suggested change

?? panic("Cannot borrow dkg participant reference")

?? panic("Cannot borrow DKG Participant Reference from path "

.concat(FlowDKG.ParticipantStoragePath)

.concat(". The signer needs to make sure their account is initialized with the DKG Participant resource."))

It would be good to start updating the error messages here too. This would apply to most of the DKG transactions here too

jordanschalm added 3 commits August 7, 2024 12:00

define ResultSubmission struct

dd90e0e

annotate required changes

7638aeb

setup test framework

4e0bc0c

jordanschalm changed the base branch from master to feature/efm-recovery August 7, 2024 16:49

jordanschalm added 2 commits August 8, 2024 12:45

add preliminary new fields to EpochCommit

22ba195

add submission tracker sketch

e37f616

joshuahannan reviewed Aug 16, 2024

View reviewed changes

jordanschalm added 21 commits September 11, 2024 16:35

ResultSubmission tests

26039c5

ResultSubmission docs

88cd509

addSubmission method

829586b

add tx todos

db7aa57

add init test, validation on ResultSubmission init

14a70ab

test comments

c6e6369

basic SubmissionTracker tests

cb02364

update other contract methods to use SubmissionTracker

e694f71

add dummy values for EpochCommit event

854a570

add docs

257ddb6

add exceedsThreshold test

5d9f137

Merge branch 'feature/efm-recovery' into jord/6213-dkg-mapping

94c8cbb

update FlowEpoch to compile

027e41d

update transactions

b1afc65

improve representation for "empty submissions"

56b12c1

add test for empty submission

507a454

test empty submission exceeds threshold

a9d24b4

fix go test: send submission

023faed

wip

5f236fb

remove '

423fb44

incredibly, the tests are passing

23ade80

jordanschalm added 8 commits September 16, 2024 15:13

note where tests still need updating

8f669f1

consistent terminology: empty submission

060fbb5

resolve some todos

3553260

- make funs view - improve docs - remove commented-out code

fix empty submission dkg tests

9db9ab8

fix epoch tests

135cad6

rename publish admin script

80f1f91

was called "publish participant" but was actually publishing admin

store as much as possible in EpochMetadata

1fd997b

build static files

9b623b3

jordanschalm marked this pull request as ready for review September 17, 2024 18:26

make more functions view

3e32ac5

jordanschalm commented Sep 17, 2024

View reviewed changes

fix test

af7e508

jordanschalm changed the title ~~[DRAFT] Add node ID to index mapping to DKG data model~~ Add node ID to index mapping to DKG data model Sep 17, 2024

jordanschalm requested a review from joshuahannan September 17, 2024 18:50

joshuahannan approved these changes Sep 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add node ID to index mapping to DKG data model #441

Add node ID to index mapping to DKG data model #441

jordanschalm commented Aug 7, 2024 •

edited

Loading

joshuahannan commented Aug 16, 2024

joshuahannan left a comment

joshuahannan Aug 16, 2024

jordanschalm Aug 16, 2024

joshuahannan Aug 16, 2024

jordanschalm commented Aug 16, 2024

jordanschalm left a comment

jordanschalm Sep 17, 2024

jordanschalm Sep 17, 2024

joshuahannan Sep 24, 2024

jordanschalm Sep 17, 2024

jordanschalm Sep 17, 2024

jordanschalm Sep 17, 2024

jordanschalm Sep 17, 2024

joshuahannan left a comment

joshuahannan Sep 24, 2024

joshuahannan Sep 24, 2024

joshuahannan Sep 24, 2024

joshuahannan Sep 24, 2024

joshuahannan Sep 24, 2024

joshuahannan Sep 24, 2024

joshuahannan Sep 24, 2024

		@@ -19,10 +19,6 @@ import (
		"github.com/onflow/flow-core-contracts/lib/go/templates"

-                self.authorized[nodeID] != nil: "must be authorized for this DKG instance"
+                self.authorized[nodeID] != nil:
+                "FlowDKG.addSubmission: Cannot add Result Submission for DKG for epoch "
+                .concat(FlowEpoch.proposedEpochCounter().toString())
+                .concat(". The node  with id ").concat(nodeID)
+                .concat(" that is submitting the Result Submission is not authorized for this DKG instance."

-            ?? panic("Cannot borrow dkg participant reference")
+            ?? panic("Cannot borrow DKG Participant Reference from path "
+                    .concat(FlowDKG.ParticipantStoragePath)
+                    .concat(". The signer needs to make sure their account is initialized with the DKG Participant resource."))

Add node ID to index mapping to DKG data model #441

Are you sure you want to change the base?

Add node ID to index mapping to DKG data model #441

Conversation

jordanschalm commented Aug 7, 2024 • edited Loading

General Context

Changes

joshuahannan commented Aug 16, 2024

joshuahannan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm commented Aug 16, 2024

jordanschalm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuahannan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm commented Aug 7, 2024 •

edited

Loading