fix: preserve shielding proposal anchors#2603
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a failure mode in transparent-to-shielded transaction proposals by ensuring that proposal steps without shielded inputs still preserve the wallet checkpoint (anchor height) selected during proposal construction, avoiding later AnchorNotFound errors when the target-height-derived anchor is not checkpointed.
Changes:
- Preserve the selected checkpoint anchor height on newly constructed proposal steps even when they have no shielded inputs.
- Treat the zero
anchorHeightsentinel as “legacy deferred anchor” semantics for decoded older proposals. - Add/adjust regression coverage and document the behavioral change in the changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| zcash_client_backend/src/proto.rs | Adjusts serialization commentary around the zero sentinel encoding for deferred legacy anchors. |
| zcash_client_backend/src/proposal.rs | Changes step construction semantics to retain anchor heights for input-less steps (while keeping legacy sentinel behavior). |
| zcash_client_backend/src/data_api/wallet.rs | Updates anchor resolution behavior/docs to reflect that only legacy decoded steps may defer anchors. |
| zcash_client_backend/src/data_api/testing/pool.rs | Updates regression test to ensure newly constructed shielding proposals serialize a preserved anchor and legacy proposals still decode/build correctly. |
| zcash_client_backend/CHANGELOG.md | Notes the bug fix under the planned release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Preserve the wallet-selected checkpoint for newly constructed input-less steps while retaining the zero sentinel for legacy deferred anchors. Reject zero anchors on steps that spend shielded inputs.
a8b0395 to
c6c7800
Compare
| // inputs: output-only Orchard and Ironwood bundles still require a tree anchor. The zero | ||
| // sentinel remains reserved for decoded legacy proposals that deferred their anchor, and | ||
| // is invalid for steps that spend shielded inputs. | ||
| let anchor_height = if u32::from(anchor_height) == 0 { |
There was a problem hiding this comment.
This comparison makes me suspicious; I would expect the anchor height to only be zero as an artifact of protobuf deserialization returning the default; however, ideally we would exclude that from the set of valid values for anchor heights, as of course genesis can never be an anchor for any valid spend. It looks to me like this PR might be fixing an error (or an API design issue) somewhere else in the stack.
There was a problem hiding this comment.
I will push a fix to this branch.
`Step::from_parts` previously accepted a non-optional `BlockHeight` and reinterpreted the value `0` as "no anchor" — leaking the protobuf `anchorHeight` scalar-default sentinel into the domain constructor and treating genesis, which can never be a valid anchor, as a magic value. Let `Step::from_parts` take `anchor_height: Option<BlockHeight>` like the field it populates, and require a concrete anchor for any step that produces a shielded bundle — one that spends shielded notes, pays to a shielded pool, or returns shielded change. Every shielded-tree lookup such a step performs, including the dummy spends that pad an output-only bundle, is bound to that anchor, so an anchor selected at proposal construction time is what keeps a routed-output transaction indistinguishable from one that spends real notes. Only a purely transparent step may carry `None`. Proposals are ephemeral, so a `0` anchor on a shielded-bundle step is malformed rather than a legacy deferral: the protobuf decoder rejects it at the parse boundary with a new `ProposalDecodingError::MissingShieldedAnchor`, and `Step::from_parts` enforces the same invariant for direct callers with a new `ProposalError::MissingShieldedAnchor`. Both are covered by tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| ); | ||
|
|
||
| // A purely transparent step may carry no anchor. | ||
| assert_matches!( |
There was a problem hiding this comment.
(nit) this assertion would be better in a different "fully_transparent_step_passes_without_anchor" test or something. Not worth blocking though.
Transparent-to-shielded proposals discarded the checkpoint selected at proposal time whenever they had no shielded inputs. The transaction builder then resolved an anchor from the target height, which may not be checkpointed in a transparent-only chain and fails with
AnchorNotFound.Preserve the selected checkpoint for newly generated input-less steps. The zero sentinel still represents legacy deferred anchors, so older serialized proposals retain their existing interpretation.
AI assistance: Codex prepared the change and regression coverage.