Skip to content

fix[venom]: make truthy or rule wrap-aware#5082

Open
banteg wants to merge 5 commits into
vyperlang:masterfrom
banteg:fix/algebraic-or-truthy
Open

fix[venom]: make truthy or rule wrap-aware#5082
banteg wants to merge 5 commits into
vyperlang:masterfrom
banteg:fix/algebraic-or-truthy

Conversation

@banteg

@banteg banteg commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

What I did

Fixes #5072.

The algebraic optimizer's truthy-position or rule treated an unwrapped literal equal to 2**256 — which is zero as an EVM word — as nonzero, folding or 2**256, %x in an assert position to constant 1 instead of depending on %x.

How I did it

One-line fix in _rule_or: the raw check ops[0].value != 0 becomes wrap-aware not lit_eq(ops[0], 0), consistent with the x | 0 -> x rule directly below it which already uses lit_eq. Audited the rest of the file for the same raw-.value truthiness pattern; this was the only instance.

How to verify it

test_or_truthy_unwrapped_zero_literal parses the issue reproducer, runs AlgebraicOptimizationPass, and asserts the or reduces to %x rather than constant 1. Fails on master.

Commit message

the truthy-position `or` rule checked raw literal truthiness
(`ops[0].value != 0`), so an unwrapped literal equal to 2**256 --
which wraps to zero as an EVM word -- was treated as nonzero and the
expression was folded to constant 1, even though it is equivalent to
`or 0, %x` and must depend on %x.

make the check wrap-aware via the existing `lit_eq` helper, consistent
with the `x | 0 -> x` rule below it.

Description for the changelog

Fix: Venom algebraic optimizer no longer folds or with an unwrapped zero literal (e.g. 2**256) to constant true in truthy positions.

Cute Animal Picture

cute animal

@charles-cooper charles-cooper left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there other bugs of this class in algebraic opts?

banteg commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Audited the rest of algebraic_optimization.py for the same raw-literal vs wrapped-EVM-word class.

Found one adjacent issue in signextend: the byte index operand is also an EVM word, so 2**256 must be treated as byte index 0, not as >= 31. I pushed follow-up commit 49cf212b4 to make the direct signextend(n, x) no-op rule and the nested signextend(n, signextend(m, x)) rule use wrapped literal values, with regressions for both cases.

The remaining raw .value uses in the pass are either immediately normalized with wrap256/lit_eq, or are in the power-of-two strength reduction path after wrapped-zero/one cases have already been handled.

Checked with:

uv run --python 3.12 -m pytest tests/unit/compiler/venom/test_algebraic_binopt.py tests/unit/compiler/venom/test_algebraic_optimizer.py
uv run --python 3.12 ruff check vyper/venom/passes/algebraic_optimization.py tests/unit/compiler/venom/test_algebraic_binopt.py

@banteg

banteg commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

I did a quick sweep of the rest of AlgebraicOptimizationPass for the same raw-literal/wrap class. I do not see another confirmed correctness bug.

The remaining direct literal reads are in the power-of-two strength-reduction probe for mul/div/mod. The wrap-zero aliases are already caught by the preceding lit_eq(..., 0) rule; non-canonical aliases like a literal that wraps to 2 but is not syntactically a power of two would just miss the strength reduction rather than produce a wrong rewrite. The comparator and signextend paths now go through wrap256/lit_word_value where the literal value affects correctness.

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.84%. Comparing base (242a4fa) to head (49cf212).
⚠️ Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
vyper/venom/passes/algebraic_optimization.py 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5082      +/-   ##
==========================================
- Coverage   92.93%   92.84%   -0.10%     
==========================================
  Files         188      191       +3     
  Lines       27840    29385    +1545     
  Branches     4831     5151     +320     
==========================================
+ Hits        25874    27282    +1408     
- Misses       1315     1411      +96     
- Partials      651      692      +41     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

Copy link
Copy Markdown

Gas Changes

No changes detected.

Summary

  • Total tests measured: 815
  • Changed: 0
  • Regressions (gas up): 0
  • Improvements (gas down): 0
  • New tests: 0
  • Deleted tests: 0
  • Newly failing: 0
  • Newly passing: 0

@github-actions

github-actions Bot commented Jun 16, 2026

Copy link
Copy Markdown

📊 Bytecode Size Changes (venom)

No changes detected.

Full bytecode sizes

