Distinguish value pool, protocol version, and bundle version by nuttycom · Pull Request #519 · zcash/orchard

nuttycom · 2026-06-29T01:21:49Z

Stacked on feat/ironwood.

Replaces BundlePoolRestrictions with BundleVersion — the (ValuePool, ProtocolVersion) of an Orchard bundle — and makes each Bundle carry its version as non-serialized context, so bundles are encodable and committable by construction.

Summary

New top-level ValuePool and ProtocolVersion; BundleVersion with safe-by-construction const fn constructors (orchard_insecure_v0 / orchard_v1 / orchard_v2 / ironwood_v2).
Bundle carries its BundleVersion; construction (from_parts / try_from_parts, builder, PCZT) validates flags against it (BundleError::UnrepresentableFlags); from_parts is now fallible.
commitment / authorizing_commitment and the decrypt_* / recover_* helpers drop their version argument; new flag_byte() / bundle_version() accessors.
CommitmentError::UnrepresentableFlags removed; proof-size enforcement derived from the version, ProofSizeEnforcement removed.
CHANGELOG [Unreleased] updated to the final API.

Tests: 138 lib + 8 integration + 1 doctest, all passing.

Opened as a draft for review. Filed with Claude Code assistance.

🤖 Generated with Claude Code

ebfull

Comments from an LLM review of this draft (044eec1).

codecov-commenter · 2026-06-29T07:51:31Z

Codecov Report

❌ Patch coverage is 79.85612% with 28 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (feat/ironwood@0715766). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/bundle.rs	81.81%	14 Missing ⚠️
src/builder.rs	78.37%	8 Missing ⚠️
src/pczt.rs	0.00%	3 Missing ⚠️
src/pczt/tx_extractor.rs	25.00%	3 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##             feat/ironwood     #519   +/-   ##
================================================
  Coverage                 ?   76.42%           
================================================
  Files                    ?       24           
  Lines                    ?     2897           
  Branches                 ?        0           
================================================
  Hits                     ?     2214           
  Misses                   ?      683           
  Partials                 ?        0

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

nuttycom · 2026-06-29T17:56:57Z

force-pushed to address comments from code review.

Copilot

Pull request overview

This PR refactors Orchard bundle versioning by separating value pool vs protocol version and making BundleVersion explicit and carried inside bundles (non-serialized), so encoding/commitment/decryption use version-correct behavior by construction.

Changes:

Introduces ValuePool, ProtocolVersion, and BundleVersion, and threads BundleVersion through bundle construction, parsing, commitments, and PCZT handling.
Updates bundle APIs to derive flag encoding and commitment/decryption domains from the bundle’s own BundleVersion, removing version parameters from several helpers.
Updates builder API to accept BundleVersion + caller-supplied Flags (validated), and updates tests/benches/changelog accordingly.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/builder.rs	Updates integration tests to use `BundleVersion` + new builder/commitment/decryption APIs.
src/pczt/verify.rs	Updates cross-address restriction error wording after version refactor.
src/pczt/tx_extractor.rs	Carries `bundle_version` through extraction and maps `UnrepresentableFlags` errors.
src/pczt/prover.rs	Updates prover docs/error wording to match new “bundle disables cross-address transfers” framing.
src/pczt/parse.rs	Parses PCZT bundles using `BundleVersion` for flag decoding and note-version checks.
src/pczt.rs	Stores `bundle_version` in PCZT bundle and adds infallible `flag_byte()` accessor.
src/note_encryption.rs	Replaces pool-restriction-based domain policy with note-version-only policy.
src/lib.rs	Adds top-level `ValuePool` and `ProtocolVersion` enums.
src/circuit.rs	Updates circuit tests/fixtures to use `BundleVersion` when decoding flags.
src/bundle/commitments.rs	Derives commitment format from `bundle.bundle_version().value_pool()`; uses `bundle.flag_byte()`.
src/bundle.rs	Introduces `BundleVersion`, validates flags at construction, and removes commitment-time flag representability errors.
src/builder.rs	Makes `Builder::new` fallible, validates `(BundleVersion, Flags)`, and decouples `BundleType` from embedded flags.
CHANGELOG.md	Updates release notes for `BundleVersion`-based API and adjusted method signatures.
benches/note_decryption.rs	Updates benchmarks to construct builders with `BundleVersion` + `default_flags()`.
benches/circuit.rs	Updates benchmarks to construct builders with `BundleVersion` + `default_flags()`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Replace `BundlePoolRestrictions` with `BundleVersion`, the `(ValuePool, ProtocolVersion)` of an Orchard bundle, built from the new top-level `ValuePool` and `ProtocolVersion` types via safe-by-construction `const fn` constructors (`orchard_insecure_v0`, `orchard_v1`, `orchard_v2`, `ironwood_v2`). Each `Bundle` now carries its `BundleVersion` as non-serialized context, so a bundle is encodable and committable by construction: - Construction (`Bundle::from_parts` / `try_from_parts`, the builder, and PCZT parsing/extraction) takes a `BundleVersion` and validates that the flags are representable under it, rejecting inconsistent combinations with the new `BundleError::UnrepresentableFlags`. `from_parts` is now fallible. - `commitment` / `authorizing_commitment` and the `decrypt_*` / `recover_*` helpers no longer take a version argument; they read it from the bundle. `Bundle::flag_byte` exposes the now-infallible flag-byte encoding and `Bundle::bundle_version` the carried version. - `CommitmentError::UnrepresentableFlags` is removed (flags are validated at construction); only `InvalidTransactionVersion` remains. Proof-size enforcement is derived from the bundle version rather than a separate `ProofSizeEnforcement` argument: it is enforced for every version except the historical pre-NU6.2 Orchard pool (`orchard_insecure_v0`), whose already-committed transactions may carry non-canonical proofs. `ProofSizeEnforcement` is removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

