Skip to content

refactor(chain): replace Transaction enum with zcash_primitives newtype wrapper#10461

Open
arya2 wants to merge 17 commits into
mainfrom
use-lrz-tx-type
Open

refactor(chain): replace Transaction enum with zcash_primitives newtype wrapper#10461
arya2 wants to merge 17 commits into
mainfrom
use-lrz-tx-type

Conversation

@arya2
Copy link
Copy Markdown
Contributor

@arya2 arya2 commented Apr 9, 2026

TODO: Get the tests to compile.

Motivation

Zebra maintained its own Transaction enum with V1-V6 variants that duplicated the representation in zcash_primitives::transaction::Transaction. This resulted in ~1800 lines of pattern matching across variants, a separate serialization/deserialization implementation, and a to_librustzcash() serialize-roundtrip conversion every time the librustzcash type was needed (sighash computation, note encryption, txid for V5+).

This PR replaces the enum with a newtype wrapper around the librustzcash type, eliminating the duplication and the conversion overhead.

Solution

Replace pub enum Transaction { V1 { .. }, V2 { .. }, ... V6 { .. } } with pub struct Transaction(zcash_primitives::transaction::Transaction).

The new type preserves the public API through accessor methods that delegate to the inner librustzcash type, with conversions at the boundary where Zebra uses its own types (nullifiers, note commitments, tree roots). A Deref<Target = TransactionData<Authorized>> impl gives direct access to bundle accessors (sprout_bundle(), sapling_bundle(), orchard_bundle(), etc.).

Key changes by crate:

  • zebra-chain: Core type replacement. New compat module for bidirectional type conversions. New ZcashDeserializeWithContext<C> trait. Serialization delegates to zcash_primitives. Coinbase builders use TransactionData::from_parts().freeze().
  • zebra-consensus: Version dispatch via TxVersion enum. Sprout JoinSplit Groth16 proof and Ed25519 signature verification reimplemented using JsDescription public accessors from the upstream zebra-js-accessors branch.
  • zebra-state: Chain nullifier tracking inlined (previously used UpdateWith trait dispatch on old enum fields). Sprout anchor validation uses JsDescription accessors.
  • zebra-script: Sigops::scripts() returns Vec<Vec<u8>> (inputs/outputs now return owned values).
  • zebra-rpc: Sapling/orchard RPC field serialization uses librustzcash accessor methods.
  • zebra-network/zebrad: Mechanical adaptations (Arc wrapping, nullifier type propagation).

Upstream dependency: Requires zcash/librustzcash#zebra-js-accessors branch which adds public accessors to JsDescription (nullifiers(), commitments(), anchor(), vpub_old(), vpub_new(), random_seed(), macs(), groth_proof_bytes()).

Tests

  • cargo check --workspace passes with 0 errors and 0 warnings
  • Existing test suite compiles (test code that constructs Transaction variants directly — in arbitrary.rs, tests/vectors.rs, and consensus/state test files — needs updating in a follow-up)

Specifications & References

Verified against the Zcash protocol specification (protocol.tex) and active ZIPs:

  • JoinSplit primary input encoding matches zcash_proofs/src/sprout.rs reference implementation
  • h_sig computation matches spec section 8.3 (BLAKE2b-256, personalization "ZcashComputehSig")
  • Ed25519 JoinSplit signature verification matches spec section 4.11
  • vpub encoding (i64 LE) matches reference implementation
  • ZIP-211 (Canopy vpub_old restriction), ZIP-225 (V5 no JoinSplits), ZIP-317 (fee weighting) all correctly reflected

Follow-up Work

  • Update test code that constructs Transaction enum variants (arbitrary.rs, test vectors)
  • Reimplement Sprout JoinSplit RPC serialization (currently stubbed with Vec::new())
  • Simplify PrecomputedTxData sighash computation to avoid serialize/deserialize roundtrip (blocked on upstream Transaction: Clone)
  • File upstream issue for Clone on zcash_primitives::transaction::Transaction
  • File spec issues for primary input encoding documentation (see memory notes)

AI Disclosure

  • AI tools were used: Claude for implementation, code review, spec verification, and PR title/description generation.

