fix: swap_constraint bug

[enricobottazzi]

This PR fixes a bug in our current swap_constraint.

In particular the current swap_constraint s * swap_bit * ((elelment_l_next - elelment_l_cur) - (elelment_r_cur - elelment_r_next)) correctly check that the swapping is performed correctly only in the case in which swap_bit is equal to 1 and therefore the swapping needs to be performed.

In the case in which swap_bit is equal to 0, meaning that the swapping between row(cur) and row(next) doesn't need to be performed, the polynomial constraint will always be equal to 0, therefore without checking that the values were carried from row(cur) to row(next) correctly without performing any swap.

The new polynomial constraint solves this issue

[sifnoc]

This reminds me of your comments in #8 (comment) when you refactored the merkle_sum_tree.

It appears that the constraint, without doubling, is also satisfied in this narrowed version of the chip.

therefore without checking that the values were carried from row(cur) to row(next) correctly without performing any swap.

I'm not entirely clear on the meaning of "performing" in this context. Does the code already perform a swap during the assignment phase here:

summa-solvency/zk_prover/src/chips/merkle_sum_tree.rs

Lines 203 to 209 in 7e39b84

	swap_bit.value().copied().map(|x| {
	(l1_val, r1_val) = if x == Fp::zero() {
	(l1_val, r1_val)
	} else {
	(r1_val, l1_val)
	};
	});

It seems to me that both the previous and fixed constraints work well without any bugs.

[enricobottazzi]

Yes, thanks @sifnoc for finding the comment! I didn't remember that we performed such a fix in the past.

The current constraint works fine (as expected) when swap_bit is set to 1

| a   | b   | swap_bit   |
| --- | --- | --- |
|  5  |  6  | 1   |
|  6  |  5  |  -  |  

s * swap_bit * ((elelment_l_next - elelment_l_cur) - (elelment_r_cur - elelment_r_next))
1 * 1 * ((6-5) - (6-5)) = 0 ✅

The problem of such constraint emerges when swap_bit is set to 0

In this case, the constraint will be satisfied (as expected) when no swap is to be performed.

| a   | b   | swap_bit   |
| --- | --- | --- |
|  5  |  6  | 0   |
|  5  |  6  |  -  |  

The constraint will be satisfied
1 * 0 * ((5-5) - (6-6)) = 0 ✅

But the same constraint will also be satisfied if the swap is performed (going against the rule of the swap_bit)

| a   | b   | swap_bit   |
| --- | --- | -    |
|  5  |  6  | 0    |
|  6  |  5  |  -   |  

The constraint will still be satisfied in this case, which shouldn't be the case!
1 * 0 * ((6-5) - (6-5)) = 0 ✅

So, to conclude, the bug in the current version is that it is not constraining the swap to not be performed when swap_bit is set to 0

[sifnoc]

a	b	swap_bit
5	6	0
6	5	-

The constraint will still be satisfied in this case, which shouldn't be the case! `1 * 0 * ((6-5) - (6-5)) = 0` white_check_mark

So, to conclude, the bug in the current version is that it is not constraining the swap to not be performed when `swap_bit` is set to 0

Thanks for the kind explanation! Oh, I see now, it's a bug!
The constraint can be satisfied even if the assigning logic is manipulated.

[alxkzmn]

Let's fix the "elelment" in the next PR

[alxkzmn]

There's a typo in "element"