feat(mpc-contract): cancel_node_migration() function in contract

[metalurgical]

Implement cancel_node_migration() function in contract, which clears the ongoing migration record.
Add tests.
Update migration-service documentation.

Related to #3774

[kevindeforth]

Thanks for the contribution!
Please avoid LLM-generated code-comments.
The unit-test in the contract can be improved by using existing test-helpers.

[metalurgical]

Hmm, see refactors have merged ahead of this one, will update it in a short while.

[metalurgical]

@gilcu3 Updated

[gilcu3]

@claude review

[claude]

Pull request overview

Adds a cancel_node_migration() entry point to the MPC contract so an operator can withdraw a pending DestinationNodeInfo record after calling start_node_migration (e.g. wrong destination info, or a destination node that never came up). The method removes the caller's entry from node_migrations and returns NodeMigrationError::MigrationNotFound when there is nothing to remove. The change is plumbed through near-mpc-contract-interface and the e2e cluster helper, and docs/migration-service.md is corrected to state that migration records survive Running → Resharing/Initializing transitions.

Changes:

• New MpcContract::cancel_node_migration() (#[handle_result], caller-authenticated via assert_caller_is_signer), plus a unit test and an updated contract ABI snapshot.
• New CANCEL_NODE_MIGRATION method name and MpcContractHandle::cancel_node_migration() client wrapper (no attached deposit).
• New e2e test migration_service__cancel_node_migration_clears_ongoing_migration_info plus MpcCluster::cancel_node_migration() and a new port seed.
• docs/migration-service.md: reverses the (incorrect) claim that state transitions cancel ongoing migrations.

Reviewed changes

Per-file summary

File	Description
crates/contract/src/lib.rs	Adds cancel_node_migration() after start_node_migration, and test_cancel_node_migration in the tests module
crates/contract/tests/snapshots/abi__abi_has_not_changed.snap	ABI entry for the new call method
crates/near-mpc-contract-interface/src/method_names.rs	CANCEL_NODE_MIGRATION constant
crates/near-mpc-contract-interface/src/client.rs	MpcContractHandle::cancel_node_migration(), MAX_GAS, no deposit
crates/e2e-tests/src/cluster.rs	MpcCluster::cancel_node_migration(node_index) via the node's operator key
crates/e2e-tests/tests/common.rs	New CANCEL_NODE_MIGRATION_PORT_SEED = 27
crates/e2e-tests/tests/migration_service.rs	E2E test: start migration, cancel, assert migration_info cleared, assert second cancel fails
docs/migration-service.md	Corrects two statements about automatic clearing of OngoingNodeMigration on state transitions

Findings

Blocking (must fix before merge):

• crates/contract/src/lib.rs:2023 — cancel_node_migration is missing the node-management deposit gate. Every other operator-authenticated method in this group (register_backup_service, start_node_migration, update_participant_url) is #[payable] + require_deposit(MINIMUM_NODE_MANAGEMENT_DEPOSIT, &account_id). The rationale is spelled out at crates/contract/src/lib.rs:94-100: "A non-zero deposit forces the call to be signed by a full-access key: the node's own key is registered as a function-call access key, which cannot attach a deposit, so a leaked node key cannot invoke these methods." As written, a leaked node key cannot redirect a migration but can repeatedly cancel one — i.e. it can veto the operator's escape from the compromised node, which is precisely the recovery path this deposit convention protects. Suggested fix:

  #[payable]
  #[handle_result]
  pub fn cancel_node_migration(&mut self) -> Result<(), Error> {
      let account_id = Self::assert_caller_is_signer();
      log!("cancel_node_migration: signer={:?}", account_id);
      if self.node_migrations.remove_migration(&account_id).is_none() {
          return Err(errors::NodeMigrationError::MigrationNotFound.into());
      }
      require_deposit(MINIMUM_NODE_MANAGEMENT_DEPOSIT, &account_id);
      Ok(())
  }

  This also requires switching the client to FunctionCallArgs::new(CANCEL_NODE_MIGRATION, b"{}".to_vec(), MAX_GAS, NearToken::from_yoctonear(MINIMUM_NODE_MANAGEMENT_DEPOSIT_YOCTONEAR)) (crates/near-mpc-contract-interface/src/client.rs:244), adding the method to the list in the MINIMUM_NODE_MANAGEMENT_DEPOSIT doc comment (crates/contract/src/lib.rs:95), and a cancel_node_migration__should_reject_when_no_deposit_attached test mirroring crates/contract/src/lib.rs:3839. If the deposit is deliberately omitted, that decision needs to be stated in the doc comment, because the asymmetry with its three siblings is otherwise unexplained.

• crates/near-mpc-contract-interface/src/client.rs:244 — New handle method not added to the wire-format catalog. The catalog test doc at client.rs:395-397 states: "every method is called once and its wire format becomes a section of the snapshot. New handle methods add a call here." cancel_node_migration is absent from mpc_contract_handle__should_match_the_wire_format_catalog, and the snapshot is unchanged, so the new method's gas/deposit/args are unpinned and the test silently keeps passing. Add handle.cancel_node_migration().await.unwrap(); in declaration order (right after start_node_migration) and regenerate the snapshot. Worth noting: that snapshot is exactly what would have surfaced the finding above — deposit: 0 NEAR sitting next to its siblings' 1 yoctoNEAR.

• crates/contract/src/lib.rs:7057 — Test does not follow the mandated naming form. CLAUDE.md / docs/engineering-standards.md require <system_under_test>__should_<assertion>(); the most recently added neighbours (start_node_migration__should_reject_when_no_deposit_attached:3839, register_backup_service__should_reject_when_no_deposit_attached:3857) already follow it. Please split into cancel_node_migration__should_remove_the_pending_destination() and cancel_node_migration__should_reject_when_no_migration_exists() — the current test bundles a success path and an error path under one name. While there, drop // Cancel the migration (:7090) and // Check migration was cancelled (:7098): both paraphrase the line beneath them, which is the comment class @kevindeforth already asked to remove from this PR.

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

• crates/node/src/migration_service/onboarding.rs:156-162 — this PR invalidates the premise of retry_conclude_onboarding's doc comment: "we also wait for active_migration to flip false … (cleared when the contract removes our migration record on success)". With cancel_node_migration in place the record can now also be removed by a cancellation, so an onboarding destination node that is mid-retry will observe active_migration → false and return Ok(()) for a migration that was actually aborted (it then falls back to WaitForStateChange, since the contract still lists the old TLS key, so the system self-heals — but the comment and the success path both claim otherwise). Worth updating the comment, and ideally re-checking the contract's participant info before declaring onboarding concluded.
• crates/e2e-tests/tests/migration_service.rs:1025-1046 — this retry block re-polls exactly the condition start_migration_and_wait already polls internally (migration_service.rs:422-442) before returning Ok. It can be deleted.
• crates/e2e-tests/tests/migration_service.rs:1008 — c.migration_targets = vec![0, 1] spins up two destination nodes but the test only uses index 2; vec![0] is enough (cf. migration_service.rs:887). More broadly: this e2e test asserts only contract state, which the unit test already covers, so it pays for a full 4-node cluster without adding coverage. Consider asserting the node-side reaction (e.g. the source's /debug/migrations reports active_migration: false, or the migration web server stops serving) so the cluster cost buys something the unit test cannot.
• crates/contract/src/lib.rs:7088-7089 — Environment::new(None, Some(account_id.clone()), None) already applies signer and predecessor via Environment::set (crates/contract/src/tee/test_utils.rs:74-83), so the following test_env.set_signer(&account_id) is a no-op repeat.
• crates/contract/src/lib.rs:2021 — the shortened doc comment drops the genuinely non-obvious invariant: unlike start_node_migration/conclude_node_migration, this method is intentionally not gated on Running. One extra line stating that (and the MigrationNotFound error) is not paraphrase — it is the part a caller cannot read off the signature.
• docs/migration-service.md:324 — the new sentence is accurate about the record, but the paragraph it sits in still asserts that "protocol state changes must have priority over any ongoing migrations", which now reads as a contradiction. Suggest stating what actually happens: the migration cannot progress while the protocol is not Running (conclude_node_migration returns ProtocolStateNotRunning), the record persists, and the migration resumes once the protocol returns to Running.
• docs/migration-service.md:464 — "not automatically cleared" is too absolute: vote_reshared schedules cleanup_orphaned_node_migrations (crates/contract/src/lib.rs:905-913), which does drop migration records for accounts that are no longer participants. Worth a clause pointing at that.
• docs/node-migration-guide.md:329 — the operator guide walks through start_node_migration with a ready-to-paste near contract call-function snippet, and the stated purpose of this PR is operator recovery from a bad destination. A short "aborting a migration" subsection with the matching snippet (plus the deposit, if added) would make the feature discoverable to the people it is for.

⚠️ Issues found

[gilcu3]

@metalurgical could you check the blocking points by Claude above? Once those are fixed I am ready to approve

[kevindeforth]

oh yeah, claude might have pointed it out already, but this should be added to the mpc_contract_handle__should_match_the_wire_format_catalog test

[kevindeforth]

It wold be good to add a sentence that explains why this is ok:
a node concluding the migration sends its keyset to the contract, which includes the epoch id and all public keys. The contract compares that against the current epoch and keyset. The transaction will fail in case there is a discrepancy.