feat: ironwood support - #2419
Merged
Merged
Conversation
…lary Six deliberately failing tests pin the migration lifecycle and privacy findings of issue #2493 before their fixes land, each asserting the mechanism-free invariant the design session ratified: the schedule never assigns a bucket below the NU6.3 activation (finding 6), a rebuilt part never keeps a target below its new window (finding 7), a migration whose every part is terminal with nothing replannable reaches completion (finding 8), a broadcast part is not rebuilt while its expiry lies beyond the wallet's spend evidence (finding 8's false-invalidation race, whose failure output shows the erased txid), catch-up does not silently skip an overdue signed part (finding 9), and value_migrated counts only confirmed part denominations (finding 10). Every test fails on the exact diagnosed defect; fixes follow on this branch, revising the finding 6, 7, and 9 assertions to their ratified final forms as the new seams appear. The design session's vocabulary rides along: the CONTEXT.md glossary gains Pool Activation (the height a pool begins to exist, derived through the workspace's single pool-to-upgrade mapping) and Spend-Evidence Height (the height through which spend and inclusion evidence is complete — the only lawful input to condemnation), and ADR 0012 records why pepper-sync owns the Pool Activation derivation and why zingo_common_components was rejected as its home. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pool Activation becomes a real construct (ADR 0012): pepper-sync owns the workspace's single pool-to-upgrade mapping (pool_upgrade) and the PoolActivation newtype whose only production constructor is the derivation over consensus parameters. pepper-sync's scan-range downgrade ladder and fallback clamp, zingolib's effective_pool_birthday, and every migration activation lookup now derive from it. The schedule module gains the floor and a placement monopoly. Bucket choice is single-source (first_permitted_bucket: after now, never below the activation floor), plan_schedule demands a PoolActivation in its signature so floor omission is unrepresentable, and every move of a part between buckets goes through place (fresh jittered target inside the new window) or place_immediate (explicitly due now) — a part can no longer carry a stale target from a previous bucket, and the rebuild, catch-up, and immediate-mode sites all converted. Reconciliation gains the Spend-Evidence Height: ChainView names the gap-free, nullifier-mapped frontier, and condemnation (Expired, hence rebuild) binds to it instead of the chain tip, closing the false-invalidation race where a part's exonerating spend sat in the unscanned gap. The completion rule unifies: when every part is terminal, the migration concludes with the residual unless a remainder worth more than the Sweep Minimum exists — Complete means nothing left to do. An overdue signed part leaves the catch-up cohort for the new AwaitingExpiry class: broadcasting its stale signature would mine a permanent lateness fingerprint (cleartext expiry, old anchor) into its denomination cohort, so it waits out its expiry and rebuilds indistinguishably; status says why nothing was sent. value_migrated becomes the sum of the current migration's confirmed part denominations, nothing else. Consent and binding become atomic: start_ironwood_migration and the immediate path's per-round bind each run as one synchronous critical section under a single write guard, with planning refactored to a wallet-level function, so the notes hashed into consent are the notes bound — the guarantee is the lock's exclusivity plus synchronousness, not a runtime check. The six red-baseline tests are green, with the finding 6, 7, and 9 tests revised to their ratified final forms (supply the floor; expect a fresh in-window target from placement; expect awaiting-expiry reporting instead of a broadcast attempt). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The per-round resume now re-verifies the state it is about to drive — mode Immediate and matching account — so the consent guarantee lives in the state machine rather than in receiver discipline at the API surface; a future scheduled-flow split driver or second client handle cannot reopen the consent collapse through the resume path. An immediate round whose current bucket boundary precedes the NU6.3 activation is refused with the new typed ActivationBoundaryPending error, naming the first anchorable boundary to retry after. Previously every part skipped on its pre-activation boundary, the vacuous confirmation wait returned instantly, and the loop burned all its rounds into a misleading SplitDidNotConverge. The SkipReason doc now states the honest healing story for the one state that can still carry such a boundary: a schedule persisted before the activation floor existed. The entry gate clears completed history before comparing accounts, so one account's finished migration cannot block another's forever, and the entry now plans before gating, so a failed precondition erases no history it never used. The schedule module gains first_anchorable_boundary beside the bucket chooser, both derived from one activation-bucket computation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CI caught the one chain-bound test this branch's completion-rule change touches: bound_note_reservation_and_external_spend_invalidation asserted the superseded semantics, expecting ReplanRemainder whenever any spendable Orchard value remained. Its scenario's remainder is exactly the Sweep Minimum (10_000 zatoshis of change), which the ratified rule (#2493 finding 8, grilling decision 6) deliberately declines to replan — a replan would strand everything it planned. The test now pins the ratified contract end to end on a live chain: no replan is offered for the Sweep-Minimum remainder, and the next reconciliation concludes the migration with MarkComplete disclosing the 10_000 residual, the phase reaching Complete — the first live coverage of the completion rule's terminal path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The wrapper method appends a checkpoint recording the current frontier
position at a new height, so its name now says so: append_checkpoint,
returning CheckpointAppendOutcome::{Appended, NotAboveNewest}. The
verb-first name also sharpens the contrast with the store-level
add_checkpoint channel, where "add" correctly connotes order-agnostic
insertion of historical checkpoints.
The NotAboveNewest docs now explain why the refusal is definitional
rather than a library limitation, and the glossary gains Checkpoint and
Frontier entries resolved during review discussion of this module.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two review findings from Oscar-Pepper. The bare-subtree-root refetch was blind to shard-range accounting: add_shard_ranges pushes a range per fetched root unconditionally, so every session's refetch appended a duplicate range — and after a reorg that moved the subtree's completing height, the stored last range was stale besides. The fix is pop-then-readd: when a pool's fetch resumes below its stored root count, the newest stored shard range is dropped and the fold rebuilds it from the refetched root — idempotent when the completing height is unchanged, corrected when a reorg moved it, pinned by a state-level unit test over both cases. The truncation plumbing also slims per review: truncate_shard_trees returns to the single-height signature and derives each tree's outcome through the pure per-pool rule at the point of application, so the cross-module plan type is gone while every decision stays pure, evidence-carrying, and exhaustively tested. plan_truncation keeps only the wallet-level routing (no-op, clear-all, or truncate-to-height). A proptest also stops starving its runner: the never-condemned property now constructs satisfying checkpoint sets instead of rejecting generated ones. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
truncate_shard_trees repeated the same plan-then-apply sequence for each of the three pool trees. The sequence now lives in one generic helper, truncate_pool_tree, which reads the tree's facts, decides its outcome through the pure per-pool rule, and applies it; the trait method is three one-line calls. A loop cannot replace the three calls because the sapling tree's node type differs from the orchard and ironwood trees', making the collection heterogeneous. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The CI doc gate (cargo-checkmate with warnings denied) rejects public documentation that intra-doc-links to a private item. Three doc sites still linked [plan_pool_truncation] after commit 21ca330 deliberately internalized that helper to pub(crate): the truncate module header, the TruncationPlan::Truncate variant, and truncate_shard_trees in the wallet traits. The helper's privacy is the design, so the links become plain code text instead of widening its visibility. Verified with RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace, which now completes clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fix: stop migrated-wallet wipes and migration consent bypasses (#2419 review highs)
…n-lifecycle-mediums
This was referenced Jul 23, 2026
Wallets are born; pools activate. The naming policy, now ratified in ADR 0012 and the glossary: a wallet has a Birthday, a network upgrade has an activation height, and a pool has its Pool Activation — the activation height of the upgrade that introduces it. No pool or upgrade concept is named a birthday, and no activation concept is named a "start". The one violation, effective_pool_birthday, dissolves: its single caller now writes the derivation inline — the wallet Birthday clamped to the Pool Activation — under the role-named local scan_floor, with the invariant comment moved to the call site. The Birthday glossary entry gains the exclusivity sentence, the Pool Activation entry and the ADR drop the conflated phrase, and the max_with doc says which term contributes which half of the clamp. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Pool Activation ADR declares a second pool-to-upgrade mapping anywhere in the workspace a defect, and add_shard_ranges still carried one: a three-arm ShieldedPool ladder over raw activation_height calls. It and the sapling-era boundary lookup in final_tree_sizes now derive through PoolActivation::of, which removes the last NetworkUpgrade reference from the sync state module entirely. The ADR also moves from 0012 to 0014, with its two code references updated: 0010 through 0013 are claimed by in-flight branches (the OP_RETURN pipeline, the typed-error arc, the sealed-wallet draft in PR #2496, and the viewmodel extraction), so keeping 0012 would collide with whichever merges next. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
No production code calls it: the live catch-up path places overdue parts through place_immediate. The function also moved parts between buckets through the raw shift transition, which preserves the stale jitter target from the old bucket — the exact mechanism the placement monopoly retires — so keeping it kept the bug class alive in dead code. The ZIP 318 shift-the-remaining-schedule rule it once gestured at is tracked as part of the wallet-reopen policy deviation in issue #2519, and any future implementation would route through place. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mediums fix: eliminate the migration-lifecycle bug classes (#2493 findings 6-11)
Review follow-ups on the reshaped branch. The bucket modulus is deserialized from the wallet file with no validation, and every bucket computation divides or multiplies by it, so a corrupt file could panic bucket_index or silently collapse every boundary to zero. The store now rejects a zero modulus at read with a typed InvalidData error, pinned by a test beside the existing never-silently-truncate precedent; the one scattered guard (activation_bucket) drops in favor of the field invariant, now stated on MigrationParams::bucket_modulus. The reassign test also grows into the rule its name states — every state except Expired refuses the transition — and the PoolActivation doc cites the ADR by its actual path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fix: reject a zero bucket modulus at wallet-file read (PR #2501 review follow-ups)
zancas
marked this pull request as ready for review
July 24, 2026 01:48
Member
|
The audit is complete. All five putative high-severity issues of #2493 were real — each harm test fails on its fix commit's parent — and all five are genuinely fixed on this branch, where the same tests pass. The full verdict, test by test:
|
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
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.
This branch makes the wallet ironwood-capable from the network edge to the command line. The pepper-sync crate scans the third shielded pool — ironwood compact-block actions, a third shard tree, and a third nullifier map — counts it in sync progress and batching budgets, and persists it behind version-gated wallet-format bumps (SyncState v4, NullifierMap v2, TreeBounds, WalletTransaction, and ShardTrees v1), each with migration tests over old blobs. Following ZIP 326, receivers and viewing keys are scoped to the Orchard protocol rather than to a pool, so ironwood shares orchard key material and unified addresses gain no new receiver. On top of that, zingolib extends note selection, balances, summaries, and the
zcash_client_backendtrait implementations to the new pool, riding the ironwood dependency train (zcash_client_backend0.24.0-rc.1,zcash_primitives0.29.0,orchard0.15.0) plus a[patch.crates-io]git pin oflightwallet-protocolthat carries the ironwood proto fields until upstream regenerates and publishes them.The largest single addition is the consent-bound orchard-to-ironwood migration engine of ZIP 318, in the new
zingolib::wallet::migrationmodule: deterministic denomination sizing and note splitting (split), part records with an explicit state machine (parts), bucket-scheduled broadcasts with jitter (schedule), versioned persistence (store), restart reconciliation (reconcile), and an immediate drain path (drain).LightClientmethods orchestrate consent, rounds, catch-up, and status reporting, andzingo-cliexposes the correspondingmigratecommands, withshieldnow targeting the ironwood pool. This work arrived through PRs #2428 and #2429, the summary and value-transfer extensions of #2463 and #2466, the sweep-floor fix of #2468, the zebrad-testing line of #2461, the stable backport of #2473, and #2475, which also carried #2477's scan-progress accounting fix and its regression test.The validation stack behind the branch runs zebrad 6.0.0 and zainod 0.6.0-rc.1-no-tls inside a content-addressed CI image (#2472, #2476). The
tip_spend_rejectionsuite documents a completed investigation: the boundary-adjacent orchard-output rejection that motivated three rounds of hypothesis discrimination was a zebra 6.0.0-rc.0 mempool-admission bug, fixed upstream, and the suite now pins acceptance as a regression sentinel. Because zainod 0.6.0 masks validator rejection text (zingolabs/zaino#1404, which holds the indexer at the release candidate), test failures are classified by a new attribution instrument inzingolib_testutilsthat judges the wallet's retained transaction bytes directly against zebrad's JSON-RPC, twice — at failure time and after five blocks of distance — and names the responsible layer.Three known reds remain on this branch and are recorded rather than resolved:
mine_to_ironwood,send_mined_ironwood_to_ironwood, andsend_orchard_back_and_forthfail deterministically on freshly mined zebrad 6.0.0 chains, each observing a balance exactly one post-funding-stream block reward (618,780,000 zatoshis) below expectation; they pass locally only where chain caches mined under rc.0 replay, because cache manifests key on the validator's type rather than its version. PRs #2469 (OP_RETURN spend pipeline) and #2470 (Nym transport design) remain open against this branch. The table below classifies every changed line by package and by logic type, using file paths as the classifier.