feat(wasm-utxo): add v6 (Ironwood) transaction codec + ZIP-244 txid#336
Conversation
OttoAllmendinger
left a comment
There was a problem hiding this comment.
we are importing zcash crates as a dev dependency, so we have an opportunity to use it for testing
| /// 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 | ||
| } |
There was a problem hiding this comment.
there is a more conventional way in this codebase
There was a problem hiding this comment.
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":
-
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.
-
Better — wrap the 32-byte digest in the
Txidtype the rest of the codebase already uses (Txidappears all overbitgo_psbt/mod.rs) and let itsDisplaydo the work:use miniscript::bitcoin::Txid; // ... Txid::from_byte_array(self.inner.txid()).to_string()
rust-bitcoin's hash
Displayemits display-order (byte-reversed) hex, so this eliminates bothto_hexand the manualtxid_display()reversal —txid()(internal order) is all the core needs to expose, and the wrapper gets the canonical id for free. It also makesgetId()return exactly what aTxidprints 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
There was a problem hiding this comment.
Deleted the hand-rolled to_hex and the core txid_display()
There was a problem hiding this comment.
getId() now uses miniscript::bitcoin::Txid::from_byte_array(self.inner.txid()).to_string()
|
edit: superseded |
4977119 to
17b4ec7
Compare
|
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:
Version mechanics: we already declare What the tests should then do (supersedes the transparent-only plan above): decode each of the three testnet fixtures with Sources: Zebra 6.0.0 release, Zebra 6.0.0-rc.0, zebra-chain on crates.io. Generated by Claude Code |
OttoAllmendinger
left a comment
There was a problem hiding this comment.
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
| fn js_err(e: String) -> JsValue { | ||
| JsValue::from_str(&e) | ||
| } |
There was a problem hiding this comment.
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_err → JsValue::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
There was a problem hiding this comment.
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>
e416d3b to
b081ef7
Compare
Bumped zebra-chain dev-dep. New zebra_oracle test module: decodes each of the three golden fixtures with |
OttoAllmendinger
left a comment
There was a problem hiding this comment.
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 signaling —
js_err/bare-string is gone.zcash.rsreturnsResult<_, WasmUtxoError>throughout, andWasmUtxoErrorgrew real typed variants (UnifiedAddress(UnifiedAddressError),ZcashV6(ZcashV6Error)) whosecode()delegates to the inner enum. So a JS caller catching a v6 decode failure gets a markedjs_sys::Errorwith.code === "ZcashV6Error.TruncatedActions"etc. — exactly the "ideal" typed-code path, not just the string-funnel minimum. Nice. - Hex/
to_hex— replaced withTxid::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 aTxiddoes everywhere else. - Independent oracle —
zebra-chain = "11.2"(with a precise comment on why 11.2 is the floor forTransaction::V6), and thezebra_oracletest module assertszebra.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:
ZcashV6Transaction::sapling_value_balanceis effectively vestigial — decode always sets it to 0, encode rejects non-zero, andcompute_v6_txidalways 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 adebug_assert!(self.sapling_value_balance == 0)incompute_v6_txid, to close the footgun.- Cosmetic:
flags = *take_bytes(&mut r, 1)?.first().unwrap()reads more naturally astake_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
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).
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.