Skip to content

feat: parallel resharing and running#438

Merged
33 commits merged intomainfrom
bookrock/keep-contract-as-is
Jun 2, 2025
Merged

feat: parallel resharing and running#438
33 commits merged intomainfrom
bookrock/keep-contract-as-is

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented May 25, 2025

No description provided.

@ghost ghost marked this pull request as ready for review May 28, 2025 12:51
Comment thread node/src/coordinator.rs Outdated
Comment thread node/src/coordinator.rs Outdated
Comment thread node/src/coordinator.rs
Comment thread node/src/coordinator.rs Outdated
Comment thread node/src/coordinator.rs Outdated
Comment thread node/src/coordinator.rs Outdated
Comment thread node/src/db.rs Outdated
Comment thread node/src/key_events.rs Outdated
Comment thread node/src/protocol.rs Outdated
Comment thread node/src/coordinator.rs
})
});

// TODO: https://github.com/Near-One/mpc/issues/441
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.

I didn't have time to look at everything yet, but if 441 is open, then what is the current mechanism to prevent including new participants in computations that require valid keyshares?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It is fine. #441 is just asking to add an explicit step that checks membership of running state before spawning a task for the running job. Currently the spawned future just returns immediately with https://github.com/Near-One/mpc/blob/d186fcf87de34fc8157d2efa56b3a9bfb1d346a0/node/src/coordinator.rs#L87 so no work is actually being done.

Copy link
Copy Markdown
Contributor

@kevindeforth kevindeforth May 30, 2025

Choose a reason for hiding this comment

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

My question was lacking precision, let me rephrase:
What prevents the leader of a computation (e.g. triple, signature or pre-signature generation) from asking a participant that was not in the previous epoch (and thus does not have a valid keyshare) to participate in the computation?

(C.f. https://github.com/Near-One/mpc/blob/2e6080462d31a1738a8c741920aaf73d0334c6bd/node/src/providers/eddsa/sign.rs#L29 )

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's a good point. I updated the signature of select_random_active_participants_including_me to also take a set of participants to consider for the sampling 59aa238#diff-69623eb195b5eca6a57555b34713b6aca6491efab4f9920106f952acf8db4cda

I've also updated the participants to be considered by the running jobs to only be intersection of nodes that are in both thre running and resharing state so we don't consider nodes we are connected to in resharing that don't have active key shares.

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.

I believe issue #441 can be deleted now

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.

please delete and also delete the comment

Copy link
Copy Markdown
Contributor

@kevindeforth kevindeforth left a comment

Choose a reason for hiding this comment

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

Thanks!

Could you please also update the pytests before merging?

Comment thread node/src/key_events.rs Outdated
Comment thread node/src/key_events.rs Outdated
Comment thread node/src/coordinator.rs Outdated
Comment thread node/src/coordinator.rs Outdated
Comment thread node/src/coordinator.rs
})
});

// TODO: https://github.com/Near-One/mpc/issues/441
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.

I believe issue #441 can be deleted now

Comment thread node/src/coordinator.rs Outdated
@ghost
Copy link
Copy Markdown
Author

ghost commented Jun 2, 2025

Thanks!

Could you please also update the pytests before merging?

Yes. Done now in a585241

@ghost ghost enabled auto-merge June 2, 2025 15:33
Copy link
Copy Markdown
Contributor

@kevindeforth kevindeforth left a comment

Choose a reason for hiding this comment

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

Thank you

Tests that signature requests are still processed while performing key resharing.

Test scenario:
1. Start with 2 nodes and 1 domain.
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.

by default init_cluster uses two domains, one for ecdsa and one for eddsa. Both should be tested here.

Comment thread node/src/coordinator.rs
})
});

// TODO: https://github.com/Near-One/mpc/issues/441
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.

please delete and also delete the comment

@ghost ghost added this pull request to the merge queue Jun 2, 2025
@kevindeforth kevindeforth removed this pull request from the merge queue due to a manual request Jun 2, 2025
@ghost ghost added this pull request to the merge queue Jun 2, 2025
Merged via the queue into main with commit 5ffda85 Jun 2, 2025
1 check passed
@ghost ghost deleted the bookrock/keep-contract-as-is branch June 2, 2025 17:54
This was referenced Jun 3, 2025
This pull request was closed.
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.

3 participants