Skip to content

Apply Ironwood integration changes onto upstream feat/ironwood#70

Draft
ValarDragon wants to merge 1 commit into
feat/ironwoodfrom
ironwood-integration-on-upstream-feat
Draft

Apply Ironwood integration changes onto upstream feat/ironwood#70
ValarDragon wants to merge 1 commit into
feat/ironwoodfrom
ironwood-integration-on-upstream-feat

Conversation

@ValarDragon

@ValarDragon ValarDragon commented Jul 4, 2026

Copy link
Copy Markdown

Summary

This draft PR applies the non-PCZT file-level changes from #44 onto the updated feat/ironwood branch, which now matches upstream/feat/ironwood at 4d98d563b1.

The PCZT crate has been restored to feat/ironwood; upstream's PCZT serialization is treated as the correct version.

Notes

  • Base branch: feat/ironwood
  • Head branch: ironwood-integration-on-upstream-feat
  • Source PR: feat: Ironwood #44 (ironwood-integration)
  • This is a one-commit transplant rather than replaying PR feat: Ironwood #44's old 84-commit history.
  • Because upstream removed some crates that PR feat: Ironwood #44 still changes and references, this restores zcash_client_memory and zcash_extensions as part of the transplant.
  • zcash_client_backend now reconstructs Orchard PCZT notes using the bundle context for the note version: V2 for Orchard, V3 for Ironwood.

Validation

  • cargo fmt --all
  • git diff --check
  • git diff --cached --check
  • cargo check --workspace --all-features

cargo check --workspace --all-features passes with one warning about unused zcash_client_backend::ANY_ORCHARD_BUNDLE_VERSION.

@v12-auditor

v12-auditor Bot commented Jul 4, 2026

Copy link
Copy Markdown

Note

Complete: Audit complete. V12 found 15 issues worth reviewing.

Open the full results here.

FindingSeverityDetails
F-95816 🟡 Medium
TZE data discarded

Under zfuture, the normal Builder::build path can count TZE values during balance validation and then discard the TZE bundle. Builder::value_balance includes self.tze_builder.value_balance(), but the standard build() path passes a zfuture build_future closure that always returns (None, vec![]). build_internal stores that closure result in the unauthenticated transaction's tze_bundle and then into the final authorized transaction. A caller that adds TZE inputs or outputs and calls build() instead of build_zfuture() can produce a transaction that passed balance checks with TZE state but contains no TZE bundle.

F-95818 🟡 Medium
Demo extension escape

The Demo TZE OPEN validator does not bind the successor output to the Demo extension ID. EpochVTest::verify dispatches to Demo based on the spent precondition's extension_id, but the OPEN branch then reads only the new output's precondition.mode and precondition.payload. It checks the transaction is TZE-only, requires exactly one output, parses the output as a Demo CLOSE precondition, and verifies the hash relation. It never checks tze_out.precondition.extension_id, so a Demo OPEN spend can create a valid-looking CLOSE payload under another extension namespace.

F-95821 🟠 High
Nu7 V6 uses V5 digests

With zcash_unstable="nu7" enabled without nu6.3, the crate still exposes TxVersion::V6, selects V6 for BranchId::Nu7, and marks V6 valid in Nu7. The V6 signing symbol is then just a re-export of v5_signature_hash, and the txid path uses the non-NU6.3 to_hash combiner. V6 Sapling txid/auth domains and Ironwood digest handling are compiled only under nu6.3; V6 parsing and serialization also read and write the Ironwood field only under nu6.3. A Nu7-only build can therefore build, parse, serialize, and sign V6 transactions using V5-style digest semantics and no Ironwood field.

F-95824 🟠 High
Transparent authorization unchecked

TransactionExtractor::extract assembles transparent inputs from PCZT data without verifying their scripts. The extraction closure calls t.extract(), and the transparent extractor requires only that each input has a script_sig before constructing TxIn values. The later verification phase checks Sapling, Orchard, and Ironwood bundles, but performs no transparent script evaluation against the spent output's script_pubkey and final transaction sighash. A PCZT with a transparent input and a syntactically present but invalid script_sig can therefore produce Ok(tx).

