Skip to content

zcash_pool_migration: Validate a plan reassembled from its parts - #2964

Draft
dannywillems wants to merge 3 commits into
mainfrom
dw/pool-migration-plan-from-parts
Draft

zcash_pool_migration: Validate a plan reassembled from its parts#2964
dannywillems wants to merge 3 commits into
mainfrom
dw/pool-migration-plan-from-parts

Conversation

@dannywillems

Copy link
Copy Markdown
Contributor

Stacked on #2962; review that first. The diff against main will show the note-cap change
too until #2962 merges.

Motivation

MigrationPlan's four fields are private and it has no constructor of any kind, so no
external crate can build one, even by composing this crate's own public pieces. That is
asymmetric with its own components: PreparationPlan::from_parts and
DenominationPlan::from_stored_parts are both public. It also blocks an out-of-tree
MigrationProver, since ProvedTransaction's only constructor is test-gated while
store_proved_transaction takes one by value.

Why this constructor validates when its siblings do not

Every other from_parts in the crate is unvalidated. This one is not, and the reason is
substantive rather than stylistic: a PreparationPlan's contents are re-checked downstream
by build_prep_tx, whereas a MigrationPlan's crossing values reach the chain unchecked.
An unvalidated public constructor would let a caller emit non-canonical crossings that join
no anonymity set, which is the failure ZIP 318 exists to prevent.

It is named from_parts, not from_stored_parts: a MigrationPlan is never persisted
(MigrationState is what a store round-trips, and it holds transactions rather than
schedules), so the stored qualifier that is accurate on DenominationPlan would be wrong
here.

What it checks

Twelve invariants that plan_migration establishes implicitly, each with its own error
variant carrying the offending indices. Several fail SILENTLY today rather than erroring,
which is what makes writing them down worthwhile:

  • A spurious trailing preparation layer shifts est_last_prep_height, moving the anchor
    floor for every transfer.
  • Non-monotone layer heights stamp a canonical expiry window for the wrong period.
  • A layer scheduled before its predecessor has mined builds a transaction guaranteed to
    fail on a missing input at broadcast.
  • A wrong total_migratable is the figure shown to the user for consent, and has no
    downstream backstop at all.

Claim resolvability is checked by running planned_run() rather than re-deriving its
first-fit rule, which makes CommitError::InconsistentPlan unreachable for a
from_parts-built plan. Canonicality reuses zip318::is_canonical_denomination.

Deliberately NOT checked, with a test pinning the non-check so it is not "fixed" later: the
transfer schedule is not monotone by crossing index. It is shuffled on purpose, because
crossings are non-increasing and pairing them with cumulative heights would spell out the
balance on chain.

Also not checkable here, and documented as such: wallet-note indices (resolved at commit as
CommitError::StalePlan), the full anchor floor (needs the bucket interval and the NU6.3
activation height, which a plan does not carry), and the per-run note count, which is
unverifiable BY CONSTRUCTION now that the cap is caller-settable in #2962.

Also here

  • Schedule::new(broadcast_height), required for the above to be usable at all. It DERIVES
    the expiry rather than accepting it: the expiry is a pure function of the broadcast
    height, and letting a caller stamp a per-transaction expiry would reintroduce the
    fingerprint the canonical rolling window removes.
  • ProvedTransaction::from_parts ungated, plus a with_lock_owner builder. A builder
    rather than a third argument because prove_transfer fills lock_owner from
    lock_spent_notes only after proving succeeds, so a required argument would force every
    prover to name a reservation even when it models no lock state.
  • MigrationPlan derives PartialEq/Eq so the round-trip test asserts plan equality
    directly rather than comparing four accessors.

Known wart, worth a reviewer's opinion