`BundleType` previously embedded part of a bundle's `Flags` (`Transactional` carried `spends_enabled`/`outputs_enabled`, and `BundleType::flags` derived the cross-address bit from the bundle version). This conflated two independent concerns: the construction discipline (how the builder pads and pairs actions) and the bundle's flag set (which spend/output/cross-address capabilities it advertises). It also meant the caller could not restrict cross-address transfers more tightly than the bundle version's default chose to. Make the two orthogonal. `BundleType` is now just the construction policy — `Transactional { bundle_required }` or `Coinbase` — and the bundle's `Flags` are supplied separately to the builder. The default flag set moves to `BundleVersion::default_flags` (spends and outputs enabled, cross-address transfers enabled unless the version mandates the restriction); a caller may pass a more restricted set the version permits. Validate the flags when the builder is constructed: `Builder::new` (and the free `bundle` function) now take `Flags` and are fallible, rejecting a flag set that cannot be encoded under the bundle version with `BuildError::UnrepresentableFlags`, and rejecting a `Coinbase` builder whose flags enable spends with `BuildError::CoinbaseSpendsEnabled`. Because the flags are validated up front, `build_bundle` can assume they are encodable (a `debug_assert!` documents the invariant). `BundleType::num_actions` now reads the spend/output/cross-address policy from the supplied `Flags` rather than re-deriving it from the bundle version. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The `try_from_parts` doc linked to the `pub(crate)` `BundleVersion::enforces_canonical_proof_size`, which trips `rustdoc::private_intra_doc_links` (an error under `-D warnings`). Point at the public `BundleVersion::orchard_insecure_v0` constructor instead: it builds the sole bundle version whose proof size is not enforced, so it is the more useful public reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Rename the `ProtocolVersion` variants and `BundleVersion` constructors from 0-based to 1-based version numbers, at the protocol developers' request: ProtocolVersion::InsecureV0 -> InsecureV1 ProtocolVersion::V1 -> V2 ProtocolVersion::V2 -> V3 BundleVersion::orchard_insecure_v0 -> orchard_insecure_v1 BundleVersion::orchard_v1 -> orchard_v2 BundleVersion::orchard_v2 -> orchard_v3 BundleVersion::ironwood_v2 -> ironwood_v3 This is a pure rename with no change in behavior; each version keeps the same mapping to consensus epochs, circuit versions, and note plaintext versions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

ebfull

Should do a couple fixes including fixing doc links, but ACK otherwise.

ebfull · 2026-06-29T22:46:19Z

    pub fn new(
-        pool_restrictions: BundlePoolRestrictions,
        bundle_type: BundleType,
+        bundle_version: BundleVersion,
+        flags: Flags,
        anchor: Anchor,
-    ) -> Self {


The ordering of args here is different from fn bundle below, not sure which ordering you wanted to do, but they should both be consistent.

This is the correct order, we'll fix the fn bundle below.

Reorder the free `bundle` function to take `bundle_type` immediately after `rng`, matching `Builder::new`'s `(bundle_type, bundle_version, flags, anchor)` order. Also note the removal of `BundleType::DISABLED` in the changelog. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

ebfull · 2026-06-29T23:22:31Z

Merging, the tests will rerun on #516 after the merge and before the next prerelease.

daira · 2026-06-29T23:44:39Z

            ),
