Merge pull request #6590 from onflow/tarak/minor-suggestions

Random beacon comments

cmd/bootstrap/run/qc.go

@@ -24,7 +24,7 @@ type Participant struct {
 	RandomBeaconPrivKey crypto.PrivateKey
 }
 
-// ParticipantData represents a subset of all consensus participants that contributing to some signing process (at the moment, we only use
+// ParticipantData represents a subset of all consensus participants that contribute to some signing process (at the moment, we only use
 // it for the contributors for the root QC). For mainnet, this a *strict subset* of all consensus participants:
 //   - In an early step during the bootstrapping process, every node operator locally generates votes for the root block from the nodes they
 //     operate. During the vote-generation step, (see function `constructRootVotes`), `Participants` represents only the operator's own
@@ -236,7 +236,7 @@ func GenerateQCParticipantData(allNodes, internalNodes []bootstrap.NodeInfo, dkg
 
 		dkgParticipant, ok := participantLookup[node.NodeID]
 		if !ok {
-			return nil, fmt.Errorf("nonexistannt node id (%x) in participant lookup", node.NodeID)
+			return nil, fmt.Errorf("nonexistent node id (%x) in participant lookup", node.NodeID)
 		}
 		dkgIndex := dkgParticipant.Index
 

cmd/util/cmd/epochs/cmd/recover.go

@@ -20,7 +20,7 @@ import (
 // EFM can be exited only by a special service event, EpochRecover, which initially originates from a manual service account transaction.
 // The full epoch data must be generated manually and submitted with this transaction in order for an
 // EpochRecover event to be emitted. This command retrieves the current protocol state identities, computes the cluster assignment using those
-// identities, generates the cluster QCs and retrieves the DKG key vector of the last successful epoch.
+// identities, generates the cluster QCs and retrieves the Random Beacon key vector of the last successful epoch.
 // This recovery process has some constraints:
 //   - The RecoveryEpoch must have exactly the same consensus committee as participated in the most recent successful DKG.
 //   - The RecoveryEpoch must contain enough "internal" collection nodes so that all clusters contain a supermajority of "internal" collection nodes (same constraint as sporks)

consensus/hotstuff/signature/randombeacon_signer_store.go

@@ -42,9 +42,9 @@ func (s *EpochAwareRandomBeaconKeyStore) ByView(view uint64) (crypto.PrivateKey,
 	}
 
 	// When DKG has completed,
-	//   - if a node successfully generated the DKG key, the valid private key will be stored in database.
-	//   - if a node failed to generate the DKG key, we will save a record in database to indicate this
-	//      node has no private key for this epoch.
+	//   - if a node successfully generated the Random Beacon key, the valid private key will be stored in database.
+	//   - if a node failed to generate the Random Beacon key, we will save a record in database to indicate this
+	//     node has no private key for this epoch.
 	// Within the epoch, we can look up my random beacon private key for the epoch. There are 3 cases:
 	//   1. DKG has completed, and the private key is stored in database, and we can retrieve it (happy path)
 	//   2. DKG has completed, but we failed to generate a private key (unhappy path)

consensus/hotstuff/verification/combined_verifier_v3.go

@@ -174,7 +174,7 @@ func (c *CombinedVerifierV3) VerifyQC(signers flow.IdentitySkeletonList, sigData
 			if protocol.IsIdentityNotFound(err) {
 				return model.NewInvalidSignerErrorf("%v is not a random beacon participant: %w", signerID, err)
 			}
-			return fmt.Errorf("unexpected error retrieving dkg key share for signer %v: %w", signerID, err)
+			return fmt.Errorf("unexpected error retrieving Random Beacon key share for signer %v: %w", signerID, err)
 		}
 		beaconPubKeys = append(beaconPubKeys, keyShare)
 	}

consensus/hotstuff/votecollector/combined_vote_processor_v2_test.go

@@ -812,7 +812,7 @@ func TestCombinedVoteProcessorV2_BuildVerifyQC(t *testing.T) {
 		identity.StakingPubKey = stakingPriv.PublicKey()
 
 		keys := &storagemock.SafeBeaconKeys{}
-		// there is no DKG key for this epoch
+		// there is no Random Beacon key for this epoch
 		keys.On("RetrieveMyBeaconPrivateKey", epochCounter).Return(nil, false, nil)
 
 		beaconSignerStore := hsig.NewEpochAwareRandomBeaconKeyStore(epochLookup, keys)
@@ -833,7 +833,7 @@ func TestCombinedVoteProcessorV2_BuildVerifyQC(t *testing.T) {
 		}
 
 		keys := &storagemock.SafeBeaconKeys{}
-		// there is DKG key for this epoch
+		// there is Random Beacon key for this epoch
 		keys.On("RetrieveMyBeaconPrivateKey", epochCounter).Return(dkgKey, true, nil)
 
 		beaconSignerStore := hsig.NewEpochAwareRandomBeaconKeyStore(epochLookup, keys)

consensus/hotstuff/votecollector/combined_vote_processor_v3_test.go

@@ -948,7 +948,7 @@ func TestCombinedVoteProcessorV3_BuildVerifyQC(t *testing.T) {
 		identity.StakingPubKey = stakingPriv.PublicKey()
 
 		keys := &storagemock.SafeBeaconKeys{}
-		// there is no DKG key for this epoch
+		// there is no Random Beacon key for this epoch
 		keys.On("RetrieveMyBeaconPrivateKey", epochCounter).Return(nil, false, nil)
 
 		beaconSignerStore := hsig.NewEpochAwareRandomBeaconKeyStore(epochLookup, keys)
@@ -970,7 +970,7 @@ func TestCombinedVoteProcessorV3_BuildVerifyQC(t *testing.T) {
 		}
 
 		keys := &storagemock.SafeBeaconKeys{}
-		// there is DKG key for this epoch
+		// there is Random Beacon key for this epoch
 		keys.On("RetrieveMyBeaconPrivateKey", epochCounter).Return(dkgKey, true, nil)
 
 		beaconSignerStore := hsig.NewEpochAwareRandomBeaconKeyStore(epochLookup, keys)

consensus/integration/epoch_test.go

@@ -117,7 +117,7 @@ func TestEpochTransition_IdentitiesOverlap(t *testing.T) {
 		newIdentity,
 	)
 
-	// generate new identities for next epoch, it will generate new DKG keys for random beacon participants
+	// generate new identities for next epoch, it will generate new Random Beacon keys for random beacon participants
 	nextEpochParticipantData := completeConsensusIdentities(t, privateNodeInfos[1:])
 	rootSnapshot = withNextEpoch(t, rootSnapshot, nextEpochIdentities, nextEpochParticipantData, consensusParticipants, 4, func(block *flow.Block) *flow.QuorumCertificate {
 		return createRootQC(t, block, firstEpochConsensusParticipants)

consensus/integration/nodes_test.go

@@ -530,7 +530,7 @@ func createNode(
 	require.NoError(t, err)
 
 	keys := &storagemock.SafeBeaconKeys{}
-	// there is DKG key for this epoch
+	// there is Random Beacon key for this epoch
 	keys.On("RetrieveMyBeaconPrivateKey", mock.Anything).Return(
 		func(epochCounter uint64) crypto.PrivateKey {
 			dkgInfo, ok := participant.beaconInfoByEpoch[epochCounter]

integration/testnet/network.go

@@ -1072,8 +1072,8 @@ func BootstrapNetwork(networkConf NetworkConfig, bootstrapDir string, chainID fl
 
 	allNodeInfos := append(toNodeInfos(stakedConfs), followerInfos...)
 
-	// IMPORTANT: we must use this ordering when writing the DKG keys as
-	//            this ordering defines the DKG participant's indices
+	// IMPORTANT: we must use this ordering when writing the Random Beacon keys as
+	//            this ordering defines the DKG participants' indices
 	stakedNodeInfos := bootstrap.Sort(toNodeInfos(stakedConfs), flow.Canonical[flow.Identity])
 
 	dkg, dkgIndexMap, err := runBeaconKG(stakedConfs)

model/convert/service_event.go

@@ -275,13 +275,13 @@ func convertServiceEventEpochCommit(event flow.Event) (*flow.ServiceEvent, error
 	// parse DKG participants
 	commit.DKGParticipantKeys, err = convertDKGKeys(cdcDKGKeys.Values)
 	if err != nil {
-		return nil, fmt.Errorf("could not convert DKG keys: %w", err)
+		return nil, fmt.Errorf("could not convert Random Beacon keys: %w", err)
 	}
 
 	// parse DKG group key
 	commit.DKGGroupKey, err = convertDKGKey(cdcDKGGroupKey)
 	if err != nil {
-		return nil, fmt.Errorf("could not convert DKG group key: %w", err)
+		return nil, fmt.Errorf("could not convert Random Beacon group key: %w", err)
 	}
 
 	// parse DKG Index Map
@@ -469,13 +469,13 @@ func convertServiceEventEpochRecover(event flow.Event) (*flow.ServiceEvent, erro
 	// parse DKG participants
 	commit.DKGParticipantKeys, err = convertDKGKeys(cdcDKGKeys.Values)
 	if err != nil {
-		return nil, fmt.Errorf("failed to decode DKG key shares from EpochRecover event: %w", err)
+		return nil, fmt.Errorf("failed to decode Random Beacon key shares from EpochRecover event: %w", err)
 	}
 
 	// parse DKG group key
 	commit.DKGGroupKey, err = convertDKGKey(cdcDKGGroupKey)
 	if err != nil {
-		return nil, fmt.Errorf("failed to decode DKG group key from EpochRecover event: %w", err)
+		return nil, fmt.Errorf("failed to decode Random Beacon group key from EpochRecover event: %w", err)
 	}
 
 	// parse DKG Index Map
@@ -986,22 +986,22 @@ func convertClusterQCVotes(cdcClusterQCs []cadence.Value) (
 	return qcVoteDatas, nil
 }
 
-// convertDKGKeys converts hex-encoded DKG public keys as received by the DKG
+// convertDKGKeys converts hex-encoded public beacon keys as received by the DKG
 // smart contract into crypto.PublicKey representations suitable for inclusion
 // in the protocol state.
 func convertDKGKeys(cdcDKGKeys []cadence.Value) ([]crypto.PublicKey, error) {
 	convertedKeys := make([]crypto.PublicKey, 0, len(cdcDKGKeys))
 	for _, value := range cdcDKGKeys {
 		pubKey, err := convertDKGKey(value)
 		if err != nil {
-			return nil, fmt.Errorf("could not decode dkg public key: %w", err)
+			return nil, fmt.Errorf("could not decode public beacon key share: %w", err)
 		}
 		convertedKeys = append(convertedKeys, pubKey)
 	}
 	return convertedKeys, nil
 }
 
-// convertDKGKey converts a single hex-encoded DKG public key as received by the DKG
+// convertDKGKey converts a single hex-encoded public beacon keys as received by the DKG
 // smart contract into crypto.PublicKey representations suitable for inclusion
 // in the protocol state.
 func convertDKGKey(cdcDKGKeys cadence.Value) (crypto.PublicKey, error) {
@@ -1014,11 +1014,11 @@ func convertDKGKey(cdcDKGKeys cadence.Value) (crypto.PublicKey, error) {
 	// decode individual public keys
 	pubKeyBytes, err := hex.DecodeString(string(keyHex))
 	if err != nil {
-		return nil, fmt.Errorf("could not decode individual public key into bytes: %w", err)
+		return nil, fmt.Errorf("converting hex to bytes failed: %w", err)
 	}
 	pubKey, err := crypto.DecodePublicKey(crypto.BLSBLS12381, pubKeyBytes)
 	if err != nil {
-		return nil, fmt.Errorf("could not decode dkg public key: %w", err)
+		return nil, fmt.Errorf("could not decode bytes into a public key: %w", err)
 	}
 	return pubKey, nil
 }

model/flow/dkg.go

@@ -36,52 +36,66 @@ func (state DKGEndState) String() string {
 	}
 }
 
-// DKGIndexMap describes the membership of the DKG committee 𝒟. Flow's random beacon utilizes
-// a threshold signature scheme, which requires a Distributed Key Generation [DKG] to generate the
-// key shares for each committee member. In the formal cryptographic protocol for DKG with n parties,
-// the individual participants are solely identified by indices {0, 1, ..., n-1} and the fact that these
-// are non-negative integer values is actively used by the DKG protocol. Accordingly, our implementation
-// of the lower-level cryptographic primitives work with these DKG index values.
-// On the protocol level, only consensus nodes (identified by their nodeIDs) are allowed to contribute
-// random beacon signature shares. Hence, the protocol level needs to map nodeIDs to DKG indices when
-// calling into the lower-level cryptographic primitives.
+// DKGIndexMap completely describes the DKG committee 𝒟 of size |𝒟| = n.
 //
 // Formal specification:
-//   - DKGIndexMap completely describes the DKG committee. If there were n parties authorized to participate
-//     in the DKG, DKGIndexMap must contain exactly n elements, i.e. n = len(DKGIndexMap)
-//   - The values in DKGIndexMap must form the set {0, 1, …, n-1}.
+//   - If n parties are authorized to participate in the DKG, DKGIndexMap must contain exactly n
+//     elements, i.e. n = len(DKGIndexMap)
+//   - The values in DKGIndexMap must form the set {0, 1, …, n-1}, as required by the low level cryptography
+//     module (convention simplifying the implementation).
 //
-// CAUTION: It is important to cleanly differentiate between the consensus committee 𝒞, the random beacon
-// committee ℛ and the DKG committee 𝒟:
+// Flow's random beacon utilizes a threshold signature scheme run by the committee 𝒟.
+// In the formal cryptographic protocol for a threshold signature with n parties, the
+// individual participants are identified by n public distinct non-negative integers, or simply indices.
+// These public indices are agreed upon by all participants and are used by the low-level
+// Shamir Secret Sharing [SSS].
+// In Flow, the threshold signature keys are generated by a Distributed Key Generation [DKG]. The DKG
+// therefore requires the same SSS indices as an input to generate the private key shares of each participant.
+// Accordingly, the lower-level cryptographic implementation of the threshold signature and DKG
+// works with these indices. The lower-level cryptographic interface requires that the indices are exactly
+// the set {0, 1, ..., n-1}.
+//
+// On the protocol level, only consensus nodes (identified by their nodeIDs) are allowed to contribute
+// random beacon signature shares. Hence, the protocol level needs to map nodeIDs to the indices when
+// calling into the lower-level cryptographic primitives.
+//
+// CAUTION: It is important to cleanly differentiate between the consensus committee 𝒞, the DKG committee 𝒟
+// and the committee ℛ:
 //   - For an epoch, the consensus committee 𝒞 contains all nodes that are authorized to vote for blocks. Authority
 //     to vote (i.e. membership in the consensus committee) is irrevocably granted for an epoch (though, honest nodes
 //     will reject votes and proposals from ejected nodes; nevertheless, ejected nodes formally remain members of
 //     the consensus committee).
-//   - Only consensus nodes are allowed to contribute to the random beacon. We define the random beacon committee ℛ
-//     as the subset of the consensus nodes, which _successfully_ completed the DKG. Hence, ℛ ⊆ 𝒞.
-//   - Lastly, there is the DKG committee 𝒟, which is the set of parties that were authorized to
-//     participate in the DKG. Mathematically, the DKGIndexMap is an injective function
-//     DKGIndexMap: 𝒟 ↦ {0,1,…,n-1}.
+//   - The DKG committee 𝒟 is the set of parties that were authorized to participate in the DKG (happy path; or
+//     eligible to receive a private key share from an alternative source on the fallback path). Mathematically,
+//     the DKGIndexMap is a bijective function DKGIndexMap: 𝒟 ↦ {0,1,…,n-1}.
+//   - Only consensus nodes are allowed to contribute to the random beacon. Informally, we define ℛ as the
+//     as the subset of the consensus committee (ℛ ⊆ 𝒞), which _successfully_ completed the DKG (hence ℛ ⊆ 𝒟).
+//     Specifically, r ∈ ℛ iff and only if r has a private Random Beacon key share matching the respective public
+//     key share in the `EpochCommit` event. In other words, consensus nodes are in ℛ iff and only if they are able
+//     to submit valid random beacon votes. Based on this definition we note that ℛ ⊆ (𝒟 ∩ 𝒞).
 //
 // The protocol explicitly ALLOWS additional parties outside the current epoch's consensus committee to participate.
 // In particular, there can be a key-value pair (d,i) ∈ DKGIndexMap, such that the nodeID d is *not* a consensus
-// committee member, i.e. d ∉ 𝒞. In terms of sets, this implies we must consistently work with the relatively
-// general assumption that 𝒟 \ 𝒞 ≠ ∅ and 𝒞 \ 𝒟 ≠ ∅.
+// committee member, i.e. d ∉ 𝒞. This may be the case when a DKG is run off-protocol to bootstrap the network.
+// In terms of sets, this implies we must consistently work with the relatively general
+// assumption that 𝒟 \ 𝒞 ≠ ∅ and 𝒞 \ 𝒟 ≠ ∅.
 // Nevertheless, in the vast majority of cases (happy path, roughly 98% of epochs) it will be the case that 𝒟 = 𝒞.
 // Therefore, we can optimize for the case 𝒟 = 𝒞, as long as we still support the more general case 𝒟 ≠ 𝒞.
 // Broadly, this makes the protocol more robust against temporary disruptions and sudden, large fluctuations in node
 // participation.
-// Nevertheless, there is an important liveness constraint: the intersection, 𝒟 ∩ 𝒞 = ℛ should be a larger number of
-// nodes. Specifically, an honest supermajority of consensus nodes must contain enough successful DKG participants
-// (about n/2) to produce a valid group signature for the random beacon [1, 3]. Therefore, we have the approximate
-// lower bound |ℛ| = |𝒟 ∩ 𝒞| = n/2 = |𝒟|/2 = len(DKGIndexMap)/2. Operating close to this lower bound would
-// require that every random beacon key-holder r ∈ ℛ remaining in the consensus committee is honest
-// (incl. quickly responsive) *all the time*. This is a lower bound, unsuited for decentralized production networks.
+//
+// Nevertheless, there is an important liveness constraint: the committee ℛ should be a large number of nodes.
+// Specifically, an honest supermajority of consensus nodes must contain enough successful DKG participants
+// (about |𝒟|/2 + 1) to produce a valid group signature for the random beacon at each block [1, 3].
+// Therefore, we have the approximate lower bound |ℛ| ≳ n/2 + 1 = |𝒟|/2 + 1 = len(DKGIndexMap)/2 + 1.
+// Operating close to this lower bound would require that every random beacon key-holder ϱ ∈ ℛ remaining in the consensus committee is honest
+// (incl. quickly responsive) *all the time*. Such a reliability assumption is unsuited for decentralized production networks.
 // To reject configurations that are vulnerable to liveness failures, the protocol uses the threshold `t_safety`
-// (heuristic, see [2]), which is implemented on the smart contract level. In a nutshell, the cardinality of intersection 𝒟 ∩ 𝒞
-// (wrt both sets 𝒟 ∩ 𝒞) should be well above 70%, values in the range 70-62% should be considered for short-term
-// recovery cases. Values of 62% or lower (i.e. |ℛ| ≤ 0.62·|𝒟| or |ℛ| ≤ 0.62·|𝒞|) are not recommended for any
-// production network, as single-node crashes are already enough to halt consensus.
+// (heuristic, see [2]), which is implemented on the smart contract level.
+// Ideally, |ℛ| and therefore |𝒟 ∩ 𝒞| (given that |ℛ| <= |𝒟 ∩ 𝒞|) should be well above 70% . |𝒟|.
+// Values in the range 70%-62% of |𝒟| should be considered for short-term recovery cases.
+// Values of 62% * |𝒟| or lower (i.e. |ℛ| ≤ 0.62·|𝒟|) are not recommended for any
+// production network, as single-node crashes may already be enough to halt consensus.
 //
 // For further details, see
 //   - [1] https://www.notion.so/flowfoundation/Threshold-Signatures-7e26c6dd46ae40f7a83689ba75a785e3?pvs=4