diff --git a/keep-mobile/src/lib.rs b/keep-mobile/src/lib.rs index dbf95d97..e551730a 100644 --- a/keep-mobile/src/lib.rs +++ b/keep-mobile/src/lib.rs @@ -592,6 +592,14 @@ impl SigningHooks for MobileSigningHooks { fn post_sign(&self, _session: &SessionInfo, _signature: &[u8; 64]) {} } +/// Identity of the single in-flight `frost_run_dkg`: its run id and the cancel +/// flag the coordinator polls. Registered in `KeepMobile::dkg_active` for the +/// run's lifetime so `frost_cancel_dkg` can target it by id. +struct DkgActiveRun { + id: u64, + cancel: Arc, +} + #[derive(uniffi::Object)] pub struct KeepMobile { pub(crate) node: Arc>>>, @@ -608,11 +616,18 @@ pub struct KeepMobile { /// stays available with the share (C1); a process restart mid-ceremony just /// re-runs `frost_dkg_begin`. dkg_subkeys: Arc>>>, - /// Cancel flag for the single in-flight `frost_run_dkg`; flipped by - /// `frost_cancel_dkg` and polled by the shared coordinator (§8/§9). - dkg_cancel: Arc, - /// Single-flight guard: `frost_run_dkg` shares `dkg_cancel`/`dkg_subkeys`, so - /// a second concurrent run would reset the first's cancel flag. Rejected while + /// Identity + cancel flag for the single in-flight `frost_run_dkg`. Each run + /// mints a fresh id (from `dkg_run_seq`) and its own cancel flag, registered + /// here for its lifetime and cleared on every exit path by `DkgInFlightGuard`. + /// `frost_cancel_dkg(run_id)` flips the flag only when `run_id` matches the + /// active run, so a cancel issued for a run that has since finished cannot + /// abort the next run (§8: no wiping a concurrent run). + dkg_active: Arc>>, + /// Monotonic allocator for `dkg_active` run ids; the run id also reaches the + /// UI via `DkgProgressUpdate::Started` so it can target its cancel. + dkg_run_seq: Arc, + /// Single-flight guard: `frost_run_dkg` shares `dkg_active`/`dkg_subkeys`, so + /// a second concurrent run would clobber the first's identity. Rejected while /// set; cleared on every exit path by `DkgInFlightGuard`. dkg_in_flight: Arc, pub(crate) runtime: tokio::runtime::Runtime, @@ -777,7 +792,8 @@ impl KeepMobile { storage, pending_requests: Arc::new(Mutex::new(Vec::new())), dkg_subkeys: Arc::new(std::sync::Mutex::new(HashMap::new())), - dkg_cancel: Arc::new(std::sync::atomic::AtomicBool::new(false)), + dkg_active: Arc::new(std::sync::Mutex::new(None)), + dkg_run_seq: Arc::new(std::sync::atomic::AtomicU64::new(0)), dkg_in_flight: Arc::new(std::sync::atomic::AtomicBool::new(false)), runtime, policy, @@ -1369,13 +1385,20 @@ impl KeepMobile { Ok(keys.public_key().to_hex()) } - /// Signal a cancel to an in-flight `frost_run_dkg`. The coordinator polls this - /// between relay fetches (§8/§9) and aborts the run promptly with a cancelled - /// error, tearing down the transport cleanly, instead of leaving a live - /// session for a `reset` to corrupt. - pub fn frost_cancel_dkg(&self) { - self.dkg_cancel - .store(true, std::sync::atomic::Ordering::Relaxed); + /// Signal a cancel to the in-flight `frost_run_dkg` identified by `run_id` + /// (the id delivered via `DkgProgressUpdate::Started`). The coordinator polls + /// the run's flag between relay fetches (§8/§9) and aborts promptly with a + /// cancelled error, tearing down the transport cleanly, instead of leaving a + /// live session for a `reset` to corrupt. A `run_id` that does not match the + /// active run is ignored, so a cancel issued for a run that has since finished + /// cannot abort a later run (§8: no wiping a concurrent run). + pub fn frost_cancel_dkg(&self, run_id: u64) { + let active = self.dkg_active.lock().unwrap_or_else(|p| p.into_inner()); + if let Some(run) = active.as_ref() { + if run.id == run_id { + run.cancel.store(true, std::sync::atomic::Ordering::Relaxed); + } + } } /// Run a full relay-driven DKG to create a new group on this device, then @@ -1398,13 +1421,19 @@ impl KeepMobile { timeout_secs: u64, progress: Arc, ) -> Result { - // Single-flight: `frost_run_dkg` shares `dkg_cancel`/`dkg_subkeys`, so a - // second concurrent call would reset the active run's cancel flag. Reject - // it, and clear the guard on every exit path via Drop. - struct DkgInFlightGuard(Arc); + // Single-flight: `frost_run_dkg` shares `dkg_active`/`dkg_subkeys`, so a + // second concurrent call would clobber the active run's identity. Reject + // it, and on every exit path deregister the active run before releasing + // the flag so a next run cannot observe a stale identity. + struct DkgInFlightGuard { + in_flight: Arc, + active: Arc>>, + } impl Drop for DkgInFlightGuard { fn drop(&mut self) { - self.0.store(false, std::sync::atomic::Ordering::Release); + *self.active.lock().unwrap_or_else(|p| p.into_inner()) = None; + self.in_flight + .store(false, std::sync::atomic::Ordering::Release); } } if self @@ -1421,7 +1450,10 @@ impl KeepMobile { msg: "a DKG run is already in progress".into(), }); } - let _in_flight = DkgInFlightGuard(self.dkg_in_flight.clone()); + let _in_flight = DkgInFlightGuard { + in_flight: self.dkg_in_flight.clone(), + active: self.dkg_active.clone(), + }; // Pre-flight the persistence constraints before the multi-round network // run so an invalid name or a full store fails fast, rather than after @@ -1480,9 +1512,19 @@ impl KeepMobile { None }; - // Fresh cancel flag for this run; `frost_cancel_dkg` flips it. - self.dkg_cancel - .store(false, std::sync::atomic::Ordering::Relaxed); + // Mint this run's identity and a fresh cancel flag, then register it so + // `frost_cancel_dkg(run_id)` targets only this run. The id reaches the UI + // via `Started` so it can issue a matching cancel. + let run_id = self + .dkg_run_seq + .fetch_add(1, std::sync::atomic::Ordering::Relaxed) + + 1; + let cancel = Arc::new(std::sync::atomic::AtomicBool::new(false)); + *self.dkg_active.lock().unwrap_or_else(|p| p.into_inner()) = Some(DkgActiveRun { + id: run_id, + cancel: cancel.clone(), + }); + progress.on_progress(DkgProgressUpdate::Started { run_id }); let passphrase = Zeroizing::new(passphrase); let timeout = Duration::from_secs(timeout_secs.max(30)); @@ -1494,7 +1536,7 @@ impl KeepMobile { &name, &passphrase, timeout, - &self.dkg_cancel, + &cancel, progress.clone(), )); @@ -5508,6 +5550,53 @@ mod dkg_pending_share_tests { ); } + // `frost_cancel_dkg` must target a run by id: a stale id for a run that has + // since finished must not flip a later run's cancel flag (§8, GH #969). + #[test] + fn cancel_targets_run_by_id() { + let storage = Arc::new(FailingShareStorage::default()); + let mobile = KeepMobile::new(storage as Arc).unwrap(); + + // Register run 1 the way `frost_run_dkg` does. + let cancel1 = Arc::new(std::sync::atomic::AtomicBool::new(false)); + *mobile.dkg_active.lock().unwrap() = Some(DkgActiveRun { + id: 1, + cancel: cancel1.clone(), + }); + + // A cancel for a different (already-finished) run is ignored. + mobile.frost_cancel_dkg(2); + assert!( + !cancel1.load(std::sync::atomic::Ordering::Relaxed), + "a mismatched run id must not cancel the active run" + ); + + // A cancel for the active run flips its flag. + mobile.frost_cancel_dkg(1); + assert!( + cancel1.load(std::sync::atomic::Ordering::Relaxed), + "the matching run id must cancel the active run" + ); + + // Simulate run 1 finishing (guard deregisters) and run 2 starting fresh. + let cancel2 = Arc::new(std::sync::atomic::AtomicBool::new(false)); + *mobile.dkg_active.lock().unwrap() = Some(DkgActiveRun { + id: 2, + cancel: cancel2.clone(), + }); + + // The stale cancel for run 1 must not touch run 2. + mobile.frost_cancel_dkg(1); + assert!( + !cancel2.load(std::sync::atomic::Ordering::Relaxed), + "a cancel for a finished run must not abort the next run" + ); + + // With no active run, a cancel is a harmless no-op. + *mobile.dkg_active.lock().unwrap() = None; + mobile.frost_cancel_dkg(2); + } + // A wrong passphrase must not clear the stash: the share stays recoverable. #[test] fn failed_recovery_retains_stash() { diff --git a/keep-mobile/src/types.rs b/keep-mobile/src/types.rs index f80bafcc..ba207e03 100644 --- a/keep-mobile/src/types.rs +++ b/keep-mobile/src/types.rs @@ -127,6 +127,10 @@ impl std::fmt::Debug for DkgConfig { /// not cross the FFI as exceptions during the run, they arrive here. #[derive(uniffi::Enum, Clone, Debug, PartialEq)] pub enum DkgProgressUpdate { + /// First event of a run, carrying the id that identifies it. Pass this id to + /// `frost_cancel_dkg` to cancel this specific run; a stale id targeting a run + /// that has since finished is ignored, so it cannot abort a later run. + Started { run_id: u64 }, /// Establishing the relay connection with the bootstrap identity. Connecting, /// Waiting on peers' round-1 packages. `received` counts distinct peers so