Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: substraction between relocatables should be signed #1218

Merged
merged 4 commits into from
Jun 10, 2023

Conversation

Oppen
Copy link
Member

@Oppen Oppen commented Jun 9, 2023

It is valid as per the reference implementation to substract two relocatables a - b where b.offset > a.offset. The current code returns an error due to negative result in that case.
The fix consists in not using the trait as implemented for Relocatable (where it represents a positive distance between pointers) but to promote the offsets to i128 and only substract them for MaybeRelocatable, as those are the language-level types.

Checklist

  • Linked to Github Issue
  • Unit tests added
  • Integration tests added.
  • This change requires new documentation.
    • Documentation has been added/updated.
    • CHANGELOG has been updated.

It is valid as per the reference implementation to substract two
relocatables `a - b` where `b.offset > a.offset`.
The fix consists in not using the trait as implemented for `Relocatable`
(where it represents a positive distance between pointers) but to
promote the offsets to `i128` and only substract them for
`MaybeRelocatable`, as those are the language-level types.
@Oppen Oppen force-pushed the fix/unexpected_sign_error branch from 23b1b99 to 2fc43d1 Compare June 9, 2023 21:16
Copy link
Contributor

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

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

LGTM. We can address TODOs and FIXMEs in following PRs.

src/tests/cairo_run_test.rs Outdated Show resolved Hide resolved
src/vm/vm_core.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Jun 9, 2023

Benchmark Results for unmodified programs 🚀

Command Mean [s] Min [s] Max [s] Relative
base blake2s_integration_benchmark 15.357 ± 0.277 15.187 16.114 1.00
head blake2s_integration_benchmark 15.424 ± 0.346 15.131 16.353 1.00 ± 0.03
Command Mean [s] Min [s] Max [s] Relative
base compare_arrays_200000 4.980 ± 0.081 4.909 5.158 1.00 ± 0.02
head compare_arrays_200000 4.971 ± 0.077 4.905 5.172 1.00
Command Mean [s] Min [s] Max [s] Relative
base dict_integration_benchmark 3.258 ± 0.062 3.200 3.410 1.00
head dict_integration_benchmark 3.282 ± 0.027 3.255 3.343 1.01 ± 0.02
Command Mean [s] Min [s] Max [s] Relative
base factorial_multirun 5.287 ± 0.043 5.232 5.387 1.00
head factorial_multirun 5.340 ± 0.076 5.243 5.505 1.01 ± 0.02
Command Mean [s] Min [s] Max [s] Relative
base fibonacci_1000_multirun 4.533 ± 0.053 4.478 4.663 1.00
head fibonacci_1000_multirun 4.549 ± 0.024 4.502 4.582 1.00 ± 0.01
Command Mean [ms] Min [ms] Max [ms] Relative
base field_arithmetic_get_square_benchmark 231.4 ± 21.3 214.4 270.2 1.07 ± 0.10
head field_arithmetic_get_square_benchmark 216.9 ± 1.9 213.3 219.8 1.00
Command Mean [s] Min [s] Max [s] Relative
base integration_builtins 14.191 ± 0.244 13.995 14.829 1.01 ± 0.02
head integration_builtins 14.097 ± 0.118 13.996 14.408 1.00
Command Mean [s] Min [s] Max [s] Relative
base keccak_integration_benchmark 15.596 ± 0.305 15.282 16.178 1.01 ± 0.02
head keccak_integration_benchmark 15.433 ± 0.073 15.290 15.534 1.00
Command Mean [s] Min [s] Max [s] Relative
base linear_search 4.954 ± 0.032 4.896 5.010 1.00
head linear_search 5.010 ± 0.069 4.894 5.115 1.01 ± 0.02
Command Mean [s] Min [s] Max [s] Relative
base math_cmp_and_pow_integration_benchmark 3.531 ± 0.030 3.484 3.590 1.00
head math_cmp_and_pow_integration_benchmark 3.552 ± 0.034 3.516 3.629 1.01 ± 0.01
Command Mean [s] Min [s] Max [s] Relative
base math_integration_benchmark 3.305 ± 0.049 3.261 3.421 1.00
head math_integration_benchmark 3.306 ± 0.037 3.266 3.397 1.00 ± 0.02
Command Mean [s] Min [s] Max [s] Relative
base memory_integration_benchmark 2.800 ± 0.016 2.778 2.825 1.00
head memory_integration_benchmark 2.812 ± 0.028 2.775 2.874 1.00 ± 0.01
Command Mean [s] Min [s] Max [s] Relative
base operations_with_data_structures_benchmarks 3.240 ± 0.019 3.218 3.286 1.00
head operations_with_data_structures_benchmarks 3.258 ± 0.019 3.228 3.288 1.01 ± 0.01
Command Mean [s] Min [s] Max [s] Relative
base pedersen 1.070 ± 0.010 1.051 1.082 1.00
head pedersen 1.076 ± 0.009 1.065 1.089 1.01 ± 0.01
Command Mean [s] Min [s] Max [s] Relative
base poseidon_integration_benchmark 1.863 ± 0.018 1.835 1.889 1.01 ± 0.01
head poseidon_integration_benchmark 1.852 ± 0.011 1.841 1.870 1.00
Command Mean [s] Min [s] Max [s] Relative
base secp_integration_benchmark 4.037 ± 0.032 3.989 4.071 1.00
head secp_integration_benchmark 4.079 ± 0.039 4.035 4.148 1.01 ± 0.01
Command Mean [s] Min [s] Max [s] Relative
base set_integration_benchmark 1.873 ± 0.030 1.838 1.937 1.01 ± 0.02
head set_integration_benchmark 1.853 ± 0.011 1.838 1.872 1.00
Command Mean [s] Min [s] Max [s] Relative
base uint256_integration_benchmark 9.771 ± 0.052 9.698 9.857 1.00
head uint256_integration_benchmark 9.814 ± 0.063 9.755 9.962 1.00 ± 0.01

src/types/relocatable.rs Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #1218 (644f7f3) into main (061b2c3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1218   +/-   ##
=======================================
  Coverage   97.59%   97.59%           
=======================================
  Files          89       89           
  Lines       36159    36161    +2     
=======================================
+ Hits        35289    35291    +2     
  Misses        870      870           
Impacted Files Coverage Δ
src/types/relocatable.rs 99.40% <100.00%> (+<0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@juanbono juanbono enabled auto-merge June 9, 2023 22:39
@juanbono juanbono added this pull request to the merge queue Jun 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 9, 2023
@Oppen Oppen added this pull request to the merge queue Jun 10, 2023
Merged via the queue into main with commit 2b7de08 Jun 10, 2023
31 checks passed
@Oppen Oppen deleted the fix/unexpected_sign_error branch June 10, 2023 00:51
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jun 23, 2023
…1218)

* fix: substraction between relocatables should be signed

It is valid as per the reference implementation to substract two
relocatables `a - b` where `b.offset > a.offset`.
The fix consists in not using the trait as implemented for `Relocatable`
(where it represents a positive distance between pointers) but to
promote the offsets to `i128` and only substract them for
`MaybeRelocatable`, as those are the language-level types.

* chore: remove uneeded stuff

* Add fix holes to if_reloc_equal.cairo test

* chore: changelog

---------

Co-authored-by: Pedro Fontana <fontana.pedro93@gmail.com>
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.

None yet

4 participants