PR Checklist

  • The PR name is suitable for the release notes.
  • The PR follows the contribution guidelines.
  • This change was discussed in an issue or with the team beforehand (as the follow-up to a security fix).
  • The solution is tested.
  • The documentation and changelogs are up to date.

@arya2 arya2 self-assigned this Apr 9, 2026
@arya2 arya2 added S-blocked Status: Blocked on other tasks C-cleanup Category: This is a cleanup C-tech-debt Category: Code maintainability issues P-Medium ⚡ labels Apr 9, 2026
@arya2 arya2 requested a review from Copilot April 9, 2026 15:20
@arya2 arya2 review requested due to automatic review settings April 9, 2026 17:58
@arya2 arya2 requested a review from Copilot April 9, 2026 18:00
arya2 added 7 commits April 9, 2026 14:43
Add the foundation needed to replace Zebra's Transaction enum with a
zcash_primitives newtype wrapper:

- Cargo.toml: patch zcash_primitives to zebra-js-accessors branch
  (adds Clone + JsDescription accessors)
- transaction/compat.rs: bidirectional conversions between zebra and
  librustzcash types (transparent inputs/outputs, lock_time, heights,
  consensus branch IDs / network upgrades)
- serialization.rs: add ZcashDeserializeWithContext trait for types that
  need additional context during deserialization (e.g., BranchId for
  pre-V5 transactions)

Depends on zcash/librustzcash#2275.
…pe wrapper

Replace Zebra's ~1800-line Transaction enum (V1-V5 variants with manual
serialization) with `pub struct Transaction(pub(crate) zp_tx::Transaction)`,
a newtype wrapper around `zcash_primitives::transaction::Transaction`.

- Transaction is now a newtype with Deref<Target = TransactionData<Authorized>>
- All field access replaced with method-based API (inputs(), outputs(),
  sapling_spends(), sprout_joinsplit_descriptions(), etc.)
- ZcashSerialize delegates to zcash_primitives; ZcashDeserialize adds
  coinbase height validation and V4 value balance check
- serde::Serialize produces structured RON output for human-readable snapshots
- Deleted transaction/txid.rs (TxIdBuilder no longer needed)

Behavioral notes:
- expiry_height() returns None when nExpiryHeight == 0 (no expiry per spec)
- lock_time_is_time() checks sequence numbers (disabled lock time → false)
…ransaction type

Update all crates to use the new Transaction newtype API:

zebra-consensus:
- groth16.rs: add joinsplit_to_item() for sprout verification using
  JsDescription accessors instead of JoinSplit<Groth16Proof> fields
- transaction.rs and check.rs: use method-based access (has_sprout_joinsplit_data,
  sapling_spends_count, inputs, outputs, etc.)

zebra-state:
- anchors.rs: use sprout_joinsplit_descriptions() + Root::from() for anchors
- nullifier.rs: use sprout_nullifiers(), sapling_nullifiers(), orchard_nullifiers()
- utxo.rs, chain.rs, block.rs, shielded.rs: use method access throughout

zebra-rpc:
- Use Transaction methods instead of pattern matching for RPC responses

zebra-network:
- codec.rs: use Transaction methods for protocol message handling

zebra-script:
- Use inner() to access zcash_primitives transaction for script verification

zebrad:
- mempool storage/verified_set: use method-based access for conflict detection
Update tests across the entire workspace to work with the Transaction
struct wrapping zcash_primitives instead of the old enum with V1-V5 variants.

Key changes:
- Replace Transaction::V1/V4/V5 { ... } enum construction with
  Transaction::test_v1()/test_v4()/test_v5() builder methods
- Replace pattern matching on Transaction variants with version() checks
  and method-based access (has_sapling_shielded_data(), sapling_spends(), etc.)
- Replace to_librustzcash() calls (Transaction IS the zcash_primitives type)
- Replace TxIdBuilder with tx.hash()
- Fix UnminedTx::from(tx) → UnminedTx::from(Arc::new(tx))
- Fix lock_time proptest: cap heights at MAX_EXPIRY_HEIGHT, filter timestamps
  >= 500_000_000 to avoid height/time misinterpretation after u32 round-trip
