Take public logup sum into account in check_relation_uses#457
Take public logup sum into account in check_relation_uses#457az-starkware wants to merge 2 commits intoadar/relation_uses_audit_fixesfrom
Conversation
PR SummaryMedium Risk Overview Extends Reviewed by Cursor Bugbot for commit e82d50f. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e82d50f. Configure here.
|
|
||
| pub fn add_yield_term(&mut self, context: &mut Context<impl IValue>, term: Var) { | ||
| self.var = self.var.map(|var| eval!(context, (var) - (term))).or(Some(term)); | ||
| } |
There was a problem hiding this comment.
add_yield_term wrong sign when var is None
Medium Severity
add_yield_term uses .or(Some(term)) as the fallback when self.var is None, which sets the sum to +term instead of -term. A yield term represents a subtraction, so an empty LogupSum getting a yield term first would produce the wrong sign. Currently the only call site (line 607) always calls add_use_term first, so the None path isn't hit — but the method's contract is broken for that case, making it a latent correctness issue.
Reviewed by Cursor Bugbot for commit e82d50f. Configure here.
d2555f3 to
e8cf28a
Compare
|
Don't we count yields? Code quote: pub fn add_yield_term(&mut self, context: &mut Context<impl IValue>, term: Var) {
self.var = self.var.map(|var| eval!(context, (var) - (term))).or(Some(term));
} |
|
Previously, ilyalesokhin-starkware wrote…
I guess the use count is also a bound on the yield_count for opcodes. |
|
Document. Code quote: LogupSum |
|
consider renaming to TrackedLogupSum or PublicLogupSum Code quote: LogupSum |


No description provided.