zcash_pool_migration: Let a wallet cap one run's prepared notes by dannywillems · Pull Request #2962 · zcash/librustzcash

dannywillems · 2026-08-12T11:47:49Z

Motivation

plan_migration and plan_migration_with hardcoded CanonicalOneTwoFive::recommended(...),
so the per-run note cap (MIGRATION_MAX_PREPARED_NOTES_PER_RUN, 50) could not be chosen by
the wallet. A wallet has real reasons to vary it: a hot-wallet account can afford a longer run,
while an account signing through an external device may want a shorter one so a run fits fewer
signing rounds. A balance beyond one run's capacity simply migrates over more runs.

The crate already documented this as a caller's choice without offering any way to make it.
MIGRATION_MAX_PREPARED_NOTES_PER_RUN is described as "a policy default of this crate, not a
ZIP 318 constant" — the ZIP fixes the denomination set and its bounds but says nothing about
how many crossings one run may prepare. This makes the API match that description.

What this does NOT do

The denomination scheme stays fixed. There is no DenominationStrategy injection: the trait
remains an internal seam, every crossing is still a canonical {1, 2, 5} * 10^k value, and
DENOM_CAP and MAX_RESIDUAL_VALUE stay pinned to their zcash_protocol::zip318 values and
appear in no new signature. Only the per-run note count, which ZIP 318 does not define, is the
caller's to choose. A doc comment on the re-export claiming those two bounds were also
caller-chosen was inaccurate and has been corrected.

New CanonicalOneTwoFive::with_max_notes exists so the ZIP 318 bounds are named in exactly
one place and no call site can vary them.

Also fixed here

estimate_migration_runs planned its preparations with plan_preparation (the default
portfolio) even when the caller had planned with a different one through plan_migration_with,
so the previewed run count could describe a different migration from the one that would be
planned. The new estimate_migration_runs_with takes both knobs in the same order as
plan_migration_with, and its docs say they must be passed to both.

Breaking changes

Acceptable at 0.1.0-rc.7; [Unreleased] already carries larger ones.

plan_migration_with takes max_notes: NonZeroUsize in second position.
denomination::plan_denominations takes the same.
MIGRATION_MAX_PREPARED_NOTES_PER_RUN is now a NonZeroUsize. A default that cannot be
passed where the parameter is expected is not much of a default; SigningRoundBudget::DEFAULT
is typed the same way. NonZeroUsize also makes a zero cap unrepresentable, which today
yields an empty split reported as NothingToMigrate/UnfundableSplit — describing the
wallet's state rather than the bad argument.

plan_migration, estimate_migration_runs, and CanonicalOneTwoFive::new are unchanged.

Testing

cargo fmt, cargo clippy --all-targets --all-features -- -D warnings, and
cargo test --release --all-features are green (280 lib + 32 integration tests). New
tests/max_notes.rs covers raising and lowering the cap, estimate/plan agreement under a
custom cap, and that the default path is unchanged. A proptest asserts the property that
matters most: for any cap and balance, the crossing count stays within the cap and every
crossing is still canonical, so the one knob cannot move the scheme or its bounds. A second
proptest pins the cap-invariance of balance_has_canonical_split's emptiness answer, which
the new doc claims.

🤖 Generated with Claude Code

dannywillems · 2026-08-12T12:37:25Z

    (
        prop::collection::vec(arb_zatoshis(), 0..8),
-        (0u64..1_000_000).prop_map(zat),
+        (0u64..1_000_000).prop_map(Zatoshis::const_from_u64),


Reviewer notes: there are probably other instances of redefined zat; we should handle that in a follow-up.

dannywillems · 2026-08-12T12:37:54Z

    Ok(())
 }

-/// Replay a strategy module's scenario table through a whole migration planned under `portfolio`


Reviewer notes: these are moved below.

dannywillems · 2026-08-12T12:38:03Z


 use core::fmt;

-#[cfg(test)]


Reviewer notes: these are moved and consolidated below.

Copilot

Pull request overview

This PR updates zcash_pool_migration’s planning and estimation APIs to let wallets choose the per-run prepared-note cap (without making the ZIP 318 denomination set or its bounds configurable), and fixes a mismatch where run estimates could be computed using different preparation strategies than the eventual plan.

Changes:

