test(cketh): adversarial end-to-end coverage of sweeper fee funding - #11097
test(cketh): adversarial end-to-end coverage of sweeper fee funding#11097mbjorkqvist wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds adversarial end-to-end coverage ensuring sweeper fee funding fails safely.
Changes:
- Tests empty fee accounts, sufficient sweeper balances, and reverted funding transactions.
- Extends the live harness with dashboard, balance, code, status, and finalization helpers.
- Adds a dedicated long-running Bazel target.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
rs/ethereum/cketh/test_utils/src/sweeper_funding.rs |
Expands live-test utilities. |
rs/ethereum/cketh/minter/tests/sweeper_funding.rs |
Uses dashboard-derived sweeper address. |
rs/ethereum/cketh/minter/tests/sweeper_funding_hardening.rs |
Adds three adversarial scenarios. |
rs/ethereum/cketh/minter/BUILD.bazel |
Registers the long-running test target. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let burn_index = setup | ||
| .in_flight_funding_burn_index() | ||
| .expect("the funding must be in flight once its burn has happened"); |
There was a problem hiding this comment.
Correct, and well spotted — fixed in 2096a99.
The ordering is as you describe: fund_sweeper_address awaits the ledger burn and records AcceptedSweeperFundingRequest only after that call returns, while await_burn watches the ledger's total supply through an external query. So there is a real window in which the supply has dropped and the dashboard still shows no in-flight funding, and reading the index there would panic on an empty row.
It now polls, with the deadline as a named constant because it has to sit between two bounds rather than merely being long enough: generous for an inter-canister hop, but well under the roughly six minutes before the transaction finalizes and clears that row again. Polling indefinitely would have traded this race for the opposite one.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
rs/ethereum/cketh/minter/tests/sweeper_funding_hardening.rs:83
- The no-burn baseline is captured after
upgrade_minter(), which starts the zero-delay funding timer on this live PocketIC instance. The task can therefore complete its ledger burn before these queries run, causing both “unchanged” assertions to compare against an already-debited state. Capture the supply and fee-account balance before re-arming the timer so the test cannot miss the behavior it is intended to reject.
let supply_before = setup.cketh_total_supply();
let fee_account_before = setup.cketh_balance_of(setup.fee_account());
| // zero-delay timer, so a minter that wrongly funded could burn before these queries returned | ||
| // and both assertions below would then compare against an already-debited state — passing | ||
| // precisely when the behaviour they reject had happened. | ||
| let supply_before = setup.cketh_total_supply(); |
There was a problem hiding this comment.
Replying to the suppressed comment about this baseline being captured after upgrade_minter() (no comment node to thread onto, hence a fresh one here).
Right, and a fair catch — fixed in 532989e.
Worth being precise about the failure mode, because it is not a flake in the current code: with a correct minter the sweeper is above the low-water mark, no funding is due, and nothing is burned, so the ordering never bites. It matters for a broken minter — if the post-upgrade check wrongly funded, its burn could land before those two queries returned, and then both "unchanged" assertions would compare against an already-debited state. The test would go green at precisely the moment it was supposed to fail.
The baselines are now read before upgrade_minter(), which is the one point where nothing can yet have happened: the fee account is funded, the sweeper is arranged, and the next scheduled check is a whole interval away. The revert test below already did it in that order — I fixed it there when addressing the in-flight race and did not carry the reasoning back up.
I left the first test as it is: its fee account is empty, so no burn can succeed at all, and its log assertion proves the task ran and declined at the burn rather than never running.
…ity#11060) Part of [DEFI-2933](https://dfinity.atlassian.net/browse/DEFI-2933) (sweeper fee funding), first of a seven-PR stack. ## Why Funding the sweeper address with gas requires knowing how much gas it already holds. The EVM RPC canister exposes no endpoint for a native ETH balance, and its Rust client offers no getter for one, so the minter currently has no way to ask. ## What Reads the balance through the EVM RPC canister's generic JSON-RPC passthrough, which forwards a payload to every provider and agrees on one answer under the configured consensus strategy. That strategy is a threshold of the providers — 3 of 4 on mainnet, 2 of 4 on Sepolia — and it is the only agreement accepted: there is no client-side reduction, so a result the canister reports as inconsistent stays an error rather than being resolved by picking a winner. Because the canister deserializes each response's `result` field, what the minter receives is the quantity itself rather than any surrounding JSON. It is therefore decoded exactly: quotes, padding, leading zeros and sign characters are the provider's own malformation and are rejected rather than repaired. A failed read is an error, never a zero. This is the decision the rest of the stack depends on: confusing "could not read the balance" with "no gas left" would burn ckETH to top up an address that is already funded, which is pure loss. The request builder and the result decoder are pure functions so both sides of that guarantee are pinned directly, including a test asserting that no error input can decode to a zero balance. The route was also proven end to end against a live EVM RPC canister and a local anvil node, reaching 3-of-4 consensus at both `latest` and `finalized`. ## Stack Merge in order; each PR targets the one above it. | # | PR | Status | |---|----|--------| | 1 | Read a native ETH balance via the EVM RPC canister | **this PR** | | 2 | dfinity#11065 — Burn ckETH from the minter's own fee subaccount | ready for review | | 3 | dfinity#11072 — Add the SweeperFunding withdrawal-request variant | ready for review | | 4 | dfinity#11083 — Burn-first accounting for sweeper fee funding | ready for review | | 5 | dfinity#11086 — Sweeper fee-funding task, with an end-to-end test | Copilot re-review pending, CI green incl. long tests | | 6 | dfinity#11094 — Sweeper funding observability and the prepaid-gas gate | open | | 7 | dfinity#11097 — Adversarial end-to-end coverage of sweeper fee funding | open | [DEFI-2933]: https://dfinity.atlassian.net/browse/DEFI-2933?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
4d829c1 to
e5adda2
Compare
Complements the happy-path e2e with the three ways funding can go wrong, each driven through the real pipeline — real ckETH ledger, real EVM RPC canister, tECDSA signature, local anvil: - An empty fee account halts funding: nothing is burned, no ETH leaves the main address, and the minter reports why: if the fee account cannot cover a sweep, no sweep is submitted. - A sweeper already above the low-water mark is left alone, so no ckETH is burned for gas that is already in place — the failure mode a wrongly-defaulted balance read would cause. - A funding transaction that fails on chain is NOT reimbursed: the supply stays reduced and the burn is tracked as prepaid gas. This is the central promise, and it is now demonstrated end to end rather than only against a mocked receipt. The last one has to be arranged, because it is otherwise unreachable: a bare value transfer to a code-less EOA cannot fail, which is exactly why the sweeper is an EOA. Placing `PUSH1 0, PUSH1 0, REVERT` at the sweeper address leaves the transfer's 21'000 base gas with nothing to execute the code, so it fails. Two of the three are bounded *negative* assertions — "the minter must not do X" cannot be proven outright, so they watch for longer than a withdrawal-timer tick, which is the practical substitute. That makes the target slow on purpose; if nine minutes per CI run is not worth it, these are the natural candidates for a nightly-only tag. No live fee-spike test: the ceiling is pinned precisely by `should_cap_resubmission_at_the_funded_amount` and `should_fail_to_create_a_transaction_when_the_fee_exceeds_the_funded_amount`, and reproducing it live would mean driving anvil's base fee up and waiting out several timer ticks for little extra signal. Recorded in the test file so the omission is deliberate rather than forgotten. The harness now reads the sweeper address from the dashboard instead of scraping the funding task's log line. That is what makes the "already topped up" scenario possible — the address is needed before the task acts — and it exercises the dashboard section as a side effect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The burn index was read once, immediately after the burn was observed. The minter records the funding only after the ledger call it awaited returns, and the burn is observed through the ledger's total supply — an external query — so there is a window where the supply has dropped and the dashboard still shows no in-flight funding. Reading there panicked on an empty row. Polled instead, with a deadline that has to sit between two bounds: generous enough for an inter-canister hop, and well under the six minutes before the transaction finalizes and clears the row again. Also drops a doc line on `await_funding_finalized` that described a main address accessor, copied from another method.
The baseline was read after `upgrade_minter()`, which re-arms a zero-delay funding check. With a correct minter nothing is due and the ordering never bites, but a minter that wrongly funded could burn before those two queries returned — and then "supply unchanged" and "fee account unchanged" would both compare against an already-debited state. The test would pass at exactly the moment it was meant to fail. Read before the upgrade instead, the one point where nothing can have happened yet: the fee account is funded, the sweeper is arranged, and the next scheduled check is a whole interval away. The revert test below already did it in this order.
The dedicated Failed status was reverted after review: it meant adding a variant to retrieve_eth_status' return type, breaking existing clients, to describe a state mainnet cannot reach — a bare transfer to a code-less address the minter's own key controls has nothing to revert in. This test reaches it anyway, by placing reverting code at the sweeper, so it now expects the pending-reimbursement status and says why that label is tolerated. What it asserts either way is the invariant: no reimbursement, and the burn stays as prepaid gas. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
532989e to
b48a599
Compare
Part of DEFI-2933 (sweeper fee funding), last of a seven-PR stack. Targets #11094.
Why
The stack's existing live test proves funding works. This one proves it fails safe. Each test drives a distinct way funding can go wrong through the real pipeline — real ckETH ledger, real EVM RPC canister, local anvil, threshold-ECDSA signature — and asserts the invariant the feature exists to protect: ETH never moves ahead of the ckETH burned to pay for it.
What
Three adversarial scenarios:
No live fee-spike test: that ceiling is pinned exactly by unit tests, and reproducing it here would mean driving anvil's base fee up over several ticks for little extra signal.
What it found
Waiting for finalization — rather than asserting mid-flight, as the earlier live test did — surfaced a production defect that would trap the withdrawal timer permanently. The fix ships in #11086, where it belongs; this PR is what made it visible.
Harness work this required
Two things had to change before these tests could run at all, both also in #11086:
anvil_setBalancemoves ETH the minter's accounting has never seen. The harness now emits a genuineReceivedEthlog for the minter to scrape, so its ETH balance counter is backed the way it is in production.PocketIc::newhard-codes a 600-second hard TTL, after which the server exits unconditionally — mid-request, ignoring in-flight work, with the budget shared across the whole test binary. Any suite past ten minutes has its backend shot out from under it, and every client then panics withConnection reset by peer. The harness starts its own server with a TTL above the Bazel timeout.Stack
Merge in order; each PR targets the one above it.