zcash_client_backend: Support creation of transparent change#2550
Conversation
80741aa to
a404d2b
Compare
a404d2b to
8e370ef
Compare
| .clone() | ||
| // Count the standard size of the P2PKH change output when change is | ||
| // to be returned to the transparent pool. | ||
| .chain(wants_transparent_change.then_some(P2PKH_STANDARD_OUTPUT_SIZE)), |
There was a problem hiding this comment.
There's a potential gap here - what happens when the input being spent is a p2sh multisig address? In that case, the transparent change should be returned to the original p2sh multisig. This isn't necessary for the transparent address handling that's needed for z_sendfromaccount at the moment, but I think that this needs an issue filed for follow-up.
There was a problem hiding this comment.
Yeah, that's a gap. I've created #2570 to track it.
|
|
||
| assert_matches!( | ||
| st.wallet_mut().reserve_next_n_internal_addresses(account_id, 1), | ||
| Err(e) if is_reached_gap_limit(&e, account_id, 5) |
There was a problem hiding this comment.
Is this pattern guard the best way to assert the error reason? Why not match the exact error code?
e.g. if this returns an unexpected error AND is_reached_gap_limit() trips, then this assert will pass when it shouldn't
There was a problem hiding this comment.
The indirection is deliberate, though the reason is non-obvious in that the specialization lives at the call site. This test is in zcash_client_backend, which can't name SqliteClientError (that would invert the crate dependency), so instead it takes a predicate, which is the same pattern as send_multi_step_proposed_transfer. The sqlite-side closure matches the exact variant, scope, and index, so an unexpected error can't trip it. I've documented that contract on the test parameter.
There was a problem hiding this comment.
We have some backend defined errors, and I wonder if this would be better as one. Not worth blocking on, but something we should log for a future decision. Maybe just a follow-up issue for now
| // This variant is placed at the end of the enum in order to preserve the encoded | ||
| // representation of the prior variants. | ||
| #[cfg(feature = "transparent-inputs")] | ||
| InternalTransparent { |
There was a problem hiding this comment.
Does InternalTransparent need to be distinct from `InternalAccount'?
There was a problem hiding this comment.
Yes! InternalAccount is converted via into_recipient_with_note and carries a shielded Note, while transparent change goes through the outpoint path and must record the on-chain-observable recipient address. This variant is just the construction-time mirror of the pre-existing Recipient::InternalTransparent / Recipient::InternalAccount distinction. There is some rationale on Recipient::InternalTransparent in zcash_client_backend/src/wallet.rs.
There was a problem hiding this comment.
Hmm, the naming is confusing, because InternalTransparent is internal and belonging to the account. Let's not block here, but later we should revisit the naming and perhaps even the deliniation of those two variants. I suspect there's an axis of symmetry we can refactor on here.
8e370ef to
7ed2c4e
Compare
Adds `fees::TransparentChangePolicy` and `with_transparent_change_policy` builders on the ZIP 317 and fixed change strategies. When a strategy is configured with `TransparentChangeAllowed` and the net flows of the transaction under construction are fully transparent, change is returned to the transparent pool as a single non-ephemeral output (represented by the new `ChangeValue::transparent` constructor) instead of being shielded to the fallback change pool. The policy has no effect on transactions that involve shielded flows, so shielded change information is never leaked to the transparent pool. The proposal protobuf encoding is wire-compatible: a transparent change value is encoded using the existing transparent value pool with `isEphemeral` unset, a combination that previously failed to decode with `ProposalDecodingError::InvalidChangeRecipient`. This is required in order for `zallet` to replicate `zcashd`'s `z_sendmany` behavior for fully-transparent spends. Part of #1839. Co-Authored-By: Claude <noreply@anthropic.com>
…outputs Adds `WalletWrite::reserve_next_n_internal_addresses`, which reserves BIP 44 internal-scope (change) transparent addresses parallel to the existing ephemeral address reservation API, and implements it for `WalletDb` using the existing scope-generic reservation machinery, subject to the internal-scope gap limit. No migration is required: internal-scope gap addresses are already generated at account creation. `create_proposed_transactions` now realizes non-ephemeral transparent change values from a proposal by reserving internal-scope addresses, adding the corresponding transparent outputs to the transaction, and recording them via the existing `Recipient::InternalTransparent` variant; the PCZT construction and extraction paths are extended accordingly. The new `PcztRecipient::InternalTransparent` variant is appended to the end of the enum to preserve the encoded representation of the prior variants. Includes end-to-end coverage of the t->t-with-change lifecycle (proposal, serialization round trip, transaction creation, internal-scope address exposure, and post-mining spendability of the change output), of the exact-balance no-change case, and of internal-scope gap limit enforcement. Part of #1839. Co-Authored-By: Claude <noreply@anthropic.com>
7ed2c4e to
d739b42
Compare
2539 has been merged. Unblocking by dismissing by request for changes.
…ielded Rename `Recipient::InternalAccount` (and the `BuildRecipient`/`PcztRecipient` mirrors) to `InternalShielded`, for symmetry with `Recipient::InternalTransparent`. The prior name did not communicate the actual distinction between the two internal variants, which is payload domain (shielded note vs. transparent address) rather than any special relationship to "the account" — as noted in review of zcash#2550: zcash#2595 The enum-level and per-variant doc comments on `Recipient` are expanded to spell out the two independent axes the variants vary along (relationship to the wallet, and payload domain). `PcztRecipient`'s variant order (and thus its postcard encoding) is unchanged; only the variant name changes for that private mirror type. Co-Authored-By: Claude <noreply@anthropic.com>
Closes #1839.
Adds support for returning change to the transparent pool when the net flows of a transaction under construction are fully transparent, enabling
zalletto replicatezcashd'sz_sendmanybehavior for fully-transparent spends. Builds on the transparent-spending support forpropose_transactionadded in #2422.All new API surface is gated on the
transparent-inputsfeature flag.Design
fees::TransparentChangePolicy— a new policy enum with variantsShieldChange(the default, preserving existing behavior) andTransparentChangeAllowed. Configured on the ZIP 317 and fixed change strategies via newwith_transparent_change_policybuilder methods. The policy only takes effect when the transaction's net flows are fully transparent; transactions involving shielded flows always shield their change, so no shielded change information is ever leaked to the transparent pool.ChangeValue::transparent(value)— a new non-ephemeral transparent change value, distinct from the ephemeral (ZIP 320) transparent output value. Transparent change is always emitted as a single output (the note-splitting policy applies only to shielded change), the exact-balance no-change case is preserved, zero-valued transparent outputs are never created, and the existingDustOutputPolicymachinery applies. The proposal protobuf encoding is wire-compatible: transparent change is encoded with the existing transparentvaluePoolandisEphemeralunset, a combination that previously failed to decode.WalletWrite::reserve_next_n_internal_addresses— reserves BIP 44 internal-scope (change) transparent addresses, parallel to the existing ephemeral reservation API. Thezcash_client_sqliteimplementation reuses the existing scope-generic reservation machinery and internal-scope gap limit; no migration is required, since internal-scope gap addresses are already generated at account creation and received change is recorded via the existingRecipient::InternalTransparenthandling.create_proposed_transactions(and the PCZT construction/extraction paths) now realize transparent change outputs by reserving internal-scope addresses and recording the outputs against the receiving account.Because general (non-shielding) transfers exclude coinbase UTXOs from input selection (
CoinbaseFilter::NonCoinbaseOnly), transparent change can never be produced by a transaction that spends coinbase outputs, structurally satisfyingzcashd's coinbase-spend-forbids-change rule.Testing
cargo fmt --check,cargo clippy --all-features --all-targets -- -D warnings, workspace checks across feature combinations (--no-default-features,transparent-inputsalone and withorchard), intra-doc link validation, and the fullzcash_client_sqlitetest suite (319 passed).AI disclosure
The commits in this PR were prepared with AI assistance (Claude / opencode) and carry
Co-Authored-Bytrailers accordingly. I have reviewed and take responsibility for all changes.