fix(cketh): provision sweep gas out of the sweeper balance bound - #11342
fix(cketh): provision sweep gas out of the sweeper balance bound#11342gregorydemay wants to merge 3 commits into
Conversation
gregorydemay
changed the title
There was a problem hiding this comment.
Pull request overview
Updates ckETH sweeper funding accounting so in-flight and finalized sweep costs correctly reduce the tracked balance lower bound.
Changes:
- Provision accepted sweeps against the sweeper balance bound.
- Refund unused fees and failed transfer values upon finalization.
- Add lifecycle tests, shared fixtures, and updated documentation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
rs/ethereum/cketh/minter/src/test_fixtures.rs |
Adds a reusable sweep request fixture. |
rs/ethereum/cketh/minter/src/state/transactions/tests.rs |
Reuses the shared sweep fixture. |
rs/ethereum/cketh/minter/src/state/sweeper_funding/tests.rs |
Tests provisioning, refunds, accounting invariants, and flooring. |
rs/ethereum/cketh/minter/src/state/sweeper_funding.rs |
Tracks provisioned and refunded sweep costs. |
rs/ethereum/cketh/minter/src/state/audit.rs |
Wires accounting into sweep lifecycle events. |
rs/ethereum/cketh/minter/src/state.rs |
Settles sweeper balance accounting at acceptance and finalization. |
rs/ethereum/cketh/docs/deposit_from_cex.md |
Documents the revised lower-bound calculation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
63a865b to
928e0c0
Compare
0651dfc to
452ee12
Compare
| /// Provisioning at acceptance rather than at spend is what keeps this a bound while sweeps are in | ||
| /// flight: gas a committed sweep will pay stops counting as available immediately, and a sweep | ||
| /// whose finalization is never observed leaves the bound too *low* — which delays a funding — | ||
| /// rather than too high, which would let the minter believe in gas that is gone. ETH sent to the |
There was a problem hiding this comment.
🤖 Confirmed against the code — the doc had the direction backwards. SweeperFundingConfig::amount_due returns None only while sweeper_balance >= low_water_mark (state/sweeper_funding.rs:178-187), so a bound that reads too low crosses the mark sooner, and the amount it then asks for is target - sweeper_balance, i.e. larger too. plan_funding's own doc already said so ("Erring low can only make a funding look due sooner than it is, never hide one that is due", sweeper.rs:149-150), so the two were contradicting each other.
Fixed in ee264fb: the bound now reads as bringing a funding forward and making it larger, with the pointer to amount_due. The safety argument is unchanged — erring low is still the safe direction, just for the reason the planner gives.
|
|
||
| /// Records the part of a finalized sweep's provision it did not need, putting it back into the | ||
| /// balance bound. | ||
| pub fn record_finalized_sweep(&mut self, refunded: Wei) { |
There was a problem hiding this comment.
cumulative_sweep_provisioned is only ever offset here, so a sweep that never finalizes keeps its provision permanently.
That's reachable. A sweep's ceiling is priced once at acceptance (sweep/mod.rs:158-161) and never re-priced. Once base + priority exceeds floor(ceiling / gas_limit) — roughly a doubling of the base fee — create_transaction fails and the request is rescheduled indefinitely (sweep/mod.rs:394-402); reschedule_request only moves it within pending_requests, and nothing else takes it out. Not a spike problem, since those mean-revert — the realistic case is a sweep accepted in a quiet trough at ~0.5 gwei, whose ~1.5 gwei ceiling can then sit unaffordable for weeks.
Small per event (~0.005–0.02 ETH at normal gas, against a 0.3 ETH target), but permanent, cumulative, and currently invisible — nothing in sweeper_funding is exported as a metric.
The signal worth alerting on is the age of the oldest unfinalized sweep, not the amount: age is independent of load and gas price, where any wei-denominated gauge moves with the base fee and can't separate "busy" from "leaking". The withdrawal pipeline already has exactly this — oldest_incomplete_request_timestamp() (transactions/mod.rs:1409-1414), exported as cketh_oldest_incomplete_eth_withdrawal_request_age_seconds (main.rs:1313-1321) — and the pieces for the sweep equivalent are already in place: SweepRequest.created_at is public, and the "incomplete" enumeration is the one outstanding_sweeper_funding already uses (transactions/mod.rs:1223-1229).
pub fn oldest_incomplete_sweep_timestamp(&self) -> Option<u64> {
self.incomplete_requests_iter().map(|r| r.created_at).min()
}A wei gauge is still worth having to size the impact on the bound, but under a steady stream it never returns to zero, so it wants a floor comparison (min_over_time(...[24h]) ratcheting upward) rather than a threshold on the level:
/// Provision still held by sweeps that have not finalized.
pub fn outstanding_sweep_provision(&self) -> Wei {
self.cumulative_sweep_provisioned
.checked_sub(self.cumulative_sweep_refunded)
.unwrap_or(Wei::ZERO)
}| /// Drives the already-accepted `request` through the sweeper pipeline to a receipt of `status`, | ||
| /// priced below its ceiling so that part of the provision comes back, and returns the fee that | ||
| /// receipt charged. | ||
| fn finalize(state: &mut State, request: &SweepRequest, status: TransactionStatus) -> Wei { |
There was a problem hiding this comment.
Would it make sense to add a test that, after SignedSweeperTransaction, apply ReplacedSweeperTransaction at a higher price still under the ceiling, finalize on the replacement's receipt, and assert the bound moved by that receipt's fee rather than by two ceilings?
Two smaller gaps:
- No test with two accepted sweeps.
cumulative_sweep_provisionedis a running total, and every test uses one sweep, so nothing would catch an overwrite instead of an add. should_floor_the_bound_at_zero_rather_than_trapnever callsdeliver(), socumulative_transferred == 0and it only proves0 - xsaturates. The case the doc describes - an upgrade that restarts the counters, or a sweeper funded before it was tracked - is a nonzero delivery smaller than the provision. Callingdeliver()with an amount belowrequest.max_transaction_feeexercises that path.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
rs/ethereum/cketh/minter/src/state/automatic_deposits/mod.rs:220
- This literal emits large runs of spaces in the operational log message, making failure diagnostics harder to read. Use Rust line continuations so source wrapping produces single spaces.
"[record_finalized_sweep_transaction]: DROPPING {request:?} from the sweep queue: {id:?} failed and the minter does not retry. Its {:?} stays at {}, and reaching it again needs the pair armed afresh.",
rs/ethereum/cketh/test_utils/src/anvil.rs:316
- Blocks are searched newest-first, but transactions within each block are still searched oldest-first. With interval mining, multiple transactions from this sender can share the head block, so this can return an earlier transaction rather than the documented most recent one. Reverse the per-block iteration too.
for transaction in transactions {
| /// The nonce is always zero, whatever the address actually holds. A deposit address is at | ||
| /// nonce zero exactly while it has never been delegated — applying an authorization spends it | ||
| /// — so the tuple either installs the delegation or is skipped, and both are correct in any | ||
| /// order the sweeps carrying them land. That is what lets a sweep authorize every address it | ||
| /// touches without tracking which ones are already delegated, at the price of the intrinsic | ||
| /// gas a skipped tuple still costs. |
| deposit_helper : text; | ||
| owner : principal; | ||
| subaccount : opt blob; | ||
| y_parity : bool; | ||
| // 32-byte signature components. | ||
| r : blob; | ||
| s : blob; | ||
| // The attestation signed by the deposit address itself. | ||
| attestation : TransactionSignature; |
| // The single ERC-20 contract this sweep moves. | ||
| token : text; | ||
| // The deposits the sweep moves, one per account. | ||
| items : vec AuthorizedSweepItem; |
The funding decision reads a lower bound on the sweeper address' balance, tracked from the minter's own events. Nothing debited it, which was true while nothing spent from that address — and the layers below have since landed the pipeline that does. With those in place the bound only ever grows: fundings top the sweeper up once, sweeps spend the gas, and the bound still reports a full sweeper, so `amount_due` declines every later funding. The sweeper drains, sweeping stalls, and every counter says it is funded. An accepted sweep now provisions the most it can cost that address — its fee ceiling, which caps every resubmission the pipeline makes for it — and gets back the fee it did not pay when it finalizes. An ERC-20 sweep moves its tokens through call data and carries no ETH value of its own, so the fee is the whole of what it can cost, and a reverted sweep is charged for its gas like any other. Provisioning at acceptance rather than debiting at spend is what keeps this a bound while sweeps are in flight. Gas a committed sweep will pay stops counting as available immediately, and a sweep whose finalization is never observed leaves the bound too low — which delays a funding — rather than too high, which would let the minter believe in gas that is gone. It is the discipline the withdrawal pipeline already applies to its own fees. The counters are kept out of `cumulative_spent`: this ETH was counted there once already, when the funding that delivered it finalized, and counting it twice would make spend overtake burn and trip the burn-first invariant. The bound floors at zero rather than trapping, since an upgrade that starts the counters from zero — or a sweeper funded before it was tracked — can legitimately leave provisioning above deliveries, and trapping in a state transition would take the replay of every later event with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YK3ggw1xtE3HZTGGBu66oD
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YK3ggw1xtE3HZTGGBu66oD
…prepaid gas Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YK3ggw1xtE3HZTGGBu66oD
542ceb8 to
a124541
Compare
Why
amount_duedeclines fundings the sweeper actually needs, and sweeping stalls while looking healthy.What
cumulative_spent— that ETH was counted there when the funding delivered it, and counting it twice would trip the burn-first invariant (R14) — and the bound floors at zero rather than trapping, since an upgrade legitimately restarts the counters with sweeps still in flight.This is #11329 (thanks @mbjorkqvist), integrated on top of the sweep stack per the hand-off on DEFI-2933: rebased onto the sweep-queue lifecycle, whose deposit release now lives in the same finalization transition the refund settles in.