From b72e64a6babf18f29daf33f28ca4ea34412eb9d7 Mon Sep 17 00:00:00 2001 From: Barakeinav1 Date: Sun, 14 Jun 2026 14:49:29 +0000 Subject: [PATCH 1/3] fix(migration): make onboarding state machine re-entrant (#3406) The migration web server's `import_keyshares` channel was owned by the `onboard()` loop and dropped when the loop returned on `OnboardingJob::Done`. For any node that was already an active participant at process start, this happened immediately, leaving the web server with a dangling sender. Subsequent `PUT /set_keyshares` calls (e.g. a back-migration targeting that node) returned `500 keyshares receiver channel is closed`, forcing a process restart to recreate the channel. Make `onboard()` a long-lived state machine. `Done` becomes a resting state (same shape as `WaitForStateChange`) rather than a terminal one. The first time the loop reaches `Done`, it fires a oneshot so the caller can unblock startup -- preserving the pre-existing gating that the coordinator only starts once keyshares are present. After that, the loop keeps watching contract + migration state. A future `start_node_migration` naming this node as destination flips `OnboardingJob` to `Onboard(keyset)`, the loop re-enters `execute_onboarding`, and the existing protocol path handles both sub-cases without branching: - epoch unchanged: storage check passes, no PUT required, conclude tx sent - epoch advanced: backup-CLI PUT lands, import_backup runs, conclude tx sent Design: docs/design/migration-onboarding-reentry.md. Tests: - Unit: onboard__should_fire_first_done_signal_and_keep_looping_past_done asserts the loop fires first_done_signal once and keeps running. - E2E: migration_service__should_handle_back_migration_a_to_b_to_a_no_restart back-migration A -> B -> A with A0 staying alive between directions; fails on main, passes here. --- crates/e2e-tests/tests/common.rs | 1 + crates/e2e-tests/tests/migration_service.rs | 193 +++++++++--------- crates/node/src/migration_service.rs | 42 ++-- .../node/src/migration_service/onboarding.rs | 91 ++++++++- docs/design/migration-onboarding-reentry.md | 161 +++++++++++++++ 5 files changed, 367 insertions(+), 121 deletions(-) create mode 100644 docs/design/migration-onboarding-reentry.md diff --git a/crates/e2e-tests/tests/common.rs b/crates/e2e-tests/tests/common.rs index eb81da0c96..be5363c9cd 100644 --- a/crates/e2e-tests/tests/common.rs +++ b/crates/e2e-tests/tests/common.rs @@ -36,6 +36,7 @@ pub const CONTRACT_UPGRADE_COMPATIBILITY_TESTNET_PORT_SEED: u16 = 19; pub const TIMEOUT_METRIC_PORT_SEED: u16 = 20; pub const MIGRATION_BACK_PORT_SEED: u16 = 21; pub const SIGTERM_HANDLER_PORT_SEED: u16 = 22; +pub const MIGRATION_BACK_NO_RESTART_PORT_SEED: u16 = 23; /// Start a cluster, wait for Running state and presignatures to buffer. /// diff --git a/crates/e2e-tests/tests/migration_service.rs b/crates/e2e-tests/tests/migration_service.rs index 34253e4cb5..4ca84f9412 100644 --- a/crates/e2e-tests/tests/migration_service.rs +++ b/crates/e2e-tests/tests/migration_service.rs @@ -655,46 +655,33 @@ async fn migration_service__should_migrate_nodes_via_backup_cli() { } } -/// Back-migration (A → B → A) end-to-end via the backup CLI. Investigation -/// context for the production scenario lives at near/mpc#2121; this test -/// does NOT regress against that bug (see Caveat below). -/// -/// Starts a cluster with 2 participating nodes (A0, A1) + 1 migration -/// target (B0). A1 stays a participant throughout so threshold (2) holds -/// during both directions. Per direction: -/// 1. Register backup service -/// 2. GET keyshares from source node -/// 3. Initiate node migration -/// 4. PUT keyshares to target node -/// 5. Verify migration finalizes and sign + ckd requests succeed -/// Between the forward and back directions, A0's CVM is killed and -/// restarted so its keyshares stay on disk through the restart. -/// After the back direction, B0 is killed so the final sign + ckd -/// assertions prove A0 + A1 carry the workload alone. -/// -/// Caveat: attestation is mocked as `{"Mock": "Valid"}` throughout, so -/// this test cannot exercise the on-chain stale-attestation failure mode — -/// which is the leading suspect for #2121's production symptom. This is a -/// positive-baseline regression test for the back-migration code path, not -/// a reproducer for #2121. -#[tokio::test] -#[expect(non_snake_case)] -async fn migration_service__should_handle_back_migration_a_to_b_to_a() { +/// Action applied to A0 between the forward and back directions. +enum BetweenDirections { + /// A0 keeps running. + None, + /// A0 is killed and restarted from its on-disk home dir; asserts + /// keyshares survive. + KillAndRestartA, +} + +/// Shared scaffolding: cluster setup, forward A → B, `between` hook, +/// back B → A, kill B0, then asserts sign+ckd succeed with A0+A1 alone. +/// Attestation is mocked throughout. +async fn run_back_migration_round_trip(port_seed: u16, between: BetweenDirections) { let backup_cli = must_get_backup_cli_path(); - // Given: 2 participants (A0, A1) + 1 migration target (B0). + // 2 participants (A0, A1) + 1 migration target (B0). A1 stays a + // participant throughout so threshold (2) holds in both directions. let num_nodes = 2; - let (mut cluster, running) = - common::must_setup_cluster(common::MIGRATION_BACK_PORT_SEED, |c| { - c.num_nodes = num_nodes; - c.threshold = num_nodes; - c.migration_targets = vec![0]; - }) - .await; + let (mut cluster, running) = common::must_setup_cluster(port_seed, |c| { + c.num_nodes = num_nodes; + c.threshold = num_nodes; + c.migration_targets = vec![0]; + }) + .await; - // A0 (idx 0) is the source we migrate from; B0 lives at `num_nodes` - // since `migration_targets[0] = 0` puts the first target right after - // the initial participants. + // B0 lives at `num_nodes` because `migration_targets[0] = 0` places + // the first target right after the initial participants. let (a_idx, b_idx) = (0usize, num_nodes); assert_eq!( cluster.nodes[a_idx].account_id().to_string(), @@ -707,7 +694,6 @@ async fn migration_service__should_handle_back_migration_a_to_b_to_a() { "migration target must have a different p2p key" ); - // Sanity: cluster is healthy before any migration. let mut rng = rand::rngs::StdRng::seed_from_u64(42); common::send_sign_request(&cluster, &running, &mut rng, cluster.default_user_account()) .await @@ -716,11 +702,8 @@ async fn migration_service__should_handle_back_migration_a_to_b_to_a() { .await .expect("ckd request failed before forward migration"); - // When: forward migration A0 → B0. run_migration_round(&cluster, a_idx, b_idx, &backup_cli, "forward").await; - // Then: forward worked — B0 is the active participant, sign + ckd still - // succeed (B0 + A1). common::send_sign_request(&cluster, &running, &mut rng, cluster.default_user_account()) .await .expect("sign request failed after forward migration"); @@ -728,71 +711,61 @@ async fn migration_service__should_handle_back_migration_a_to_b_to_a() { .await .expect("ckd request failed after forward migration"); - // Simulate operator decommissioning the old node and bringing it back - // online later for the back-migration. `kill_nodes` + `start_nodes` - // preserves the node's home dir (keyshares stay on disk) — that disk - // state is the trigger for the suspected P1 early-return. - // - // Snapshot A0's keyshare-file listing before kill so we can verify - // it's still there after start. Keyshares live in - // `home_dir/permanent_keys/` (see `crates/node/src/keyshare/local.rs`). - let a0_keyshares_before = snapshot_permanent_keys(&cluster, a_idx); - assert!( - !a0_keyshares_before.is_empty(), - "A0's permanent_keys/ is empty before kill — test setup did not produce keyshares, \ - so the back-migration P1 precondition can't be modeled" - ); - // Snapshot A0's indexer height before the kill so the post-restart - // readiness check (below) can prove the indexer has resumed and - // advanced past where it was — not just bound the web port. - let a0_indexer_height_before_kill = common::current_node_indexer_height(&cluster, a_idx) - .await - .expect("failed to read A0's indexer height before kill") - .expect("A0's indexer should be reporting a block height before the kill"); - cluster - .kill_nodes(&[a_idx]) - .expect("failed to kill A0 before back-migration"); - cluster - .start_nodes(&[a_idx]) - .expect("failed to restart A0 for back-migration"); - cluster - .wait_for_node_healthy(a_idx) - .await - .expect("A0 did not become healthy after restart"); - // `wait_for_node_healthy` only checks the web port; wait for the - // indexer to actually resume before back-migration (see #3366). - common::wait_for_node_indexer_height_above( - &cluster, - a_idx, - a0_indexer_height_before_kill, - Duration::from_secs(60), - ) - .await - .expect("A0's indexer did not resume + advance within 60s after restart"); - // Validate the assumption the test rests on: A0's keyshares are - // still on disk after the restart. If they're missing or different, - // the test no longer models production's back-migration scenario - // (e.g. someone swapped `start_nodes` for `reset_and_start_nodes`, - // which wipes the home dir). - let a0_keyshares_after = snapshot_permanent_keys(&cluster, a_idx); - assert_eq!( - a0_keyshares_before, a0_keyshares_after, - "A0's permanent_keys/ contents changed across kill+start — \ - keyshares were NOT preserved on disk" - ); + match between { + BetweenDirections::None => {} + BetweenDirections::KillAndRestartA => { + // Keyshares live in `home_dir/permanent_keys/` + // (`crates/node/src/keyshare/local.rs`). Snapshot before/after + // the restart to verify they're preserved on disk. + let a0_keyshares_before = snapshot_permanent_keys(&cluster, a_idx); + assert!( + !a0_keyshares_before.is_empty(), + "A0's permanent_keys/ is empty before kill — test setup did not produce \ + keyshares, so the back-migration P1 precondition can't be modeled" + ); + let a0_indexer_height_before_kill = + common::current_node_indexer_height(&cluster, a_idx) + .await + .expect("failed to read A0's indexer height before kill") + .expect("A0's indexer should be reporting a block height before the kill"); + cluster + .kill_nodes(&[a_idx]) + .expect("failed to kill A0 before back-migration"); + cluster + .start_nodes(&[a_idx]) + .expect("failed to restart A0 for back-migration"); + cluster + .wait_for_node_healthy(a_idx) + .await + .expect("A0 did not become healthy after restart"); + // `wait_for_node_healthy` only checks the web port; wait for + // the indexer to actually resume before back-migration + // (see #3366). + common::wait_for_node_indexer_height_above( + &cluster, + a_idx, + a0_indexer_height_before_kill, + Duration::from_secs(60), + ) + .await + .expect("A0's indexer did not resume + advance within 60s after restart"); + let a0_keyshares_after = snapshot_permanent_keys(&cluster, a_idx); + assert_eq!( + a0_keyshares_before, a0_keyshares_after, + "A0's permanent_keys/ contents changed across kill+start — \ + keyshares were NOT preserved on disk" + ); + } + } - // When: back-migration B0 → A0 via the same helper, indices swapped. run_migration_round(&cluster, b_idx, a_idx, &backup_cli, "back").await; - // Kill B0 before the final assertions so the sign + ckd requests cannot - // be served by the now-demoted node — that proves A0 + A1 are carrying - // the workload alone, mirroring what the forward test does by killing - // the source after migration. + // Kill B0 so the final assertions can't be served by the demoted node + // — proves A0 + A1 carry the workload alone. cluster .kill_nodes(&[b_idx]) .expect("failed to kill B0 after back-migration"); - // Then: A0 + A1 are the participants again and sign + ckd still succeed. common::send_sign_request(&cluster, &running, &mut rng, cluster.default_user_account()) .await .expect("sign request failed after back-migration"); @@ -800,3 +773,27 @@ async fn migration_service__should_handle_back_migration_a_to_b_to_a() { .await .expect("ckd request failed after back-migration"); } + +/// Back-migration A → B → A with A0 staying alive between the two +/// directions (no kill+restart). +#[tokio::test] +#[expect(non_snake_case)] +async fn migration_service__should_handle_back_migration_a_to_b_to_a_no_restart() { + run_back_migration_round_trip( + common::MIGRATION_BACK_NO_RESTART_PORT_SEED, + BetweenDirections::None, + ) + .await; +} + +/// Back-migration A → B → A with A0 killed + restarted between the two +/// directions. +#[tokio::test] +#[expect(non_snake_case)] +async fn migration_service__should_handle_back_migration_a_to_b_to_a() { + run_back_migration_round_trip( + common::MIGRATION_BACK_PORT_SEED, + BetweenDirections::KillAndRestartA, + ) + .await; +} diff --git a/crates/node/src/migration_service.rs b/crates/node/src/migration_service.rs index 822c09055a..87e037fc73 100644 --- a/crates/node/src/migration_service.rs +++ b/crates/node/src/migration_service.rs @@ -3,7 +3,7 @@ use std::{net::SocketAddr, sync::Arc}; use ed25519_dalek::SigningKey; use near_account_id::AccountId; use onboarding::onboard; -use tokio::sync::{RwLock, watch}; +use tokio::sync::{RwLock, oneshot, watch}; use types::MigrationInfo; use crate::{ @@ -30,6 +30,11 @@ impl From<&SecretsConfig> for MigrationSecrets { } } +/// Spawns the migration web server and the onboarding state machine, +/// then awaits the first `OnboardingJob::Done` before returning. The +/// onboarding loop keeps running in the background past that point so +/// future migrations can re-enter `Onboard(keyset)` without a restart. +/// See `docs/design/migration-onboarding-reentry.md`. pub async fn spawn_recovery_server_and_run_onboarding( migration_web_ui: SocketAddr, migration_secrets: MigrationSecrets, @@ -37,7 +42,7 @@ pub async fn spawn_recovery_server_and_run_onboarding( keyshare_storage: Arc>, my_migration_info_receiver: watch::Receiver, contract_state_receiver: watch::Receiver, - tx_sender: impl TransactionSender, + tx_sender: impl TransactionSender + 'static, ) -> anyhow::Result<()> { let (import_keyshares_sender, import_keyshares_receiver) = tokio::sync::watch::channel(vec![]); let web_server_state = web::types::WebServerState { @@ -53,15 +58,28 @@ pub async fn spawn_recovery_server_and_run_onboarding( &migration_secrets.p2p_private_key, ) .await?; - onboard( - contract_state_receiver, - my_migration_info_receiver.clone(), - my_near_account_id.clone(), - migration_secrets.p2p_private_key.verifying_key(), - tx_sender, - keyshare_storage.clone(), - import_keyshares_receiver, - ) - .await?; + + let (first_done_tx, first_done_rx) = oneshot::channel(); + let tls_public_key = migration_secrets.p2p_private_key.verifying_key(); + tokio::spawn(async move { + if let Err(err) = onboard( + contract_state_receiver, + my_migration_info_receiver, + my_near_account_id, + tls_public_key, + tx_sender, + keyshare_storage, + import_keyshares_receiver, + Some(first_done_tx), + ) + .await + { + tracing::error!(?err, "onboarding state machine exited unexpectedly"); + } + }); + + // Err here = the spawned task dropped the sender; the panic/error is + // already logged above. Either way, startup proceeds. + let _ = first_done_rx.await; Ok(()) } diff --git a/crates/node/src/migration_service/onboarding.rs b/crates/node/src/migration_service/onboarding.rs index 31dfa37699..3110745dca 100644 --- a/crates/node/src/migration_service/onboarding.rs +++ b/crates/node/src/migration_service/onboarding.rs @@ -6,7 +6,7 @@ use ed25519_dalek::VerifyingKey; use futures::TryFutureExt; use near_account_id::AccountId; use near_mpc_crypto_types::Keyset; -use tokio::sync::{RwLock, watch}; +use tokio::sync::{RwLock, oneshot, watch}; use tokio_util::sync::CancellationToken; use crate::{ @@ -19,12 +19,12 @@ use crate::{ migration_service::types::{MigrationInfo, OnboardingJob, OnboardingTask}, }; -/// Waits until the node becomes an active participant in the current epoch or -/// terminates if the keyshare channel closes. -/// Internally, this function monitors contract and migration state changes and -/// runs onboarding tasks as needed. -/// -/// Returns `Ok(())` when this node is an active participant in the current epoch. +/// Runs the onboarding state machine for the lifetime of the process. +/// `Done` is a resting state — the loop keeps watching so a later +/// contract transition can re-enter `Onboard(keyset)` without a restart. +/// `first_done_signal` fires once, on first `Done`, so the caller can +/// unblock startup. See `docs/design/migration-onboarding-reentry.md`. +#[expect(clippy::too_many_arguments)] pub(crate) async fn onboard( contract_state_receiver: watch::Receiver, my_migration_info_receiver: watch::Receiver, @@ -33,6 +33,7 @@ pub(crate) async fn onboard( tx_sender: impl TransactionSender, keyshare_storage: Arc>, keyshare_receiver: watch::Receiver>, + mut first_done_signal: Option>, ) -> anyhow::Result<()> { tracing::info!(?my_near_account_id, "starting onboarding"); let (cancel_monitoring_task, mut onboarding_job_receiver) = start_onboarding_monitoring_task( @@ -51,8 +52,11 @@ pub(crate) async fn onboard( match job { OnboardingJob::Done => { tracing::info!(?my_near_account_id, "done onboarding"); - cancel_monitoring_task.cancel(); - return Ok(()); + if let Some(tx) = first_done_signal.take() { + let _ = tx.send(()); + } + cancellation_token.cancelled().await; + continue; } OnboardingJob::WaitForStateChange => { tracing::info!(?my_near_account_id, "waiting for state change"); @@ -336,7 +340,7 @@ mod tests { use rand::SeedableRng as _; use tokio::{ - sync::{RwLock, watch}, + sync::{RwLock, mpsc, oneshot, watch}, time::timeout, }; use tokio_util::sync::CancellationToken; @@ -358,10 +362,11 @@ mod tests { }, }, }, + tests::common::MockTransactionSender, }; use tracing_test::{self, traced_test}; - use super::start_onboarding_monitoring_task; + use super::{onboard, start_onboarding_monitoring_task}; const EPOCH_ID: u64 = 3; const NUM_KEYS: u64 = 5; @@ -630,4 +635,68 @@ mod tests { ) .await; } + + /// `onboard()` no longer returns on `Done`: it fires `first_done_signal` + /// (once) and keeps looping so a subsequent contract transition can + /// re-enter `Onboard(keyset)` without a process restart. See #3406 and + /// `docs/design/migration-onboarding-reentry.md`. + #[tokio::test] + #[expect(non_snake_case)] + async fn onboard__should_fire_first_done_signal_and_keep_looping_past_done() { + // Given: a node that is already an Active participant in the contract, + // so the very first OnboardingJob the loop reads is `Done`. + let onboarding_node = gen_participant(); + let (running_case, _keyset) = make_running_contract_case(onboarding_node.p2p_public_key); + let participant_node = running_case.participant_node.clone(); + let mut contract = running_case.contract.clone(); + // Make the loop's account_id + p2p_public_key match a current + // participant (Active(Active) → Done). + contract.change_participant_pk( + &participant_node.account_id, + participant_node.p2p_public_key, + ); + + let (contract_state_sender, contract_state_receiver) = watch::channel(contract); + let (_my_migration_info_sender, my_migration_info_receiver) = + watch::channel(INACTIVE_MIGRATION); + let (_keyshare_sender, keyshare_receiver) = watch::channel(vec![]); + let (tx_sender, _tx_receiver) = mpsc::channel(10); + let tx_sender = MockTransactionSender { + transaction_sender: tx_sender, + }; + let (first_done_tx, first_done_rx) = oneshot::channel(); + let (config, _temp_dir) = generate_key_storage_config(); + let keyshare_storage = Arc::new(RwLock::new(config.create().await.unwrap())); + + // When: spawn onboard() in the background. + let onboard_handle = tokio::spawn(onboard( + contract_state_receiver, + my_migration_info_receiver, + participant_node.account_id.clone(), + participant_node.p2p_public_key, + tx_sender, + keyshare_storage, + keyshare_receiver, + Some(first_done_tx), + )); + + // Then: first_done_signal fires within a short window (Done was reached). + timeout(Duration::from_secs(5), first_done_rx) + .await + .expect("first_done_signal should fire within 5s") + .expect("first_done_signal should not be dropped before firing"); + + // And: the loop is still running — Done is a resting state, not a terminal one. + // Give it some time to potentially exit before asserting still-running. + tokio::time::sleep(Duration::from_millis(200)).await; + assert!( + !onboard_handle.is_finished(), + "onboard() should keep looping past first Done; it must not return on Done" + ); + + // Keep `contract_state_sender` alive until after the assertion so the + // loop's receiver channel doesn't close (which would make it exit). + drop(contract_state_sender); + onboard_handle.abort(); + } } diff --git a/docs/design/migration-onboarding-reentry.md b/docs/design/migration-onboarding-reentry.md new file mode 100644 index 0000000000..76ae6efd53 --- /dev/null +++ b/docs/design/migration-onboarding-reentry.md @@ -0,0 +1,161 @@ +# Design: Re-entrant migration onboarding + +Issue: [#3406](https://github.com/near/mpc/issues/3406). + +## Motivation + +The migration system was designed with one-direction migration in mind +(a → b). Back-migration (a → b → a) — bringing the original node back as +the active participant after it was migrated away — is a useful +operational pattern that isn't a first-class scenario today: + +1. **Quick revert.** If something goes wrong with node B after a forward + migration (operator misconfig, perf regression, attestation issue), + the fastest recovery is to back-migrate to A instead of doing a full + fresh-node setup. +2. **Primary + state-synced standby.** Keep two instances alive — one + active, one on standby — and switch between them with a migration + round-trip when the active needs maintenance or replacement. + +## Today's restrictions + +Back-migration to a previously-active node works today, but only when +both of these hold: + +1. **Node A must be restarted** between the forward and back directions. + Without a restart, A0's `PUT /set_keyshares` is rejected with + `500 keyshares receiver channel is closed`. +2. **Node A must have valid attestation on the contract** for its key + when `conclude_node_migration` is sent. Tracked separately in + [#3362](https://github.com/near/mpc/pull/3362) (re-attest before + concluding back-migration); see also [#2121](https://github.com/near/mpc/issues/2121). + +This PR addresses restriction (1). Restriction (2) is independent +work. + +## The restart restriction + +`onboard()` (`migration_service/onboarding.rs`) owns the keyshare-receiver +end of the channel that the migration web server writes incoming PUTs to. +When that loop reaches `OnboardingJob::Done` it returns, dropping the +receiver. For nodes that start up already-active, this happens immediately +at process start. The web server keeps running with a now-dangling sender; +any subsequent `PUT /set_keyshares` returns +`500 keyshares receiver channel is closed` +(`migration_service/web/server.rs:185-191`). + +Restarting the process is the only way to recreate the channel — which is +the constraint a back-migration to a previously-active node hits today. + +## Fix + +Make `onboard()` a long-lived state machine instead of a one-shot: + +- Replace `return Ok(())` on `Done` with `cancellation_token.cancelled().await; continue;` + (same shape as `WaitForStateChange`). +- The first time the loop hits `Done`, fire a oneshot so the caller can + unblock startup. Subsequent `Done` transitions are silent. +- `spawn_recovery_server_and_run_onboarding`: `tokio::spawn(onboard(...))` + instead of `.await onboard(...)`. Await the oneshot before returning. + +Nothing else changes. The contract surface, the storage layer's epoch +checks (`keyshare.rs:376-449`, `keyshare/permanent.rs:155-196`), and the +operator workflow are unchanged. The monitoring task at +`onboarding.rs:100-153` already pushes `Onboard(keyset)` when contract +state names this node as a destination, so re-entry from `Done` → +`Onboard` happens automatically. Both sub-cases (epoch unchanged vs. +epoch advanced) fall out of the existing storage check inside +`execute_onboarding`. + +## Current behavior + +```mermaid +sequenceDiagram + participant Run as run.rs + participant Spawn as spawn_recovery_server_
and_run_onboarding + participant Ch as watch::channel
(Sender + Receiver) + participant Web as Web server task
(holds Sender) + participant Onb as onboard() loop
(holds Receiver) + participant Op as Operator + backup-CLI + + Note over Run,Op: PHASE 1 -- Node startup (A0 is already an active participant) + + Run->>Spawn: .await + Spawn->>Ch: create + Spawn->>Web: spawn (Sender moved in) + Note over Web: web server runs forever + Spawn->>Onb: .await onboard() (Receiver moved in) + Onb->>Onb: OnboardingJob::new(...) = Done + Onb-->>Spawn: return Ok(()) -- Receiver dropped (BUG) + Note over Ch: Sender now has no peer
(but is still held by Web) + Spawn-->>Run: Ok(()) + Run->>Run: start coordinator + + Note over Run,Op: ... forward migration A to B happens (A0 becomes non-participant) ... + + Note over Run,Op: PHASE 2 -- Operator initiates back-migration B to A + + Op->>Op: register_backup_service (already done) + Op->>Op: get-keyshares from B + Op->>Op: start_node_migration (destination = A0 key) + Op->>Web: PUT /set_keyshares (TLS handshake OK) + Web->>Ch: Sender.send(keyshares) + Ch-->>Web: Err -- no Receiver + Web-->>Op: 500 "keyshares receiver channel is closed" +``` + +## Proposed behavior + +```mermaid +sequenceDiagram + participant Run as run.rs + participant Spawn as spawn_recovery_server_
and_run_onboarding + participant Ch as watch::channel
(Sender + Receiver) + participant Web as Web server task
(holds Sender) + participant Onb as onboard() loop
(background task,
holds Receiver) + participant Op as Operator + backup-CLI + + Note over Run,Op: PHASE 1 -- Node startup (A0 is already an active participant) + + Run->>Spawn: .await + Spawn->>Ch: create + Spawn->>Web: spawn (Sender moved in) + Spawn->>Onb: tokio::spawn (Receiver + oneshot_tx moved in) + Onb->>Onb: OnboardingJob::new(...) = Done + Onb-->>Spawn: oneshot fires -- loop keeps running (FIX) + Spawn-->>Run: Ok(()) + Run->>Run: start coordinator + Note over Onb: keeps looping -- Receiver stays alive + + Note over Run,Op: ... forward migration A to B happens (A0 becomes non-participant) ... + + Note over Run,Op: PHASE 2 -- Operator initiates back-migration B to A + + Op->>Op: register_backup_service (already done) + Op->>Op: get-keyshares from B + Op->>Op: start_node_migration (destination = A0 key) + Note over Onb: monitoring task sees state change
=> OnboardingJob::Onboard(keyset) + Op->>Web: PUT /set_keyshares + Web->>Ch: Sender.send(keyshares) + Ch->>Onb: Receiver picks up + Onb->>Onb: import_backup (no-op if storage already has them) + Onb->>Onb: conclude_node_migration (sent by node automatically) + Note over Onb: => Done again, loop continues +``` + +## Risk + +In-memory state from A0's prior tenure as a participant could leak into +the re-entered `Onboard` cycle (P2P mesh in `network.rs`/`p2p.rs`, triple ++ presignature pools in `db.rs`). A0 has been `Inactive` for its own key +between forward and back migration, so the coordinator should have wound +those down — but this is the one non-trivial invariant introduced by the +change. Worth an explicit audit; tracking separately if anything is +found. + +## References + +- Issue: [#3406](https://github.com/near/mpc/issues/3406); related + operator-messaging: [#3407](https://github.com/near/mpc/issues/3407). +- Migration protocol: [`docs/migration-service.md`](../migration-service.md), + [`docs/node-migration-guide.md`](../node-migration-guide.md). From 9781816f4bb82dc085bacf3f5d357ea1ff3a9ded Mon Sep 17 00:00:00 2001 From: Barakeinav1 Date: Sun, 14 Jun 2026 15:13:24 +0000 Subject: [PATCH 2/3] fix(migration): apply Copilot review findings 1. spawn_recovery_server_and_run_onboarding now propagates errors from first_done_rx.await with context. Previously the error was discarded, so a panicking onboarding task could let startup return Ok(()) with no live listener on the keyshare channel. 2. Test cleanup in onboard__should_fire_first_done_signal... had a misleading comment claiming drop(contract_state_sender) made the loop exit. It doesn't -- abort() does. Drop the line and the comment; rename the unused binding to _contract_state_sender. --- crates/node/src/migration_service.rs | 9 ++++++--- crates/node/src/migration_service/onboarding.rs | 5 +---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/node/src/migration_service.rs b/crates/node/src/migration_service.rs index 87e037fc73..5378ebb1b0 100644 --- a/crates/node/src/migration_service.rs +++ b/crates/node/src/migration_service.rs @@ -1,5 +1,6 @@ use std::{net::SocketAddr, sync::Arc}; +use anyhow::Context as _; use ed25519_dalek::SigningKey; use near_account_id::AccountId; use onboarding::onboard; @@ -78,8 +79,10 @@ pub async fn spawn_recovery_server_and_run_onboarding( } }); - // Err here = the spawned task dropped the sender; the panic/error is - // already logged above. Either way, startup proceeds. - let _ = first_done_rx.await; + // Propagate so startup fails if onboarding dies before signaling — + // otherwise the keyshare channel has no listener. + first_done_rx + .await + .context("onboarding task died before reaching first Done")?; Ok(()) } diff --git a/crates/node/src/migration_service/onboarding.rs b/crates/node/src/migration_service/onboarding.rs index 3110745dca..e19e434946 100644 --- a/crates/node/src/migration_service/onboarding.rs +++ b/crates/node/src/migration_service/onboarding.rs @@ -656,7 +656,7 @@ mod tests { participant_node.p2p_public_key, ); - let (contract_state_sender, contract_state_receiver) = watch::channel(contract); + let (_contract_state_sender, contract_state_receiver) = watch::channel(contract); let (_my_migration_info_sender, my_migration_info_receiver) = watch::channel(INACTIVE_MIGRATION); let (_keyshare_sender, keyshare_receiver) = watch::channel(vec![]); @@ -694,9 +694,6 @@ mod tests { "onboard() should keep looping past first Done; it must not return on Done" ); - // Keep `contract_state_sender` alive until after the assertion so the - // loop's receiver channel doesn't close (which would make it exit). - drop(contract_state_sender); onboard_handle.abort(); } } From 2cd0db0af3a06ef4617d0438ad767dfba28b72ab Mon Sep 17 00:00:00 2001 From: Barakeinav1 Date: Sun, 14 Jun 2026 15:25:49 +0000 Subject: [PATCH 3/3] docs(design): link #3551 from the Risk section --- docs/design/migration-onboarding-reentry.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/design/migration-onboarding-reentry.md b/docs/design/migration-onboarding-reentry.md index 76ae6efd53..03e64ab7a3 100644 --- a/docs/design/migration-onboarding-reentry.md +++ b/docs/design/migration-onboarding-reentry.md @@ -150,8 +150,7 @@ the re-entered `Onboard` cycle (P2P mesh in `network.rs`/`p2p.rs`, triple + presignature pools in `db.rs`). A0 has been `Inactive` for its own key between forward and back migration, so the coordinator should have wound those down — but this is the one non-trivial invariant introduced by the -change. Worth an explicit audit; tracking separately if anything is -found. +change. Tracked in [#3551](https://github.com/near/mpc/issues/3551). ## References