Contract legacy-O2 legacy-Os -O2 -O3 -Os
curvefi/amm/stableswap/meta_implementation/meta_implementation_v_700.vy 23610 22805 20786 19755 19315
curvefi/legacy/CurveStableSwapMetaNG.vy 24952 23578 20490 19871 19175
curvefi/amm/stableswap/implementation/implementation_v_700.vy 24962 23769 20264 19393 18845
curvefi/legacy/CurveStableSwapNG.vy 24473 23298 19677 18844 18314
curvefi/amm/tricryptoswap/implementation/implementation_v_200.vy 20724 19959 18020 17518 16961
yearnfi/VaultV3.vy 19972 19063 17017 15131 14477
curvefi/amm/twocryptoswap/implementation/implementation_v_210.vy 17634 16894 15685 15132 14648
curvefi/legacy/CurveCryptoSwap2.vy 18947 18382 15634 15092 14711
yearnfi/VaultV2.vy 16676 15763 13863 13405 12574
curvefi/amm/stableswap/factory/factory_v_100.vy 14558 13978 13317 11825 11788
curvefi/gauge/child_gauge/implementation/implementation_v_110.vy 12338 11561 10270 9775 9182
curvefi/gauge/child_gauge/implementation/implementation_v_100.vy 12017 11249 9998 9502 8920
curvefi/amm/stableswap/views/views_v_120.vy 12784 12368 9970 9413 9532
curvefi/amm/tricryptoswap/math/math_v_200.vy 11189 11126 9520 8162 8424
curvefi/legacy/CurveCryptoMathOptimized3.vy 11188 11125 9519 8162 8424
curvefi/gauge/child_gauge/implementation/implementation_v_020.vy 10665 9947 9013 8565 8028
curvefi/helpers/router/router_v_110.vy 6717 6717 7003 6376 6607
curvefi/amm/tricryptoswap/views/views_v_200.vy 7821 7776 6697 6445 6514
curvefi/registries/metaregistry/metaregistry_v_110.vy 7590 6732 6651 5888 5727
curvefi/helpers/stable_swap_meta_zap/stable_swap_meta_zap_v_100.vy 7302 7067 6379 6012 6114
curvefi/amm/twocryptoswap/views/views_v_200.vy 6991 6946 6272 6026 6089
curvefi/amm/twocryptoswap/factory/factory_v_200.vy 5540 5252 5994 4697 4845
curvefi/amm/tricryptoswap/factory/factory_v_200.vy 5246 5021 5923 4926 5039
curvefi/registries/metaregistry/registry_handlers/stableswap/handler_v_110.vy 6633 6259 5859 5094 5562
curvefi/amm/twocryptoswap/math/math_v_210.vy 6800 6800 5581 5107 5112
curvefi/gauge/child_gauge/factory/factory_v_201.vy 4844 4547 4266 4014 3780
yearnfi/VaultFactory.vy 3765 3617 3985 2505 2965
curvefi/registries/metaregistry/registry_handlers/tricryptoswap/handler_v_110.vy 4241 3939 3838 3547 3547
curvefi/registries/metaregistry/registry_handlers/twocryptoswap/handler_v_110.vy 4186 3884 3777 3417 3433
curvefi/gauge/child_gauge/factory/factory_v_100.vy 4183 3914 3694 3428 3213
curvefi/helpers/rate_provider/rate_provider_v_101.vy 3260 3260 2779 2437 2450
curvefi/registries/address_provider/address_provider_v_201.vy 2973 2782 2713 2569 2424
curvefi/amm/stableswap/math/math_v_100.vy 3067 3046 2598 2403 2423
curvefi/helpers/rate_provider/rate_provider_v_100.vy 2847 2841 2357 2050 2058
curvefi/helpers/deposit_and_stake_zap/deposit_and_stake_zap_v_100.vy 2322 2316 2123 1889 1921
curvefi/governance/relayer/taiko/relayer_v_001.vy 2068 2064 1784 1579 1612
curvefi/governance/relayer/polygon_cdk/relayer_v_101.vy 1556 1523 1587 1405 1406
curvefi/governance/relayer/arb_orbit/relayer_v_101.vy 1266 1262 1280 1124 1155
curvefi/governance/relayer/op_stack/relayer_v_101.vy 1186 1182 1219 1066 1094
curvefi/governance/relayer/not_rollup/relayer_v_100.vy 1168 1153 1208 1061 1073
curvefi/governance/vault/vault_v_100.vy 964 941 885 866 864
curvefi/governance/relayer/relayer_v_100.vy 496 496 600 505 510
curvefi/governance/agent/agent_v_100.vy 541 541 443 415 419
curvefi/governance/agent/agent_v_101.vy 541 541 443 415 419

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Algebraic optimizer treats unwrapped zero literal as truthy in or

3 participants