feat: correlate governancethreshold with reconstructionthreshold by SimonRastikian · Pull Request #3578 · near/mpc

SimonRastikian · 2026-06-15T23:08:59Z

…same number of participants

claude · 2026-06-15T23:18:10Z

Pull request overview

This PR enforces the cross-domain rule max(ReconstructionThreshold) <= GovernanceThreshold <= floor(0.8 * n) (closes #3499). The upper cap prevents a small minority from locking governance, and the lower bound keeps trust assumptions aligned with the cryptographic reconstruction requirement. The DTO conversion for ThresholdParameters/ProposedThresholdParameters becomes fallible (TryIntoContractType), and a new helper validate_governance_against_reconstruction layers the cross-domain rule on top of the existing absolute/relative bounds. The relation is enforced at every mutation point (init_running, vote_new_parameters, vote_add_domains, verify_tee) and re-validated once during 3.11.2 migration, where a violation hard-panics.

Changes:

Add MAX_THRESHOLD_NUMERATOR/DENOMINATOR upper cap (clamped up to the 60% lower bound for small n) and new error variants MaxRelRequirementFailed, BelowReconstructionThreshold, ReconstructionThresholdExceedsGovernance.
New ThresholdParameters::validate_governance_against_reconstruction(n, governance, max_reconstruction) and helper max_threshold(n) in test_utils.
DTO mapping switches to TryIntoContractType to validate at the contract boundary.
assert_proposal_meets_all_thresholds enforces the relation across effective per-domain thresholds; vote_add_domains enforces reconstruction <= governance per added domain; verify_tee refuses TEE-driven reshares that would break the relation.
One-time migration pass in v3_11_2_state::validate_threshold_relation_on_migration panics if the 3.11.2 state violates the new rules.
Sandbox/integration tests adjusted: retained subsets now use n >= ceil(threshold * 5 / 4) so the upper cap is satisfied; design doc §7.1 marked RESOLVED.

Reviewed changes

Per-file summary

File	Description
`crates/contract/src/primitives/thresholds.rs`	Adds upper cap to `validate_threshold` (clamped to lower bound), introduces `validate_governance_against_reconstruction`; updates and adds tests.
`crates/contract/src/errors.rs`	New error variants: `MaxRelRequirementFailed`, `BelowReconstructionThreshold`, `ReconstructionThresholdExceedsGovernance`.
`crates/contract/src/dto_mapping.rs`	DTO -> contract conversion for thresholds becomes fallible; rejects invalid params at the boundary.
`crates/contract/src/lib.rs`	`vote_new_parameters`, `init`, `init_running`, `verify_tee` wired to the new validation; `assert_proposal_meets_all_thresholds` enforces the relation; new tests.
`crates/contract/src/state/running.rs`	`vote_add_domains` rejects new domains whose reconstruction threshold exceeds governance; new tests.
`crates/contract/src/state/resharing.rs`	Reproposal test uses thresholds inside the new cap.
`crates/contract/src/v3_11_2_state.rs`	Migration re-validates the relation and panics on violation.
`crates/contract/src/primitives/test_utils.rs`	New `max_threshold(n)` helper; `gen_threshold_params` samples within the new window.
`crates/contract/tests/sandbox/{sign,tee_cleanup_after_resharing,update_votes_cleanup_after_resharing}.rs`	Retained subsets resized to satisfy the upper cap.
`docs/design/domain-separation.md`	§7.1 marked RESOLVED with concrete rules and implementation pointers.

Findings

Blocking (must fix before merge):

None.

Non-blocking (worth addressing before/after merge):

crates/contract/src/v3_11_2_state.rs:170 — Production safety: the migration hard-panics when existing state violates the new bounds. Pre-PR rules allowed GovernanceThreshold up to n (no upper cap), so any deployed contract whose current governance threshold sits above floor(0.8 * n) will fail to upgrade. Worth confirming against current mainnet/testnet state before merging, and worth adding a unit test that exercises this panic path so future regressions in the upper-bound logic are caught. The fix path the message points users to ("correct via vote_new_parameters before upgrading") only works on the old contract; consider calling this out in the changelog / upgrade notes too.
crates/contract/src/primitives/thresholds.rs:73 — The clamp (MAX_NUMERATOR * n_shares / MAX_DENOMINATOR).max(lower_relative_bound) is correct (and matches the pattern of (3 * n_shares).div_ceil(5) two lines above), but at the smallest valid sizes (n=3,4) the window collapses to a single value ({2} and {3} respectively). That's intentional but quite restrictive — e.g. for n=4 only one governance threshold is allowed (3). Consider whether to call this out in operator documentation, since operators choosing a 4-node cluster now have zero room to tune.
crates/contract/src/lib.rs:1650 — The kickout-refusal log emits only the error's Debug formatting; consider also logging the (remaining, governance, max_reconstruction) tuple explicitly so operators reading logs don't need to decode the error variant to know which bound failed.
crates/contract/src/state/running.rs:218 — vote_add_domains checks each new domain's reconstruction threshold against the current governance threshold with an inline comparison rather than going through validate_governance_against_reconstruction. Correct, but consider unifying so the cross-domain invariant has a single source of truth that's harder to drift away from.
crates/contract/src/lib.rs:944 — assert_proposal_meets_all_thresholds iterates only domains present in the current registry and silently ignores per_domain_thresholds entries pointing at unknown domain IDs. process_new_parameters_proposal rejects unknown IDs downstream (running.rs:159), so this is defense-in-depth and not a correctness gap — worth a brief comment so a future reader doesn't try to "fix" the apparent omission.

✅ Approved

… possible value due to 60%-80% range

…pper cap

…_bound as helpers

SimonRastikian

The reviewer please double (triple) check that I have covered all the necessary functions with these checks, namely all functions that:
add / remove / kick out participants
update the contract participant set
update the reconstruction threshold
update the governance threshold

SimonRastikian · 2026-06-16T10:37:59Z

        for domain in &effective_domains {
            validate_domain_threshold(domain, new_num_participants)?;
        }

+        // The GovernanceThreshold must dominate every domain's effective ReconstructionThreshold;
+        // enforced here so the state transition is self-contained (single source of truth).
+        ThresholdParameters::validate_governance_against_reconstruction(
+            new_num_participants,
+            proposal.threshold(),
+            max_reconstruction_threshold(&effective_domains),
+        )?;


Here there is a tiny redundancy in checks namely that reconstruction <= num_participants. but otherwise 179-181 checks extra that reconstruction > 2 and that 2*reconstruction-1 < num_participants when in DamgardEtAl.

kevindeforth

I am not sure we want to cap the threshold at 80% in the code.
Makes testing harder and it's not obvious to me why we would impose such a requirement.

…nstructionthreshold

…inator) Colleagues did not agree on an 80% upper bound for the GovernanceThreshold, so set MAX_THRESHOLD_NUMERATOR = MAX_THRESHOLD_DENOMINATOR (5/5 = 100%). The relative upper cap structure is kept but never binds below the absolute `k <= n` check, so the GovernanceThreshold may again go up to the participant count. The cross-domain rule (GovernanceThreshold >= max(ReconstructionThreshold)) is unchanged. Revert the test changes that were only needed to satisfy the 80% cap (dropping thresholds / raising participant counts) and remove the now-meaningless dedicated upper-cap tests: - thresholds.rs: restore 5/5-participant thresholds; drop reject-above-cap test - dto_mapping.rs / lib.rs: drop the upper-cap rejection tests - lib.rs verify_tee: rework the kickout-refusal fixture to break the relation via the participant-count ceiling instead of the cap - running.rs: make the reconstruction>governance test regenerate until gov < n - sandbox + e2e + node resharing tests: restore original participant/threshold values - docs/design/domain-separation.md: describe the cap as disabled (100%) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…hreshold' of github.com:near/mpc into 3499-correlate-governancethreshold-with-reconstructionthreshold

