ci: gate contract sizes on headroom, not just the limit - #497
Open
pankajjagtapp wants to merge 2 commits into
Open
ci: gate contract sizes on headroom, not just the limit#497pankajjagtapp wants to merge 2 commits into
pankajjagtapp wants to merge 2 commits into
Conversation
forge build --sizes already errors past EIP-170, but only at the limit itself, which is the point where there is nothing left to do. This fails at a configured fraction instead and posts the table on every PR, so margin is visible rather than measured by hand. Fails at 95% of either limit (EIP-170 runtime, EIP-3860 initcode) and warns at 90%. On master the tightest contract is EtherFiNodesManager at 92.5% with 1831 bytes spare; six contracts warn. Only the 44 concrete contracts under src/ are checked -- invariant handlers exceed the limit by design and are never deployed. A contract can exceed the threshold only via an entry in script/ci/contract-size-budget.json carrying a max_runtime_bytes ceiling and a reason. That is a recorded decision, not an exemption: the contract still cannot grow past the ceiling, so the override list doubles as the debt register. Verified all three paths: passes on master, fails a contract at 98.6%, accepts it under an override and still fails when it grows past the override's ceiling. actionlint and zizmor clean, actions pinned to SHAs. Closes STAKE-1928
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5a11ed7. Configure here.
📊 Forge Coverage ReportGenerated by workflow run #807 |
Both Bugbot findings on #497 were real. An initcode-only failure was reported as a runtime one: the Over budget section always printed runtime size against the runtime ceiling and pointed at max_runtime_bytes, which cannot clear an initcode failure. A contract at 60% runtime and 96% initcode produced a message claiming it was over budget on runtime. And warnings only looked at runtime, so a contract could sit above warn_pct of the initcode limit in silence while the report header claimed warnings apply at warn_pct of a limit. Each dimension now carries its own ceiling, override key and pass/fail state. Failure bullets name the failing dimension and the key that would raise it, warnings name the dimension, the summary line reports whichever is tighter, and rows sort by the tighter of the two so an initcode-heavy contract is not buried. Overrides accept max_runtime_bytes, max_init_bytes, or both. Verified: passes on master; an initcode-only failure names initcode and max_init_bytes; a 92% initcode contract now warns; max_init_bytes accepts then ceilings; both dimensions over lists two bullets and both keys; and a max_runtime_bytes override no longer clears an initcode failure.
Contributor
Author
|
Both Bugbot findings were real. Fixed in 86d7ace. The root cause was one shared: runtime and initcode were checked symmetrically for failure but reported and warned as if only runtime existed. Each dimension now carries its own ceiling, override key and pass/fail state. Initcode failures misreported as runtime — the Over budget section always printed runtime size against the runtime ceiling and pointed at Initcode never warned — Two follow-ons the reports made obvious: rows sort by the tighter of the two dimensions, so an initcode-heavy contract is not buried below runtime-heavy ones; and the summary line reports whichever dimension is actually tighter rather than assuming runtime. Verified six paths:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes STAKE-1928.
forge build --sizesalready errors past EIP-170, but only at the limit itself, which is the point where there is nothing left to do. This fails at a configured fraction instead and posts the table on every PR, so margin is visible rather than measured by hand.What it does
run-forge-tests.yaml.src/.ProtocolInvariantsHandler(119% of the limit) andFrozenRateWithdrawalHandler(114%) are test harnesses that are never deployed, so a check over all 129 compiled artifacts would fail permanently.Overrides are a decision, not an exemption
A contract can exceed the threshold only through an entry in
script/ci/contract-size-budget.jsoncarrying amax_runtime_bytesceiling and a reason. The ceiling still binds, so the contract cannot grow further without another deliberate change, and the override list doubles as the debt register. Without this the gate would just be a wall people disable.Current state on master
EtherFiNodesManagerLiquidityPoolEtherFiRedemptionManagerEtherFiRestakerPriorityWithdrawalQueueMembershipManagerV0Six contracts warn, none fail.
Verification
All three paths exercised:
actionlintandzizmorboth clean. All four actions pinned to SHAs withpersist-credentials: false.Note on #485
The ticket's figures (24,035 bytes, 541 spare) are from #485, not master. #485 pushes
EtherFiNodesManagerto roughly 98.6%, so this gate will fail that PR as written — which is the ticket's own next step: reclaim bytes, or add the override entry to record the decision. I have not pre-added one; that call is deliberate and belongs in #485.Since then #485 has reclaimed 235 bytes by dropping the id-based
setProofSubmitteroverload, putting it at 23,986 (97.6%) — still over the 95% threshold.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Note
Low Risk
CI-only addition with no protocol or runtime changes. Risk is limited to false-failing PRs that grow contracts past the new 95% threshold.
Overview
Adds a CI gate that fails when deployable
src/contracts exceed 95% of the EIP-170 runtime or EIP-3860 initcode limits (warns at 90%), so size pressure is visible before bytecode is undeployable.A new workflow builds with
forge build --sizes, checksscript/ci/contract-size-budget.json, and posts/updates a single PR comment with the size table. Only concrete contracts undersrc/are checked (test harnesses are skipped). Per-contractoverridescan raise a still-binding byte ceiling with a recorded reason.Reviewed by Cursor Bugbot for commit 86d7ace. Bugbot is set up for automated code reviews on this repo. Configure here.