feat: allow modifying the upper bound in bits.ToBinary - #1809
Open
zexoverz wants to merge 1 commit into
Open
Conversation
Closes Consensys-Incorporated#1434. ToBinary compares the decomposition against the native modulus minus one, which is the weakest bound that still keeps the decomposition unique. Callers who know their value lives in a smaller range had no way to say so. WithUpperBound sets that constant. It is rejected at or above the modulus, since a larger bound readmits the a / a+r ambiguity the check exists to prevent, and it is refused alongside OmitModulusCheck because the two ask for opposite things. An explicit bound always enforces the comparison, including when WithNbDigits is below the field bitlength and the check would otherwise be skipped: the digit count does not imply the caller's bound. Constant inputs are checked at compile time, matching how WithNbDigits already reports an out-of-range constant. With the option unset every path is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1434.
ToBinarycompares the decomposition against the native modulus minus one, which is the weakest bound that still keeps the decomposition unique. Callers who already know their value lives in a smaller range had no way to say so, which came up in the discussion on #1420.WithUpperBoundsets that constant. Two guards come with it. A bound at or above the modulus is rejected, because it readmits thea/a+rambiguity the check exists to prevent. And it is refused alongsideOmitModulusCheck, since one asks for a bound check and the other removes it.The part worth reviewing closely is that an explicit bound always enforces the comparison, including when
WithNbDigitsis below the field bitlength and the check would otherwise be skipped. The digit count implies its own bound, not the caller's, so without thisWithNbDigits(8)together withWithUpperBound(100)would have silently constrained nothing. There is a test for exactly that case.Constant inputs are checked at compile time, matching how
WithNbDigitsalready reports an out-of-range constant.With the option unset every path is unchanged. Tests run across the curves and backends via
CheckCircuit;std/rangecheck,std/math/cmpandstd/selectorstill pass.Note
Cursor Bugbot is generating a summary for commit d8cd6fa. Configure here.