…hreshold' of github.com:near/mpc into 3499-correlate-governancethreshold-with-reconstructionthreshold # Conflicts: # crates/contract/src/dto_mapping.rs

Co-authored-by: kevindeforth <32777623+kevindeforth@users.noreply.github.com>

…ance threshold in tests

… cap is disabled

gilcu3

Thank you!

Just a couple of minor changes requested

There is a stale TODO
There is a removed test that seems to cover an execution path no longer covered by other tests, see this comment (not sure the link will work)

gilcu3 · 2026-06-22T13:37:21Z

-    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);


technically this should be a parameter to the function, but for the sake of keeping this simple we can leave it here

As you said, it's simpler this way

I don't think this is an improvement without passing the RNG as a parameter, because this is a pretty low-level helper method, so we will now generate the exact same threshold params for any given n for most of our tests.

@SimonRastikian please modify in this PR or open a follow-up for this.

gilcu3 · 2026-06-22T14:19:45Z

-    #[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.
-        let (mut contract, participants, signer, _domain_id) =
-            setup_running_contract_with_domain(3, 3, 3);
-        // ...and a proposal that shrinks the participant set to 2 without touching
-        // the per-domain thresholds.
-        let proposal = ProposedThresholdParameters::new(
-            ThresholdParameters::new(participants.subset(0..2), Threshold::new(2)).unwrap(),
-            BTreeMap::new(),
-        );
-
-        // 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.
-        assert_matches!(
-            result.unwrap_err(),
-            Error::DomainError(DomainError::ReconstructionThresholdExceedsParticipants {
-                threshold: 3,
-                participants: 2,
-            })
-        );
-    }
-


