Skip to content
This repository has been archived by the owner on Aug 27, 2021. It is now read-only.

Get bits from non-native allocation #45

Merged
merged 6 commits into from
Aug 7, 2021

Conversation

alex-ozdemir
Copy link
Contributor

@alex-ozdemir alex-ozdemir commented Jul 14, 2021

Description

When doing nonnative arithmetic, bit-allocating witnesses requires a non-trivial number of constraints. This PR adds an allocation function which also returns the bits, instead of just dropping them. That way if you need the bits, you don't have to duplicate constraints.

Motivation: In a recent circuit, I wanted to (a) bit-allocate some non-native field elements (b) do some simple (and cheap!) math on them and (c) do some other stuff with the bits. It seemed that bit-allocation would actually be a bottleneck here, so de-duplicating that work was important. Figured I'd submit the patch upstream, in case you wanted it.

Design: Basically I just moved the allocation code to a new fn that exposes the bits, and had the original allocation function call the new one.

P.S. Thanks for putting this library together. It's very nice to not have to roll my own multiprecision machinery again :)


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@weikengchen
Copy link
Member

I will take a look soon. There is a challenge about the interface, which I will find, hopefully, a cute solution.

  • On the one hand, the new function does not return any bits when the mode is not a witness, which may confuse the developers as this is not obvious from its name. One solution is to rename it as new_witness_with_le_bits, so it only deals with witnesses.
  • On the other hand, if we make the change above, there would be repeated code, as the allocation gadget would need a separate code.

Indeed thanks for your code, as the current version of the gadget is largely based on yours. We actually only partly migrate your machinery. The current version does not track the maximal value, so there can be redundancy.

Comment on lines 583 to 600
if mode == AllocationMode::Witness {
for limb in limbs.iter().rev().take(params.num_limbs - 1) {
bits.extend(
Reducer::<TargetField, BaseField>::limb_to_bits(limb, params.bits_per_limb)?
.into_iter()
.rev(),
);
}

bits.extend(
Reducer::<TargetField, BaseField>::limb_to_bits(
&limbs[0],
TargetField::size_in_bits() - (params.num_limbs - 1) * params.bits_per_limb,
)?
.into_iter()
.rev(),
);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think when the mode is not Witness, we can still generate bits easily:

  • if mode == Constant, we can return Boolean::Constant values (this requires no constraints)
  • if mode == Input, we can just reuse the existing machinery for the mode == Witness case.

If the concern is adding potentially unnecessary constraints, we can make a helper function which generates bits only if a flag is true.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, my concern is that when mode == Input, our current allocation gadget, which uses this function as a black box, would generate the bits, which would be unnecessary.

Maybe let us extend this function so that one can specify, via a bool, whether or not bits are wanted.

@alex-ozdemir
Copy link
Contributor Author

alex-ozdemir commented Jul 14, 2021

On the one hand, the new function does not return any bits when the mode is not a witness, which may confuse the developers as this is not obvious from its name.

Yeah. Probably returning an Option<_> would be better than an empty vec, but still undesirable.

On the other hand, if we make the change above, there would be repeated code,

That's the challenge.

One solution is to split the machinery into two private functions. One builds the limbs, and the other does the range-checks. Then, new_variable and new_witness_with_le_bits (taking your naming suggestion here) can delegate to these helpers.

Happy to make the change if that sounds promising to you.

@Pratyush
Copy link
Member

Yeah, that sounds like a good strategy!

CHANGELOG.md Outdated Show resolved Hide resolved
alex-ozdemir and others added 2 commits August 7, 2021 00:09
Co-authored-by: Pratyush Mishra <[email protected]>
Copy link
Member

@Pratyush Pratyush left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@Pratyush Pratyush merged commit ae19124 into arkworks-rs:master Aug 7, 2021
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.

3 participants