Skip to content

feat: adding reconstruction threshold in node - #3640

Merged
SimonRastikian merged 139 commits into
mainfrom
3164-adding-ReconstructionThreshold-in-node
Jul 21, 2026
Merged

feat: adding reconstruction threshold in node#3640
SimonRastikian merged 139 commits into
mainfrom
3164-adding-ReconstructionThreshold-in-node

Conversation

@SimonRastikian

@SimonRastikian SimonRastikian commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Closes #3164
Closes #1680 in 0b8b201

SimonRastikian and others added 30 commits June 15, 2026 23:12
…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: Mårten Blankfors <marten@blankfors.se>
gilcu3
gilcu3 previously approved these changes Jul 17, 2026

@gilcu3 gilcu3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Left only minor comments. Slightly concerned about some tests becoming flaky, but that's all.

Comment thread crates/node/src/providers.rs
Comment on lines +24 to +25
c.triples_to_buffer = 2;
c.presignatures_to_buffer = 2;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we usually put enough triples in the buffer for all the signatures in the test to try to avoid flakiness as much as possible.

Comment on lines +104 to +115
tokio::select! {
res = wait_metric_on_nodes(
&cluster,
&[0, 1, 2, 3, 4],
metrics::TIMEOUTS_INDEXED,
|v| v >= 1,
CLUSTER_WAIT_TIMEOUT,
) => res.unwrap_or_else(|_| panic!(
"{} did not reach 1 on the surviving nodes — Cait-Sith request was answered \
despite only 5 of its 6 required signers being alive",
metrics::TIMEOUTS_INDEXED
)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How long will this take? The timeout of a signature, 60 seconds?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's the default 200 blocks of yield_timeout_length_in_blocks

Comment thread crates/node/src/providers/ecdsa_common.rs Outdated

@kevindeforth kevindeforth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found a comment that wasn't addressed.

Comment thread crates/e2e-tests/tests/common.rs Outdated
Comment on lines +433 to +438
/// Sign with every scheme in `running`, asserting each request succeeds.
pub async fn sign_all_schemes(
cluster: &MpcCluster,
running: &RunningContractState,
rng: &mut impl rand::Rng,
) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimonRastikian I don't think this comment was addressed.

@kevindeforth kevindeforth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, looks good.
I still have one file to review, but please proceed by resolving conflicts with the base branch.

Comment thread crates/node/src/indexer/participants.rs Outdated
Comment on lines +162 to +166
debug_assert_eq!(
participants.len(),
threshold.value(),
"triple routing infers t from channel size (see run_triple_generation_follower)"
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, please remove it. We run tests in release mode in CI, so I doubt this will do anything.


pub const SUPPORTED_TRIPLE_GENERATION_BATCH_SIZE: usize = 64;

const TRIPLE_METRICS_REPORTING_INTERVAL: Duration = Duration::from_millis(500);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit aggressive for a pure monitoring function. We lock a mutex each time we call this.

I think every 5 seconds is sufficient here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread crates/node/src/providers/ecdsa_common.rs
Comment thread crates/node/src/providers/ecdsa_common.rs
kevindeforth
kevindeforth previously approved these changes Jul 21, 2026

@kevindeforth kevindeforth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with 80% confidence (in my own review - pretty sure there are some things that I wouldn't catch here).
Would have been helpful to break this into smaller PRs.

gilcu3
gilcu3 previously approved these changes Jul 21, 2026

@gilcu3 gilcu3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again for the hard work. If we have still some doubts after this big change, we can always add more test coverage before the next release if needed.

Comment thread crates/e2e-tests/tests/common.rs
@SimonRastikian
SimonRastikian dismissed stale reviews from gilcu3 and kevindeforth via 97a24ae July 21, 2026 12:32
@SimonRastikian
SimonRastikian enabled auto-merge July 21, 2026 12:37

@kevindeforth kevindeforth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@SimonRastikian
SimonRastikian added this pull request to the merge queue Jul 21, 2026
Merged via the queue into main with commit b9eed26 Jul 21, 2026
15 checks passed
@SimonRastikian
SimonRastikian deleted the 3164-adding-ReconstructionThreshold-in-node branch July 21, 2026 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node consumes per-domain ReconstructionThreshold from DomainConfig Simplify PresignatureStorage::new parameters

5 participants