I believe the two tests are not equivalent, appear to touch a different path of execution, so please restore this one. That they assert the same error does not mean they cover the same cases

…hreshold' of github.com:near/mpc into 3499-correlate-governancethreshold-with-reconstructionthreshold

gilcu3

Thank you!

left only a nit (over a hack)

kevindeforth

Thank you, looks good!

I have one concern about making a previously randomized helper method non-random, which means that many tests now us the exact same setup. Needs to be addressed, but can be done in a follow-up.

kevindeforth · 2026-06-23T08:07:55Z

+    pub fn validate_governance_against_reconstruction(
+        num_participants: u64,
+        governance: Threshold,
+        max_reconstruction_threshold: Option<ReconstructionThreshold>,


Note (non-blocking): I don't like this being optional. I would prefer this to be a u64.
Reason for this is that as a reviewer, I get more suspicious when I see a function call passing a magical 0, compared to a function passing a magical None.

kevindeforth · 2026-06-23T08:12:56Z

+    #[test]
+    fn validate_threshold__should_not_produce_empty_window_for_small_n() {
+        // The relative upper cap is clamped up to the ceil(0.6n) lower bound, so the
+        // feasible window must always hold at least one valid threshold.
+        for n in 2..=12u64 {
+            let lower = governance_threshold_lower_relative_bound(n);
+            let upper = governance_threshold_upper_relative_bound(n);
+            assert!(upper >= lower, "empty window at n={n}: [{lower}, {upper}]");
+            // The clamped boundary value must validate.
+            ThresholdParameters::validate_threshold(n, Threshold::new(upper)).unwrap();
+        }
+    }


Note: This test seems redundant, as it is covered by test_validate_threshold, but we can keep it.

kevindeforth · 2026-06-23T08:18:52Z

+    fn validate_governance_against_reconstruction__should_reject_governance_below_max_reconstruction()
+     {
+        // Given 10 participants and a governance threshold of 6 (a valid value on its own).
+        let n = 10;
+        let governance = Threshold::new(6);
+        // When the largest reconstruction threshold is 7 (above governance).
+        // Then the relation is rejected.
+        assert_matches!(
+            ThresholdParameters::validate_governance_against_reconstruction(
+                n,
+                governance,
+                Some(ReconstructionThreshold::new(7))
+            ),
+            Err(Error::InvalidThreshold(
+                InvalidThreshold::BelowReconstructionThreshold {
+                    reconstruction_threshold: 7,
+                    governance_threshold: 6,
+                }
+            ))
+        );
+        // ...but is accepted when governance meets or exceeds the max reconstruction threshold.
+        ThresholdParameters::validate_governance_against_reconstruction(
+            n,
+            governance,
+            Some(ReconstructionThreshold::new(6)),
+        )
+        .unwrap();
+        ThresholdParameters::validate_governance_against_reconstruction(
+            n,
+            governance,
+            Some(ReconstructionThreshold::new(5)),
+        )


Note: this test would be nicer if it looped over a bunch of values instead of just one.

kevindeforth · 2026-06-23T08:21:18Z

-    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);


I don't think this is an improvement without passing the RNG as a parameter, because this is a pretty low-level helper method, so we will now generate the exact same threshold params for any given n for most of our tests.

@SimonRastikian please modify in this PR or open a follow-up for this.

kevindeforth · 2026-06-23T08:28:33Z

        env.set_signer(&state.parameters.participants().participants()[0].0);
-        let n = state.parameters.participants().len() as u64;
-        let proposal = single_domain_proposal(&state, Protocol::CaitSith, DomainPurpose::Sign, n);
+        // Use the GovernanceThreshold as the ReconstructionThreshold (the maximum allowed).


Do our tests pass if we have more variety here and select a threshold in [2, governance_threshold]?

kevindeforth · 2026-06-23T08:33:26Z

 }
-/// Generates a Running state that contains this many domains.
+/// Generates a Running state that contains this many domains, with randomly
+/// generated threshold parameters.


Please update the doc comment to reflect the new behavior gen_threshold_params will output the exact same threshold for the same n, it's not really "random" anymore.

kevindeforth · 2026-06-23T08:37:31Z

+/// Like [`gen_running_state`], but pins the participant count and
+/// GovernanceThreshold instead of randomizing them.


Note: It's overhead for the reader to look-up what gen_running_state does.

…r#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>

SimonRastikian added 5 commits June 15, 2026 23:12

make the two threshold DTO conversions fallible & validating

02dd798

Adding validation (upper cap and wrt the ReconstructionThreshold)

44e9e76

Updating tests now GovThreshold is capped at 80% no more allowed the …

