Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryMedium Risk Overview Tests assert that overflow panics use Reviewed by Cursor Bugbot for commit 5fc5ece. Bugbot is set up for automated code reviews on this repo. Configure here. |
862e8bd to
c5f3655
Compare
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on orizi and TomerStarkware).
corelib/src/test/integer_test.cairo line 1837 at r1 (raw file):
#[test] #[should_panic(expected: ('i128_mul Overflow',))]
What error did we have before?
crates/cairo-lang-starknet/test_data/libfuncs_coverage__libfuncs_coverage.sierra line 13310 at r1 (raw file):
downcast<u128, BoundedInt<0, 170141183460469231731687303715884105727>>([33], [31]) { fallthrough([37], [38]) F179_B4([39]) }; branch_align() -> (); upcast<BoundedInt<0, 170141183460469231731687303715884105727>, i128>([38]) -> ([40]);
Why such a big regression?
Additionally improved performance of the implementation.
c5f3655 to
5fc5ece
Compare
0a554e2 to
219825c
Compare
orizi
left a comment
There was a problem hiding this comment.
@orizi made 2 comments.
Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on eytan-starkware and TomerStarkware).
corelib/src/test/integer_test.cairo line 1837 at r1 (raw file):
Previously, eytan-starkware wrote…
What error did we have before?
just u128_mul - as this was the internal implementation.
crates/cairo-lang-starknet/test_data/libfuncs_coverage__libfuncs_coverage.sierra line 13310 at r1 (raw file):
Previously, eytan-starkware wrote…
Why such a big regression?
it is a full change of the i128-mul - so it all changes.
you can see there's only one panic message here - and the usage of more efficient libfuncs.
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 3 files, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on TomerStarkware).
TomerStarkware
left a comment
There was a problem hiding this comment.
@TomerStarkware reviewed all commit messages and made 1 comment.
Reviewable status:
Summary
Fixes
i128multiplication to correctly detect overflow by usingu128_wide_mulinstead of relying on afelt252round-trip conversion. The result's high 128-bit word is checked to be zero, and the low word is then validated against the appropriate signed bounds (i128::MAXfor positive results,i128::MINmagnitude for negative results) usingdowncastbefore converting. Also adds a test for the previously undetected overflow case ofi128::MIN * -1.Type of change
Please check one:
Why is this change needed?
The previous implementation converted the product of two
u127values tofelt252and then attempted atry_intotoi128. Becausefelt252arithmetic wraps around the field modulus rather than at 128-bit boundaries, certain overflow cases (e.g.,i128::MIN * -1) were silently accepted instead of panicking.What was the behavior or documentation before?
i128multiplication could silently produce incorrect results or fail to panic on overflows that crossed thei128range but happened to round-trip throughfelt252without triggering thetry_intofailure.What is the behavior or documentation after?
i128multiplication now usesu128_wide_multo obtain a 256-bit product, immediately panics with'i128_mul Overflow'if the high word is non-zero, and then separately checks the low word against the correct signed bound for positive and negative results. Overflow tests now assert the specific panic message'i128_mul Overflow', and a new test covers theMIN * -1case.Related issue or discussion (if any)
None provided.
Additional context
The existing overflow tests were updated from bare
#[should_panic]to#[should_panic(expected: ('i128_mul Overflow',))], making them more precise and ensuring the correct panic message is emitted.