bug: Re-enforce the parse-time consensus checks dropped by the zcash_primitives transaction refactor (#10461)

[mpguerra]

Summary

The zcash_primitives transaction newtype refactor (PR #10461, merged 2026-08-22) moved production transaction parsing to zcash_primitives/zcash_transparent, which do not enforce several of Zebra's parse-time consensus checks. The refactor re-added most of those checks explicitly in Transaction::zcash_deserialize, but two were missed, so on main two consensus rules are no longer enforced on the path that released nodes actually run:

1. Coinbase scriptSig length must be in {2 .. 100} bytes (protocol spec 7.1.2).
2. Non-coinbase nExpiryHeight must be less than or equal to 499,999,999 (ZIP-203, spec 7.1.2).

Both are the same shape: a check that lived in Zebra's own enum-based deserializer is not reproduced on the zcash_primitives path, so main now accepts transactions the spec requires rejecting. Both were reported by @ouicate.

No released version is affected. v6.0.0 through v6.3.0 parse inputs through Zebra's own deserializer and enforce both bounds. This is a release blocker for the next release cut from main.

Details

Verified on main and on tags v6.3.0 and v6.0.0 (fetched 2026-09-02).

On main, production parsing goes through zp_tx::Transaction::read (zebra-chain/src/transaction.rs:823, 872, 876). The refactor re-added three parse-time checks after that call, each with a comment noting that zcash_primitives does not enforce it: coinbase height encoding (transaction.rs:884-891, via parse_coinbase_height), coinbase-must-not-have-Sapling-spends (transaction.rs:893-913, citing GHSA-rgwx-8r98-p34c), and the V4 empty-Sapling valueBalance rule (transaction.rs:915-931). The two rules below were not re-added.

1. Coinbase scriptSig length {2 .. 100}

parse_coinbase_height (zebra-chain/src/transparent/serialize.rs, the function around lines 55-97) validates only the canonical height prefix and returns the remaining bytes as inert data. It applies no total-length bound. So a coinbase input whose scriptSig begins with a canonical height push but runs past 100 bytes (or is a single OP_N byte, length 1) parses successfully.

The {2 .. 100} check still exists, but only inside Input::zcash_deserialize (transparent/serialize.rs:168, bound at :194-201, constants imported at :7 from zcash_transparent::coinbase). On main the transaction serializer no longer calls Vec<transparent::Input>::zcash_deserialize, so that check and its regression tests have no production callers. Nothing in zebra-consensus re-checks the length. Upstream zcash_transparent-0.10.0 enforces the bound only in the TxIn::coinbase builder, not in TxIn::read.

On v6.3.0 (and back to v6.0.0), inputs are parsed via Vec<transparent::Input> = Vec::zcash_deserialize(...) (transaction/serialize.rs:985, 1108, 1167), which runs the {2 .. 100} check per input, so releases are unaffected.

2. Non-coinbase nExpiryHeight <= 499,999,999

On main, Transaction is a newtype over zcash_primitives (transaction.rs:53), and Transaction::expiry_height() converts the inner value through compat::block_height_to_height(bh).ok() (transaction/compat.rs:160-165). That conversion is block::Height::try_from(bh), which errors for any value above Height::MAX = u32::MAX / 2 = 2_147_483_647 (block/height.rs:67, 133-149); .ok() then maps the error to None. Because validate_expiry_height_max skips None (zebra-consensus/src/transaction/check.rs, fn validate_expiry_height_max), a raw nExpiryHeight in [2^31, 2^32-1] is treated as "no expiry" and the 499,999,999 bound is not applied. The coinbase path is unaffected (it requires expiry_height == Some(block_height), so None is rejected).

On v6.3.0, Transaction is an enum storing expiry_height: block::Height verbatim (transaction.rs:76 onward), the deserializer tuple-constructs block::Height(read_u32()) preserving the raw value (transaction/serialize.rs:956, 997, 1105, 1164), the accessor returns Some(block::Height(raw)) (transaction.rs:510), and validate_expiry_height_max rejects the over-range value with MaximumExpiryHeight (transaction/check.rs:571-589). Releases are unaffected.

Conformance reference

The pre-refactor architecture, which released Zebra still uses, rejects both classes of transaction at parse time, matching the protocol spec. The independent Zebra-derived implementation Zakura (v1.3.1), which did not adopt the newtype refactor, also enforces both bounds. Restoring these checks brings main back into parity with the spec and with released Zebra.

Suggested fix

Re-add both checks on the production parse path, alongside the checks the refactor already reinstated in Transaction::zcash_deserialize.

1. Coinbase length: enforce {2 .. 100} on the production path. The cleanest single site is parse_coinbase_height: check the total script_sig length against MIN_COINBASE_SCRIPT_LEN/MAX_COINBASE_SCRIPT_LEN (already imported) before parsing the height. That covers both production callers (transaction.rs:888 and the txin_to_input conversion in compat.rs:39) in one place. If instead the check is added at the transaction.rs:884-891 block, mirror it in compat.rs so both callers are covered.

2. Expiry max: enforce the bound on the raw u32, independent of the Height-range conversion, so an over-range value is rejected rather than collapsed to None. Either compare the inner expiry as a u32 against 499,999,999 in validate_expiry_height_max/non_coinbase_expiry_height, or reject at parse time in Transaction::zcash_deserialize next to the coinbase-height re-check. Keep 0 meaning "no expiry"; only values above 499,999,999 are rejected.

Audit result (the reason both defects exist)

These two are instances of one pattern: the refactor manually re-added parse-time consensus checks but missed some. The full inventory of parse-time consensus rejections in v6.3.0's transaction, transparent, Sapling, Orchard, Sprout, and lock-time deserializers has been traced against main, the pinned upstream crates (zcash_primitives 0.30.0, zcash_transparent 0.10.0, orchard 0.15.3, sapling-crypto 0.7.0, reddsa 0.5.1), and main's consensus-layer checks. See the appendix. Result: the two rules above are the complete set of dropped parse-time consensus rules. Every other rule is either re-added on main, enforced by upstream at parse, or enforced by a later verification/consensus step (rejected regardless, at a different layer).

Still required in the fix: audit accessors that convert an inner zcash_primitives field through a fallible compat:: conversion with .ok() or a default. Any that turns an out-of-range wire value into None/default can hide a consensus-relevant value the way expiry_height() does. A parse-rejection inventory does not cover this class; the expiry defect is the found instance.

Scope guardrails

• Parse-time consensus enforcement only. Do not change consensus validation semantics elsewhere; the rules are the spec's {2 .. 100} and <= 499,999,999, nothing stricter.
• Do not conflate "no expiry" (nExpiryHeight == 0, legal) with over-range. Only 0 maps to "no expiry".
• Preserve the genesis coinbase special-case. compat.rs special-cases GENESIS_COINBASE_SCRIPT_SIG (77 bytes, within bounds); the transaction.rs:884-891 block does not, and only works on the genesis coinbase because its bytes happen to parse as a valid height push. Confirm the genesis block still parses after the change, and consider aligning the two sites.

Regression tests

Add tests that exercise the production path (through Transaction::zcash_deserialize or block deserialization), not only Input::zcash_deserialize:

• Coinbase length: reject an oversized coinbase (canonical height push padded past 100 bytes) and an undersized coinbase (single-byte OP_N); accept the boundary lengths 2 and 100; confirm the genesis coinbase still parses.
• Expiry max: reject nExpiryHeight = 0xFFFFFFFF for non-coinbase V4, V5, and V6; reject 500,000,000; accept 499,999,999 and 0; confirm coinbase acceptance is unchanged.

Also add end-to-end rejection tests for the rules that moved from parse to a later layer (see appendix), so that enforcement is pinned by a test rather than by the current upstream implementation and does not silently move again on a dependency bump:

• A V5 transaction with a pre-NU5 branch ID and a V6 transaction with a pre-NU6.3 branch ID are rejected by the verifier (WrongConsensusBranchId).
• An input with a null prevout hash and index other than 0xFFFFFFFF is rejected (as the first transaction of a block, and as a later transaction).
• A Sapling spend whose rk is a small-order point, and a Sapling output whose epk is a small-order point, are rejected by the verifier.

Affected version

No released version. main only, since PR #10461 (merged 2026-08-22). Release blocker for the next release cut from main.

Related

• Add per-field size bounds to Halo2Proof and transparent::Script deserializers #10554: per-field allocation bounds on Script. Related surface (the coinbase script bytes are allocated by upstream Script::read before the length can be checked, bounded only by the outer MAX_BLOCK_BYTES reader cap), but a different observable. This issue restores the consensus length bound only; the pre-allocation bound is tracked there.

Credit

Both findings identified and responsibly disclosed by @ouicate. We appreciate your contribution in identifying and mitigating what would have been a nasty consensus bug in Zebra before we shipped it.

Appendix: parse-time consensus check inventory (v6.3.0 vs main)

Every consensus rejection performed by Zebra's own deserializers on v6.3.0, and where the same rule is enforced on main. Verified against source on 2026-09-02.

Rule (enforced at parse on v6.3.0)	v6.3.0 site	Enforcement on main
Coinbase scriptSig length in {2 .. 100}	transparent/serialize.rs:194-201	Not enforced. Restored by this issue.
Non-coinbase nExpiryHeight <= 499,999,999	value preserved by enum, checked in check.rs:571-589	Not enforced (over-range collapsed to None). Restored by this issue.
Coinbase height encoding is canonical	transparent/serialize.rs parse_coinbase_height	Re-added: transaction.rs:884-891
Coinbase MUST NOT have Sapling spends (V4, V5)	transaction/serialize.rs:215, 1013	Re-added: transaction.rs:893-913
V4 with no spends/outputs: valueBalanceSapling = 0	transaction/serialize.rs:1051	Re-added: transaction.rs:915-931
nVersionGroupId matches version (V3, V4, V5, V6)	transaction/serialize.rs:941, 968, 1082, 1146	Upstream zcash_primitives rejects at parse
nConsensusBranchId is a known branch ID	transaction/serialize.rs:1088, 1150	Upstream BranchId::try_from rejects at parse
Unknown version / header combination	transaction/serialize.rs:1220	Upstream rejects at parse
Canonical jubjub::Fq, pallas::Scalar, pallas::Base	transaction/serialize.rs:36, 50, 64	Upstream field parsers reject non-canonical encodings
Orchard flags: reserved bits zero; V6 enableCrossAddress permitted	orchard/shielded_data.rs:338	Upstream, version-aware: orchard 0.15.3 Flags::from_byte(byte, bundle_version), FLAG_V6_CROSS_ADDRESS_ENABLED; zcash_primitives read_flags with ironwood_v3
Orchard action rk is not the identity	orchard/action.rs:86	Upstream orchard::Action::from_parts rejects
Sapling cv is not small order	Zebra commitment type	Upstream ValueCommitment::from_bytes_not_small_order at parse (zcash_primitives sapling.rs:93)
V5 requires NU5+ branch ID; V6 requires NU6.3+ branch ID	transaction/serialize.rs:1096, 1156	Layer shift: check::consensus_branch_id (check.rs:885-901) in check_structure_and_network_rules, reached from both the block and mempool verifiers
Coinbase input index must be 0xFFFFFFFF	transparent/serialize.rs:177	Layer shift: a null-hash input with another index is a non-coinbase input on main (prevout != OutPoint::NULL, matching zcashd), rejected by the coinbase-position rule or the missing-UTXO check
Sapling spend rk is not small order	sapling/spend.rs:217-220 via ValidatingKey::try_from (sapling/keys.rs)	Layer shift: sapling-crypto check_spend (verifier.rs:48-49), run by BatchValidator::check_bundle, which main routes through at zebra-consensus/src/transaction.rs:1339
Sapling output epk is not small order	Zebra output parser	Layer shift: sapling-crypto check_output (verifier.rs:108)

The Sprout and lock-time deserializers on v6.3.0 contain no parse-time consensus rejections.

Rows marked "layer shift" are rejected on main with the same outcome, at verification rather than at parse. Rejection therefore happens after the transaction has been allocated and queued rather than during read; the pre-allocation bounds for that are tracked at #10554 and are out of scope here. The regression tests above pin these rules at their new layer.

[mpguerra]

See #11261 (comment) also, we might want to fix at the same time at this and/or create a new issue to track this

[ouicate]

Thanks @mpguerra for the writeup and the credit.

To put the stakes on the record: both findings are consensus-divergence regressions — the highest-severity class for a validator, where Zebra accepts a transaction or block that the Zcash spec, released Zebra, and every other conforming implementation (e.g. Zakura) reject. On a shipped release the consequence is a chain split: Zebra nodes accept and build on a chain the rest of the network rejects, partitioning the two node populations onto different histories, with the reorg and double-spend exposure that follows. This is the same class as Zebra's earlier Critical consensus-divergence advisories.

The two rules that #10461 dropped:

1. Coinbase scriptSig length {2..100} — main accepts a coinbase whose scriptSig is a canonical BIP-34 height push padded past 100 bytes; the spec and released Zebra reject it. Block-level divergence: a single such block splits Zebra from the rest of the network.
2. Non-coinbase nExpiryHeight ≤ 499,999,999 — main accepts V4/V5/V6 transactions with a raw nExpiryHeight in [2³¹, 2³²−1] that the spec and released Zebra reject. Unauthenticated and craftable by anyone at the transaction layer, and network-splitting once mined into a block.

Now the part I actually care about: no released version is affected — this lived only on main, post-#10461, and I found it while the refactor was still unreleased. I could have waited for it to ship and then reported both as live, in-the-wild Criticals, with CVEs and published advisories — the same paper trail the earlier consensus-divergence findings carry. I chose the opposite: report them now and get two would-be Critical consensus splits eliminated before they touched a single production node. Catching a critical before it ships is a better outcome for the network than documenting one after it does — and that's the behavior I'd want the ecosystem to encourage.

Thanks again for the fast triage and the clear fix/test plan.

[alchemydc]

The accessor audit called for in the issue body — "audit accessors that convert an inner zcash_primitives field through a fallible compat:: conversion with .ok() or a default" — now has a home in #11386, along with the checklist of the remaining Option-returning accessors on the newtype.

#11386 is the second found instance of that class. #10461 also changed Transaction::sprout_value_balance() from a fallible checked sum into an infallible accessor that silently returns zero when the aggregate Sprout JoinSplit balance leaves [-MAX_MONEY, MAX_MONEY]. As with the two defects here, no released version is affected. Unlike them, it is not exploitable: ZIP-211 and the mandatory-checkpoint floor block one direction, and Groth16 plus anchor soundness block the other. It is a lost invariant and a rule-level divergence from zcashd rather than a live divergence, so it is a correctness fix rather than a release blocker.

This issue's parse-time inventory is unaffected by that finding. The Sprout aggregate bound was never a parse-time check — on v6.3.0 it lived in sprout_value_balance()'s sum(), at the value-balance layer — so the conclusion that the coinbase scriptSig length and non-coinbase nExpiryHeight rules are the complete set of dropped parse-time consensus rules still holds, as does the appendix note that the Sprout deserializer contains no parse-time consensus rejections.