Uh oh!
There was an error while loading. Please reload this page.
fix: guard mul() exponent-sum overflow, surface as ExponentOverflow - #244
fix: guard mul() exponent-sum overflow, surface as ExponentOverflow#244thedavidmeister wants to merge 11 commits into
Conversation
Previously, when `pow` used exponentiation-by-squaring with a very large or very negative exponent, repeated squaring of the base could drive the base exponent beyond int256 range, causing a checked addition to panic with Panic(0x11) instead of surfacing the domain-level ExponentOverflow. Added an exact overflow guard to `mul()`: the addition `exponentA + exponentB` overflows int256 only when both operands share the same sign and their sum exceeds int256 bounds. Opposite-sign pairs can never overflow, so the previous per-operand EXPONENT_MAX check was too broad (it incorrectly fired in div round-trip tests where exponents of opposite signs combined safely). Pins `testPowNegativeExponentSquaringPanic` to the exact args, updates deploy constants and ABI artifact for the implementation change. Closes#239 Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughDecimal-float arithmetic now enforces exponent bounds across core operations. Tests cover boundary and overflow behavior, while deployment constants, bytecode artifacts, and the build script naming are updated. ChangesDecimal-float exponent domain
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ce (bytecode changed by mul() exponent-sum overflow fix)
Previous 3b-attempt used wrong jq filter (missing deployedBytecode, had
linkReferences). CopyArtifacts.sol uses:
{abi, bytecode: {object:.bytecode.object}, deployedBytecode: {object:.deployedBytecode.object}}
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>ZOLTU_DEPLOYED_DECIMAL_FLOAT_ADDRESS and DECIMAL_FLOAT_CONTRACT_HASH are now set to the values computed from this PR's bytecode (mul() exponent-sum overflow fix changes source). Computed by testDeployAddress failure output: address: 0xAEBcD4963d328b83f28c5BFDD6C3e6B1692F01fC hash: 0x73f226457d40a7eada41cb774ac4d7736f00bd0e7cb902a6be0ff6c092ca4426 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pow-panic-exponent-overflow
thedavidmeister
commented
Jul 5, 2026
Producer note: WAITING-DEPLOY. Only red is the 5 testProdDeployment* network pins ('DecimalFloat not deployed'); deploy constants already regenerated to this PR's bytecode. Main is red on the same pins. Heads-up for triage: this PR and #242 both fix the #239 pow exponent-overflow bug — one of the two should be picked and the other closed as duplicate. |
thedavidmeister
commented
Jul 6, 2026
🤖 ai:producer |
🤖 ai:vetter |
thedavidmeister
commented
Jul 12, 2026
🤖 ai:producer |
…#244) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thedavidmeister
commented
Jul 14, 2026
Rework note (human): rejecting for rework — the adversarial-mutation pass confirmed (live, in-clone) that this guard does not close #239; it converts only the one pinned negative-squaring repro. 1. The #239 Panic survives. The guard protects only the first add ( 2. Factor-of-2 domain hole. The guard is at the int256 bound, not the library's 3. Inconsistent siblings. Work order: enforce the domain Tests already on this branch (head 3574c20): the adv-mut run pushed 8 mutation-validated tests. 6 pin correct behaviours — keep them. But 2 — |
…ub/inv Wire the previously-dead EXPONENT_MAX/EXPONENT_MIN constants into every arithmetic operator as one shared invariant: nonzero operands outside [EXPONENT_MIN, EXPONENT_MAX] revert ExponentOverflow, and no operation returns a nonzero coefficient at an out-of-domain exponent. - enforceExponentDomain: shared operand/result check, zero-coefficient exempt (zero is zero at any exponent). - mul: operand checks make the exponent sum panic-free; a pre-adjust guard keeps the decimal renormalization from overflowing int256 or the domain; a result check catches the coefficient-rounding +1 and below-domain sums. - div: operand checks ahead of maximization; a result check covers the plain path and the deep-underflow truncation path, whose nonzero int256.min-exponent returns now revert. The sub-1e75 scaling leaves and exponent-spill machinery only engage for out-of-domain divisors, so they are now defensively dead within the domain. - add: operand checks; the coefficient-overflow rescale reverts at EXPONENT_MAX instead of int256.max; a result check catches maximized sums below the domain. Exact cancellation still returns zero at any exponent. - minus: the type(int256).min normalization increment reverts at EXPONENT_MAX instead of int256.max so it cannot escape the domain. - sub/inv inherit via minus+add and div. pow's squaring loop now surfaces ExponentOverflow at the first out-of-domain intermediate instead of Panic(0x11), closing #239 for the whole class rather than the single pinned repro. Tests: boundary pairs (success at the bound, revert one past it) for every guard; the enshrining boundary tests inverted per the rework ruling; div machinery tests inverted to pin operand rejection; fuzz bounds moved to the domain where results are defined; testSubPacked now models the packLossy-vs-packArithmeticResult underflow divergence (pre-existing latent flake, reproduced on clean main). Deploy constants + committed ABI regenerated for the new bytecode; frozen 0.1.7 pins added (also red on main, fix-main PR opened separately). REQUIRES redeploy at land. Co-Authored-By: Claude <noreply@anthropic.com>
thedavidmeister
commented
Jul 18, 2026
🤖 ai:producer What changed
Consequences surfaced for review
QA
WAITING-DEPLOY: local suite is 532 passed / 5 failed — the 5 are |
thedavidmeister
commented
Jul 18, 2026
🤖 ai:producer |
…lake fmt [3b-attempt] The rainix copy-artifacts reusable requires the codegen script at script/Build.sol (the org convention siblings already follow); rename BuildPointers.sol accordingly and update the docs. The rs-static no-skip gate forbids vm.skip in any form, so the tagged-constants test's unreachable-registry branch becomes a vacuous pass (nothing to verify) instead of a skip. Format with the rainix-pinned forge so the static fmt check agrees. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/implementation/LibDecimalFloatImplementation.sol`:
- Around line 196-200: Move exponent-domain validation before zero-result fast
paths: in src/lib/implementation/LibDecimalFloatImplementation.sol lines
196-200, validate both multiplication operands before the isZero return; at
lines 328-334, after division-by-zero validation, validate the nonzero divisor
before returning for a zero dividend; and at lines 686-690, validate both
addition operands before the eitherZero return. Add regressions covering zero
multiplied by, divided by, and added to a nonzero operand with an out-of-domain
exponent, ensuring public arithmetic reverts with ExponentOverflow or
ExponentUnderflow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0fa3a2ad-95b4-450b-b3c6-a1bacbc044a6
📒 Files selected for processing (15)
CLAUDE.mdcrates/float/abi/DecimalFloat.jsonscript/Build.solsrc/lib/deploy/LibDecimalFloatDeploy.solsrc/lib/implementation/LibDecimalFloatImplementation.soltest/src/lib/LibDecimalFloat.mul.t.soltest/src/lib/LibDecimalFloat.pow.t.soltest/src/lib/LibDecimalFloat.sub.t.soltest/src/lib/deploy/LibDecimalFloatDeployTaggedConstants.t.soltest/src/lib/implementation/LibDecimalFloatImplementation.add.t.soltest/src/lib/implementation/LibDecimalFloatImplementation.div.t.soltest/src/lib/implementation/LibDecimalFloatImplementation.inv.t.soltest/src/lib/implementation/LibDecimalFloatImplementation.minus.t.soltest/src/lib/implementation/LibDecimalFloatImplementation.mul.t.soltest/src/lib/implementation/LibDecimalFloatImplementation.sub.t.sol
| // Operands must be in the exponent domain. In-domain exponents sum | ||
| // to at most one bit past the domain bound, which cannot overflow | ||
| // int256, so the addition below is panic-free. | ||
| enforceExponentDomain(signedCoefficientA, exponentA); | ||
| enforceExponentDomain(signedCoefficientB, exponentB); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Move exponent-domain validation ahead of zero-result fast paths.
A zero operand currently suppresses validation of the other nonzero operand, contradicting the documented operand-domain contract.
src/lib/implementation/LibDecimalFloatImplementation.sol#L196-L200: validate both multiplication operands before theisZeroreturn.src/lib/implementation/LibDecimalFloatImplementation.sol#L328-L334: after checking division by zero, validate the nonzero divisor before returning for a zero dividend.src/lib/implementation/LibDecimalFloatImplementation.sol#L686-L690: validate both addition operands before theeitherZeroreturn.
Add regressions for mul(0, …, nonzero, outOfDomain), div(0, …, nonzero, outOfDomain), and add(0, …, nonzero, outOfDomain).
As per coding guidelines, “Public arithmetic must revert on exponent overflow and underflow using ExponentOverflow or ExponentUnderflow.”
📍 Affects 1 file
src/lib/implementation/LibDecimalFloatImplementation.sol#L196-L200(this comment)src/lib/implementation/LibDecimalFloatImplementation.sol#L328-L334src/lib/implementation/LibDecimalFloatImplementation.sol#L686-L690
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/implementation/LibDecimalFloatImplementation.sol` around lines 196 -
200, Move exponent-domain validation before zero-result fast paths: in
src/lib/implementation/LibDecimalFloatImplementation.sol lines 196-200, validate
both multiplication operands before the isZero return; at lines 328-334, after
division-by-zero validation, validate the nonzero divisor before returning for a
zero dividend; and at lines 686-690, validate both addition operands before the
eitherZero return. Add regressions covering zero multiplied by, divided by, and
added to a nonzero operand with an out-of-domain exponent, ensuring public
arithmetic reverts with ExponentOverflow or ExponentUnderflow.
Source: Coding guidelines
thedavidmeister
commented
Aug 6, 2026
👤 human |
thedavidmeister
commented
Aug 6, 2026
Rework note @9b0cc58c287f416ef04dbb5597f26256b3050112: rework the PR to fit the split release lifecycle — deploys never gate merges (the deploy-before-merge choreography is superseded); remove or restructure anything in the PR that waits on a deploy; where deploy constants/pins are involved, follow the *.deploy repo convention (audited code only; version ↔ snapshot ↔ pins internally consistent; tag-release lifecycle). Whatever states follow the rework (including a typed blocked-on the repo's migration if one is genuinely needed) are the producer's ordinary transitions. Executes the 2026-08-06 ruling: rainlanguage/issue-pr-cron#221 |
…pow-panic-exponent-overflow
The deploy-before-merge choreography is superseded by the split release lifecycle: no merge waits on a deploy, and the on-chain deploy is a manual workflow dispatch decoupled from any PR's flow. Drop the "trigger the deploy on the PR's branch before merge / do NOT wait for merge before deploying" instruction from the repo's agent context. The constants paragraph below it is left alone: it still describes what CI actually checks today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thedavidmeister
commented
Aug 18, 2026
🤖 ai:producer |
Summary
powwith very large or very negative integer exponents uses exponentiation-by-squaring, which repeatedly squares the base. When the base exponent is extreme, repeated doubling drives it pastint256bounds, causing a checked addition inmul()to panic withPanic(0x11)instead of the expectedExponentOverflow.mul(): checks thatexponentA + exponentBitself would overflow before performing the addition. The previous approach (checking each operand againstEXPONENT_MAXindividually) was too broad — opposite-sign pairs can never overflow int256, but the div round-trip tests pass exponents of opposite signs (one neartype(int256).max, the othertype(int256).min) whose sum is safely near zero.testPowNegativeExponentSquaringPanicpins the exact revert args for the negative-exponent squaring-loop case.testProdDeployment*will pass:gh workflow run manual-sol-artifacts.yaml --repo rainlanguage/rain.math.float --ref 2026-06-17-issue-239-pow-panic-exponent-overflow -f suite=decimal-floatCloses#239
Test plan
testPowNegativeExponentSquaringPanicpasses (ExponentOverflow instead of Panic)testPowIntegerExponentSquaringOverflowstill passes (unchanged error path)testDivAdjustExponent*)testArtifactsCommittedpassestestDeployAddress/testExpectedCodeHashDecimalFloatpasstestRoundTripFuzzPow)testProdDeployment*passes🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Documentation
QA
test/src/lib/LibDecimalFloat.mul.t.soland the implementationmul/div/add/sub/inv/minustest files — a boundary pair per guard (success atEXPONENT_MAX/EXPONENT_MIN, revert one past it) — plustestPowNegativeExponentSquaringPanicandtestMulRenormalizationOverflowGuard. Each fails on base by construction: base has no domain enforcement, so it returns out-of-domainFloats orPanic(0x11)exactly where these expectExponentOverflow, and the two inverted tests (testMulPositiveExponentBoundaryNoRevert,testMulNegativeExponentBoundaryNoRevert) previously asserted the un-guarded return the rework ruling rejected. Evidence recorded by the producer run at head 85e9757;src/is byte-identical at this head (git diff 85e9757 HEAD -- srcis empty), andtest/has changed only byforge fmtreflow and onevm.skipremoval in the unrelated tagged-constants test.enforceExponentDomainupper arm dead → 6 killers; lower arm dead → 30 killers across all five operators; zero-coefficient exemption removed → killed uniquely bytestAddCancellationBelowDomainReturnsZero; themulpost-adjust guard onexponent += int256(adjustExponent)disabled → killed uniquely bytestMulRenormalizationOverflowGuard(Panic(0x11)vsExponentOverflow);add's rescale bound reverted fromEXPONENT_MAXtoint256.max→ killed by revert-payload discrimination;minus's bound reverted → killed uniquely; both domain bounds made exclusive → killed by the boundary-success tests. Entry/exit call-site removals are pinned by the same killers via operand- vs result-payload discrimination. All applied and suite-validated at head 85e9757, whosesrc/this head reproduces byte-for-byte.[EXPONENT_MIN, EXPONENT_MAX]enforced across the operator set) plus bounds derived independently of the implementation —2·EXPONENT_MAX == int256.max - 1, maximization shift ≤ 76,adjustExponent ∈ [0, 76];LibDecimalFloatSlowremains the independent value oracle for the in-domain fuzz properties.pow's exponentiation-by-squaring surfaceExponentOverflowinstead ofPanic(0x11)— covered; the rework ruling's wider order (enforce the domain acrossmul/div/add/sub/invrather than patchingmulalone, invert the 2 tests that enshrined out-of-domain returns, keep the 6 mutation-validated keepers) is covered by the same diff. Deploy: nothing here waits on a deploy — under the split release lifecycle the on-chain deploy is a decoupled manual dispatch and no merge waits on it; the "Deploy needs manual trigger" line in the Summary above is superseded (see the rework note at 9b0cc58), andCLAUDE.md's deploy-before-merge instruction is removed in 2909b39.