7d86856

…same number of participants

Panic in migration misconfig

b33f455

Remove migration function after migration

02c5313

SimonRastikian self-assigned this Jun 16, 2026

SimonRastikian added 16 commits June 16, 2026 09:48

Raising the number of participants in the test to meet the 80% upper cap

a378c56

Raising extra tests number of particpants to meet the 80% cap

3da1652

Adding note documentation about cases where the threshold is a single…

aeed6e7

… possible value due to 60%-80% range

Using already existent helpers in vote_add_domains

7257a16

Bump the number of participants for parallel_sign_calls to meet 80% u…

a20a7a8

…pper cap

make process_new_parameters_proposal self-contained

1afd958

Removing redundant function

db47b85

Obsolete test as the check happens in the underlying function

0cf1b53

make function private

27e40bf

Fixing tests with govthreshold = number of participants

7878ac8

More participants during resharing

1f28966

Raising the number of participants to 7

7bca4d8

Using governance_threshold_lower_bound and governance_threshold_upper…

263bc8d

…_bound as helpers

cargo fmt

f45ebf5

reducing code redundancy

c8fec49

Some more redundancy reduction

6e1ce47

SimonRastikian commented Jun 16, 2026

View reviewed changes

kevindeforth reviewed Jun 16, 2026

View reviewed changes

SimonRastikian and others added 5 commits June 16, 2026 20:39

Merge branch 'main' into 3499-correlate-governancethreshold-with-reco…

5056109

…nstructionthreshold

Merge branch '3499-correlate-governancethreshold-with-reconstructiont…

fbf22be

…hreshold' of github.com:near/mpc into 3499-correlate-governancethreshold-with-reconstructionthreshold

delimiter fix

b310e84

Merge branch '3499-correlate-governancethreshold-with-reconstructiont…

ca8b199

…hreshold' of github.com:near/mpc into 3499-correlate-governancethreshold-with-reconstructionthreshold # Conflicts: # crates/contract/src/dto_mapping.rs

SimonRastikian and others added 10 commits June 19, 2026 15:44

Update crates/contract/src/lib.rs

6c2a728

Co-authored-by: kevindeforth <32777623+kevindeforth@users.noreply.github.com>

using expect

ceff551

expect

e72e801

Renaming properly the functions

c1d3625

Relaxed comment

7d46e93

Using more expect and deleting deadcode

187de2a

cargo fmt

c7b3efb

Add gen_running_state_with_params to pin participant count and govern…

37465bd

…ance threshold in tests

Revert new_threshold_2 to new_threshold now that the governance upper…

c9ab983

… cap is disabled

Route resharing tests through gen_running_state_with_params

105a449

SimonRastikian enabled auto-merge June 22, 2026 13:34

gilcu3 requested changes Jun 22, 2026

View reviewed changes

SimonRastikian added 5 commits June 22, 2026 17:16

Merge branch '3499-correlate-governancethreshold-with-reconstructiont…

01ffb27

…hreshold' of github.com:near/mpc into 3499-correlate-governancethreshold-with-reconstructionthreshold

Remove todo

afbe6b8

Reinserting test

d8f7e81

Making 3 functions 2

f455d91

Failing example

5e97933

gilcu3 previously approved these changes Jun 22, 2026

View reviewed changes

Comment thread crates/contract/src/state/running.rs Outdated

no need for the hack

0d6e0f6

SimonRastikian dismissed gilcu3’s stale review via 0d6e0f6 June 22, 2026 21:54

kevindeforth approved these changes Jun 23, 2026

View reviewed changes

gilcu3 approved these changes Jun 23, 2026

View reviewed changes

SimonRastikian added this pull request to the merge queue Jun 23, 2026

SimonRastikian removed this pull request from the merge queue due to a manual request Jun 23, 2026

SimonRastikian added this pull request to the merge queue Jun 23, 2026

SimonRastikian mentioned this pull request Jun 23, 2026

Fix Kevin's left over comments in https://github.com/near/mpc/pull/3578#pullrequestreview-4551235815 #3649

Open

Merged via the queue into main with commit 729c3a6 Jun 23, 2026
15 checks passed

SimonRastikian deleted the 3499-correlate-governancethreshold-with-reconstructionthreshold branch June 23, 2026 09:10

		/// Like [`gen_running_state`], but pins the participant count and
		/// GovernanceThreshold instead of randomizing them.

Uh oh!

Conversation

SimonRastikian commented Jun 15, 2026

Uh oh!

claude Bot commented Jun 15, 2026

Pull request overview

Reviewed changes

Findings

Uh oh!

SimonRastikian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevindeforth left a comment

Choose a reason for hiding this comment

Uh oh!

gilcu3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gilcu3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevindeforth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants