fix multiset checks: running product column should explicitly shift multiplicand

[iammadab]

Describe your changes

WIP for #1185

Checklist before requesting a review

• Repo forked and branch created from next according to naming convention.
• Commit messages and codestyle follow conventions.
• Relevant issues are linked in the PR description.
• Tests added for new functionality.
• Documentation/comments updated according to changes.

[iammadab]

@plafer noticed there are two ways to shift the multiplicands:

• we could decide to not shift the multiplicative identity i.e E::ONE
• or we shift all values including the multiplicative identity

my current approach is to shift all values, which means that in the tests I'd need to sometimes explicitly multiply the expected by (ONE + alpha) or (ONE + alpha).inv() (this is fine and works out well)

wondering if you have any considerations regarding what approach might be better e.g. more correct, more efficient e.t.c.

cc: @bobbinth

[plafer]

I'm not sure I follow, but ultimately the tests should not change as a result of this change. In the case of this test specifically, the value in b_chip[2] (line 72 pre-change) is going to be the same before and after the change.

All that we're changing is where in the code we do alphas[0] + .... That would be here and here for the requests, and here for the responses. And correspondingly, we'd remove the alphas[0] + from all the implementors (e.g. here and here). This is the general idea.

Let me know if that clears things up

[bobbinth]

ultimately the tests should not change as a result of this change.

Yes, that's how I think about this as well. We are just updating representation for ease of readability - the test should not change as a part of this PR (maybe it would make sense to update them, but that should happen in a different PR).

[iammadab]

I realised that it’s not quite straightforward to move alpha[0] + from the implementors and have the tests still pass without change.

Model
Going to define the multiset region as the region that takes some compressed value v, adds alpha[0] to it and then modifies the aux column with this combined value.
v —multiset region—> v + alpha[0]

Before this PR, alpha was implicitly added to v before entering the multiset region and the value was taken as is, inside the multiset region.
v + alpha[0] —multiset region—> v + alpha[0] (no modification)

Most of the implementors don't follow the simple path of v + alpha[0]

• (alphas[0] + rest1) * (alphas[0] + rest2)

  • example here
  • Before PR
    • some implementors created v by multiplying (alphas[0] + rest1) * (alphas[0] + rest2)
    • to get alphas[0]^2 + alphas[0]rest1 + alphas[0]rest2 + rest1rest2
    • this is sent to the multiset region without modification
  • After PR
    • just removing alphas[0] from both factors won’t work
    • we'd get rest1 * rest2 = rest1rest2
    • rest1rest2 —> rest1rest2 + alphas[0]
  • We have a mismatch
    • alphas[0]^2 + alphas[0]rest1 + alphas[0]rest2 + rest1rest2 ≠ rest1rest2 + alphas[0]

• alphas.zip(elem)

  • example here
  • in this case alphas[0] is multiplied by elem[0]
  • no straightforward way to cleanly remove alphas[0] from the inner product

• E::ONE

  • example here
  • Before PR
    • noticed that when implementors send E::ONE they don’t shift it by alphas[0]
    • the multiset region gets
      • v + alphas[0] —> v + alphas[0] (no modification)
      • E::ONE —> E::ONE (no modification)
    • the above is what the tests expect
  • After PR
    • we remove the alphas[0] from v
    • the multiset region gets
      • v → v + alphas[0] (shift)
      • E::ONE → E::ONE + alphas[0] (shift)
  • We have a mismatch
    • E::ONE ≠ E::ONE + alphas[0]
  • we can fix this by not shifting E::ONE by alpha in the multiset region

  Full list of the cases can be found here

@plafer @bobbinth

[plafer]

Thank you for this comprehensive list! I had assumed when creating the issue that we were consistent always shifting by alphas[0]; turns out we weren't (which is the kind of issue that comes to light with such refactorings).

Given this new information, I would say: whenever we don't shift by alphas[0], then modify the test to expect a shift by alphas[0]. Otherwise, leave untouched.

A few notable cases:

• remove alphas[0] from factor_1 and factor_2 in build_mstream_request
  • Similarly, for other cases where we multiply factors together where each was shifted by alphas[0], we can remove the alphas[0] from each factor - e.g. build_mrupdate_request.
• For get_block_stack_table_removal_multiplicand here, element[0] == 1, such that alphas[0] * element[0] == alphas[0], so the test should remain unchanged. That inner product would then now loop over 11 elements instead of 12 (by removing alphas[0] and element[0] from it)
• The E::ONE cases (such as here): those were theoretically incorrect! Those rows don't get shifted by a random number, but they should. I suggest we modify all requests/responses that return E::ONE to return E::ZERO instead (it's nicer from the theoretical standpoint that we are taking a linear combination of an empty vector, i.e. 0, as discussed in the issue). The tests would need to change for those ones as well.

I didn't look into each and every case but I think these guidelines should address everything - let me know if it doesn't.

@bobbinth would like your thoughts on this.

[iammadab]

To make things easier to review, I will do this in 2 steps.

1. I will only change the code and leave the tests the same.

   • this naturally means I'd have to do more work on the code side to make the tests still pass
   • e.g. after removing alphas[0] from the factors to get rest1rest2, I'd have to explicitly add
   • alphas[0]^2 + alphas[0]rest1 + alphas[0]rest2 - alpha[0] to the resulting compressed value to make the test pass as is

2. In the second step I will just move the extra things added from the code to the test

This way, the first PR or the first commit should only contain code changes, and the second PR or commit should only contain code deletions and test additions.

This will enforce some constraints on the kind of changes I can make and make the review easier.

[plafer]

@iammadab I appreciate you trying to make the review easier, but really I think it'd be simpler to do it all in one PR. It'll actually make the review easier: we can check the code to make sure everything is as discussed, and then similarly the tests.

[plafer]

@iammadab I believe this got closed because we merged the al-simplify-aux branch. Can you reopen this targetting the next branch?