feat: adding resharing per-domain by SimonRastikian · Pull Request #3326 · near/mpc

SimonRastikian · 2026-05-22T12:08:38Z

SimonRastikian

Not sure if I need to add requirements for validate_incoming_proposal. In fact the requirements there feel to me very randomly chosen and without clear basis. I wouldn't know if something is overdone or something is missing. However, I am pretty sure there is good logic behind them but it's not clear to me what.
Would need someone's wisdom on that.

SimonRastikian · 2026-05-22T13:17:18Z

@@ -215,6 +215,7 @@ impl IntoContractType<Participants> for dtos::Participants {
 impl IntoContractType<ThresholdParameters> for dtos::ThresholdParameters {
    fn into_contract_type(self) -> ThresholdParameters {
        ThresholdParameters::new_unvalidated(self.participants.into_contract_type(), self.threshold)


I did not want to change that but question to the public: Why is this a new_unvalidated instead of the normal new (with validation)?

@gilcu3 do you have an idea?

I think to allow low threshold values in tests that would otherwise not pass in production (e.g. >= 60%)

gilcu3

Thank you!

I have two main problems with the current solution.

The first is that it tries to reuse ThresholdParameters, while I think it now makes sense to have one type for the running state:

pub struct ThresholdParameters {
    participants: Participants,
    threshold: Threshold,
}

and another for the voting:

pub struct ThresholdParametersVote {
    participants: Participants,
    threshold: Threshold,
    per_domain_thresholds: BTreeMap<DomainId, ReconstructionThreshold>, 
}

The second is that the way to achieve backward compatibility in this PR seems hacky, as it uses:

#[serde(default, skip_serializing_if = "BTreeMap::is_empty")]

It should be better to define Compat versions of the modified struct in the interface crate, as we did with the previous migrations.

I am also torn about the overlay concept. Most manual resharing operations will anyway change the thresholds per domain (as they add or remove participants). On the other hand, automatic resharing operations (in the future) should have no problem creating the correct parameters. Therefore, it seems strictly better, to have a simpler system, to require that the votes always contain a ReconstructionThreshold per domain.

gilcu3 · 2026-05-27T06:18:38Z

+    /// Clears the per-domain overlay. Called when a resharing proposal has
+    /// been applied to the [`super::domain::DomainRegistry`] — the proposal
+    /// map is no longer meaningful as part of the running state's parameters.
+    pub fn clear_per_domain_thresholds(&mut self) {
+        self.per_domain_thresholds.clear();
+    }


So this is needed because ThresholdParameters is used both for voting and for storing the current parameter set. Wouldn't it make sense to separate the types then?

I agree it would be cleaner to keep ThresholdParameters as is and add something like
ProposedThresholdParameters { parameters, per_domain_thresholds } used only by vote_new_parameters, the votes map, and KeyEvent.

This will require some more refactor I prefer to push into a newer PR.

+1 for separate structs for voting and storing parameters.

…esharing

anodar · 2026-05-29T14:04:24Z

+    /// Clears the per-domain overlay. Called when a resharing proposal has
+    /// been applied to the [`super::domain::DomainRegistry`] — the proposal
+    /// map is no longer meaningful as part of the running state's parameters.
+    pub fn clear_per_domain_thresholds(&mut self) {
+        self.per_domain_thresholds.clear();
+    }


+1 for separate structs for voting and storing parameters.

anodar · 2026-05-29T14:12:37Z

@@ -215,6 +215,7 @@ impl IntoContractType<Participants> for dtos::Participants {
 impl IntoContractType<ThresholdParameters> for dtos::ThresholdParameters {
    fn into_contract_type(self) -> ThresholdParameters {
        ThresholdParameters::new_unvalidated(self.participants.into_contract_type(), self.threshold)


I think to allow low threshold values in tests that would otherwise not pass in production (e.g. >= 60%)

SimonRastikian · 2026-06-01T13:19:40Z

@anodar Validation is not the job of the DTO mapping, the contract that accepts the parameter will call a certain validate and validate_incoming_proposal down the stream. I added a comment for it

…onstruction_thresholds

gilcu3 · 2026-06-02T15:42:22Z

@claude review

Pay attention to what in the changes is making the contract binary bigger, would be nice to keep the current limits but not a problem if we need to bump them a bit

claude · 2026-06-02T15:49:39Z

Pull request overview

Replaces the single ThresholdParameters used for both storage and voting with two distinct types — a stored ThresholdParameters and a new ProposedThresholdParameters carrying an optional per_domain_thresholds: BTreeMap<DomainId, ReconstructionThreshold> overlay. The overlay is validated at vote acceptance against the proposed participant count and the 3.11 CaitSith uniform-threshold invariant, then folded into the DomainRegistry via with_overlaid_thresholds at the final resharing transition. Adds v3_10_state migration shadows so 3.10 votes load as proposals with an empty overlay, and updates devnet/e2e/indexer call sites accordingly.

Changes:

New ProposedThresholdParameters type (interface + contract) replacing ThresholdParameters in vote_new_parameters, ThresholdParametersVotes, and KeyEvent consumers.
DomainRegistry::with_overlaid_thresholds + new UnknownDomainInProposal error.
RunningContractState::process_new_parameters_proposal now validates effective per-domain thresholds (overlay-applied) and re-runs the CaitSith uniform-threshold check; extracted validate_caitsith_uniform_threshold shared with vote_add_domains.
ResharingContractState gains per_domain_thresholds; vote_reshared applies the overlay to the registry on completion.
v3_10_state migration: OldThresholdParametersVotes / OldRunningContractState / OldProtocolContractState shadow the 3.10 borsh layout and lift old votes into empty-overlay proposals.
Devnet CLI gains --per-domain-threshold DOMAIN_ID:T repeated flag; e2e cluster + fake indexer updated.

Reviewed changes

Per-file summary

File	Description
`crates/contract/src/dto_mapping.rs`	DTO mappings for `ProposedThresholdParameters`; comment clarifying why `new_unvalidated` is used.
`crates/contract/src/errors.rs`	Adds `DomainError::UnknownDomainInProposal`.
`crates/contract/src/lib.rs`	`vote_new_parameters` accepts `ProposedThresholdParameters`; TEE-driven resharing wraps the parameters with an empty overlay.
`crates/contract/src/primitives/domain.rs`	New `validate_caitsith_uniform_threshold` + `DomainRegistry::with_overlaid_thresholds`.
`crates/contract/src/primitives/test_utils.rs`	Adds `gen_proposed_threshold_params`.
`crates/contract/src/primitives/threshold_votes.rs`	Votes now carry `ProposedThresholdParameters`.
`crates/contract/src/primitives/thresholds.rs`	New `ProposedThresholdParameters` struct.
`crates/contract/src/state.rs`	`ProtocolContractState::vote_new_parameters` signature updated.
`crates/contract/src/state/resharing.rs`	Stores `per_domain_thresholds`; applies overlay on transition; deterministic overlay test.
`crates/contract/src/state/running.rs`	New overlay validation in `process_new_parameters_proposal`; CaitSith lock factored out and reused.
`crates/contract/src/state/test_utils.rs`	`gen_valid_params_proposal` returns `ProposedThresholdParameters`.
`crates/contract/src/v3_10_state.rs`	Shadow types for the 3.10 borsh layout.
`crates/contract/tests/inprocess/attestation_submission.rs`	Updated to use new proposal type.
Snapshot files (`...borsh_schema...snap`, `abi__abi_has_not_changed.snap`)	Schema/ABI snapshots reflect the new type and added field.
`crates/devnet/src/cli.rs`, `crates/devnet/src/mpc.rs`	`--per-domain-threshold` CLI flag wired through.
`crates/e2e-tests/src/cluster.rs`	Updated to use new proposal type.
`crates/near-mpc-contract-interface/src/lib.rs`, `.../types/state.rs`	New `ProposedThresholdParameters` exported; `ResharingContractState` gains `per_domain_thresholds`.
`crates/node/src/indexer/fake.rs`	Constructs `ResharingContractState` with empty overlay.

Findings

Non-blocking (nits, follow-ups, suggestions):

crates/contract/src/state/running.rs:137-141 and :213-217 — Two u64::try_from(proposal.participants().len()).map_err(|e| ConversionError::DataConversion { reason: format!(...) })? for usize → u64. On every supported target including wasm32 (where usize == u32) this conversion is statically infallible, so both error branches are dead code that still bake a format! string and an Error variant into the contract wasm. The same file's vote_add_domains:184 and :216 use proposal.participants().len() as u64. Switching both new sites to as u64 (or computing participants_len once and reusing it) directly addresses @gilcu3's binary-size concern and keeps the function consistent with its neighbors. The engineering-standards "Don't panic / dead code paths" guidance also discourages plumbing error handling for impossible failures.
crates/contract/src/lib.rs:~1593-1599 (TEE-driven resharing) — The TEE path constructs ProposedThresholdParameters::new(threshold_parameters, BTreeMap::new()) and goes straight to transition_to_resharing_no_checks, skipping the new effective-threshold check in process_new_parameters_proposal. With an empty overlay this only matters if the new participant count drops below some domain's existing reconstruction_threshold. That gap is pre-existing (the previous code didn't validate it either), so not blocking on this PR — but this is the natural place to also run validate_domain_threshold over existing domains against the new participant count, since validate_caitsith_uniform_threshold / validate_domain_threshold are now reusable helpers. Worth a follow-up.
crates/contract/src/state/resharing.rs:148-150 — In vote_reshared, with_overlaid_thresholds(&self.per_domain_thresholds)? is called after self.reshared_keys.push(new_key). In practice the overlay was already validated at vote acceptance and the domain set cannot mutate during resharing, so this call cannot fail here; the ? is defensive. Contract semantics roll the state back on Err, so this is safe — just noting it as a "cannot fail unless an upstream invariant breaks" path.
crates/contract/src/v3_10_state.rs:194-204 — The migration test borsh-roundtrips a single ThresholdParameters and wraps it manually in ProposedThresholdParameters::new(decoded, BTreeMap::new()). It doesn't actually exercise the OldThresholdParametersVotes → ThresholdParametersVotes From impl on a serialized vote map. A test that builds an OldThresholdParametersVotes, borsh-encodes it, decodes via the shadow, and runs the conversion would be a stronger guard against future shape drift.

✅ Approved — subject to the binary-size tidy-up on the two try_from sites if you want to handle @gilcu3's concern in this PR.

…esharing

anodar

LGTM, please fix the test as part of fixing conflict with main.

anodar · 2026-06-03T12:47:23Z

+    use crate::primitives::test_utils::{NUM_PROTOCOLS, gen_participants};
+    use crate::primitives::thresholds::Threshold;
+
+    /// Borsh round-trip: write a `ThresholdParametersVotes` in the OLD layout


It's not really testing it though is it? Seems to serialize ThresholdParameters and deserialize it into ThresholdParameters again. I don't see OldThresholdParametersVotes used here.

gilcu3

Partial review, dropped quite a few comments.

The most concerning for me are the current backward compatibility approach, and the interpretation of ThresholdParamters as cryptographic (while I believe it is only about governance now).

As I had mentioned in slack, we need to cleanup the 3.10 migrations before this can land.

gilcu3 · 2026-06-03T13:13:47Z

@@ -165,6 +168,68 @@ impl ThresholdParameters {
    }
 }


Isnt this wrong? I thought this type was the governance threshold after this PR?

I also don't understand the text:

so this type can never hold a meaningless overlay

The comment should just mention what it is

gilcu3 · 2026-06-03T13:16:08Z

+    /// Builder-style helper: replace the per-domain reconstruction-threshold
+    /// overlay. Convenient for constructing proposals (notably in tests).
+    pub fn with_per_domain_thresholds(
+        mut self,
+        per_domain_thresholds: BTreeMap<DomainId, ReconstructionThreshold>,
+    ) -> Self {
+        self.per_domain_thresholds = per_domain_thresholds;
+        self
+    }


if it happens to be used only in tests, we should feature gate it to save contract space.

gilcu3 · 2026-06-03T14:06:56Z

          }
        },
+        "ProposedThresholdParameters": {
+          "description": "A proposed set of threshold parameters submitted to `vote_new_parameters`. Carries the new [`ThresholdParameters`] plus an optional per-domain `ReconstructionThreshold` overlay for the resharing it would trigger.",


What do you mean by "new [ThresholdParameters]" this is not accurate

gilcu3 · 2026-06-03T14:17:33Z

+#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize, BorshSerialize, BorshDeserialize)]
+#[cfg_attr(
+    all(feature = "abi", not(target_arch = "wasm32")),
+    derive(schemars::JsonSchema)
+)]
+pub struct ProposedThresholdParameters {
+    #[serde(flatten)]
+    pub parameters: ThresholdParameters,
+    #[serde(default)]
+    pub per_domain_thresholds: BTreeMap<DomainId, ReconstructionThreshold>,
+}


I don't see how we would remove #[serde(flatten)] after 3.12. Please make sure we have a migration path forward, using compat structs if needed

gilcu3 · 2026-06-03T14:19:06Z

+pub struct ProposedThresholdParameters {
+    #[serde(flatten)]
+    pub parameters: ThresholdParameters,
+    #[serde(default)]
+    pub per_domain_thresholds: BTreeMap<DomainId, ReconstructionThreshold>,
+}


Also, what's the reason for having this type duplicate? We are trying to avoid that to keep contract size low. I see that ThresholdParameters is also duplicate (not in this PR), but wondering if that's the only reason, because in that case we could solve it first

gilcu3 · 2026-06-03T14:19:41Z

Another PR needs to land so that these changes are made against the 3.11 state instead

gilcu3 · 2026-06-03T14:29:35Z

+/// Enforces the 3.11-transition lock: every CaitSith domain (ForeignTx
+/// included) must share a single `reconstruction_threshold` so the legacy
+/// unprefixed `DBCol::Triple` mirror (#3292) can't collide. Domains using
+/// other protocols are unconstrained. The first CaitSith domain encountered
+/// fixes the expected value; any later CaitSith domain with a different
+/// threshold is rejected. Remove once #3298 drops the mirror.
+pub fn validate_caitsith_uniform_threshold(domains: &[DomainConfig]) -> Result<(), Error> {


#3298 already landed, so this is a bit outdated, but I think we have an issue for it #3306

gilcu3 · 2026-06-03T15:16:02Z

+    #[expect(non_snake_case)]
+    mod per_domain_threshold_overlay {
+        use super::*;
+        use crate::state::key_event::tests::Environment;
+        use crate::state::test_utils::gen_running_state;
+        use near_mpc_contract_interface::types::ReconstructionThreshold;
+        use std::collections::BTreeMap;


why using a different mod for these tests?

Adding a new voting mechanism for resharing

83d682c

SimonRastikian self-assigned this May 22, 2026

SimonRastikian marked this pull request as draft May 22, 2026 12:13

SimonRastikian added 2 commits May 22, 2026 14:23

Useless compat struct

7feb1b1

Adding comment

a9bd4eb

SimonRastikian changed the title ~~Adding a new voting mechanism for resharing~~ feat: adding resharing per-domain May 22, 2026

Accepted snap

b48bbf1

SimonRastikian commented May 22, 2026

View reviewed changes

SimonRastikian marked this pull request as ready for review May 22, 2026 13:59

SimonRastikian added 2 commits May 22, 2026 16:22

Solving conflicts

2b84e86

abi changed

ba25723

gilcu3 mentioned this pull request May 26, 2026

Lock CaitSith threshold during 3.11 transition #3304

Closed

gilcu3 requested changes May 27, 2026

View reviewed changes

SimonRastikian added 11 commits May 27, 2026 10:58

Some small nits by Reynaldo

376beb3

Merge branch 'main' into 3169-per-domain-ReconstructionThreshold-in-r…

871f188

…esharing

No need for skip_serializing_if

7187ab3

Merge branch 'main' into 3169-per-domain-ReconstructionThreshold-in-r…

d3df8b5

…esharing

fixing the bug generated due to conflict solving

b93c5d2

Merge branch 'main' into 3169-per-domain-ReconstructionThreshold-in-r…

a1c8d56

…esharing

Merge branch 'main' into 3169-per-domain-ReconstructionThreshold-in-r…

b64f863

…esharing

cargo fmt

a724aed

abi test

220132f

Merge branch 'main' into 3169-per-domain-ReconstructionThreshold-in-r…

a37858b

…esharing

Merge branch 'main' into 3169-per-domain-ReconstructionThreshold-in-r…

188521b

…esharing

anodar reviewed May 29, 2026

View reviewed changes

SimonRastikian enabled auto-merge May 31, 2026 23:28

SimonRastikian added 2 commits June 1, 2026 14:13

Conversion Error

a025892

cargo fmt

274cb6a

comment

59a1fae

SimonRastikian added 5 commits June 1, 2026 15:34

reject proposals that would leave CaitSith domains with differing rec…

fca347b

…onstruction_thresholds

Less flaky tests

cdae919

cargo fmt

34ba83c

ProposedThresholdParameters type-split

af28d30

fixing clippy

8e8adc8

SimonRastikian added 3 commits June 3, 2026 10:28

Merge branch 'main' into 3169-per-domain-ReconstructionThreshold-in-r…

cd87025

…esharing

Merge branch 'main' into 3169-per-domain-ReconstructionThreshold-in-r…

fa1790a

…esharing

fixing syntax

caa00f1

anodar previously approved these changes Jun 3, 2026

View reviewed changes

gilcu3 reviewed Jun 3, 2026

View reviewed changes

SimonRastikian added 4 commits June 5, 2026 23:13

flatten overlaid-thresholds tests into tests module

5511ec9

gate with_per_domain_thresholds behind cfg(test)

e804ee5

fix vote test comment wording

df34102

test migration through the shadow vote type

76e5269

SimonRastikian dismissed anodar’s stale review via 76e5269 June 5, 2026 21:15

Conversation

SimonRastikian commented May 22, 2026

Uh oh!

SimonRastikian left a comment

Choose a reason for hiding this comment

Uh oh!

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!

gilcu3 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SimonRastikian commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilcu3 commented Jun 2, 2026

Uh oh!

claude Bot commented Jun 2, 2026

Pull request overview

Reviewed changes

Findings

Uh oh!

anodar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilcu3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

SimonRastikian commented Jun 1, 2026 •

edited

Loading