Remove slip10-like derivation and add stark-specific derivation

[survived]

• Remove slip10-like derivation that can be instantiated with any curve: it is very inefficient
  when instantiated with certain curves, and may also enable attacker to perform DoS attack by
  finding derivation path that results into very long computation
• Add stark-specific derivation: secure and efficient derivation for stark curve

[maurges]

Why not (order_log2 + 128 + 7) / 8?

[survived]

I wanted to explicitly use ceiling function to have less opportunities to mess up, and to match the formula from RFC precisely

[maurges]

You don't need two variables here: as soon as x_lo <= x you can take hi = lo - 1

[survived]

What you suggest is supposed to decrease amount of the code, but adding let hi = lo - 1 actually adds extra line of code

[survived]

(if I correctly understood what you suggested)

[maurges]

No, I'm suggesting reducing the amount of computation done, not necessarily code. You remove hi in the loop, you remove x_hi altogether, and when the comparison succeeds, you compute hi = lo - 1. By my calculations, the amount of lines of code will stay the same, but the lines will get shorter and we drop one comparison

[survived]

if $2^{-k} \le x$, that does not necessarily mean that $x \le 2^{-k+1}$ in general. That’s probably true from the fact that we check that $0 &lt; x &lt; 1$, but again, in this tool I prefer to double check everything, because we do complex computations and it’s essential to be assured that everything is correct. I prioritize auditability/readability here over performance/allocations

[maurges]

In general no, but the loop finds not a general k, but a minimal k such that $x \le 2^{-k}$. Since it's minimal, it follows that $x &gt; 2^{-(k - 1)}$.

Reformulating imperatively, you're iterating lo <- [1, 2, ..], you compute x_lo = 1 / (1 << lo). You condition for breaking is x_lo <= x. that means if you're evaluating the loop, you have x_lo' = 1 / (1 << (lo - 1)) and x_lo' > x. Note that this x_lo' is exactly x_hi.

What it means is that you can skip comparing x_hi, and move computing x_hi altogether into the same branch as return.

[maurges]

I get your view about not optimizing much for performance, since it's a piece of code ran rarely. But also when I first saw it, it took me some time to verify that hi is indeed computed from k-1, so readability gain here is dubious

[survived]

Here's my logic: when I see in the output of the program that "stat diff lies in $2^{-k} \le x \le 2^{-k+1}$", my first thought is to check if it's correct. Suppose $x$ is calculated correctly, I need to check if boundaries are correct as well. To do that, I have to analyze the function. Right now, it's very easy to check that boundaries are identified correctly, as the only way this function returns something is when the condition $2^{-k} \le x \le 2^{-k+1}$ is true:

hd-wallet/examples/curves_analysis.rs

Lines 126 to 130 in 158b65c

	let x_lo = rug::Rational::from((rug::Integer::ONE, rug::Integer::ONE << lo));
	let x_hi = rug::Rational::from((rug::Integer::ONE, rug::Integer::ONE << hi));
	if x_lo <= *x && *x <= x_hi {
	return (lo, hi);
	}

So that's very easy to audit and analyze. If we do what you suggested, in my opinion, it will be more difficult to analyze the function, as it's now required to understand how the loop works. As you said, in general, this approach does not yield correct result, but it does in this particular case, and it will take some time to come to the same conclusion.

In this case, I want to keep the option that looks correct right away,

[survived]

But also when I first saw it, it took me some time to verify that hi is indeed computed from k-1

How come? It's very clearly defined in the loop that lo goes in 1.. while hi goes in 0..

[maurges]

Wait, why is concatenation not done in the calling function? Does this optimize better?

[survived]

To have less of repeating code. Two different function are calling this one, if we were doing concat in the calling functions, we needed to write concat twice.

[maurges]

But this way you leak implementation details! It's not a big problem since it's an internal function, but it looks like a bizzare choice to me to mangle the domains like this to save one (1) line

[survived]

Since it's an internal function, it's not leaking implementation detail

[survived]

I dunno, it feels like a natural choice for me...

[maurges]

You know, other than your weird new obsession with saving single lines of code, the maths and the changes look good

[survived]

I'm not obsessed with saving single lines of code, I'm lazy to change something unless I see a good motivation for that change :)