test(cketh): [experiment] strip added fixture helpers to expose implicit-mock dependencies

[gregorydemay]

Analysis instrument, not intended for merge as-is. PR #10955 migrated the cketh integration tests from StateMachine to PocketIC and, alongside the mechanical port, added extra fixture machinery with no counterpart in the pre-migration StateMachine fixture: drain_startup_http_outcalls, drain_stray_latest_block_refresh_calls, is_latest_block_refresh, the OutcallPolicy/await_call_draining_outcalls split, and find_rpc_call_retrying's retry/jump-forward behavior. The hypothesis under test: some of the tests this machinery was propping up could instead be adapted with explicit mocks that represent real productive scenarios (e.g. explicitly stubbing the minter's periodic latest-block-refresh JSON-RPC calls), keeping the fixture lean and closer to the original.

This branch removes that machinery, leaves the crate compiling cleanly, and lets the affected integration tests fail on purpose so the failures can be cataloged and reviewed before deciding whether to reintroduce fixture machinery or add targeted explicit mocks per test. Tests are intentionally red — do not fix them here.

Kept vs removed

Removed (no counterpart in the pre-migration StateMachine fixture):

• drain_startup_http_outcalls and its call in CkEthSetup::new
• drain_stray_latest_block_refresh_calls and its call in upgrade_minter
• is_latest_block_refresh (mock.rs) and its uses in JsonRpcRequestMatcher::matches's fallback and the request-polling loop
• The OutcallPolicy enum, poll_for_call, await_call, await_call_draining_outcalls — call sites now use the real PocketIc::await_call (per the API mapping await_ingress(id, MAX_TICKS) → await_call(id)) or the simplified stop_ongoing_https_outcalls
• find_rpc_call_retrying's retry/jump-forward loop — expect_rpc_call now uses the plain find_rpc_call
• stop_ongoing_https_outcalls's retry loop and its drain_pending_https_outcalls helper — reduced to the original's single reply-500-then-move-on pass
• The now-unused REJECT_CODE_SYS_TRANSIENT const and reject_stray_http_outcall helper

Kept (straightforward API-mapping ports, not the target machinery):

• update_call/query_call wrappers, canister create/install/cycles plumbing
• submit_stop_minter (the plain submit_call_with_effective_principal port of stop_canister_non_blocking)
• try_stop_minter_without_stopping_ongoing_https_outcalls (a direct, non-machinery port)
• find_rpc_call's plain 10-tick tick_until_next_http_request poll, matching the original
• reply_500 and REJECT_CODE_SYS_FATAL (used by the size-limit-exceeded response path, itself a direct port)

Failure catalog