from_parts takes a PoolMigrationConstants for the canonicality bounds, but
Zip318Params is test-only, so there is no shipped implementor in this crate's
dependencies; real callers would use zcash_client_backend's PoolMigrationParams. And
SchedulingParams, which MigrationBackend::scheduling_params() already returns, does not
implement the trait. So a wallet holding a backend still needs a second source for bounds it
arguably already has. Implementing the trait for SchedulingParams, or sourcing the
constants from the backend, would both fix it; I did not want to pick one unilaterally.

Testing

cargo fmt, cargo clippy --all-targets --all-features -- -D warnings, and
cargo test --release --all-features are green: 330 tests, 0 failures.

A round-trip proptest feeds plan_migration output back through from_parts. Every error
variant has a rejection test built from a hand-made two-layer fixture, hand-made because a
planner cannot produce the invalid neighbours, which is the whole point of validating a
constructor that planners bypass. Canonicality is tested at both failure modes: off the
series, and on the series but below MAX_RESIDUAL_VALUE.

🤖 Generated with Claude Code

@dannywillems
dannywillems marked this pull request as draft August 12, 2026 12:39

@dannywillems dannywillems left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we want that, but the agents was already spawned. Keeping in draft in case we find this useful. Feel free to close. Not important now.

@dannywillems
dannywillems force-pushed the dw/pool-migration-plan-from-parts branch from 86289ea to fa4d02e Compare August 14, 2026 13:26
Base automatically changed from dw/pool-migration-note-cap to main August 17, 2026 12:17
dannywillems and others added 3 commits August 17, 2026 09:17
A `MigrationPlan` is never persisted: it is a transient value between
`plan_migration` and `commit_preparation`, and what a store round-trips is a
`MigrationState`, which holds transactions rather than schedules. So this is
not a store constructor and its parts are not "as stored". It exists for an
application that plans elsewhere, a preview computed on another device or a
plan carried across a process boundary, and then commits with it.

Unlike its siblings it VALIDATES. `PreparationPlan::from_parts` and
`PrepTransaction::from_parts` hand back parts that `build_prep_tx` re-checks
against the notes actually available, so an inconsistent preparation plan
fails loudly at the build. A plan's crossing values have no such backstop:
they are what the user consents to and what the transfers put on chain, and a
value off the `{1, 2, 5} * 10^k` series joins no anonymity set and is
unrecoverable once mined. Twelve invariants are enforced, arity before
content, in a fixed order, each with its own `MigrationPlanError` variant
carrying the offending indices and values.

Claim resolution is asked of the plan's own `planned_run` rather than
re-derived, so the first-fit rule pairing a crossing with its funding note
stays stated in one place; that is what makes `CommitError::InconsistentPlan`
unreachable for a plan this returns. The layer-serialization bound skips
empty layers, which is weaker than the base `plan_migration` advances, so it
cannot reject a plan the planner produced.

Two omissions are deliberate. The transfer schedule is not required to be
monotone in crossing index, since it is shuffled precisely because the
crossings are non-increasing; and the crossing count is not judged against the
per-run note cap, which is caller-settable and which a plan carries no record
of, so it is unverifiable here by construction.

`Schedule::new` derives its expiry rather than taking one, because a
caller-stamped per-transaction expiry would reintroduce the fingerprint the
canonical rolling window exists to remove. `ProvedTransaction::from_parts`
loses its test-only gate, with a `with_lock_owner` builder for the reservation
`prove_transfer` reads from `lock_spent_notes`: `MigrationProver` is a public
trait and `store_proved_transaction` takes the value by move, so an
out-of-tree prover could not otherwise hand its result to a store in a release
build.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The round-trip test reports a rejected plan through `prop_assert`, and
compares the rebuilt plan with `prop_assert_eq` so a failure prints both
sides, but the test module imported only `proptest` itself. Neither macro is
brought in by that import, so the lib test target did not build.

Also reflow two blocks rustfmt reformats.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dannywillems
dannywillems force-pushed the dw/pool-migration-plan-from-parts branch from fa4d02e to 03a3097 Compare August 17, 2026 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant