fix[venom]: guard copy-forwarding against deep-callee gas bombs#4974
Open
harkal wants to merge 6 commits into
Open
fix[venom]: guard copy-forwarding against deep-callee gas bombs#4974harkal wants to merge 6 commits into
harkal wants to merge 6 commits into
Conversation
`CopyForwardingPolicy.should_block_forwarding` previously returned False unconditionally when the mcopy source couldn't be resolved to an alloca (typically a function `param`). That's a soundness gap: if the function gets inlined later, the param resolves to a caller alloca whose liveness now extends across the deep invoke, and the conflict graph can force it to a high address. The quadratic memory-expansion gas this produces can dwarf the forwarding win by many orders of magnitude (observed: ~135M gas on snekmate ERC1155 batch revert paths under branches whose inlining decisions expose this shape). Two changes: - `_callee_reserved_intervals` now walks transitive callees in its pre-allocator fallback. The old local-only walk silently missed deeper allocas — a precision bug that mattered regardless of the unresolved-source branch. - `should_block_forwarding` now estimates the per-invoke memory- expansion penalty even when the source is unresolved, using the copy size as a proxy. Bail only when the penalty exceeds an absolute 1M-gas threshold, so small-frame forwardings still win (hundreds of gas) but ABI-encode-buffer-scale frames (megabytes → millions of gas) are caught. No change in measured gas on the snekmate corpus under master's inlining patterns — the bomb shape is not currently triggered there. The fix is precautionary on master and load-bearing on any branch whose inlining is less aggressive (e.g. feat/venom/dyn_alloca). A `defer-to-post-inline-pass` alternative was evaluated and rejected: it eliminates the magic number but loses ~67 forwarding wins across snekmate because non-inlined functions with `param` sources have `src=None` in both passes, so the post-inline pass can't tell "never-inlined" apart from "will-be-inlined" and conservatively keeps the mcopy.
Gas ChangesNo changes detected. Summary
|
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4974 +/- ##
==========================================
+ Coverage 92.92% 92.94% +0.01%
==========================================
Files 188 188
Lines 27820 27847 +27
Branches 4828 4836 +8
==========================================
+ Hits 25853 25882 +29
+ Misses 1316 1315 -1
+ Partials 651 650 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39a25bde03
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
HodanPlodky
requested changes
May 23, 2026
HodanPlodky
left a comment
HodanPlodky
left a comment
Collaborator
There was a problem hiding this comment.
Just two small things otherwise lgtm
HodanPlodky
approved these changes
May 31, 2026
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
3 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.
What I did
Hardened
CopyForwardingPolicyagainst quadratic memory-expansion gas bombs when the mcopy source operand can't be resolved to an alloca (typically a functionparam). On master alone, no contract in the snekmate corpus currently triggers this shape, but it materializes on branches whose inlining decisions are less aggressive (observed at ~135M gas per call on snekmate ERC1155 batch revert paths).How I did it
Two structural changes in
vyper/venom/passes/copy_forwarding.py:_callee_reserved_intervalsnow walks transitive callees in its pre-allocator fallback. The previous local-only walk silently missed deeper allocas, which is a precision bug independent of the bomb scenario.should_block_forwardingnow has an explicit branch for unresolved sources. It estimates the per-invoke memory-expansion penalty using the copy size as a proxy and bails when the penalty exceeds an absolute 1M-gas threshold. Small-frame forwardings still win (hundreds of gas); ABI-encode-buffer-scale frames (megabytes -> millions of gas) are blocked.An alternative defer all unresolved-source forwarding decisions to the post-inline per-function pass (was evaluated and rejected). It eliminates the magic number but loses ~67 forwarding wins across the snekmate corpus, because non-inlined functions with
paramsources keepsrc=Nonein both passes and the post-inline pass can't tell "never-inlined" apart from "will-be-inlined."How to verify it
Commit message
Description for the changelog
Cute Animal Picture