Skip to content

feat: constant value binary decomposition #1510

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

Merged
merged 10 commits into from
Jun 6, 2025
Merged

Conversation

ivokub
Copy link
Collaborator

@ivokub ivokub commented Jun 6, 2025

Description

Handle binary decomposition for constant inputs.

Fixes #913. #1481 is duplicate of it, but it was not well implemented.

I also updated the test engine to mod-reduce the witness when copying. This mimics better the behaviour of actual solver.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How has this been tested?

  • TestToBinaryConstantInput
  • TestFromBinaryConstantInput
  • TestFromBinaryInvalidInput

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@ivokub ivokub requested review from gbotrel and Copilot June 6, 2025 12:46
@ivokub ivokub self-assigned this Jun 6, 2025
@ivokub ivokub added the bug Something isn't working label Jun 6, 2025
@ivokub ivokub added the fuzzing Issue found using a fuzzing tool label Jun 6, 2025
Copilot

This comment was marked as outdated.

@ivokub ivokub requested a review from Copilot June 6, 2025 12:50
Copilot

This comment was marked as outdated.

@ivokub
Copy link
Collaborator Author

ivokub commented Jun 6, 2025

Uh, ton of tests started to fail now. I'll see if there is something systematic.

@ivokub ivokub requested a review from Copilot June 6, 2025 14:04
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements constant value binary decomposition and improves the handling of constant inputs in binary conversion routines, along with updating the test engine behavior for witness reduction.

  • Improved witness reduction logic by mod reducing values when copying witness data.
  • Enhanced binary conversion functions to separately handle constant inputs and added comprehensive tests for both valid and invalid cases.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/engine.go Removed obsolete IsConstant and updated witness copying to mod reduce values.
std/math/bits/conversion_test.go Added tests to validate binary conversion with constant inputs and ensured consistency between decomposed and constant binary representations.
std/math/bits/conversion_binary.go Refactored handling of constant inputs in FromBinary and ToBinary; potential error in loop iteration in constant branch of ToBinary.
frontend/cs/scs/api_assertions.go Refactored AssertIsLessOrEqual to use a switch for clarity and improved constant handling.
frontend/cs/r1cs/api_assertions.go Similar refactoring as in scs assertions to handle constant bounds via a switch statement.

@ivokub ivokub requested a review from gbotrel June 6, 2025 14:33
@ivokub
Copy link
Collaborator Author

ivokub commented Jun 6, 2025

Uh, ton of tests started to fail now. I'll see if there is something systematic.

Found the bugs. One was from my change and other one was another existing one.

@ivokub ivokub merged commit 57a79c2 into master Jun 6, 2025
6 checks passed
@ivokub ivokub deleted the feat/constant-decomposition branch June 6, 2025 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fuzzing Issue found using a fuzzing tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

perf: api.FromBinary(api.ToBinary(constant)) is not a constant
2 participants