Conversation
ebd8911 to
f2624ed
Compare
…ap` for improved lookup performance Fixes #1572
f2624ed to
7288d28
Compare
…Map-based Participants
…-setmap-based-data-structure-in-participants-for-improved-performance
…uplicate search (now it's O(log n))
…-setmap-based-data-structure-in-participants-for-improved-performance
…-setmap-based-data-structure-in-participants-for-improved-performance
…-setmap-based-data-structure-in-participants-for-improved-performance
…ec-with-setmap-based-data-structure-in-participants-for-improved-performance
…place-vec-with-setmap-based-data-structure-in-participants-for-improved-performance
|
@claude review |
Code ReviewI've analyzed this PR which migrates the internal Critical Issues1. Performance Regression: O(n) ID Uniqueness CheckLocation: The PR removed the Recommendation: Restore the 2. Missing Validation in
|
…-setmap-based-data-structure-in-participants-for-improved-performance
|
@claude review |
Code ReviewBased on my analysis of the PR, I've identified the following: Critical Issues Found:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…cture-in-participants-for-improved-performance
…-setmap-based-data-structure-in-participants-for-improved-performance
… update migration
…cture-in-participants-for-improved-performance
Code ReviewI've reviewed the full diff for this Vec-to-BTreeMap migration. The PR is well-structured overall — migration logic is comprehensive, gas improvements are meaningful, and test coverage has been updated. Previous review rounds have addressed the major issues (ParticipantId uniqueness validation in Issues1. Silent data loss in contract-interface deserialization (previously flagged, appears unresolved)
ParticipantsField::Vec(vec) => Ok(vec
.into_iter()
.map(|(account, id, info)| (account, ParticipantData { id, info }))
.collect()),
ParticipantsField::Vec(vec) => {
let expected_len = vec.len();
let map: BTreeMap<_, _> = vec
.into_iter()
.map(|(account, id, info)| (account, ParticipantData { id, info }))
.collect();
if map.len() \!= expected_len {
return Err(serde::de::Error::custom("duplicate AccountIds in legacy Vec format"));
}
Ok(map)
}2. Migration uses
impl From<OldThresholdParameters> for crate::primitives::thresholds::ThresholdParameters {
fn from(old: OldThresholdParameters) -> Self {
crate::primitives::thresholds::ThresholdParameters::new_unvalidated(
old.participants.into(),
old.threshold,
)
}
}While the data was previously valid on-chain, note that 3. ABI schema/wire-format change is a breaking change for external consumers The 4. The Minor
VerdictThe core logic is sound. Issue #1 (silent data loss in DTO deserialization) is the most actionable item — a two-line length check would add defense-in-depth. Issue #2 is informational. The breaking ABI change (#3) should be confirmed as intentional. |
gilcu3
left a comment
There was a problem hiding this comment.
This was a big PR, I had several comments, but none of them blocking IMO. There are also the comments above by claude. I think many of them, both mine and claude's are good to have.
| @@ -47,65 +48,128 @@ pub struct ParticipantInfo { | |||
| pub sign_pk: String, | |||
There was a problem hiding this comment.
I missed this in the PR the other day (sorry), but this should be a proper type that we already have defined in this crate Ed25519PublicKey
| /// Returns a subset of the participants according to the given range of indices. | ||
| pub fn subset(&self, range: std::ops::Range<usize>) -> Participants { |
There was a problem hiding this comment.
I did not know we had this function!
| //! | ||
| //! ## Changes since 3.4.1 | ||
| //! - `Participants` struct changed from serializing participants as | ||
| //! `Vec<(AccountId, ParticipantId, ParticipantInfo)>` to | ||
| //! `BTreeMap<AccountId, ParticipantData>` where `ParticipantData { id, info }`. |
There was a problem hiding this comment.
we have a few more changes to add, for example all the foreign tx stuff
| /// StaleData in v3.4.1 is empty (participant_attestations was cleaned up in 3.4.0 → 3.4.1). | ||
| #[derive(Debug, Default, BorshSerialize, BorshDeserialize)] | ||
| struct StaleData {} |
There was a problem hiding this comment.
why do we need this struct if that's the case?
| pub const CURRENT_CONTRACT_DEPLOY_DEPOSIT: NearToken = NearToken::from_millinear(14000); | ||
| pub const CURRENT_CONTRACT_DEPLOY_DEPOSIT: NearToken = NearToken::from_millinear(14201); |
There was a problem hiding this comment.
I general I would rather leave some room here, putting an exact values calls for future flakiness
| /// Submit mock attestations for all participants in parallel. | ||
| /// Submit mock attestations for the given accounts in parallel. | ||
| pub async fn submit_attestations( | ||
| contract: &Contract, | ||
| accounts: &[Account], | ||
| participants: &Participants, | ||
| ) { | ||
| let futures: Vec<_> = participants | ||
| .participants() | ||
| let futures: Vec<_> = accounts | ||
| .iter() | ||
| .zip(accounts) | ||
| .enumerate() | ||
| .map(|(i, ((_, _, participant), account))| async move { |
There was a problem hiding this comment.
this is worth fixing in a future PR, we could use the much simpler async transactions instead. See for example the function execute_async_transactions
| // Build new participants: all except mpc_signer_accounts[0] | ||
| let subset_dto = dtos::Participants { | ||
| next_id: initial_participants.next_id, | ||
| participants: initial_participants | ||
| .participants | ||
| .iter() | ||
| .filter(|(account_id, _)| account_id.0 != excluded_account) | ||
| .map(|(k, v)| (k.clone(), v.clone())) | ||
| .collect(), | ||
| }; | ||
| let new_participants: Participants = (&subset_dto).into_contract_type(); |
There was a problem hiding this comment.
why are we doing this double conversion here? Shouldn't we just construct the contract type?
| /// Wrapper around [`execute_key_generation_and_add_random_state_with_proposal`] | ||
| /// that serialises the threshold proposal in the legacy Vec-of-tuples format | ||
| /// expected by old contract binaries. | ||
| async fn add_random_state_to_old_contract( |
There was a problem hiding this comment.
more accurate, and seems to align better with existing code
| async fn add_random_state_to_old_contract( | |
| async fn execute_key_generation_and_add_random_state_to_old_contract( |
| if participants_with_valid_attestation.len() != participants.len() { | ||
| let participants_with_valid_attestation = | ||
| Participants::init(participants.next_id(), participants_with_valid_attestation); | ||
| if !invalid_accounts.is_empty() { | ||
| let mut participants_with_valid_attestation = participants.clone(); | ||
| for account in &invalid_accounts { | ||
| participants_with_valid_attestation.remove(account); | ||
| } |
There was a problem hiding this comment.
this change added the requirement of the "remove" function to the prod contruct that was previously test only. I think the previous code was slightly more efficient, wdyt?
There was a problem hiding this comment.
This PR currently does two things:
- it changes the contract-internal Participants representation
- it propagates those changes to the contract-interface crate.
It is unclear to me, why we need to add the new internal representation to the contract-interface crate. It makes reviewing this PR much harder.
To me, it looks like there is a breaking change in the abi.
I would prefer we split this up into two separate PR's, with the first one only doing the contract-internal changes. If we stay backwards-compatible, then this shouldn't require changes to any of our other crates.
Requesting changes for now - but we can discuss this on slack or over a call.
| #[serde(deserialize_with = "deserialize_participants")] | ||
| pub participants: BTreeMap<AccountId, ParticipantData>, | ||
| } | ||
|
|
||
| /// Custom deserializer for the `participants` field that accepts both the | ||
| /// current map format (`{account: {id, info}}`) and the legacy vec-of-tuples | ||
| /// format (`[[account, id, info], ...]`). | ||
| fn deserialize_participants<'de, D>( | ||
| deserializer: D, | ||
| ) -> Result<BTreeMap<AccountId, ParticipantData>, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| #[derive(Deserialize)] | ||
| #[serde(untagged)] | ||
| enum ParticipantsField { | ||
| Map(BTreeMap<AccountId, ParticipantData>), | ||
| Vec(Vec<(AccountId, ParticipantId, ParticipantInfo)>), | ||
| } | ||
|
|
||
| match ParticipantsField::deserialize(deserializer)? { | ||
| ParticipantsField::Map(map) => Ok(map), | ||
| ParticipantsField::Vec(vec) => Ok(vec | ||
| .into_iter() | ||
| .map(|(account, id, info)| (account, ParticipantData { id, info })) | ||
| .collect()), | ||
| } |
There was a problem hiding this comment.
Hmm, so two things:
- I don't think this PR requires any changes to the contract-interface. The only method that needs to be modified is the
into_dto_type(), but that lives in the contract crate. - If we realy want to add the new Participants type to the contract-interface crate, then I would opt to do that in a follow-up PR, not this one. It's just a lot of changes otherwise.
There was a problem hiding this comment.
Why does this file require changes if we stay backwards compatible?
IIRC, then the backup-service only depends on the contract-interface type and thus, would expect the same format as before?
| ], | ||
| "maxItems": 3, | ||
| "minItems": 3 | ||
| "type": "object", |
There was a problem hiding this comment.
isn't his a breaking change?
There was a problem hiding this comment.
Unfortunately we figured that the current PR introduces breaking changes wrt the 3.4.1 contract, checked with localnet automation:
mpc on 1572-replace-vec-with-setmap-based-data-structure-in-participants-for-improved-performance via 🦀 v1.86.0 took 59s
❯ ./scripts/launch-localnet.sh
Using mpc-contract binary from ./target/near/mpc_contract/mpc_contract.wasm
Creating network with 2 mpc nodes and threshold 2
Logs will be stored in /tmp/mpc-localnet.9QpDvx
Cleaning ~/.near folder
Started: neard PID: 261214
Waiting 60 seconds for neard to start properly
Creating mpc-contract account
Deploying mpc-contract
Creating mpc-node accounts
Creating mpc nodes configuration
Starting mpc nodes
Started: mpc-node-1.test.near PID: 266942
Started: mpc-node-2.test.near PID: 266944
Waiting 20 seconds for mpc nodes to start properly
Adding account keys for the nodes
Initializing contract
Adding domains to contract
Waiting 20 seconds for key generation to happen
Command near contract call-function as-read-only mpc-contract.test.near state json-args {} network-config mpc-localnet now 2>&1 | grep Running failed. Retrying in 2s...
Command near contract call-function as-read-only mpc-contract.test.near state json-args {} network-config mpc-localnet now 2>&1 | grep Running failed. Retrying in 2s...
Command near contract call-function as-read-only mpc-contract.test.near state json-args {} network-config mpc-localnet now 2>&1 | grep Running failed. Retrying in 2s...
Command near contract call-function as-read-only mpc-contract.test.near state json-args {} network-config mpc-localnet now 2>&1 | grep Running failed. Retrying in 2s...
For some reason the old contract does not transition to Running state
|
Thanks for all the reviews! I’m putting this on hold for now, since addressing all the concerns would take too much time and we need to prioritize the HOT Wallet migration work. |
Makes sense, marking it as draft so that the intention of work in progress is explicit |
Fixes #1572