F-95826 🟠 High
Unbounded PCZT parsing

Pczt::parse accepts arbitrary bytes and delegates the body to postcard_from_exact, which only checks that all bytes were consumed. The v2 wire structs contain unbounded vectors for actions and proof bytes, and global/proprietary metadata uses unbounded maps, strings, and byte vectors. No top-level byte limit, action count, map count, proof length, ciphertext length, or recursion budget is enforced before deserialization allocates. Exact parsing rejects trailing bytes, but it does not stop oversized well-formed PCZTs from driving proportional memory and CPU usage.

F-95828 🟡 Medium
Send-max drops Ironwood inputs

propose_send_max computes the recipient amount from all spendable notes, including V3/Ironwood Orchard notes, but can discard those V3 notes from the retained input set. It counts V3 notes in ironwood_input_count and subtracts them from orchard_input_count; after NU6.3, Orchard receiver outputs route to Ironwood, so requested_orchard_actions can be zero. use_orchard is then set from orchard_action_count > 0 only, ignoring ironwood_action_count. SimpleNoteRetention.orchard controls retention for all Orchard-pool notes regardless of note version, so a V3-only wallet can produce a proposal amount based on V3 value but retain no V3 inputs.

F-95833 🟡 Medium
Account deletion corrupts state

MemoryWalletDb::delete_account mutates account state before panicking. It removes the account from self.accounts.accounts and then immediately calls todo! for cleanup of transactions associated with the account. There is no transaction boundary or rollback around the in-memory mutation. If the application catches unwinding or continues after the panic, wallet tables can reference an account that has already been removed.

F-95834 🟡 Medium
Memory wallet stubs panic

The new memory wallet advertises WalletRead and WalletWrite implementations while several ordinary trait methods remain todo! or unimplemented!. User- or workflow-driven operations such as creating accounts from seeds in non-test builds, setting transaction trust, truncating or rewinding to a chain state, deriving an address for an index, notifying transparent address checks, listing addresses, and querying received outputs can abort the process. These are public trait entrypoints, not private test helpers. Callers therefore cannot safely rely on the memory wallet as a fallible backend for normal wallet operations.

F-95835 🔵 Low
Nu6.3 range overlaps Nu7

BranchId::Nu6_3.height_bounds declares an unbounded upper edge even when a later Nu7 epoch is compiled and configured. The Nu6_2 arm computes its upper bound by cascading to the next enabled upgrade, but the Nu6_3 arm returns (lower, None). In builds that enable both nu6.3 and nu7, for_height returns Nu7 at Nu7 heights because UPGRADES_IN_ORDER includes Nu7 and scans in reverse. The public epoch-range API therefore disagrees with the height-to-branch API.

F-95836 🔵 Low
Branch IDs collide

Nu6_3, Nu7, and ZFuture all serialize to the same placeholder consensus branch ID 0xffff_ffff. TryFrom<u32> contains duplicate match arms for that value under their respective cfgs, and From<BranchId> returns the same value for all three variants. Builds that enable two of these unstable variants therefore have a non-injective branch mapping where serializing a later branch and parsing it back can resolve to an earlier arm. The version group ID placeholders also collide for V6 and ZFuture.

F-95837 🔵 Low
ZFuture never selected

ZFuture is modeled as a network upgrade and branch, but it is absent from UPGRADES_IN_ORDER. BranchId::for_height derives the active branch solely by scanning that list, so it can never return BranchId::ZFuture. LocalNetwork exposes a caller-configurable z_future activation height, and Builder::new selects the default transaction version from BranchId::for_height. At a ZFuture-active height, the builder therefore selects the preceding branch and V5/V6 rather than TxVersion::ZFuture.

F-95838 🟡 Medium
Direct PCZT role APIs skip global version and branch validation

