Merge pull request #5633 from oasisprotocol/peternose/bugfix/churp

go/keymanager/churp: Minor fixes

go/consensus/cometbft/apps/keymanager/churp/epoch.go

@@ -31,11 +31,11 @@ func (ext *churpExt) onEpochChange(ctx *tmapi.Context, epoch beacon.EpochTime) e
 
 			switch epoch {
 			case status.NextHandoff:
-				// The epoch for the handoff just started, meaning that registrations
-				// are now closed. If not enough nodes applied for the next committee,
-				// we need to reset applications and start collecting again.
-				minCommitteeSize := int(status.Threshold)*2 + 1
-				if len(status.Applications) >= minCommitteeSize {
+				// The epoch for the handoff just started, meaning that
+				// application submissions are now closed. If not enough
+				// nodes applied for the next committee, we need to reset
+				// applications and start collecting again.
+				if len(status.Applications) >= status.MinApplicants() {
 					continue
 				}
 			case status.NextHandoff + 1:

go/consensus/cometbft/apps/keymanager/churp/state/interop/interop.go

@@ -105,6 +105,7 @@ func InitializeTestKeyManagerSecretsState(ctx context.Context, mkvs mkvs.Tree) e
 		Identity:        identity,
 		GroupID:         churp.EccNistP384,
 		Threshold:       2,
+		ExtraShares:     1,
 		HandoffInterval: 3,
 		Policy:          sigPolicy,
 		Handoff:         4,

go/consensus/cometbft/apps/keymanager/churp/txs.go

@@ -15,10 +15,6 @@ import (
 	"github.com/oasisprotocol/oasis-core/go/registry/api"
 )
 
-// extraShareholders is the minimum number of shares that can be lost while
-// still allowing the secret to be recovered.
-const extraShareholders = 2
-
 func (ext *churpExt) create(ctx *tmapi.Context, req *churp.CreateRequest) error {
 	// Prepare state.
 	state := churpState.NewMutableState(ctx.State())
@@ -91,6 +87,7 @@ func (ext *churpExt) create(ctx *tmapi.Context, req *churp.CreateRequest) error
 		Identity:        req.Identity,
 		GroupID:         req.GroupID,
 		Threshold:       req.Threshold,
+		ExtraShares:     req.ExtraShares,
 		HandoffInterval: req.HandoffInterval,
 		Policy:          req.Policy,
 		Handoff:         0,
@@ -155,6 +152,11 @@ func (ext *churpExt) update(ctx *tmapi.Context, req *churp.UpdateRequest) error
 		return fmt.Errorf("keymanager: churp: invalid config: %w", err)
 	}
 
+	// Handle extra shares change.
+	if req.ExtraShares != nil {
+		status.ExtraShares = *req.ExtraShares
+	}
+
 	// Handle handoff interval change.
 	if req.HandoffInterval != nil {
 		switch {
@@ -452,20 +454,14 @@ func tryFinalizeHandoff(status *churp.Status, epochChange bool) bool {
 		}
 	}
 
-	// Verify the size of the committee.
-	switch epochChange {
-	case true:
-		// At the end of the handoff epoch, a threshold number of applicants
-		// will suffice.
-		minCommitteeSize := status.Threshold + 1 + extraShareholders
-		for len(committee) < int(minCommitteeSize) {
-			return false
-		}
-	case false:
-		// During the handoff epoch, all applicants must send confirmation.
-		for len(committee) != len(status.Applications) {
-			return false
-		}
+	// Verify the committee size if the number of extra shares has changed.
+	if len(committee) < status.MinCommitteeSize() {
+		return false
+	}
+
+	// During the handoff epoch, all applicants must send confirmation.
+	if !epochChange && len(committee) != len(status.Applications) {
+		return false
 	}
 
 	// Sort the committee to ensure a deterministic order.

go/consensus/cometbft/apps/keymanager/churp/txs_test.go

@@ -199,10 +199,10 @@ func (s *TxTestSuite) TestCreate() {
 			Identity: churp.Identity{
 				RuntimeID: s.keymanagerRuntimes[0].ID,
 			},
-			Threshold: 0,
+			GroupID: 100,
 		}
 		err := s.ext.create(s.txCtx, &req)
-		require.ErrorContains(s.T(), err, "invalid config: threshold must be at least 1, got 0")
+		require.ErrorContains(s.T(), err, "invalid config: unsupported group, ID 100")
 	})
 
 	s.Run("happy path - handoffs disabled", func() {
@@ -219,6 +219,7 @@ func (s *TxTestSuite) TestCreate() {
 			Identity:        identity,
 			GroupID:         churp.EccNistP384,
 			Threshold:       1,
+			ExtraShares:     2,
 			HandoffInterval: 0,
 			Policy:          policy,
 		}
@@ -236,6 +237,7 @@ func (s *TxTestSuite) TestCreate() {
 		require.Equal(s.T(), s.keymanagerRuntimes[0].ID, status.RuntimeID)
 		require.Equal(s.T(), churp.EccNistP384, status.GroupID)
 		require.Equal(s.T(), uint8(1), status.Threshold)
+		require.Equal(s.T(), uint8(2), status.ExtraShares)
 		require.Equal(s.T(), beacon.EpochTime(0), status.HandoffInterval)
 		require.Equal(s.T(), policy, status.Policy)
 		require.Equal(s.T(), beacon.EpochTime(0), status.Handoff)

go/keymanager/churp/requests.go

@@ -32,6 +32,10 @@ type CreateRequest struct {
 	// to reconstruct a key.
 	Threshold uint8 `json:"threshold,omitempty"`
 
+	// ExtraShares represents the minimum number of shares that can be lost
+	// to render the secret unrecoverable.
+	ExtraShares uint8 `json:"extra_shares,omitempty"`
+
 	// HandoffInterval is the time interval in epochs between handoffs.
 	//
 	// A zero value disables handoffs.
@@ -43,9 +47,6 @@ type CreateRequest struct {
 
 // ValidateBasic performs basic config validity checks.
 func (c *CreateRequest) ValidateBasic() error {
-	if c.Threshold < 1 {
-		return fmt.Errorf("threshold must be at least 1, got %d", c.Threshold)
-	}
 	if c.GroupID > 0 {
 		return fmt.Errorf("unsupported group, ID %d", c.GroupID)
 	}
@@ -63,6 +64,10 @@ func (c *CreateRequest) ValidateBasic() error {
 type UpdateRequest struct {
 	Identity
 
+	// ExtraShares represents the minimum number of shares that can be lost
+	// to render the secret unrecoverable.
+	ExtraShares *uint8 `json:"extra_shares,omitempty"`
+
 	// HandoffInterval is the time interval in epochs between handoffs.
 	//
 	// Zero value disables handoffs.
@@ -74,7 +79,7 @@ type UpdateRequest struct {
 
 // ValidateBasic performs basic config validity checks.
 func (c *UpdateRequest) ValidateBasic() error {
-	if c.HandoffInterval == nil && c.Policy == nil {
+	if c.ExtraShares == nil && c.HandoffInterval == nil && c.Policy == nil {
 		return fmt.Errorf("update config should not be empty")
 	}
 
@@ -127,7 +132,7 @@ type ConfirmationRequest struct {
 
 	// Epoch is the epoch of the handoff for which the node reconstructed
 	// the share.
-	Epoch beacon.EpochTime `json:"handoff"`
+	Epoch beacon.EpochTime `json:"epoch"`
 
 	// Checksum is the hash of the verification matrix.
 	Checksum hash.Hash `json:"checksum"`
@@ -139,7 +144,7 @@ type SignedConfirmationRequest struct {
 	Confirmation ConfirmationRequest `json:"confirmation"`
 
 	// Signature is the RAK signature of the confirmation request.
-	Signature signature.RawSignature `json:"signature,omitempty"`
+	Signature signature.RawSignature `json:"signature"`
 }
 
 // VerifyRAK verifies the runtime attestation key (RAK) signature.

go/keymanager/churp/status.go

@@ -1,6 +1,8 @@
 package churp
 
 import (
+	"math"
+
 	beacon "github.com/oasisprotocol/oasis-core/go/beacon/api"
 	"github.com/oasisprotocol/oasis-core/go/common"
 	"github.com/oasisprotocol/oasis-core/go/common/crypto/hash"
@@ -18,6 +20,34 @@ const (
 	EccNistP384 uint8 = iota
 )
 
+// HandoffKind represents the kind of a handoff.
+type HandoffKind int
+
+const (
+	// HandoffKindDealingPhase represents the initial setup phase.
+	HandoffKindDealingPhase HandoffKind = iota
+	// HandoffKindCommitteeUnchanged represents a handoff where the committee
+	// doesn't change.
+	HandoffKindCommitteeUnchanged
+	// HandoffKindCommitteeChanged represents a handoff where the committee
+	// changes.
+	HandoffKindCommitteeChanged
+)
+
+// String returns the string representation of the HandoffKind.
+func (h HandoffKind) String() string {
+	switch h {
+	case HandoffKindDealingPhase:
+		return "dealing phase"
+	case HandoffKindCommitteeUnchanged:
+		return "committee unchanged"
+	case HandoffKindCommitteeChanged:
+		return "committee changed"
+	default:
+		return "unknown"
+	}
+}
+
 // ConsensusParameters are the key manager CHURP consensus parameters.
 type ConsensusParameters struct {
 	GasCosts transaction.Costs `json:"gas_costs,omitempty"`
@@ -48,6 +78,13 @@ type Status struct {
 	// recovered.
 	Threshold uint8 `json:"threshold"`
 
+	// ExtraShares represents the minimum number of shares that can be lost
+	// to render the secret unrecoverable.
+	//
+	// If t and e represent the threshold and extra shares, respectively,
+	// then the minimum size of the committee is t+e+1.
+	ExtraShares uint8 `json:"extra_shares"`
+
 	// HandoffInterval is the time interval in epochs between handoffs.
 	//
 	// A zero value disables handoffs.
@@ -100,6 +137,50 @@ type Status struct {
 	Applications map[signature.PublicKey]Application `json:"applications,omitempty"`
 }
 
+// HandoffKind returns the type of the next handoff depending on which nodes
+// submitted an application to form the next committee.
+func (s *Status) HandoffKind() HandoffKind {
+	if len(s.Committee) == 0 {
+		return HandoffKindDealingPhase
+	}
+
+	if len(s.Committee) != len(s.Applications) {
+		return HandoffKindCommitteeChanged
+	}
+
+	for _, id := range s.Committee {
+		if _, ok := s.Applications[id]; !ok {
+			return HandoffKindCommitteeChanged
+		}
+	}
+
+	return HandoffKindCommitteeUnchanged
+}
+
+// MinCommitteeSize returns the minimum number of nodes in the committee.
+func (s *Status) MinCommitteeSize() int {
+	t := int(s.Threshold)
+	e := int(s.ExtraShares)
+	return t + e + 1
+}
+
+// MinApplicants returns the minimum number of nodes that must participate
+// in a handoff.
+func (s *Status) MinApplicants() int {
+	t := int(s.Threshold)
+	e := int(s.ExtraShares)
+
+	switch s.HandoffKind() {
+	case HandoffKindDealingPhase, HandoffKindCommitteeUnchanged:
+		return t + e + 1
+	case HandoffKindCommitteeChanged:
+		return max(t+e+1, 2*t+1)
+	default:
+		// Dead code.
+		return math.MaxInt
+	}
+}
+
 // HandoffsDisabled returns true if and only if handoffs are disabled.
 func (s *Status) HandoffsDisabled() bool {
 	return s.HandoffInterval == HandoffsDisabled

keymanager/src/churp/policy.rs

@@ -16,18 +16,20 @@ use crate::policy::verify_data_and_trusted_signers;
 
 use super::Error;
 
-/// Policy, which manages the key manager policy.
+/// A collection of cached key manager policies.
 pub struct VerifiedPolicies {
     policies: Mutex<HashMap<u8, Arc<VerifiedPolicy>>>,
 }
 
 impl VerifiedPolicies {
+    /// Creates a new collection of cached verified policies.
     pub fn new() -> Self {
         Self {
             policies: Mutex::new(HashMap::new()),
         }
     }
 
+    /// Verifies and caches the given policy.
     pub fn verify(&self, policy: &SignedPolicySGX) -> Result<Arc<VerifiedPolicy>> {
         let mut policies = self.policies.lock().unwrap();
 

runtime/src/consensus/keymanager/churp.rs

@@ -56,6 +56,13 @@ pub struct Status {
     /// recovered.
     pub threshold: u8,
 
+    /// The minimum number of shares that can be lost to render the secret
+    /// unrecoverable.
+    ///
+    /// If t and e represent the threshold and extra shares, respectively,
+    /// then the minimum size of the committee is t+e+1.
+    pub extra_shares: u8,
+
     /// The time interval in epochs between handoffs.
     ///
     /// A zero value disables handoffs.

runtime/src/consensus/state/beacon.rs

@@ -154,7 +154,7 @@ mod test {
         let mock_consensus_root = Root {
             version: 1,
             root_type: RootType::State,
-            hash: Hash::from("8c8ca6fd2cb57c8f3ba9caf07e8bdc7e57962697aa9a4cdf337c519dc987fc68"),
+            hash: Hash::from("3c4cfd92b0aea7da65e6d083c69e4424aa87376735a8f0a5ada7fd0547343f66"),
             ..Default::default()
         };
         let mkvs = Tree::builder()

runtime/src/consensus/state/keymanager.rs

@@ -166,7 +166,7 @@ mod test {
         let mock_consensus_root = Root {
             version: 1,
             root_type: RootType::State,
-            hash: Hash::from("8c8ca6fd2cb57c8f3ba9caf07e8bdc7e57962697aa9a4cdf337c519dc987fc68"),
+            hash: Hash::from("3c4cfd92b0aea7da65e6d083c69e4424aa87376735a8f0a5ada7fd0547343f66"),
             ..Default::default()
         };
         let mkvs = Tree::builder()

runtime/src/consensus/state/keymanager/churp.rs

@@ -76,7 +76,7 @@ mod test {
         let mock_consensus_root = Root {
             version: 1,
             root_type: RootType::State,
-            hash: Hash::from("8c8ca6fd2cb57c8f3ba9caf07e8bdc7e57962697aa9a4cdf337c519dc987fc68"),
+            hash: Hash::from("3c4cfd92b0aea7da65e6d083c69e4424aa87376735a8f0a5ada7fd0547343f66"),
             ..Default::default()
         };
         let mkvs = Tree::builder()
@@ -148,6 +148,7 @@ mod test {
             runtime_id,
             group_id: GroupID::NistP384,
             threshold: 2,
+            extra_shares: 1,
             handoff_interval: 3,
             policy: SignedPolicySGX {
                 policy: PolicySGX {

runtime/src/consensus/state/registry.rs

@@ -132,7 +132,7 @@ mod test {
         let mock_consensus_root = Root {
             version: 1,
             root_type: RootType::State,
-            hash: Hash::from("8c8ca6fd2cb57c8f3ba9caf07e8bdc7e57962697aa9a4cdf337c519dc987fc68"),
+            hash: Hash::from("3c4cfd92b0aea7da65e6d083c69e4424aa87376735a8f0a5ada7fd0547343f66"),
             ..Default::default()
         };
         let mkvs = Tree::builder()

runtime/src/consensus/state/roothash.rs

@@ -120,7 +120,7 @@ mod test {
         let mock_consensus_root = Root {
             version: 1,
             root_type: RootType::State,
-            hash: Hash::from("8c8ca6fd2cb57c8f3ba9caf07e8bdc7e57962697aa9a4cdf337c519dc987fc68"),
+            hash: Hash::from("3c4cfd92b0aea7da65e6d083c69e4424aa87376735a8f0a5ada7fd0547343f66"),
             ..Default::default()
         };
         let mkvs = Tree::builder()

runtime/src/consensus/state/staking.rs

@@ -221,7 +221,7 @@ mod test {
         let mock_consensus_root = Root {
             version: 1,
             root_type: RootType::State,
-            hash: Hash::from("8c8ca6fd2cb57c8f3ba9caf07e8bdc7e57962697aa9a4cdf337c519dc987fc68"),
+            hash: Hash::from("3c4cfd92b0aea7da65e6d083c69e4424aa87376735a8f0a5ada7fd0547343f66"),
             ..Default::default()
         };
         let mkvs = Tree::builder()