fix(mpc): HasSigningQuorum uses operator-facing threshold semantics

Follow-up to e08dba1. The CLI/CRD `--threshold` flag surfaces the OPERATOR-facing "minimum signers required" count (e.g. threshold=2 for a 2-of-3 ensemble, threshold=3 for 3-of-5). The CGGMP21 signing API internally stores the polynomial degree (one below signers-required) in keyInfo.Threshold — so the sign code continues to compare `len(signerPeerIDs) >= keyInfo.Threshold + 1`. Those two surfaces are off-by-one by protocol library convention. HasSigningQuorum is operator-facing (used by /healthz) so it uses the CRD/CLI semantics directly: quorum holds when `readyCount >= threshold`. Scientist's repro: 3-node 2-of-3 ensemble (CRD threshold=2) with one peer down — readyCount=2, threshold=2 → quorum holds, /healthz returns 200. With the previous commit the arithmetic was readyCount>=threshold+1 which would have required all 3 nodes up. Tests updated accordingly (Test_HasSigningQuorum_3Node_2of3 now uses threshold=2 matching the deployed CRD, Test_HasSigningQuorum_5Node_3of5 uses threshold=3, boundary test covers the new arithmetic).
2026-09-24 00:26:24 +00:00 · 2026-04-20 18:52:49 -07:00
parent e08dba1faa
commit 631cbbabc5
4 changed files with 89 additions and 57 deletions
@@ -480,12 +480,15 @@ func runNodeConsensus(ctx context.Context, c *cli.Command) error {
 		// 2-of-3 signing quorum between them.
 		//
 		// Fixed behaviour: the node is healthy when it has enough ready peers
-		// (including self) to participate in a signing round for the
-		// configured CGGMP21/FROST threshold — i.e. `readyCount >= threshold+1`.
+		// (including self) to form a t-of-n signing quorum using the
+		// operator-facing threshold (CRD `threshold` / mpcd --threshold flag
+		// = minimum signers required). For a 2-of-3 ensemble (threshold=2),
+		// quorum holds when 2 or more nodes are ready. See
+		// HasSigningQuorum in pkg/transport/registry.go for the arithmetic.
 		//
 		// Keygen of fresh wallets still requires all peers (checked separately
 		// on the /keygen endpoint via peerRegistry.ArePeersReady()). Signing
-		// with an existing key only needs threshold+1 parties, which is what
+		// with an existing key only needs threshold parties, which is what
 		// /healthz now reports.
 		healthHandler := func(w http.ResponseWriter, r *http.Request) {
 			// allReady: every expected peer connected. Used for observability.
@@ -519,7 +522,9 @@ func runNodeConsensus(ctx context.Context, c *cli.Command) error {
 				"signing_quorum":  quorum,
 				"ready_count":     readyCount,
 				"threshold":       threshold,
-				"required_signers": threshold + 1,
+				// threshold = minimum signers required (operator surface).
+				// Present alongside ready_count so an operator can see how
+				// close to losing quorum the ensemble is.
 				"version":         Version,
 			}
 			json.NewEncoder(w).Encode(resp)
@@ -23,11 +23,17 @@ type PeerRegistry interface {
 	// Required for keygen (fresh wallets need every peer's contribution).
 	ArePeersReady() bool
 	// HasSigningQuorum returns true when ready peers (including self) are
-	// sufficient to participate in a signing round for a key with the given
-	// polynomial-degree threshold. Stricter than "any peer reachable",
-	// weaker than ArePeersReady — survives partial node loss up to f = n-t-1
-	// failures without marking the node unhealthy. See pkg/transport/registry.go
-	// for the full rationale.
+	// sufficient to form a t-of-n signing quorum.
+	//
+	// `threshold` uses the operator-facing "signers required" semantics
+	// (matches LiquidMPC CRD `threshold` field and mpcd --threshold CLI
+	// flag). For a 2-of-3 ensemble threshold=2; for 3-of-5 threshold=3.
+	// Quorum holds when readyCount >= threshold.
+	//
+	// NOTE: the CGGMP21 signing APIs still compare against keyInfo.Threshold+1
+	// internally — that's the polynomial degree, off by one from the
+	// operator threshold by protocol library convention. See
+	// pkg/transport/registry.go for the full rationale.
 	HasSigningQuorum(threshold int) bool
 	WatchPeersReady()
 	// Resign is called by the node when it is going to shutdown
@@ -207,12 +213,14 @@ func (r *registry) ArePeersReady() bool {
 	return r.ready
 }

-// HasSigningQuorum: see PeerRegistry interface comment. For the Consul-backed
-// legacy registry the readyCount is maintained the same way as the consensus
-// registry, so the check is identical.
+// HasSigningQuorum: see PeerRegistry interface comment and pkg/transport
+// for full semantics. Consul-backed registry behaves identically to the
+// consensus-backed one for this check.
 func (r *registry) HasSigningQuorum(threshold int) bool {
-	required := int64(threshold + 1)
-	return atomic.LoadInt64(&r.readyCount) >= required
+	if threshold <= 0 {
+		return atomic.LoadInt64(&r.readyCount) >= 1
+	}
+	return atomic.LoadInt64(&r.readyCount) >= int64(threshold)
 }

 func (r *registry) GetTotalPeersCount() int64 {
@@ -230,25 +230,31 @@ func (r *Registry) ArePeersReady() bool {
 }

 // HasSigningQuorum returns true when this node has enough ready peers
-// (including itself) to participate in a threshold signing round.
+// (including itself) to participate in a threshold signing round for the
+// operator-configured t-of-n ensemble.
 //
-// The CGGMP21 / FROST / LSS / SR25519 / BLS signing APIs require
-// `len(signerPeerIDs) >= keyInfo.Threshold + 1` — i.e. threshold+1 parties
-// (where threshold is the degree of the polynomial). A healthy node therefore
-// needs at least `threshold + 1` peers ready (including self) to contribute
-// to any signing round.
+// Semantics match the LiquidMPC CRD `threshold` field and the mpcd
+// --threshold CLI flag: `threshold` is the MINIMUM NUMBER OF SIGNERS
+// REQUIRED to produce a signature (t in t-of-n). A 3-node 2-of-3 ensemble
+// has threshold=2. A 5-node 3-of-5 ensemble has threshold=3.
 //
-// This is stricter than "can reach >= 1 peer" but more permissive than
-// ArePeersReady. For a 3-node ensemble with CGGMP21 `threshold=1` (2-of-3
-// signing), losing 1 peer leaves 2 ready → signing quorum holds. Losing 2
-// peers leaves 1 ready → no quorum, health reports degraded.
+// Quorum therefore holds when `readyCount >= threshold`, where readyCount
+// includes self. A 2-of-3 cluster with one peer down has readyCount=2 and
+// threshold=2 — quorum holds. If two peers drop, readyCount=1 < threshold=2
+// and quorum is lost.
 //
-// Callers should pass the CGGMP21/FROST polynomial-degree threshold, which is
-// what the KeyInfo.Threshold field stores. The "signers required" count is
-// threshold+1.
+// NOTE on the CGGMP21 vs operator-threshold split: pkg/mpc/node.go still
+// compares `len(signerPeerIDs) >= keyInfo.Threshold + 1`, where
+// KeyInfo.Threshold is the CGGMP21 polynomial degree (one below the
+// signers-required count). Those two variables are OFF BY ONE by design
+// of the protocol library: the CLI/CRD surfaces "signers required", while
+// the cryptographic API stores the polynomial degree. HasSigningQuorum
+// uses the operator surface — callers pass the CLI/CRD threshold directly.
 func (r *Registry) HasSigningQuorum(threshold int) bool {
-	required := int64(threshold + 1)
-	return atomic.LoadInt64(&r.readyCount) >= required
+	if threshold <= 0 {
+		return atomic.LoadInt64(&r.readyCount) >= 1
+	}
+	return atomic.LoadInt64(&r.readyCount) >= int64(threshold)
 }

 // GetReadyPeersCount returns number of ready peers
@@ -5,7 +5,11 @@
 // returned HTTP 503 because ArePeersReady() reported false (not all peers
 // connected), even though 2 nodes were sufficient to form a 2-of-3 signing
 // quorum. The fix added HasSigningQuorum(threshold) which is true when
-// readyCount >= threshold+1 (CGGMP21/FROST signers-required count).
+// readyCount >= threshold, where threshold uses the operator-facing
+// "minimum signers required" semantics (matches the LiquidMPC CRD
+// `threshold` field and mpcd --threshold CLI flag).
+//
+// For a 2-of-3 ensemble threshold=2; for 3-of-5 threshold=3.
 package transport

 import (
@@ -47,28 +51,28 @@ func markPeerDisconnected(r *Registry, peerID string) {
 }

 // Test_HasSigningQuorum_3Node_2of3 — the Scientist repro.
-// 3 nodes total, threshold=1 (i.e. 2-of-3: need 2 signers = threshold+1).
+// 3 nodes total, operator threshold=2 (2-of-3: 2 signers required).
 // Kill 1 peer → 2 ready (self + 1). Must still report quorum.
 func Test_HasSigningQuorum_3Node_2of3(t *testing.T) {
 	r := newTestRegistry("node0", []string{"node0", "node1", "node2"})
-	threshold := 1 // "2-of-3" in CGGMP21 semantics (signers = threshold+1 = 2)
+	threshold := 2 // 2-of-3: 2 signers required

-	// Bootstrap: only self ready.
+	// Bootstrap: only self ready (1 of 2 needed).
 	if r.HasSigningQuorum(threshold) {
-		t.Fatal("alone: 1 peer ready, threshold=1, required=2 — expected no quorum")
+		t.Fatal("alone: 1 peer ready, threshold=2 — expected no quorum")
 	}

 	// Peer 1 joins → 2 ready. Should form quorum.
 	markPeerReady(r, "node1")
 	if !r.HasSigningQuorum(threshold) {
-		t.Fatalf("2 ready, threshold=1, required=2 — expected quorum, got none. readyCount=%d",
+		t.Fatalf("2 ready, threshold=2 — expected quorum, got none. readyCount=%d",
 			r.GetReadyPeersCount())
 	}

 	// Peer 2 joins → 3 ready. Still quorum (no regression).
 	markPeerReady(r, "node2")
 	if !r.HasSigningQuorum(threshold) {
-		t.Fatalf("3 ready, threshold=1 — expected quorum")
+		t.Fatalf("3 ready, threshold=2 — expected quorum")
 	}

 	// Peer 2 disconnects → back to 2 ready. This is the Scientist repro:
@@ -78,21 +82,21 @@ func Test_HasSigningQuorum_3Node_2of3(t *testing.T) {
 		t.Fatalf("after peer disconnect expected readyCount=2, got %d", got)
 	}
 	if !r.HasSigningQuorum(threshold) {
-		t.Fatal("REGRESSION (Scientist P0-1): 2 ready, threshold=1 — quorum lost on single peer loss")
+		t.Fatal("REGRESSION (Scientist P0-1): 2 ready, threshold=2 — quorum lost on single peer loss")
 	}

 	// Peer 1 also disconnects → 1 ready. Now we genuinely have no quorum.
 	markPeerDisconnected(r, "node1")
 	if r.HasSigningQuorum(threshold) {
-		t.Fatal("1 ready alone, threshold=1, required=2 — expected no quorum")
+		t.Fatal("1 ready alone, threshold=2 — expected no quorum")
 	}
 }

 // Test_HasSigningQuorum_5Node_3of5 — larger ensemble.
-// 5 nodes, threshold=2 (3-of-5: signers = threshold+1 = 3).
+// 5 nodes, operator threshold=3 (3-of-5: 3 signers required).
 func Test_HasSigningQuorum_5Node_3of5(t *testing.T) {
 	r := newTestRegistry("node0", []string{"node0", "node1", "node2", "node3", "node4"})
-	threshold := 2
+	threshold := 3

 	// 1 ready (self only) — no quorum.
 	if r.HasSigningQuorum(threshold) {
@@ -108,20 +112,20 @@ func Test_HasSigningQuorum_5Node_3of5(t *testing.T) {
 	// 3 ready — exactly at required. Quorum.
 	markPeerReady(r, "node2")
 	if !r.HasSigningQuorum(threshold) {
-		t.Fatalf("3 ready, threshold=2, required=3 — expected quorum, got none. readyCount=%d",
+		t.Fatalf("3 ready, threshold=3 — expected quorum, got none. readyCount=%d",
 			r.GetReadyPeersCount())
 	}

 	// 4 ready — above required. Quorum.
 	markPeerReady(r, "node3")
 	if !r.HasSigningQuorum(threshold) {
-		t.Fatal("4 ready, threshold=2 — expected quorum")
+		t.Fatal("4 ready, threshold=3 — expected quorum")
 	}

 	// 5 ready — all ready. Quorum AND ArePeersReady would be true if transport set it.
 	markPeerReady(r, "node4")
 	if !r.HasSigningQuorum(threshold) {
-		t.Fatal("5 ready, threshold=2 — expected quorum")
+		t.Fatal("5 ready, threshold=3 — expected quorum")
 	}

 	// Two simultaneous failures → 3 ready. Still quorum (no regression).
@@ -131,13 +135,13 @@ func Test_HasSigningQuorum_5Node_3of5(t *testing.T) {
 		t.Fatalf("after 2 disconnects expected readyCount=3, got %d", got)
 	}
 	if !r.HasSigningQuorum(threshold) {
-		t.Fatal("3 ready after 2 failures, threshold=2 — expected quorum")
+		t.Fatal("3 ready after 2 failures, threshold=3 — expected quorum")
 	}

 	// Third failure → 2 ready. Quorum lost (correctly).
 	markPeerDisconnected(r, "node2")
 	if r.HasSigningQuorum(threshold) {
-		t.Fatal("2 ready, threshold=2, required=3 — expected NO quorum (f-1 tolerance exceeded)")
+		t.Fatal("2 ready, threshold=3 — expected NO quorum (f-1 tolerance exceeded)")
 	}
 }

@@ -146,13 +150,13 @@ func Test_HasSigningQuorum_5Node_3of5(t *testing.T) {
 // HasSigningQuorum is the relaxed check used by /healthz.
 func Test_HasSigningQuorum_ArePeersReady_Decoupled(t *testing.T) {
 	r := newTestRegistry("node0", []string{"node0", "node1", "node2"})
-	threshold := 1
+	threshold := 2

 	// Two peers ready, one missing. HasSigningQuorum=true, ArePeersReady=false.
 	markPeerReady(r, "node1")

 	if !r.HasSigningQuorum(threshold) {
-		t.Fatal("expected quorum with 2 of 3 ready")
+		t.Fatal("expected quorum with 2 of 3 ready and threshold=2")
 	}

 	// ArePeersReady derives from r.ready which is set via checkPeerConnections;
@@ -164,21 +168,30 @@ func Test_HasSigningQuorum_ArePeersReady_Decoupled(t *testing.T) {
 }

 // Test_HasSigningQuorum_BoundaryCheck covers the exact threshold arithmetic
-// to pin the semantics: required = threshold + 1, NOT threshold.
+// to pin the semantics: required = threshold (the operator-facing minimum
+// signers required), NOT threshold+1.
 func Test_HasSigningQuorum_BoundaryCheck(t *testing.T) {
 	cases := []struct {
-		name        string
-		readyCount  int64
-		threshold   int
-		wantQuorum  bool
+		name       string
+		readyCount int64
+		threshold  int
+		wantQuorum bool
 	}{
+		// threshold<=0 is a degenerate config — treat as "any live node is fine".
 		{"threshold=0, ready=0 → no quorum (need >=1)", 0, 0, false},
-		{"threshold=0, ready=1 → quorum (single-party sign)", 1, 0, true},
-		{"threshold=1, ready=1 → no quorum (need >=2)", 1, 1, false},
-		{"threshold=1, ready=2 → quorum", 2, 1, true},
-		{"threshold=2, ready=2 → no quorum (need >=3)", 2, 2, false},
+		{"threshold=0, ready=1 → quorum (degenerate)", 1, 0, true},
+		{"threshold=-1, ready=1 → quorum (degenerate)", 1, -1, true},
+		// 2-of-3 (threshold=2) boundary.
+		{"threshold=2, ready=1 → no quorum", 1, 2, false},
+		{"threshold=2, ready=2 → quorum (2-of-3 survives 1 failure)", 2, 2, true},
 		{"threshold=2, ready=3 → quorum", 3, 2, true},
-		{"threshold=2, ready=4 → quorum", 4, 2, true},
+		// 3-of-5 (threshold=3) boundary.
+		{"threshold=3, ready=2 → no quorum", 2, 3, false},
+		{"threshold=3, ready=3 → quorum (3-of-5 survives 2 failures)", 3, 3, true},
+		{"threshold=3, ready=5 → quorum", 5, 3, true},
+		// Larger ensembles.
+		{"threshold=4, ready=3 → no quorum", 3, 4, false},
+		{"threshold=4, ready=4 → quorum", 4, 4, true},
 	}

 	for _, tc := range cases {