refactor!: answer the #2428 review threads that were still live in code

zingolib/src/lightclient/error.rs

@@ -117,13 +117,13 @@ pub enum MigrationError {
     /// Note splitting kept producing new rounds past the round bound.
     #[error("Migration did not converge within {0} rounds.")]
     SplitDidNotConverge(usize),
-    /// A note-splitting transaction failed or disappeared from the wallet.
-    #[error("Note-splitting transaction {0} failed or disappeared.")]
-    SplitTransactionFailed(TxId),
-    /// Note-splitting transactions were not confirmed within the polling
-    /// window.
-    #[error("Timed out waiting for note-splitting transactions to confirm.")]
-    SplitConfirmationTimeout,
+    /// A migration transaction (a note split or a part) failed or
+    /// disappeared from the wallet.
+    #[error("Migration transaction {0} failed or disappeared.")]
+    MigrationTransactionFailed(TxId),
+    /// Migration transactions were not confirmed within the polling window.
+    #[error("Timed out waiting for migration transactions to confirm.")]
+    MigrationConfirmationTimeout,
     /// The scheduled flow was asked to start over a plan that still needs
     /// note splitting, which no scheduled-flow driver executes yet.
     #[error(

zingolib/src/lightclient/migrate.rs

@@ -584,8 +584,10 @@ impl LightClient {
         &self,
         account: zip32::AccountId,
     ) -> Result<MigrationPlan, LightClientError> {
-        let wallet = self.wallet().read().await;
-        Ok(wallet.plan_ironwood_migration_now(account)?)
+        self.wallet()
+            .read()
+            .await
+            .plan_ironwood_migration_now(account)
     }
 
     /// Records the user's consent to a proposed migration plan and persists
@@ -738,9 +740,7 @@ impl LightClient {
                     // The round's outputs enter planning once the anchor
                     // reaches their confirmation heights; replanning earlier
                     // would read a note set with the round half-applied.
-                    let (_, anchor_height) = wallet
-                        .get_migration_heights()?
-                        .ok_or(WalletError::NoSyncData)?;
+                    let (_, anchor_height) = wallet.get_migration_heights()?;
                     let unanchored = pending_txids.iter().any(|txid| {
                         wallet
                             .transaction_confirmed_height(txid)
@@ -970,14 +970,6 @@ impl LightClient {
         self.broadcast_due_parts_selected(client, None).await
     }
 
-    /// The due-part broadcast loop, optionally narrowed to a single part so
-    /// catch-up can sequence sends with spacing.
-    ///
-    /// Proving is parallelised across all due parts via
-    /// [`tokio::task::spawn_blocking`]: wallet reads happen under the write
-    /// lock (Phase A), all Halo2/Groth16 work runs concurrently on the
-    /// blocking thread pool without holding the lock (Phase B), and wallet
-    /// writes + submission happen sequentially under the lock again (Phase C).
     /// The due-part broadcast loop, optionally narrowed to a single part so
     /// catch-up can sequence sends with spacing.
     ///
@@ -1527,7 +1519,7 @@ impl LightClient {
                     MigrationPhase::Planned | MigrationPhase::NoteSplitting { .. } => {
                         let plan = crate::wallet::migration::plan_migration(
                             &wallet.live_v2_note_values(state.account),
-                            wallet.splits_confirm_post_activation(),
+                            wallet.splits_confirm_post_activation()?,
                             &state.params,
                         );
                         (plan.parts.len() as u32, plan.parts.iter().sum())
@@ -2188,14 +2180,14 @@ impl LightClient {
                 )
             };
             if let Some(txid) = failed {
-                return Err(MigrationError::SplitTransactionFailed(txid).into());
+                return Err(MigrationError::MigrationTransactionFailed(txid).into());
             }
             if all_confirmed {
                 return Ok(());
             }
             tokio::time::sleep(CONFIRMATION_POLL_INTERVAL).await;
         }
-        Err(MigrationError::SplitConfirmationTimeout.into())
+        Err(MigrationError::MigrationConfirmationTimeout.into())
     }
 }
 
@@ -2210,28 +2202,29 @@ impl crate::wallet::LightWallet {
     pub(crate) fn plan_ironwood_migration_now(
         &self,
         account: zip32::AccountId,
-    ) -> Result<MigrationPlan, crate::wallet::error::WalletError> {
+    ) -> Result<MigrationPlan, LightClientError> {
         let params = MigrationParams::provisional(self.chain_type());
         Ok(plan_migration(
             &self.migration_note_values(account)?,
-            self.splits_confirm_post_activation(),
+            self.splits_confirm_post_activation()?,
             &params,
         ))
     }
 
     /// Whether a transaction built now confirms at or after NU6.3
     /// activation. Note-splitting fees depend on it (the Orchard bundle's
     /// cross-address rules change the action count).
-    pub(crate) fn splits_confirm_post_activation(&self) -> bool {
+    pub(crate) fn splits_confirm_post_activation(&self) -> Result<bool, LightClientError> {
         match (
             self.sync_state.last_known_chain_height(),
             pepper_sync::wallet::PoolActivation::of(
                 &self.chain_type(),
                 zcash_protocol::ShieldedPool::Ironwood,
             ),
         ) {
-            (Some(chain_height), Some(activation)) => chain_height + 1 >= activation.height(),
-            _ => false,
+            (None, Some(_)) => Err(LightClientError::WalletError(WalletError::NoSyncData)),
+            (Some(chain_height), Some(activation)) => Ok(chain_height + 1 >= activation.height()),
+            _ => Ok(false),
         }
     }
 
@@ -3183,7 +3176,7 @@ mod tests {
         let params = MigrationParams::provisional(wallet.chain_type());
         let expected = crate::wallet::migration::plan_migration(
             &[600_000_000, 600_000_000],
-            wallet.splits_confirm_post_activation(),
+            wallet.splits_confirm_post_activation().unwrap(),
             &params,
         );
         assert!(

zingolib/src/wallet/balance.rs

@@ -584,8 +584,11 @@ impl LightWallet {
         account_id: zip32::AccountId,
         include_potentially_spent_notes: bool,
     ) -> Result<Zatoshis, BalanceError> {
-        // Zero while ironwood notes carry no positions, which is right
-        // because those notes are not witnessable.
+        // An ironwood note carries no position until a scan locates its
+        // block in the commitment tree; the wallet records its own outputs
+        // (migration parts included) at broadcast, before any scan. A
+        // positionless note is not witnessable, so counting it as
+        // unspendable here is right.
         let ironwood_balance = match self
             .spendable_balance::<IronwoodNote>(account_id, include_potentially_spent_notes)
         {

zingolib/src/wallet/migration/parts.rs

@@ -985,9 +985,7 @@ impl crate::wallet::LightWallet {
             ));
         }
 
-        let (_, anchor_height) = self
-            .get_migration_heights()?
-            .ok_or(WalletError::NoSyncData)?;
+        let (_, anchor_height) = self.get_migration_heights()?;
         let mut ready: Vec<(u64, BoundNote)> = Vec::new();
         for note in self
             .spendable_notes::<pepper_sync::wallet::OrchardNote>(

zingolib/src/wallet/migration/split.rs

@@ -494,9 +494,7 @@ impl crate::wallet::LightWallet {
     ) -> Result<Vec<u64>, crate::wallet::error::WalletError> {
         use pepper_sync::wallet::{NoteInterface as _, OutputInterface as _};
 
-        let (_, anchor_height) = self
-            .get_migration_heights()?
-            .ok_or(crate::wallet::error::WalletError::NoSyncData)?;
+        let (_, anchor_height) = self.get_migration_heights()?;
         Ok(self
             .spendable_notes::<pepper_sync::wallet::OrchardNote>(
                 anchor_height,
@@ -546,9 +544,7 @@ impl crate::wallet::LightWallet {
     ) -> Result<bool, crate::wallet::error::WalletError> {
         use pepper_sync::wallet::{KeyIdInterface as _, NoteInterface as _, OutputInterface as _};
 
-        let (_, anchor_height) = self
-            .get_migration_heights()?
-            .ok_or(crate::wallet::error::WalletError::NoSyncData)?;
+        let (_, anchor_height) = self.get_migration_heights()?;
         // A failed transaction carries no confirmed height
         Ok(self
             .wallet_transactions
@@ -617,20 +613,27 @@ impl crate::wallet::LightWallet {
         )
     }
 
+    /// The target and anchor heights every migration site plans and builds
+    /// against. A thin wrapper over `get_target_and_anchor_heights` that pins
+    /// the wallet's own `min_confirmations` and turns the no-sync-data case
+    /// into the typed [`WalletError::NoSyncData`], so no caller re-derives
+    /// either.
+    ///
+    /// [`WalletError::NoSyncData`]: crate::wallet::error::WalletError::NoSyncData
     pub(crate) fn get_migration_heights(
         &self,
     ) -> Result<
-        Option<(
+        (
             zcash_protocol::consensus::BlockHeight,
             zcash_protocol::consensus::BlockHeight,
-        )>,
+        ),
         crate::wallet::error::WalletError,
     > {
         use zcash_client_backend::data_api::WalletRead as _;
-        Ok(self
-            .get_target_and_anchor_heights(self.wallet_settings.min_confirmations)
+        self.get_target_and_anchor_heights(self.wallet_settings.min_confirmations)
             .expect("infallible")
-            .map(|(target, anchor)| (target.into(), anchor)))
+            .map(|(target, anchor)| (target.into(), anchor))
+            .ok_or(crate::wallet::error::WalletError::NoSyncData)
     }
 
     #[allow(clippy::result_large_err)]
@@ -646,9 +649,7 @@ impl crate::wallet::LightWallet {
 
         use crate::wallet::error::WalletError;
 
-        let (target_height, anchor_height) = self
-            .get_migration_heights()?
-            .ok_or(WalletError::NoSyncData)?;
+        let (target_height, anchor_height) = self.get_migration_heights()?;
 
         // Pick one spendable V2 note per planned input value (distinct notes
         // for repeated values), copying out what the builder needs so the