Add a caller-provided max_notes: NonZeroUsize to plan_migration_with / plan_denominations, and introduce estimate_migration_runs_with so estimation uses the same knobs (portfolio + cap) as planning.
Make MIGRATION_MAX_PREPARED_NOTES_PER_RUN a NonZeroUsize default and thread it through internal call sites.
Add integration/property tests covering cap behavior and plan/estimate agreement, and update changelog accordingly.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
zcash_pool_migration/tests/signing_rounds_e2e.rs	Adjust assertions for `MIGRATION_MAX_PREPARED_NOTES_PER_RUN: NonZeroUsize` via `.get()`.
zcash_pool_migration/tests/max_notes.rs	New tests validating cap effects, estimate/plan agreement under custom caps, and scheme invariants.
zcash_pool_migration/tests/engine_plan.rs	Update test helper to pass the default `NonZeroUsize` cap into `plan_denominations`.
zcash_pool_migration/src/testing/generators.rs	Simplify Zatoshis generation by using `Zatoshis::const_from_u64` directly.
zcash_pool_migration/src/preparation/layered_greedy.rs	Update test helper call path after moving helper under `crate::preparation::tests`.
zcash_pool_migration/src/preparation/first_fit_decreasing.rs	Same as above: update test helper call path.
zcash_pool_migration/src/preparation.rs	Relocate `assert_scenarios_under` under `#[cfg(test)] pub(crate) mod tests` and thread default cap into `plan_migration_with`.
zcash_pool_migration/src/engine.rs	Add `max_notes` to `plan_migration_with`, fix estimation to use `plan_preparation_with(portfolio, ...)`, and add `estimate_migration_runs_with`.
zcash_pool_migration/src/denomination/strategies.rs	Add `CanonicalOneTwoFive::with_max_notes` and make `recommended` delegate to it; add cap-invariance proptest.
zcash_pool_migration/src/denomination.rs	Make default cap a `NonZeroUsize`, add `max_notes` parameter to `plan_denominations` and `balance_has_canonical_split`, and update docs.
zcash_pool_migration/src/build/end_to_end.rs	Update end-to-end build test to pass the default cap into `plan_denominations`.
zcash_pool_migration/CHANGELOG.md	Document new APIs and breaking changes (typed cap + signature changes + estimation fix).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+/// Both are NORMATIVE: they are the bounds ZIP 318 fixes for the denomination set, so they are not
+/// caller-settable. A wallet chooses how many notes a run prepares (see
+/// [`MIGRATION_MAX_PREPARED_NOTES_PER_RUN`]), never which values may cross.


The per-run note count is a policy choice: it bounds a run's transaction and proving cost, and ZIP 318 says nothing about it. The denomination SCHEME is not: the `{1, 2, 5} * 10^k` set and its DENOM_CAP and MAX_RESIDUAL_VALUE bounds are normative, and the privacy argument rests on every wallet publishing values from that one set. So expose exactly the count and nothing else. `CanonicalOneTwoFive::with_max_notes` takes only the count; the two bounds stay inside, and `recommended` delegates to it. `MIGRATION_MAX_PREPARED_NOTES_PER_RUN` becomes a `NonZeroUsize`, since a run preparing no notes migrates nothing. `plan_denominations` and `plan_migration_with` take the cap, and `plan_migration` keeps its signature exactly, passing the default. There is no `DenominationStrategy` injection at the wallet boundary; the trait stays an internal seam. `estimate_migration_runs_with` mirrors `plan_migration_with`'s knobs in the same order, so an application can preview the runs it is about to plan. Adding it also fixes a pre-existing defect: the estimate always planned its runs' preparations with the default portfolio, so an application using `plan_migration_with` with another one was previewed a run count its own plans would not produce. The tests pin both halves. A proptest shows the emptiness of a canonical split is invariant across caps, which is the lemma `balance_has_canonical_split` rests on when it distinguishes "nothing to migrate" from "these notes cannot fund it". An integration test drives the wallet-facing API as an external crate sees it: raising the cap crosses more and leaves less, the estimate previews the run the plan builds, and, for any wallet and any cap, every published crossing is still a canonical denomination within the ZIP 318 bounds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Five review points, none of which change behaviour: - Drop the `residual` helper in `tests/max_notes.rs`. `MigrationPlan::residual` already computes it; the helper was a second copy of the same expression. It returns `Zatoshis`, which is `Ord` but has no `Display`, so one assertion message moves to `{:?}`. - Drop the local `zat` helpers in `tests/max_notes.rs` and `src/testing/generators.rs` in favour of `Zatoshis::const_from_u64`, which is public in `zcash_protocol` and has identical semantics. The copies in `denomination.rs` and `preparation.rs` stay: their `expect` messages record why the conversion is infallible there, which a bare assertion would discard. - Move `assert_scenarios_under` into `preparation`'s `mod tests`. AGENTS.md asks that each `#[cfg(...)]` predicate be written once and that the weakest correct predicate be preferred; inside an already-gated module the correct predicate is none, so this deletes five `#[cfg(test)]` attributes and adds none. - Fold a call-site argument comment in `tests/engine_plan.rs` into the block comment above the call. - Consolidate the imports of `tests/max_notes.rs` into one `use` per crate root, per AGENTS.md. Neither `cargo fmt` nor clippy can check this: the relevant rustfmt option is nightly-only, and rustfmt leaves `#[cfg]`-gated imports alone by design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>