Add Ironwood migration batch-limit test control#80
Open
czarcas7ic wants to merge 53 commits into
Open
Conversation
Point librustzcash at valargroup/librustzcash 9e0af8fd (adam/pczt-elidable-fields-ironwood-wallet), zcash_voting at valargroup/zcash_voting 4a136f1f (adam/pczt-elidable-fields-pins), and patch orchard onto zcash/orchard 0aa06368 (PR 523, adam/pczt-recompute-fields, 0.15.0-pre.2). This is the same optional-field PCZT stack the Keystone firmware is pinned to, so wallet and device produce byte-identical migration PCZTs. The graph resolves to a single orchard node.
Emit optional-field (elided) Orchard/Ironwood actions from the wallet: plan and verify which derived fields (cmx, nullifier, rk, cv_net, ephemeral_key, enc_ciphertext) the device can recompute, strip them from the signer request, and refill from the wallet's clone on the way back. Covered by batch_redaction_elides_verified_fields_and_signs_identically, which asserts the redact/refill round-trip signs to a byte-identical tx. Also includes test scaffolding for exercising the large-batch round-trip on device: the temporary 50-part force-split and a ~2x fountain redundancy dial, both clearly marked as temporary in the code.
Full-screen animated-QR view (tap to dismiss) plus a "Full screen QR" button on the signing modal, so a struggling Keystone camera can scan a physically larger code. Also raises the request fragment length to 300 B and the frame interval to 200 ms (5 fps) as temporary dials for the bad-camera 50-note migration experiment.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a6492dd53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # rust/Cargo.toml # rust/src/wallet/sync/pczt.rs # rust/src/wallet/sync/send.rs
The dependency stack now keeps BundleType::DEFAULT padded; migration child proposals and PCZTs explicitly request DEFAULT_UNPADDED via the new change-strategy knob, create_pczt_from_proposal parameter, and direct migration builders, so children keep the 1-spend/1-output action shape and 10000-zat fee the planner assumes. Denomination prep transactions, regular sends, and shielding stay padded. Also bumps the orchard, librustzcash, and zcash_voting pins onto the padded-default revisions so the whole graph resolves to single nodes.
The dependency stack now models padding as pad_to_minimum: Option<u8> with BundleType::UNPADDED replacing DEFAULT_UNPADDED; rename the references and move the orchard, librustzcash, and zcash_voting pins onto that lineage. No behavior change: migration children stay unpadded, everything else stays padded.
Retire the 2552-era fork pins: librustzcash moves to the adam/pczt-2557-teststack-wallet head (upstream PCZT v2 wire and roles plus the Ironwood wallet layer), zcash_voting to its matching teststack-wallet pins, and the orchard patch to the merged unpadded-bundles head on zcash/orchard. Version requirements follow the pinned crates, and shardtree/incrementalmerkletree gain a [patch.crates-io] onto the incrementalmerkletree rev the pinned stack builds against (the retained-checkpoint ShardStore API has no published release, and [patch] is not transitive). API adaptations that come with the repin: - spend nullifier and rk are required fields again; drop the fork-era Option handling in pczt_spend_nullifiers and the anchor/witness updater. - FeeRule::fee_required takes a separate ironwood_action_count; the migration fee estimate now counts 1 Orchard + 1 Ironwood action (identical fee), and the legacy V5 paths pass 0. - create_pczt_from_proposal takes a target expiry height override (pass None to keep the builder-derived expiry) and now pins V5; the transparent-shielding flow requests its version explicitly from the proposal's target height so post-NU6.3 shielding still lands in the Ironwood pool. - propose_transaction takes a TransparentSpendPolicy; reserved-note sends stay ShieldedOnly. propose_send_max_transfer no longer takes a proposed tx version, so the send-max estimate drops its version plumbing. - TransparentOutputFilter is now CoinbaseFilter, the transaction Builder lost its lifetime parameter, with_ironwood_tree_mut reports a missing Ironwood tree as None, and ShieldedPool grew an Ironwood variant. The fork's derived-field elision machinery does not exist on this stack; the next commit reworks the batch redaction onto the upstream compact-PCZT format, and the crate does not build in between.
The teststack replaces the fork's four-field optionality and MemoKind tagging with the upstream compact-PCZT format: cv_net is redactable (recomputed from the wire values and rcv), and a decryptable output enc_ciphertext travels as its stripped memo plaintext, re-encrypted deterministically from the wire note fields by Pczt::resolve_fields. nullifier, rk, cmx, and ephemeral_key are required wire fields again. Rework redact_pczt_for_batch_signer accordingly: keep the batch spend fvk / spend_auth_sig / output metadata clears, drop the fork format's plan/self-check machinery (the upstream swap self-guards by replacing only ciphertexts the wire note fields actually decrypt, and resolve_fields hard-fails on any ephemeral-key mismatch), and clear cv_net, the bundle bsks, and the v6 anchors the sighash does not commit to. Both migration-child outputs — the Ironwood migration output and the deterministic zero-value Orchard output — decrypt from their wire note fields, so both ciphertexts ride as memo plaintext. The reworked elision test proves the round-trip byte-identically and now proves "signs identically" literally: the resolved compact request yields a byte-identical v6 shielded sighash to the unredacted base, and a base-side signature verifies under the request's own sighash and wire rk. The signatures-only response decode is untouched.
The upstream optional-anchor and Orchard-v2 optional-spend-field work has merged, and current librustzcash main changes the v2 PCZT wire layout (nullifier/rk become optional slots). A wallet pinned to the older da388cbf carry therefore emits batches that a main-pinned Keystone firmware misparses; move this branch onto the same upstream rev the firmware now pins. - Cargo.toml/Cargo.lock: all librustzcash deps -> 877e574adb. Drop the crates-io patch redirects for the librustzcash family: the whole graph already resolves those crates through the git deps transitively, so the lock carries no registry copies with or without them. zcash_voting/vote-commitment-tree -> the matching upstream-main-pinned rev (7c1139c1). - send.rs/pczt.rs/mod.rs: thread the decided tx version through StoredProposal and apply it via with_proposed_version at PCZT/transaction build time, replacing the propose-time carry. Change-pool selection follows input pools upstream now, so zip317_helper's V5 legacy-change override is gone. set_v6_orchard_anchor is renamed set_orchard_anchor upstream. NetworkUpgrade::Nu6_3 is no longer cfg-gated, so the transparent-max ironwood probe loses its cfg fallback. The shielding test checkpoints the empty trees because propose_shielding derives its heights from scan progress. Verified: 276 wallet tests pass at these pins, and this exact tree state matches the build used for the firmware cross-stack wire proof and the successful on-device Keystone batch flow.
czarcas7ic
changed the title
czarcas7ic
changed the title
Use the shared Keystone registry types around the PCZT-owned Postcard payloads, validate echoed request IDs before positional result mapping, and assign a distinct transport ID to each migration batch chunk.
…nwood Send two Orchard actions in migration children
Trim migration PCZTs and remove obsolete NU6.3 cfg
# Conflicts: # rust/src/wallet/keystone.rs # rust/src/wallet/sync/migration.rs # rust/src/wallet/sync/pczt.rs # rust/src/wallet/sync/send.rs
czarcas7ic
changed the title
# Conflicts: # lib/src/features/migration/models/migration_view_state.dart # rust/src/wallet/sync/migration.rs
…OFF)" This reverts commit acfc5b4.
czarcas7ic
changed the title
czarcas7ic
changed the title
…gration-pczt-optional-fields
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a temporary public-testnet denomination override above the production migration flow. For the one-input hardware test wallet, it forces 37 migration outputs; the planner produces three padded split PCZTs, so the request fills the 40-message Keystone batch ceiling exactly.
This branch uses ordinary testnet mode, the public testnet endpoint, and canonical testnet activation heights inherited from the production flow. The 37-output override is validation-only scaffolding and must be removed before the migration ships.