Skip to content

test(state, rpc): handle coinbase Sapling spends in proptests broken by #10527 - #10533

Merged
conradoplg merged 3 commits into
mainfrom
fix-rpc-proptests-coinbase-sapling
May 1, 2026
Merged

test(state, rpc): handle coinbase Sapling spends in proptests broken by #10527#10533
conradoplg merged 3 commits into
mainfrom
fix-rpc-proptests-coinbase-sapling

Conversation

@upbqdn

@upbqdn upbqdn commented May 1, 2026

Copy link
Copy Markdown
Member

Motivation

Several CI jobs are red on main:

  1. Five proptests in zebra-rpc::methods::tests::prop fail after fix(chain): reject coinbase Sapling spends during deserialization #10527. The any::<Transaction>() (and any::<[Transaction; 2]>()) arbitrary strategies can produce coinbase transactions containing Sapling spends. After fix(chain): reject coinbase Sapling spends during deserialization #10527 those are rejected during deserialization, so send_raw_transaction returns a deserialization error before reaching the mempool, and each affected test waits indefinitely for a mempool request that never arrives.
  2. Three proptests in zebra-state::service::finalized_state panic for the same reason: chains generated by PreparedChain contain coinbase transactions with Sapling shielded data that fail to round-trip through IntoDisk/FromDisk. roundtrip_transaction hits the same panic via any::<Transaction>().
  3. Cargo.lock is out of date (missing the rand 0.8.5rand 0.8.6 bump for zebra-script).
  4. cargo fmt --check fails.

#10527 updated the transaction_roundtrip proptest in zebra-chain for the same case but missed the downstream proptests that consume the same arbitrary strategy.

Solution

  • Skip the invalid coinbase-with-Sapling-spends combination with
    prop_assume! in the five affected RPC proptests:
    • mempool_receives_raw_tx
    • mempool_errors_are_forwarded
    • rejected_txs_are_reported
    • rpc_queue_main_loop
    • rpc_queue_receives_all_txs_from_channel
  • For zebra-state, fix the chain-builder fixup layer: clear
    sapling_shielded_data on coinbase transactions in
    fix_generated_transaction. This covers blocks_with_v5_transactions
    and all_upgrades_and_wrong_commitments_with_fake_activation_heights,
    which both build chains via PreparedChain.
  • Add a prop_assume! to roundtrip_transaction since it bypasses the
    chain builder and uses any::<Transaction>() directly.
  • The Transaction::Arbitrary strategy is intentionally left unchanged
    so the rejection path is still exercised by transaction_roundtrip
    and the GHSA-rgwx-8r98-p34c reproduction vector in zebra-chain (per
    the design choice in fix(chain): reject coinbase Sapling spends during deserialization #10527).
  • Refresh Cargo.lock.
  • Apply cargo fmt --all.

AI Disclosure

  • AI tools were used: Claude (Opus 4) for investigation and patch drafting.

PR Checklist

  • The PR title follows conventional commits format.
  • The PR follows the contribution guidelines.
  • This change was discussed in an issue or with the team beforehand. (Maintainer-driven: fixes CI breakage on main.)
  • The solution is tested.
  • The documentation and changelogs are up to date. (No changelog entry — test-only / build-only changes.)

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

arya2
arya2 previously approved these changes May 1, 2026
upbqdn added 2 commits May 1, 2026 20:53
…ction proptests

The `any::<Transaction>()` arbitrary strategy can produce coinbase
transactions that contain Sapling spends. Since #10527 (GHSA-rgwx-8r98-p34c),
those transactions are rejected during deserialization, so
`send_raw_transaction` returns a deserialization error before reaching the
mempool. The five affected proptests then time out waiting for a mempool
request that never arrives.

Skip those cases with `prop_assume!` so each test exercises the mempool
path it is intended to cover. The `transaction_roundtrip` proptest in
`zebra-chain` was updated to handle the same combination in #10527.
@upbqdn

upbqdn commented May 1, 2026

Copy link
Copy Markdown
Member Author

Force-pushed. More proptests were failing than originally fixed.

arya2
arya2 previously approved these changes May 1, 2026
After #10527 (GHSA-rgwx-8r98-p34c) rejects coinbase transactions with
Sapling spends during deserialization, three `zebra-state` proptests
panic when arbitrary chains contain such transactions:

- `roundtrip_transaction` reads back a generated `Transaction` via
  `IntoDisk`/`FromDisk` and panics in `FromDisk::from_bytes`.
- `blocks_with_v5_transactions` and
  `all_upgrades_and_wrong_commitments_with_fake_activation_heights`
  commit chains generated by `PreparedChain` to the finalized state,
  whose underlying `partial_chain_strategy` produces coinbase
  transactions with arbitrary Sapling shielded data.

Fix the chain-builder path by clearing `sapling_shielded_data` on
coinbase transactions in `fix_generated_transaction`. Add a
`prop_assume!` to `roundtrip_transaction` since it bypasses the chain
builder and uses `any::<Transaction>()` directly.

The `Transaction::Arbitrary` strategy is intentionally left unchanged so
the rejection path is still exercised by the `transaction_roundtrip`
proptest and the GHSA-rgwx-8r98-p34c reproduction vector in
`zebra-chain` (per the design choice in #10527).
@upbqdn upbqdn changed the title test(rpc): skip invalid coinbase Sapling-spend txs in send_raw_transaction proptests test(state, rpc): handle coinbase Sapling spends in proptests broken by #10527 May 1, 2026
@upbqdn
upbqdn requested a review from arya2 May 1, 2026 20:13
@upbqdn upbqdn self-assigned this May 1, 2026
@upbqdn upbqdn mentioned this pull request May 1, 2026
56 tasks
@conradoplg

Copy link
Copy Markdown
Collaborator

Admin-merging due to stuck disconnects_from_misbehaving_peers

@conradoplg
conradoplg merged commit a60ff83 into main May 1, 2026
140 of 142 checks passed
@conradoplg
conradoplg deleted the fix-rpc-proptests-coinbase-sapling branch May 1, 2026 21:35
judah-caruso pushed a commit to ShieldedLabs/zebra-crosslink-staging that referenced this pull request May 28, 2026
ZcashFoundation#10527 (ZcashFoundation#10533)

* test(rpc): skip invalid coinbase Sapling-spend txs in send_raw_transaction proptests

The `any::<Transaction>()` arbitrary strategy can produce coinbase
transactions that contain Sapling spends. Since ZcashFoundation#10527 (GHSA-rgwx-8r98-p34c),
those transactions are rejected during deserialization, so
`send_raw_transaction` returns a deserialization error before reaching the
mempool. The five affected proptests then time out waiting for a mempool
request that never arrives.

Skip those cases with `prop_assume!` so each test exercises the mempool
path it is intended to cover. The `transaction_roundtrip` proptest in
`zebra-chain` was updated to handle the same combination in ZcashFoundation#10527.

* style: rustfmt drift in inbound/downloads.rs

* test(state): handle coinbase Sapling spends in finalized-state proptests

After ZcashFoundation#10527 (GHSA-rgwx-8r98-p34c) rejects coinbase transactions with
Sapling spends during deserialization, three `zebra-state` proptests
panic when arbitrary chains contain such transactions:

- `roundtrip_transaction` reads back a generated `Transaction` via
  `IntoDisk`/`FromDisk` and panics in `FromDisk::from_bytes`.
- `blocks_with_v5_transactions` and
  `all_upgrades_and_wrong_commitments_with_fake_activation_heights`
  commit chains generated by `PreparedChain` to the finalized state,
  whose underlying `partial_chain_strategy` produces coinbase
  transactions with arbitrary Sapling shielded data.

Fix the chain-builder path by clearing `sapling_shielded_data` on
coinbase transactions in `fix_generated_transaction`. Add a
`prop_assume!` to `roundtrip_transaction` since it bypasses the chain
builder and uses `any::<Transaction>()` directly.

The `Transaction::Arbitrary` strategy is intentionally left unchanged so
the rejection path is still exercised by the `transaction_roundtrip`
proptest and the GHSA-rgwx-8r98-p34c reproduction vector in
`zebra-chain` (per the design choice in ZcashFoundation#10527).
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.

3 participants