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(blockifier): bouncer_weights remove derives of add, addassign, sub. use 'checked' instead #2835

Conversation

avivg-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@avivg-starkware avivg-starkware marked this pull request as ready for review December 19, 2024 14:49
@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/bouncerweights_change_add_to_checked_add branch 2 times, most recently from 6785874 to df8829c Compare December 19, 2024 15:20
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avivg-starkware, @noaov1, and @Yoni-Starkware)


crates/blockifier/src/bouncer.rs line 116 at r1 (raw file):

        *self = self.checked_add(other).expect("Addition overflow");
    }
}

you are making + operations behave like "checked_add_or_panic".
is this what we want?
I prefer to be explicit, how painful would it be to replace all?
(if it helps, you can derive the arithmetic traits in testing mode)

Code quote:

impl std::ops::Add for BouncerWeights {
    type Output = BouncerWeights;

    fn add(self, other: BouncerWeights) -> BouncerWeights {
        self.checked_add(other).expect("Addition overflow")
    }
}
impl std::ops::AddAssign for BouncerWeights {
    fn add_assign(&mut self, other: BouncerWeights) {
        *self = self.checked_add(other).expect("Addition overflow");
    }
}

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/bouncerweights_change_add_to_checked_add branch from df8829c to ec43cb6 Compare December 22, 2024 10:01
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @noaov1 and @Yoni-Starkware)

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/bouncerweights_change_add_to_checked_add branch from ec43cb6 to 7af5968 Compare December 22, 2024 10:15
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avivg-starkware, @noaov1, and @Yoni-Starkware)


crates/blockifier/src/bouncer.rs line 0 at r3 (raw file):

  1. do we want checked or saturating?
  2. if we want checked, please add more info in the expect message: (a) the text in the two cases should be different (easier to identify the source of panic) and (b) maybe also output the weights being added

Copy link
Contributor Author

@avivg-starkware avivg-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @Yoni-Starkware)


crates/blockifier/src/bouncer.rs line 116 at r1 (raw file):

Previously, dorimedini-starkware wrote…

you are making + operations behave like "checked_add_or_panic".
is this what we want?
I prefer to be explicit, how painful would it be to replace all?
(if it helps, you can derive the arithmetic traits in testing mode)

Done


crates/blockifier/src/bouncer.rs line at r3 (raw file):

Previously, dorimedini-starkware wrote…
  1. do we want checked or saturating?
  2. if we want checked, please add more info in the expect message: (a) the text in the two cases should be different (easier to identify the source of panic) and (b) maybe also output the weights being added

@Yoni-Starkware WDYT? i would assume checked, if overflow is something that shouldn't happen. but perhaps saturating is sufficient?

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @noaov1)


crates/blockifier/src/bouncer.rs line at r3 (raw file):

Previously, avivg-starkware wrote…

@Yoni-Starkware WDYT? i would assume checked, if overflow is something that shouldn't happen. but perhaps saturating is sufficient?

lgtm

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

@avivg-starkware avivg-starkware force-pushed the avivg/blockifier/bouncerweights_change_add_to_checked_add branch from 7af5968 to d149bc2 Compare December 23, 2024 13:00
@avivg-starkware
Copy link
Contributor Author

crates/blockifier/src/bouncer.rs line at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

lgtm

@dorimedini-starkware how does the info I've added looks to you? should I change anything?

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @noaov1)


crates/blockifier/src/bouncer.rs line at r3 (raw file):

Previously, avivg-starkware wrote…

@dorimedini-starkware how does the info I've added looks to you? should I change anything?

lgtm

@avivg-starkware avivg-starkware merged commit 4362da0 into main Dec 23, 2024
14 checks passed
@avivg-starkware avivg-starkware deleted the avivg/blockifier/bouncerweights_change_add_to_checked_add branch December 23, 2024 17:20
@github-actions github-actions bot locked and limited conversation to collaborators Dec 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants