feat(wasm-utxo): add signing support for shielding transactions - #353
Conversation
0595c51 to
1be8781
Compare
1be8781 to
b09c481
Compare
|
@claude review this for correctness, architecture, reusability, and how much this affects existing functionality |
| .any(|input| !input.partial_sigs.is_empty()); | ||
| if !already_signed { | ||
| self.set_ironwood_out_ciphertext_for_user(0, xpriv, root_wallet_keys, secp) | ||
| .map_err(|e| format!("{e} (the user must sign a v6 shielding PSBT first)"))?; |
There was a problem hiding this comment.
So, are we saying that backup + bitgo signing is not allowed for v6 txns?
There was a problem hiding this comment.
It is allowed, but the first key that should be used via this flow should be the user key, as the ovk derived depends on the user key.
davidkaplanbitgo
left a comment
There was a problem hiding this comment.
Adds client-managed ovk derivation (ECDH between the wallet's BitGo-root pubkey and user-root privkey) so a client can re-encrypt an Ironwood shielded output's out_ciphertext without the server ever learning the ovk, plus a sign()/sign_ironwood_v6 flow that enforces the user signs first (to fix out_ciphertext before the ZIP-244 sighash is computed).
Verified: cargo test --lib — 543/543 pass, 0 failures (including the new ironwood/ecdh tests); cargo clippy --lib clean. Full wasm build couldn't be exercised locally (missing wasm32 clang target for secp256k1-sys's ECDH module) — confirmed this is a pre-existing local environment limitation, not something this branch introduces (reproduces identically on master's Cargo.toml/Cargo.lock).
Correctness
- ECDH symmetry is exploited correctly and is the crux of the design: client derives
ECDH(bitgo_pubkey, user_privkey), server independently derivesECDH(user_pubkey, bitgo_privkey)— same shared secret, without either side transmitting theovkor the user's key. This is tested end-to-end (server_can_independently_derive_ovk_and_validate_out_ciphertext_before_signing) including the negative case (wrong user pubkey on file → server's derivedovkdoesn't match, validation correctly fails). compute_out_ciphertextguards against splicing in bad ciphertext: it reconstructs the note from the PCZT's plaintextrecipient/value/rseed/rhoand checks it against the action's already-committedcmxbefore encrypting — otherwise a stale/corrupted PCZT would silently produce anout_ciphertextwith aneskinconsistent with the fixedephemeral_key(garbage that only reveals itself when someone later tries to recover the note). Good defensive check, well tested (compute_out_ciphertext_rejects_a_note_that_does_not_match_the_committed_cmx).- Sighash-ordering is enforced, not just documented:
set_ironwood_out_ciphertextrejects being called once any transparentpartial_sigsexist (sinceout_ciphertextis ZIP-244 sighash-committed and changing it after a signature would silently invalidate that signature). Tested directly. - Signing-order enforcement (
sign_ironwood_v6): only the user root key may open the first signing round (since that round is what fixesout_ciphertextunder the wallet'sovk); Bitgo or backup signing first is rejected outright rather than silently deriving anovknobody can reproduce. This is exactly the failure mode you'd want to catch loudly — a "successfully broadcasts, silently unrecoverable" bug — and it's well covered (sign_ironwood_v6_rejects_a_first_round_opened_by_a_non_user_key, JS equivalent too). set_ironwood_out_ciphertext_for_uservalidates the passed xpriv actually matches the wallet'suser_key()pubkey — guards against a caller accidentally passing the backup/BitGo key or an unrelated xpriv.- Golden/oracle test (
golden_shield1zec_out_ciphertext_round_trips_on_real_on_chain_note_data) confirms the primitive against a real on-chain Zcash note'scv/cmx/rho, and separately confirms the on-chain reference tx's ownout_ciphertextwas built keyless (not decryptable under any realovk) — good rigor distinguishing "our own construction is correct" from "matches a fixture that happens to also be keyless." - One design point worth being explicit about (already documented in the code, not a bug): the
ovkis derived from the two root keys, so it's the same value for every transaction of a given wallet, forever. That matches Zcash's own account-levelovkconvention, but it does mean ifuser_privkeyis ever compromised, every past and future outgoing note for that wallet becomes decryptable — worth being aware of at the product level, not something to fix in this diff. - Noted consequence, explicitly called out in comments: backup-key recovery cannot open the first signing round of a shielding transaction (since the
ovkis defined by the user key). Intentional and documented, not accidental.
Reusability / Maintainability
derive_client_ovk/compute_out_ciphertextare cleanly separated pure functions inironwood_build.rs, reused identically by both the raw-key (set_ironwood_out_ciphertext) and wallet-key (set_ironwood_out_ciphertext_for_user) entry points — no duplicated ECDH/encryption logic between them.sign_ironwood_v6reimplements key resolution viabip32_derivation+GetKeyrather than reusingminiscript's built-in PSBT signer — justified in the doc comment (the v6 ZIP-244 digest isn't somethingminiscriptknows how to compute), so this isn't unnecessary duplication, just an unavoidable consequence of v6 needing its own sighash.- The TS
sign()override onZcashIronwoodBitGoPsbtis a bit involved (three overload signatures to stay assignable to the base class's(inputIndex, key)deprecated overload) but the comments explain exactly why each piece exists, and it correctly throws (rather than misbehaving) for each unsupported base-class shape (index-based signing, raw ECPair keys, omittedrootWalletKeys). - The
secp256k1crate is now a direct dependency purely to reach theecdhmodule, pinned to the same version already resolved transitively viaminiscript/bitcoin— correctly reasoned in theCargo.tomlcomment so it doesn't create a second copy of the crate in the dependency graph.Cargo.lockdiff confirms no version duplication. zeroize'd copy of the user's derived private-key bytes inset_ironwood_out_ciphertext_for_userreduces (but, as with most Rust FFI/secp bindings, doesn't fully eliminate —secp256k1::SecretKeyitself doesn't zero on drop) the exposure window; reasonable best-effort given the stated goal ("the ovk never leaves this call").
Impact on non-ZEC coins
Zero. Every new symbol (derive_client_ovk, compute_out_ciphertext, with_out_ciphertext, set_ironwood_out_ciphertext[_for_user], sign_ironwood_v6) lives under zcash/ or is a Zcash-specific method on ZcashBitGoPsbt/ZcashIronwoodBitGoPsbt. The one generic-surface file touched, wasm/fixed_script_wallet/mod.rs, only adds two new BitGoPsbt methods that delegate to self.zcash_mut() — pure additions, no changes to any existing shared code path. The new secp256k1/zcash_note_encryption/zeroize dependencies are additive and don't alter behavior for other coins.
Summary
Strong, well-tested piece of new functionality with a coherent security story (ECDH symmetry lets the server validate without ever learning the secret) and good defensive checks against the two realistic failure modes: signing before out_ciphertext is final, and someone other than the user opening the signing round. No correctness issues found; only a documentation-level note that the wallet-wide static ovk has full-history blast radius if the user root key is ever compromised, which is inherent to the design and already acknowledged in the code's own comments.
Lets the client derive out_ciphertext's ovk on the fly as the ECDH agreement of the BitGo cosigner pubkey and its own signing privkey, instead of the server ever holding or seeing it.
Ticket: CSHLD-1377