feat: correlate governancethreshold with reconstructionthreshold (near#3578)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Mårten Blankfors <marten@blankfors.se>
Co-authored-by: kevindeforth <32777623+kevindeforth@users.noreply.github.com>

Author: 4 people

crates/contract/src/errors.rs

@@ -178,11 +178,22 @@ pub enum InvalidThreshold {
     #[error("Threshold does not meet the minimum absolute requirement")]
     MinAbsRequirementFailed,
     #[error(
-        "Threshold does not meet the minimum relative requirement: require at least {required}, found {found}"
+        "GovernanceThreshold is below the minimum required relative to the participant count: require at least {required}, found {found}"
     )]
     MinRelRequirementFailed { required: u64, found: u64 },
     #[error("Threshold must not exceed number of participants: max {max}, found {found}")]
     MaxRequirementFailed { max: u64, found: u64 },
+    #[error(
+        "GovernanceThreshold exceeds the maximum allowed relative to the participant count: max {max}, found {found}"
+    )]
+    MaxRelRequirementFailed { max: u64, found: u64 },
+    #[error(
+        "GovernanceThreshold {governance_threshold} is below the largest ReconstructionThreshold {reconstruction_threshold}"
+    )]
+    BelowReconstructionThreshold {
+        reconstruction_threshold: u64,
+        governance_threshold: u64,
+    },
 }
 
 #[derive(Debug, PartialEq, Eq, Clone, thiserror::Error)]

crates/contract/src/lib.rs

@@ -58,8 +58,7 @@ use crypto_shared::{
     types::{PublicKeyExtended, PublicKeyExtendedConversionError},
 };
 use errors::{
-    DomainError, InvalidParameters, InvalidState, InvalidThreshold, PublicKeyError, RespondError,
-    TeeError,
+    DomainError, InvalidParameters, InvalidState, PublicKeyError, RespondError, TeeError,
 };
 use k256::elliptic_curve::PrimeField;
 use near_mpc_contract_interface::types::Ed25519PublicKey;
@@ -79,7 +78,7 @@ use near_sdk::{
 };
 use node_migrations::NodeMigrations;
 use primitives::{
-    domain::DomainRegistry,
+    domain::{DomainRegistry, max_reconstruction_threshold},
     key_state::{AuthenticatedParticipantId, EpochId, KeyEventId, Keyset},
     signature::{SignRequestArgs, SignatureRequest, YieldIndex},
     thresholds::{ProposedThresholdParameters, Threshold, ThresholdParameters},
@@ -925,10 +924,6 @@ impl MpcContract {
             proposal,
         );
 
-        // Defense in depth: never reshare into a participant set smaller than any
-        // threshold (see `assert_proposal_meets_all_thresholds`).
-        self.assert_proposal_meets_all_thresholds(&proposal)?;
-
         let tee_upgrade_deadline_duration =
             Duration::from_secs(self.config.tee_upgrade_deadline_duration_seconds);
 
@@ -971,47 +966,6 @@ impl MpcContract {
         }
     }
 
-    /// Defense-in-depth guard for [`Self::vote_new_parameters`]: rejects a
-    /// proposal whose participant set is smaller than the proposed signing
-    /// threshold or any domain's effective reconstruction threshold (proposed
-    /// override if present, else the domain's current value). Such a set would
-    /// leave a key un-signable or un-reconstructible. Redundant with
-    /// `RunningContractState::process_new_parameters_proposal`.
-    fn assert_proposal_meets_all_thresholds(
-        &self,
-        proposal: &ProposedThresholdParameters,
-    ) -> Result<(), Error> {
-        let num_participants = u64::try_from(proposal.participants().len())
-            .expect("participant list should be wayyyy smaller than u64::MAX");
-
-        let threshold = proposal.threshold().value();
-        if threshold > num_participants {
-            return Err(InvalidThreshold::MaxRequirementFailed {
-                max: num_participants,
-                found: threshold,
-            }
-            .into());
-        }
-
-        let domains = self.protocol_state.domain_registry()?;
-        let updates = proposal.per_domain_thresholds();
-        for domain in domains.domains() {
-            let effective = updates
-                .get(&domain.id)
-                .copied()
-                .unwrap_or(domain.reconstruction_threshold)
-                .inner();
-            if effective > num_participants {
-                return Err(DomainError::ReconstructionThresholdExceedsParticipants {
-                    threshold: effective,
-                    participants: num_participants,
-                }
-                .into());
-            }
-        }
-        Ok(())
-    }
-
     /// Propose adding a new set of domains for the MPC network.
     /// If a threshold number of votes are reached on the exact same proposal, this will transition
     /// the contract into the Initializing state to generate keys for the new domains.
@@ -1744,26 +1698,24 @@ impl MpcContract {
             TeeValidationResult::Partial {
                 participants_with_valid_attestation,
             } => {
-                let threshold = current_params.threshold().value() as usize;
                 let remaining = participants_with_valid_attestation.len();
-                // Defense in depth: the surviving participant set must cover every
-                // threshold the network is bound to — the governance threshold and
-                // each domain's reconstruction threshold (the kickout keeps the
-                // existing per-domain thresholds). Resharing into a smaller set
-                // would leave a key un-signable, so we refuse and wait for manual
-                // intervention.
-                let max_reconstruction_threshold = running_state
-                    .domains
-                    .domains()
-                    .iter()
-                    .map(|domain| domain.reconstruction_threshold.inner() as usize)
-                    .max()
-                    .unwrap_or(0);
-                let required = threshold.max(max_reconstruction_threshold);
-                if required > remaining {
+                // Defense in depth: the surviving participant set must keep the full
+                // threshold relation intact — the GovernanceThreshold must still sit
+                // within its bounds for the smaller set (in particular it must not
+                // exceed the remaining participant count or the upper cap) and must
+                // remain at least every domain's ReconstructionThreshold (the kickout
+                // keeps the existing per-domain thresholds). Otherwise we refuse and
+                // wait for manual intervention.
+                let max_reconstruction_threshold =
+                    max_reconstruction_threshold(running_state.domains.domains());
+                if let Err(err) = ThresholdParameters::validate_governance_against_reconstruction(
+                    u64::try_from(remaining).expect("participant count fits in u64"),
+                    current_params.threshold(),
+                    max_reconstruction_threshold,
+                ) {
                     log!(
-                        "Fewer than the required number of participants ({}) are left with a valid TEE status ({}). This requires manual intervention. We will not accept new signature requests as a safety precaution.",
-                        required,
+                        "Kicking out participants with an invalid TEE status would break the threshold relation ({:?}); {} participants remain with a valid TEE status. This requires manual intervention. We will not accept new signature requests as a safety precaution.",
+                        err,
                         remaining,
                     );
                     self.accept_requests = false;
@@ -1778,7 +1730,8 @@ impl MpcContract {
                 //let n_participants_new = new_participants.len();
                 //let new_threshold = (3 * n_participants_new + 4) / 5; // minimum 60%
                 //let new_threshold = new_threshold.max(2); // but also minimum 2
-                let new_threshold = threshold;
+                let new_threshold = usize::try_from(current_params.threshold().value())
+                    .expect("threshold value fits in usize");
 
                 let threshold_parameters = ThresholdParameters::new(
                     participants_with_valid_attestation,
@@ -2009,6 +1962,12 @@ impl MpcContract {
         for domain in domains.domains() {
             crate::primitives::domain::validate_domain_threshold(domain, num_participants)?;
         }
+        // Keep the GovernanceThreshold at least as large as the largest ReconstructionThreshold.
+        ThresholdParameters::validate_governance_against_reconstruction(
+            num_participants,
+            parameters.threshold(),
+            max_reconstruction_threshold(domains.domains()),
+        )?;
 
         // Check that the domains match exactly those in the keyset.
         let domain_ids_from_domains = domains.domains().iter().map(|d| d.id).collect::<Vec<_>>();
@@ -2668,7 +2627,7 @@ mod tests {
     };
 
     use super::*;
-    use crate::errors::NodeMigrationError;
+    use crate::errors::{InvalidCandidateSet, InvalidThreshold, NodeMigrationError};
     use crate::pending_requests::MAX_PENDING_REQUEST_FAN_OUT;
     use crate::primitives::participants::{ParticipantId, ParticipantInfo, Participants};
     use crate::primitives::test_utils::{
@@ -4128,11 +4087,10 @@ mod tests {
     }
 
     #[test]
-    fn vote_new_parameters__should_reject_when_shrinking_below_unchanged_domain_threshold() {
-        // Given: a Running contract with 3 participants and a domain whose
-        // reconstruction threshold is 3.
+    fn vote_new_parameters__should_reject_when_shrinking_below_governance_threshold() {
+        // Given: a Running contract with 4 participants and a GovernanceThreshold of 3.
         let (mut contract, participants, signer, _domain_id) =
-            setup_running_contract_with_domain(3, 3, 3);
+            setup_running_contract_with_domain(4, 3, 3);
         // ...and a proposal that shrinks the participant set to 2 without touching
         // the per-domain thresholds.
         let proposal = ProposedThresholdParameters::new(
@@ -4143,14 +4101,13 @@ mod tests {
         // When
         let result = vote_params(&mut contract, &signer, &proposal);
 
-        // Then: the domain's unchanged threshold of 3 exceeds the 2 proposed
-        // participants, so the guard rejects it.
+        // Then: the candidate-set guard rejects the proposal first, because only 2
+        // old participants remain — fewer than the GovernanceThreshold of 3. (Under
+        // the GovernanceThreshold >= max(ReconstructionThreshold) invariant this guard
+        // always fires before any per-domain ReconstructionThreshold check could.)
         assert_matches!(
             result.unwrap_err(),
-            Error::DomainError(DomainError::ReconstructionThresholdExceedsParticipants {
-                threshold: 3,
-                participants: 2,
-            })
+            Error::InvalidCandidateSet(InvalidCandidateSet::InsufficientOldParticipants)
         );
     }
 
@@ -4177,15 +4134,15 @@ mod tests {
 
     #[test]
     fn vote_new_parameters__should_accept_per_domain_threshold_within_participant_count() {
-        // Given: a Running contract with 3 participants and one domain.
+        // Given: a Running contract with 5 participants (GovernanceThreshold 4) and one domain.
         let (mut contract, participants, signer, domain_id) =
-            setup_running_contract_with_domain(3, 2, 2);
-        // ...and a proposal raising the domain's reconstruction threshold to 3,
-        // which still fits the 3 participants.
+            setup_running_contract_with_domain(5, 4, 2);
+        // ...and a proposal raising the domain's reconstruction threshold to 4,
+        // which fits the 5 participants and does not exceed the GovernanceThreshold.
         let mut per_domain = BTreeMap::new();
-        per_domain.insert(domain_id, ReconstructionThreshold::new(3));
+        per_domain.insert(domain_id, ReconstructionThreshold::new(4));
         let proposal = ProposedThresholdParameters::new(
-            ThresholdParameters::new(participants, Threshold::new(2)).unwrap(),
+            ThresholdParameters::new(participants, Threshold::new(4)).unwrap(),
             per_domain,
         );
 
@@ -4196,6 +4153,33 @@ mod tests {
         assert_matches!(result, Ok(()));
     }
 
+    #[test]
+    fn vote_new_parameters__should_reject_governance_below_max_reconstruction() {
+        // Given: a Running contract with 5 participants, GovernanceThreshold 4, and a
+        // domain whose reconstruction threshold is 4.
+        let (mut contract, participants, signer, _domain_id) =
+            setup_running_contract_with_domain(5, 4, 4);
+        // ...and a proposal lowering the GovernanceThreshold to 3 (valid on its own)
+        // while the domain keeps its reconstruction threshold of 4.
+        let proposal = ProposedThresholdParameters::new(
+            ThresholdParameters::new(participants, Threshold::new(3)).unwrap(),
+            BTreeMap::new(),
+        );
+
+        // When
+        let result = vote_params(&mut contract, &signer, &proposal);
+
+        // Then: the GovernanceThreshold (3) would fall below the domain's
+        // reconstruction threshold (4), so the proposal is rejected.
+        assert_matches!(
+            result.unwrap_err(),
+            Error::InvalidThreshold(InvalidThreshold::BelowReconstructionThreshold {
+                reconstruction_threshold: 4,
+                governance_threshold: 3,
+            })
+        );
+    }
+
     #[test]
     #[should_panic(expected = "Caller must be the signer account")]
     fn vote_new_parameters__should_panic_when_predecessor_differs_from_signer() {
@@ -5652,19 +5636,26 @@ mod tests {
     }
 
     /// Tests that [`MpcContract::verify_tee`] refuses to reshare when a TEE
-    /// kickout would leave fewer participants than a domain's reconstruction
-    /// threshold, even though the remaining set still meets the governance
-    /// threshold. The contract stays Running and stops accepting requests.
+    /// kickout would leave fewer participants than the threshold relation requires.
+    /// The contract stays Running and stops accepting requests.
     #[test]
-    fn verify_tee__should_refuse_kickout_below_domain_reconstruction_threshold() {
+    fn verify_tee__should_refuse_kickout_when_remaining_breaks_threshold_relation() {
         const PARTICIPANT_COUNT: usize = 5;
         const ATTESTATION_EXPIRY_SECONDS: u64 = 5;
         const TEE_UPGRADE_DURATION: Duration = Duration::MAX;
 
-        // Given: 5 participants, governance threshold 3, and one domain whose
-        // reconstruction threshold is 5 (every participant is needed to sign).
+        // Given: 5 participants, GovernanceThreshold 5, and one domain whose
+        // reconstruction threshold is 5 (every participant is needed to sign). Dropping
+        // to 4 participants would leave the GovernanceThreshold above the participant
+        // count, breaking the threshold relation.
         let participants = gen_participants(PARTICIPANT_COUNT);
-        let parameters = ThresholdParameters::new(participants.clone(), Threshold::new(3)).unwrap();
+        let parameters = ThresholdParameters::new(
+            participants.clone(),
+            Threshold::new(
+                u64::try_from(PARTICIPANT_COUNT).expect("participant count fits in u64"),
+            ),
+        )
+        .unwrap();
         let domain_id = DomainId::default();
         let domains = vec![DomainConfig {
             id: domain_id,
@@ -5717,9 +5708,9 @@ mod tests {
         // When
         let result = contract.verify_tee();
 
-        // Then: the 4 surviving participants meet the governance threshold (3) but
-        // not the domain's reconstruction threshold (5), so verify_tee refuses to
-        // reshare, stays Running, and stops accepting requests.
+        // Then: with only 4 surviving participants the GovernanceThreshold of 5 would
+        // exceed the participant count, breaking the threshold relation, so verify_tee
+        // refuses to reshare, stays Running, and stops accepting requests.
         assert_matches!(result, Ok(false));
         assert_matches!(contract.protocol_state, ProtocolContractState::Running(_));
         assert!(!contract.accept_requests);

crates/contract/src/primitives/domain.rs

@@ -72,6 +72,16 @@ pub fn validate_domain_threshold(
     Ok(())
 }
 
+/// The largest `ReconstructionThreshold` across `domains`, or `None` if there are none
+/// (an empty set imposes no cross-domain lower bound on the GovernanceThreshold).
+/// Feeds [`ThresholdParameters::validate_governance_against_reconstruction`](crate::primitives::thresholds::ThresholdParameters::validate_governance_against_reconstruction).
+pub fn max_reconstruction_threshold(domains: &[DomainConfig]) -> Option<ReconstructionThreshold> {
+    domains
+        .iter()
+        .map(|domain| domain.reconstruction_threshold)
+        .max()
+}
+
 /// All the domains present in the contract, as well as the next domain ID which is kept to ensure
 /// that we never reuse domain IDs. (Domains may be deleted in only one case: when we decided to
 /// add domains but ultimately canceled that process.)

crates/contract/src/primitives/test_utils.rs

@@ -3,15 +3,18 @@ use crate::{
     crypto_shared::types::{PublicKeyExtended, serializable::SerializableEdwardsPoint},
     primitives::{
         participants::{ParticipantInfo, Participants},
-        thresholds::{ProposedThresholdParameters, Threshold, ThresholdParameters},
+        thresholds::{
+            ProposedThresholdParameters, Threshold, ThresholdParameters,
+            governance_threshold_lower_relative_bound, governance_threshold_upper_relative_bound,
+        },
     },
 };
 use curve25519_dalek::edwards::CompressedEdwardsY;
 use near_account_id::AccountId;
 use near_mpc_contract_interface::types::{
     DomainConfig, DomainId, DomainPurpose, Protocol, ReconstructionThreshold,
 };
-use rand::{Rng, distributions::Uniform};
+use rand::{Rng, SeedableRng, distributions::Uniform, rngs::StdRng};
 use std::collections::BTreeMap;
 // Re-export for convenience
 
@@ -129,10 +132,6 @@ pub fn gen_participant(i: usize) -> (AccountId, ParticipantInfo) {
     )
 }
 
-pub fn min_thrershold(n: usize) -> usize {
-    ((n as f64) * 0.6).ceil() as usize
-}
-
 pub fn gen_accounts_and_info(n: usize) -> BTreeMap<AccountId, ParticipantInfo> {
     (0..n).map(gen_participant).collect()
 }
@@ -157,9 +156,11 @@ pub fn gen_threshold_params(max_n: usize) -> ThresholdParameters {
     // Lower bound is 3 (not 2) so the produced parameters are compatible with
     // every protocol — `DamgardEtAl` requires `n >= 2t - 1`, which forces
     // `n >= 3` even at the minimum `t = 2`.
-    let n: usize = rand::thread_rng().gen_range(3..max_n + 1);
-    let k_min = min_thrershold(n);
-    let k = rand::thread_rng().gen_range(k_min..n + 1);
+    let mut rng = StdRng::seed_from_u64(42);
+    let n: usize = rng.gen_range(3..max_n + 1);
+    let k_min = governance_threshold_lower_relative_bound(n as u64) as usize;
+    let k_max = governance_threshold_upper_relative_bound(n as u64) as usize;
+    let k = rng.gen_range(k_min..k_max + 1);
     ThresholdParameters::new(gen_participants(n), Threshold::new(k as u64)).unwrap()
 }