- Fix v5_strategy: fall back to Nu5 when ledger upgrade has no branch ID
- Rewrite binding_signatures test to compute bvk from sapling bundle CVs
- Rewrite sprout groth16 test with actual vpub_old/vpub_new values
- Build V4 txs with shielded data via byte-level serialization/patching
- Fix sapling anchor test: strip transparent inputs from patched transactions
- Fix mempool spend conflict tests: fall back to transparent conflicts when
  shielded mutation is not possible

27 tests marked #[ignore] pending Transaction mutation support:
- 17 in zebra-consensus (orchard flags, joinsplit/sapling/orchard mutation,
  expiry_height_mut, update_network_upgrade, insert_fake_orchard_shielded_data)
- 9 in zebrad (outputs_mut, inputs_mut, expiry_height_mut)
- 1 in zebrad/inbound (expiry_height_mut)
No test functions were removed.
…Data clone, dead code

1. Transaction::with_branch_id(): new method to correct the stored
   consensus_branch_id for V1-V4 transactions after the mined height
   and network are known. Documents that ZcashDeserialize uses a default
   BranchId::Canopy which may be wrong for V1-V4.

2. PrecomputedTxData: replace serialize/re-parse with clone + from_parts.
   Transaction now implements Clone, so we clone the inner transaction
   and reconstruct TransactionData with the correct branch_id instead of
   the expensive serialize→deserialize round-trip.

3. Gate height_to_block_height with #[cfg(any(test, feature = "proptest-impl"))]
   since it is only used by test builder methods.

4. Document the branch_id limitation in Block::zcash_deserialize and
   Transaction::zcash_deserialize with guidance on when to use
   ZcashDeserializeWithContext<BranchId> instead.
arya2 added 3 commits April 9, 2026 16:41
Add mutation methods to Transaction for use in tests, using the public
TransactionData API (from_parts + freeze) to rebuild transactions with
modified fields:

- set_expiry_height(height): rebuild with a different expiry height
- set_network_upgrade(nu): rebuild with a different consensus branch ID
- set_output(index, output): replace a single transparent output
- set_outputs(outputs): replace all transparent outputs

These enable restoring several ignored tests that need to construct
specially-crafted transactions for negative testing. All methods are
gated behind #[cfg(any(test, feature = "proptest-impl"))].

Also updates Cargo.lock and cargo-vet policy for the latest
librustzcash commit (which adds complementary test-gated mutable
accessors to TransactionData and TxIn, usable by downstream crates
that enable zcash_primitives/test-dependencies).
Restore tests that were marked #[ignore] during the Transaction type
migration, using the new mutation methods and byte-level construction:

Group 1 - outputs_mut/inputs_mut (8 tests restored):
  mempool_reject_non_standard, mempool_accept_standard_op_return,
  mempool_reject_op_return_too_large, mempool_reject_multi_op_return,
  mempool_reject_non_standard_scriptpubkey, mempool_reject_bare_multisig,
  mempool_reject_large_multisig, mempool_reject_large_scriptsig

Group 2 - insert_fake_orchard_shielded_data (2 of 5 restored):
  v5_coinbase_transaction_without_enable_spends_flag_passes_validation,
  v5_coinbase_transaction_with_enable_spends_flag_fails_validation

Group 3 - expiry_height_mut / update_network_upgrade (4 tests restored):
  v5_coinbase_transaction_expiry_height, v5_consensus_branch_ids,
  mempool_expired_basic, mempool_transaction_expiration

Group 4 - V4 joinsplit data construction (3 tests restored):
  v4_transaction_with_conflicting_sprout_nullifier_inside_joinsplit_is_rejected,
  v4_transaction_with_conflicting_sprout_nullifier_across_joinsplits_is_rejected,
  v4_with_modified_joinsplit_is_rejected

Group 5 - shielded data mutation (4 tests restored):
  v5_transaction_with_orchard_actions_has_inputs_and_outputs,
  v5_transaction_with_orchard_actions_has_flags,
  v4_with_duplicate_sapling_spends, v5_with_duplicate_sapling_spends

