Skip to content

feat(wasm-utxo): add v6 (Ironwood) transaction codec + ZIP-244 txid#336

Merged
veetragjain merged 1 commit into
masterfrom
zcash-v6-codec
Jul 23, 2026
Merged

feat(wasm-utxo): add v6 (Ironwood) transaction codec + ZIP-244 txid#336
veetragjain merged 1 commit into
masterfrom
zcash-v6-codec

Conversation

@veetragjain

Copy link
Copy Markdown
Contributor

PR3 of 3 (stacked on the unified-address PR; shares the zec-namespaced WASM module). Standalone v6/NU6.3 wire codec — the v6 header is reordered vs v4 (consensusBranchId/lockTime/expiryHeight move to the front) so it is a separate codec and the v4 path is untouched (its decoder now guards against v6 bytes).

  • IronwoodAction/IronwoodBundle/ZcashV6Transaction types, decode/encode with byte-identical round trips.
  • ZIP-244 ironwood_digest (three-way split) + five-component txid.
  • ZcashV6Transaction WASM+TS wrapper exposing the txid as an instance method getId() returning the canonical display-order id — addresses review feedback (no static-over-bytes, no internal/display byte-order footgun) and matches the getId() convention the build endpoint's responseBuilder already calls.

Review fix: the untrusted nActionsIronwood count is validated against the remaining input length before Vec::with_capacity, so a malformed count cannot trigger an unbounded pre-allocation (OOM/abort). Regression test included.

Validated against three real testnet transactions (shield / self-send / unshield) in test/fixtures/zcash/: each parses, re-encodes byte-identically, and reproduces its canonical txid. Vendors the v6 wire/ZIP-244 reference and the build-endpoint integration plan into docs/zcash-v6-transaction.md.

@OttoAllmendinger OttoAllmendinger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are importing zcash crates as a dev dependency, so we have an opportunity to use it for testing

Comment thread packages/wasm-utxo/src/wasm/zcash.rs Outdated
Comment on lines +93 to +102
/// Lowercase hex encoding (the `hex` crate is only a dev/`inspect` dependency).
fn to_hex(bytes: &[u8]) -> String {
const HEX: &[u8; 16] = b"0123456789abcdef";
let mut s = String::with_capacity(bytes.len() * 2);
for b in bytes {
s.push(HEX[(b >> 4) as usize] as char);
s.push(HEX[(b & 0x0f) as usize] as char);
}
s
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a more conventional way in this codebase

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To expand: the hand-rolled to_hex (plus the separate txid_display() byte-reversal) reinvents what rust-bitcoin already gives us, and the doc-comment rationale ("the hex crate is only a dev/inspect dependency") doesn't hold — miniscript is a non-optional dependency and re-exports hex (hex-conservative) as miniscript::bitcoin::hex, so the conventional encoders are available in the default build without adding anything.

Two conventional options, in increasing order of "more idiomatic here":

  1. Minimal — use DisplayHex, which we already use elsewhere (src/fixed_script_wallet/bitgo_psbt/p2tr_musig2_input.rs:1127-1128):

    use miniscript::bitcoin::hex::DisplayHex;
    // ...
    self.inner.txid_display().to_lower_hex_string()

    Drops the hand-rolled loop.

  2. Better — wrap the 32-byte digest in the Txid type the rest of the codebase already uses (Txid appears all over bitgo_psbt/mod.rs) and let its Display do the work:

    use miniscript::bitcoin::Txid;
    // ...
    Txid::from_byte_array(self.inner.txid()).to_string()

    rust-bitcoin's hash Display emits display-order (byte-reversed) hex, so this eliminates both to_hex and the manual txid_display() reversal — txid() (internal order) is all the core needs to expose, and the wrapper gets the canonical id for free. It also makes getId() return exactly what a Txid prints everywhere else, so there's one txid-string notion in the codebase instead of a Zcash-specific one.

Either way to_hex goes away; option 2 also lets txid_display() go away.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted the hand-rolled to_hex and the core txid_display()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getId() now uses miniscript::bitcoin::Txid::from_byte_array(self.inner.txid()).to_string()

OttoAllmendinger commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

edit: superseded

@veetragjain
veetragjain force-pushed the zcash-unified-address branch from 4977119 to 17b4ec7 Compare July 23, 2026 11:48
Base automatically changed from zcash-unified-address to master July 23, 2026 12:05

Copy link
Copy Markdown
Contributor

Correction to my previous comment — I was wrong that "NU6.3 isn't upstream" and that zebra can't cover the Ironwood bundle. It is upstream now, so bumping the dep for real Ironwood coverage (as suggested) is the right call and gets us much more than the transparent-only cross-check I described:

  • Zebra 6.0.0 activates the NU6.3 "Ironwood" shielded pool and the v6 transaction format (testnet already; mainnet at height 3,428,143, ~2026-07-28).
  • zebra-chain at the v6.0.0-rc.0 tag has a Transaction::V6 variant with an ironwood_shielded_data field ("The Ironwood bundle reuses the v6 Orchard bundle shape but commits into a separate note commitment tree and nullifier set") — i.e. a full independent implementation of exactly what this PR codes.
  • Latest published zebra-chain is 11.2.0 (2026-07-17); the 11.0/11.1/11.2 line all landed in the NU6.3 window.

Version mechanics: we already declare zebra-chain = "11" (^11), so this is likely just cargo update -p zebra-chain to pick up 11.2.0 — check whether Cargo.lock is pinned to an older 11.x that predates the V6 variant, and bump/pin = "11.2" if you want it explicit. It drags in the pre-release zcash_*/orchard cohort (orchard 0.15.0-pre.1, zcash_primitives 0.29.0-pre.0), but since it's a cfg(not(wasm32)) dev-dependency only, the pre-release churn stays out of the shipped artifact.

What the tests should then do (supersedes the transparent-only plan above): decode each of the three testnet fixtures with zebra_chain::transaction::Transaction, and assert our decoding and our ZIP-244 txid match zebra's — including ironwood_shielded_data (actions, value balance, anchor, flags) and transaction.hash(). That turns the txid from a value we generated into one an independent NU6.3 implementation agrees on, which is the whole point of having the codec. The vendored docs/zcash-v6-transaction.md reference stays useful for humans, but the oracle becomes zebra rather than our own fixtures.

Sources: Zebra 6.0.0 release, Zebra 6.0.0-rc.0, zebra-chain on crates.io.


Generated by Claude Code

@OttoAllmendinger OttoAllmendinger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two follow-ups: expanded the hex/to_hex and zebra-testing threads (see inline reply + PR comment), and flagged the js_err error-signaling issue carried over from #331 (inline) — it's worth fixing here since this file spreads the bare-string pattern to the new v6 wrapper.


Generated by Claude Code

Comment thread packages/wasm-utxo/src/wasm/zcash.rs Outdated
Comment on lines +9 to +11
fn js_err(e: String) -> JsValue {
JsValue::from_str(&e)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same error-signaling issue as on #331, and it's more pervasive here since every method in both wrappers (ZcashUnifiedAddress::{parse,contains}, ZcashV6Transaction::{from_bytes,to_bytes}) routes through js_err.

js_errJsValue::from_str throws a bare JS string. The crate convention is to return Result<T, WasmUtxoError> and let impl From<WasmUtxoError> for JsValue (src/wasm/try_into_js_value.rs) produce a real js_sys::Error carrying .message, a structured .code, and the Symbol.for("@bitgo/wasm-utxo/error") marker. As-is, a JS caller catching a v6 decode failure gets a string with no .code, not an Error — inconsistent with every other wasm-utxo surface.

Fix: delete js_err, make these methods -> Result<_, WasmUtxoError>. WasmUtxoError: From<String> already exists (src/error.rs), so .map_err(js_err) collapses to ?. Ideally the two cores (zcash::unified_address and zcash::v6) grow typed error enums (#[derive(strum::IntoStaticStr)] + impl_wasm_error_code!, like ParseTransactionError) so .code is meaningful (ZcashV6Error.TruncatedActions, etc.); minimum is funnelling the existing String through WasmUtxoError at the boundary. Predates this PR (moved from the old zcash_branch_id_for_height), but this is the file that spreads it to the new v6 surface.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added typed ZcashV6Error enum

PR3 of 3 (stacked on the unified-address PR; shares the zec-namespaced WASM
module). Standalone v6/NU6.3 wire codec — the v6 header is reordered vs v4
(consensusBranchId/lockTime/expiryHeight move to the front) so it is a separate
codec and the v4 path is untouched (its decoder now guards against v6 bytes).

- IronwoodAction/IronwoodBundle/ZcashV6Transaction types, decode/encode with
  byte-identical round trips.
- ZIP-244 ironwood_digest (three-way split) + five-component txid.
- ZcashV6Transaction WASM+TS wrapper exposing the txid as an instance method
  getId() returning the canonical display-order id — addresses review feedback
  (no static-over-bytes, no internal/display byte-order footgun) and matches the
  getId() convention the build endpoint's responseBuilder already calls.

Review fix: the untrusted nActionsIronwood count is validated against the
remaining input length before Vec::with_capacity, so a malformed count cannot
trigger an unbounded pre-allocation (OOM/abort). Regression test included.

Validated against three real testnet transactions (shield / self-send / unshield)
in test/fixtures/zcash/: each parses, re-encodes byte-identically, and reproduces
its canonical txid. Vendors the v6 wire/ZIP-244 reference and the build-endpoint
integration plan into docs/zcash-v6-transaction.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@veetragjain

Copy link
Copy Markdown
Contributor Author

What the tests should then do (supersedes the transparent-only plan above): decode each of the three testnet fixtures with zebra_chain::transaction::Transaction, and assert our decoding and our ZIP-244 txid match zebra's — including ironwood_shielded_data (actions, value balance, anchor, flags) and transaction.hash(). That turns the txid from a value we generated into one an independent NU6.3 implementation agrees on, which is the whole point of having the codec. The vendored docs/zcash-v6-transaction.md reference stays useful for humans, but the oracle becomes zebra rather than our own fixtures.

Bumped zebra-chain dev-dep. New zebra_oracle test module: decodes each of the three golden fixtures with zebra_chain::Transaction, and asserts zebra's hash() (ZIP-244 txid) == ours == the fixture, plus matching transparent I/O and Ironwood action counts.

@veetragjain
veetragjain marked this pull request as ready for review July 23, 2026 13:03
@veetragjain
veetragjain requested a review from a team as a code owner July 23, 2026 13:03

@OttoAllmendinger OttoAllmendinger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at b081ef7. All prior feedback is addressed, and the correctness story is now much stronger than fixtures alone. LGTM 🎉

Prior points, all resolved:

  • Error signalingjs_err/bare-string is gone. zcash.rs returns Result<_, WasmUtxoError> throughout, and WasmUtxoError grew real typed variants (UnifiedAddress(UnifiedAddressError), ZcashV6(ZcashV6Error)) whose code() delegates to the inner enum. So a JS caller catching a v6 decode failure gets a marked js_sys::Error with .code === "ZcashV6Error.TruncatedActions" etc. — exactly the "ideal" typed-code path, not just the string-funnel minimum. Nice.
  • Hex/to_hex — replaced with Txid::from_byte_array(self.inner.txid()).to_string(). Hand-rolled encoder and the separate display-order reversal both gone; getId() now prints the same way a Txid does everywhere else.
  • Independent oraclezebra-chain = "11.2" (with a precise comment on why 11.2 is the floor for Transaction::V6), and the zebra_oracle test module asserts zebra.hash() == our display txid plus structural counts on all three fixtures. CI (test / Test wasm-utxo) is green, so this genuinely compiles and agrees against an external NU6.3 implementation — the txid is no longer self-referential.

Codec correctness (fresh pass on v6.rs): header/transparent/sapling/orchard/ironwood five-component tree matches ZIP-244; the transparent txid digest correctly commits only prevouts/sequences/outputs (no scriptSigs) and collapses to the empty personalized hash when there's no transparent I/O; the Ironwood three-way split (compact/memos/noncompact) excludes anchor/proof/sigs, which the digest_excludes_* tests pin. Untrusted-count handling is sound — actions are length-checked before with_capacity, and proof/sigs go through take_bytes (slice-before-alloc), so no unbounded preallocation anywhere. The v4 decoder's ZCASH_IRONWOOD_VERSION_GROUP_ID guard cleanly prevents mis-parsing v6 bytes on the old path.

Two tiny non-blocking nits:

  1. ZcashV6Transaction::sapling_value_balance is effectively vestigial — decode always sets it to 0, encode rejects non-zero, and compute_v6_txid always hashes the empty Sapling digest regardless of the field. A hand-built tx with a non-zero value there would yield a txid that corresponds to no encodable transaction. Consider dropping the field, or a debug_assert!(self.sapling_value_balance == 0) in compute_v6_txid, to close the footgun.
  2. Cosmetic: flags = *take_bytes(&mut r, 1)?.first().unwrap() reads more naturally as take_array::<1>(&mut r)?[0], matching the other fixed-width reads.

Neither blocks. Great iteration on the error taxonomy and the zebra cross-check.


Generated by Claude Code

@veetragjain
veetragjain merged commit 82232d9 into master Jul 23, 2026
13 checks passed
@veetragjain
veetragjain deleted the zcash-v6-codec branch July 23, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants