Some contract optimizations

[dtebbs]

There are likely to be further optimizations that can be made in future passes, but this PR represents several improvement based on measurements of current contract cost areas.

[dtebbs]

• enable compiler optimizations. Saves approx 2.5% (pre-Istanbul)
• optimizations in proof verification (remove unnecessary memory usage, simplify data structures, assembly version of proof checker to avoid lots of memory copying and manipulation). Reoves approx 3% of proof validation cost, measured pre-Istanbul.
• optimizations in merkle tree (improve memory usage, avoid some redundant computation and cross-contract calls). Saves approx 7.5% (measured using Istanbul)
• hugely simplified restoring of digests, saves ~55,000 gas.

mix calls (with the default merkle depth of 4) are now down to about 1M gas.

[rrtoledo]

See issue #133 for a small optimization :)

[AntoineRondelet]

Just a quick comment on the types. It may be nice - in a second time - to refine the types used in the contracts. For example, here I initially used a uint for the depth which is overkilled. In practice the depth would be set to 64, which fits into a uint8. The corresponding number of leaves would be 2^64 and so the currentNodeIndex could become a uint64, and so on and so forth.
Doing so would enable to minimize the number of slots (256-bit words) used for storage; by "packing" multiple variables into the same word and save gas here and there.

[dtebbs]

Ah yes, there are definitely some things that can be packed.
I would hope that constants don't actually get put into storage, but I'd need to check that.

Definitely places where we can pack things, though.

[AntoineRondelet]

Why use such require here if you modified the depth to be a constant?

[dtebbs]

Ideally, the depth constant would come from a single configuration location (e.g. zeth.h), but since it is hard coded here I kept this as a way to check (at deployment time) that the configuration in the contract matches the client / prover server.

[AntoineRondelet]

since it is hard coded here I kept this as a way to check (at deployment time) that the configuration in the contract matches the client / prover server.

