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.

Sign up for GitHub

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

Jump to bottom

Address potential security bug and improve testing for the logic constraint function #348

Merged
merged 3 commits into from
Apr 25, 2023
Merged

Address potential security bug and improve testing for the logic constraint function #348

merged 3 commits into from
Apr 25, 2023

Conversation

maramihali
Copy link
Contributor

@maramihali maramihali commented Apr 12, 2023 •
edited
Loading

Description

Ensures the result of the create_logic_constraint function is indeed produced using the left and right operand that are passed as arguments.

Adds test to highlight a potential issue with the range constraints in the ultra composer.

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • There are no circuit changes, OR specifications in /markdown/specs have been updated.
  • There are no circuit changes, OR a cryptographer has been assigned for review.
  • I've updated any terraform that needs updating (e.g. environment variables) for deployment.
  • The branch has been rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.
  • New functions, classes, etc. have been documented according to the doxygen comment format. Classes and structs must have @brief describing the intended functionality.
  • If existing code has been modified, such documentation has been added or updated.

@maramihali maramihali force-pushed the mm/logic-constraints branch 4 times, most recently from 5b99327 to e0e2225 Compare April 21, 2023 13:52
@maramihali maramihali changed the title wip Address potential security bug and improve testing for the logic constraint function Apr 21, 2023
@maramihali maramihali force-pushed the mm/logic-constraints branch 2 times, most recently from 718fef4 to 70a98d7 Compare April 21, 2023 14:26
@maramihali maramihali marked this pull request as ready for review April 21, 2023 14:28
@maramihali maramihali requested a review from codygunton April 21, 2023 14:28
@maramihali maramihali force-pushed the mm/logic-constraints branch from 70a98d7 to 6610868 Compare April 21, 2023 14:57
@maramihali maramihali self-assigned this Apr 21, 2023
@maramihali maramihali force-pushed the mm/logic-constraints branch from 6610868 to 1512b00 Compare April 23, 2023 10:50
@maramihali maramihali requested a review from codygunton April 24, 2023 09:01
@maramihali maramihali force-pushed the mm/logic-constraints branch 3 times, most recently from ecd2f86 to 0f2efb8 Compare April 24, 2023 14:25
@maramihali maramihali requested a review from Rumata888 April 24, 2023 14:30
maramihali and others added 2 commits April 25, 2023 10:19
@maramihali
add constraints to ensure that the result of the logic constraint is
4d9af82 
indeed produced from the left and right operands of the functions and
improve testings.
@zac-williamson @maramihali
fixed error where applying copy constraints to range-constrained indi…
a6b7987 
…ces would create an unbalanced set membership check
@maramihali maramihali force-pushed the mm/logic-constraints branch from d4d05ca to 69ae53a Compare April 25, 2023 10:20
@maramihali maramihali force-pushed the mm/logic-constraints branch from 69ae53a to 8a1f3b2 Compare April 25, 2023 10:27
@maramihali maramihali merged commit 6ef5df5 into master Apr 25, 2023
@maramihali maramihali deleted the mm/logic-constraints branch April 25, 2023 11:34
@codygunton codygunton mentioned this pull request Apr 25, 2023
Closed
11 tasks
codygunton
codygunton reviewed May 5, 2023
View reviewed changes
cpp/src/barretenberg/stdlib/primitives/logic/logic.test.cpp

auto composer = Composer();
for (size_t i = 8; i < 248; i += 8) {
run_test(8, composer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this test is only being run for one value! 🤞

Copy link
Collaborator

Choose a reason for hiding this comment

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

Resolved in #432

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codygunton codygunton codygunton left review comments

@Rumata888 Rumata888 Awaiting requested review from Rumata888

Assignees

@maramihali maramihali

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maramihali @codygunton @zac-williamson