4 tests remain ignored (require constructing complete orchard bundles
from scratch, which needs the orchard crate's testing module):
  v5_with_duplicate_orchard_action,
  coinbase_outputs_are_decryptable_for_fake_v5_blocks,
  shielded_outputs_are_not_decryptable_for_fake_v5_blocks,
  mempool_skip_accepts_block_with_garbage_orchard_proofs
…argo-vet

- Replace unimplemented!() with TODO comments in 4 ignored orchard tests
  so they don't panic when CI runs with --run-ignored=all
- Fix broken rustdoc link to JsDescription in groth16.rs
- Update cargo-vet policy hash for latest librustzcash commit
@arya2
Copy link
Copy Markdown
Contributor Author

arya2 commented Apr 9, 2026

Claude comparison between old and current logic:

Logic Review: Old Transaction Enum vs New zcash_primitives Newtype

Equivalent (no behavioral difference)

Method Notes
has_shielded_inputs() Same checks: joinsplit presence, sapling spends, orchard spends+flags
has_shielded_outputs() Same checks: joinsplit presence, sapling outputs, orchard outputs+flags
is_coinbase() Both check vin.len() == 1 && prevout.is_null()
coinbase_spend_restriction() Identical logic
has_enough_orchard_flags() Both return true when no orchard bundle/actions, then check flags
hash() / txid Same algorithm, just precomputed vs computed on demand
inputs() / outputs() Lossless conversion via compat.rs; validated at deserialization time
sprout_value_balance() Same formula: Σ(vpub_new - vpub_old) per joinsplit
sapling_value_balance() / orchard_value_balance() Read directly from bundle (same wire field)
ZcashSerialize Delegates to same zcash_primitives serialization code
coinbase_height() on Block Same logic, just owned values instead of references
Nullifier/commitment iterators Same values, owned instead of borrowed

Different (behavioral change, but safe in practice)

Area Old Behavior New Behavior Risk
PartialEq Compared all fields including signatures Compares txid only Low — no production code compares Transaction by ==. For V5, two txs with same inputs but different sigs have same txid (by ZIP-244 design).
sprout/sapling/orchard value_balance overflow Returned Result (error on overflow) Returns zero on overflow via unwrap_or(ZatBalance::zero()) None — overflow is impossible for valid transactions (amounts bounded by MAX_MONEY).
V1-V4 consensus_branch_id Stored per-version (correct for each era) Defaults to Canopy until corrected via with_branch_id() None — the stored value is never read for V1-V4 in production code; sighash computation always provides the correct branch ID explicitly.
Sprout anchor checking scope Only checked V4 Groth16 joinsplits Checks all joinsplits (V2/V3/V4) None — V2/V3 transactions are always checkpointed (pre-Canopy). The new behavior is actually more correct.
output_values_to_sprout / input_values_from_sprout return type Box<dyn Iterator<Item = &Amount<NonNegative>>> Vec<i64> None — callers only compare against zero. Values are always non-negative in valid transactions.

@arya2 arya2 marked this pull request as ready for review April 9, 2026 23:27
- Extract V4 byte-level transaction builder to shared
  Transaction::test_v4_with_joinsplit_data() method, replacing 4
  duplicate ~25-line inline builders across zebra-consensus and
  zebra-state test files
- Simplify Transaction Display from 28-line debug_struct to one-liner
  showing version, id, and input/output counts

Net reduction: -113 lines
Rebuild the orchard bundle via orchard::Bundle::from_parts +
TransactionData::from_parts (both already public) instead of byte-level
manipulation. Adds a cfg-gated with_orchard_bundle helper on Transaction
and a nonempty dev-dependency for constructing the new action list.
@arya2 arya2 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Apr 17, 2026
@oxarbitrage
Copy link
Copy Markdown
Contributor

Arya has thoroughly reviewed the large changes and is confident this works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-cleanup Category: This is a cleanup C-tech-debt Category: Code maintainability issues P-Medium ⚡ S-blocked Status: Blocked on other tasks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants