Conversation
|
👋 athegaul, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
There was a problem hiding this comment.
AI Review
Review — Pass 1 (original) & Pass 2 (refreshed)
Reviewed against CCIP-12848 (Tempo 30M per-tx gas cap blocking mainnet MCMS signer config update).
Pass 1 (original review, kept for audit)
Various claims were made in the original review. Items struck below have since been withdrawn or downgraded after conversation with the author and on-chain analysis. See Pass 2 for the authoritative version.
1. [Critical] Quorum is not scaled during rollout — the multisig can become unapprovable (or setConfig rejected).
mergePartialRolloutConfig copies desired.Quorum into the merged config verbatim and requires current.Quorum == desired.Quorum. So every intermediate stage keeps the final quorum while holding far fewer signers. MCMS quorum is "number of member signers required." For a 70-signer config with quorum ≈ majority, a stage with only ~20 live signers has quorum > signerCount → no proposal can reach quorum → stuck. Verdict: RETRACTED as a code defect — the "few live signers → can't do batched MCMS changes" condition is the pre-existing operational status quo, not a new hazard introduced by the PR. The PR is the escape (expanding the roster), deliberately run in a window with no pending Tempo requests. It does, however, belong in the PR description as a documented operational constraint (no interleaved ops during the rollout).
2. [Medium] Partial rollout is strictly additive-only, but that's implicit and fragile.
If any on-chain signer isn't in the desired set, it errors, so this can never remove/replace signers, and the failure mode is a hard abort of the whole batch. Verdict: DOWNGRADED — additive-only is intentional and, in fact, load-bearing (see Pass 2 #1). What survives is the ergonomic point: these constraint violations fail mid-apply rather than being validated up front.
3. [Medium] The batch size (20) is a hardcoded gas heuristic, not a measurement. The ticket asks for capping txs at ~29M. 20 isn't derived from any measurement, and PartialRolloutSignerBatchSize isn't referenced at the call site. Verdict: STILL VALID — the most actionable finding. The whole ticket is about gas, yet the batch size is magic-numbered and unchecked against Tempo's 30M cap.
4. [Low] Shared remaining *int is one budget for the whole config tree (all groups + top-level). Verdict: part of the intended behavior — 20 is per-contract per-rollout; worth a comment.
5. [Low] Misplaced constant. PartialRolloutSignerBatchSize lives in deployment/deploy/mcms.go but is consumed in the EVM adapter. Verdict: STILL VALID (cosmetic).
6. [Low] Cross-contract interaction. Each MCM contract's config is merged independently with a fresh budget. Verdict: confirmed correct intent.
Tests. Coverage for happy path, idempotency, order/dup, structural mismatch is good. Missing cases from Pass 1 (quorum > persistent signers, signer-removal) are superseded by Pass 2 — add-only guarantees quorum reachability is monotone, so the "stuck multisig" case isn't reachable; a removal test still documents the (intentional) boundary.
Pass 2 (refreshed, authoritative)
Context confirmed with author: current Tempo roster has only ~4 signers actually live; with so few, no batched MCMS changes can be approved. This PR is the mechanism to expand to the full roster — add-only, in batches of 20, split per contract (3 contracts × ~4 rounds = 12 proposals), executed in a window with no pending Tempo requests.
Verdict: this is a working additive-rollout mechanism, not a broken change. Remaining items, by actionability:
1. [Medium — the one thing to change] Batch size is a hardcoded gas guess and the whole ticket is gas. PartialRolloutSignerBatchSize = 20 is never derived from, or validated against, Tempo's actual per-tx cap. The entire plan ("12 proposals of ~20") rests on the assumption that a ~20-signer setConfig stays under 30M. Recommend: make it per-chain/configurable and validate via a gas estimate, rather than magic-numbering it. This is the highest-value follow-up.
2. [Positive — should be documented] Add-only is load-bearing, not a limitation. Because each rollout only adds signers and never changes quorum, every intermediate live-set can still sign the next proposal; if signers were removed mid-sequence, a middle step could drop below quorum and brick the rollout. This guarantee is unstated and worth a comment.
3. [Low/Medium — note] The 12 proposals are sequential, order-dependent, and not tied together. Nothing links them atomically. A missed/mis-scheduled proposal leaves a partial-but-usable-not-final config, which is hard to recover from with a small live set. Recommend documenting that all 12 must run in order with no interleaved ops (author already plans "no operations that day" — put it in the PR description).
4. [Low — ergonomic] Constraint failures happen mid-apply, not at verification. Removals, quorum changes, and topology changes are rejected by mergePartialRolloutConfig during apply. These are up-front input constraints and should be validated in updateMCMSConfigVerify (deployment/deploy/mcms.go:87) so a bad PartialRollout config fails fast.
5. [Trivial] Constant placement (PartialRolloutSignerBatchSize in deployment/deploy/mcms.go, consumed in the EVM adapter) is cosmetic. The EVM-only guard in the apply path is correct and good.
Open lever (optional): the surviving gas assumption can be settled empirically with cast estimate-gas of a 20-signer vs full-roster setConfig on Tempo. Recommend doing this before relying on the 12×20 plan against the 30M cap.
Overall: logic is sound for its intended use. Address #1 (make batch size gas-aware/verifiable) and capture #2/#3 in the PR description; the rest are nits.
This branch has not been deployed
2 participants
Uh oh!
There was an error while loading. Please reload this page.