feat(cketh): enqueue a batched sweep from the sweep queue - #11258
feat(cketh): enqueue a batched sweep from the sweep queue#11258gregorydemay wants to merge 12 commits into
Conversation
ab82a18 to
330c4a2
Compare
6344a8d to
ce8f921
Compare
330c4a2 to
0c311b6
Compare
ce8f921 to
21711b3
Compare
0c311b6 to
ecb6556
Compare
21711b3 to
4f2146b
Compare
ecb6556 to
5a6b150
Compare
4f2146b to
32aeffc
Compare
5a6b150 to
0f65e2e
Compare
32aeffc to
e4933e1
Compare
0f65e2e to
7915d8a
Compare
e4933e1 to
68aa462
Compare
7915d8a to
cb49193
Compare
bb73432 to
54fa93a
Compare
6b08117 to
9379178
Compare
54fa93a to
1d54656
Compare
9379178 to
02ba4a5
Compare
56569ec to
fca3bc8
Compare
02ba4a5 to
1aa46a5
Compare
4ee7b66 to
f2c5e04
Compare
86755ae to
c0bd44b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
rs/ethereum/cketh/minter/src/sweep/mod.rs:421
- Rejecting the fixed-size batch can indefinitely starve this token even when a smaller affordable sweep exists.
next_batch_ofselects the same first 20 deposits on every tick; for example, with the 0.15 ETH mainnet ceiling and a 61 gwei max fee, 20 addresses prepay about 0.205 ETH while 14 prepay about 0.145 ETH. This branch therefore repeatedly re-signs authorizations and makes no progress until fees fall. Size the selected prefix to the number that fits the fee ceiling, preferably before signing, instead of rejecting the whole batch.
if request.max_transaction_fee > ceiling {
The sweep enqueue puts the compressed minter at 1_309_035 bytes, past the 1.3 MB cap.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
rs/ethereum/cketh/minter/src/sweep/mod.rs:284
- Finalizing a sweep never debits its gas cost from the balance used by the funding planner.
SweeperFundingAccounting::sweeper_balance_lower_boundonly returns cumulative transfers (state/sweeper_funding.rs:76-82), while finalization only updates the transaction pipeline and deposit queue. After the first top-up reaches its target, the planner therefore continues to see that full balance after every sweep, never funds again, and the sweeper eventually becomes stuck onInsufficientFunds. Record each finalized receipt's effective fee in replayable state and subtract cumulative sweep spend from the lower bound.
transaction_receipt,
| /// check and at most one transfer per address and [`SweepRequest::gas_limit`] stays linear in the | ||
| /// batch — ~3.4M gas at twenty deposits. A batch spanning several tokens would grow as their | ||
| /// product instead, which is why [`sweep_batches_by_token`] never builds one. | ||
| const MAX_DEPOSITS_PER_SWEEP: usize = 20; |
There was a problem hiding this comment.
Doesn't the test in deposit_from_cex_demo.rs show that the sweet spot is 10?
There was a problem hiding this comment.
🤖 It does — done in 8311a24, the cap is now ten.
The measured attested scenarios put a deposit at 98'000 gas alone, 60'943 in a batch of ten and 59'645 in a batch of twenty: the second doubling saves ~2%, which is one transaction's intrinsic 21'000 spread over the extra deposits. Against that, twenty doubles how many deposits a single revert drops — sweepErc20Batch has no per-item error handling, so a failed sweep discards its whole batch until DEFI-2981 — and doubles what one sweep prepays against the sweeper's low-water mark, which is what makes max_sweep_transaction_fee bind for a minter configured with a smaller minimum withdrawal amount.
The constant now carries that reasoning instead of just the shape argument. It also makes the e2e a full-batch test: it already deposits ten per token, so each token's sweep is now exactly at the cap.
The measured attested scenarios put a deposit at 60'943 gas in a batch of ten and 59'645 in a batch of twenty, so the second doubling saves ~2% while doubling both how many deposits one revert drops and what a sweep prepays against the sweeper's low-water mark.
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 #11144 and #11237 have since landed the pipeline that does, with #11258 about to connect it to the sweep queue. 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 — the ETH it moves plus its fee ceiling, which caps every resubmission the pipeline makes for it — and gets back what it did not need when it finalizes: the fee it did not pay, plus the value it did not move if it failed. 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, and the one this stack described in review before sweeping landed. 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 Opus 5 (1M context) <noreply@anthropic.com>
e6e7cf3 to
8311a24
Compare
A token's batch was collected in full and truncated afterwards, so a long queue grew a Vec of every deposit it held only to drop all but the first MAX_DEPOSITS_PER_SWEEP, and the trailing map rebuilt the whole BTreeMap to do it. Stop pushing once a batch is full instead. `next_batch_of` had no other caller, so its rationale moves onto `sweep_batches_by_token`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| request.max_transaction_fee = gas_fee_estimate | ||
| .clone() | ||
| .to_price(request.gas_limit()) | ||
| .max_transaction_fee(); | ||
| let ceiling = read_state(max_sweep_transaction_fee); | ||
| if request.max_transaction_fee > ceiling { |
| request.deposits.len(), | ||
| request.authorizations.len() | ||
| ); | ||
| mutate_state(|s| process_event(s, EventType::AcceptedSweepRequest(request))); |
| /// per-item error handling, so it doubles how many deposits one revert drops (see | ||
| /// [`sweep_batches_by_token`]), and it doubles what a single sweep prepays against the sweeper's | ||
| /// low-water mark (see [`max_sweep_transaction_fee`]). | ||
| const MAX_DEPOSITS_PER_SWEEP: usize = 10; |
Reading the current time is the one thing in recording an event that needs a canister, which is why `apply_state_transition` is public: no code path calling `process_event` can be unit tested. Add `process_event_at` and `record_event_at`, taking the timestamp as an argument, and let the existing functions delegate to them. Purely additive: no call site changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The delegate, the gas estimate and the creation time were threaded through the enqueue path one argument at a time, and the time was read per sweep even though every sweep of a tick is decided at the same instant. Carry them in one struct instead, read the time once, and record with `process_event_at`, so nothing below `enqueue_batched_sweep` reaches for the clock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signing is the only part of building a sweep that leaves the canister, so put a trait in front of it and make the enqueue path generic over it: a test can then supply the signatures and drive everything the productive code decides with them. `EcdsaSigner` wraps `sign_digest` and nothing else — no business logic and no state mutation behind it, so recording an attestation stays in the code under test rather than moving into the production implementation. `Authorization::sign` and `sign_attestation` take the signer instead of calling the primitive; both had a single caller, and neither is used outside the crate, so both narrow to `pub(crate)`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing covered `enqueue_token_sweep`: which deposits reach the call data, what the sweep is priced at, when it is skipped, and which signatures it spends. Drive it against a mock signer, which sees only the digest and the derivation path, so the tests pin the ones the productive code computes. Covers the happy path, an unconfigured deposit helper, a failed attestation and a failed authorization, an address left unauthorized because it could not be attested, a reused attestation, the fee ceiling, and the deposits leaving the queue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Falling through cost two state reads and logged that the sweep was skipped because none of its zero deposits could be signed for, which reads as a failure in the log of a tick that simply had nothing to do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neither an empty sweep queue nor an unconfigured deposit helper may cost a threshold-ECDSA signature, enqueue a sweep, advance the sweep id, or take anything off the queue. Supersedes the test that covered the unconfigured helper by name only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The hand-rolled fake recorded what it was asked to sign so the tests could assert on it afterwards. Expectations say the same thing up front and say it better: a request nothing expects fails on the spot, so a test asserts what a sweep must not sign by simply not allowing it — an empty mock is the whole assertion for a sweep that must sign nothing. Follows the `mock!` block ckBTC's minter uses for `CanisterRuntime`, though without `async_trait`: mockall handles the native `async fn` in the trait. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…once The one deposit helper and chain every request binds to are read once for the whole batch rather than per account. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
rs/ethereum/cketh/minter/src/sweep/mod.rs:68
- The PR description says batches are capped at 20 deposits, but this implementation deliberately caps them at 10. Please align the documented behavior and implementation so the rollout has an unambiguous batch-size limit.
const MAX_DEPOSITS_PER_SWEEP: usize = 10;
| request.max_transaction_fee = context | ||
| .gas_fee_estimate | ||
| .clone() | ||
| .to_price(request.gas_limit()) | ||
| .max_transaction_fee(); | ||
| let ceiling = read_state(max_sweep_transaction_fee); | ||
| if request.max_transaction_fee > ceiling { |
| /// The sweep queue folded into one batch per token, each holding that token's queued deposits in | ||
| /// queue order, up to [`MAX_DEPOSITS_PER_SWEEP`] of them. | ||
| /// | ||
| /// One sweep per token, never a batch spanning several. The delegate's batch entry point runs its | ||
| /// whole token list against every deposit address it touches, so a mixed batch pays a `balanceOf` at | ||
| /// every `(address, token)` pair — including the pairs holding nothing, which is most of them once | ||
| /// several tokens are supported — and its gas grows as addresses × tokens rather than with the | ||
| /// deposits it moves. Grouping keeps that product equal to the addresses, which is what lets | ||
| /// [`MAX_DEPOSITS_PER_SWEEP`] bound a sweep's gas at all. It costs one transaction's 21'000 | ||
| /// intrinsic gas per token, which a multi-token sweep pays anyway in the cold storage write each | ||
| /// token's balance at the minter needs. | ||
| /// | ||
| /// Every deposit gets one sweep and no more. `sweepErc20Batch` has no per-item error handling, so | ||
| /// whatever makes a sweep revert — one address blacklisted for the token, say — reverts every batch | ||
| /// that deposit is in; a failed sweep therefore drops its whole batch from the queue rather than | ||
| /// leaving anything behind to retry (DEFI-2981). |
There was a problem hiding this comment.
No Rust docs on private method, remove
Why
What