To make sure that we worked with the same merkle tree depth as the one set on the client (which needed to be set to the same value as the one used by the prover_server - ie. the one in zeth.h) we previously just set the value of the storage variable depth to the value passed to the constructor, it was not a constant (https://github.com/clearmatics/zeth/blob/pyclient-cli/zeth-contracts/contracts/BaseMerkleTree.sol#L23). Since setting the depth to be a constant (which value equals the depth set on the client and prover) would avoid some variable assignment, and remove an argument in the constructor, we could definitely do it (ie. switch depth to a constant). However for now, switching this var to being a constant and adding this extra check in the constructor seems quite odd to me.
What is the status of this PR? Is it WIP?

[dtebbs]

Since setting the depth to be a constant (which value equals the depth set on the client and prover) would avoid some variable assignment, and remove an argument in the constructor, we could definitely do it (ie. switch depth to a constant). However for now, switching this var to being a constant and adding this extra check in the constructor seems quite odd to me.

The purpose of having the depth as a constant is so that the data structure sizes are known at compile time. This makes the storage structures and the memory structures much simpler and cheaper to access, and saves a lot of gas in the average mix call (by my measurement about 20,000 just for depth of 4).

Avoiding the assignment / argument is not really a goal in and of itself - it is unlikely to save a significant amount of gas in comparison.

So I think it's worth having the depth as a constant, but what I'm concerned about is that it's currently very easy for the contract, client and prover_server configs to get out of sync. Similarly if we want to change a setting it's not always obvious which parts need changing. Ideally, we'd get some error at compile time if there is a difference but that's not currently easy. In the worst case, we would not get an error until someone tries to execute a mix. Here, I've kept the depth parameter to the constructor as it has almost no cost and gives an error at deployment time if client and contract have different settings. Not as good as an error at compile time, or the previous case of it just being fully adaptive, but it seems like a fair trade-off given the cost savings.

Similar to the number of joinsplit inputs / outputs, eventually it would be nice if the constants could come from a sinlge configuration file shared with the client and prover #143.

What is the status of this PR? Is it WIP?

I removed the WIP from this PR. It doesn't represent every possible optimization, but it represents what I think is a decent first-pass, based on measuring where the highest costs are. It makes sense to re-measure some of these changes, since Istanbul was introduced part-way through, but each change introduced here was measured to make a meaningful difference to cost.

[AntoineRondelet]

The purpose of having the depth as a constant is so that the data structure sizes are known at compile time. This makes the storage structures and the memory structures much simpler and cheaper to access, and saves a lot of gas in the average mix call

Yes I agree with that, we should switch to const when possible to save gas.
What I didn't get, was the require on the depth in the constructor. That's weird to give the user the possibility to set input values to a function that crashes for all but one value baked in the contract.

You said:

Here, I've kept the depth parameter to the constructor as it has almost no cost and gives an error at deployment time if client and contract have different settings. Not as good as an error at compile time, or the previous case of it just being fully adaptive, but it seems like a fair trade-off given the cost savings.

So it looks like you've done this on purpose to get an error if the config off-chain and the config on the contract diverge. I'm not entirely seduced by the idea though. This looks like a hack/tweak to keep around while we dev, but we need to find a better way to handle this, maybe with #143 as you suggested. Ultimately, nothing will prevent the client and prover_server configs to get out of sync. You can deploy a mixer and decide to change the constants on the prover_server which would "run out of sync" with the config of the contract you are interacting with.

[dtebbs]

Ultimately, nothing will prevent the client and prover_server configs to get out of sync. You can deploy a mixer and decide to change the constants on the prover_server which would "run out of sync" with the config of the contract you are interacting with.

Sure. I'll remove the param if you still think that's better, but just to explain, catching the case where a client changes their config after the contract has been deployed is not a goal of this. The point is to be able to catch a mismatch in config somewhere (ideally during deployment) with more than just an opaque message like "invalid poof". I thought of it more as a sanity check, given that different deployments may have different configs. But I guess you could think of it as being for development.

we need to find a better way to handle this, maybe with #143 as you suggested

Yes, it would be nice to have a single config somewhere. I've been looking into this further and there seems to be no nice / easy solution. So we should probably discuss the options for #143. In the meantime it would be nice to continue with the rest of this PR, so just let me know if you still prefer to remove depth here.

[AntoineRondelet]

You can also change the array of primary_inputs to become fixed size (since we know the number of primary inputs). It'll be cheaper

[dtebbs]

This is a bit more invasive than it first appears. primary_inputs is passed to several functions, including to the verifier. The verifier cannot use constants defined on other contracts, so the only way to make this work would be to re-specify the number of inputs, outputs and the calculation of the number of public inputs. i.e. we'd re-specify this info in 3 places, which is a bit of a maintenance nightmare when anything changes.

I'd been considering turning the verifier into a library anyway, to avoid the cost of the call. A library also connot share constants from some other contract, so in the end the changes I've made to do this are:

• verifier methods and types moved into the mixer
• mixer now holds the verification key
• inputs array size set statically to the pre-existing constant BaseMixer.nbInputs

With this change there are now no other contracts to be deployed for a Zeth instance, which also simplifies the client code.

[dtebbs]

I've reordered the changes and measured the gas costs after several of these commits, so we can better assess the tradeoff of readability and maintainability vs gas saving in each case.

Here, Cost1 and Cost2 are the costs of the 1st and 2nd mix calls (Alice deposits, and Alice sends to Bob) made during the test_zeth_client scripts, all with merkle depth 4.

  Cost1   Cost2      After Commit:
 ------- -------   ------------------
 2024506 2009812 : 1e4881ab pyClient: print gas costs of mix transactions
 1219198 1203904 : 6b5db525 contracts: use istanbul version of ganache-cli
 1182816 1167349 : a79ac19d pyclient: compile contracts with optimizations
                 : eeb65eee contracts: remove unnecessary data passed to/from precompiled contracts
                 : f6633c18 contracts: negate Proof.A instead of 3 RHS elements
                 : cd7ada47 contracts: simplify some structs, remove pointers and save some gas
                 : 17b1ce30 contracts: avoid reading whole verification key into memory. optimize linear comb loop.
 1156376 1141357 : b38a4c2b contracts: assembly version of pairing check
                 : a234d67e contracts: small style fixes
 1127255 1112044 : 313b369b contracts: more efficient computation of merkle root without keeping full tree
 1122692 1107161 : 3fc07cf7 contracts: better mem usage in mimc7 hash and cache seed
 1082905 1067374 : 7e441fd4 contracts: MiMC7 as a library rather than separate contract
 1064083 1048744 : 7fe2109d contracts: statically sized merkle tree data structures
                 : bb6148a7 contracts: some indentation consistency
                 : b95b40b8 contracts: consistent formatting
                 : bcb6faac contracts: solium and check command
                 : 3ad39616 travis: run solium check
 1056932 1041657 : 482be0bb contracts: optimize vk parameter and signature verification
 1055361 1040214 : 9dea7fc6 contracts: improve memory use in digest unpacking
 1002435  987480 : 8bd2621b contracts: simplify unpacking code for large gas saving
 1001117  986098 : abd12b81 pyclient: flatten G2 element in groth16 proofs to save a bit of gas
  993200  977989 : 048f45db contracts: merge verifier and mixer, and use const-sized array for inputs

Approximate gas saved (i.e. the value of each group of changes):

 805908 : 6b5db525 contracts: use istanbul version of ganache-cli
  36555 : a79ac19d pyclient: compile contracts with optimizations
        : eeb65eee contracts: remove unnecessary data passed to/from precompiled contracts
        : f6633c18 contracts: negate Proof.A instead of 3 RHS elements
        : cd7ada47 contracts: simplify some structs, remove pointers and save some gas
        : 17b1ce30 contracts: avoid reading whole verification key into memory. optimize linear comb loop.
  25992 : b38a4c2b contracts: assembly version of pairing check
        : a234d67e contracts: small style fixes
  29313 : 313b369b contracts: more efficient computation of merkle root without keeping full tree
   4883 : 3fc07cf7 contracts: better mem usage in mimc7 hash and cache seed
  39787 : 7e441fd4 contracts: MiMC7 as a library rather than separate contract
  18630 : 7fe2109d contracts: statically sized merkle tree data structures
        : bb6148a7 contracts: some indentation consistency
        : b95b40b8 contracts: consistent formatting
        : bcb6faac contracts: solium and check command
        : 3ad39616 travis: run solium check
   7087 : 482be0bb contracts: optimize vk parameter and signature verification
   1443 : 9dea7fc6 contracts: improve memory use in digest unpacking
  52734 : 8bd2621b contracts: simplify unpacking code for large gas saving
   1382 : abd12b81 pyclient: flatten G2 element in groth16 proofs to save a bit of gas
   8109 : 048f45db contracts: merge verifier and mixer, and use const-sized array for inputs

[rrtoledo]

Just looking through some of the changes

[dtebbs]

Addressed comments and rebased onto latest develop.

[AntoineRondelet]

Thanks @dtebbs LGTM

[AntoineRondelet]

Related issue: #94