+            TxExtractorError::UnrepresentableFlags => write!(
+                f,
+                "Orchard bundle flags are not representable under its value pool and protocol version",


"Orchard" in this error message refers to the Orchard protocol, which could be misleading if the problem was for an Ironwood-pool bundle. But in practice Ironwood will allow all of the flag combinations, so that case won't happen. At some point I will do a full pass for accuracy of terminology, but it doesn't need to block anything.

daira · 2026-06-30T00:11:29Z

            let fvk = FullViewingKey::from(&self.sk);
-            let pool_restrictions = BundlePoolRestrictions::OrchardNu6_2Only;
-            let mut builder = Builder::new(pool_restrictions, BundleType::DEFAULT, self.anchor);
+            let bundle_version = BundleVersion::orchard_v2();


This misses test coverage of the Ironwood pool, or of other Orchard versions.

daira · 2026-06-30T00:17:09Z

+    fn default_flags_match_pool_policy() {
+        // The builder's default flags enable spends and outputs and leave cross-address transfers
+        // as permissive as consensus allows. The bundle version leaves the cross-address choice free
+        // everywhere except Orchard from NU6.3 onward, where it is mandatorily disabled.


I think this is incorrect; the cross-address choice is not free for Orchard pre-NU6.3. Cross-address must be allowed there. So the comment is wrong and the test coverage that would show it to be wrong is missing: we are only testing the default flags here, not whether the choice of flags is free.

daira · 2026-06-30T00:51:38Z

+        match self.value_pool {
+            ValuePool::Orchard => NoteVersion::V2,
+            ValuePool::Ironwood => NoteVersion::V3,
        }


Grump: I dislike note plaintext lead bytes being referred to as "versions"; the specs never do that. "Version" is far too overloaded (and we just added yet another kind of "version" for bundles).

Pretty please can we stop naming things completely differently than the specs? Even just naming the variants Byte0x02 and Byte0x03 would be better. My eyes are bleeding with all these V... that mean different things.

daira · 2026-06-30T01:00:43Z

+        !matches!(
+            (self.protocol_version, self.value_pool),
+            (ProtocolVersion::V3, ValuePool::Orchard)
+        )


Nit: this could be written more clearly and concisely as self != Self::orchard_v3().

daira · 2026-06-30T01:04:03Z

 }

-/// Orchard-specific flags.
+/// Flags denoting what operations may be performed by the Orchard actions


Suggested change

/// Flags denoting what operations may be performed by the Orchard actions

/// Flags denoting what operations may be performed by the Orchard-protocol actions

daira · 2026-06-30T01:06:03Z

+    /// the Orchard value pool.
+    ///
+    /// Uses the historical unsound Orchard circuit. Cross-address transfers are permitted and
+    /// notes use the V2 plaintext format. Used to reconstruct the historical verifying key and to


Suggested change

/// notes use the V2 plaintext format. Used to reconstruct the historical verifying key and to

/// notes use the lead-byte 0x02 plaintext format. Used to reconstruct the historical verifying key and to

daira · 2026-06-30T01:06:19Z

+    /// Orchard value pool.
+    ///
+    /// Uses the post-NU6.2 fixed Orchard circuit. Cross-address transfers are permitted and notes
+    /// use the V2 plaintext format.


Suggested change

/// use the V2 plaintext format.

/// use the lead-byte 0x02 plaintext format.

daira · 2026-06-30T01:06:43Z

+    ///
+    /// For transactional bundles affecting the [`ValuePool::Orchard`] value pool,
+    /// `enableCrossAddress = 0` is required by consensus, so cross-address transfers are
+    /// prohibited and Orchard actions are disallowed in coinbase. Notes use V2 plaintexts.


Suggested change

/// prohibited and Orchard actions are disallowed in coinbase. Notes use V2 plaintexts.

/// prohibited and Orchard actions are disallowed in coinbase. Notes use lead-byte 0x02 plaintexts.

daira · 2026-06-30T01:06:57Z

+    /// prohibited and Orchard actions are disallowed in coinbase. Notes use V2 plaintexts.
+    ///
+    /// For transactional bundles affecting the [`ValuePool::Ironwood`] value pool, cross-address
+    /// transfers are permitted and notes use V3 plaintexts.


Suggested change

/// transfers are permitted and notes use V3 plaintexts.

/// transfers are permitted and notes use lead-byte 0x03 plaintexts.

daira · 2026-06-30T01:09:17Z

 pub type IronwoodDomain = NoteEncryptionDomain<IronwoodVersion>;

-/// Bundle-pool-restricted note encryption logic.
+/// Note encryption logic restricted to a single note plaintext version.


Suggested change

/// Note encryption logic restricted to a single note plaintext version.

/// Note encryption logic restricted to a single note plaintext format.

daira · 2026-06-30T01:10:28Z

-/// decryption succeeds, the revealed note plaintext lead byte selects the note
-/// version and the bundle pool restrictions are enforced against it.
+/// This domain is used by public bundle helpers that are given the bundle's
+/// [`NoteVersion`]. Trial decryption still happens once; after decryption


Suggested change

/// [`NoteVersion`]. Trial decryption still happens once; after decryption

/// note plaintext format ([`NoteVersion`]). Trial decryption still happens once; after decryption

daira · 2026-06-30T01:10:48Z

-/// version and the bundle pool restrictions are enforced against it.
+/// This domain is used by public bundle helpers that are given the bundle's
+/// [`NoteVersion`]. Trial decryption still happens once; after decryption
+/// succeeds, the revealed note plaintext lead byte selects the note version, which is


Suggested change

/// succeeds, the revealed note plaintext lead byte selects the note version, which is

/// succeeds, the revealed note plaintext lead byte selects the note plaintext format, which is

daira · 2026-06-30T01:47:52Z

-    /// The [`NoteVersion`] associated with this bundle pool restriction.
+    /// The [`NoteVersion`] associated with this bundle version.
    ///
    /// Orchard pools use V2 note plaintexts, and Ironwood pools use V3 note


Suggested change

/// Orchard pools use V2 note plaintexts, and Ironwood pools use V3 note

/// Orchard pools use lead-byte 0x02 note plaintexts, and Ironwood pools use lead-byte 0x03 note

daira · 2026-06-30T02:15:14Z

-        // Bit 2 can only be 1 for an Ironwood bundle post-NU6.3 (necessarily a v6+ transaction).
-        // Otherwise it MUST be 0, independent of the tx version:
+        // Bit 2 can only be 1 for an Ironwood bundle. For Orchard it MUST be 0, independent of the
+        // tx version:


Suggested change

// tx version:

// tx version and protocol version:

daira · 2026-06-30T02:23:25Z

        authorization: T,
+        bundle_version: BundleVersion,
    ) -> Self {
+        debug_assert!(flags.to_byte(bundle_version).is_some());


We don't normally use debug_assert!. Why not assert!, and document the panic condition?

daira · 2026-06-30T02:25:11Z

+    pub fn flag_byte(&self) -> u8 {
+        self.flags
+            .to_byte(self.bundle_version)
+            .expect("flags are validated against the bundle version at construction")


This being an .expect reinforces that the check on line 531 should be an assert!.

daira · 2026-06-30T02:31:02Z

+    ///
+    /// The arbitrary-bundle strategies generate flags independently of the version; this pairs them
+    /// into a combination that a `Bundle` can actually be constructed from.
+    fn flags_for_version(bundle_version: BundleVersion, flags: Flags) -> Flags {


This arguably should be arb_flags_for_version and should return flags with enableCrossAddress either set or clear in the IronwoodV3 case. Otherwise we're getting less test coverage for the (perfectly valid) case where enableCrossAddress is clear for an Ironwood-pool bundle.

daira · 2026-06-30T08:16:33Z

    /// Crate-internal: the builder supplies `cross_address_enabled` from its prover-side default
-    /// for the pool restrictions (see [`Builder`](crate::builder::Builder)).
+    /// for the bundle version (see [`Builder`](crate::builder::Builder)).
    pub(crate) const fn from_parts(


Why does this need to be crate-internal? You can already create a Flags with any combination of the defined flags (via Flags::from_byte(b, BundleVersion::IronwoodV3)). This being crate-internal just obscures that fact.

Suggested change

pub(crate) const fn from_parts(

pub const fn from_parts(

daira · 2026-06-30T08:32:38Z

-                Flags::from_byte(value, BundlePoolRestrictions::OrchardNu6_3Onward).unwrap();
-            let ironwood_flags =
-                Flags::from_byte(value, BundlePoolRestrictions::IronwoodNu6_3Onward).unwrap();
+            let pre_nu6_3_flags = Flags::from_byte(value, BundleVersion::orchard_v2()).unwrap();


Nit: rename these as orchard_v2_flags etc.
Coverage: test orchard_insecure_v1 here as well.

daira · 2026-06-30T08:38:50Z

+            };
+
+            // Orchard pre-NU6.3 has cross-address implicitly enabled and Orchard NU6.3 has it
+            // disabled, but both encode to the same wire byte, so their commitments agree.


Suggested change

// disabled, but both encode to the same wire byte, so their commitments agree.

// disabled, but both encode to the same wire byte, so their commitments agree.

// (The top-level txid or auth digest will still vary due to the consensus branch ID.)

daira · 2026-06-30T08:42:19Z

-            // the flags representable in every format under test.
-            let mut flags = *bundle.flags();
-            flags.cross_address_enabled = false;
+            // the flags representable in every version under test.


This is one place arb_flags_for_version would help.

daira · 2026-06-30T08:47:06Z

-                (BundlePoolRestrictions::OrchardNu6_3Onward, TxVersion::V5, true),
-                (BundlePoolRestrictions::OrchardNu6_3Onward, TxVersion::V6, false),
-                (BundlePoolRestrictions::IronwoodNu6_3Onward, TxVersion::V6, false),
+            for (bundle_version, tx, anchor_in_txid_digest) in [


Consider adding BundleVersion::supports_anchor_update().