Verification: cargo check clean, cargo clippy (project's pinned invocation) clean, //rs/ethereum/cketh/test_utils:lib_tests passes. Both integration suites below are intentionally red.

//rs/ethereum/cketh/minter:integration_tests_tests/cketh_test — 3 passed / 23 failed

All 23 failures panic at the same site, test_utils/src/mock.rs:179 inside StubOnce::expect_rpc_call, with "no request found matching the stub ... EthGetBlockByNumber ...":

should_block_withdrawal_to_blocked_address, should_mint_when_1_error_with_3_out_of_4_strategy, should_block_deposit_from_blocked_address, should_not_overlap_when_scrapping_logs, should_not_finalize_transaction_when_receipts_do_not_match, should_fail_to_withdraw_too_small_amount, cketh_evm_rpc::should_retrieve_block_number, should_fail_to_withdraw_when_insufficient_funds, should_not_send_eth_transaction_when_fee_history_inconsistent, should_deposit_and_withdraw, should_not_mint_when_logs_too_inconsistent, should_half_range_of_scrapped_logs_when_response_over_two_mega_bytes, should_fail_to_withdraw_without_approval, should_be_able_to_stop_canister_during_scraping, should_panic_when_last_finalized_block_in_the_past, should_resubmit_new_transaction_when_price_increased, should_reimburse, should_resubmit_transaction_as_is_when_price_still_actual, should_retry_from_same_block_when_scrapping_fails, should_scrap_one_block_when_at_boundary_with_last_finalized_block, should_retrieve_minter_info, should_retrieve_cache_transaction_price, should_skip_single_block_containing_too_many_events.

Root-cause hypothesis: the minter runs a periodic timer (REFRESH_LATEST_BLOCK_HEIGHT_INTERVAL) that issues its own eth_getBlockByNumber("latest") JSON-RPC outcall, unrelated to log scraping, purely to refresh a metric. With is_latest_block_refresh removed, JsonRpcRequestMatcher::matches no longer excludes this call from an unconstrained stub (match_request_params: None now falls back to true for any request, matching the original StateMachine-era behavior). Most of these tests build a generic, unparameterized EthGetBlockByNumber stub meant to answer the scrape-triggered call; that stub can instead match and consume the unrelated refresh call, or the refresh call can occupy the outcall slot long enough that the real scrape call never appears inside the fixture's fixed MAX_TICKS = 10 polling budget, so expect_rpc_call gives up. should_be_able_to_stop_canister_during_scraping is the one exception with an explicit ["finalized", false] stub — there the refresh call (always ["latest", false]) can't be matched by the stub directly, so the same periodic call is instead starving the finite poll window rather than being consumed by it.

Sketch of an explicit-mock fix: before triggering a flow that depends on a scrape-triggered EthGetBlockByNumber stub, add a dedicated MockJsonRpcProviders::when(EthGetBlockByNumber).with_request_params(json!(["latest", false])).respond_for_all_with(...) stub for the refresh call, consumed up front (mirroring the two tests that already do this explicitly at tests/cketh.rs:842 and tests/ckerc20.rs:258), so the scrape-only stub can no longer collide with it.

//rs/ethereum/cketh/minter:integration_tests_tests/ckerc20_test — 3 passed / 37 failed

All 37 failures panic identically at packages/pocket-ic/src/nonblocking.rs:1719, with (400, "BadIngressMessage(\"Failed to answer to ingress ... after 100 rounds.\")"):

deposit_erc20::should_not_record_one_event_per_registered_deposit_address, deposit_erc20::should_record_address_to_deposit, deposit_erc20::should_trap_when_called_from_anonymous_principal, deposit_erc20::should_update_latest_block_height_and_never_decrease, should_add_ckusdc_and_ckusdt_to_minter_via_orchestrator, should_block_deposit_from_blocked_address, should_block_deposit_from_corrupted_principal, should_deposit_ckerc20, should_deposit_cketh_and_ckerc20, should_deposit_cketh_and_ckerc20_when_ledger_temporary_offline, should_fail_to_mint_from_unsupported_erc20_contract_address, should_mint_with_ckerc20_setup, should_not_change_address_of_other_contracts_when_adding_new_contract, should_not_scrape_when_no_erc20_token, should_retrieve_minter_info, should_retry_to_add_usdc_when_minter_stopped, should_scrape_from_last_scraped_after_upgrade, should_skip_single_block_containing_too_many_events, and all 19 withdraw_erc20::* tests.

Root-cause hypothesis: CkErc20Setup::new calls upgrade_minter_to_add_orchestrator_id then upgrade_minter_to_add_erc20_helper_contract, each of which calls stop_minter. stop_minter submits stop_canister, does a single reply-500-then-move-on pass over whatever HTTP outcalls are pending right now (stop_ongoing_https_outcalls, matching the original StateMachine fixture's single-payload behavior), then awaits completion via the real PocketIc::await_call. On PocketIC, if the block-height refresh timer re-fires a fresh outcall in the gap between that single reply pass and the canister settling, that new call is never answered, so the minter's stop_canister call stays blocked on it indefinitely; PocketIC's own await_call gives up after 100 rounds and the sync client panics client-side. Since almost every ckerc20 test builds its fixture through CkErc20Setup::default()/::new(), essentially all of them hit this during setup.

Sketch of an explicit-mock fix: either (a) explicitly stub the refresh call's response before/around each stop_minter invocation during setup so it never becomes a fresh unanswered outcall, or (b) if the retry-until-stable behavior genuinely reflects a real operational scenario (the minter's timers keep firing while it's asked to stop), keep a bounded, explicitly-scoped retry in stop_ongoing_https_outcalls itself rather than reaching for the more general OutcallPolicy mechanism — a call this task leaves to Greg's review of the tradeoff.

🤖 Generated with Claude Code

[gregorydemay]

🤖🧐 🟠 Removing drain_startup_http_outcalls is the first-order cause of all 23 cketh_test failures, and it is forced by a real harness difference rather than being optional machinery.

Probe evidence (should_retrieve_minter_info): at this point 8 outcalls are pending and never answered — ids 0-3 ["finalized"] (startup scrape) and ids 4-7 ["latest"] (startup refresh). The first advance_time(SCRAPING_ETH_LOGS_INTERVAL) (180 s) crosses CANISTER_HTTP_TIMEOUT_INTERVAL = 60 s (rs/types/types/src/canister_http.rs:80), so Provider1's stub answers a stale startup request, and the very next tick() times out the other 7 at once. Provider2's stub then sees an empty list — hence Current requests is empty in every one of the 23 panics, which refutes the "refresh call occupies the slot" hypothesis.

ic-state-machine-tests never times these out, so on StateMachine the 8 contexts sat there forever in CallbackId order and the stubs deterministically consumed the 4 ["finalized"] ones. That is why the original fixture needed no drain.

Note the drain must reject, not answer: answering with real block responses would set last_observed_block_number, breaking tests/cketh.rs:1201 and tests/ckerc20.rs:1891. Recommend keeping this helper.

[gregorydemay]

🤖🧐 🔴 This single-pass drain cannot work on PocketIC, and this — not "the refresh timer re-fires in the gap" — is what deadlocks all 37 ckerc20_test setups.

Probe of stop_minter inside CkErc20Setup::new (should_not_scrape_when_no_erc20_token):

• 1st stop_minter (add_orchestrator_id): 8 requests pending, drained, settles at round 1. ✓
• upgrade_canister + start_minter → post_upgrade re-runs setup_timers() → 8 new outcalls (ids 8-15: 4 ["finalized"] + 4 ["latest"]) created in that same round.
• 2nd stop_minter: get_canister_http() here logs empty — PocketIC does not surface requests created in a round that has not been ticked. This loop answers nothing, then ticks; only after that tick do all 8 appear.
• The minter is now Stopping holding 8 unanswered outcalls. Ticks advance ~1 ns, so the 60 s http timeout never rescues it either → deadlock → await_call gives up at 100 rounds.

Two corrections to the catalog: it is not a re-fire in a gap (the calls already existed, just invisible), and it is not refresh-specific (half are the scrape's ["finalized"]). Consequence for the decision: explicit per-test mocks cannot fix this cluster — there is nothing to stub at the moment the drain runs, and the path is CkEthSetup::upgrade_minter, reachable only through the fixture.

Empirically, replacing this body with a plain for _ in 0..MAX_TICKS { drain; tick; } alone takes ckerc20_test from 3/40 to 20/40. Recommend keeping a bounded drain loop here (the catalog's option (b)), simplified to an early return on two consecutive empty polls.

[gregorydemay]

🤖🧐 🟠 This unwrap_or(true) restores the original fallback faithfully, but it is the reason the explicit-mock route costs more than the catalog estimates.

Ablation: with the two drains restored but this left as unwrap_or(true), cketh_test is 5/26; restoring is_latest_block_refresh here takes it to 24/26. So this is the highest-leverage single helper in the stripped set.

The blast radius of replacing it with explicit mocks is not "one line in a builder". matches short-circuits here on None → true, and find_rpc_call takes the first match in get_canister_http() order — so an unconstrained scrape stub and an explicit ["latest"] stub are ambiguous by construction: whichever request comes first in the vec wins. Disambiguating means adding .with_request_params(json!(["finalized", false])) to every currently-unconstrained scrape stub — all 19 MockJsonRpcProviders::when(EthGetBlockByNumber) sites (13 in tests/cketh.rs, 5 in tests/ckerc20.rs, plus flow.rs:337 and test_utils/src/ckerc20.rs:784) — i.e. restating in 19 places the invariant this helper states once (G5).

(Ordering itself is not the problem: get_canister_http() preserves creation order — observed ids 0..7 in order — matching StateMachine's BTreeMap<CallbackId, _> iteration.)

Suggested middle ground that keeps Greg's "explicit mock" instinct without the 19-site edit: default the builder's params to ["finalized", false] (or add a for_scrape()), so the intent lives on the stub and this fallback disappears entirely.

[gregorydemay]

🤖🧐 🔵 On the removed find_rpc_call_retrying: its premise is real — the minter log does emit [ic-cdk-timers] canister_global_timer: too many concurrent calls for single timer (5), rescheduling for next possible execution time, exactly as its comment claimed. So it was not invented.

But it is also the piece the failure data justifies least: once the startup drain, the stop-drain loop, and the is_latest_block_refresh exclusion are in place, cketh_test is already at 24/26 with this helper absent. And it burns up to 180 s of simulated time on every lookup — a large hidden side effect for something named find_*, which its own comment concedes makes it unusable for negative expectations.

Same recommendation as for OutcallPolicy: re-run #10955 without it and keep it only for the specific tests that then fail, as a named list rather than a blanket policy in expect_rpc_call.

[gregorydemay]

🤖🧐 ANALYSIS: PARTIALLY CONFIRMED — counts and per-test lists reproduce exactly; both root-cause mechanisms are wrong. Net: the two drains are forced by real PocketIC-vs-StateMachine semantic gaps and must stay; is_latest_block_refresh is worth keeping as a DRY encoding (possibly re-expressed as an explicit builder default); OutcallPolicy and find_rpc_call_retrying are the two pieces the failure data does not justify — re-measure them.

Evidence, ablation table, and per-cluster analysis

1. Was the strip faithful? — YES

Checked line-by-line against git show ic_DEFI-2262_2_cketh-prep:rs/ethereum/cketh/test_utils/{lib,mock}.rs. tick_until_next_http_request, find_rpc_call, matches's unwrap_or(true), stop_ongoing_https_outcalls's single reply-500 pass, stop_minter's submit→drain→await, and CkEthSetup::new without a drain all reproduce the pre-migration fixture exactly. Every removed item genuinely has no StateMachine counterpart. No plain port was collateral damage, no test body changed, Cargo Build/Cargo Lint green. The failure data is not contaminated.

Two fidelity footnotes (neither can manufacture failures):

• crate::await_call(env, id) → env.await_call(id) is more generous than the original await_ingress(id, MAX_TICKS = 10) (pocket-ic caps at 100 rounds). Deviation is in the permissive direction, and it is the plan's documented mapping.
• tests/ckerc20.rs:113-115's comment — "stop_ongoing_https_outcalls above guarantees no outcall is left pending (it panics otherwise)" — is now false on this branch, since the strip removed the panic. Harmless for an experiment; must be reconciled if any of this survives.

2. Counts — reproduced exactly

• cketh_test: 3 passed / 23 failed. The 23 names match the catalog 1:1.
• ckerc20_test: 3 passed / 37 failed. One wording slip: "all 19 withdraw_erc20::* tests" is 19 of 20 — withdraw_erc20::should_trap_when_ckerc20_feature_not_active passes.
• Base test(cketh): migrate integration tests to PocketIC #10955 CI is green, so the machinery does work in aggregate.

3. Cluster A (cketh_test) — mechanism REFUTED, effect real but second-order

Catalog: the unconstrained EthGetBlockByNumber stub swallows the refresh call, or the refresh call starves the 10-tick window.

Instrumented run of should_retrieve_minter_info (temporary probe in expect_rpc_calls, reverted):

1. After CkEthSetup::new, 8 outcalls are pending and never answered — ids 0-3 ["finalized"] (startup scrape) and ids 4-7 ["latest"] (startup refresh).
2. handle_deposit's advance_time(SCRAPING_ETH_LOGS_INTERVAL) = 180 s pushes simulated time past CANISTER_HTTP_TIMEOUT_INTERVAL = 60 s (rs/types/types/src/canister_http.rs:80).
3. Provider1's stub matches and answers id 0 — a stale startup request, not a fresh scrape one — then env.tick(). That one tick makes consensus time out all 7 remaining requests at once (Canister http request timed out).
4. Provider2's stub then sees an empty pending set; 10 ticks × 1 ns produce nothing. Panic.

Current requests is empty in all 23 failures — which is flatly inconsistent with "the refresh call is occupying the slot". The first-order cause is that PocketIC times out unanswered canister-http requests and ic-state-machine-tests does not. On StateMachine those 8 contexts sat in canister_http_request_contexts() forever, ordered by CallbackId, so the 4 stubs deterministically consumed the 4 ["finalized"] ones and the 4 ["latest"] ones were simply never touched — which is exactly why the original needed neither a startup drain nor is_latest_block_refresh.

The catalog's refresh-collision effect is real, just downstream. Ablation (each variant run against both suites, then reverted):

variant	cketh_test	ckerc20_test
PR as-is	3 / 26	3 / 40
+ startup drain + drain-loop in stop_ongoing_https_outcalls	5 / 26	20 / 40
+ also restore is_latest_block_refresh matcher exclusion	24 / 26	20 / 40

Also confirmed genuine: the minter log emits [ic-cdk-timers] canister_global_timer: too many concurrent calls for single timer (5), rescheduling for next possible execution time — the exact effect find_rpc_call_retrying's comment describes. That premise was not invented.

4. Cluster B (ckerc20_test) — mechanism REFUTED, conclusion upheld

Catalog: the refresh timer re-fires a fresh outcall in the gap between the single reply pass and the canister settling.

Instrumented stop_minter on should_not_scrape_when_no_erc20_token:

1. 1st stop_minter (add_orchestrator_id): drain sees 8 pending, answers them, settles at round 1. ✓
2. upgrade_canister + start_minter → post_upgrade re-runs setup_timers() → 8 new outcalls (ids 8-15: 4 ["finalized"] + 4 ["latest"]) created in that same round.
3. 2nd stop_minter: the probe logs before drain, pending: (empty) — get_canister_http() does not surface requests created in a round that has not been ticked. The drain answers nothing, then ticks; only after that tick do all 8 appear.
4. Minter goes Stopping holding 8 unanswered outcalls. Nothing advances time (ticks are ~1 ns), so the 60 s timeout never fires either. Deadlock; await_call gives up at 100 rounds.

So it is not a re-fire in a gap and not refresh-specific — half the stuck calls are the scrape's ["finalized"]. It is a one-tick visibility lag in get_canister_http() meeting a single-shot drain. Explicit per-test mocks cannot reach it: there is nothing to stub at the moment the drain runs, and the path is CkEthSetup::upgrade_minter, reachable only through the fixture. Recommendation (b) in the catalog is right; its stated reason is not. A 3-line unconditional drain+tick loop alone takes ckerc20 from 3 to 20 passing.

5. Decision guidance

Can cluster A be fixed with explicit per-test mocks? Partly — but it is per-test surgery, not a one-line builder addition, and it does not remove the need for the startup drain.

• Blast radius: 19 MockJsonRpcProviders::when(EthGetBlockByNumber) sites (13 in tests/cketh.rs, 5 in tests/ckerc20.rs, plus flow.rs:337 and test_utils/src/ckerc20.rs:784), each needing a companion stub placed at the right point in the consumption order.
• Matcher semantics do not let an unconstrained stub coexist unambiguously with an explicit ["latest"] one: matches short-circuits on match_request_params: None → true, and find_rpc_call takes the first match in get_canister_http() order. Making the explicit-mock route safe means also adding .with_request_params(json!(["finalized", false])) to every currently-unconstrained scrape stub — touching all 19 sites, to encode in 19 places the one invariant is_latest_block_refresh states once (G5).
• Ordering itself is fine: get_canister_http() preserves creation order (observed ids 0..7 in order), matching StateMachine's BTreeMap<CallbackId, _> iteration. The ambiguity is purely about params.
• The "productive scenario" argument is legitimate — tests/cketh.rs:836-845 already stubs ["finalized"] and ["latest"] explicitly, and did so before the migration. But that very test, should_be_able_to_stop_canister_during_scraping, is among the 23 failures and dies at its ["finalized"] Provider2 stub, before ever reaching its ["latest"] stub. It is the catalog's own proposed pattern, already applied, still failing — explicit mocks are necessary-but-not-sufficient there.
• Explicit stubs cannot replace the startup drain at all: answering the startup calls with real block responses sets last_observed_block_number, breaking tests/cketh.rs:1201 and tests/ckerc20.rs:1891, which assert last_observed_block_number: None. Rejecting them — with the same SysTransient / "Canister http request timed out" reject PocketIC would itself produce 60 s later — is the honest translation.

Net recommendation (hybrid).

Keep, as forced by PocketIC semantics:

• startup drain in CkEthSetup::new — forced by CANISTER_HTTP_TIMEOUT_INTERVAL (60 s), which ic-state-machine-tests does not implement. Reject rather than answer, and keep the comment saying why.
• drain-until-quiet loop in stop_ongoing_https_outcalls — forced by the one-tick visibility lag. Worth simplifying to for _ in 0..MAX_TICKS { drain; tick; } with an early return on two consecutive empty polls, and dropping the advance_time(REFRESH_LATEST_BLOCK_HEIGHT_INTERVAL) jump-forward unless it is shown load-bearing.
• is_latest_block_refresh — highest-leverage single helper (cketh 5 → 24). Suggest re-expressing Greg's instinct rather than deleting it: instead of a silent exclusion buried in matches, default the builder's params to ["finalized", false] (or add a for_scrape()), so the intent lives on the stub and the fallback disappears. Same DRY property, explicit at the call site.

Re-measure before keeping (the failure data does not justify these):

• OutcallPolicy / poll_for_call / await_call_draining_outcalls — cluster B is resolved by the stop-drain loop and cluster A by the startup drain + matcher exclusion; neither points at a general await policy. Also note MAX_AWAIT_TICKS = 1000 is a 100× weakening of the original await_ingress(id, MAX_TICKS = 10) bound that used to catch livelocks. Re-run test(cketh): migrate integration tests to PocketIC #10955 with plain env.await_call everywhere and keep the split only for the tests that then fail — as a named list, not a blanket policy.
• find_rpc_call_retrying — premise verified real, but it burns up to 180 s of simulated time on every lookup, a large hidden side effect for a "find" helper (its own comment already warns it must not be used for negative expectations). Same treatment: re-measure with the two drains + matcher exclusion in place.

6. What the catalog missed

• CANISTER_HTTP_TIMEOUT_INTERVAL = 60 s and the fact that StateMachine does not implement it — the single biggest semantic gap, and the whole of cluster A.
• get_canister_http()'s one-tick visibility lag — the actual cause of cluster B.
• That should_be_able_to_stop_canister_during_scraping already implements the catalog's own explicit-mock sketch and still fails.

[gregorydemay]

🤖 New commit 5b15aee24d — the failure catalog in the description above is now stale; here is the corrected picture.

What actually creates the dangling genesis outcalls: CkEthSetup::new ended with cketh.minter_address(), the update call backing the MINTER_ADDRESS assertion. Nothing in new() ticks explicitly, but that call is a cross-canister request to the management canister for ecdsa_public_key, so PocketIC executes rounds until it completes — and those rounds are what let ic_cdk_timers fire the minter's genesis scrape and block-height-refresh timers. Instrumented counts:

after-ledger-install:   outcalls=0  time=…005
after-evmrpc-install:   outcalls=0  time=…006
after-minter-install:   outcalls=0  time=…007
before-minter_address:  outcalls=0  time=…007
after-minter_address:   outcalls=8  time=…010

The three installs produce nothing; minter_address() burns ~3 rounds and yields 8 outcalls (4 ["finalized"] + 4 ["latest"]), all stamped at genesis. Every test then inherits requests that cross PocketIC's 60s CANISTER_HTTP_TIMEOUT_INTERVAL the moment it advances time — ic-state-machine-tests never timed out unanswered outcalls, which is why this is new.

Effect of moving the assertion into its own test (should_derive_minter_address), with no fixture machinery reintroduced and no other test touched:

	cketh_test
stripped fixture (previous commit)	3 passed / 23 failed
+ assertion moved out of new()	20 passed / 7 failed (27 incl. the new test)

//rs/ethereum/cketh/test_utils:lib_tests still passes 2/2.

Remaining 7 failures, a genuinely different population needing separate diagnosis: should_retrieve_minter_info, should_deposit_and_withdraw, should_not_overlap_when_scrapping_logs, should_retry_from_same_block_when_scrapping_fails, should_half_range_of_scrapped_logs_when_response_over_two_mega_bytes, should_panic_when_last_finalized_block_in_the_past, should_not_panic_when_evm_rpc_canister_is_stopped.

Note for the design decision: answering the genesis outcalls with explicit per-test mocks is not state-equivalent to the fixture rejecting them — a reply to the ["latest"] query sets last_observed_block_number, which should_retrieve_minter_info asserts is None (cketh.rs:1201).