Fix bounds (b, b_1, b_2) on logproof

[ryanorendorff]

The prior bounds for the short discrete log proof were not quite accurate for some inputs. Specifically, the following changes were made to match the paper:

• b: log2(B) + 1 -> ⌈log2(B)⌉ + 1
• b₁: log2(mdB + d||f||_inf) -> ⌈log2(mdB + d||f||_inf)⌉
• b₂ log2(q) + 1 -> ⌈log2(q)⌉

Specifically, b₂ was correct unless q was a power of 2, which essentially never happens in practice.

[samtay]

Logic looks right, and double checked that this matches the paper. Only thing I'm not sure of is if the check for power of two needs to be constant time or not. I think since it is only being used when computing things from public params it is okay.

[ryanorendorff]

Logic looks right, and double checked that this matches the paper. Only thing I'm not sure of is if the check for power of two needs to be constant time or not. I think since it is only being used when computing things from public params it is okay.

So another option, closer to what the standard library does for uints (which is call self.count_ones() as a method on each uint size) is to do the following:

fn is_power_of_two_bigint<const N: usize>(b: &BigInt<N>) -> bool {
    b.as_ref().iter().map(|u| u.count_ones()).sum::<u32>() == 1
}

It is certainly cleaner looking, so that seems nice. I may switch to this since it is more readable.

This isn't taking into account negative numbers, which I do not think BigInt in the ark_ff crate supports, but log is not defined on negative numbers anyway.

[samtay]

I don't think you can get around checking the bits individually for a power of two, so the timing will seemingly always be at worst the number of bits in the integer.

In this context, constant time doesn't mean O(1), it means the time it takes to execute doesn't change depending on the input. The original version was definitely not constant time, since it short circuited at the second 1 bit it found. So a timing attack could reveal e.g. that the number was not a power of two and, with enough runs & statistical analysis, even which bit position is 1.

Unfortunately, even leaking 1 bit can be catastrophic 😞

What I was saying above was that these ceil_log2 functions look like they are only used on public parameters, in which case it's not a problem to leak that information.

[ryanorendorff]

Ah I see! The newer version may be more immune to timing attacks; I unfortunately don't know if the underlying count_ones() is constant time. If it is, then the current setup has a constant calculation time for a given number of limbs in the BigInt (ex: BigInt<2>s would calculate in the same time, but BigInt<3> would have a different runtime).

As you mentioned, it is currently only being used on public parameters, which are agreed on before running the SDLP. But it will be good to keep in mind in case we need the same computation on a more sensitive input.