Multiple direct PCZT role APIs operate on Orchard, Sapling, and transparent bundles without first enforcing the same global transaction validation used by extract_tx_data. The affected paths parse protocol-specific bundles and then immediately invoke caller-supplied callbacks or begin proof generation in update_orchard_with, with_orchard, sign_orchard_with, create_orchard_proof, sign_sapling_with, sign_transparent_with, and create_sapling_proofs. In the Orchard case, this is compounded by orchard_bundle_format, which maps an unrecognized consensus_branch_id to BundleVersion::orchard_insecure_v1() instead of failing. As a result, PCZTs with unsupported transaction versions or invalid version/branch pairings can still reach sensitive low-level role flows that were expected to run only on extractable transactions. The fix is to centralize the tx_version/version_group_id/consensus_branch_id checks and require all direct role entrypoints, including Orchard format selection, to fail closed before parsing bundles or invoking work.

F-95839 🟡 Medium
Provider-controlled scan inputs can panic instead of erroring

Both scanning paths trust provider-controlled block data enough to pass it through panic-prone conversions before any recoverable validation occurs. In the compact path, block.hash(), block.prev_hash(), block.height(), tx.txid(), and TxIndex::try_from(tx.index).expect(...) can abort the process when hashes are not 32 bytes, heights do not fit u32, or transaction metadata is oversized. In the full-block path, parsing accepts a provider-supplied transaction count into usize, and later the scanner converts enumerated positions with TxIndex::try_from(tx_index).expect(...), so a syntactically valid oversized block can still panic during scanning. These failures all happen on the chain-provider boundary, where malformed or adversarial input should instead be rejected through normal error handling. The fix pattern is the same across both inputs: replace panic-based helper calls and expect-based index conversions with fallible validation that maps invalid block metadata and oversized transaction vectors into Result/ScanError paths.

F-95840 🟡 Medium
Public PCZT builder inputs can trigger panic paths

The public PCZT creator APIs treat caller-controlled branch IDs and Orchard/Ironwood flag selections as if they were already valid, then later unwrap fallible conversions with expect. In Creator, new accepts any u32 as consensus_branch_id, but with_orchard_flags assumes that value can always be converted with BranchId::try_from and also assumes orchard_flags.to_byte(...) is always encodable for the selected branch. In V6Creator, both with_orchard_flags and with_ironwood_flags expose public orchard::bundle::Flags inputs while still using expect on fallible encoding. These are all public builder-style entry points that return Self instead of surfacing validation failure, so malformed inputs reach a panic path rather than a normal error. The implementation work to fix them is the same: validate these public inputs up front or make the setters/constructors return Result and propagate encoding failures instead of unwrapping.

F-95841 🟡 Medium
Incoming payment storage paths panic in multiple pools

The memory wallet's receive-side persistence paths are incomplete and still contain reachable todo!() calls for ordinary incoming funds. In store_decrypted_tx, both Sapling and Orchard outputs classified as TransferType::Incoming panic instead of being recorded as received notes. With transparent-inputs enabled, put_received_transparent_utxo can also reach a todo!() when the first recognized transparent output is inserted and the code tries to populate ReceivedTransparentOutput. These cases all occur on normal wallet receive flows rather than on exceptional or test-only paths. The common remediation is to fully implement received-output persistence metadata for each pool, including the missing transparent key-scope lookup, and ensure these entrypoints return typed errors instead of aborting if a prerequisite is unavailable.

And four more auto-invalidated findings.

Analyzed 105 files, diff 4d98d56...5fef3b0.

Comment thread .github/workflows/ci.yml
--release
--workspace
--tests
${{ steps.prepare.outputs.feature-flags }}
@socket-security

socket-security Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedwasm_sync@​0.1.210010093100100
Addedorchard@​0.15.0-pre.110010097100100

View full report

Transplant the file-level changes from #44 onto the updated upstream feat/ironwood base.

Co-Authored-By: OpenAI Codex <codex@openai.com>
@ValarDragon
ValarDragon force-pushed the ironwood-integration-on-upstream-feat branch from 5fef3b0 to 707b0e8 Compare July 4, 2026 19:01
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