Introduce mul_by_inverse_unchecked, and use it

[Pratyush]

Description

Resolves #70 (comment).

---

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

[Pratyush]

Do you have a good way to add a regression test for this behaviour?

[tsunrise]

looks good to me (though we need to make sure domain.offset != 0 if we plan to use unchecked)

[Pratyush]

Ok I added a check to make sure domain.offset is non-zero by construction. I don't know if this is a good idea: is there ever a case where domain.offset is supposed to be zero? If so, then I can change it to enforce the check inside lagrange_interpolate_with_non_constant_offset, instead of in Radix2DomainVar::new

cc @ValarDragon @tsunrise

[ValarDragon]

domain.offset should never be zero for multiplicative domains, but I don't think that needs to be proven in circuit. Thats about the circuit definition being valid. (The circuit definition has defined an invalid codeword domain if its zero, the offset derivation shouldn't allow construction of zero domain offsets)

[Pratyush]

I think @tsunrise's code require offset to be a variable, so we have to enforce non-zeroness at some point.

[ValarDragon]

Oh I see. If necessary as a temporary defense in depth for correctness seems like an alright fix then.

What I'd rather see is we assert this on the DomainVar itself (that its offset and generator are non-zero). Basically in the constructor for DomainVar, include an argument for the inputs being guaranteed to be non-zero. If false, then do the non-zero checks once there.

[Pratyush]

What I'd rather see is we assert this on the DomainVar itself (that its offset and generator are non-zero). Basically in the constructor for DomainVar, include an argument for the inputs being guaranteed to be non-zero. If false, then do the non-zero checks once there.

Yeah, that's what I changed it to do:

r1cs-std/src/poly/domain/mod.rs

Line 28 in 213e53d

pub fn new(gen: F, dimension: u64, offset: FpVar<F>) -> Result<Self, SynthesisError> {

[ValarDragon]

I meant explicitly including an argument to not get a constraint created. Currently it seems like a constraint is always created if offset is not a constant?

[Pratyush]

Ah so like a new_unchecked method? For the time being I'm fine with keeping the new method and using that, because I assume the domain is constructed only a few times (< 10?) in the entire circuit

[ValarDragon]

Its constructed once per query per round, so ~200 times.

[Pratyush]

I see, with a different offset each time?

[ValarDragon]

Yeah. The offset is based on the query coset

[Pratyush]

I think I addressed all comments, except for the new_unchecked for DomainVar. I think for the time being it's fine to use new, as it only adds a couple hundred constraints, which would be dominated by the rest of the circuit.

Technically, this is a breaking change due to the changes to the DomainVar API, but since the only consumer of that is ark-ldt, I believe, I think it's fine to do that.

(Maybe we should have "in-flux" features hidden behind a unstable feature flag?)