feat(ethexe/compute): use proptests instead of custom mock()

[playX18]

No description provided.

[semanticdiff-com]

Review changes with  

Changed Files

File	Status
ethexe/compute/src/prepare.rs	67% smaller
ethexe/compute/src/service.rs	55% smaller
ethexe/compute/src/compute.rs	39% smaller
ethexe/compute/src/tests.rs	36% smaller
Cargo.lock	Unsupported file format
ethexe/compute/Cargo.toml	Unsupported file format

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the testing suite for the ethexe/compute module by migrating from custom mock-based tests to property-based testing using proptest. This change aims to increase the reliability and coverage of the compute service tests by automatically generating a wider variety of test scenarios, such as different blockchain configurations and event sequences. The infrastructure for these tests has been centralized in ethexe/compute/src/tests.rs to provide reusable strategies and helpers for future test development.

Highlights

• Proptest Integration: Replaced custom mock-based tests with property-based testing using the proptest crate to improve test coverage and robustness.
• Test Infrastructure: Introduced new test utilities, including strategies for generating blockchain states and code IDs, and helper functions for async test execution.
• Dependency Update: Added proptest to Cargo.toml and updated Cargo.lock to include the new testing dependency.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request migrates the ethexe/compute test suite to property-based testing using the proptest library, introducing strategies for blockchain generation and promise handling. These changes significantly enhance the verification of block preparation, announce computation, and code processing logic. The review feedback focuses on optimizing the async test runner by utilizing a thread-local Tokio runtime to reduce overhead, improving test robustness by increasing the complexity and volume of generated test cases, and ensuring safety by replacing unnecessary unsafe code with safe, idiomatic Rust conversions.

[playX18]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a56eda7ef0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[playX18]

@claude review

[claude]

Claude finished @playX18's task in 6m 34s —— View job

---

Code Review: feat(ethexe/compute): use proptests instead of custom mock()

Tasks

• Gather context
• Read changed files
• Analyze proptest migration
• Post review

---

Summary

This PR replaces the ad-hoc #[tokio::test] tests in ethexe-compute with proptest-based property tests, introducing a shared tests.rs infrastructure (thread-local runtime, strategy helpers, async event helpers). The migration is clean and the test logic is largely preserved. The new tests catch more edge cases — notably, prepare_and_assert_block now correctly handles the events_in_block = 0 case (no codes to request), which the original would have panicked on via unwrap_request_load_codes().

The unsafe-in-tests concern Gemini raised (unsafe { HashOf::new(hash) }) is a non-issue — HashOf::new is pub unsafe fn by design; playX18's response is correct.

---

medium: test_compute_with_promises has an implicit fragile contract between strategy and event access

File: ethexe/compute/src/compute.rs line 677

let tx = announce.injected_transactions[0].clone().into_data();

This panics if injected_transactions is empty. It's currently safe because requestable_indexes = (2..blockchain_len) and announces with i >= 2 always have exactly one injected transaction. But the invariant is implicit — nothing in the code at the call site signals this constraint. If requestable_indexes were changed to include index 1 (no injected txs), this would silently become a panic. At minimum, document the invariant with a comment; better would be:

let tx = announce.injected_transactions.first()
    .expect("requestable indexes start at 2, guaranteeing one injected tx per announce")
    .clone()
    .into_data();

---

medium: No overall timeout for Wasmtime-heavy proptest cases

File: ethexe/compute/src/compute.rs lines 609-757

The original test_compute_with_promises and test_compute_with_early_break used #[ntest::timeout(60000)] (60 second wall-clock limit). The proptest versions run 64 cases of test_compute_with_promises and test_compute_with_early_break, each involving real Wasmtime execution (demo-ping processing), chains up to 16 blocks long, 5 PING messages per block, and sequential predecessor computation. On slow CI machines this could blow past the per-event ASYNC_EVENT_TIMEOUT (3s) but still exceed a reasonable total budget.

The Codex reviewer flagged a related concern about the per-event timeout. The real risk here is total runtime — e.g. 64 cases × 15 blocks × 5 messages × Wasmtime overhead. Consider reducing case count for the Wasmtime-heavy tests (16–32 instead of 64), or re-adding #[ntest::timeout] at the proptest block level:

proptest! {
    #![proptest_config(ProptestConfig::with_cases(32))]
    #[test]
    fn test_compute_with_promises(...) { ... }

---

low: test_compute_with_promises — predecessor announce computation is silent but event count is exact-match

File: ethexe/compute/src/compute.rs lines 693-696

let observed_events =
    collect_compute_events(&mut compute_service, expected_events.len()).await;
assert_eq!(observed_events, expected_events);

collect_compute_events blocks until exactly expected_events.len() events are received, then assert_eq. This is correct as long as predecessors really are computed silently (no events emitted). That invariant holds today (compute_one doesn't push to the stream), but it's a subtle assumption. If a future refactor makes predecessors emit AnnounceComputed events, this test would collect wrong events and produce a confusing mismatch. A brief comment would make the intent clear:

// Predecessors are computed silently (no AnnounceComputed emitted for them);
// only the explicitly requested announces emit events.

---

low: blockchain_len variable shadowed inconsistently across strategy and test body

File: ethexe/compute/src/compute.rs lines 526-536 vs 625

promise_test_inputs_strategy() generates blockchain_len as the chain-length parameter. In the test body, blockchain_len is recomputed as blockchain.blocks.len() - 1, which happens to equal the strategy's value (since block_chain_strategy(n) produces n+1 blocks). This works, but it's easy to miss. Rename the local to chain_depth or add a comment that the recomputation is identity:

// blockchain.blocks includes genesis, so blocks.len() - 1 == the strategy's blockchain_len param
let chain_depth = blockchain.blocks.len() - 1;

---

nit: assert_no_compute_event timeout increased from 100ms → 500ms

File: ethexe/compute/src/tests.rs line 51

const NO_EVENT_TIMEOUT: Duration = Duration::from_millis(500);

The original test used 100ms for the "no follow-up event" assertion. 500ms is less flaky on slow machines — that's fine — but it adds 500ms to every assert_no_compute_event call. Currently only used in one test (code_validation_request_for_already_processed_code_does_not_request_loading), so not a problem in practice.

---

What's good

• Thread-local runtime reuse (TEST_RUNTIME) is the right pattern — no per-iteration runtime allocation overhead.
• run_async_test signature simplified: returning F::Output and panicking internally is cleaner for proptest's prop_assert! integration.
• distinct_code_ids using BTreeSet strategy guarantees uniqueness without rejection sampling.
• prepare_and_assert_block now handles both RequestLoadCodes and BlockPrepared as first events — this is a correctness improvement over the original, which assumed codes are always requested.
• The collect_not_computed_predecessors_work_correctly proptest (128 cases, synchronous) tests the chain traversal logic thoroughly across chain lengths 2–16.

[grishasobol]

@claude opus review

[claude]

Claude finished @grishasobol's task in 3m 7s —— View job

---

Code Review (Opus)

Tasks

• Gather context (read changed files)
• Analyze proptest migration vs original
• Identify bugs, regressions, robustness issues
• Post review

---

Summary

The PR replaces #[tokio::test] ad-hoc tests with proptest-based property tests, centralized in tests.rs. The migration is well-structured: a thread-local Tokio runtime avoids per-iteration runtime allocation, strategies are reusable, and the prepare_and_assert_block helper now tolerates both RequestLoadCodes and BlockPrepared as the first event (a correctness improvement). Logic is preserved, including the gas-allowance/early-break behavior.

The findings below are mostly hardening notes — there's no correctness regression. The largest practical concern is total Wasmtime runtime under proptest's case multiplier on CI.

---

medium: total Wasmtime runtime under proptest will be high on slow CI

File: ethexe/compute/src/compute.rs lines 562–757

Two heavy property tests now run with cases = 64:

• test_compute_with_promises: chain length 4–16, 5 PING canonical events per block + 1 injected tx per requested block, then real Processor::process_programs runs over the demo-ping WASM. With up to ~14 requested announces per case, each running through Wasmtime, the worst case can take many seconds. Multiply by 64 cases.
• test_compute_with_early_break: 100–300 injected txs per case × 64 cases = up to ~19,200 demo-ping executions.

Each individual event has a 3 s timeout (ASYNC_EVENT_TIMEOUT), but no test-level wall-clock budget. The original tests had #[ntest::timeout(60000)]; that protection is gone. On a slow runner you can hit a flaky timeout on a single event before total budget is exhausted, with no clear signal that the issue is "test too heavy" rather than "code regressed."

Suggestions, in increasing order of disruption:

• Reduce cases for the Wasmtime-heavy tests to 16–32; they exercise many code paths but each case is expensive.
• Cap tx_count lower in test_compute_with_early_break (e.g. 30..=100) — the property being tested ("early break completes within tx_count+1 events") doesn't need 300 to be meaningful.
• Re-add a per-case wall-clock timeout via #[ntest::timeout(...)] outside the proptest! block, or bump ASYNC_EVENT_TIMEOUT for the heavy tests to ~10 s.

Fix this →

---

medium: implicit invariant on injected_transactions[0] is fragile

File: ethexe/compute/src/compute.rs line 677

let tx = announce.injected_transactions[0].clone().into_data();

This panics if injected_transactions is empty. It's currently safe because:

• requestable_indexes = (2..blockchain_len) (line 529) → request indexes start at 2
• The build loop only sets injected_transactions when i != 1 (lines 650–653) → indexes ≥ 2 always have one tx

But the invariant is implicit and load-bearing. If anyone ever lowers requestable_indexes to start at 1 (intuitive when reading "request_indexes"), this becomes a panic that proptest will minimize but produce a confusing error. Document or assert:

let tx = announce.injected_transactions
    .first()
    .expect("requestable indexes start at 2; each requested announce carries one injected tx")
    .clone()
    .into_data();

---

low: request_indexes order coupling is correct but undocumented

File: ethexe/compute/src/compute.rs lines 533, 672–696

prop::sample::subsequence preserves input order, so request_indexes is monotonically increasing. The test then iterates them in order, calling compute_announce and queuing expected Promise+AnnounceComputed pairs. Because announces are linked into a chain and compute_one for predecessors uses promise_out_tx = None (silent), the observed event sequence equals the expected one only if:

1. Predecessors really emit no events.
2. Subsequence is order-preserving.
3. Each requested announce produces exactly one Promise (one injected tx, demo-ping deterministically replies once).

All three hold today, but each is non-obvious from the call site. A short comment near line 694 documenting "predecessors are computed silently; only request_indexes emit events" would prevent surprise during a future refactor.

---

low: distinct_code_ids returns sorted Vec

File: ethexe/compute/src/tests.rs line 57–61

collection::btree_set(any::<[u8; 32]>().prop_map(CodeId::from), count)
    .prop_map(|ids| ids.into_iter().collect())
    .boxed()

BTreeSet → Vec always yields ascending byte-order. So in test_prepare_one_block, code1_id < code2_id always; you never test the reverse ordering. For test_prepare_one_block this happens not to matter (the assertion is on a HashSet-derived codes_queue), but for any future test that depends on input order you'd silently lose half the input space. Consider returning a randomly-shuffled Vec if order should be exercised, or rename to distinct_code_ids_sorted to make the property explicit.

---

low: predecessor_test_inputs_strategy redundantly threads blockchain_len

File: ethexe/compute/src/compute.rs lines 540–547

fn predecessor_test_inputs_strategy() -> BoxedStrategy<(BlockChain, usize)> {
    (2usize..=16)
        .prop_flat_map(|blockchain_len| {
            block_chain_strategy(blockchain_len as u32)
                .prop_map(move |chain| (chain, blockchain_len))
        })
        .boxed()
}

blockchain_len is then immediately recovered as blockchain.blocks.len() - 1 style indexing in the test (e.g. blockchain_len - 1 indexes into blocks). Could be simpler to derive blockchain_len from chain.blocks.len() inside the test and drop the second tuple element. Same observation applies to chain_with_event_count_strategy — events_in_block is genuinely independent there, but the value from the strategy is used as both an index bound and a count, so worth a comment about the relationship.

---

nit: mark_as_not_prepared clears all non-genesis announces

File: ethexe/compute/src/tests.rs lines 200–211

This is correct for the current consumers but couples "not prepared" to "no announces". If a future test wants to reset prepared state while keeping announces, this helper would silently corrupt the chain. Worth either renaming to reset_to_unprepared or splitting the two operations.

---

What's good

• TEST_RUNTIME thread-local pattern eliminates the per-case runtime allocation overhead Gemini flagged. Right call, no need for unsafe.
• prepare_and_assert_block correctly handles the events_in_block = 0 case via the BlockPrepared arm — the original #[tokio::test] version would have panicked here.
• predecessor_test_inputs_strategy covers the chain-length-2 edge case (no non-computed predecessors) cleanly: expected_not_computed_announces is empty, and the assertion still holds.
• The unsafe { HashOf::new(hash) } in announce_hash_strategy is correct — HashOf::new is unsafe by design (caller asserts the hash is a real Blake2 of an Announce), and a test that mints arbitrary 32-byte values must opt in. Author's response on that thread is right.
• 128 cases for the two synchronous proptests (test_prepare_one_block, collect_not_computed_predecessors_work_correctly) is well-calibrated — they're cheap and benefit from breadth.

---

— branch ap/compute-proptest

[playX18]

/review-delta

[playX18]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved in 0bff454. I merged the latest master, fixed the conflicts in ethexe/compute/src/compute.rs, and pushed the merge commit.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• dl.google.com
  • Triggering command: /usr/lib/apt/methods/https /usr/lib/apt/methods/https k-avxifma.s DELETE T5 -DOPENSSL_CPUID_OBJ -DOPENSSL_IA32_SSE2 -fPI -I penssl-sys-efd05062af3058e9/out/openssl-build/install/include/openssl/ct.h ome/REDACTED/work/gear/gear/target/debug/build/openssl-sys-efd0506crypto/modes/ghash-x86_64.s u/13/cc1 pto-lib-ecp_nistbasename stz256-x86_64.s --noexecstack u/13/cc1 --gd�� 062af3058e9/out/openssl-build/install/include/openssl/hmac.h --noexecstack /usr/sbin/as ypto-lib-rc4-x86chmod 86_64.s /usr/bin/as as (dns block)
• https://api.github.com/repos/gear-tech/parity-wasm/commits/v0.45.0-sign-ext
  • Triggering command: /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo metadata --format-version 1 --manifest-path /home/REDACTED/work/gear/gear/target/debug/wbuild/ethexe-runtime/Cargo.toml rocksdb/third-party/gtest-1.8.1/fused-src/ -I snappy/ -I . -Wall 20-x�� 20-x86_64-unknow-g 20-x86_64-unknow-gdwarf-4 2c9a1cad1328.rli-fno-omit-frame-pointer wind-6d6d5f1558e/usr/libexec/gcc/x86_64-linux-gnu/13/cc1plus ut/053adb4869b9269c-block_based_table_factory.o _build-75a980e50-I 4ed7.derive_syn_rocksdb/include/ (http block)
  • Triggering command: /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo rustc --target=wasm32v1-none --color=always --manifest-path=/home/REDACTED/work/gear/gear/target/debug/build/demo-ping-d8cd3400f06c158b/out/Cargo.toml --profile dev --features ethexe -- -Clink-arg=--import-memory -C link-arg=--mllvm=-mcpu=mvp -C link-arg=--mllvm=-mattr=+mutable-globals -C linker-plugin-lto ibrocksdb-sys-14-m64 -fno-omit-frame-/tmp/rustcZzbzk4/symbols.o as (http block)
• https://api.github.com/repos/gear-tech/polkadot-sdk/commits/gear-polkadot-stable2409-wasm32v1-none
  • Triggering command: /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo metadata --format-version 1 --manifest-path /home/REDACTED/work/gear/gear/target/debug/wbuild/ethexe-runtime/Cargo.toml rocksdb/third-party/gtest-1.8.1/fused-src/ -I snappy/ -I . -Wall 20-x�� 20-x86_64-unknow-g 20-x86_64-unknow-gdwarf-4 2c9a1cad1328.rli-fno-omit-frame-pointer wind-6d6d5f1558e/usr/libexec/gcc/x86_64-linux-gnu/13/cc1plus ut/053adb4869b9269c-block_based_table_factory.o _build-75a980e50-I 4ed7.derive_syn_rocksdb/include/ (http block)
• https://api.github.com/repos/gear-tech/wasm-instrument/commits/v0.4.0-sign-ext
  • Triggering command: /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo rustc --target=wasm32v1-none --color=always --manifest-path=/home/REDACTED/work/gear/gear/target/debug/build/demo-ping-d8cd3400f06c158b/out/Cargo.toml --profile dev --features ethexe -- -Clink-arg=--import-memory -C link-arg=--mllvm=-mcpu=mvp -C link-arg=--mllvm=-mattr=+mutable-globals -C linker-plugin-lto ibrocksdb-sys-14-m64 -fno-omit-frame-/tmp/rustcZzbzk4/symbols.o as (http block)
• https://api.github.com/repos/gear-tech/wasm-tools/commits/gear-stable-1.230
  • Triggering command: /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo rustc --target=wasm32v1-none --color=always --manifest-path=/home/REDACTED/work/gear/gear/target/debug/build/demo-ping-d8cd3400f06c158b/out/Cargo.toml --profile dev --features ethexe -- -Clink-arg=--import-memory -C link-arg=--mllvm=-mcpu=mvp -C link-arg=--mllvm=-mattr=+mutable-globals -C linker-plugin-lto ibrocksdb-sys-14-m64 -fno-omit-frame-/tmp/rustcZzbzk4/symbols.o as (http block)
• https://api.github.com/repos/gear-tech/wasmi/commits/v0.13.2-sign-ext
  • Triggering command: /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo metadata --format-version 1 --manifest-path /home/REDACTED/work/gear/gear/target/debug/wbuild/ethexe-runtime/Cargo.toml rocksdb/third-party/gtest-1.8.1/fused-src/ -I snappy/ -I . -Wall 20-x�� 20-x86_64-unknow-g 20-x86_64-unknow-gdwarf-4 2c9a1cad1328.rli-fno-omit-frame-pointer wind-6d6d5f1558e/usr/libexec/gcc/x86_64-linux-gnu/13/cc1plus ut/053adb4869b9269c-block_based_table_factory.o _build-75a980e50-I 4ed7.derive_syn_rocksdb/include/ (http block)
  • Triggering command: /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo /home/REDACTED/.rustup/toolchains/nightly-2025-10-20-x86_64-REDACTED-linux-gnu/bin/cargo rustc --target=wasm32v1-none --color=always --manifest-path=/home/REDACTED/work/gear/gear/target/debug/build/demo-ping-d8cd3400f06c158b/out/Cargo.toml --profile dev --features ethexe -- -Clink-arg=--import-memory -C link-arg=--mllvm=-mcpu=mvp -C link-arg=--mllvm=-mattr=+mutable-globals -C linker-plugin-lto ibrocksdb-sys-14-m64 -fno-omit-frame-/tmp/rustcZzbzk4/symbols.o as (http block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)