test(pow): widen testRoundTripFuzzPow range, drop arbitrary 8e8 fuzz bound#232
test(pow): widen testRoundTripFuzzPow range, drop arbitrary 8e8 fuzz bound#232thedavidmeister wants to merge 2 commits into
Conversation
`testRoundTripFuzzPow` carried an arbitrary `vm.assume(exponentInv <= 8e8)` that narrowed the fuzz input range. The bound existed only to dodge an `ExponentOverflow` in `pow`'s exponentiation-by-squaring loop: raising to a large integer exponent squares the base in place, so the base exponent grows by roughly a factor of two per bit and overflows once the integer exponent is large enough. The inverse leg of a round trip can land such an exponent. That overflow is now caught directly: the round-trip `pow` is already wrapped in a try/catch that treats a revert as "can't round-trip this input" rather than a math regression, so the magic `8e8` ceiling is redundant. Removing it lets the fuzzer exercise the full Float input range. Also add `testPowIntegerExponentSquaringOverflow` to pin the squaring-loop boundary deterministically: 2^1e9 succeeds, 2^1e10 reverts with `ExponentOverflow`. This documents the limitation the assume was masking and guards the catch path so the wider fuzz range is genuinely covered, not just silently skipped. Verification: full pow suite green (9/9), full `forge test` 452/452 passing (the only 5 failures are the pre-existing `LibDecimalFloatDeployProd` fork tests that need RPC env vars, identical on clean `origin/main`). `testRoundTripFuzzPow` also green at 100,000 fuzz runs without the assume. Test-only change: no source touched, deploy constants unaffected. Fixes #163 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 50 minutes and 17 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
Both legs of the round-trip pow fuzz used a catch-ALL
`catch (bytes memory) {}` that silently swallowed ANY revert, masking
real bugs. Replace each with `assertExpectedPowError`, which decodes the
selector and tolerates only the custom errors `pow` is designed to throw
(derived from the implementation):
ZeroNegativePower, PowNegativeBase, ExponentOverflow, ExponentUnderflow,
WithTargetExponentOverflow, MaximizeOverflow, MulDivOverflow
DivisionByZero / Log10Zero / Log10Negative are unreachable because `pow`
only ever inverts/logs a strictly positive base. A low-level `Panic`
(e.g. 0x11 arithmetic overflow) is no longer tolerated, so an unexpected
revert now fails the test instead of being swallowed. Expected reverts
remain tolerated; the round trip simply is not asserted for them.
This surfaces a latent defect: for a large positive base whose inverse
has a large-magnitude exponent combined with a large negative integer
exponent (e.g. pow(1e1700000000, -8e69)), the exponentiation-by-squaring
loop overflows the checked `exponentA + exponentB` in
`LibDecimalFloatImplementation.mul` and reverts `Panic(0x11)` rather than
the clean `ExponentOverflow` documented by
`testPowIntegerExponentSquaringOverflow`. The tightened fuzz can reach
this input, so it may fail on some fuzz seeds until `pow` reverts
cleanly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
What
Remove the arbitrary
vm.assume(exponentInv <= 8e8)fromtestRoundTripFuzzPowso the fuzz test exercises a wider range of inputs, and add a deterministic regression test pinning the underlying squaring-loop overflow boundary.Why
powraises to the integer part of the exponent via exponentiation by squaring, which squares the base in place. The base exponent therefore grows by roughly a factor of two per bit of the integer exponent and eventually overflowsExponentOverflow. The inverse leg of a round trip (c.pow(b.inv())) can land an integer exponent above that ceiling, which is the only reason the test carried the8e8magic bound — exactly the arbitrary limit called out in #163.That overflow no longer needs a fuzz-narrowing assume to avoid: the round-trip
powis already wrapped in atry/catchthat treats a revert as "can't round-trip this input" (not a math regression). So the8e8ceiling is redundant dead weight that only shrinks the input range. Removing it lets the fuzzer cover the fullFloatspace.To make the wider range genuinely covered rather than silently skipped, this PR also adds
testPowIntegerExponentSquaringOverflow, which pins the boundary deterministically:2 ^ 1e9succeeds (edge of representable).2 ^ 1e10reverts withExponentOverflow— the squaring-loop limitation the assume was masking.Verification
powsuite green: 9/9 passing.forge test: 452/452 passing. The only 5 failures are the pre-existingLibDecimalFloatDeployProdfork tests, which require live RPC env vars (ARBITRUM_RPC_URL, etc.) not present in the sandbox — they fail identically on a cleanorigin/maincheckout and are unrelated to this change.testRoundTripFuzzPowalso stayed green at 100,000 fuzz runs with the assume removed, confirming thetry/catchfully subsumes the old bound.forge fmt --checkpasses.Test-only change: no source touched, so deploy constants and bytecode are unaffected.
Fixes #163
🤖 Generated with Claude Code