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

[zancas]

Your review of #2428 left thirteen threads unresolved when the PR merged. This PR is the collected answer. It changes the code where a comment still described it, and it accounts for every other thread below, so each can be resolved from here.

Changed in this PR

get_migration_heights returns the heights directly (thread). Agreed, and done. The helper wrapped an infallible call yet returned Result<Option<_>>, and all five callers unwrapped the None case with the same ok_or(WalletError::NoSyncData). The function now returns the heights directly and maps the None case to NoSyncData itself, which deleted the identical ok_or at all five call sites. The wrapper survives for one reason, which its doc now states: it pins the wallet's own min_confirmations and types the no-sync-data case once.

The confirmation-wait errors drop their Split names (thread). Agreed. The function is generic over migration transactions, yet its errors said "split". This PR renames the two shared variants to MigrationTransactionFailed and MigrationConfirmationTimeout; SplitDidNotConverge keeps its name, being genuinely split-only. The rename is breaking on the public MigrationError enum.

The duplicated comment is gone (thread). The doc on broadcast_due_parts_selected carried its two paragraphs twice, verbatim. Fixed.

The ironwood balance comment now answers your question (thread). You asked why compact block scanning would fail to find positions for ironwood. It does not fail: scanning finds ironwood positions, and pepper-sync carries ironwood located trees and a nullifier-to-position map. A note lacks a position only until a scan locates its block in the commitment tree, and the wallet records its own outputs at broadcast, before any scan, so those notes hold no position and cannot be witnessed. The comment in shielded_spendable_balance now states this reason instead of asserting the conclusion.

Resolved by work that landed since the review

The COIN constant (thread). Agreed: in this codebase "coin" names a transparent output, and the constant collided with that sense. The local constant is gone. params.rs now imports zcash_protocol::value::COIN, so the workspace defines no such name of its own.

The dust_floor field (thread). The field is renamed, though not to denom_floor. Your name has the virtue of saying what the field does: it floors the denomination ladder. We chose max_residual_value instead, because ZIP 318 names the quantity MAX_RESIDUAL_VALUE and the reference crate exports it under that name; a field that mirrors the specification reads without translation.

An error when sync data is missing (thread). Done as you suggested. WalletError::NoSyncData now exists, and every planning and broadcast path returns it when last_known_chain_height is None. This PR folds the same mapping into get_migration_heights, so the migration height reads are typed as well.

Visibility of migration_note_values (thread). The function is pub(crate) now.

Visibility of build_note_split_transaction (thread). The function is pub(crate) now.

Protection before NU6.3 activation (thread). Later work added the protection you asked about. The start path refuses with IronwoodEraTooYoung until the first window boundary after NU6.3 activation, and the scheduler floors every anchor at activation, so a pre-activation release fails once with a typed error instead of striking the build error repeatedly.

Checkpoint count versus bucket modulus (thread). The code landed where your third comment arrived. Raising the checkpoint retention would have guaranteed every boundary a witness, but, as you said, it would store extra data forever for the sake of a single migration. Instead, refresh_part_witnesses caches each part's witness while its boundary checkpoint is still retained, and a part whose boundary is pruned takes the MissedBoundary skip and is re-assigned.

Kept as it is

The SyncDomain re-export (thread). Removing the re-export would give SyncDomain a single canonical path and spare the reader one indirection. We kept it nonetheless: wallet/traits.rs and sync/spend.rs both import crate::SyncDomain through it, and because it is pub(crate) it adds nothing to the crate's public surface.

Tracked onward

The dust-floor economics (thread). Later work resolved most of this in your direction. The floor is no longer local policy: it is ZIP 318's MAX_RESIDUAL_VALUE (0.01 ZEC), imported from the reference crate. Notes at or below sweep_min, and any pool too small to fund the smallest denomination, now stay in the pre-Ironwood pool as residual — recorded in the plan and covered by the consent hash — rather than folding into fees. One fee-fold remains: when a pool funds at least one denomination, the sizing surplus below the smallest denomination becomes the final split transaction's fee. That surplus is bounded below 0.01 ZEC per migration, but your underlying point stands — the user should not pay value away silently — so #2573 tracks whether the surplus should become a residual V2 change note instead.

🤖 Generated with Claude Code

[Oscar-Pepper]

not merging until #2582 (review) is resolved