diff --git a/crates/contract/src/lib.rs b/crates/contract/src/lib.rs index dba93afa9..77e870ba4 100644 --- a/crates/contract/src/lib.rs +++ b/crates/contract/src/lib.rs @@ -2640,6 +2640,32 @@ impl MpcContract { Ok(()) } + /// Cancels a previously started node migration for the calling account. + /// + /// Removes the caller's pending `DestinationNodeInfo` record. This is useful if the new node + /// is not functioning correctly or the wrong information was provided when calling + /// [`Self::start_node_migration`]. + /// + /// This function is callable regardless of whether the protocol is in a `Running` state or + /// whether the signer is a current participant. + /// + /// # Errors + /// - `NodeMigrationError::MigrationNotFound`: if no migration record exists for the caller + #[handle_result] + pub fn cancel_node_migration(&mut self) -> Result<(), Error> { + let account_id = Self::assert_caller_is_signer(); + + match self.node_migrations.remove_migration(&account_id) { + Some(destination_node_info) => log!( + "cancel_node_migration: signer={:?}, destination_node_info={:?}", + account_id, + destination_node_info + ), + None => return Err(errors::NodeMigrationError::MigrationNotFound.into()), + } + Ok(()) + } + /// Updates the calling participant's registered URL, keeping the TLS key and participant ID. /// /// Requires a deposit of at least [`MINIMUM_NODE_MANAGEMENT_DEPOSIT`] (excess is refunded), so @@ -8225,4 +8251,55 @@ mod tests { {WORST_CASE_ENTRY_COST_CEILING} at today's storage price" ); } + + #[test] + fn test_cancel_node_migration() { + let running_state = ProtocolContractState::Running(gen_running_state(NUM_DOMAINS)); + let mut contract = MpcContract::new_from_protocol_state(running_state); + let participants = { + let ProtocolContractState::Running(running) = &contract.protocol_state else { + panic!("expected running state"); + }; + running.parameters.participants().clone() + }; + let (account_id, _, _) = participants + .participants() + .first() + .expect("expected at least one participant") + .clone(); + + testing_env!( + VMContextBuilder::new() + .signer_account_id(account_id.clone()) + .predecessor_account_id(account_id.clone()) + .attached_deposit(NearToken::from_yoctonear(1)) + .build() + ); + let destination_node_info = gen_random_destination_info(); + contract + .start_node_migration(destination_node_info.clone()) + .expect("participant should be able to start node migration"); + assert_eq!( + migration_info(&contract, &account_id), + (account_id.clone(), None, Some(destination_node_info)) + ); + + let mut test_env = Environment::new(None, Some(account_id.clone()), None); + test_env.set_signer(&account_id); + // Cancel the migration + contract + .cancel_node_migration() + .expect("caller should be able to cancel their pending migration"); + assert_eq!( + migration_info(&contract, &account_id), + (account_id.clone(), None, None) + ); + // Check migration was cancelled + assert!(contract.migration_info().is_empty()); + let res = contract.cancel_node_migration(); + assert_matches!( + res.unwrap_err(), + Error::NodeMigrationError(NodeMigrationError::MigrationNotFound) + ); + } } diff --git a/crates/contract/tests/snapshots/abi__abi_has_not_changed.snap b/crates/contract/tests/snapshots/abi__abi_has_not_changed.snap index 0ec9c8ab7..906bd3c7c 100644 --- a/crates/contract/tests/snapshots/abi__abi_has_not_changed.snap +++ b/crates/contract/tests/snapshots/abi__abi_has_not_changed.snap @@ -407,6 +407,17 @@ expression: abi } } }, + { + "name": "cancel_node_migration", + "doc": " Cancels a previously started node migration for the calling account.\n\n Removes the caller's pending `DestinationNodeInfo` record. This is useful if the new node\n is not functioning correctly or the wrong information was provided when calling\n [`Self::start_node_migration`].\n\n This function is callable regardless of whether the protocol is in a `Running` state or\n whether the signer is a current participant.\n\n # Errors\n - `NodeMigrationError::MigrationNotFound`: if no migration record exists for the caller", + "kind": "call", + "result": { + "serialization_type": "json", + "type_schema": { + "type": "null" + } + } + }, { "name": "clean_foreign_chain_data", "doc": " Private endpoint to clean up foreign chain policy votes and node configurations\n for non-participants after resharing.", diff --git a/crates/e2e-tests/src/cluster.rs b/crates/e2e-tests/src/cluster.rs index 73a69db54..dc79c323f 100644 --- a/crates/e2e-tests/src/cluster.rs +++ b/crates/e2e-tests/src/cluster.rs @@ -1029,6 +1029,17 @@ impl MpcCluster { .await } + /// Cancel a previously started node migration for a specific node. + pub async fn cancel_node_migration( + &self, + node_index: usize, + ) -> anyhow::Result { + let client = self.operator_client_for(node_index)?; + self.contract + .call_from(&client, method_names::CANCEL_NODE_MIGRATION, json!({})) + .await + } + /// Update the registered URL of a specific node, called from that node's own operator account. pub async fn update_participant_url( &self, diff --git a/crates/e2e-tests/tests/common.rs b/crates/e2e-tests/tests/common.rs index 816d0dd0a..916761807 100644 --- a/crates/e2e-tests/tests/common.rs +++ b/crates/e2e-tests/tests/common.rs @@ -40,7 +40,7 @@ pub const SIGTERM_HANDLER_PORT_SEED: u16 = 22; pub const DISTINCT_RECONSTRUCTION_THRESHOLDS_PORT_SEED: u16 = 23; pub const UPDATE_PARTICIPANT_URL_PORT_SEED: u16 = 24; pub const AVAILABLE_FOREIGN_CHAINS_PORT_SEED: u16 = 25; - +pub const CANCEL_NODE_MIGRATION_PORT_SEED: u16 = 26; /// Start a cluster, wait for Running state and presignatures to buffer. /// /// Uses `configure` to override defaults (3 nodes, threshold 2, 3 domains). diff --git a/crates/e2e-tests/tests/migration_service.rs b/crates/e2e-tests/tests/migration_service.rs index 64092b8b9..3899ff927 100644 --- a/crates/e2e-tests/tests/migration_service.rs +++ b/crates/e2e-tests/tests/migration_service.rs @@ -802,3 +802,98 @@ async fn migration_service__should_handle_back_migration_a_to_b_to_a() { .await .expect("ckd request failed after back-migration"); } + +/// Test to ensure `cancel_node_migration` removes a pending migration for the +/// calling account. +#[tokio::test] +#[expect(non_snake_case)] +async fn migration_service__cancel_node_migration_clears_ongoing_migration_info() { + // Given: a cluster with 2 participants and 2 migration targets. + let (cluster, _running) = + common::must_setup_cluster(common::CANCEL_NODE_MIGRATION_PORT_SEED, |c| { + c.num_nodes = 2; + c.threshold = 2; + c.migration_targets = vec![0, 1]; + }) + .await; + let source_idx = 0; + let target_idx = 2; + let source_account_id = cluster.nodes[source_idx].account_id().to_string(); + assert_eq!( + cluster.nodes[target_idx].account_id().to_string(), + source_account_id, + "migration target must share the source account" + ); + + // When: the source registers a migration destination. + start_migration_and_wait(&cluster, source_idx, target_idx) + .await + .expect("start_migration_and_wait failed"); + + // Then: migration_info reports a pending destination for the source. + (|| async { + let info: serde_json::Value = cluster + .view_migration_info() + .await + .context("failed to view migration info")?; + let entry = info.get(&source_account_id); + anyhow::ensure!( + entry.is_some_and(|e| !e.get(1).unwrap_or(&serde_json::Value::Null).is_null()), + "contract has not indexed migration information yet" + ); + Ok(()) + }) + .retry( + ConstantBuilder::default() + .with_delay(common::POLL_INTERVAL) + .with_max_times( + (INDEXER_SYNC_TIMEOUT.as_millis() / common::POLL_INTERVAL.as_millis()) as usize, + ), + ) + .await + .expect("timed out waiting for migration info"); + + // When: the source cancels the migration. + let outcome = cluster + .cancel_node_migration(source_idx) + .await + .expect("failed to call cancel_node_migration"); + assert!( + outcome.is_success(), + "cancel_node_migration failed: {:?}", + outcome.failure_message() + ); + + // Then: migration_info no longer shows a destination for that account. + (|| async { + let info: serde_json::Value = cluster + .view_migration_info() + .await + .context("failed to view migration info")?; + let entry = info.get(&source_account_id); + anyhow::ensure!( + entry.is_none(), + "expected destination to be cleared after cancel, got {entry:?}" + ); + Ok(()) + }) + .retry( + ConstantBuilder::default() + .with_delay(common::POLL_INTERVAL) + .with_max_times( + (INDEXER_SYNC_TIMEOUT.as_millis() / common::POLL_INTERVAL.as_millis()) as usize, + ), + ) + .await + .expect("timed out waiting for contract to reflect cancelled migration"); + + // And: cancelling again fails — the record was already removed + let outcome = cluster + .cancel_node_migration(source_idx) + .await + .expect("failed to call cancel_node_migration a second time"); + assert!( + !outcome.is_success(), + "expected the second cancel_node_migration call to fail, but it succeeded" + ); +} diff --git a/crates/near-mpc-contract-interface/src/method_names.rs b/crates/near-mpc-contract-interface/src/method_names.rs index 9b292a756..d2de250d1 100644 --- a/crates/near-mpc-contract-interface/src/method_names.rs +++ b/crates/near-mpc-contract-interface/src/method_names.rs @@ -51,6 +51,7 @@ pub const UPDATE_PARTICIPANT_URL: &str = "update_participant_url"; pub const VERIFY_TEE: &str = "verify_tee"; pub const CONCLUDE_NODE_MIGRATION: &str = "conclude_node_migration"; pub const START_NODE_MIGRATION: &str = "start_node_migration"; +pub const CANCEL_NODE_MIGRATION: &str = "cancel_node_migration"; pub const REGISTER_BACKUP_SERVICE: &str = "register_backup_service"; pub const CLEANUP_ORPHANED_NODE_MIGRATIONS: &str = "cleanup_orphaned_node_migrations"; pub const CLEAN_TEE_STATUS: &str = "clean_tee_status"; diff --git a/docs/migration-service.md b/docs/migration-service.md index e7b26f069..9b67ff664 100644 --- a/docs/migration-service.md +++ b/docs/migration-service.md @@ -248,7 +248,7 @@ flowchart TD For security reasons and to avoid edge cases and race conditions, the MPC network allows migration of nodes only while the protocol is in a `Running` state (as opposed to `Resharing` or `Initializing`, which are the two other well-defined states). Note that starting a migration workflow does not require a signing quorum. Instead, each participant can migrate their node at their own discretion. However, to avoid making the migration process a DoS attack vector, protocol state changes must have priority over any ongoing migrations. -If the protocol state changes into a `Resharing` or `Initializing` state, any ongoing migration processes will simply be cancelled. +If the protocol state changes into a `Resharing` or `Initializing` state, the pending `OngoingNodeMigration` record itself is **not** cleared by the transition and remains unless the operator explicitly withdraws it with `cancel_node_migration` or starts a new migration which will replace it. ## Implementation Details @@ -358,7 +358,7 @@ The contract provides the following methods: - **`start_node_migration(destination_node_info: ParticipantInfo)`** - Initiates a node migration: - Called by the node operator - - Creates an `OngoingNodeMigration` record for the given `AccountId` + - Creates an `OngoingNodeMigration` record for the node operator's account. - Stores the destination node's `ParticipantInfo` (new TLS keys, etc.) - Can be called multiple times to update the destination node info (only the last value is retained) - Returns an error if the protocol is not in `Running` state @@ -366,7 +366,7 @@ The contract provides the following methods: - **`cancel_node_migration()`** - Cancels an ongoing node migration: - Called by the node operator - - Removes the `OngoingNodeMigration` record for the given `AccountId` + - Removes the `OngoingNodeMigration` record for the node operator's account. - Useful if the new node is not functioning correctly or wrong information was provided - **`conclude_node_migration(keyset: &Keyset)`** - Finalizes a node migration: @@ -388,7 +388,7 @@ The contract provides the following methods: #### Migration Related Behavior -- The `OngoingNodeMigration` records are automatically cleared when the protocol transitions from `Running` state to `Resharing` or `Initializing` state, effectively cancelling any in-progress migrations. +- The `OngoingNodeMigration` records are **not** automatically cleared when the protocol transitions from `Running` state to `Resharing` or `Initializing` state. - **Future Enhancement**: It may be desirable for the contract to verify that calls to `conclude_node_migration(keyset)` come from the actual onboarding node by checking the transaction signer's public key _(see [(#1086)](https://github.com/near/mpc/issues/1086))_. This would prevent ill-behaved decommissioned nodes from making spurious migration calls. This would require: - Comparing `env::signer_account_pk()` with the public key associated with the participant (note: this is different from the TLS key currently stored as [`signer_pk`](https://github.com/near/mpc/blob/b5a9d1b2eef4de47d19b66cb25b577da2b897560/crates/contract/src/tee/tee_state.rs#L32) in TEEState) - Including this public key in the TEE attestation